Skip to content

Commit

Permalink
sds: fix combined validation context validation bypassing (#115)
Browse files Browse the repository at this point in the history 
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 <[email protected]>
Co-authored-by: Oliver Liu <[email protected]>
  • Loading branch information
@myidpt @lizan
myidpt authored and lizan committed Mar 3, 2020
1 parent 836d6ba commit b49b560
Show file tree
Hide file tree
Showing 4 changed files with 163 additions and 41 deletions.
2 changes: 2 additions & 0 deletions docs/root/intro/version_history.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ Version history
1.12.3 (Pending)
================
* rbac: added :ref:`url_path <envoy_api_field_config.rbac.v2.Permission.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)
==========================
Expand Down
9 changes: 9 additions & 0 deletions source/common/secret/sds_api.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,9 @@ class TlsCertificateSdsApi : public SdsApi, public TlsCertificateConfigProvider
return nullptr;
}
Common::CallbackHandle* addUpdateCallback(std::function<void()> callback) override {
if (secret()) {
callback();
}
return update_callback_manager_.add(callback);
}

Expand Down Expand Up @@ -174,6 +177,9 @@ class CertificateValidationContextSdsApi : public SdsApi,
return certificate_validation_context_secrets_.get();
}
Common::CallbackHandle* addUpdateCallback(std::function<void()> callback) override {
if (secret()) {
callback();
}
return update_callback_manager_.add(callback);
}

Expand Down Expand Up @@ -237,6 +243,9 @@ class TlsSessionTicketKeysSdsApi : public SdsApi, public TlsSessionTicketKeysCon
}

Common::CallbackHandle* addUpdateCallback(std::function<void()> callback) override {
if (secret()) {
callback();
}
return update_callback_manager_.add(callback);
}

Expand Down
56 changes: 30 additions & 26 deletions source/extensions/transport_sockets/tls/context_config_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,34 +158,40 @@ 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<Ssl::CertificateValidationContextConfigImpl>(
*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) {
tls_certificate_configs_.emplace_back(*provider->secret(), &factory_context, api_);
}
}
}
// 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<Ssl::CertificateValidationContextConfigImpl>(
*certificate_validation_context_provider_->secret(), api_);
}
}

Ssl::CertificateValidationContextConfigPtr ContextConfigImpl::getCombinedValidationContextConfig(
Expand Down Expand Up @@ -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() +
Expand Down
137 changes: 122 additions & 15 deletions test/integration/sds_dynamic_integration_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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);

Expand All @@ -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<Extensions::TransportSockets::Tls::ServerContextConfigImpl>(
tls_context, factory_context_);
static Stats::Scope* upstream_stats_store = new Stats::TestIsolatedStoreImpl();
return std::make_unique<Extensions::TransportSockets::Tls::ServerSslSocketFactory>(
std::move(cfg), context_manager_, *upstream_stats_store, std::vector<std::string>{});
}

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,
Expand Down Expand Up @@ -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:
Expand Down

0 comments on commit b49b560

Please sign in to comment.