From 56e8c45b1b340c4a4f8f02ec2488354c31806d59 Mon Sep 17 00:00:00 2001 From: Rei Shimizu Date: Wed, 3 Nov 2021 06:01:52 +0900 Subject: [PATCH] tls: allow cert validation by only leaf trusted CA's CRL (#18289) Commit Message: Allow cert validation by only leaf trusted CAs CRL Additional Description: Close #18268. In the previous implementation, we don't have availability to validate certs when all trusted CAs don't have their own CRLs if any trusted CAs have that. This feature allows validating even if all trusted CAs don't have CRLs. Risk Level: Low Testing: Unit Docs Changes: Required Release Notes: Required Signed-off-by: Shikugawa --- .../transport_sockets/tls/v3/common.proto | 13 ++- docs/root/version_history/current.rst | 1 + .../certificate_validation_context_config.h | 5 ++ ...tificate_validation_context_config_impl.cc | 2 +- ...rtificate_validation_context_config_impl.h | 3 + .../tls/cert_validator/default_validator.cc | 9 ++- .../tls/cert_validator/test_common.h | 1 + .../transport_sockets/tls/ssl_socket_test.cc | 79 +++++++++++++++++++ test/mocks/ssl/mocks.h | 1 + 9 files changed, 108 insertions(+), 6 deletions(-) diff --git a/api/envoy/extensions/transport_sockets/tls/v3/common.proto b/api/envoy/extensions/transport_sockets/tls/v3/common.proto index 82dcb37cd7ca..ab88abcbe2ce 100644 --- a/api/envoy/extensions/transport_sockets/tls/v3/common.proto +++ b/api/envoy/extensions/transport_sockets/tls/v3/common.proto @@ -253,7 +253,7 @@ message CertificateProviderPluginInstance { string certificate_name = 2; } -// [#next-free-field: 14] +// [#next-free-field: 15] message CertificateValidationContext { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.auth.CertificateValidationContext"; @@ -292,6 +292,9 @@ message CertificateValidationContext { // that if a CRL is provided for any certificate authority in a trust chain, a CRL must be // provided for all certificate authorities in that chain. Failure to do so will result in // verification failure for both revoked and unrevoked certificates from that chain. + // The behavior of requiring all certificates to contain CRLs if any do can be altered by + // setting :ref:`only_verify_leaf_cert_crl ` + // true. If set to true, only the final certificate in the chain undergoes CRL verification. // // See :ref:`the TLS overview ` for a list of common // system CA locations. @@ -417,7 +420,9 @@ message CertificateValidationContext { // for any certificate authority in a trust chain, a CRL must be provided // for all certificate authorities in that chain. Failure to do so will // result in verification failure for both revoked and unrevoked certificates - // from that chain. + // from that chain. This default behavior can be altered by setting + // :ref:`only_verify_leaf_cert_crl ` to + // true. config.core.v3.DataSource crl = 7; // If specified, Envoy will not reject expired certificates. @@ -433,4 +438,8 @@ message CertificateValidationContext { // Refer to the documentation for the specified validator. If you do not want a custom validation algorithm, do not set this field. // [#extension-category: envoy.tls.cert_validator] config.core.v3.TypedExtensionConfig custom_validator_config = 12; + + // If this option is set to true, only the certificate at the end of the + // certificate chain will be subject to validation by :ref:`CRL `. + bool only_verify_leaf_cert_crl = 14; } diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 51da2dda37cf..354ef9637ae3 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -67,6 +67,7 @@ New Features * thrift_proxy: add upstream metrics to show decoding errors and whether exception is from local or remote, e.g. ``cluster.cluster_name.thrift.upstream_resp_exception_remote``. * thrift_proxy: add host level success/error metrics where success is a reply of type success and error is any other response to a call. * thrift_proxy: support subset lb when using request or route metadata. +* tls: added support for only verifying the leaf CRL in the certificate chain with :ref:`only_verify_leaf_cert_crl `. * transport_socket: added :ref:`envoy.transport_sockets.tcp_stats ` which generates additional statistics gathered from the OS TCP stack. * udp: add support for multiple listener filters. * upstream: added the ability to :ref:`configure max connection duration ` for upstream clusters. diff --git a/envoy/ssl/certificate_validation_context_config.h b/envoy/ssl/certificate_validation_context_config.h index 544215a73dae..d8a1bf6b0af2 100644 --- a/envoy/ssl/certificate_validation_context_config.h +++ b/envoy/ssl/certificate_validation_context_config.h @@ -78,6 +78,11 @@ class CertificateValidationContextConfig { * @return a reference to the api object. */ virtual Api::Api& api() const PURE; + + /** + * @return whether to validate certificate chain with all CRL or not. + */ + virtual bool onlyVerifyLeafCertificateCrl() const PURE; }; using CertificateValidationContextConfigPtr = std::unique_ptr; diff --git a/source/common/ssl/certificate_validation_context_config_impl.cc b/source/common/ssl/certificate_validation_context_config_impl.cc index 40dc20f6ef3a..564325c3fb66 100644 --- a/source/common/ssl/certificate_validation_context_config_impl.cc +++ b/source/common/ssl/certificate_validation_context_config_impl.cc @@ -35,7 +35,7 @@ CertificateValidationContextConfigImpl::CertificateValidationContextConfigImpl( ? absl::make_optional( config.custom_validator_config()) : absl::nullopt), - api_(api) { + api_(api), only_verify_leaf_cert_crl_(config.only_verify_leaf_cert_crl()) { if (ca_cert_.empty() && custom_validator_config_ == absl::nullopt) { if (!certificate_revocation_list_.empty()) { throw EnvoyException(fmt::format("Failed to load CRL from {} without trusted CA", diff --git a/source/common/ssl/certificate_validation_context_config_impl.h b/source/common/ssl/certificate_validation_context_config_impl.h index 7cf045a35184..038abe08b1e9 100644 --- a/source/common/ssl/certificate_validation_context_config_impl.h +++ b/source/common/ssl/certificate_validation_context_config_impl.h @@ -48,6 +48,8 @@ class CertificateValidationContextConfigImpl : public CertificateValidationConte Api::Api& api() const override { return api_; } + bool onlyVerifyLeafCertificateCrl() const override { return only_verify_leaf_cert_crl_; } + private: const std::string ca_cert_; const std::string ca_cert_path_; @@ -61,6 +63,7 @@ class CertificateValidationContextConfigImpl : public CertificateValidationConte TrustChainVerification trust_chain_verification_; const absl::optional custom_validator_config_; Api::Api& api_; + const bool only_verify_leaf_cert_crl_; }; } // namespace Ssl diff --git a/source/extensions/transport_sockets/tls/cert_validator/default_validator.cc b/source/extensions/transport_sockets/tls/cert_validator/default_validator.cc index 691b5018189d..e638b98b4284 100644 --- a/source/extensions/transport_sockets/tls/cert_validator/default_validator.cc +++ b/source/extensions/transport_sockets/tls/cert_validator/default_validator.cc @@ -100,7 +100,9 @@ int DefaultCertValidator::initializeSslContexts(std::vector contexts, absl::StrCat("Failed to load trusted CA certificates from ", config_->caCertPath())); } if (has_crl) { - X509_STORE_set_flags(store, X509_V_FLAG_CRL_CHECK | X509_V_FLAG_CRL_CHECK_ALL); + X509_STORE_set_flags(store, config_->onlyVerifyLeafCertificateCrl() + ? X509_V_FLAG_CRL_CHECK + : X509_V_FLAG_CRL_CHECK | X509_V_FLAG_CRL_CHECK_ALL); } verify_mode = SSL_VERIFY_PEER; verify_trusted_ca_ = true; @@ -136,8 +138,9 @@ int DefaultCertValidator::initializeSslContexts(std::vector contexts, X509_STORE_add_crl(store, item->crl); } } - - X509_STORE_set_flags(store, X509_V_FLAG_CRL_CHECK | X509_V_FLAG_CRL_CHECK_ALL); + X509_STORE_set_flags(store, config_->onlyVerifyLeafCertificateCrl() + ? X509_V_FLAG_CRL_CHECK + : X509_V_FLAG_CRL_CHECK | X509_V_FLAG_CRL_CHECK_ALL); } } diff --git a/test/extensions/transport_sockets/tls/cert_validator/test_common.h b/test/extensions/transport_sockets/tls/cert_validator/test_common.h index b958f1727207..11d4022cf043 100644 --- a/test/extensions/transport_sockets/tls/cert_validator/test_common.h +++ b/test/extensions/transport_sockets/tls/cert_validator/test_common.h @@ -72,6 +72,7 @@ class TestCertificateValidationContextConfig } Api::Api& api() const override { return *api_; } + bool onlyVerifyLeafCertificateCrl() const override { return false; } private: bool allow_expired_certificate_{false}; diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc index 24b1952640a5..4685f03829a4 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -4610,6 +4610,85 @@ TEST_P(SslSocketTest, RevokedIntermediateCertificateCRLInTrustedCA) { testUtil(complete_unrevoked_test_options.setExpectedSerialNumber(TEST_SAN_DNS4_CERT_SERIAL)); } +TEST_P(SslSocketTest, NotRevokedLeafCertificateOnlyLeafCRLValidation) { + // The test checks that revoked certificate will makes the validation success even if we set + // only_verify_leaf_cert_crl to true. + // + // Trust chain contains: + // - Root authority certificate (i.e., ca_cert.pem) + // - Intermediate authority certificate (i.e., intermediate_ca_cert.pem) + // - Intermediate authority certificate revocation list (i.e., intermediate_ca_cert.crl) + // + // Trust chain omits (But this test will succeed): + // - Root authority certificate revocation list (i.e., ca_cert.crl) + const std::string incomplete_server_ctx_yaml = R"EOF( + common_tls_context: + tls_certificates: + certificate_chain: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_cert.pem" + private_key: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_key.pem" + validation_context: + trusted_ca: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/intermediate_ca_cert_chain_with_crl.pem" + only_verify_leaf_cert_crl: true +)EOF"; + + // This should succeed, since the certificate has not been revoked. + const std::string unrevoked_client_ctx_yaml = R"EOF( + common_tls_context: + tls_certificates: + certificate_chain: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/san_dns4_cert.pem" + private_key: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/san_dns4_key.pem" +)EOF"; + + TestUtilOptions complete_unrevoked_test_options(unrevoked_client_ctx_yaml, + incomplete_server_ctx_yaml, true, GetParam()); + testUtil(complete_unrevoked_test_options.setExpectedSerialNumber(TEST_SAN_DNS4_CERT_SERIAL)); +} + +TEST_P(SslSocketTest, RevokedLeafCertificateOnlyLeafCRLValidation) { + // The test checks that revoked certificate will makes the validation fails even if we set + // only_verify_leaf_cert_crl to true. + // + // Trust chain contains: + // - Root authority certificate (i.e., ca_cert.pem) + // - Intermediate authority certificate (i.e., intermediate_ca_cert.pem) + // - Intermediate authority certificate revocation list (i.e., intermediate_ca_cert.crl) + // + // Trust chain omits (But this test will succeed): + // - Root authority certificate revocation list (i.e., ca_cert.crl) + const std::string incomplete_server_ctx_yaml = R"EOF( + common_tls_context: + tls_certificates: + certificate_chain: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_cert.pem" + private_key: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_key.pem" + validation_context: + trusted_ca: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/intermediate_ca_cert_chain_with_crl.pem" + only_verify_leaf_cert_crl: true +)EOF"; + + // This should fail, since the certificate has been revoked. + const std::string revoked_client_ctx_yaml = R"EOF( + common_tls_context: + tls_certificates: + certificate_chain: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/san_dns3_cert.pem" + private_key: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/san_dns3_key.pem" +)EOF"; + + TestUtilOptions complete_revoked_test_options(revoked_client_ctx_yaml, incomplete_server_ctx_yaml, + false, GetParam()); + testUtil(complete_revoked_test_options.setExpectedServerStats("ssl.fail_verify_error") + .setExpectedVerifyErrorCode(X509_V_ERR_CERT_REVOKED)); +} + TEST_P(SslSocketTest, GetRequestedServerName) { envoy::config::listener::v3::Listener listener; envoy::config::listener::v3::FilterChain* filter_chain = listener.add_filter_chains(); diff --git a/test/mocks/ssl/mocks.h b/test/mocks/ssl/mocks.h index 43ad275fce27..ae545d126d55 100644 --- a/test/mocks/ssl/mocks.h +++ b/test/mocks/ssl/mocks.h @@ -167,6 +167,7 @@ class MockCertificateValidationContextConfig : public CertificateValidationConte MOCK_METHOD(envoy::extensions::transport_sockets::tls::v3::CertificateValidationContext:: TrustChainVerification, trustChainVerification, (), (const)); + MOCK_METHOD(bool, onlyVerifyLeafCertificateCrl, (), (const)); }; class MockPrivateKeyMethodManager : public PrivateKeyMethodManager {