From 68d1a87dd18bfce9acd3249a089c238559a5f5fe Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Tue, 3 Mar 2020 13:05:27 -0800 Subject: [PATCH] sds: fix combined validation context validation bypassing (#114) (#174) 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 Co-authored-by: Oliver Liu --- source/common/secret/sds_api.h | 9 ++ .../tls/context_config_impl.cc | 56 +++---- .../sds_dynamic_integration_test.cc | 138 ++++++++++++++++-- 3 files changed, 162 insertions(+), 41 deletions(-) diff --git a/source/common/secret/sds_api.h b/source/common/secret/sds_api.h index af2b1c3fb002..db382082b84c 100644 --- a/source/common/secret/sds_api.h +++ b/source/common/secret/sds_api.h @@ -128,6 +128,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); } @@ -180,6 +183,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); } @@ -247,6 +253,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 49f5a4811a4a..36bbb867c33e 100644 --- a/source/extensions/transport_sockets/tls/context_config_impl.cc +++ b/source/extensions/transport_sockets/tls/context_config_impl.cc @@ -166,21 +166,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::extensions::transport_sockets::tls::v3::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::extensions::transport_sockets::tls::v3::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) { @@ -188,12 +200,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( @@ -381,12 +387,10 @@ ServerContextConfigImpl::ServerContextConfigImpl( [this](const envoy::extensions::transport_sockets::tls::v3::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 68d7e3c83101..13242efd4a9c 100644 --- a/test/integration/sds_dynamic_integration_test.cc +++ b/test/integration/sds_dynamic_integration_test.cc @@ -15,6 +15,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" @@ -263,21 +264,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); @@ -286,16 +273,90 @@ 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::extensions::transport_sockets::tls::v3::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::extensions::transport_sockets::tls::v3::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::extensions::transport_sockets::tls::v3::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, @@ -333,6 +394,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: