tls: allow multiple TLS certificates in the upstream when using a custom TLS certificate selector - #46479
tls: allow multiple TLS certificates in the upstream when using a custom TLS certificate selector#46479Berro321 wants to merge 12 commits into
Conversation
a custom tls cert selector is used for the upstream. Signed-off-by: Berro321 <matchburn321@gmail.com>
|
Hi @Berro321, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
GitHub Actions
Error
Signed-off-by: Berro321 <matchburn321@gmail.com>
GitHub Actions
Error
Signed-off-by: Berro321 <matchburn321@gmail.com>
GitHub Actions
Inactive
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
|
Assigning @botengyao as codeowner |
GitHub Actions
Inactive
|
/retest |
GitHub Actions
Error
Signed-off-by: Berro321 <matchburn321@gmail.com>
GitHub Actions
Error
|
/retest |
GitHub Actions
Error
botengyao
left a comment
botengyao
left a comment
There was a problem hiding this comment.
thanks for adding this support, module some high level comments.
/wait
| ASSERT(tls_contexts_.size() == 1); | ||
| return tls_contexts_[0]; | ||
| } | ||
| const Ssl::TlsContext& getTlsContext() const override { return tls_contexts_[0]; } |
There was a problem hiding this comment.
I do not think we should simply remove this assertion. Ssl::ClientContext still documents that client contexts have exactly one TLS context, while returning [0] now silently means “bootstrap/default context,” not necessarily the selected context. Since the selector already uses getTlsContexts(), please keep getTlsContext() restricted to single-context callers and update its interface documentation, or redesign the public accessor to represent multiple contexts explicitly.
There was a problem hiding this comment.
I have left it restricted to single context callers
| // invariant ever changes, getTlsContext() must be updated to handle multiple TLS contexts. | ||
| if (tls_contexts_.size() != 1) { | ||
| // If a custom TLS certificate selector is used, allow more than one TLS cert. | ||
| if (!config.tlsCertificateSelectorFactory() && tls_contexts_.size() != 1) { |
There was a problem hiding this comment.
This needs to account for session resumption before allowing multiple client identities. newSsl() installs a cached session before the certificate callback, and the cache is keyed only by SNI. A session created after selecting cert A can therefore be resumed by a connection whose selector would choose cert B. On an accepted TLS 1.2 resumption, the server does not send a new CertificateRequest, so selectTlsContext() is never invoked and the resumed connection retains A’s authenticated session identity. Since max_session_keys defaults to 1, this is enabled by default. Please either require max_session_keys: 0 for this configuration or introduce selector-aware session-cache partitioning that is determined before ClientHello.
There was a problem hiding this comment.
Added comment for setting max_session_keys to 0
GitHub Actions
Error
Signed-off-by: Berro321 <matchburn321@gmail.com>
GitHub Actions
Error
3 participants
Commit Message: Allow multiple TLS certificates in the upstream tls context when using a custom TLS certificate selector. The single cert check is kept if one is not defined.
Additional Description: We are working on an internal TLS certificate selector for UpstreamTLSContext that can select between multiple client certificate. By default only one is allowed, so we need a way to allow multiple once our custom TLS certificate selector is enabled.
Risk Level: Medium
Testing: Added unit tests, tested manually to check it works with our custom cert selector.
Docs Changes: Added documentation in the API proto for
tls_certificatesto indicate multiple certs are allowed in a client context when a custom TLS certificate selector is used.Release Notes: Added a release note under new features.
Platform Specific Features: N/A