Skip to content
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

tls: add enable_client_cipher_preference option #35106

Merged
merged 12 commits into from
Jul 23, 2024
7 changes: 6 additions & 1 deletion api/envoy/extensions/transport_sockets/tls/v3/tls.proto
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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 <arch_overview_ssl_cert_select>`.
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.
Expand Down
5 changes: 5 additions & 0 deletions changelogs/current.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@ removed_config_or_runtime:
path.

new_features:
- area: tls
change: |
Added :ref:`prefer_client_ciphers
<envoy_v3_api_field_extensions.transport_sockets.tls.v3.DownstreamTlsContext.prefer_client_ciphers>`
to support enabling client cipher preference instead of server's for TLS handshakes.
- area: ext_authz
change: |
Added config field
Expand Down
5 changes: 5 additions & 0 deletions envoy/ssl/context_config.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<ServerContextConfig>;
Expand Down
5 changes: 0 additions & 5 deletions source/common/tls/context_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
3 changes: 2 additions & 1 deletion source/common/tls/server_context_config_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 2 additions & 0 deletions source/common/tls/server_context_config_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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
Expand Down
5 changes: 5 additions & 0 deletions source/common/tls/server_context_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand Down
2 changes: 2 additions & 0 deletions test/common/tls/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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",
],
Expand Down Expand Up @@ -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",
Expand Down
51 changes: 51 additions & 0 deletions test/common/tls/context_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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<Network::TransportSocket> 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<Network::TransportSocket> 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:
Expand Down
9 changes: 1 addition & 8 deletions test/common/tls/handshaker_factory_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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<SslSocket*>(socket);
SSL* ssl = ssl_socket->rawSslForTest();
return SSL_get_SSL_CTX(ssl);
}

NiceMock<Server::Configuration::MockServerFactoryContext> server_factory_context_;
Stats::IsolatedStoreImpl stats_store_;
std::unique_ptr<Extensions::TransportSockets::Tls::ContextManagerImpl> context_manager_;
Expand Down
38 changes: 38 additions & 0 deletions test/common/tls/integration/ssl_integration_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 2 additions & 0 deletions test/common/tls/integration/ssl_integration_test_base.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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_,
Expand Down
2 changes: 2 additions & 0 deletions test/common/tls/integration/ssl_integration_test_base.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,13 @@ class SslIntegrationTestBase : public HttpIntegrationTest {
protected:
bool server_tlsv1_3_{false};
std::vector<std::string> server_curves_;
std::vector<std::string> 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.
Expand Down
10 changes: 10 additions & 0 deletions test/common/tls/ssl_test_utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
#include <string>
#include <vector>

#include "source/common/tls/ssl_socket.h"

#include "test/test_common/environment.h"

#include "gtest/gtest.h"
Expand Down Expand Up @@ -36,6 +38,14 @@ inline bssl::UniquePtr<STACK_OF(X509)> 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<SslSocket*>(socket);
SSL* ssl = ssl_socket->rawSslForTest();
return SSL_get_SSL_CTX(ssl);
}

} // namespace Tls
} // namespace TransportSockets
} // namespace Extensions
Expand Down
5 changes: 5 additions & 0 deletions test/config/utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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(
Expand All @@ -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()};
Expand Down
12 changes: 12 additions & 0 deletions test/config/utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<std::string>& ciphers) {
ciphers_ = ciphers;
return *this;
}

ServerSslOptions& setCurves(const std::vector<std::string>& curves) {
curves_ = curves;
return *this;
Expand Down Expand Up @@ -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<std::string> curves_;
std::vector<std::string> ciphers_;
bool expect_client_ecdsa_cert_{false};
bool keylog_local_filter_{false};
bool keylog_remote_filter_{false};
Expand Down
1 change: 1 addition & 0 deletions test/mocks/ssl/mocks.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<std::reference_wrapper<const TlsCertificateConfig>>, tlsCertificates, (),
(const));
MOCK_METHOD(const CertificateValidationContextConfig*, certificateValidationContext, (), (const));
Expand Down