-
Notifications
You must be signed in to change notification settings - Fork 451
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
Add GEP-3155: Complete Backend mTLS Configuration #3180
Add GEP-3155: Complete Backend mTLS Configuration #3180
Conversation
Welcome @mkosieradzki! |
Hi @mkosieradzki. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
/cc @shaneutt |
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.
A few pieces of feedback here, but I think that the number one thing that's needed is use cases that show how some of these proposals work, or justify their existence.
In particular, I'm reluctant to add free-text map[string]string
fields to the API without a very good explanation and example use cases.
Co-authored-by: Mike Morris <mikemorris@users.noreply.github.com>
/cc |
…a-px596j54gqh6v6g
…a-px596j54gqh6v6g
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.
Hello @mkosieradzki , if we specify client cert on Gateway then can you please share some insights as to how would the scenarios where there are services which do not need MTLS but need only End2End TLS coexist in the same Gateway -> Route configurations.
There could be a route targeting service (music) which needs MTLS and another route targeting service (shopping) which only needs End to End TLS.
These scenarios would not work on same Gateway if we use MTLS to all the services
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 think the distinction here is that we're providing a client cert at the Gateway level that should be presented if it's requested by the backend. @mkosieradzki had previously included a per-Service override for this config in BackendTLSPolicy. We ran into issues there where the personas attached to BackendTLSPolicy weren't particularly clear (see #3226 to chime in on that discussion), so this iteration of the GEP will leave per-Service overrides out, but I think that's still very much a longer term goal.
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.
There could be a route targeting service (music) which needs MTLS and another route targeting service (shopping) which only needs End to End TLS.
To my best understanding whether the Gateway will send the certificate to the Backend or not depends on the backend server configuration, i.e. whether it will send CertificateRequest
message during the handshake as per https://datatracker.ietf.org/doc/html/rfc5246#section-7.3 or not.
Therefore if backend is not configured to request a client certificate, the Gateway will not send it, even it is configured. My original rationale for adding per-service overrides was about corner cases:
- where server application expects a certificate issued by a different CA
- where server application can work in two modes: if the cert is available or not, and lack of certificate is perfectly fine for the server (and causes fallback to a different authentication method).
I think we should revisit this scenario as soon as we figure out the #3226.
geps/gep-3155/index.md
Outdated
#### Gateway-level (Core support) | ||
Specifying credentials at the gateway level is the default operation mode, where all | ||
backends will be presented with a single gateway 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.
Re all
backends - should the client cert be presented even when no BackendTLSPolicy targeting the Service backend is present?
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.
In my longer term vision, I don't think we should limit it to this scope. Ideally Gateway owners could provide a set of CAs they trust for certs from backends at the Gateway level and the validation in BackendTLSPolicy actually becomes more of a per-Service override.
Although I don't think it's strictly necessary, I think it could be acceptable to start with this restriction. Of course I certainly don't want to stick with that limitation long term.
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.
Ideally Gateway owners could provide a set of CAs they trust for certs from backends at the Gateway level
I think this makes sense and could potentially address the non-mutual end-to-end TLS functionality @jaishals is describing in #3180 (comment), where the presence of BackendTLSPolicy additional validation is specifically to enforce mutual TLS, restricting the set of acceptable client certificates? Would any additional signal like mode: mutual
be needed somewhere to clarify this?
geps/gep-3155/index.md
Outdated
|
||
### SANs on BackendTLSPolicy | ||
|
||
This change enables the certificate to have a different identity than the SNI |
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 think this is intended to be applicable to the "server"/destination cert, but can you please specify more precisely which cert is affected here?
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.
Let's use "Backend" here to line up with the terminology in https://gateway-api.sigs.k8s.io/geps/gep-2907/#frontend-and-backend.
geps/gep-3155/index.md
Outdated
// When specified, at least one of certificate's Subject Alternate Names MUST | ||
// match at least one of the specified SubjectAltNames. |
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.
What happens if it doesn't? Suggested status
condition on the policy indicated it's invalid?
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.
Apparently we haven't defined how a request should be rejected here:
gateway-api/apis/v1alpha3/backendtlspolicy_types.go
Lines 128 to 130 in c94d401
// 1. Hostname MUST be used as the SNI to connect to the backend (RFC 6066). | |
// 2. Hostname MUST be used for authentication and MUST match the certificate | |
// served by the matching backend. |
At a minimum I think we can agree that the request should be rejected, ideally we can be more specific here for the sake of writing conformance tests and overall portability.
As far as populating status, I think that most controllers are separated from the dataplane and would not be able to translate runtime TLS validation errors to config time (k8s) status.
backend. This adds that configuration to both Gateway and Service (via | ||
BackendTLSPolicy). | ||
|
||
#### Gateway-level (Core support) |
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.
This is missing an API example - seems to be suggesting the addition of a new field under the Gateway spec
stanza https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io%2fv1.Gateway?
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, I think this should have a quick sketch of what we need to add to Gateway.
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.
Thanks @mkosieradzki! A few more comments, but mostly LGTM.
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 think the distinction here is that we're providing a client cert at the Gateway level that should be presented if it's requested by the backend. @mkosieradzki had previously included a per-Service override for this config in BackendTLSPolicy. We ran into issues there where the personas attached to BackendTLSPolicy weren't particularly clear (see #3226 to chime in on that discussion), so this iteration of the GEP will leave per-Service overrides out, but I think that's still very much a longer term goal.
geps/gep-3155/index.md
Outdated
#### Gateway-level (Core support) | ||
Specifying credentials at the gateway level is the default operation mode, where all | ||
backends will be presented with a single gateway 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.
In my longer term vision, I don't think we should limit it to this scope. Ideally Gateway owners could provide a set of CAs they trust for certs from backends at the Gateway level and the validation in BackendTLSPolicy actually becomes more of a per-Service override.
Although I don't think it's strictly necessary, I think it could be acceptable to start with this restriction. Of course I certainly don't want to stick with that limitation long term.
geps/gep-3155/index.md
Outdated
|
||
### SANs on BackendTLSPolicy | ||
|
||
This change enables the certificate to have a different identity than the SNI |
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.
Let's use "Backend" here to line up with the terminology in https://gateway-api.sigs.k8s.io/geps/gep-2907/#frontend-and-backend.
geps/gep-3155/index.md
Outdated
// When specified, at least one of certificate's Subject Alternate Names MUST | ||
// match at least one of the specified SubjectAltNames. |
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.
Apparently we haven't defined how a request should be rejected here:
gateway-api/apis/v1alpha3/backendtlspolicy_types.go
Lines 128 to 130 in c94d401
// 1. Hostname MUST be used as the SNI to connect to the backend (RFC 6066). | |
// 2. Hostname MUST be used for authentication and MUST match the certificate | |
// served by the matching backend. |
At a minimum I think we can agree that the request should be rejected, ideally we can be more specific here for the sake of writing conformance tests and overall portability.
As far as populating status, I think that most controllers are separated from the dataplane and would not be able to translate runtime TLS validation errors to config time (k8s) status.
/ok-to-test |
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've been picky about wording, but once the two wording changes I've asked for are handled, and we have an example of what we are adding to Gateway, then this LGTM.
…a-px596j54gqh6v6g
- Added clarifications
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.
Thanks @mkosieradzki! Added a few formatting nits but otherwise LGTM.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mkosieradzki, robscott, youngnick The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
With that last round of changes, this LGTM. I'll leave the final unhold for @robscott though, since he has some outstanding format changes. /lgtm |
Co-authored-by: Rob Scott <rob.scott87@gmail.com>
Co-authored-by: Rob Scott <rob.scott87@gmail.com>
Co-authored-by: Rob Scott <rob.scott87@gmail.com>
Co-authored-by: Rob Scott <rob.scott87@gmail.com>
Thanks @mkosieradzki! /lgtm |
* Initial commit * Pending changes exported from your codespace * Update geps/gep-3155/index.md Co-authored-by: Mike Morris <mikemorris@users.noreply.github.com> * Initial commit * Initial commit * Initial commit * Initial commit * Fix enum values * Initial commit * Pending changes exported from your codespace * Fix indentation * Fix indentation * Update geps/gep-3155/index.md Co-authored-by: Nick Young <inocuo@gmail.com> * Update geps/gep-3155/index.md Co-authored-by: Nick Young <inocuo@gmail.com> * Removed per-service override from the proposal * - Reverted back changes necessary for the GatewaySpec - Added clarifications * Update geps/gep-3155/index.md Co-authored-by: Rob Scott <rob.scott87@gmail.com> * Update geps/gep-3155/index.md Co-authored-by: Rob Scott <rob.scott87@gmail.com> * Update geps/gep-3155/index.md Co-authored-by: Rob Scott <rob.scott87@gmail.com> * Update geps/gep-3155/index.md Co-authored-by: Rob Scott <rob.scott87@gmail.com> --------- Co-authored-by: Mike Morris <mikemorris@users.noreply.github.com> Co-authored-by: Nick Young <inocuo@gmail.com> Co-authored-by: Rob Scott <rob.scott87@gmail.com>
* Initial commit * Pending changes exported from your codespace * Update geps/gep-3155/index.md Co-authored-by: Mike Morris <mikemorris@users.noreply.github.com> * Initial commit * Initial commit * Initial commit * Initial commit * Fix enum values * Initial commit * Pending changes exported from your codespace * Fix indentation * Fix indentation * Update geps/gep-3155/index.md Co-authored-by: Nick Young <inocuo@gmail.com> * Update geps/gep-3155/index.md Co-authored-by: Nick Young <inocuo@gmail.com> * Removed per-service override from the proposal * - Reverted back changes necessary for the GatewaySpec - Added clarifications * Update geps/gep-3155/index.md Co-authored-by: Rob Scott <rob.scott87@gmail.com> * Update geps/gep-3155/index.md Co-authored-by: Rob Scott <rob.scott87@gmail.com> * Update geps/gep-3155/index.md Co-authored-by: Rob Scott <rob.scott87@gmail.com> * Update geps/gep-3155/index.md Co-authored-by: Rob Scott <rob.scott87@gmail.com> --------- Co-authored-by: Mike Morris <mikemorris@users.noreply.github.com> Co-authored-by: Nick Young <inocuo@gmail.com> Co-authored-by: Rob Scott <rob.scott87@gmail.com>
What type of PR is this?
/kind gep
What this PR does / why we need it:
Proposal for backend mTLS configuration
Does this PR introduce a user-facing change?: