tls: fix issues in DefaultCertValidator::doVerifyCertChain and `Con…

…textImpl::verifyCertChain` (#16982) Signed-off-by: Tianyu Xia <[email protected]>
envoyproxy · Jul 15, 2021 · 6b575e3 · 6b575e3
1 parent db53365
commit 6b575e3
Show file tree

Hide file tree

Showing 3 changed files with 34 additions and 4 deletions.
diff --git a/source/extensions/transport_sockets/tls/cert_validator/default_validator.cc b/source/extensions/transport_sockets/tls/cert_validator/default_validator.cc
@@ -215,8 +215,15 @@ int DefaultCertValidator::doVerifyCertChain(
     }
   }
 
-  return allow_untrusted_certificate_ ? 1
-                                      : (validated != Envoy::Ssl::ClientValidationStatus::Failed);
+  // If `trusted_ca` exists, it is already verified in the code above. Thus, we just need to make
+  // sure the verification for other validation context configurations doesn't fail (i.e. either
+  // `NotValidated` or `Validated`). If `trusted_ca` doesn't exist, we will need to make sure other
+  // configurations are verified and the verification succeed.
+  int validation_status = verify_trusted_ca_
+                              ? validated != Envoy::Ssl::ClientValidationStatus::Failed
+                              : validated == Envoy::Ssl::ClientValidationStatus::Validated;
+
+  return allow_untrusted_certificate_ ? 1 : validation_status;
 }
 
 Envoy::Ssl::ClientValidationStatus DefaultCertValidator::verifyCertificate(

diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc
@@ -1168,16 +1168,20 @@ bool TlsContext::isCipherEnabled(uint16_t cipher_id, uint16_t client_version) {
 bool ContextImpl::verifyCertChain(X509& leaf_cert, STACK_OF(X509) & intermediates,
                                   std::string& error_details) {
   bssl::UniquePtr<X509_STORE_CTX> ctx(X509_STORE_CTX_new());
+
+  ASSERT(!tls_contexts_.empty());
   // It doesn't matter which SSL context is used, because they share the same
   // cert validation config.
-  X509_STORE* store = SSL_CTX_get_cert_store(tls_contexts_[0].ssl_ctx_.get());
+  const SSL_CTX* ssl_ctx = tls_contexts_[0].ssl_ctx_.get();
+  X509_STORE* store = SSL_CTX_get_cert_store(ssl_ctx);
   if (!X509_STORE_CTX_init(ctx.get(), store, &leaf_cert, &intermediates)) {
     error_details = "Failed to verify certificate chain: X509_STORE_CTX_init";
     return false;
   }
 
   int res = cert_validator_->doVerifyCertChain(ctx.get(), nullptr, leaf_cert, nullptr);
-  if (res <= 0) {
+  // If |SSL_VERIFY_NONE|, the error is non-fatal, but we keep the error details.
+  if (res <= 0 && SSL_CTX_get_verify_mode(ssl_ctx) != SSL_VERIFY_NONE) {
     const int n = X509_STORE_CTX_get_error(ctx.get());
     const int depth = X509_STORE_CTX_get_error_depth(ctx.get());
     error_details = absl::StrCat("X509_verify_cert: certificate verification error at depth ",

diff --git a/test/extensions/transport_sockets/tls/cert_validator/default_validator_test.cc b/test/extensions/transport_sockets/tls/cert_validator/default_validator_test.cc
@@ -146,6 +146,25 @@ TEST(DefaultCertValidatorTest, TestCertificateVerificationWithSANMatcher) {
   EXPECT_EQ(stats.fail_verify_san_.value(), 1);
 }
 
+TEST(DefaultCertValidatorTest, TestCertificateVerificationWithNoValidationContext) {
+  Stats::TestUtil::TestStore test_store;
+  SslStats stats = generateSslStats(test_store);
+  // Create the default validator object.
+  auto default_validator =
+      std::make_unique<Extensions::TransportSockets::Tls::DefaultCertValidator>(
+          /*CertificateValidationContextConfig=*/nullptr, stats,
+          Event::GlobalTimeSystem().timeSystem());
+
+  EXPECT_EQ(default_validator->verifyCertificate(/*cert=*/nullptr, /*verify_san_list=*/{},
+                                                 /*subject_alt_name_matchers=*/{}),
+            Envoy::Ssl::ClientValidationStatus::NotValidated);
+  X509 cert = {};
+  EXPECT_EQ(default_validator->doVerifyCertChain(/*store_ctx=*/nullptr,
+                                                 /*ssl_extended_info=*/nullptr, /*leaf_cert=*/cert,
+                                                 /*transport_socket_options=*/nullptr),
+            0);
+}
+
 } // namespace Tls
 } // namespace TransportSockets
 } // namespace Extensions