tls: add enable_client_cipher_preference option #35106
Conversation
Signed-off-by: Arul Thileeban Sagayam <[email protected]>
Signed-off-by: Arul Thileeban Sagayam <[email protected]>
Signed-off-by: Arul Thileeban Sagayam <[email protected]>
Signed-off-by: Arul Thileeban Sagayam <[email protected]>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
source/common/tls/context_impl.cc
Outdated
| @@ -309,7 +309,9 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c | |||
|
|
|||
| // use the server's cipher list preferences | |||
| for (auto& ctx : tls_contexts_) { | |||
| SSL_CTX_set_options(ctx.ssl_ctx_.get(), SSL_OP_CIPHER_SERVER_PREFERENCE); | |||
| if (!config.enableClientCipherPreference()) { | |||
There was a problem hiding this comment.
Does this require a runtime guard?
There was a problem hiding this comment.
No, configuration guard is fine because it is off-by-default.
| // By default, Envoy as a server uses its preferred cipher during the handshake. | ||
| // Setting this to true would allow the downstream client's preferred cipher to be used instead. | ||
| // Has no effect when using TLSv1_3. | ||
| bool enable_client_cipher_preference = 6; |
There was a problem hiding this comment.
Maybe prefer_client_ciphers?
test/common/tls/context_impl_test.cc
Outdated
| envoy::extensions::transport_sockets::tls::v3::DownstreamTlsContext tls_context; | ||
| TestUtility::loadFromYaml(TestEnvironment::substitute(yaml), tls_context); | ||
| auto cfg = *ServerContextConfigImpl::create(tls_context, factory_context_); | ||
| EXPECT_EQ(cfg.get()->enableClientCipherPreference(), false); |
There was a problem hiding this comment.
You should test that the SSL_CTX has the correct flags set on it instead of just looking at the cfg.
There was a problem hiding this comment.
Yeah, I wanted to test it, but the SSL_CTX object didn't seem directly accessible or downgradable from these objects. Let me check further
There was a problem hiding this comment.
Had to move a helper function to util file, but this was doable. Addressed other comments as well.
| @@ -275,6 +275,40 @@ TEST_P(SslIntegrationTest, RouterDownstreamDisconnectBeforeResponseComplete) { | |||
| checkStats(); | |||
| } | |||
|
|
|||
| // Test server preference of cipher suites. Default server order is ECDHE-RSA-AES128-GCM-SHA256 | |||
There was a problem hiding this comment.
In these two tests, it will be clearer what is happening, and less likely to break in the future, if you explicitly set the server ciphers as well, instead of relying on the default.
Signed-off-by: Arul Thileeban Sagayam <[email protected]>
Signed-off-by: Arul Thileeban Sagayam <[email protected]>
|
/wait I think this needs to be updated to resolve the merge conflict @arulthileeban |
Signed-off-by: Arul Thileeban Sagayam <[email protected]>
Updated it. Check seems to fail due to lack of agents? |
| // By default, Envoy as a server uses its preferred cipher during the handshake. | ||
| // Setting this to true would allow the downstream client's preferred cipher to be used instead. | ||
| // Has no effect when using TLSv1_3. | ||
| bool prefer_client_ciphers = 6; |
There was a problem hiding this comment.
Should this field be in DownstreamTlsContext instead of here? If I'm understanding the intent here correctly, this seems to apply to the downstream connection only, not to upstream connections. But the TlsParameters message is used in CommonTlsContext, which is used in both UpstreamTlsContext and DownstreamTlsContext.
There was a problem hiding this comment.
That's a good catch! Yes, agree this should be in the downstream context
There was a problem hiding this comment.
Oops, Thanks for pointing that out. I've moved it to downstream context now.
Signed-off-by: Arul Thileeban Sagayam <[email protected]>
Signed-off-by: Arul Thileeban Sagayam <[email protected]>
Signed-off-by: Arul Thileeban Sagayam <[email protected]>
Signed-off-by: Arul Thileeban Sagayam <[email protected]>
|
Unassigning @markdroth because he's out this week. /assign-from @envoyproxy/api-shepherds |
|
@envoyproxy/api-shepherds assignee is @adisuissa |
|
@arulthileeban sorry there's a merge conflict again. Should be able to merge after that's resolved. /wait |
Signed-off-by: Arul Thileeban Sagayam <[email protected]>
Fixed. Thanks! |
Fixes envoyproxy#34604 Signed-off-by: Arul Thileeban Sagayam <[email protected]> Signed-off-by: Martin Duke <[email protected]>
Fixes envoyproxy#34604 Signed-off-by: Arul Thileeban Sagayam <[email protected]> Signed-off-by: asingh-g <[email protected]>
Commit Message: Add option to allow enabling client cipher preference for TLS handshakes instead of the server's (default).
Risk Level: Low
Testing: Unit/Integration/Manual
Docs Changes: Added
Release Notes: Added
Fixes #34604