Skip to content

sds: fix combined validation context validation bypassing (#114) #174

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
istio-testing merged 1 commit into from
Mar 3, 2020
Merged

sds: fix combined validation context validation bypassing (#114) #174

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions source/common/secret/sds_api.h
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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