Skip to content

Remove support for verify_subject_alt_name in CertificateValidationContext #16978

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
alyssawilk merged 3 commits into from
Jun 21, 2021
Merged

Remove support for verify_subject_alt_name in CertificateValidationContext #16978

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b6ad2c4
Signed-off-by: Tianyu Xia <tyxia@google.com>
tyxia May 27, 2021
1fb5e23
Remove support for v2 verify_subject_alt_name field
tyxia May 27, 2021
2fceeb6
Merge branch 'main' of https://github.com/envoyproxy/envoy into v2_re…
tyxia Jun 15, 2021
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
5 changes: 0 additions & 5 deletions envoy/ssl/certificate_validation_context_config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,6 @@ class CertificateValidationContextConfig {
*/
virtual const std::string& certificateRevocationListPath() const PURE;

/**
* @return The subject alt names to be verified, if enabled.
*/
virtual const std::vector<std::string>& verifySubjectAltNameList() const PURE;

/**
* @return The subject alt name matchers to be verified, if enabled.
*/
Expand Down
5 changes: 1 addition & 4 deletions source/common/ssl/certificate_validation_context_config_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,6 @@ CertificateValidationContextConfigImpl::CertificateValidationContextConfigImpl(
certificate_revocation_list_path_(
Config::DataSource::getPath(config.crl())
.value_or(certificate_revocation_list_.empty() ? EMPTY_STRING : INLINE_STRING)),
verify_subject_alt_name_list_(
config.hidden_envoy_deprecated_verify_subject_alt_name().begin(),
config.hidden_envoy_deprecated_verify_subject_alt_name().end()),
subject_alt_name_matchers_(config.match_subject_alt_names().begin(),
config.match_subject_alt_names().end()),
verify_certificate_hash_list_(config.verify_certificate_hash().begin(),
Expand All @@ -44,7 +41,7 @@ CertificateValidationContextConfigImpl::CertificateValidationContextConfigImpl(
throw EnvoyException(fmt::format("Failed to load CRL from {} without trusted CA",
certificateRevocationListPath()));
}
if (!subject_alt_name_matchers_.empty() || !verify_subject_alt_name_list_.empty()) {
if (!subject_alt_name_matchers_.empty()) {
throw EnvoyException("SAN-based verification of peer certificates without "
"trusted CA is insecure and not allowed");
}
Expand Down
4 changes: 0 additions & 4 deletions source/common/ssl/certificate_validation_context_config_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,6 @@ class CertificateValidationContextConfigImpl : public CertificateValidationConte
const std::string& certificateRevocationListPath() const final {
return certificate_revocation_list_path_;
}
const std::vector<std::string>& verifySubjectAltNameList() const override {
return verify_subject_alt_name_list_;
}
const std::vector<envoy::type::matcher::v3::StringMatcher>&
subjectAltNameMatchers() const override {
return subject_alt_name_matchers_;
Expand Down Expand Up @@ -56,7 +53,6 @@ class CertificateValidationContextConfigImpl : public CertificateValidationConte
const std::string ca_cert_path_;
const std::string certificate_revocation_list_;
const std::string certificate_revocation_list_path_;
const std::vector<std::string> verify_subject_alt_name_list_;
const std::vector<envoy::type::matcher::v3::StringMatcher> subject_alt_name_matchers_;
const std::vector<std::string> verify_certificate_hash_list_;
const std::vector<std::string> verify_certificate_spki_list_;
Expand Down
10 changes: 2 additions & 8 deletions source/extensions/clusters/dynamic_forward_proxy/cluster.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,8 @@ Cluster::Cluster(
// support these parameters dynamically in the future. This is not an exhaustive list of
// parameters that don't make sense but should be the most obvious ones that a user might set
// in error.
if (!cluster.hidden_envoy_deprecated_tls_context().sni().empty() ||
!cluster.hidden_envoy_deprecated_tls_context()
.common_tls_context()
.validation_context()
.hidden_envoy_deprecated_verify_subject_alt_name()
.empty()) {
throw EnvoyException(
"dynamic_forward_proxy cluster cannot configure 'sni' or 'verify_subject_alt_name'");
if (!cluster.hidden_envoy_deprecated_tls_context().sni().empty()) {
throw EnvoyException("dynamic_forward_proxy cluster cannot configure 'sni'");
}
}

Expand Down
24 changes: 6 additions & 18 deletions source/extensions/transport_sockets/tls/cert_validator/default_validator.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,11 +143,6 @@ int DefaultCertValidator::initializeSslContexts(std::vector<SSL_CTX*> contexts,

const Envoy::Ssl::CertificateValidationContextConfig* cert_validation_config = config_;
if (cert_validation_config != nullptr) {
if (!cert_validation_config->verifySubjectAltNameList().empty()) {
verify_subject_alt_name_list_ = cert_validation_config->verifySubjectAltNameList();
verify_mode = verify_mode_validation_context;
}

if (!cert_validation_config->subjectAltNameMatchers().empty()) {
for (const envoy::type::matcher::v3::StringMatcher& matcher :
cert_validation_config->subjectAltNameMatchers()) {
Expand Down Expand Up @@ -204,13 +199,12 @@ int DefaultCertValidator::doVerifyCertChain(
}
}

Envoy::Ssl::ClientValidationStatus validated = verifyCertificate(
&leaf_cert,
transport_socket_options &&
!transport_socket_options->verifySubjectAltNameListOverride().empty()
? transport_socket_options->verifySubjectAltNameListOverride()
: verify_subject_alt_name_list_,
subject_alt_name_matchers_);
Envoy::Ssl::ClientValidationStatus validated =
verifyCertificate(&leaf_cert,
transport_socket_options != nullptr
? transport_socket_options->verifySubjectAltNameListOverride()
: std::vector<std::string>{},
Comment on lines +204 to +206
Copy link
Copy Markdown
Member Author

@tyxia tyxia Jun 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one thing to discuss with reviewers here:
This function arg becomes optional after my change.
Other than current code (creating an empty vector), there are also some other options 1) absl::optional 2) pointer and using nullptr to indicate "not exist".
I currently choose an empty vector approach because 1)it is a lightweight approach from implementation perspective 2) there is a !verify_san_list.empty() check inside verifyCertificate(), which basically provides the same functionality as absl::optional's has_value. And this is better because absl::optional doesn't support pass by const ref and will cause a copy.

But if this path is performance critical, using pointer/nullptr might be a slightly preferred approach. Please let me know what do you think. Thanks!

Copy link
Copy Markdown
Member

@htuch htuch Jun 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine, thanks!

subject_alt_name_matchers_);

if (ssl_extended_info) {
if (ssl_extended_info->certificateValidationStatus() ==
Expand Down Expand Up @@ -381,12 +375,6 @@ void DefaultCertValidator::updateDigestForSessionId(bssl::ScopedEVP_MD_CTX& md,

rc = EVP_DigestUpdate(md.get(), hash_buffer, hash_length);
RELEASE_ASSERT(rc == 1, Utility::getLastCryptoError().value_or(""));

// verify_subject_alt_name_list_ can only be set with a ca_cert
for (const std::string& name : verify_subject_alt_name_list_) {
rc = EVP_DigestUpdate(md.get(), name.data(), name.size());
RELEASE_ASSERT(rc == 1, Utility::getLastCryptoError().value_or(""));
}
}

for (const auto& hash : verify_certificate_hash_list_) {
Expand Down
1 change: 0 additions & 1 deletion source/extensions/transport_sockets/tls/cert_validator/default_validator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,6 @@ class DefaultCertValidator : public CertValidator {
std::vector<Matchers::StringMatcherImpl> subject_alt_name_matchers_;
std::vector<std::vector<uint8_t>> verify_certificate_hash_list_;
std::vector<std::vector<uint8_t>> verify_certificate_spki_list_;
std::vector<std::string> verify_subject_alt_name_list_;
bool verify_trusted_ca_{false};
};

Expand Down
2 changes: 0 additions & 2 deletions test/common/quic/envoy_quic_proof_source_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,6 @@ class TestGetProofCallback : public quic::ProofSource::Callback {
ON_CALL(cert_validation_ctx_config_, certificateRevocationListPath())
.WillByDefault(ReturnRef(path_string));
const std::vector<std::string> empty_string_list;
ON_CALL(cert_validation_ctx_config_, verifySubjectAltNameList())
.WillByDefault(ReturnRef(empty_string_list));
const std::vector<envoy::type::matcher::v3::StringMatcher> san_matchers;
ON_CALL(cert_validation_ctx_config_, subjectAltNameMatchers())
.WillByDefault(ReturnRef(san_matchers));
Expand Down
2 changes: 0 additions & 2 deletions test/common/quic/envoy_quic_proof_verifier_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,6 @@ class EnvoyQuicProofVerifierTest : public testing::Test {
.WillRepeatedly(ReturnRef(empty_string_));
EXPECT_CALL(cert_validation_ctx_config_, certificateRevocationListPath())
.WillRepeatedly(ReturnRef(path_string_));
EXPECT_CALL(cert_validation_ctx_config_, verifySubjectAltNameList())
.WillRepeatedly(ReturnRef(empty_string_list_));
EXPECT_CALL(cert_validation_ctx_config_, subjectAltNameMatchers())
.WillRepeatedly(ReturnRef(san_matchers_));
EXPECT_CALL(cert_validation_ctx_config_, verifyCertificateHashList())
Expand Down
30 changes: 2 additions & 28 deletions test/extensions/clusters/dynamic_forward_proxy/cluster_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,34 +254,8 @@ connect_timeout: 0.25s
filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ca_cert.pem"
)EOF");

EXPECT_THROW_WITH_MESSAGE(
createCluster(yaml_config, false), EnvoyException,
"dynamic_forward_proxy cluster cannot configure 'sni' or 'verify_subject_alt_name'");
}

// Verify that using 'verify_subject_alt_name' causes a failure.
TEST_F(ClusterFactoryTest, DEPRECATED_FEATURE_TEST(InvalidVerifySubjectAltName)) {
TestDeprecatedV2Api _deprecated_v2_api;
const std::string yaml_config = TestEnvironment::substitute(R"EOF(
name: name
connect_timeout: 0.25s
cluster_type:
name: dynamic_forward_proxy
typed_config:
"@type": type.googleapis.com/envoy.config.cluster.dynamic_forward_proxy.v2alpha.ClusterConfig
dns_cache_config:
name: foo
tls_context:
common_tls_context:
validation_context:
trusted_ca:
filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ca_cert.pem"
verify_subject_alt_name: [api.lyft.com]
)EOF");

EXPECT_THROW_WITH_MESSAGE(
createCluster(yaml_config, false), EnvoyException,
"dynamic_forward_proxy cluster cannot configure 'sni' or 'verify_subject_alt_name'");
EXPECT_THROW_WITH_MESSAGE(createCluster(yaml_config, false), EnvoyException,
"dynamic_forward_proxy cluster cannot configure 'sni'");
}

TEST_F(ClusterFactoryTest, InvalidUpstreamHttpProtocolOptions) {
Expand Down
3 changes: 0 additions & 3 deletions test/extensions/transport_sockets/tls/cert_validator/test_common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,6 @@ class TestCertificateValidationContextConfig
const std::string& certificateRevocationListPath() const final {
CONSTRUCT_ON_FIRST_USE(std::string, "");
}
const std::vector<std::string>& verifySubjectAltNameList() const override {
CONSTRUCT_ON_FIRST_USE(std::vector<std::string>, {});
}
const std::vector<envoy::type::matcher::v3::StringMatcher>&
subjectAltNameMatchers() const override {
return san_matchers_;
Expand Down
1 change: 0 additions & 1 deletion test/mocks/ssl/mocks.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,6 @@ class MockCertificateValidationContextConfig : public CertificateValidationConte
MOCK_METHOD(const std::string&, caCertPath, (), (const));
MOCK_METHOD(const std::string&, certificateRevocationList, (), (const));
MOCK_METHOD(const std::string&, certificateRevocationListPath, (), (const));
MOCK_METHOD(const std::vector<std::string>&, verifySubjectAltNameList, (), (const));
MOCK_METHOD(const std::vector<envoy::type::matcher::v3::StringMatcher>&, subjectAltNameMatchers,
(), (const));
MOCK_METHOD(const std::vector<std::string>&, verifyCertificateHashList, (), (const));
Expand Down