From 5faafc3fc4761ebd4e1b98fa3df72a1768745867 Mon Sep 17 00:00:00 2001 From: Shikugawa Date: Tue, 28 Sep 2021 10:53:16 +0000 Subject: [PATCH 01/11] tls: allow cert validation by only leaf trusted CA's CRL Signed-off-by: Shikugawa --- .../transport_sockets/tls/v3/common.proto | 7 ++++- docs/root/version_history/current.rst | 1 + .../certificate_validation_context_config.h | 5 ++++ ...tificate_validation_context_config_impl.cc | 3 +- ...rtificate_validation_context_config_impl.h | 3 ++ .../tls/cert_validator/default_validator.cc | 9 ++++-- .../transport_sockets/tls/ssl_socket_test.cc | 28 +++++++++++++++++++ 7 files changed, 51 insertions(+), 5 deletions(-) diff --git a/api/envoy/extensions/transport_sockets/tls/v3/common.proto b/api/envoy/extensions/transport_sockets/tls/v3/common.proto index 82dcb37cd7ca0..1c940ddf200ae 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,7 @@ 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. + // This behavior is disabled by configuring :ref:`crl_verify_all ` false. // // See :ref:`the TLS overview ` for a list of common // system CA locations. @@ -433,4 +434,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; + + // This option is true by default. If false, only the certificate at the end of the + // certificate chain will be subject to validation by CRL. + google.protobuf.BoolValue crl_verify_all = 14; } diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 8a3a2d6a1f08c..443f350ee6485 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -131,6 +131,7 @@ New Features * router: added an optional :ref:`override_auto_sni_header ` to support setting SNI value from an arbitrary header other than host/authority. * sxg_filter: added filter to transform response to SXG package to :ref:`contrib images `. This can be enabled by setting :ref:`SXG ` configuration. * thrift_proxy: added support for :ref:`mirroring requests `. +* tls: added support for allowing that all trust CA doesn't have to have CRL even if any trust CA has CRL with :ref:`crl_verify_all ` * udp: allows updating filter chain in-place through LDS, which is supported by Quic listener. Such listener config will be rejected in other connection-less UDP listener implementations. It can be reverted by ``envoy.reloadable_features.udp_listener_updates_filter_chain_in_place``. * udp: disallow L4 filter chain in config which configures connection-less UDP listener. It can be reverted by ``envoy.reloadable_features.udp_listener_updates_filter_chain_in_place``. diff --git a/envoy/ssl/certificate_validation_context_config.h b/envoy/ssl/certificate_validation_context_config.h index 544215a73daea..8e92411900375 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 crlVerifyAll() 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 40dc20f6ef3a3..3db2acf795c7e 100644 --- a/source/common/ssl/certificate_validation_context_config_impl.cc +++ b/source/common/ssl/certificate_validation_context_config_impl.cc @@ -35,7 +35,8 @@ CertificateValidationContextConfigImpl::CertificateValidationContextConfigImpl( ? absl::make_optional( config.custom_validator_config()) : absl::nullopt), - api_(api) { + api_(api), + crl_verify_all_(!config.has_crl_verify_all() ? true : config.crl_verify_all().value()) { 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 7cf045a351841..4227b7f42d6f0 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 crlVerifyAll() const override { return crl_verify_all_; } + 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 crl_verify_all_; }; } // 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 6cd624f2e38fa..9314673458a6f 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_->crlVerifyAll() + ? X509_V_FLAG_CRL_CHECK | X509_V_FLAG_CRL_CHECK_ALL + : X509_V_FLAG_CRL_CHECK); } 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_->crlVerifyAll() + ? X509_V_FLAG_CRL_CHECK | X509_V_FLAG_CRL_CHECK_ALL + : X509_V_FLAG_CRL_CHECK); } } diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc index 24b2e1422c7ee..1f7e94416ef09 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -4584,6 +4584,34 @@ TEST_P(SslSocketTest, RevokedIntermediateCertificateCRLInTrustedCA) { testUtil(complete_unrevoked_test_options.setExpectedSerialNumber(TEST_SAN_DNS4_CERT_SERIAL)); } +TEST_P(SslSocketTest, RevokedLeafCertificateOnlyLeafCRLValidation) { + 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" + crl_verify_all: false +)EOF"; + + 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, GetRequestedServerName) { envoy::config::listener::v3::Listener listener; envoy::config::listener::v3::FilterChain* filter_chain = listener.add_filter_chains(); From 3d4f9d72d3ea4c0e18e037211d05aa1006952d4e Mon Sep 17 00:00:00 2001 From: Shikugawa Date: Wed, 29 Sep 2021 02:21:27 +0000 Subject: [PATCH 02/11] fix Signed-off-by: Shikugawa --- .../transport_sockets/tls/v3/common.proto | 12 +++-- docs/root/version_history/current.rst | 2 +- .../certificate_validation_context_config.h | 2 +- ...tificate_validation_context_config_impl.cc | 4 +- ...rtificate_validation_context_config_impl.h | 4 +- .../tls/cert_validator/default_validator.cc | 12 ++--- .../transport_sockets/tls/ssl_socket_test.cc | 54 ++++++++++++++++++- 7 files changed, 73 insertions(+), 17 deletions(-) diff --git a/api/envoy/extensions/transport_sockets/tls/v3/common.proto b/api/envoy/extensions/transport_sockets/tls/v3/common.proto index 1c940ddf200ae..350111b74d54a 100644 --- a/api/envoy/extensions/transport_sockets/tls/v3/common.proto +++ b/api/envoy/extensions/transport_sockets/tls/v3/common.proto @@ -292,7 +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. - // This behavior is disabled by configuring :ref:`crl_verify_all ` false. + // The behavior of requiring all certificates to contain CRLs if any do can be altered by + // setting :ref:`only_verify_final_certificate_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. @@ -418,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_final_certificate_crl ` + // true. config.core.v3.DataSource crl = 7; // If specified, Envoy will not reject expired certificates. @@ -435,7 +439,7 @@ message CertificateValidationContext { // [#extension-category: envoy.tls.cert_validator] config.core.v3.TypedExtensionConfig custom_validator_config = 12; - // This option is true by default. If false, only the certificate at the end of the + // This option is false by default. If true, only the certificate at the end of the // certificate chain will be subject to validation by CRL. - google.protobuf.BoolValue crl_verify_all = 14; + google.protobuf.BoolValue only_verify_final_certificate_crl = 14; } diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 443f350ee6485..7f3adaa1395b2 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -131,7 +131,7 @@ New Features * router: added an optional :ref:`override_auto_sni_header ` to support setting SNI value from an arbitrary header other than host/authority. * sxg_filter: added filter to transform response to SXG package to :ref:`contrib images `. This can be enabled by setting :ref:`SXG ` configuration. * thrift_proxy: added support for :ref:`mirroring requests `. -* tls: added support for allowing that all trust CA doesn't have to have CRL even if any trust CA has CRL with :ref:`crl_verify_all ` +* tls: added support for allowing that all trust CA doesn't have to have CRL even if any trust CA has CRL with :ref:`only_verify_final_certificate_crl `. * udp: allows updating filter chain in-place through LDS, which is supported by Quic listener. Such listener config will be rejected in other connection-less UDP listener implementations. It can be reverted by ``envoy.reloadable_features.udp_listener_updates_filter_chain_in_place``. * udp: disallow L4 filter chain in config which configures connection-less UDP listener. It can be reverted by ``envoy.reloadable_features.udp_listener_updates_filter_chain_in_place``. diff --git a/envoy/ssl/certificate_validation_context_config.h b/envoy/ssl/certificate_validation_context_config.h index 8e92411900375..61ba5d5684db2 100644 --- a/envoy/ssl/certificate_validation_context_config.h +++ b/envoy/ssl/certificate_validation_context_config.h @@ -82,7 +82,7 @@ class CertificateValidationContextConfig { /** * @return whether to validate certificate chain with all CRL or not. */ - virtual bool crlVerifyAll() const PURE; + virtual bool onlyVerifyFinalCertificateCrl() 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 3db2acf795c7e..065da02079dfb 100644 --- a/source/common/ssl/certificate_validation_context_config_impl.cc +++ b/source/common/ssl/certificate_validation_context_config_impl.cc @@ -36,7 +36,9 @@ CertificateValidationContextConfigImpl::CertificateValidationContextConfigImpl( config.custom_validator_config()) : absl::nullopt), api_(api), - crl_verify_all_(!config.has_crl_verify_all() ? true : config.crl_verify_all().value()) { + only_verify_final_certificate_crl_(!config.has_only_verify_final_certificate_crl() + ? false + : config.only_verify_final_certificate_crl().value()) { 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 4227b7f42d6f0..8815c8571440d 100644 --- a/source/common/ssl/certificate_validation_context_config_impl.h +++ b/source/common/ssl/certificate_validation_context_config_impl.h @@ -48,7 +48,7 @@ class CertificateValidationContextConfigImpl : public CertificateValidationConte Api::Api& api() const override { return api_; } - bool crlVerifyAll() const override { return crl_verify_all_; } + bool onlyVerifyFinalCertificateCrl() const override { return only_verify_final_certificate_crl_; } private: const std::string ca_cert_; @@ -63,7 +63,7 @@ class CertificateValidationContextConfigImpl : public CertificateValidationConte TrustChainVerification trust_chain_verification_; const absl::optional custom_validator_config_; Api::Api& api_; - const bool crl_verify_all_; + const bool only_verify_final_certificate_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 9314673458a6f..32615faec1a8b 100644 --- a/source/extensions/transport_sockets/tls/cert_validator/default_validator.cc +++ b/source/extensions/transport_sockets/tls/cert_validator/default_validator.cc @@ -100,9 +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, config_->crlVerifyAll() - ? X509_V_FLAG_CRL_CHECK | X509_V_FLAG_CRL_CHECK_ALL - : X509_V_FLAG_CRL_CHECK); + X509_STORE_set_flags(store, config_->onlyVerifyFinalCertificateCrl() + ? 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; @@ -138,9 +138,9 @@ int DefaultCertValidator::initializeSslContexts(std::vector contexts, X509_STORE_add_crl(store, item->crl); } } - X509_STORE_set_flags(store, config_->crlVerifyAll() - ? X509_V_FLAG_CRL_CHECK | X509_V_FLAG_CRL_CHECK_ALL - : X509_V_FLAG_CRL_CHECK); + X509_STORE_set_flags(store, config_->onlyVerifyFinalCertificateCrl() + ? X509_V_FLAG_CRL_CHECK + : X509_V_FLAG_CRL_CHECK | X509_V_FLAG_CRL_CHECK_ALL); } } diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc index 1f7e94416ef09..aba8f6b8e801c 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -4584,7 +4584,17 @@ TEST_P(SslSocketTest, RevokedIntermediateCertificateCRLInTrustedCA) { testUtil(complete_unrevoked_test_options.setExpectedSerialNumber(TEST_SAN_DNS4_CERT_SERIAL)); } -TEST_P(SslSocketTest, RevokedLeafCertificateOnlyLeafCRLValidation) { +TEST_P(SslSocketTest, NotRevokedLeafCertificateOnlyLeafCRLValidation) { + // This should succeed, since the crl chain is incomplete but only_verify_final_certificate_crl is + // configured 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: @@ -4595,9 +4605,10 @@ TEST_P(SslSocketTest, RevokedLeafCertificateOnlyLeafCRLValidation) { validation_context: trusted_ca: filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/intermediate_ca_cert_chain_with_crl.pem" - crl_verify_all: false + only_verify_final_certificate_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: @@ -4612,6 +4623,45 @@ TEST_P(SslSocketTest, RevokedLeafCertificateOnlyLeafCRLValidation) { testUtil(complete_unrevoked_test_options.setExpectedSerialNumber(TEST_SAN_DNS4_CERT_SERIAL)); } +TEST_P(SslSocketTest, RevokedLeafCertificateOnlyLeafCRLValidation) { + // This should succeed, since the crl chain is incomplete but only_verify_final_certificate_crl is + // configured 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_final_certificate_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")); +} + TEST_P(SslSocketTest, GetRequestedServerName) { envoy::config::listener::v3::Listener listener; envoy::config::listener::v3::FilterChain* filter_chain = listener.add_filter_chains(); From 0fd74a0fedd45b16c77b3e9ebbe96f79142910db Mon Sep 17 00:00:00 2001 From: Shikugawa Date: Wed, 29 Sep 2021 03:24:34 +0000 Subject: [PATCH 03/11] fix Signed-off-by: Shikugawa --- .../transport_sockets/tls/cert_validator/test_common.h | 1 + 1 file changed, 1 insertion(+) 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 b958f17272075..1f95f4ced37db 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 onlyVerifyFinalCertificateCrl() const override { return false; } private: bool allow_expired_certificate_{false}; From faf39f1e4f23fc4e92d811b24e71fe6ebddc395b Mon Sep 17 00:00:00 2001 From: Shikugawa Date: Wed, 29 Sep 2021 08:41:33 +0000 Subject: [PATCH 04/11] fix Signed-off-by: Shikugawa --- test/mocks/ssl/mocks.h | 1 + 1 file changed, 1 insertion(+) diff --git a/test/mocks/ssl/mocks.h b/test/mocks/ssl/mocks.h index 43ad275fce277..a92d41c7462ba 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, onlyVerifyFinalCertificateCrl, (), (const)); }; class MockPrivateKeyMethodManager : public PrivateKeyMethodManager { From dc9081c93c521a2500eb7b360c800923563a8ad1 Mon Sep 17 00:00:00 2001 From: Shikugawa Date: Thu, 30 Sep 2021 02:15:20 +0000 Subject: [PATCH 05/11] fix Signed-off-by: Shikugawa --- .../extensions/transport_sockets/tls/v3/common.proto | 8 ++++---- docs/root/version_history/current.rst | 2 +- .../ssl/certificate_validation_context_config_impl.cc | 5 +---- test/extensions/transport_sockets/tls/ssl_socket_test.cc | 8 ++++---- 4 files changed, 10 insertions(+), 13 deletions(-) diff --git a/api/envoy/extensions/transport_sockets/tls/v3/common.proto b/api/envoy/extensions/transport_sockets/tls/v3/common.proto index 350111b74d54a..af52a913cf6f6 100644 --- a/api/envoy/extensions/transport_sockets/tls/v3/common.proto +++ b/api/envoy/extensions/transport_sockets/tls/v3/common.proto @@ -421,7 +421,7 @@ message CertificateValidationContext { // 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. This default behavior can be altered by setting - // :ref:`only_verify_final_certificate_crl ` + // :ref:`only_verify_final_certificate_crl ` to // true. config.core.v3.DataSource crl = 7; @@ -439,7 +439,7 @@ message CertificateValidationContext { // [#extension-category: envoy.tls.cert_validator] config.core.v3.TypedExtensionConfig custom_validator_config = 12; - // This option is false by default. If true, only the certificate at the end of the - // certificate chain will be subject to validation by CRL. - google.protobuf.BoolValue only_verify_final_certificate_crl = 14; + // 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_final_certificate_crl = 14; } diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index ddc4e0bd093d4..7fcdd1e1c3baf 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -134,7 +134,7 @@ New Features * router: added an optional :ref:`override_auto_sni_header ` to support setting SNI value from an arbitrary header other than host/authority. * sxg_filter: added filter to transform response to SXG package to :ref:`contrib images `. This can be enabled by setting :ref:`SXG ` configuration. * thrift_proxy: added support for :ref:`mirroring requests `. -* tls: added support for allowing that all trust CA doesn't have to have CRL even if any trust CA has CRL with :ref:`only_verify_final_certificate_crl `. +* tls: added support for only verifying the final CRL in the certificate chain with :ref:`only_verify_final_certificate_crl `. * udp: allows updating filter chain in-place through LDS, which is supported by Quic listener. Such listener config will be rejected in other connection-less UDP listener implementations. It can be reverted by ``envoy.reloadable_features.udp_listener_updates_filter_chain_in_place``. * udp: disallow L4 filter chain in config which configures connection-less UDP listener. It can be reverted by ``envoy.reloadable_features.udp_listener_updates_filter_chain_in_place``. diff --git a/source/common/ssl/certificate_validation_context_config_impl.cc b/source/common/ssl/certificate_validation_context_config_impl.cc index 065da02079dfb..28e26fbbd9bca 100644 --- a/source/common/ssl/certificate_validation_context_config_impl.cc +++ b/source/common/ssl/certificate_validation_context_config_impl.cc @@ -35,10 +35,7 @@ CertificateValidationContextConfigImpl::CertificateValidationContextConfigImpl( ? absl::make_optional( config.custom_validator_config()) : absl::nullopt), - api_(api), - only_verify_final_certificate_crl_(!config.has_only_verify_final_certificate_crl() - ? false - : config.only_verify_final_certificate_crl().value()) { + api_(api), only_verify_final_certificate_crl_(config.only_verify_final_certificate_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/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc index aba8f6b8e801c..53a14e3c3699a 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -4585,8 +4585,8 @@ TEST_P(SslSocketTest, RevokedIntermediateCertificateCRLInTrustedCA) { } TEST_P(SslSocketTest, NotRevokedLeafCertificateOnlyLeafCRLValidation) { - // This should succeed, since the crl chain is incomplete but only_verify_final_certificate_crl is - // configured true. + // The test checks that revoked certificate will makes the validation fails even if we set + // only_verify_final_certificate_crl to true. // // Trust chain contains: // - Root authority certificate (i.e., ca_cert.pem) @@ -4624,8 +4624,8 @@ TEST_P(SslSocketTest, NotRevokedLeafCertificateOnlyLeafCRLValidation) { } TEST_P(SslSocketTest, RevokedLeafCertificateOnlyLeafCRLValidation) { - // This should succeed, since the crl chain is incomplete but only_verify_final_certificate_crl is - // configured true. + // The test checks that revoked certificate will makes the validation fails even if we set + // only_verify_final_certificate_crl to true. // // Trust chain contains: // - Root authority certificate (i.e., ca_cert.pem) From 093305fed5ea1d6d23fa9e4765839a0c2f913f77 Mon Sep 17 00:00:00 2001 From: Shikugawa Date: Fri, 8 Oct 2021 09:33:42 +0000 Subject: [PATCH 06/11] fix Signed-off-by: Shikugawa --- .../extensions/transport_sockets/tls/v3/common.proto | 6 +++--- docs/root/version_history/current.rst | 2 +- .../ssl/certificate_validation_context_config_impl.cc | 2 +- .../ssl/certificate_validation_context_config_impl.h | 4 ++-- test/extensions/transport_sockets/tls/ssl_socket_test.cc | 8 ++++---- 5 files changed, 11 insertions(+), 11 deletions(-) diff --git a/api/envoy/extensions/transport_sockets/tls/v3/common.proto b/api/envoy/extensions/transport_sockets/tls/v3/common.proto index af52a913cf6f6..ab88abcbe2ce6 100644 --- a/api/envoy/extensions/transport_sockets/tls/v3/common.proto +++ b/api/envoy/extensions/transport_sockets/tls/v3/common.proto @@ -293,7 +293,7 @@ message CertificateValidationContext { // 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_final_certificate_crl ` + // 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 @@ -421,7 +421,7 @@ message CertificateValidationContext { // 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. This default behavior can be altered by setting - // :ref:`only_verify_final_certificate_crl ` to + // :ref:`only_verify_leaf_cert_crl ` to // true. config.core.v3.DataSource crl = 7; @@ -441,5 +441,5 @@ message CertificateValidationContext { // 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_final_certificate_crl = 14; + bool only_verify_leaf_cert_crl = 14; } diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 7fcdd1e1c3baf..549ece130838b 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -134,7 +134,7 @@ New Features * router: added an optional :ref:`override_auto_sni_header ` to support setting SNI value from an arbitrary header other than host/authority. * sxg_filter: added filter to transform response to SXG package to :ref:`contrib images `. This can be enabled by setting :ref:`SXG ` configuration. * thrift_proxy: added support for :ref:`mirroring requests `. -* tls: added support for only verifying the final CRL in the certificate chain with :ref:`only_verify_final_certificate_crl `. +* tls: added support for only verifying the final CRL in the certificate chain with :ref:`only_verify_leaf_cert_crl `. * udp: allows updating filter chain in-place through LDS, which is supported by Quic listener. Such listener config will be rejected in other connection-less UDP listener implementations. It can be reverted by ``envoy.reloadable_features.udp_listener_updates_filter_chain_in_place``. * udp: disallow L4 filter chain in config which configures connection-less UDP listener. It can be reverted by ``envoy.reloadable_features.udp_listener_updates_filter_chain_in_place``. diff --git a/source/common/ssl/certificate_validation_context_config_impl.cc b/source/common/ssl/certificate_validation_context_config_impl.cc index 28e26fbbd9bca..564325c3fb662 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), only_verify_final_certificate_crl_(config.only_verify_final_certificate_crl()) { + 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 8815c8571440d..e82dd1cd1aa02 100644 --- a/source/common/ssl/certificate_validation_context_config_impl.h +++ b/source/common/ssl/certificate_validation_context_config_impl.h @@ -48,7 +48,7 @@ class CertificateValidationContextConfigImpl : public CertificateValidationConte Api::Api& api() const override { return api_; } - bool onlyVerifyFinalCertificateCrl() const override { return only_verify_final_certificate_crl_; } + bool onlyVerifyFinalCertificateCrl() const override { return only_verify_leaf_cert_crl_; } private: const std::string ca_cert_; @@ -63,7 +63,7 @@ class CertificateValidationContextConfigImpl : public CertificateValidationConte TrustChainVerification trust_chain_verification_; const absl::optional custom_validator_config_; Api::Api& api_; - const bool only_verify_final_certificate_crl_; + const bool only_verify_leaf_cert_crl_; }; } // namespace Ssl diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc index 53a14e3c3699a..443d109bb8ee1 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -4586,7 +4586,7 @@ TEST_P(SslSocketTest, RevokedIntermediateCertificateCRLInTrustedCA) { TEST_P(SslSocketTest, NotRevokedLeafCertificateOnlyLeafCRLValidation) { // The test checks that revoked certificate will makes the validation fails even if we set - // only_verify_final_certificate_crl to true. + // only_verify_leaf_cert_crl to true. // // Trust chain contains: // - Root authority certificate (i.e., ca_cert.pem) @@ -4605,7 +4605,7 @@ TEST_P(SslSocketTest, NotRevokedLeafCertificateOnlyLeafCRLValidation) { validation_context: trusted_ca: filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/intermediate_ca_cert_chain_with_crl.pem" - only_verify_final_certificate_crl: true + only_verify_leaf_cert_crl: true )EOF"; // This should succeed, since the certificate has not been revoked. @@ -4625,7 +4625,7 @@ TEST_P(SslSocketTest, NotRevokedLeafCertificateOnlyLeafCRLValidation) { TEST_P(SslSocketTest, RevokedLeafCertificateOnlyLeafCRLValidation) { // The test checks that revoked certificate will makes the validation fails even if we set - // only_verify_final_certificate_crl to true. + // only_verify_leaf_cert_crl to true. // // Trust chain contains: // - Root authority certificate (i.e., ca_cert.pem) @@ -4644,7 +4644,7 @@ TEST_P(SslSocketTest, RevokedLeafCertificateOnlyLeafCRLValidation) { validation_context: trusted_ca: filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/intermediate_ca_cert_chain_with_crl.pem" - only_verify_final_certificate_crl: true + only_verify_leaf_cert_crl: true )EOF"; // This should fail, since the certificate has been revoked. From 240507b95a5c7119c357530a62f61e8c54e8dc71 Mon Sep 17 00:00:00 2001 From: Shikugawa Date: Fri, 15 Oct 2021 14:13:12 +0900 Subject: [PATCH 07/11] fix Signed-off-by: Shikugawa --- envoy/ssl/certificate_validation_context_config.h | 2 +- .../common/ssl/certificate_validation_context_config_impl.h | 2 +- .../transport_sockets/tls/cert_validator/default_validator.cc | 4 ++-- .../transport_sockets/tls/cert_validator/test_common.h | 2 +- test/extensions/transport_sockets/tls/ssl_socket_test.cc | 3 ++- test/mocks/ssl/mocks.h | 2 +- 6 files changed, 8 insertions(+), 7 deletions(-) diff --git a/envoy/ssl/certificate_validation_context_config.h b/envoy/ssl/certificate_validation_context_config.h index 61ba5d5684db2..d8a1bf6b0af20 100644 --- a/envoy/ssl/certificate_validation_context_config.h +++ b/envoy/ssl/certificate_validation_context_config.h @@ -82,7 +82,7 @@ class CertificateValidationContextConfig { /** * @return whether to validate certificate chain with all CRL or not. */ - virtual bool onlyVerifyFinalCertificateCrl() const PURE; + virtual bool onlyVerifyLeafCertificateCrl() const PURE; }; using CertificateValidationContextConfigPtr = std::unique_ptr; diff --git a/source/common/ssl/certificate_validation_context_config_impl.h b/source/common/ssl/certificate_validation_context_config_impl.h index e82dd1cd1aa02..038abe08b1e96 100644 --- a/source/common/ssl/certificate_validation_context_config_impl.h +++ b/source/common/ssl/certificate_validation_context_config_impl.h @@ -48,7 +48,7 @@ class CertificateValidationContextConfigImpl : public CertificateValidationConte Api::Api& api() const override { return api_; } - bool onlyVerifyFinalCertificateCrl() const override { return only_verify_leaf_cert_crl_; } + bool onlyVerifyLeafCertificateCrl() const override { return only_verify_leaf_cert_crl_; } private: const std::string ca_cert_; 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 e8311c6d51f79..d53cba838ef6a 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,7 @@ 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, config_->onlyVerifyFinalCertificateCrl() + X509_STORE_set_flags(store, config_->onlyVerifyLeafCertificateCrl() ? X509_V_FLAG_CRL_CHECK : X509_V_FLAG_CRL_CHECK | X509_V_FLAG_CRL_CHECK_ALL); } @@ -138,7 +138,7 @@ int DefaultCertValidator::initializeSslContexts(std::vector contexts, X509_STORE_add_crl(store, item->crl); } } - X509_STORE_set_flags(store, config_->onlyVerifyFinalCertificateCrl() + 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 1f95f4ced37db..11d4022cf0436 100644 --- a/test/extensions/transport_sockets/tls/cert_validator/test_common.h +++ b/test/extensions/transport_sockets/tls/cert_validator/test_common.h @@ -72,7 +72,7 @@ class TestCertificateValidationContextConfig } Api::Api& api() const override { return *api_; } - bool onlyVerifyFinalCertificateCrl() const override { return false; } + 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 a08e54c1dc7fe..1e03bf28e3fd1 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -4685,7 +4685,8 @@ TEST_P(SslSocketTest, RevokedLeafCertificateOnlyLeafCRLValidation) { 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")); + testUtil(complete_revoked_test_options.setExpectedServerStats("ssl.fail_verify_error") + .setExpectedVerifyErrorCode(X509_V_ERR_CERT_REVOKED)); } TEST_P(SslSocketTest, GetRequestedServerName) { diff --git a/test/mocks/ssl/mocks.h b/test/mocks/ssl/mocks.h index a92d41c7462ba..ae545d126d55f 100644 --- a/test/mocks/ssl/mocks.h +++ b/test/mocks/ssl/mocks.h @@ -167,7 +167,7 @@ class MockCertificateValidationContextConfig : public CertificateValidationConte MOCK_METHOD(envoy::extensions::transport_sockets::tls::v3::CertificateValidationContext:: TrustChainVerification, trustChainVerification, (), (const)); - MOCK_METHOD(bool, onlyVerifyFinalCertificateCrl, (), (const)); + MOCK_METHOD(bool, onlyVerifyLeafCertificateCrl, (), (const)); }; class MockPrivateKeyMethodManager : public PrivateKeyMethodManager { From 79f987ed2f97ea1907b35f99d7819daed3903025 Mon Sep 17 00:00:00 2001 From: Shikugawa Date: Fri, 15 Oct 2021 07:32:39 +0000 Subject: [PATCH 08/11] fix Signed-off-by: Shikugawa --- docs/root/version_history/current.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index f63812e3b1d20..32a36d5be266d 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -41,8 +41,8 @@ New Features * api: added support for *xds.type.v3.TypedStruct* in addition to the now-deprecated *udpa.type.v1.TypedStruct* proto message, which is a wrapper proto used to encode typed JSON data in a *google.protobuf.Any* field. * ext_authz: added :ref:`query_parameters_to_set ` and :ref:`query_parameters_to_remove ` for adding and removing query string parameters when using a gRPC authorization server. * http: added support for :ref:`retriable health check status codes `. -* tls: added support for only verifying the leaf CRL in the certificate chain with :ref:`only_verify_leaf_cert_crl `. * thrift_proxy: add upstream response zone metrics in the form ``cluster.cluster_name.zone.local_zone.upstream_zone.thrift.upstream_resp_success``. +* tls: added support for only verifying the leaf CRL in the certificate chain with :ref:`only_verify_leaf_cert_crl `. * upstream: added the ability to :ref:`configure max connection duration ` for upstream clusters. * vcl_socket_interface: added VCL socket interface extension for fd.io VPP integration to :ref:`contrib images `. This can be enabled via :ref:`VCL ` configuration. From 4a206810906444d7925f2245245ec00359723092 Mon Sep 17 00:00:00 2001 From: Shikugawa Date: Fri, 29 Oct 2021 18:41:23 +0900 Subject: [PATCH 09/11] fix Signed-off-by: Shikugawa --- test/extensions/transport_sockets/tls/ssl_socket_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc index 1e03bf28e3fd1..4685f03829a4f 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -4611,7 +4611,7 @@ TEST_P(SslSocketTest, RevokedIntermediateCertificateCRLInTrustedCA) { } TEST_P(SslSocketTest, NotRevokedLeafCertificateOnlyLeafCRLValidation) { - // The test checks that revoked certificate will makes the validation fails even if we set + // 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: From bc7e61ac5f37f8ca80c0b785e55eeeaf664d0077 Mon Sep 17 00:00:00 2001 From: Shikugawa Date: Fri, 29 Oct 2021 18:42:21 +0900 Subject: [PATCH 10/11] fix Signed-off-by: Shikugawa --- docs/root/version_history/current.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 6ef4d6ef69265..b6fdcda7a1e9e 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -58,10 +58,10 @@ New Features * listener: added API for extensions to access :ref:`typed_filter_metadata ` configured in the listener's :ref:`metadata ` field. * oauth filter: added :ref:`cookie_names ` to allow overriding (default) cookie names (``BearerToken``, ``OauthHMAC``, and ``OauthExpires``) set by the filter. * thrift_proxy: add upstream response zone metrics in the form ``cluster.cluster_name.zone.local_zone.upstream_zone.thrift.upstream_resp_success``. -* tls: added support for only verifying the leaf CRL in the certificate chain with :ref:`only_verify_leaf_cert_crl `. * 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. From 5059098bfb77aa49cc37cec41aee588416c14b6f Mon Sep 17 00:00:00 2001 From: Shikugawa Date: Tue, 2 Nov 2021 17:56:02 +0900 Subject: [PATCH 11/11] Kick CI Signed-off-by: Shikugawa