Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gatewayapi: validate tls secret in listener certificateRef #868

Merged
merged 1 commit into from
Jan 6, 2023

Conversation

LanceEa
Copy link
Contributor

@LanceEa LanceEa commented Jan 6, 2023

Previously, the tls secret referenced in a listener certifcateRef would only validate that data existed within the tls.key and tls.cert fields. This adds additional validation to ensure the data in the tls.cert is a valid pem encoded cert and the tls.key contains a valid pem encoded private key. The GatewayInvalidTLSConfiguration conformance test can now be enabled and passes.

Fixes #783

@LanceEa LanceEa requested a review from a team as a code owner January 6, 2023 12:12
@LanceEa LanceEa force-pushed the laustin/bad-cert-ref-conformance branch 3 times, most recently from bcaf619 to 0855114 Compare January 6, 2023 12:25
@codecov-commenter
Copy link

Codecov Report

Merging #868 (daefedf) into main (085e6fa) will increase coverage by 0.09%.
The diff coverage is 70.00%.

@@            Coverage Diff             @@
##             main     #868      +/-   ##
==========================================
+ Coverage   63.89%   63.99%   +0.09%     
==========================================
  Files          50       51       +1     
  Lines        6523     6526       +3     
==========================================
+ Hits         4168     4176       +8     
+ Misses       2093     2090       -3     
+ Partials      262      260       -2     
Impacted Files Coverage Δ
internal/gatewayapi/validate.go 90.51% <11.11%> (-1.65%) ⬇️
internal/gatewayapi/tls.go 87.09% <87.09%> (ø)
internal/provider/kubernetes/helpers.go 73.77% <0.00%> (-1.64%) ⬇️
internal/provider/kubernetes/controller.go 47.39% <0.00%> (+0.42%) ⬆️
internal/gatewayapi/contexts.go 78.24% <0.00%> (+2.30%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@LanceEa
Copy link
Contributor Author

LanceEa commented Jan 6, 2023

Ok, sorry for all the pushed commits and fixups. I should finish my coffee before doing early morning PR's 😄.

I fixed up the lint issue reported by CI and the broken gatewayapi tests. With the new validation logic a few of the tests with TLS now require valid tls secrets for the inputs and expected outputs so I have adjusted those test so they match the new tls validation behavior.

@LanceEa
Copy link
Contributor Author

LanceEa commented Jan 6, 2023

Looks like my fixup commits didn't get the DCO. Let me know if you need me to squash those or I think the project squash merges so that should take care of them.

@danehans
Copy link
Contributor

danehans commented Jan 6, 2023

@LanceEa each commit needs to be signed for the DCO job to pass.

internal/gatewayapi/tls.go Outdated Show resolved Hide resolved
internal/gatewayapi/tls.go Outdated Show resolved Hide resolved
internal/gatewayapi/tls.go Outdated Show resolved Hide resolved
Previously, the tls secret referenced in a listener certifcateRef would
only validate that data existed within the `tls.key` and `tls.cert` fields.
This adds additional validation to ensure the data in the `tls.cert`
is a valid pem encoded cert and  the `tls.key` contains a valid pem
encoded private key.

The GatewayInvalidTLSConfiguration conformance test can now be enabled
and passes.

Signed-off-by: Lance Austin <laustin@datawire.io>
@LanceEa LanceEa force-pushed the laustin/bad-cert-ref-conformance branch from daefedf to e73ac3d Compare January 6, 2023 19:48
@LanceEa
Copy link
Contributor Author

LanceEa commented Jan 6, 2023

Sorry for the pain with the forced push, I went ahead and squashed the fixup commits to address the DCO and addressed feedback. I will be sure to get the DCO applied to fixup commits too, in the future.

Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for fixing this !

@arkodg arkodg merged commit a0be2d8 into envoyproxy:main Jan 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable GatewayInvalidTLSConfiguration conformance test
5 participants