xds: Only pass relevant TLS config for originating/terminating TLS #31903
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is to simply rename getCiliumTLSContext function to pave the way for subsequent changes. Signed-off-by: James Laverack <james@isovalent.com>
0008cc0 to
57ed685
Compare
57ed685 to
a9ea5ed
Compare
|
/test |
a9ea5ed to
88acbe6
Compare
sayboras
approved these changes
Apr 12, 2024
|
/test |
Previously, this test incorrectly tested tls key and cert behaviour on an originatingTLS config, but this should be used on a terminatingTLS context. Both contexts are converted the same way, so the test didn't fail before. Signed-off-by: James Laverack <james@isovalent.com>
88acbe6 to
cbe2bbc
Compare
|
/test |
Previously, the secrets were scanned for keys named 'ca.crt', 'tls.crt', and 'tls.key'. However 'ca.crt' is only relevant for terminatingTLS and 'tls.crt'/'tls.key' only for originatingTLS. By including all three keys in both the originating and terminating blocks, we can incorrectly pass extra configuration to Envoy. The result of this is that if a 'ca.crt' key is present in the secret passed to terminatingTLS, then Envoy will be mistakenly configured with a CA certificate and will then expect signed client certificates to be passed from clients (i.e., Pods). Along with a by-default fix, add --use-full-tls-context to the agent. When set, this will force the agent's XDS server into the old incorrect behaviour for Envoy TLS configuration. This is provided as an 'escape hatch' for users that might be depending on the existing incorrect behaviour. This flag may be removed in future releases. Fixes: cilium#31761 Signed-off-by: James Laverack <james@isovalent.com>
cbe2bbc to
e7115ac
Compare
|
/test |
|
@JamesLaverack With the new behavior it is impossible to configure a case where client certificates are needed. Do we think that we'll never need to support TLS interception case where the final server requires the use of client certificate? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Please ensure your pull request adheres to the following guidelines:
description and a
Fixes: #XXXline if the commit addresses a particularGitHub issue.
Fixes: <commit-id>tag, thenplease add the commit author[s] as reviewer[s] to this issue.
See the release note for a summary of the change, and #31761 for discussion of the bug this fixes. A flag is provided for backwards compatibility, set to false for future versions of Cilium. The intent is to set this to true in backports, so as not to change behavior unexpectedly in a patch release.
Fixes: #31761