From fb0e92148f037f16fff4ca464556e5e5ff2a3feb Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Sun, 26 Aug 2018 22:29:51 -0700 Subject: [PATCH 1/7] Add certificate validation context config provider. Signed-off-by: JimmyCYJ --- include/envoy/secret/BUILD | 1 + include/envoy/secret/secret_manager.h | 18 ++++++ include/envoy/secret/secret_provider.h | 6 ++ include/envoy/ssl/BUILD | 6 ++ .../certificate_validation_context_config.h | 61 +++++++++++++++++++ include/envoy/ssl/context_config.h | 42 +------------ source/common/common/logger.h | 1 + source/common/secret/BUILD | 1 + source/common/secret/secret_manager_impl.cc | 26 ++++++++ source/common/secret/secret_manager_impl.h | 18 +++++- source/common/secret/secret_provider_impl.cc | 8 +++ source/common/secret/secret_provider_impl.h | 17 ++++++ source/common/ssl/BUILD | 12 ++++ source/common/ssl/context_config_impl.cc | 58 ++++++++---------- source/common/ssl/context_config_impl.h | 37 +++-------- source/common/ssl/context_impl.cc | 60 ++++++++++-------- test/mocks/secret/mocks.cc | 6 ++ test/mocks/secret/mocks.h | 6 ++ 18 files changed, 258 insertions(+), 126 deletions(-) create mode 100644 include/envoy/ssl/certificate_validation_context_config.h diff --git a/include/envoy/secret/BUILD b/include/envoy/secret/BUILD index 6304003c5546..958ca83a4144 100644 --- a/include/envoy/secret/BUILD +++ b/include/envoy/secret/BUILD @@ -19,6 +19,7 @@ envoy_cc_library( deps = [ ":secret_callbacks_interface", "//include/envoy/common:callback", + "//include/envoy/ssl:certificate_validation_context_config_interface", "//include/envoy/ssl:tls_certificate_config_interface", ], ) diff --git a/include/envoy/secret/secret_manager.h b/include/envoy/secret/secret_manager.h index 85f5e7228f72..b5b6eda8059d 100644 --- a/include/envoy/secret/secret_manager.h +++ b/include/envoy/secret/secret_manager.h @@ -31,12 +31,30 @@ class SecretManager { virtual TlsCertificateConfigProviderSharedPtr findStaticTlsCertificateProvider(const std::string& name) const PURE; + /** + * @param name a name of the static CertificateValidationContextConfigProviderSharedPtr. + * @return the CertificateValidationContextConfigProviderSharedPtr. Returns nullptr + * if the static certificate validation context is not found. + */ + virtual CertificateValidationContextConfigProviderSharedPtr + findStaticCertificateValidationContextProvider(const std::string& name) const PURE; + /** * @param tls_certificate the protobuf config of the TLS certificate. * @return a TlsCertificateConfigProviderSharedPtr created from tls_certificate. */ virtual TlsCertificateConfigProviderSharedPtr createInlineTlsCertificateProvider( const envoy::api::v2::auth::TlsCertificate& tls_certificate) PURE; + + /** + * @param tls_certificate the protobuf config of the certificate validation context. + * @return a CertificateValidationContextConfigProviderSharedPtr created from + * certificate_validation_context. + */ + virtual CertificateValidationContextConfigProviderSharedPtr + createInlineCertificateValidationContextProvider( + const envoy::api::v2::auth::CertificateValidationContext& certificate_validation_context) + PURE; }; } // namespace Secret diff --git a/include/envoy/secret/secret_provider.h b/include/envoy/secret/secret_provider.h index b43b06ed727a..0e130f41caa9 100644 --- a/include/envoy/secret/secret_provider.h +++ b/include/envoy/secret/secret_provider.h @@ -4,6 +4,7 @@ #include "envoy/common/callback.h" #include "envoy/common/pure.h" +#include "envoy/ssl/certificate_validation_context_config.h" #include "envoy/ssl/tls_certificate_config.h" namespace Envoy { @@ -34,5 +35,10 @@ template class SecretProvider { typedef SecretProvider TlsCertificateConfigProvider; typedef std::shared_ptr TlsCertificateConfigProviderSharedPtr; +typedef SecretProvider + CertificateValidationContextConfigProvider; +typedef std::shared_ptr + CertificateValidationContextConfigProviderSharedPtr; + } // namespace Secret } // namespace Envoy diff --git a/include/envoy/ssl/BUILD b/include/envoy/ssl/BUILD index 315b9a5071d5..ace87b66039d 100644 --- a/include/envoy/ssl/BUILD +++ b/include/envoy/ssl/BUILD @@ -22,6 +22,7 @@ envoy_cc_library( name = "context_config_interface", hdrs = ["context_config.h"], deps = [ + ":certificate_validation_context_config_interface", ":tls_certificate_config_interface", ], ) @@ -40,3 +41,8 @@ envoy_cc_library( name = "tls_certificate_config_interface", hdrs = ["tls_certificate_config.h"], ) + +envoy_cc_library( + name = "certificate_validation_context_config_interface", + hdrs = ["certificate_validation_context_config.h"], +) diff --git a/include/envoy/ssl/certificate_validation_context_config.h b/include/envoy/ssl/certificate_validation_context_config.h new file mode 100644 index 000000000000..0b8efada52c3 --- /dev/null +++ b/include/envoy/ssl/certificate_validation_context_config.h @@ -0,0 +1,61 @@ +#pragma once + +#include +#include + +#include "envoy/common/pure.h" + +namespace Envoy { +namespace Ssl { + +class CertificateValidationContextConfig { +public: + virtual ~CertificateValidationContextConfig() {} + + /** + * @return The CA certificate to use for peer validation. + */ + virtual const std::string& caCert() const PURE; + + /** + * @return Path of the CA certificate to use for peer validation or "" + * if the CA certificate was inlined. + */ + virtual const std::string& caCertPath() const PURE; + + /** + * @return The CRL to check if a cert is revoked. + */ + virtual const std::string& certificateRevocationList() const PURE; + + /** + * @return Path of the certificate revocation list, or "" if the CRL + * was inlined. + */ + virtual const std::string& certificateRevocationListPath() const PURE; + + /** + * @return The subject alt names to be verified, if enabled. Otherwise, "" + */ + virtual const std::vector& verifySubjectAltNameList() const PURE; + + /** + * @return A list of a hex-encoded SHA-256 certificate hashes to be verified. + */ + virtual const std::vector& verifyCertificateHashList() const PURE; + + /** + * @return A list of a hex-encoded SHA-256 SPKI hashes to be verified. + */ + virtual const std::vector& verifyCertificateSpkiList() const PURE; + + /** + * @return whether to ignore expired certificates (both too new and too old). + */ + virtual bool allowExpiredCertificate() const PURE; +}; + +typedef std::unique_ptr CertificateValidationContextConfigPtr; + +} // namespace Ssl +} // namespace Envoy diff --git a/include/envoy/ssl/context_config.h b/include/envoy/ssl/context_config.h index 749cc8451656..aebbc49c441e 100644 --- a/include/envoy/ssl/context_config.h +++ b/include/envoy/ssl/context_config.h @@ -5,6 +5,7 @@ #include #include "envoy/common/pure.h" +#include "envoy/ssl/certificate_validation_context_config.h" #include "envoy/ssl/tls_certificate_config.h" namespace Envoy { @@ -39,52 +40,15 @@ class ContextConfig { */ virtual const std::string& ecdhCurves() const PURE; - /** - * @return The CA certificate to use for peer validation. - */ - virtual const std::string& caCert() const PURE; - - /** - * @return Path of the CA certificate to use for peer validation or "" - * if the CA certificate was inlined. - */ - virtual const std::string& caCertPath() const PURE; - - /** - * @return The CRL to check if a cert is revoked. - */ - virtual const std::string& certificateRevocationList() const PURE; - - /** - * @return Path of the certificate revocation list, or "" if the CRL - * was inlined. - */ - virtual const std::string& certificateRevocationListPath() const PURE; - /** * @return TlsCertificateConfig the certificate config used to identify the local side. */ virtual const TlsCertificateConfig* tlsCertificate() const PURE; /** - * @return The subject alt names to be verified, if enabled. Otherwise, "" - */ - virtual const std::vector& verifySubjectAltNameList() const PURE; - - /** - * @return A list of a hex-encoded SHA-256 certificate hashes to be verified. - */ - virtual const std::vector& verifyCertificateHashList() const PURE; - - /** - * @return A list of a hex-encoded SHA-256 SPKI hashes to be verified. - */ - virtual const std::vector& verifyCertificateSpkiList() const PURE; - - /** - * @return whether to ignore expired certificates (both too new and too old). + * @return CertificateValidationContextConfig the certificate validation context config. */ - virtual bool allowExpiredCertificate() const PURE; + virtual const CertificateValidationContextConfig* certificateValidationContext() const PURE; /** * @return The minimum TLS protocol version to negotiate. diff --git a/source/common/common/logger.h b/source/common/common/logger.h index 3746377a8b1b..6e86eaa5acb9 100644 --- a/source/common/common/logger.h +++ b/source/common/common/logger.h @@ -44,6 +44,7 @@ namespace Logger { FUNCTION(router) \ FUNCTION(runtime) \ FUNCTION(stats) \ + FUNCTION(secret) \ FUNCTION(testing) \ FUNCTION(thrift) \ FUNCTION(tracing) \ diff --git a/source/common/secret/BUILD b/source/common/secret/BUILD index a92edf5117bc..49245ef4ee97 100644 --- a/source/common/secret/BUILD +++ b/source/common/secret/BUILD @@ -26,6 +26,7 @@ envoy_cc_library( hdrs = ["secret_provider_impl.h"], deps = [ "//include/envoy/secret:secret_provider_interface", + "//source/common/ssl:certificate_validation_context_config_impl_lib", "//source/common/ssl:tls_certificate_config_impl_lib", "@envoy_api//envoy/api/v2/auth:cert_cc", ], diff --git a/source/common/secret/secret_manager_impl.cc b/source/common/secret/secret_manager_impl.cc index f3f2c9549ea8..e47ccdd01e35 100644 --- a/source/common/secret/secret_manager_impl.cc +++ b/source/common/secret/secret_manager_impl.cc @@ -4,6 +4,7 @@ #include "common/common/assert.h" #include "common/secret/secret_provider_impl.h" +#include "common/ssl/certificate_validation_context_config_impl.h" #include "common/ssl/tls_certificate_config_impl.h" namespace Envoy { @@ -21,6 +22,17 @@ void SecretManagerImpl::addStaticSecret(const envoy::api::v2::auth::Secret& secr } break; } + case envoy::api::v2::auth::Secret::TypeCase::kValidationContext: { + auto secret_provider = std::make_shared( + secret.validation_context()); + if (!static_certificate_validation_context_providers_ + .insert(std::make_pair(secret.name(), secret_provider)) + .second) { + throw EnvoyException(fmt::format( + "Duplicate static CertificateValidationContext secret name {}", secret.name())); + } + break; + } default: throw EnvoyException("Secret type not implemented"); } @@ -32,10 +44,24 @@ SecretManagerImpl::findStaticTlsCertificateProvider(const std::string& name) con return (secret != static_tls_certificate_providers_.end()) ? secret->second : nullptr; } +CertificateValidationContextConfigProviderSharedPtr +SecretManagerImpl::findStaticCertificateValidationContextProvider(const std::string& name) const { + auto secret = static_certificate_validation_context_providers_.find(name); + return (secret != static_certificate_validation_context_providers_.end()) ? secret->second + : nullptr; +} + TlsCertificateConfigProviderSharedPtr SecretManagerImpl::createInlineTlsCertificateProvider( const envoy::api::v2::auth::TlsCertificate& tls_certificate) { return std::make_shared(tls_certificate); } +CertificateValidationContextConfigProviderSharedPtr +SecretManagerImpl::createInlineCertificateValidationContextProvider( + const envoy::api::v2::auth::CertificateValidationContext& certificate_validation_context) { + return std::make_shared( + certificate_validation_context); +} + } // namespace Secret } // namespace Envoy diff --git a/source/common/secret/secret_manager_impl.h b/source/common/secret/secret_manager_impl.h index 6af790f50c42..4adf4afb998c 100644 --- a/source/common/secret/secret_manager_impl.h +++ b/source/common/secret/secret_manager_impl.h @@ -4,6 +4,7 @@ #include "envoy/secret/secret_manager.h" #include "envoy/secret/secret_provider.h" +#include "envoy/ssl/certificate_validation_context_config.h" #include "envoy/ssl/tls_certificate_config.h" #include "common/common/logger.h" @@ -11,17 +12,32 @@ namespace Envoy { namespace Secret { -class SecretManagerImpl : public SecretManager, Logger::Loggable { +class SecretManagerImpl : public SecretManager, Logger::Loggable { public: void addStaticSecret(const envoy::api::v2::auth::Secret& secret) override; + TlsCertificateConfigProviderSharedPtr findStaticTlsCertificateProvider(const std::string& name) const override; + + CertificateValidationContextConfigProviderSharedPtr + findStaticCertificateValidationContextProvider(const std::string& name) const override; + TlsCertificateConfigProviderSharedPtr createInlineTlsCertificateProvider( const envoy::api::v2::auth::TlsCertificate& tls_certificate) override; + CertificateValidationContextConfigProviderSharedPtr + createInlineCertificateValidationContextProvider( + const envoy::api::v2::auth::CertificateValidationContext& certificate_validation_context) + override; + private: + // Manages pairs of secret name and TlsCertificateConfigProviderSharedPtr. std::unordered_map static_tls_certificate_providers_; + + // Manages pairs of secret name and CertificateValidationContextConfigProviderSharedPtr. + std::unordered_map + static_certificate_validation_context_providers_; }; } // namespace Secret diff --git a/source/common/secret/secret_provider_impl.cc b/source/common/secret/secret_provider_impl.cc index 961924bef5a8..314106638b57 100644 --- a/source/common/secret/secret_provider_impl.cc +++ b/source/common/secret/secret_provider_impl.cc @@ -1,6 +1,7 @@ #include "common/secret/secret_provider_impl.h" #include "common/common/assert.h" +#include "common/ssl/certificate_validation_context_config_impl.h" #include "common/ssl/tls_certificate_config_impl.h" namespace Envoy { @@ -10,5 +11,12 @@ TlsCertificateConfigProviderImpl::TlsCertificateConfigProviderImpl( const envoy::api::v2::auth::TlsCertificate& tls_certificate) : tls_certificate_(std::make_unique(tls_certificate)) {} +CertificateValidationContextConfigProviderImpl::CertificateValidationContextConfigProviderImpl( + const envoy::api::v2::auth::CertificateValidationContext& certificate_validation_context) + : certificate_validation_context_(std::make_unique( + certificate_validation_context)) { + std::cout << "called CertificateValidationContextConfigProviderImpl()" << std::endl; +} + } // namespace Secret } // namespace Envoy diff --git a/source/common/secret/secret_provider_impl.h b/source/common/secret/secret_provider_impl.h index 959eecef5fa9..17e458773b30 100644 --- a/source/common/secret/secret_provider_impl.h +++ b/source/common/secret/secret_provider_impl.h @@ -4,6 +4,7 @@ #include "envoy/api/v2/auth/cert.pb.h" #include "envoy/secret/secret_provider.h" +#include "envoy/ssl/certificate_validation_context_config.h" #include "envoy/ssl/tls_certificate_config.h" namespace Envoy { @@ -21,5 +22,21 @@ class TlsCertificateConfigProviderImpl : public TlsCertificateConfigProvider { Ssl::TlsCertificateConfigPtr tls_certificate_; }; +class CertificateValidationContextConfigProviderImpl + : public CertificateValidationContextConfigProvider { +public: + CertificateValidationContextConfigProviderImpl( + const envoy::api::v2::auth::CertificateValidationContext& certificate_validation_context); + + const Ssl::CertificateValidationContextConfig* secret() const override { + return certificate_validation_context_.get(); + } + + Common::CallbackHandle* addUpdateCallback(std::function) override { return nullptr; } + +private: + Ssl::CertificateValidationContextConfigPtr certificate_validation_context_; +}; + } // namespace Secret } // namespace Envoy diff --git a/source/common/ssl/BUILD b/source/common/ssl/BUILD index abc98f802ff0..98bd64847d42 100644 --- a/source/common/ssl/BUILD +++ b/source/common/ssl/BUILD @@ -85,6 +85,18 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "certificate_validation_context_config_impl_lib", + srcs = ["certificate_validation_context_config_impl.cc"], + hdrs = ["certificate_validation_context_config_impl.h"], + deps = [ + "//include/envoy/ssl:certificate_validation_context_config_interface", + "//source/common/common:empty_string", + "//source/common/config:datasource_lib", + "@envoy_api//envoy/api/v2/auth:cert_cc", + ], +) + envoy_cc_library( name = "utility_lib", srcs = ["utility.cc"], diff --git a/source/common/ssl/context_config_impl.cc b/source/common/ssl/context_config_impl.cc index fd9f49ed210c..160855018299 100644 --- a/source/common/ssl/context_config_impl.cc +++ b/source/common/ssl/context_config_impl.cc @@ -40,6 +40,29 @@ getTlsCertificateConfigProvider(const envoy::api::v2::auth::CommonTlsContext& co return nullptr; } +Secret::CertificateValidationContextConfigProviderSharedPtr +getCertificateValidationContextConfigProvider(const envoy::api::v2::auth::CommonTlsContext& config, + Secret::SecretManager& secret_manager) { + if (config.has_validation_context()) { + return secret_manager.createInlineCertificateValidationContextProvider( + config.validation_context()); + } + if (config.has_validation_context_sds_secret_config()) { + const auto& sds_secret_config = config.validation_context_sds_secret_config(); + if (!sds_secret_config.has_sds_config()) { + // static secret + auto secret_provider = + secret_manager.findStaticCertificateValidationContextProvider(sds_secret_config.name()); + if (!secret_provider) { + throw EnvoyException(fmt::format("Unknown static certificate validation context: {}", + sds_secret_config.name())); + } + return secret_provider; + } + } + return nullptr; +} + } // namespace const std::string ContextConfigImpl::DEFAULT_CIPHER_SUITES = @@ -66,40 +89,13 @@ ContextConfigImpl::ContextConfigImpl(const envoy::api::v2::auth::CommonTlsContex RepeatedPtrUtil::join(config.tls_params().cipher_suites(), ":"), DEFAULT_CIPHER_SUITES)), ecdh_curves_(StringUtil::nonEmptyStringOrDefault( RepeatedPtrUtil::join(config.tls_params().ecdh_curves(), ":"), DEFAULT_ECDH_CURVES)), - ca_cert_(Config::DataSource::read(config.validation_context().trusted_ca(), true)), - ca_cert_path_(Config::DataSource::getPath(config.validation_context().trusted_ca()) - .value_or(EMPTY_STRING)), - certificate_revocation_list_( - Config::DataSource::read(config.validation_context().crl(), true)), - certificate_revocation_list_path_( - Config::DataSource::getPath(config.validation_context().crl()).value_or(EMPTY_STRING)), tls_certficate_provider_(getTlsCertificateConfigProvider(config, secret_manager)), - verify_subject_alt_name_list_(config.validation_context().verify_subject_alt_name().begin(), - config.validation_context().verify_subject_alt_name().end()), - verify_certificate_hash_list_(config.validation_context().verify_certificate_hash().begin(), - config.validation_context().verify_certificate_hash().end()), - verify_certificate_spki_list_(config.validation_context().verify_certificate_spki().begin(), - config.validation_context().verify_certificate_spki().end()), - allow_expired_certificate_(config.validation_context().allow_expired_certificate()), + certficate_validation_context_provider_( + getCertificateValidationContextConfigProvider(config, secret_manager)), min_protocol_version_( tlsVersionFromProto(config.tls_params().tls_minimum_protocol_version(), TLS1_VERSION)), - max_protocol_version_( - tlsVersionFromProto(config.tls_params().tls_maximum_protocol_version(), TLS1_2_VERSION)) { - if (ca_cert_.empty()) { - if (!certificate_revocation_list_.empty()) { - throw EnvoyException(fmt::format("Failed to load CRL from {} without trusted CA", - certificateRevocationListPath())); - } - if (!verify_subject_alt_name_list_.empty()) { - throw EnvoyException(fmt::format("SAN-based verification of peer certificates without " - "trusted CA is insecure and not allowed")); - } - if (allow_expired_certificate_) { - throw EnvoyException( - fmt::format("Certificate validity period is always ignored without trusted CA")); - } - } -} + max_protocol_version_(tlsVersionFromProto(config.tls_params().tls_maximum_protocol_version(), + TLS1_2_VERSION)) {} unsigned ContextConfigImpl::tlsVersionFromProto( const envoy::api::v2::auth::TlsParameters_TlsProtocol& version, unsigned default_version) { diff --git a/source/common/ssl/context_config_impl.h b/source/common/ssl/context_config_impl.h index cb6b1c92c969..25663ca4208e 100644 --- a/source/common/ssl/context_config_impl.h +++ b/source/common/ssl/context_config_impl.h @@ -23,31 +23,14 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig { const std::string& altAlpnProtocols() const override { return alt_alpn_protocols_; } const std::string& cipherSuites() const override { return cipher_suites_; } const std::string& ecdhCurves() const override { return ecdh_curves_; } - const std::string& caCert() const override { return ca_cert_; } - const std::string& caCertPath() const override { - return (ca_cert_path_.empty() && !ca_cert_.empty()) ? INLINE_STRING : ca_cert_path_; - } - const std::string& certificateRevocationList() const override { - return certificate_revocation_list_; - } - const std::string& certificateRevocationListPath() const override { - return (certificate_revocation_list_path_.empty() && !certificate_revocation_list_.empty()) - ? INLINE_STRING - : certificate_revocation_list_path_; - } const TlsCertificateConfig* tlsCertificate() const override { return tls_certficate_provider_ == nullptr ? nullptr : tls_certficate_provider_->secret(); } - const std::vector& verifySubjectAltNameList() const override { - return verify_subject_alt_name_list_; - }; - const std::vector& verifyCertificateHashList() const override { - return verify_certificate_hash_list_; - }; - const std::vector& verifyCertificateSpkiList() const override { - return verify_certificate_spki_list_; - }; - bool allowExpiredCertificate() const override { return allow_expired_certificate_; }; + const CertificateValidationContextConfig* certificateValidationContext() const override { + return certficate_validation_context_provider_ == nullptr + ? nullptr + : certficate_validation_context_provider_->secret(); + } unsigned minProtocolVersion() const override { return min_protocol_version_; }; unsigned maxProtocolVersion() const override { return max_protocol_version_; }; @@ -67,15 +50,9 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig { const std::string alt_alpn_protocols_; const std::string cipher_suites_; const std::string ecdh_curves_; - const std::string ca_cert_; - const std::string ca_cert_path_; - const std::string certificate_revocation_list_; - const std::string certificate_revocation_list_path_; Secret::TlsCertificateConfigProviderSharedPtr tls_certficate_provider_; - const std::vector verify_subject_alt_name_list_; - const std::vector verify_certificate_hash_list_; - const std::vector verify_certificate_spki_list_; - const bool allow_expired_certificate_; + Secret::CertificateValidationContextConfigProviderSharedPtr + certficate_validation_context_provider_; const unsigned min_protocol_version_; const unsigned max_protocol_version_; }; diff --git a/source/common/ssl/context_impl.cc b/source/common/ssl/context_impl.cc index a1dced0bec55..e42e8d666f63 100644 --- a/source/common/ssl/context_impl.cc +++ b/source/common/ssl/context_impl.cc @@ -64,18 +64,19 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const ContextConfig& config) } int verify_mode = SSL_VERIFY_NONE; - - if (!config.caCert().empty()) { - ca_file_path_ = config.caCertPath(); + if (config.certificateValidationContext() != nullptr && + !config.certificateValidationContext()->caCert().empty()) { + ca_file_path_ = config.certificateValidationContext()->caCertPath(); bssl::UniquePtr bio( - BIO_new_mem_buf(const_cast(config.caCert().data()), config.caCert().size())); + BIO_new_mem_buf(const_cast(config.certificateValidationContext()->caCert().data()), + config.certificateValidationContext()->caCert().size())); RELEASE_ASSERT(bio != nullptr, ""); // Based on BoringSSL's X509_load_cert_crl_file(). bssl::UniquePtr list( PEM_X509_INFO_read_bio(bio.get(), nullptr, nullptr, nullptr)); if (list == nullptr) { - throw EnvoyException( - fmt::format("Failed to load trusted CA certificates from {}", config.caCertPath())); + throw EnvoyException(fmt::format("Failed to load trusted CA certificates from {}", + config.certificateValidationContext()->caCertPath())); } X509_STORE* store = SSL_CTX_get_cert_store(ctx_.get()); @@ -92,8 +93,8 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const ContextConfig& config) } } if (ca_cert_ == nullptr) { - throw EnvoyException( - fmt::format("Failed to load trusted CA certificates from {}", config.caCertPath())); + throw EnvoyException(fmt::format("Failed to load trusted CA certificates from {}", + config.certificateValidationContext()->caCertPath())); } verify_mode = SSL_VERIFY_PEER; verify_trusted_ca_ = true; @@ -102,15 +103,17 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const ContextConfig& config) // directly. However, our new callback is still calling X509_verify_cert() under // the hood. Therefore, to ignore cert expiration, we need to set the callback // for X509_verify_cert to ignore that error. - if (config.allowExpiredCertificate()) { + if (config.certificateValidationContext()->allowExpiredCertificate()) { X509_STORE_set_verify_cb(store, ContextImpl::ignoreCertificateExpirationCallback); } } - if (!config.certificateRevocationList().empty()) { - bssl::UniquePtr bio( - BIO_new_mem_buf(const_cast(config.certificateRevocationList().data()), - config.certificateRevocationList().size())); + if (config.certificateValidationContext() != nullptr && + !config.certificateValidationContext()->certificateRevocationList().empty()) { + bssl::UniquePtr bio(BIO_new_mem_buf( + const_cast( + config.certificateValidationContext()->certificateRevocationList().data()), + config.certificateValidationContext()->certificateRevocationList().size())); RELEASE_ASSERT(bio != nullptr, ""); // Based on BoringSSL's X509_load_cert_crl_file(). @@ -118,7 +121,8 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const ContextConfig& config) PEM_X509_INFO_read_bio(bio.get(), nullptr, nullptr, nullptr)); if (list == nullptr) { throw EnvoyException( - fmt::format("Failed to load CRL from {}", config.certificateRevocationListPath())); + fmt::format("Failed to load CRL from {}", + config.certificateValidationContext()->certificateRevocationListPath())); } X509_STORE* store = SSL_CTX_get_cert_store(ctx_.get()); @@ -131,13 +135,16 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const ContextConfig& config) X509_STORE_set_flags(store, X509_V_FLAG_CRL_CHECK | X509_V_FLAG_CRL_CHECK_ALL); } - if (!config.verifySubjectAltNameList().empty()) { - verify_subject_alt_name_list_ = config.verifySubjectAltNameList(); + if (config.certificateValidationContext() != nullptr && + !config.certificateValidationContext()->verifySubjectAltNameList().empty()) { + verify_subject_alt_name_list_ = + config.certificateValidationContext()->verifySubjectAltNameList(); verify_mode = SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT; } - if (!config.verifyCertificateHashList().empty()) { - for (auto hash : config.verifyCertificateHashList()) { + if (config.certificateValidationContext() != nullptr && + !config.certificateValidationContext()->verifyCertificateHashList().empty()) { + for (auto hash : config.certificateValidationContext()->verifyCertificateHashList()) { // Remove colons from the 95 chars long colon-separated "fingerprint" // in order to get the hex-encoded string. if (hash.size() == 95) { @@ -152,8 +159,9 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const ContextConfig& config) verify_mode = SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT; } - if (!config.verifyCertificateSpkiList().empty()) { - for (auto hash : config.verifyCertificateSpkiList()) { + if (config.certificateValidationContext() != nullptr && + !config.certificateValidationContext()->verifyCertificateSpkiList().empty()) { + for (auto hash : config.certificateValidationContext()->verifyCertificateSpkiList()) { const auto decoded = Base64::decode(hash); if (decoded.size() != SHA256_DIGEST_LENGTH) { throw EnvoyException(fmt::format("Invalid base64-encoded SHA-256 {}", hash)); @@ -501,9 +509,11 @@ ServerContextImpl::ServerContextImpl(Stats::Scope& scope, const ServerContextCon if (config.tlsCertificate() == nullptr) { throw EnvoyException("Server TlsCertificates must have a certificate specified"); } - if (!config.caCert().empty()) { + if (config.certificateValidationContext() != nullptr && + !config.certificateValidationContext()->caCert().empty()) { bssl::UniquePtr bio( - BIO_new_mem_buf(const_cast(config.caCert().data()), config.caCert().size())); + BIO_new_mem_buf(const_cast(config.certificateValidationContext()->caCert().data()), + config.certificateValidationContext()->caCert().size())); RELEASE_ASSERT(bio != nullptr, ""); // Based on BoringSSL's SSL_add_file_cert_subjects_to_stack(). bssl::UniquePtr list(sk_X509_NAME_new( @@ -517,7 +527,7 @@ ServerContextImpl::ServerContextImpl(Stats::Scope& scope, const ServerContextCon X509_NAME* name = X509_get_subject_name(cert.get()); if (name == nullptr) { throw EnvoyException(fmt::format("Failed to load trusted client CA certificates from {}", - config.caCertPath())); + config.certificateValidationContext()->caCertPath())); } // Check for duplicates. if (sk_X509_NAME_find(list.get(), nullptr, name)) { @@ -526,7 +536,7 @@ ServerContextImpl::ServerContextImpl(Stats::Scope& scope, const ServerContextCon bssl::UniquePtr name_dup(X509_NAME_dup(name)); if (name_dup == nullptr || !sk_X509_NAME_push(list.get(), name_dup.release())) { throw EnvoyException(fmt::format("Failed to load trusted client CA certificates from {}", - config.caCertPath())); + config.certificateValidationContext()->caCertPath())); } } // Check for EOF. @@ -535,7 +545,7 @@ ServerContextImpl::ServerContextImpl(Stats::Scope& scope, const ServerContextCon ERR_clear_error(); } else { throw EnvoyException(fmt::format("Failed to load trusted client CA certificates from {}", - config.caCertPath())); + config.certificateValidationContext()->caCertPath())); } SSL_CTX_set_client_CA_list(ctx_.get(), list.release()); diff --git a/test/mocks/secret/mocks.cc b/test/mocks/secret/mocks.cc index 7461830a5693..3bacd5fe9894 100644 --- a/test/mocks/secret/mocks.cc +++ b/test/mocks/secret/mocks.cc @@ -13,6 +13,12 @@ MockSecretManager::MockSecretManager() { .WillByDefault(Invoke([](const envoy::api::v2::auth::TlsCertificate& tls_certificate) { return std::make_shared(tls_certificate); })); + ON_CALL(*this, createInlineCertificateValidationContextProvider(_)) + .WillByDefault(Invoke([](const envoy::api::v2::auth::CertificateValidationContext& + certificate_validation_context) { + return std::make_shared( + certificate_validation_context); + })); } MockSecretManager::~MockSecretManager() {} diff --git a/test/mocks/secret/mocks.h b/test/mocks/secret/mocks.h index 907d7b66e8e5..e5d0488c34bb 100644 --- a/test/mocks/secret/mocks.h +++ b/test/mocks/secret/mocks.h @@ -18,9 +18,15 @@ class MockSecretManager : public SecretManager { MOCK_METHOD1(addStaticSecret, void(const envoy::api::v2::auth::Secret& secret)); MOCK_CONST_METHOD1(findStaticTlsCertificateProvider, TlsCertificateConfigProviderSharedPtr(const std::string& name)); + MOCK_CONST_METHOD1(findStaticCertificateValidationContextProvider, + CertificateValidationContextConfigProviderSharedPtr(const std::string& name)); MOCK_METHOD1(createInlineTlsCertificateProvider, TlsCertificateConfigProviderSharedPtr( const envoy::api::v2::auth::TlsCertificate& tls_certificate)); + MOCK_METHOD1(createInlineCertificateValidationContextProvider, + CertificateValidationContextConfigProviderSharedPtr( + const envoy::api::v2::auth::CertificateValidationContext& + certificate_validation_context)); }; class MockSecretCallbacks : public SecretCallbacks { From 6b61c631253461f3900fe9a3852cec6aed4f5c42 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Sun, 26 Aug 2018 22:30:22 -0700 Subject: [PATCH 2/7] Add more files. Signed-off-by: JimmyCYJ --- ...tificate_validation_context_config_impl.cc | 47 +++++++++++++++++++ ...rtificate_validation_context_config_impl.h | 47 +++++++++++++++++++ 2 files changed, 94 insertions(+) create mode 100644 source/common/ssl/certificate_validation_context_config_impl.cc create mode 100644 source/common/ssl/certificate_validation_context_config_impl.h diff --git a/source/common/ssl/certificate_validation_context_config_impl.cc b/source/common/ssl/certificate_validation_context_config_impl.cc new file mode 100644 index 000000000000..8a79d224f046 --- /dev/null +++ b/source/common/ssl/certificate_validation_context_config_impl.cc @@ -0,0 +1,47 @@ +#include "common/ssl/certificate_validation_context_config_impl.h" + +#include "envoy/common/exception.h" + +#include "common/common/empty_string.h" +#include "common/common/fmt.h" +#include "common/config/datasource.h" + +namespace Envoy { +namespace Ssl { + +static const std::string INLINE_STRING = ""; + +CertificateValidationContextConfigImpl::CertificateValidationContextConfigImpl( + const envoy::api::v2::auth::CertificateValidationContext& config) + : ca_cert_(Config::DataSource::read(config.trusted_ca(), true)), + ca_cert_path_(Config::DataSource::getPath(config.trusted_ca()) + .value_or(ca_cert_.empty() ? EMPTY_STRING : INLINE_STRING)), + certificate_revocation_list_(Config::DataSource::read(config.crl(), true)), + 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.verify_subject_alt_name().begin(), + config.verify_subject_alt_name().end()), + verify_certificate_hash_list_(config.verify_certificate_hash().begin(), + config.verify_certificate_hash().end()), + verify_certificate_spki_list_(config.verify_certificate_spki().begin(), + config.verify_certificate_spki().end()), + allow_expired_certificate_(config.allow_expired_certificate()) { + if (ca_cert_.empty()) { + if (!certificate_revocation_list_.empty()) { + throw EnvoyException(fmt::format("Failed to load CRL from {} without trusted CA", + certificateRevocationListPath())); + } + if (!verify_subject_alt_name_list_.empty()) { + throw EnvoyException(fmt::format("SAN-based verification of peer certificates without " + "trusted CA is insecure and not allowed")); + } + if (allow_expired_certificate_) { + throw EnvoyException( + fmt::format("Certificate validity period is always ignored without trusted CA")); + } + } +} + +} // namespace Ssl +} // namespace Envoy \ No newline at end of file diff --git a/source/common/ssl/certificate_validation_context_config_impl.h b/source/common/ssl/certificate_validation_context_config_impl.h new file mode 100644 index 000000000000..692b34b2872f --- /dev/null +++ b/source/common/ssl/certificate_validation_context_config_impl.h @@ -0,0 +1,47 @@ +#pragma once + +#include + +#include "envoy/api/v2/auth/cert.pb.h" +#include "envoy/ssl/certificate_validation_context_config.h" + +namespace Envoy { +namespace Ssl { + +class CertificateValidationContextConfigImpl : public CertificateValidationContextConfig { +public: + CertificateValidationContextConfigImpl( + const envoy::api::v2::auth::CertificateValidationContext& config); + + const std::string& caCert() const override { return ca_cert_; } + const std::string& caCertPath() const override { return ca_cert_path_; } + const std::string& certificateRevocationList() const override { + return certificate_revocation_list_; + } + const std::string& certificateRevocationListPath() const override { + return certificate_revocation_list_path_; + } + const std::vector& verifySubjectAltNameList() const override { + return verify_subject_alt_name_list_; + } + const std::vector& verifyCertificateHashList() const override { + return verify_certificate_hash_list_; + } + const std::vector& verifyCertificateSpkiList() const override { + return verify_certificate_spki_list_; + } + bool allowExpiredCertificate() const override { return allow_expired_certificate_; } + +private: + const std::string ca_cert_; + const std::string ca_cert_path_; + const std::string certificate_revocation_list_; + const std::string certificate_revocation_list_path_; + const std::vector verify_subject_alt_name_list_; + const std::vector verify_certificate_hash_list_; + const std::vector verify_certificate_spki_list_; + const bool allow_expired_certificate_; +}; + +} // namespace Ssl +} // namespace Envoy \ No newline at end of file From 1aba595aac8a55a65e7913f76861ed04204f3682 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Mon, 27 Aug 2018 10:19:14 -0700 Subject: [PATCH 3/7] Remove debug log. Signed-off-by: JimmyCYJ --- source/common/secret/secret_provider_impl.cc | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/source/common/secret/secret_provider_impl.cc b/source/common/secret/secret_provider_impl.cc index 314106638b57..237cd0e2ed81 100644 --- a/source/common/secret/secret_provider_impl.cc +++ b/source/common/secret/secret_provider_impl.cc @@ -14,9 +14,7 @@ TlsCertificateConfigProviderImpl::TlsCertificateConfigProviderImpl( CertificateValidationContextConfigProviderImpl::CertificateValidationContextConfigProviderImpl( const envoy::api::v2::auth::CertificateValidationContext& certificate_validation_context) : certificate_validation_context_(std::make_unique( - certificate_validation_context)) { - std::cout << "called CertificateValidationContextConfigProviderImpl()" << std::endl; -} + certificate_validation_context)) {} } // namespace Secret } // namespace Envoy From 2c5d3d468ae4edb52e817014e14fa25902e76df5 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Tue, 28 Aug 2018 11:22:18 -0700 Subject: [PATCH 4/7] Add unit tests. Signed-off-by: JimmyCYJ --- .../common/secret/secret_manager_impl_test.cc | 77 ++++++++++++++- test/common/ssl/context_impl_test.cc | 94 +++++++++++++++++++ 2 files changed, 170 insertions(+), 1 deletion(-) diff --git a/test/common/secret/secret_manager_impl_test.cc b/test/common/secret/secret_manager_impl_test.cc index bf27f559fd4f..27e7c2a2e8e1 100644 --- a/test/common/secret/secret_manager_impl_test.cc +++ b/test/common/secret/secret_manager_impl_test.cc @@ -17,7 +17,7 @@ namespace { class SecretManagerImplTest : public testing::Test {}; -TEST_F(SecretManagerImplTest, SecretLoadSuccess) { +TEST_F(SecretManagerImplTest, TlsCertificateSecretLoadSuccess) { envoy::api::v2::auth::Secret secret_config; std::string yaml = @@ -50,6 +50,81 @@ name: "abc.com" secret_manager->findStaticTlsCertificateProvider("abc.com")->secret()->privateKey()); } +TEST_F(SecretManagerImplTest, DuplicateStaticTlsCertificateSecret) { + envoy::api::v2::auth::Secret secret_config; + + std::string yaml = + R"EOF( + name: "abc.com" + tls_certificate: + certificate_chain: + filename: "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_cert.pem" + private_key: + filename: "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_key.pem" + )EOF"; + + MessageUtil::loadFromYaml(TestEnvironment::substitute(yaml), secret_config); + + std::unique_ptr secret_manager(new SecretManagerImpl()); + + secret_manager->addStaticSecret(secret_config); + + ASSERT_NE(secret_manager->findStaticTlsCertificateProvider("abc.com"), nullptr); + + EXPECT_THROW_WITH_MESSAGE(secret_manager->addStaticSecret(secret_config), EnvoyException, + "Duplicate static TlsCertificate secret name abc.com"); +} + +TEST_F(SecretManagerImplTest, CertificateValidationContextSecretLoadSuccess) { + envoy::api::v2::auth::Secret secret_config; + + std::string yaml = + R"EOF( + name: "abc.com" + validation_context: + trusted_ca: { filename: "{{ test_rundir }}/test/common/ssl/test_data/ca_cert.pem" } + allow_expired_certificate: true + )EOF"; + + MessageUtil::loadFromYaml(TestEnvironment::substitute(yaml), secret_config); + + std::unique_ptr secret_manager(new SecretManagerImpl()); + + secret_manager->addStaticSecret(secret_config); + + ASSERT_EQ(secret_manager->findStaticCertificateValidationContextProvider("undefined"), nullptr); + + ASSERT_NE(secret_manager->findStaticCertificateValidationContextProvider("abc.com"), nullptr); + + const std::string cert_pem = "{{ test_rundir }}/test/common/ssl/test_data/ca_cert.pem"; + EXPECT_EQ(TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(cert_pem)), + secret_manager->findStaticCertificateValidationContextProvider("abc.com")->caCert()); +} + +TEST_F(SecretManagerImplTest, DuplicateStaticCertificateValidationContextSecret) { + envoy::api::v2::auth::Secret secret_config; + + std::string yaml = + R"EOF( + name: "abc.com" + validation_context: + trusted_ca: { filename: "{{ test_rundir }}/test/common/ssl/test_data/ca_cert.pem" } + allow_expired_certificate: true + )EOF"; + + MessageUtil::loadFromYaml(TestEnvironment::substitute(yaml), secret_config); + + std::unique_ptr secret_manager(new SecretManagerImpl()); + + secret_manager->addStaticSecret(secret_config); + + ASSERT_NE(secret_manager->findStaticCertificateValidationContextProvider("abc.com"), nullptr); + + EXPECT_THROW_WITH_MESSAGE( + secret_manager->findStaticCertificateValidationContextProvider(secret_config), EnvoyException, + "Duplicate static CertificateValidationContext secret name abc.com"); +} + TEST_F(SecretManagerImplTest, NotImplementedException) { envoy::api::v2::auth::Secret secret_config; diff --git a/test/common/ssl/context_impl_test.cc b/test/common/ssl/context_impl_test.cc index 4c533ea17c19..a7720277e4f2 100644 --- a/test/common/ssl/context_impl_test.cc +++ b/test/common/ssl/context_impl_test.cc @@ -451,6 +451,54 @@ name: "abc.com" client_context_config.tlsCertificate()->privateKey()); } +TEST(ClientContextConfigImplTest, StaticCertificateValidationContext) { + envoy::api::v2::auth::Secret tls_certificate_secret_config; + + std::string tls_certificate_yaml = R"EOF( + name: "abc.com" + tls_certificate: + certificate_chain: + filename: "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_cert.pem" + private_key: + filename: "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_key.pem" + )EOF"; + + MessageUtil::loadFromYaml(TestEnvironment::substitute(tls_certificate_yaml), + tls_certificate_secret_config); + + std::unique_ptr secret_manager(new Secret::SecretManagerImpl()); + secret_manager->addStaticSecret(tls_certificate_secret_config); + + envoy::api::v2::auth::Secret certificate_validation_context_secret_config; + + std::string certificate_validation_context_yaml = R"EOF( + name: "def.com" + validation_context: + trusted_ca: { filename: "{{ test_rundir }}/test/common/ssl/test_data/ca_cert.pem" } + allow_expired_certificate: true + )EOF"; + + MessageUtil::loadFromYaml(TestEnvironment::substitute(certificate_validation_context_yaml), + certificate_validation_context_secret_config); + + secret_manager->addStaticSecret(certificate_validation_context_secret_config); + + envoy::api::v2::auth::UpstreamTlsContext tls_context; + tls_context.mutable_common_tls_context() + ->mutable_tls_certificate_sds_secret_configs() + ->Add() + ->set_name("abc.com"); + tls_context.mutable_common_tls_context() + ->mutable_validation_context_sds_secret_config() + ->set_name("def.com"); + + ClientContextConfigImpl client_context_config(tls_context, *secret_manager.get()); + + const std::string cert_pem = "{{ test_rundir }}/test/common/ssl/test_data/ca_cert.pem"; + EXPECT_EQ(TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(cert_pem)), + client_context_config.certificateValidationContext()->caCert()); +} + TEST(ClientContextConfigImplTest, MissingStaticSecretTlsCertificates) { envoy::api::v2::auth::Secret secret_config; @@ -480,6 +528,52 @@ name: "abc.com" EnvoyException, "Static secret is not defined: missing"); } +TEST(ClientContextConfigImplTest, MissingStaticCertificateValidationContext) { + envoy::api::v2::auth::Secret tls_certificate_secret_config; + + std::string tls_certificate_yaml = R"EOF( + name: "abc.com" + tls_certificate: + certificate_chain: + filename: "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_cert.pem" + private_key: + filename: "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_key.pem" + )EOF"; + + MessageUtil::loadFromYaml(TestEnvironment::substitute(tls_certificate_yaml), + tls_certificate_secret_config); + + std::unique_ptr secret_manager(new Secret::SecretManagerImpl()); + secret_manager->addStaticSecret(tls_certificate_secret_config); + + envoy::api::v2::auth::Secret certificate_validation_context_secret_config; + + std::string certificate_validation_context_yaml = R"EOF( + name: "def.com" + validation_context: + trusted_ca: { filename: "{{ test_rundir }}/test/common/ssl/test_data/ca_cert.pem" } + allow_expired_certificate: true + )EOF"; + + MessageUtil::loadFromYaml(TestEnvironment::substitute(certificate_validation_context_yaml), + certificate_validation_context_secret_config); + + secret_manager->addStaticSecret(certificate_validation_context_secret_config); + + envoy::api::v2::auth::UpstreamTlsContext tls_context; + tls_context.mutable_common_tls_context() + ->mutable_tls_certificate_sds_secret_configs() + ->Add() + ->set_name("abc.com"); + tls_context.mutable_common_tls_context() + ->mutable_validation_context_sds_secret_config() + ->set_name("missing"); + + EXPECT_THROW_WITH_MESSAGE( + ClientContextConfigImpl client_context_config(tls_context, *secret_manager.get()), + EnvoyException, "Unknown static certificate validation context: missing"); +} + // Multiple TLS certificates are not yet supported, but one is expected for // server. // TODO(PiotrSikora): Support multiple TLS certificates. From 8e25759b236d2184da85fc77bce4556376171691 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Tue, 28 Aug 2018 11:48:02 -0700 Subject: [PATCH 5/7] Fix compile issue. Signed-off-by: JimmyCYJ --- test/common/secret/secret_manager_impl_test.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/common/secret/secret_manager_impl_test.cc b/test/common/secret/secret_manager_impl_test.cc index 27e7c2a2e8e1..b8a7e4c59254 100644 --- a/test/common/secret/secret_manager_impl_test.cc +++ b/test/common/secret/secret_manager_impl_test.cc @@ -98,7 +98,9 @@ TEST_F(SecretManagerImplTest, CertificateValidationContextSecretLoadSuccess) { const std::string cert_pem = "{{ test_rundir }}/test/common/ssl/test_data/ca_cert.pem"; EXPECT_EQ(TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(cert_pem)), - secret_manager->findStaticCertificateValidationContextProvider("abc.com")->caCert()); + secret_manager->findStaticCertificateValidationContextProvider("abc.com") + ->secret() + ->caCert()); } TEST_F(SecretManagerImplTest, DuplicateStaticCertificateValidationContextSecret) { From a353a5e3b32ca2a3fec0754c287cc88bde55def4 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Tue, 28 Aug 2018 12:39:27 -0700 Subject: [PATCH 6/7] Fix test. Signed-off-by: JimmyCYJ --- test/common/secret/secret_manager_impl_test.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/test/common/secret/secret_manager_impl_test.cc b/test/common/secret/secret_manager_impl_test.cc index b8a7e4c59254..81ded104606c 100644 --- a/test/common/secret/secret_manager_impl_test.cc +++ b/test/common/secret/secret_manager_impl_test.cc @@ -122,9 +122,8 @@ TEST_F(SecretManagerImplTest, DuplicateStaticCertificateValidationContextSecret) ASSERT_NE(secret_manager->findStaticCertificateValidationContextProvider("abc.com"), nullptr); - EXPECT_THROW_WITH_MESSAGE( - secret_manager->findStaticCertificateValidationContextProvider(secret_config), EnvoyException, - "Duplicate static CertificateValidationContext secret name abc.com"); + EXPECT_THROW_WITH_MESSAGE(secret_manager->addStaticSecret(secret_config), EnvoyException, + "Duplicate static CertificateValidationContext secret name abc.com"); } TEST_F(SecretManagerImplTest, NotImplementedException) { From 7a5a7939572ef225eeb1aec958f3ae3a676e2ee4 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Tue, 28 Aug 2018 14:55:13 -0700 Subject: [PATCH 7/7] Add comments and reduce vertical spaces. Signed-off-by: JimmyCYJ --- .../common/secret/secret_manager_impl_test.cc | 39 +++++++------------ test/common/ssl/context_impl_test.cc | 35 +++++++---------- 2 files changed, 26 insertions(+), 48 deletions(-) diff --git a/test/common/secret/secret_manager_impl_test.cc b/test/common/secret/secret_manager_impl_test.cc index 81ded104606c..35374fca8c44 100644 --- a/test/common/secret/secret_manager_impl_test.cc +++ b/test/common/secret/secret_manager_impl_test.cc @@ -17,10 +17,10 @@ namespace { class SecretManagerImplTest : public testing::Test {}; +// Validate that secret manager adds static TLS certificate secret successfully. TEST_F(SecretManagerImplTest, TlsCertificateSecretLoadSuccess) { envoy::api::v2::auth::Secret secret_config; - - std::string yaml = + const std::string yaml = R"EOF( name: "abc.com" tls_certificate: @@ -29,15 +29,11 @@ name: "abc.com" private_key: filename: "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_key.pem" )EOF"; - MessageUtil::loadFromYaml(TestEnvironment::substitute(yaml), secret_config); - std::unique_ptr secret_manager(new SecretManagerImpl()); - secret_manager->addStaticSecret(secret_config); ASSERT_EQ(secret_manager->findStaticTlsCertificateProvider("undefined"), nullptr); - ASSERT_NE(secret_manager->findStaticTlsCertificateProvider("abc.com"), nullptr); const std::string cert_pem = "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_cert.pem"; @@ -50,10 +46,11 @@ name: "abc.com" secret_manager->findStaticTlsCertificateProvider("abc.com")->secret()->privateKey()); } +// Validate that secret manager throws an exception when adding duplicated static TLS certificate +// secret. TEST_F(SecretManagerImplTest, DuplicateStaticTlsCertificateSecret) { envoy::api::v2::auth::Secret secret_config; - - std::string yaml = + const std::string yaml = R"EOF( name: "abc.com" tls_certificate: @@ -62,40 +59,31 @@ TEST_F(SecretManagerImplTest, DuplicateStaticTlsCertificateSecret) { private_key: filename: "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_key.pem" )EOF"; - MessageUtil::loadFromYaml(TestEnvironment::substitute(yaml), secret_config); - std::unique_ptr secret_manager(new SecretManagerImpl()); - secret_manager->addStaticSecret(secret_config); ASSERT_NE(secret_manager->findStaticTlsCertificateProvider("abc.com"), nullptr); - EXPECT_THROW_WITH_MESSAGE(secret_manager->addStaticSecret(secret_config), EnvoyException, "Duplicate static TlsCertificate secret name abc.com"); } +// Validate that secret manager adds static certificate validation context secret successfully. TEST_F(SecretManagerImplTest, CertificateValidationContextSecretLoadSuccess) { envoy::api::v2::auth::Secret secret_config; - - std::string yaml = + const std::string yaml = R"EOF( name: "abc.com" validation_context: trusted_ca: { filename: "{{ test_rundir }}/test/common/ssl/test_data/ca_cert.pem" } allow_expired_certificate: true )EOF"; - MessageUtil::loadFromYaml(TestEnvironment::substitute(yaml), secret_config); - std::unique_ptr secret_manager(new SecretManagerImpl()); - secret_manager->addStaticSecret(secret_config); ASSERT_EQ(secret_manager->findStaticCertificateValidationContextProvider("undefined"), nullptr); - ASSERT_NE(secret_manager->findStaticCertificateValidationContextProvider("abc.com"), nullptr); - const std::string cert_pem = "{{ test_rundir }}/test/common/ssl/test_data/ca_cert.pem"; EXPECT_EQ(TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(cert_pem)), secret_manager->findStaticCertificateValidationContextProvider("abc.com") @@ -103,33 +91,32 @@ TEST_F(SecretManagerImplTest, CertificateValidationContextSecretLoadSuccess) { ->caCert()); } +// Validate that secret manager throws an exception when adding duplicated static certificate +// validation context secret. TEST_F(SecretManagerImplTest, DuplicateStaticCertificateValidationContextSecret) { envoy::api::v2::auth::Secret secret_config; - - std::string yaml = + const std::string yaml = R"EOF( name: "abc.com" validation_context: trusted_ca: { filename: "{{ test_rundir }}/test/common/ssl/test_data/ca_cert.pem" } allow_expired_certificate: true )EOF"; - MessageUtil::loadFromYaml(TestEnvironment::substitute(yaml), secret_config); - std::unique_ptr secret_manager(new SecretManagerImpl()); - secret_manager->addStaticSecret(secret_config); ASSERT_NE(secret_manager->findStaticCertificateValidationContextProvider("abc.com"), nullptr); - EXPECT_THROW_WITH_MESSAGE(secret_manager->addStaticSecret(secret_config), EnvoyException, "Duplicate static CertificateValidationContext secret name abc.com"); } +// Validate that secret manager throws an exception when adding static secret of a type that is not +// supported. TEST_F(SecretManagerImplTest, NotImplementedException) { envoy::api::v2::auth::Secret secret_config; - std::string yaml = + const std::string yaml = R"EOF( name: "abc.com" session_ticket_keys: diff --git a/test/common/ssl/context_impl_test.cc b/test/common/ssl/context_impl_test.cc index a7720277e4f2..e418db454a69 100644 --- a/test/common/ssl/context_impl_test.cc +++ b/test/common/ssl/context_impl_test.cc @@ -418,10 +418,11 @@ TEST(ClientContextConfigImplTest, MultipleTlsCertificates) { "Multiple TLS certificates are not supported for client contexts"); } +// Validate that client context config with static TLS certificates is created successfully. TEST(ClientContextConfigImplTest, StaticTlsCertificates) { envoy::api::v2::auth::Secret secret_config; - std::string yaml = R"EOF( + const std::string yaml = R"EOF( name: "abc.com" tls_certificate: certificate_chain: @@ -451,10 +452,11 @@ name: "abc.com" client_context_config.tlsCertificate()->privateKey()); } +// Validate that client context config with static certificate validation context is created +// successfully. TEST(ClientContextConfigImplTest, StaticCertificateValidationContext) { envoy::api::v2::auth::Secret tls_certificate_secret_config; - - std::string tls_certificate_yaml = R"EOF( + const std::string tls_certificate_yaml = R"EOF( name: "abc.com" tls_certificate: certificate_chain: @@ -462,25 +464,19 @@ TEST(ClientContextConfigImplTest, StaticCertificateValidationContext) { private_key: filename: "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_key.pem" )EOF"; - MessageUtil::loadFromYaml(TestEnvironment::substitute(tls_certificate_yaml), tls_certificate_secret_config); - std::unique_ptr secret_manager(new Secret::SecretManagerImpl()); secret_manager->addStaticSecret(tls_certificate_secret_config); - envoy::api::v2::auth::Secret certificate_validation_context_secret_config; - - std::string certificate_validation_context_yaml = R"EOF( + const std::string certificate_validation_context_yaml = R"EOF( name: "def.com" validation_context: trusted_ca: { filename: "{{ test_rundir }}/test/common/ssl/test_data/ca_cert.pem" } allow_expired_certificate: true )EOF"; - MessageUtil::loadFromYaml(TestEnvironment::substitute(certificate_validation_context_yaml), certificate_validation_context_secret_config); - secret_manager->addStaticSecret(certificate_validation_context_secret_config); envoy::api::v2::auth::UpstreamTlsContext tls_context; @@ -491,7 +487,6 @@ TEST(ClientContextConfigImplTest, StaticCertificateValidationContext) { tls_context.mutable_common_tls_context() ->mutable_validation_context_sds_secret_config() ->set_name("def.com"); - ClientContextConfigImpl client_context_config(tls_context, *secret_manager.get()); const std::string cert_pem = "{{ test_rundir }}/test/common/ssl/test_data/ca_cert.pem"; @@ -499,10 +494,12 @@ TEST(ClientContextConfigImplTest, StaticCertificateValidationContext) { client_context_config.certificateValidationContext()->caCert()); } +// Validate that constructor of client context config throws an exception when static TLS +// certificate is missing. TEST(ClientContextConfigImplTest, MissingStaticSecretTlsCertificates) { envoy::api::v2::auth::Secret secret_config; - std::string yaml = R"EOF( + const std::string yaml = R"EOF( name: "abc.com" tls_certificate: certificate_chain: @@ -528,10 +525,11 @@ name: "abc.com" EnvoyException, "Static secret is not defined: missing"); } +// Validate that constructor of client context config throws an exception when static certificate +// validation context is missing. TEST(ClientContextConfigImplTest, MissingStaticCertificateValidationContext) { envoy::api::v2::auth::Secret tls_certificate_secret_config; - - std::string tls_certificate_yaml = R"EOF( + const std::string tls_certificate_yaml = R"EOF( name: "abc.com" tls_certificate: certificate_chain: @@ -539,25 +537,19 @@ TEST(ClientContextConfigImplTest, MissingStaticCertificateValidationContext) { private_key: filename: "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_key.pem" )EOF"; - MessageUtil::loadFromYaml(TestEnvironment::substitute(tls_certificate_yaml), tls_certificate_secret_config); - std::unique_ptr secret_manager(new Secret::SecretManagerImpl()); secret_manager->addStaticSecret(tls_certificate_secret_config); - envoy::api::v2::auth::Secret certificate_validation_context_secret_config; - - std::string certificate_validation_context_yaml = R"EOF( + const std::string certificate_validation_context_yaml = R"EOF( name: "def.com" validation_context: trusted_ca: { filename: "{{ test_rundir }}/test/common/ssl/test_data/ca_cert.pem" } allow_expired_certificate: true )EOF"; - MessageUtil::loadFromYaml(TestEnvironment::substitute(certificate_validation_context_yaml), certificate_validation_context_secret_config); - secret_manager->addStaticSecret(certificate_validation_context_secret_config); envoy::api::v2::auth::UpstreamTlsContext tls_context; @@ -568,7 +560,6 @@ TEST(ClientContextConfigImplTest, MissingStaticCertificateValidationContext) { tls_context.mutable_common_tls_context() ->mutable_validation_context_sds_secret_config() ->set_name("missing"); - EXPECT_THROW_WITH_MESSAGE( ClientContextConfigImpl client_context_config(tls_context, *secret_manager.get()), EnvoyException, "Unknown static certificate validation context: missing");