Various small cleanups to PlatformBridgeCertValidator (#2700)

Remove unused next_iteration_callback_ member.
Remove unused config_ member.
Make some members const.
Use hostname instead of host_name.
Pass in the list of subject alt names instead of passing in the transport socket options.

Risk Level: Low
Testing: No behavior change
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Ryan Hamilton rch@google.com

Author: RyanTheOptimist

library/common/extensions/cert_validator/platform_bridge/platform_bridge_cert_validator.cc

@@ -14,15 +14,14 @@ namespace Tls {
 PlatformBridgeCertValidator::PlatformBridgeCertValidator(
     const Envoy::Ssl::CertificateValidationContextConfig* config, SslStats& stats,
     const envoy_cert_validator* platform_validator)
-    : config_(config), stats_(stats), platform_validator_(platform_validator) {
+    : allow_untrusted_certificate_(config != nullptr &&
+                                   config->trustChainVerification() ==
+                                       envoy::extensions::transport_sockets::tls::v3::
+                                           CertificateValidationContext::ACCEPT_UNTRUSTED),
+      platform_validator_(platform_validator), stats_(stats) {
   ENVOY_BUG(config != nullptr && config->caCert().empty() &&
                 config->certificateRevocationList().empty(),
             "Invalid certificate validation context config.");
-  if (config_ != nullptr) {
-    allow_untrusted_certificate_ = config_->trustChainVerification() ==
-                                   envoy::extensions::transport_sockets::tls::v3::
-                                       CertificateValidationContext::ACCEPT_UNTRUSTED;
-  }
 }
 
 PlatformBridgeCertValidator::~PlatformBridgeCertValidator() {
@@ -34,17 +33,12 @@ PlatformBridgeCertValidator::~PlatformBridgeCertValidator() {
   }
 }
 
-int PlatformBridgeCertValidator::initializeSslContexts(std::vector<SSL_CTX*> /*contexts*/,
-                                                       bool /*handshaker_provides_certificates*/) {
-  return SSL_VERIFY_PEER;
-}
-
 ValidationResults PlatformBridgeCertValidator::doVerifyCertChain(
     STACK_OF(X509) & cert_chain, Ssl::ValidateResultCallbackPtr callback,
     Ssl::SslExtendedSocketInfo* ssl_extended_info,
     const Network::TransportSocketOptionsConstSharedPtr& transport_socket_options,
     SSL_CTX& /*ssl_ctx*/, const CertValidator::ExtraValidationContext& /*validation_context*/,
-    bool is_server, absl::string_view host_name) {
+    bool is_server, absl::string_view hostname) {
   ASSERT(!is_server);
   if (sk_X509_num(&cert_chain) == 0) {
     if (ssl_extended_info) {
@@ -78,10 +72,18 @@ ValidationResults PlatformBridgeCertValidator::doVerifyCertChain(
       !transport_socket_options->verifySubjectAltNameListOverride().empty()) {
     host = transport_socket_options->verifySubjectAltNameListOverride()[0];
   } else {
-    host = host_name;
+    host = hostname;
   }
+
+  std::vector<std::string> subject_alt_names;
+  if (transport_socket_options != nullptr) {
+    subject_alt_names = transport_socket_options->verifySubjectAltNameListOverride();
+  } else {
+    subject_alt_names = {std::string(hostname)};
+  }
+
   auto validation = std::make_unique<PendingValidation>(
-      *this, std::move(certs), host, std::move(transport_socket_options), std::move(callback));
+      *this, std::move(certs), host, std::move(subject_alt_names), std::move(callback));
   PendingValidation* validation_ptr = validation.get();
   validations_.insert(std::move(validation));
   std::thread verification_thread(&PendingValidation::verifyCertsByPlatform, validation_ptr);
@@ -91,16 +93,16 @@ ValidationResults PlatformBridgeCertValidator::doVerifyCertChain(
 }
 
 void PlatformBridgeCertValidator::verifyCertChainByPlatform(
-    std::vector<envoy_data>& cert_chain, const std::string& host_name,
+    const std::vector<envoy_data>& cert_chain, const std::string& hostname,
     const std::vector<std::string>& subject_alt_names, PendingValidation& pending_validation) {
   ASSERT(!cert_chain.empty());
-  ENVOY_LOG(trace, "Start verifyCertChainByPlatform for host {}", host_name);
+  ENVOY_LOG(trace, "Start verifyCertChainByPlatform for host {}", hostname);
   // This is running in a stand alone thread other than the engine thread.
   envoy_data leaf_cert_der = cert_chain[0];
   bssl::UniquePtr<X509> leaf_cert(d2i_X509(
       nullptr, const_cast<const unsigned char**>(&leaf_cert_der.bytes), leaf_cert_der.length));
   envoy_cert_validation_result result =
-      platform_validator_->validate_cert(cert_chain.data(), cert_chain.size(), host_name.c_str());
+      platform_validator_->validate_cert(cert_chain.data(), cert_chain.size(), hostname.c_str());
   bool success = result.result == ENVOY_SUCCESS;
   if (!success) {
     ENVOY_LOG(debug, result.error_details);
@@ -126,20 +128,15 @@ void PlatformBridgeCertValidator::verifyCertChainByPlatform(
 }
 
 void PlatformBridgeCertValidator::PendingValidation::verifyCertsByPlatform() {
-  parent_.verifyCertChainByPlatform(
-      certs_, host_name_,
-      (transport_socket_options_ != nullptr
-           ? transport_socket_options_->verifySubjectAltNameListOverride()
-           : std::vector<std::string>{host_name_}),
-      *this);
+  parent_.verifyCertChainByPlatform(certs_, hostname_, subject_alt_names_, *this);
 }
 
 void PlatformBridgeCertValidator::PendingValidation::postVerifyResultAndCleanUp(
     bool success, absl::string_view error_details, uint8_t tls_alert,
     OptRef<Stats::Counter> error_counter) {
   ENVOY_LOG(trace,
             "Finished platform cert validation for {}, post result callback to network thread",
-            host_name_);
+            hostname_);
 
   if (parent_.platform_validator_->validation_cleanup) {
     parent_.platform_validator_->validation_cleanup();
@@ -153,7 +150,7 @@ void PlatformBridgeCertValidator::PendingValidation::postVerifyResultAndCleanUp(
     if (weak_alive_indicator.expired()) {
       return;
     }
-    ENVOY_LOG(trace, "Got validation result for {} from platform", host_name_);
+    ENVOY_LOG(trace, "Got validation result for {} from platform", hostname_);
     parent_.validation_threads_[thread_id].join();
     parent_.validation_threads_.erase(thread_id);
     if (error_counter.has_value()) {

library/common/extensions/cert_validator/platform_bridge/platform_bridge_cert_validator.h

@@ -56,53 +56,51 @@ class PlatformBridgeCertValidator : public CertValidator, Logger::Loggable<Logge
                     const Network::TransportSocketOptionsConstSharedPtr& transport_socket_options,
                     SSL_CTX& ssl_ctx,
                     const CertValidator::ExtraValidationContext& validation_context, bool is_server,
-                    absl::string_view host_name) override;
-  // As CA path will not be configured, make sure the return value won’t be SSL_VERIFY_NONE because
-  // of that, so that doVerifyCertChain() will be called from the TLS stack.
-  int initializeSslContexts(std::vector<SSL_CTX*> contexts,
-                            bool handshaker_provides_certificates) override;
+                    absl::string_view hostname) override;
+  // Returns SSL_VERIFY_PEER so that doVerifyCertChain() will be called from the TLS stack.
+  int initializeSslContexts(std::vector<SSL_CTX*> /*contexts*/,
+                            bool /*handshaker_provides_certificates*/) override {
+    return SSL_VERIFY_PEER;
+  }
 
 private:
   class PendingValidation {
   public:
     PendingValidation(PlatformBridgeCertValidator& parent, std::vector<envoy_data> certs,
-                      absl::string_view host_name,
-                      const Network::TransportSocketOptionsConstSharedPtr transport_socket_options,
+                      absl::string_view hostname, std::vector<std::string> subject_alt_names,
                       Ssl::ValidateResultCallbackPtr result_callback)
-        : parent_(parent), certs_(std::move(certs)), host_name_(host_name),
-          result_callback_(std::move(result_callback)),
-          transport_socket_options_(std::move(transport_socket_options)) {}
+        : parent_(parent), certs_(std::move(certs)), hostname_(hostname),
+          subject_alt_names_(std::move(subject_alt_names)),
+          result_callback_(std::move(result_callback)) {}
 
     // Ensure that this class is never moved or copied to guarantee pointer stability.
     PendingValidation(const PendingValidation&) = delete;
     PendingValidation(PendingValidation&&) = delete;
 
+    // Calls into platform APIs in a stand-alone thread to verify the given certs.
+    // Once the validation is done, the result will be posted back to the current
+    // thread to trigger callback and update verify stats.
     void verifyCertsByPlatform();
 
     void postVerifyResultAndCleanUp(bool success, absl::string_view error_details,
                                     uint8_t tls_alert, OptRef<Stats::Counter> error_counter);
 
   private:
-    Event::SchedulableCallbackPtr next_iteration_callback_;
     PlatformBridgeCertValidator& parent_;
-    std::vector<envoy_data> certs_;
-    std::string host_name_;
+    const std::vector<envoy_data> certs_;
+    const std::string hostname_;
+    const std::vector<std::string> subject_alt_names_;
     Ssl::ValidateResultCallbackPtr result_callback_;
-    const Network::TransportSocketOptionsConstSharedPtr transport_socket_options_;
   };
 
-  // Calls into platform APIs in a stand-alone thread to verify the given certs.
-  // Once the validation is done, the result will be posted back to the current
-  // thread to trigger callback and update verify stats.
-  void verifyCertChainByPlatform(std::vector<envoy_data>& cert_chain, const std::string& host_name,
+  void verifyCertChainByPlatform(const std::vector<envoy_data>& cert_chain,
+                                 const std::string& hostname,
                                  const std::vector<std::string>& subject_alt_names,
                                  PendingValidation& pending_validation);
 
-  const Envoy::Ssl::CertificateValidationContextConfig* config_;
-  SslStats& stats_;
-  bool allow_untrusted_certificate_ = false;
-  // latches the platform extension API.
+  const bool allow_untrusted_certificate_;
   const envoy_cert_validator* platform_validator_;
+  SslStats& stats_;
   absl::flat_hash_map<std::thread::id, std::thread> validation_threads_;
   absl::flat_hash_set<std::unique_ptr<PendingValidation>> validations_;
   std::shared_ptr<size_t> alive_indicator_{new size_t(1)};