From 226942b4858699c38605567f8d3b70927a19cc96 Mon Sep 17 00:00:00 2001 From: Arul Thileeban Sagayam Date: Tue, 23 Jul 2024 16:51:18 -0400 Subject: [PATCH] tls: add enable_client_cipher_preference option (#35106) Fixes #34604 Signed-off-by: Arul Thileeban Sagayam --- .../transport_sockets/tls/v3/tls.proto | 7 ++- changelogs/current.yaml | 5 ++ envoy/ssl/context_config.h | 5 ++ source/common/tls/context_impl.cc | 5 -- .../common/tls/server_context_config_impl.cc | 3 +- .../common/tls/server_context_config_impl.h | 2 + source/common/tls/server_context_impl.cc | 5 ++ test/common/tls/BUILD | 2 + test/common/tls/context_impl_test.cc | 51 +++++++++++++++++++ test/common/tls/handshaker_factory_test.cc | 9 +--- .../tls/integration/ssl_integration_test.cc | 38 ++++++++++++++ .../integration/ssl_integration_test_base.cc | 2 + .../integration/ssl_integration_test_base.h | 2 + test/common/tls/ssl_test_utility.h | 10 ++++ test/config/utility.cc | 5 ++ test/config/utility.h | 12 +++++ test/mocks/ssl/mocks.h | 1 + 17 files changed, 149 insertions(+), 15 deletions(-) diff --git a/api/envoy/extensions/transport_sockets/tls/v3/tls.proto b/api/envoy/extensions/transport_sockets/tls/v3/tls.proto index 9d465c973210..c22d580e6f5d 100644 --- a/api/envoy/extensions/transport_sockets/tls/v3/tls.proto +++ b/api/envoy/extensions/transport_sockets/tls/v3/tls.proto @@ -63,7 +63,7 @@ message UpstreamTlsContext { google.protobuf.BoolValue enforce_rsa_key_usage = 5; } -// [#next-free-field: 11] +// [#next-free-field: 12] message DownstreamTlsContext { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.auth.DownstreamTlsContext"; @@ -140,6 +140,11 @@ message DownstreamTlsContext { // If the client provides SNI but no such cert matched, it will decide to full scan certificates or not based on this config. // Defaults to false. See more details in :ref:`Multiple TLS certificates `. google.protobuf.BoolValue full_scan_certs_on_sni_mismatch = 9; + + // 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 = 11; } // TLS key log configuration. diff --git a/changelogs/current.yaml b/changelogs/current.yaml index e625fe665676..7147c211c781 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -33,6 +33,11 @@ removed_config_or_runtime: Removed ``envoy.reloadable_features.dns_cache_set_first_resolve_complete`` runtime flag and legacy code paths. new_features: +- area: tls + change: | + Added :ref:`prefer_client_ciphers + ` + to support enabling client cipher preference instead of server's for TLS handshakes. - area: ext_authz change: | Added config field diff --git a/envoy/ssl/context_config.h b/envoy/ssl/context_config.h index 87c26f0250d0..a247c71533ad 100644 --- a/envoy/ssl/context_config.h +++ b/envoy/ssl/context_config.h @@ -199,6 +199,11 @@ class ServerContextConfig : public virtual ContextConfig { * downstream TLS handshake, false otherwise. */ virtual bool fullScanCertsOnSNIMismatch() const PURE; + + /** + * @return true if the client cipher preference is enabled, false otherwise. + */ + virtual bool preferClientCiphers() const PURE; }; using ServerContextConfigPtr = std::unique_ptr; diff --git a/source/common/tls/context_impl.cc b/source/common/tls/context_impl.cc index 29bdc17c0b6a..8e997df07399 100644 --- a/source/common/tls/context_impl.cc +++ b/source/common/tls/context_impl.cc @@ -308,11 +308,6 @@ 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); - } - parsed_alpn_protocols_ = parseAlpnProtocols(config.alpnProtocols(), creation_status); SET_AND_RETURN_IF_NOT_OK(creation_status, creation_status); diff --git a/source/common/tls/server_context_config_impl.cc b/source/common/tls/server_context_config_impl.cc index bfd611dbdb60..e40ebd9d15ce 100644 --- a/source/common/tls/server_context_config_impl.cc +++ b/source/common/tls/server_context_config_impl.cc @@ -123,7 +123,8 @@ ServerContextConfigImpl::ServerContextConfigImpl( disable_stateless_session_resumption_(getStatelessSessionResumptionDisabled(config)), disable_stateful_session_resumption_(config.disable_stateful_session_resumption()), full_scan_certs_on_sni_mismatch_( - PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, full_scan_certs_on_sni_mismatch, false)) { + PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, full_scan_certs_on_sni_mismatch, false)), + prefer_client_ciphers_(config.prefer_client_ciphers()) { SET_AND_RETURN_IF_NOT_OK(creation_status, creation_status); if (session_ticket_keys_provider_ != nullptr) { // Validate tls session ticket keys early to reject bad sds updates. diff --git a/source/common/tls/server_context_config_impl.h b/source/common/tls/server_context_config_impl.h index 5cacbbb0d438..b11ddba70802 100644 --- a/source/common/tls/server_context_config_impl.h +++ b/source/common/tls/server_context_config_impl.h @@ -40,6 +40,7 @@ class ServerContextConfigImpl : public ContextConfigImpl, public Envoy::Ssl::Ser } bool fullScanCertsOnSNIMismatch() const override { return full_scan_certs_on_sni_mismatch_; } + bool preferClientCiphers() const override { return prefer_client_ciphers_; } private: ServerContextConfigImpl( @@ -71,6 +72,7 @@ class ServerContextConfigImpl : public ContextConfigImpl, public Envoy::Ssl::Ser const bool disable_stateless_session_resumption_; const bool disable_stateful_session_resumption_; bool full_scan_certs_on_sni_mismatch_; + const bool prefer_client_ciphers_; }; } // namespace Tls diff --git a/source/common/tls/server_context_impl.cc b/source/common/tls/server_context_impl.cc index 00b6d7051057..b7ac27c718a6 100644 --- a/source/common/tls/server_context_impl.cc +++ b/source/common/tls/server_context_impl.cc @@ -159,6 +159,11 @@ ServerContextImpl::ServerContextImpl(Stats::Scope& scope, this); } + if (!config.preferClientCiphers()) { + // Use server cipher preference based on config. + SSL_CTX_set_options(ctx.ssl_ctx_.get(), SSL_OP_CIPHER_SERVER_PREFERENCE); + } + // If the handshaker handles session tickets natively, don't call // `SSL_CTX_set_tlsext_ticket_key_cb`. if (config.disableStatelessSessionResumption()) { diff --git a/test/common/tls/BUILD b/test/common/tls/BUILD index 464ad5b8c79f..af168e029f10 100644 --- a/test/common/tls/BUILD +++ b/test/common/tls/BUILD @@ -145,6 +145,7 @@ envoy_cc_test_library( "ssl_test_utility.h", ], deps = [ + "//source/common/tls:ssl_socket_lib", "//source/common/tls:utility_lib", "//test/test_common:environment_lib", ], @@ -197,6 +198,7 @@ envoy_cc_test( name = "handshaker_factory_test", srcs = ["handshaker_factory_test.cc"], deps = [ + ":ssl_test_utils", "//source/common/stream_info:stream_info_lib", "//source/common/tls:context_lib", "//source/common/tls:server_context_config_lib", diff --git a/test/common/tls/context_impl_test.cc b/test/common/tls/context_impl_test.cc index 04c7bfd82aca..0bbd94dbd4ae 100644 --- a/test/common/tls/context_impl_test.cc +++ b/test/common/tls/context_impl_test.cc @@ -13,6 +13,7 @@ #include "source/common/tls/context_config_impl.h" #include "source/common/tls/context_impl.h" #include "source/common/tls/server_context_config_impl.h" +#include "source/common/tls/server_ssl_socket.h" #include "source/common/tls/utility.h" #include "test/common/tls/ssl_certs_test.h" @@ -141,6 +142,56 @@ TEST_F(SslContextImplTest, TestCipherSuites) { "ciphers were rejected when tried individually: BOGUS1-SHA256, BOGUS2-SHA"); } +// Envoy's default cipher preference is server's. +TEST_F(SslContextImplTest, TestServerCipherPreference) { + const std::string yaml = R"EOF( + common_tls_context: + tls_certificates: + certificate_chain: + filename: "{{ test_rundir }}/test/common/tls/test_data/unittest_cert.pem" + private_key: + filename: "{{ test_rundir }}/test/common/tls/test_data/unittest_key.pem" + )EOF"; + + envoy::extensions::transport_sockets::tls::v3::DownstreamTlsContext tls_context; + TestUtility::loadFromYaml(TestEnvironment::substitute(yaml), tls_context); + auto cfg = ServerContextConfigImpl::create(tls_context, factory_context_).value(); + ASSERT_FALSE(cfg.get()->preferClientCiphers()); + + auto socket_factory = *Extensions::TransportSockets::Tls::ServerSslSocketFactory::create( + std::move(cfg), manager_, *store_.rootScope(), {}); + std::unique_ptr socket = + socket_factory->createDownstreamTransportSocket(); + SSL_CTX* ssl_ctx = extractSslCtx(socket.get()); + + EXPECT_TRUE(SSL_CTX_get_options(ssl_ctx) & SSL_OP_CIPHER_SERVER_PREFERENCE); +} + +TEST_F(SslContextImplTest, TestPreferClientCiphers) { + const std::string yaml = R"EOF( + common_tls_context: + tls_certificates: + certificate_chain: + filename: "{{ test_rundir }}/test/common/tls/test_data/unittest_cert.pem" + private_key: + filename: "{{ test_rundir }}/test/common/tls/test_data/unittest_key.pem" + prefer_client_ciphers: true + )EOF"; + + envoy::extensions::transport_sockets::tls::v3::DownstreamTlsContext tls_context; + TestUtility::loadFromYaml(TestEnvironment::substitute(yaml), tls_context); + auto cfg = ServerContextConfigImpl::create(tls_context, factory_context_).value(); + ASSERT_TRUE(cfg.get()->preferClientCiphers()); + + auto socket_factory = *Extensions::TransportSockets::Tls::ServerSslSocketFactory::create( + std::move(cfg), manager_, *store_.rootScope(), {}); + std::unique_ptr socket = + socket_factory->createDownstreamTransportSocket(); + SSL_CTX* ssl_ctx = extractSslCtx(socket.get()); + + EXPECT_FALSE(SSL_CTX_get_options(ssl_ctx) & SSL_OP_CIPHER_SERVER_PREFERENCE); +} + TEST_F(SslContextImplTest, TestExpiringCert) { const std::string yaml = R"EOF( common_tls_context: diff --git a/test/common/tls/handshaker_factory_test.cc b/test/common/tls/handshaker_factory_test.cc index aac0aef2b407..a22f85832321 100644 --- a/test/common/tls/handshaker_factory_test.cc +++ b/test/common/tls/handshaker_factory_test.cc @@ -11,6 +11,7 @@ #include "source/common/tls/ssl_handshaker.h" #include "source/server/process_context_impl.h" +#include "test/common/tls/ssl_test_utility.h" #include "test/mocks/network/connection.h" #include "test/mocks/server/transport_socket_factory_context.h" #include "test/test_common/registry.h" @@ -105,14 +106,6 @@ class HandshakerFactoryTest : public testing::Test { custom_handshaker->mutable_typed_config()->PackFrom(ProtobufWkt::StringValue()); } - // Helper for downcasting a socket to a test socket so we can examine its - // SSL_CTX. - SSL_CTX* extractSslCtx(Network::TransportSocket* socket) { - SslSocket* ssl_socket = dynamic_cast(socket); - SSL* ssl = ssl_socket->rawSslForTest(); - return SSL_get_SSL_CTX(ssl); - } - NiceMock server_factory_context_; Stats::IsolatedStoreImpl stats_store_; std::unique_ptr context_manager_; diff --git a/test/common/tls/integration/ssl_integration_test.cc b/test/common/tls/integration/ssl_integration_test.cc index fd2e6e844381..4ad8d5867935 100644 --- a/test/common/tls/integration/ssl_integration_test.cc +++ b/test/common/tls/integration/ssl_integration_test.cc @@ -275,6 +275,44 @@ TEST_P(SslIntegrationTest, RouterDownstreamDisconnectBeforeResponseComplete) { checkStats(); } +// Test server preference of cipher suites. Server order is ECDHE-RSA-AES128-GCM-SHA256 +// followed by ECDHE-RSA-AES256-GCM-SHA384. "ECDHE-RSA-AES128-GCM-SHA256" should be used based on +// server preference. +TEST_P(SslIntegrationTest, TestServerCipherPreference) { + server_ciphers_.push_back("ECDHE-RSA-AES128-GCM-SHA256"); + server_ciphers_.push_back("ECDHE-RSA-AES256-GCM-SHA384"); + initialize(); + codec_client_ = makeHttpConnection(makeSslClientConnection( + ClientSslTransportOptions{} + .setTlsVersion(envoy::extensions::transport_sockets::tls::v3::TlsParameters::TLSv1_2) + .setCipherSuites({"ECDHE-RSA-AES256-GCM-SHA384:ECDHE-RSA-AES128-GCM-SHA256"}))); + auto response = + sendRequestAndWaitForResponse(default_request_headers_, 0, default_response_headers_, 0); + + const std::string counter_name = listenerStatPrefix("ssl.ciphers.ECDHE-RSA-AES128-GCM-SHA256"); + Stats::CounterSharedPtr counter = test_server_->counter(counter_name); + EXPECT_EQ(1, test_server_->counter(counter_name)->value()); +} + +// Test client preference of cipher suites. Same server preference is followed as in the previous. +// "ECDHE-RSA-AES256-GCM-SHA384" should be used based on client preference. +TEST_P(SslIntegrationTest, ClientCipherPreference) { + prefer_client_ciphers_ = true; + server_ciphers_.push_back("ECDHE-RSA-AES128-GCM-SHA256"); + server_ciphers_.push_back("ECDHE-RSA-AES256-GCM-SHA384"); + initialize(); + codec_client_ = makeHttpConnection(makeSslClientConnection( + ClientSslTransportOptions{} + .setTlsVersion(envoy::extensions::transport_sockets::tls::v3::TlsParameters::TLSv1_2) + .setCipherSuites({"ECDHE-RSA-AES256-GCM-SHA384:ECDHE-RSA-AES128-GCM-SHA256"}))); + auto response = + sendRequestAndWaitForResponse(default_request_headers_, 0, default_response_headers_, 0); + + const std::string counter_name = listenerStatPrefix("ssl.ciphers.ECDHE-RSA-AES256-GCM-SHA384"); + Stats::CounterSharedPtr counter = test_server_->counter(counter_name); + EXPECT_EQ(1, test_server_->counter(counter_name)->value()); +} + // This test must be here vs integration_admin_test so that it tests a server with loaded certs. TEST_P(SslIntegrationTest, AdminCertEndpoint) { DISABLE_IF_ADMIN_DISABLED; // Admin functionality. diff --git a/test/common/tls/integration/ssl_integration_test_base.cc b/test/common/tls/integration/ssl_integration_test_base.cc index 2a89ffee0972..0539b833a9c2 100644 --- a/test/common/tls/integration/ssl_integration_test_base.cc +++ b/test/common/tls/integration/ssl_integration_test_base.cc @@ -10,8 +10,10 @@ void SslIntegrationTestBase::initialize() { .setEcdsaCert(server_ecdsa_cert_) .setEcdsaCertOcspStaple(server_ecdsa_cert_ocsp_staple_) .setOcspStapleRequired(ocsp_staple_required_) + .setPreferClientCiphers(prefer_client_ciphers_) .setTlsV13(server_tlsv1_3_) .setCurves(server_curves_) + .setCiphers(server_ciphers_) .setExpectClientEcdsaCert(client_ecdsa_cert_) .setTlsKeyLogFilter(keylog_local_, keylog_remote_, keylog_local_negative_, diff --git a/test/common/tls/integration/ssl_integration_test_base.h b/test/common/tls/integration/ssl_integration_test_base.h index 63366d2d1ff9..31bbda465023 100644 --- a/test/common/tls/integration/ssl_integration_test_base.h +++ b/test/common/tls/integration/ssl_integration_test_base.h @@ -31,11 +31,13 @@ class SslIntegrationTestBase : public HttpIntegrationTest { protected: bool server_tlsv1_3_{false}; std::vector server_curves_; + std::vector server_ciphers_; bool server_rsa_cert_{true}; bool server_rsa_cert_ocsp_staple_{false}; bool server_ecdsa_cert_{false}; bool server_ecdsa_cert_ocsp_staple_{false}; bool ocsp_staple_required_{false}; + bool prefer_client_ciphers_{false}; bool client_ecdsa_cert_{false}; // Set this true to debug SSL handshake issues with openssl s_client. The // verbose trace will be in the logs, openssl must be installed separately. diff --git a/test/common/tls/ssl_test_utility.h b/test/common/tls/ssl_test_utility.h index a8bf453ed50a..e437ad970b0c 100644 --- a/test/common/tls/ssl_test_utility.h +++ b/test/common/tls/ssl_test_utility.h @@ -3,6 +3,8 @@ #include #include +#include "source/common/tls/ssl_socket.h" + #include "test/test_common/environment.h" #include "gtest/gtest.h" @@ -36,6 +38,14 @@ inline bssl::UniquePtr readCertChainFromFile(const std::string& return certChain; } +// Helper for downcasting a socket to a test socket so we can examine its +// SSL_CTX. +SSL_CTX* extractSslCtx(Network::TransportSocket* socket) { + SslSocket* ssl_socket = dynamic_cast(socket); + SSL* ssl = ssl_socket->rawSslForTest(); + return SSL_get_SSL_CTX(ssl); +} + } // namespace Tls } // namespace TransportSockets } // namespace Extensions diff --git a/test/config/utility.cc b/test/config/utility.cc index ca67cec4274b..0f908a434394 100644 --- a/test/config/utility.cc +++ b/test/config/utility.cc @@ -1315,6 +1315,7 @@ void ConfigHelper::addSslConfig(const ServerSslOptions& options) { tls_context.set_ocsp_staple_policy( envoy::extensions::transport_sockets::tls::v3::DownstreamTlsContext::MUST_STAPLE); } + tls_context.set_prefer_client_ciphers(options.prefer_client_ciphers_); filter_chain->mutable_transport_socket()->set_name("envoy.transport_sockets.tls"); filter_chain->mutable_transport_socket()->mutable_typed_config()->PackFrom(tls_context); } @@ -1506,6 +1507,9 @@ void ConfigHelper::initializeTls( for (const auto& curve : options.curves_) { common_tls_context.mutable_tls_params()->add_ecdh_curves(curve); } + for (const auto& cipher : options.ciphers_) { + common_tls_context.mutable_tls_params()->add_cipher_suites(cipher); + } if (options.rsa_cert_) { auto* tls_certificate = common_tls_context.add_tls_certificates(); tls_certificate->mutable_certificate_chain()->set_filename( @@ -1528,6 +1532,7 @@ void ConfigHelper::initializeTls( "test/config/integration/certs/server_ecdsa_ocsp_resp.der")); } } + if (!options.san_matchers_.empty()) { *validation_context->mutable_match_typed_subject_alt_names() = {options.san_matchers_.begin(), options.san_matchers_.end()}; diff --git a/test/config/utility.h b/test/config/utility.h index a1e5cff2bf60..c9016cb15691 100644 --- a/test/config/utility.h +++ b/test/config/utility.h @@ -70,6 +70,16 @@ class ConfigHelper { return *this; } + ServerSslOptions& setPreferClientCiphers(bool prefer_client_ciphers) { + prefer_client_ciphers_ = prefer_client_ciphers; + return *this; + } + + ServerSslOptions& setCiphers(const std::vector& ciphers) { + ciphers_ = ciphers; + return *this; + } + ServerSslOptions& setCurves(const std::vector& curves) { curves_ = curves; return *this; @@ -127,9 +137,11 @@ class ConfigHelper { bool rsa_cert_ocsp_staple_{true}; bool ecdsa_cert_{false}; bool ecdsa_cert_ocsp_staple_{false}; + bool prefer_client_ciphers_{false}; bool ocsp_staple_required_{false}; bool tlsv1_3_{false}; std::vector curves_; + std::vector ciphers_; bool expect_client_ecdsa_cert_{false}; bool keylog_local_filter_{false}; bool keylog_remote_filter_{false}; diff --git a/test/mocks/ssl/mocks.h b/test/mocks/ssl/mocks.h index 96923831725e..7a3e35a95707 100644 --- a/test/mocks/ssl/mocks.h +++ b/test/mocks/ssl/mocks.h @@ -126,6 +126,7 @@ class MockServerContextConfig : public ServerContextConfig { MOCK_METHOD(const std::string&, cipherSuites, (), (const)); MOCK_METHOD(const std::string&, ecdhCurves, (), (const)); MOCK_METHOD(const std::string&, signatureAlgorithms, (), (const)); + MOCK_METHOD(bool, preferClientCiphers, (), (const)); MOCK_METHOD(std::vector>, tlsCertificates, (), (const)); MOCK_METHOD(const CertificateValidationContextConfig*, certificateValidationContext, (), (const));