From b49b56079c6eb65f7a5f7d06eaa8078c1045c4cc Mon Sep 17 00:00:00 2001 From: Oliver Liu Date: Fri, 7 Feb 2020 11:17:13 -0800 Subject: [PATCH] sds: fix combined validation context validation bypassing (#115) Previously, the update callback was called only when the secret was received for the first time or when its value changed. This meant that if the same secret (e.g. trusted CA) was used in multiple resources, then resources using it but configured after the secret was already received, remained unconfigured until the secret's value changed. The missing callback should have resulted in transport factories stuck in the "not ready" state, however, because of an incorrect code, the available secret was processed like inlined validation context, and only rules from the "secret" part of the validation context were applied, leading to a complete bypass of rules from the "default" part. Signed-off-by: Piotr Sikora Co-authored-by: Oliver Liu --- docs/root/intro/version_history.rst | 2 + source/common/secret/sds_api.h | 9 ++ .../tls/context_config_impl.cc | 56 +++---- .../sds_dynamic_integration_test.cc | 137 ++++++++++++++++-- 4 files changed, 163 insertions(+), 41 deletions(-) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index f3c8292b6287..20ee702b5b7d 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -4,6 +4,8 @@ Version history 1.12.3 (Pending) ================ * rbac: added :ref:`url_path ` for matching URL path without the query and fragment string. +========================== +* sds: fixed the SDS vulnerability that TLS validation context (e.g., subject alt name or hash) cannot be effectively validated in some cases. 1.12.2 (December 10, 2019) ========================== diff --git a/source/common/secret/sds_api.h b/source/common/secret/sds_api.h index 695ecd02e8af..7ab69c1e3168 100644 --- a/source/common/secret/sds_api.h +++ b/source/common/secret/sds_api.h @@ -124,6 +124,9 @@ class TlsCertificateSdsApi : public SdsApi, public TlsCertificateConfigProvider return nullptr; } Common::CallbackHandle* addUpdateCallback(std::function callback) override { + if (secret()) { + callback(); + } return update_callback_manager_.add(callback); } @@ -174,6 +177,9 @@ class CertificateValidationContextSdsApi : public SdsApi, return certificate_validation_context_secrets_.get(); } Common::CallbackHandle* addUpdateCallback(std::function callback) override { + if (secret()) { + callback(); + } return update_callback_manager_.add(callback); } @@ -237,6 +243,9 @@ class TlsSessionTicketKeysSdsApi : public SdsApi, public TlsSessionTicketKeysCon } Common::CallbackHandle* addUpdateCallback(std::function callback) override { + if (secret()) { + callback(); + } return update_callback_manager_.add(callback); } diff --git a/source/extensions/transport_sockets/tls/context_config_impl.cc b/source/extensions/transport_sockets/tls/context_config_impl.cc index e7bcc9f6610b..a3f2704deeaa 100644 --- a/source/extensions/transport_sockets/tls/context_config_impl.cc +++ b/source/extensions/transport_sockets/tls/context_config_impl.cc @@ -158,21 +158,33 @@ ContextConfigImpl::ContextConfigImpl( default_min_protocol_version)), max_protocol_version_(tlsVersionFromProto(config.tls_params().tls_maximum_protocol_version(), default_max_protocol_version)) { - if (default_cvc_ && certificate_validation_context_provider_ != nullptr) { - // We need to validate combined certificate validation context. - // The default certificate validation context and dynamic certificate validation - // context could only contain partial fields, which is okay to fail the validation. - // But the combined certificate validation context should pass validation. If - // validation of combined certificate validation context fails, - // getCombinedValidationContextConfig() throws exception, validation_context_config_ will not - // get updated. - cvc_validation_callback_handle_ = - certificate_validation_context_provider_->addValidationCallback( - [this](const envoy::api::v2::auth::CertificateValidationContext& dynamic_cvc) { - getCombinedValidationContextConfig(dynamic_cvc); - }); + if (certificate_validation_context_provider_ != nullptr) { + if (default_cvc_) { + // We need to validate combined certificate validation context. + // The default certificate validation context and dynamic certificate validation + // context could only contain partial fields, which is okay to fail the validation. + // But the combined certificate validation context should pass validation. If + // validation of combined certificate validation context fails, + // getCombinedValidationContextConfig() throws exception, validation_context_config_ will not + // get updated. + cvc_validation_callback_handle_ = + certificate_validation_context_provider_->addValidationCallback( + [this](const envoy::api::v2::auth::CertificateValidationContext& dynamic_cvc) { + getCombinedValidationContextConfig(dynamic_cvc); + }); + } + // Load inlined, static or dynamic secret that's already available. + if (certificate_validation_context_provider_->secret() != nullptr) { + if (default_cvc_) { + validation_context_config_ = + getCombinedValidationContextConfig(*certificate_validation_context_provider_->secret()); + } else { + validation_context_config_ = std::make_unique( + *certificate_validation_context_provider_->secret(), api_); + } + } } - // Load inline or static secret into tls_certificate_config_. + // Load inlined, static or dynamic secrets that are already available. if (!tls_certificate_providers_.empty()) { for (auto& provider : tls_certificate_providers_) { if (provider->secret() != nullptr) { @@ -180,12 +192,6 @@ ContextConfigImpl::ContextConfigImpl( } } } - // Load inline or static secret into validation_context_config_. - if (certificate_validation_context_provider_ != nullptr && - certificate_validation_context_provider_->secret() != nullptr) { - validation_context_config_ = std::make_unique( - *certificate_validation_context_provider_->secret(), api_); - } } Ssl::CertificateValidationContextConfigPtr ContextConfigImpl::getCombinedValidationContextConfig( @@ -369,12 +375,10 @@ ServerContextConfigImpl::ServerContextConfigImpl( [this](const envoy::api::v2::auth::TlsSessionTicketKeys& keys) { getSessionTicketKeys(keys); }); - } - - // Load inline or static secrets. - if (session_ticket_keys_provider_ != nullptr && - session_ticket_keys_provider_->secret() != nullptr) { - session_ticket_keys_ = getSessionTicketKeys(*session_ticket_keys_provider_->secret()); + // Load inlined, static or dynamic secret that's already available. + if (session_ticket_keys_provider_->secret() != nullptr) { + session_ticket_keys_ = getSessionTicketKeys(*session_ticket_keys_provider_->secret()); + } } if ((config.common_tls_context().tls_certificates().size() + diff --git a/test/integration/sds_dynamic_integration_test.cc b/test/integration/sds_dynamic_integration_test.cc index 60f697af7cc0..38d8391444e9 100644 --- a/test/integration/sds_dynamic_integration_test.cc +++ b/test/integration/sds_dynamic_integration_test.cc @@ -10,6 +10,7 @@ #include "extensions/transport_sockets/tls/context_config_impl.h" #include "extensions/transport_sockets/tls/context_manager_impl.h" +#include "extensions/transport_sockets/tls/ssl_socket.h" #include "test/common/grpc/grpc_client_integration.h" #include "test/config/integration/certs/clientcert_hash.h" @@ -257,21 +258,7 @@ class SdsDynamicDownstreamCertValidationContextTest : public SdsDynamicDownstrea TestEnvironment::runfilesPath("test/config/integration/certs/servercert.pem")); tls_certificate->mutable_private_key()->set_filename( TestEnvironment::runfilesPath("test/config/integration/certs/serverkey.pem")); - - if (use_combined_validation_context_) { - // Modify the listener context validation type to use combined certificate validation - // context. - auto* combined_config = common_tls_context->mutable_combined_validation_context(); - auto* default_validation_context = combined_config->mutable_default_validation_context(); - default_validation_context->add_verify_certificate_hash(TEST_CLIENT_CERT_HASH); - auto* secret_config = combined_config->mutable_validation_context_sds_secret_config(); - setUpSdsConfig(secret_config, validation_secret_); - } else { - // Modify the listener context validation type to use dynamic certificate validation - // context. - auto* secret_config = common_tls_context->mutable_validation_context_sds_secret_config(); - setUpSdsConfig(secret_config, validation_secret_); - } + setUpSdsValidationContext(common_tls_context); transport_socket->set_name("envoy.transport_sockets.tls"); transport_socket->mutable_typed_config()->PackFrom(tls_context); @@ -280,16 +267,89 @@ class SdsDynamicDownstreamCertValidationContextTest : public SdsDynamicDownstrea sds_cluster->MergeFrom(bootstrap.static_resources().clusters()[0]); sds_cluster->set_name("sds_cluster"); sds_cluster->mutable_http2_protocol_options(); + + envoy::api::v2::auth::UpstreamTlsContext upstream_tls_context; + if (share_validation_secret_) { + // Configure static cluster with SDS config referencing "validation_secret", + // which is going to be processed before LDS resources. + ASSERT(use_lds_); + setUpSdsValidationContext(upstream_tls_context.mutable_common_tls_context()); + } + // Enable SSL/TLS with a client certificate in the first cluster. + auto* upstream_tls_certificate = + upstream_tls_context.mutable_common_tls_context()->add_tls_certificates(); + upstream_tls_certificate->mutable_certificate_chain()->set_filename( + TestEnvironment::runfilesPath("test/config/integration/certs/clientcert.pem")); + upstream_tls_certificate->mutable_private_key()->set_filename( + TestEnvironment::runfilesPath("test/config/integration/certs/clientkey.pem")); + auto* upstream_transport_socket = + bootstrap.mutable_static_resources()->mutable_clusters(0)->mutable_transport_socket(); + upstream_transport_socket->set_name("envoy.transport_sockets.tls"); + upstream_transport_socket->mutable_typed_config()->PackFrom(upstream_tls_context); }); HttpIntegrationTest::initialize(); + registerTestServerPorts({"http"}); client_ssl_ctx_ = createClientSslTransportSocketFactory({}, context_manager_, *api_); } + void setUpSdsValidationContext(envoy::api::v2::auth::CommonTlsContext* common_tls_context) { + if (use_combined_validation_context_) { + // Modify the listener context validation type to use combined certificate validation + // context. + auto* combined_config = common_tls_context->mutable_combined_validation_context(); + auto* default_validation_context = combined_config->mutable_default_validation_context(); + default_validation_context->add_verify_certificate_hash(TEST_CLIENT_CERT_HASH); + auto* secret_config = combined_config->mutable_validation_context_sds_secret_config(); + setUpSdsConfig(secret_config, validation_secret_); + } else { + // Modify the listener context validation type to use dynamic certificate validation + // context. + auto* secret_config = common_tls_context->mutable_validation_context_sds_secret_config(); + setUpSdsConfig(secret_config, validation_secret_); + } + } + + void createUpstreams() override { + // Fake upstream with SSL/TLS for the first cluster. + fake_upstreams_.emplace_back(new FakeUpstream( + createUpstreamSslContext(), 0, FakeHttpConnection::Type::HTTP1, version_, timeSystem())); + create_xds_upstream_ = true; + } + + Network::TransportSocketFactoryPtr createUpstreamSslContext() { + envoy::api::v2::auth::DownstreamTlsContext tls_context; + auto* common_tls_context = tls_context.mutable_common_tls_context(); + auto* tls_certificate = common_tls_context->add_tls_certificates(); + tls_certificate->mutable_certificate_chain()->set_filename( + TestEnvironment::runfilesPath("test/config/integration/certs/clientcert.pem")); + tls_certificate->mutable_private_key()->set_filename( + TestEnvironment::runfilesPath("test/config/integration/certs/clientkey.pem")); + + auto cfg = std::make_unique( + tls_context, factory_context_); + static Stats::Scope* upstream_stats_store = new Stats::TestIsolatedStoreImpl(); + return std::make_unique( + std::move(cfg), context_manager_, *upstream_stats_store, std::vector{}); + } + + void TearDown() override { + cleanUpXdsConnection(); + + client_ssl_ctx_.reset(); + cleanupUpstreamAndDownstream(); + codec_client_.reset(); + + test_server_.reset(); + fake_upstreams_.clear(); + } + void enableCombinedValidationContext(bool enable) { use_combined_validation_context_ = enable; } + void shareValidationSecret(bool share) { share_validation_secret_ = share; } private: bool use_combined_validation_context_{false}; + bool share_validation_secret_{false}; }; INSTANTIATE_TEST_SUITE_P(IpVersionsClientType, SdsDynamicDownstreamCertValidationContextTest, @@ -327,6 +387,53 @@ TEST_P(SdsDynamicDownstreamCertValidationContextTest, CombinedCertValidationCont testRouterHeaderOnlyRequestAndResponse(&creator); } +// A test that verifies that both: static cluster and LDS listener are updated when using +// the same verification secret (standalone validation context) from the SDS server. +TEST_P(SdsDynamicDownstreamCertValidationContextTest, BasicWithSharedSecret) { + shareValidationSecret(true); + on_server_init_function_ = [this]() { + createSdsStream(*(fake_upstreams_[1])); + sendSdsResponse(getCvcSecret()); + }; + initialize(); + + // Wait for "ssl_context_updated_by_sds" counters to indicate that both resources + // depending on the verification_secret were updated. + test_server_->waitForCounterGe( + "cluster.cluster_0.client_ssl_socket_factory.ssl_context_update_by_sds", 1); + test_server_->waitForCounterGe( + listenerStatPrefix("server_ssl_socket_factory.ssl_context_update_by_sds"), 1); + + ConnectionCreationFunction creator = [&]() -> Network::ClientConnectionPtr { + return makeSslClientConnection(); + }; + testRouterHeaderOnlyRequestAndResponse(&creator); +} + +// A test that verifies that both: static cluster and LDS listener are updated when using +// the same verification secret (combined validation context) from the SDS server. +TEST_P(SdsDynamicDownstreamCertValidationContextTest, CombinedValidationContextWithSharedSecret) { + enableCombinedValidationContext(true); + shareValidationSecret(true); + on_server_init_function_ = [this]() { + createSdsStream(*(fake_upstreams_[1])); + sendSdsResponse(getCvcSecretWithOnlyTrustedCa()); + }; + initialize(); + + // Wait for "ssl_context_updated_by_sds" counters to indicate that both resources + // depending on the verification_secret were updated. + test_server_->waitForCounterGe( + "cluster.cluster_0.client_ssl_socket_factory.ssl_context_update_by_sds", 1); + test_server_->waitForCounterGe( + listenerStatPrefix("server_ssl_socket_factory.ssl_context_update_by_sds"), 1); + + ConnectionCreationFunction creator = [&]() -> Network::ClientConnectionPtr { + return makeSslClientConnection(); + }; + testRouterHeaderOnlyRequestAndResponse(&creator); +} + // Upstream SDS integration test: a static cluster has ssl cert from SDS. class SdsDynamicUpstreamIntegrationTest : public SdsDynamicIntegrationBaseTest { public: