Skip to content

Commit

Permalink
sds: fix combined validation context validation bypassing (envoyproxy…
Browse files Browse the repository at this point in the history 
…#114) (envoyproxy#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 <piotrsikora@google.com>
Co-authored-by: Oliver Liu <yonggangl@google.com>

Co-authored-by: Oliver Liu <yonggangl@google.com>
  • Loading branch information
@PiotrSikora @myidpt
PiotrSikora and myidpt committed Mar 3, 2020
1 parent 1af8b4e commit 68d1a87
Show file tree
Hide file tree
Showing 3 changed files with 162 additions and 41 deletions.
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 @@ -128,6 +128,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 @@ -180,6 +183,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 @@ -247,6 +253,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 @@ -166,34 +166,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::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<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 @@ -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() +
Expand Down
138 changes: 123 additions & 15 deletions test/integration/sds_dynamic_integration_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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);

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

0 comments on commit 68d1a87

Please sign in to comment.