-
Notifications
You must be signed in to change notification settings - Fork 611
gep: refine CACertificateRefs description for frontend TLS #4183
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Norwin Schnyder <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: snorwin The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
Signed-off-by: Norwin Schnyder <[email protected]>
| // | ||
| // +listType=atomic | ||
| // +kubebuilder:validation:MaxItems=8 | ||
| // +kubebuilder:validation:MinItems=1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kl52752 I’m wondering if cACertificateRefs should be optional when using the AllowInsecureFallback mode, or if that case would justify adding a separate mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well Insecure mode is design for validating the setup (fg. using mtl headers) not to bypass it (fg in testing env). Why would some wants to have mTLS enalbed without CA set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to summarize all the different cases and how we interpreted the specification and how we’ve implemented it so far.
| Mode | Client certificate presented | Response of the Gateway |
|---|---|---|
AllowInsecureFallback |
No certificate (TLS) | Accepted |
AllowInsecureFallback |
Certificate not signed by the trusted CA | Accepted |
AllowInsecureFallback |
Certificate signed by the trusted CA | Accepted |
AllowValidOnly |
No certificate (TLS) | Rejected (TLS handshake fails) |
AllowValidOnly |
Certificate not signed by the trusted CA | Rejected (TLS handshake fails) |
AllowValidOnly |
Certificate signed by the trusted CA | Accepted |
@kl52752 could you please check if this table matches your expectations?
If it looks good, I’ll probably include it (or a similar version) in the GEP.
According to the cases described in the table, in my opinion the Gateway’s behavior wouldn’t change depending on whether client CA certificates are configured when using AllowInsecureFallback.
This raises the question, why do we enforce configuring CA certificates for AllowInsecureFallback if it doesn’t directly affect the Gateway’s behavior?
However, it does have a noticeable impact on the client side, the client behaves quite differently when the Gateway presents the client certificate CA during negotiation, e.g., without additional workarounds, it’s not possible to connect with a browser or the Go HTTP client to a Gateway with AllowInsecureFallback using a self-signed client certificate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes this table summarized it all 👍
This raises the question, why do we enforce configuring CA certificates for AllowInsecureFallback if it doesn’t directly affect the Gateway’s behavior?
The purpose for AllowInsecureFallback is for testing purposes (or for a short period of time when credentials need to be rotated). This should be used either on testing environments or together with mTLS custom headers where the information about tls handshakes are passed. That's why I don't think that we should allow for empty CA cert list because this mode is not meant for bypassing the validation but move the "decision" for allowing the traffic to backend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to leave it as it is, but before resolving the discussion, I would appreciate some input from others as well.
What type of PR is this?
/kind gep
What this PR does / why we need it:
In the current version of GEP-91, the semantics of an invalid
CACertificateRefsare not fully specified. In addition, it is unclear whether theInsecureFrontendValidationModecondition should ever be removed or set to False.This PR clarifies the expected behavior by aligning the semantics of invalid references in frontend validation with the patterns already established in BackendTLSPolicy and the approach proposed in #4123 for backend TLS configuration on the Gateway.
Which issue(s) this PR fixes:
N/A
Does this PR introduce a user-facing change?: