diff --git a/api/envoy/extensions/transport_sockets/tls/v3/tls.proto b/api/envoy/extensions/transport_sockets/tls/v3/tls.proto index 44325bdbee6a4..02287de5875fb 100644 --- a/api/envoy/extensions/transport_sockets/tls/v3/tls.proto +++ b/api/envoy/extensions/transport_sockets/tls/v3/tls.proto @@ -216,8 +216,14 @@ message CommonTlsContext { // Configs for fetching TLS certificates via SDS API. Note SDS API allows certificates to be // fetched/refreshed over the network asynchronously with respect to the TLS handshake. + // + // The same number and types of certificates as :ref:`tls_certificates ` + // are valid in the the certificates fetched through this setting. + // + // If :ref:`tls_certificates ` + // is non-empty, this field is ignored. repeated SdsSecretConfig tls_certificate_sds_secret_configs = 6 - [(validate.rules).repeated = {max_items: 1}]; + [(validate.rules).repeated = {max_items: 2}]; // Certificate provider for fetching TLS certificates. // [#not-implemented-hide:] diff --git a/api/envoy/extensions/transport_sockets/tls/v4alpha/tls.proto b/api/envoy/extensions/transport_sockets/tls/v4alpha/tls.proto index 7ddf55dfa6fab..b92cae619dd9c 100644 --- a/api/envoy/extensions/transport_sockets/tls/v4alpha/tls.proto +++ b/api/envoy/extensions/transport_sockets/tls/v4alpha/tls.proto @@ -221,8 +221,14 @@ message CommonTlsContext { // Configs for fetching TLS certificates via SDS API. Note SDS API allows certificates to be // fetched/refreshed over the network asynchronously with respect to the TLS handshake. + // + // The same number and types of certificates as :ref:`tls_certificates ` + // are valid in the the certificates fetched through this setting. + // + // If :ref:`tls_certificates ` + // is non-empty, this field is ignored. repeated SdsSecretConfig tls_certificate_sds_secret_configs = 6 - [(validate.rules).repeated = {max_items: 1}]; + [(validate.rules).repeated = {max_items: 2}]; // Certificate provider for fetching TLS certificates. // [#not-implemented-hide:] diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 327f4a3ac56c4..98ec78e0aa562 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -76,6 +76,7 @@ New Features * metric service: added support for sending metric tags as labels. This can be enabled by setting the :ref:`emit_tags_as_labels ` field to true. * tcp: added support for :ref:`preconnecting `. Preconnecting is off by default, but recommended for clusters serving latency-sensitive traffic. * thrift_proxy: added per upstream metrics within the :ref:`thrift router ` for request and response size histograms. +* tls: allow dual ECDSA/RSA certs via SDS. Previously, SDS only supported a single certificate per context, and dual cert was only supported via non-SDS. * udp_proxy: added :ref:`key ` as another hash policy to support hash based routing on any given key. Deprecated diff --git a/generated_api_shadow/envoy/extensions/transport_sockets/tls/v3/tls.proto b/generated_api_shadow/envoy/extensions/transport_sockets/tls/v3/tls.proto index 44325bdbee6a4..02287de5875fb 100644 --- a/generated_api_shadow/envoy/extensions/transport_sockets/tls/v3/tls.proto +++ b/generated_api_shadow/envoy/extensions/transport_sockets/tls/v3/tls.proto @@ -216,8 +216,14 @@ message CommonTlsContext { // Configs for fetching TLS certificates via SDS API. Note SDS API allows certificates to be // fetched/refreshed over the network asynchronously with respect to the TLS handshake. + // + // The same number and types of certificates as :ref:`tls_certificates ` + // are valid in the the certificates fetched through this setting. + // + // If :ref:`tls_certificates ` + // is non-empty, this field is ignored. repeated SdsSecretConfig tls_certificate_sds_secret_configs = 6 - [(validate.rules).repeated = {max_items: 1}]; + [(validate.rules).repeated = {max_items: 2}]; // Certificate provider for fetching TLS certificates. // [#not-implemented-hide:] diff --git a/generated_api_shadow/envoy/extensions/transport_sockets/tls/v4alpha/tls.proto b/generated_api_shadow/envoy/extensions/transport_sockets/tls/v4alpha/tls.proto index 7ddf55dfa6fab..b92cae619dd9c 100644 --- a/generated_api_shadow/envoy/extensions/transport_sockets/tls/v4alpha/tls.proto +++ b/generated_api_shadow/envoy/extensions/transport_sockets/tls/v4alpha/tls.proto @@ -221,8 +221,14 @@ message CommonTlsContext { // Configs for fetching TLS certificates via SDS API. Note SDS API allows certificates to be // fetched/refreshed over the network asynchronously with respect to the TLS handshake. + // + // The same number and types of certificates as :ref:`tls_certificates ` + // are valid in the the certificates fetched through this setting. + // + // If :ref:`tls_certificates ` + // is non-empty, this field is ignored. repeated SdsSecretConfig tls_certificate_sds_secret_configs = 6 - [(validate.rules).repeated = {max_items: 1}]; + [(validate.rules).repeated = {max_items: 2}]; // Certificate provider for fetching TLS certificates. // [#not-implemented-hide:] diff --git a/source/extensions/transport_sockets/tls/context_config_impl.cc b/source/extensions/transport_sockets/tls/context_config_impl.cc index 40f113c95fefc..d20f55d817e84 100644 --- a/source/extensions/transport_sockets/tls/context_config_impl.cc +++ b/source/extensions/transport_sockets/tls/context_config_impl.cc @@ -26,8 +26,8 @@ namespace { std::vector getTlsCertificateConfigProviders( const envoy::extensions::transport_sockets::tls::v3::CommonTlsContext& config, Server::Configuration::TransportSocketFactoryContext& factory_context) { + std::vector providers; if (!config.tls_certificates().empty()) { - std::vector providers; for (const auto& tls_certificate : config.tls_certificates()) { if (!tls_certificate.has_private_key_provider() && !tls_certificate.has_certificate_chain() && !tls_certificate.has_private_key()) { @@ -39,20 +39,22 @@ std::vector getTlsCertificateConf return providers; } if (!config.tls_certificate_sds_secret_configs().empty()) { - const auto& sds_secret_config = config.tls_certificate_sds_secret_configs(0); - if (sds_secret_config.has_sds_config()) { - // Fetch dynamic secret. - return {factory_context.secretManager().findOrCreateTlsCertificateProvider( - sds_secret_config.sds_config(), sds_secret_config.name(), factory_context)}; - } else { - // Load static secret. - auto secret_provider = factory_context.secretManager().findStaticTlsCertificateProvider( - sds_secret_config.name()); - if (!secret_provider) { - throw EnvoyException(fmt::format("Unknown static secret: {}", sds_secret_config.name())); + for (const auto& sds_secret_config : config.tls_certificate_sds_secret_configs()) { + if (sds_secret_config.has_sds_config()) { + // Fetch dynamic secret. + providers.push_back(factory_context.secretManager().findOrCreateTlsCertificateProvider( + sds_secret_config.sds_config(), sds_secret_config.name(), factory_context)); + } else { + // Load static secret. + auto secret_provider = factory_context.secretManager().findStaticTlsCertificateProvider( + sds_secret_config.name()); + if (!secret_provider) { + throw EnvoyException(fmt::format("Unknown static secret: {}", sds_secret_config.name())); + } + providers.push_back(secret_provider); } - return {secret_provider}; } + return providers; } return {}; } @@ -247,25 +249,27 @@ Ssl::CertificateValidationContextConfigPtr ContextConfigImpl::getCombinedValidat } void ContextConfigImpl::setSecretUpdateCallback(std::function callback) { - if (!tls_certificate_providers_.empty()) { - // Once tls_certificate_config_ receives new secret, this callback updates - // ContextConfigImpl::tls_certificate_config_ with new secret. - tc_update_callback_handle_ = - tls_certificate_providers_[0]->addUpdateCallback([this, callback]() { - // This breaks multiple certificate support, but today SDS is only single cert. - // TODO(htuch): Fix this when SDS goes multi-cert. + // When any of tls_certificate_providers_ receives a new secret, this callback updates + // ContextConfigImpl::tls_certificate_configs_ with new secret. + for (const auto& tls_certificate_provider : tls_certificate_providers_) { + tc_update_callback_handles_.push_back( + tls_certificate_provider->addUpdateCallback([this, callback]() { tls_certificate_configs_.clear(); - tls_certificate_configs_.emplace_back(*tls_certificate_providers_[0]->secret(), nullptr, - api_); + for (const auto& tls_certificate_provider : tls_certificate_providers_) { + auto* secret = tls_certificate_provider->secret(); + if (secret != nullptr) { + tls_certificate_configs_.emplace_back(*secret, nullptr, api_); + } + } callback(); - }); + })); } if (certificate_validation_context_provider_) { if (default_cvc_) { // Once certificate_validation_context_provider_ receives new secret, this callback updates // ContextConfigImpl::validation_context_config_ with a combined certificate validation - // context. The combined certificate validation context is created by merging new secret into - // default_cvc_. + // context. The combined certificate validation context is created by merging new secret + // into default_cvc_. cvc_update_callback_handle_ = certificate_validation_context_provider_->addUpdateCallback([this, callback]() { validation_context_config_ = getCombinedValidationContextConfig( diff --git a/source/extensions/transport_sockets/tls/context_config_impl.h b/source/extensions/transport_sockets/tls/context_config_impl.h index 85ef556d874b7..549575aa0937e 100644 --- a/source/extensions/transport_sockets/tls/context_config_impl.h +++ b/source/extensions/transport_sockets/tls/context_config_impl.h @@ -87,7 +87,7 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig { default_cvc_; std::vector tls_certificate_providers_; // Handle for TLS certificate dynamic secret callback. - Envoy::Common::CallbackHandlePtr tc_update_callback_handle_; + std::vector tc_update_callback_handles_; Secret::CertificateValidationContextConfigProviderSharedPtr certificate_validation_context_provider_; // Handle for certificate validation context dynamic secret callback. diff --git a/test/integration/sds_dynamic_integration_test.cc b/test/integration/sds_dynamic_integration_test.cc index 19b2636fcb5fb..3b9dd698393bb 100644 --- a/test/integration/sds_dynamic_integration_test.cc +++ b/test/integration/sds_dynamic_integration_test.cc @@ -83,13 +83,11 @@ class SdsDynamicIntegrationBaseTest : public Grpc::BaseGrpcClientIntegrationPara public: SdsDynamicIntegrationBaseTest() : HttpIntegrationTest(Http::CodecClient::Type::HTTP1, GetParam().ip_version), - server_cert_("server_cert"), validation_secret_("validation_secret"), - client_cert_("client_cert"), test_quic_(GetParam().test_quic) {} + test_quic_(GetParam().test_quic) {} SdsDynamicIntegrationBaseTest(Http::CodecClient::Type downstream_protocol, Network::Address::IpVersion version, const std::string& config) - : HttpIntegrationTest(downstream_protocol, version, config), server_cert_("server_cert"), - validation_secret_("validation_secret"), client_cert_("client_cert"), + : HttpIntegrationTest(downstream_protocol, version, config), test_quic_(GetParam().test_quic) {} Network::Address::IpVersion ipVersion() const override { return GetParam().ip_version; } @@ -116,9 +114,9 @@ class SdsDynamicIntegrationBaseTest : public Grpc::BaseGrpcClientIntegrationPara setGrpcService(*grpc_service, "sds_cluster", fake_upstreams_.back()->localAddress()); } - envoy::extensions::transport_sockets::tls::v3::Secret getServerSecret() { + envoy::extensions::transport_sockets::tls::v3::Secret getServerSecretRsa() { envoy::extensions::transport_sockets::tls::v3::Secret secret; - secret.set_name(server_cert_); + secret.set_name(server_cert_rsa_); auto* tls_certificate = secret.mutable_tls_certificate(); tls_certificate->mutable_certificate_chain()->set_filename( TestEnvironment::runfilesPath("test/config/integration/certs/servercert.pem")); @@ -181,9 +179,10 @@ class SdsDynamicIntegrationBaseTest : public Grpc::BaseGrpcClientIntegrationPara } } - const std::string server_cert_; - const std::string validation_secret_; - const std::string client_cert_; + const std::string server_cert_rsa_{"server_cert_rsa"}; + const std::string server_cert_ecdsa_{"server_cert_ecdsa"}; + const std::string validation_secret_{"validation_secret"}; + const std::string client_cert_{"client_cert"}; bool v3_resource_api_{false}; bool test_quic_; }; @@ -232,8 +231,40 @@ class SdsDynamicDownstreamIntegrationTest : public SdsDynamicIntegrationBaseTest validation_context->add_verify_certificate_hash(TEST_CLIENT_CERT_HASH); // Modify the listener ssl cert to use SDS from sds_cluster - auto* secret_config = common_tls_context.add_tls_certificate_sds_secret_configs(); - setUpSdsConfig(secret_config, "server_cert"); + auto* secret_config_rsa = common_tls_context.add_tls_certificate_sds_secret_configs(); + setUpSdsConfig(secret_config_rsa, server_cert_rsa_); + + // Add an additional SDS config for an EC cert (the base test has SDS config for an RSA cert). + // This is done via the filesystem instead of gRPC to simplify the test setup. + if (dual_cert_) { + auto* secret_config_ecdsa = common_tls_context.add_tls_certificate_sds_secret_configs(); + + secret_config_ecdsa->set_name(server_cert_ecdsa_); + auto* config_source = secret_config_ecdsa->mutable_sds_config(); + constexpr absl::string_view sds_template = + R"EOF( +--- +version_info: "0" +resources: +- "@type": type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.Secret + name: "{}" + tls_certificate: + certificate_chain: + filename: "{}" + private_key: + filename: "{}" +)EOF"; + + const std::string sds_content = fmt::format( + sds_template, server_cert_ecdsa_, + TestEnvironment::runfilesPath("test/config/integration/certs/server_ecdsacert.pem"), + TestEnvironment::runfilesPath("test/config/integration/certs/server_ecdsakey.pem")); + + auto sds_path = + TestEnvironment::writeStringToFileForTest("server_cert_ecdsa.sds.yaml", sds_content); + config_source->set_path(sds_path); + config_source->set_resource_api_version(envoy::config::core::v3::ApiVersion::V3); + } } void createUpstreams() override { @@ -281,6 +312,7 @@ class SdsDynamicDownstreamIntegrationTest : public SdsDynamicIntegrationBaseTest protected: Network::TransportSocketFactoryPtr client_ssl_ctx_; + bool dual_cert_{false}; }; INSTANTIATE_TEST_SUITE_P(IpVersionsClientType, SdsDynamicDownstreamIntegrationTest, @@ -290,7 +322,7 @@ class SdsDynamicKeyRotationIntegrationTest : public SdsDynamicDownstreamIntegrat protected: envoy::extensions::transport_sockets::tls::v3::Secret getCurrentServerSecret() { envoy::extensions::transport_sockets::tls::v3::Secret secret; - secret.set_name(server_cert_); + secret.set_name(server_cert_rsa_); auto* tls_certificate = secret.mutable_tls_certificate(); tls_certificate->mutable_certificate_chain()->set_filename( TestEnvironment::temporaryPath("root/current/servercert.pem")); @@ -333,8 +365,8 @@ TEST_P(SdsDynamicKeyRotationIntegrationTest, BasicRotation) { TestEnvironment::temporaryPath("root/current")); waitForSdsUpdateStats(2); // The rotation is not a SDS attempt, so no change to these stats. - EXPECT_EQ(1, test_server_->counter("sds.server_cert.update_success")->value()); - EXPECT_EQ(0, test_server_->counter("sds.server_cert.update_rejected")->value()); + EXPECT_EQ(1, test_server_->counter("sds.server_cert_rsa.update_success")->value()); + EXPECT_EQ(0, test_server_->counter("sds.server_cert_rsa.update_rejected")->value()); // First request with server_ecdsa{cert,key}.pem. testRouterHeaderOnlyRequestAndResponse(&creator); @@ -365,11 +397,11 @@ TEST_P(SdsDynamicKeyRotationIntegrationTest, EmptyRotation) { // Rotate to an empty directory, this should fail. TestEnvironment::renameFile(TestEnvironment::temporaryPath("root/empty"), TestEnvironment::temporaryPath("root/current")); - test_server_->waitForCounterEq("sds.server_cert.key_rotation_failed", 1); + test_server_->waitForCounterEq("sds.server_cert_rsa.key_rotation_failed", 1); waitForSdsUpdateStats(1); // The rotation is not a SDS attempt, so no change to these stats. - EXPECT_EQ(1, test_server_->counter("sds.server_cert.update_success")->value()); - EXPECT_EQ(0, test_server_->counter("sds.server_cert.update_rejected")->value()); + EXPECT_EQ(1, test_server_->counter("sds.server_cert_rsa.update_success")->value()); + EXPECT_EQ(0, test_server_->counter("sds.server_cert_rsa.update_rejected")->value()); // Requests continue to work with key/cert pair. testRouterHeaderOnlyRequestAndResponse(&creator); @@ -380,18 +412,64 @@ TEST_P(SdsDynamicKeyRotationIntegrationTest, EmptyRotation) { TEST_P(SdsDynamicDownstreamIntegrationTest, BasicSuccess) { on_server_init_function_ = [this]() { createSdsStream(*(fake_upstreams_[1])); - sendSdsResponse(getServerSecret()); + sendSdsResponse(getServerSecretRsa()); + }; + initialize(); + + ConnectionCreationFunction creator = [&]() -> Network::ClientConnectionPtr { + return makeSslClientConnection(); + }; + testRouterHeaderOnlyRequestAndResponse(&creator); + + // Success + EXPECT_EQ(1, test_server_->counter("sds.server_cert_rsa.update_success")->value()); + EXPECT_EQ(0, test_server_->counter("sds.server_cert_rsa.update_rejected")->value()); +} + +TEST_P(SdsDynamicDownstreamIntegrationTest, DualCert) { + on_server_init_function_ = [this]() { + createSdsStream(*(fake_upstreams_[1])); + sendSdsResponse(getServerSecretRsa()); }; + + dual_cert_ = true; initialize(); ConnectionCreationFunction creator = [&]() -> Network::ClientConnectionPtr { return makeSslClientConnection(); }; + + client_ssl_ctx_ = createClientSslTransportSocketFactory( + ClientSslTransportOptions() + .setTlsVersion(envoy::extensions::transport_sockets::tls::v3::TlsParameters::TLSv1_2) + .setCipherSuites({"ECDHE-ECDSA-AES128-GCM-SHA256"}), + context_manager_, *api_); + testRouterHeaderOnlyRequestAndResponse(&creator); + + cleanupUpstreamAndDownstream(); + client_ssl_ctx_ = createClientSslTransportSocketFactory( + ClientSslTransportOptions() + .setTlsVersion(envoy::extensions::transport_sockets::tls::v3::TlsParameters::TLSv1_2) + .setCipherSuites({"ECDHE-RSA-AES128-GCM-SHA256"}), + context_manager_, *api_); testRouterHeaderOnlyRequestAndResponse(&creator); // Success - EXPECT_EQ(1, test_server_->counter("sds.server_cert.update_success")->value()); - EXPECT_EQ(0, test_server_->counter("sds.server_cert.update_rejected")->value()); + EXPECT_EQ(1, test_server_->counter("sds.server_cert_rsa.update_success")->value()); + EXPECT_EQ(0, test_server_->counter("sds.server_cert_rsa.update_rejected")->value()); + EXPECT_EQ(1, test_server_->counter("sds.server_cert_ecdsa.update_success")->value()); + EXPECT_EQ(0, test_server_->counter("sds.server_cert_ecdsa.update_rejected")->value()); + + // QUIC ignores the `setTlsVersion` set above and always uses TLS 1.3, and TLS 1.3 ignores the + // `setCipherSuites`, so in the QUIC config, this test only uses one of the certs. + if (!test_quic_) { + EXPECT_EQ(1, + test_server_->counter(listenerStatPrefix("ssl.ciphers.ECDHE-RSA-AES128-GCM-SHA256")) + ->value()); + EXPECT_EQ(1, + test_server_->counter(listenerStatPrefix("ssl.ciphers.ECDHE-ECDSA-AES128-GCM-SHA256")) + ->value()); + } } // A test that SDS server send a bad secret for a static listener, @@ -400,7 +478,7 @@ TEST_P(SdsDynamicDownstreamIntegrationTest, BasicSuccess) { TEST_P(SdsDynamicDownstreamIntegrationTest, WrongSecretFirst) { on_server_init_function_ = [this]() { createSdsStream(*(fake_upstreams_[1])); - sendSdsResponse(getWrongSecret(server_cert_)); + sendSdsResponse(getWrongSecret(server_cert_rsa_)); }; initialize(); @@ -410,10 +488,10 @@ TEST_P(SdsDynamicDownstreamIntegrationTest, WrongSecretFirst) { codec_client_->connection()->close(Network::ConnectionCloseType::NoFlush); // Failure - EXPECT_EQ(0, test_server_->counter("sds.server_cert.update_success")->value()); - EXPECT_EQ(1, test_server_->counter("sds.server_cert.update_rejected")->value()); + EXPECT_EQ(0, test_server_->counter("sds.server_cert_rsa.update_success")->value()); + EXPECT_EQ(1, test_server_->counter("sds.server_cert_rsa.update_rejected")->value()); - sendSdsResponse(getServerSecret()); + sendSdsResponse(getServerSecretRsa()); // Wait for sds update counter. waitForSdsUpdateStats(1); @@ -424,8 +502,8 @@ TEST_P(SdsDynamicDownstreamIntegrationTest, WrongSecretFirst) { testRouterHeaderOnlyRequestAndResponse(&creator); // Success - EXPECT_EQ(1, test_server_->counter("sds.server_cert.update_success")->value()); - EXPECT_EQ(1, test_server_->counter("sds.server_cert.update_rejected")->value()); + EXPECT_EQ(1, test_server_->counter("sds.server_cert_rsa.update_success")->value()); + EXPECT_EQ(1, test_server_->counter("sds.server_cert_rsa.update_rejected")->value()); } class SdsDynamicDownstreamCertValidationContextTest : public SdsDynamicDownstreamIntegrationTest {