envoyproxy · mattklein123 · Mar 22, 2021 · Mar 16, 2021 · Mar 16, 2021 · Mar 16, 2021
@@ -38,7 +38,13 @@ SPIFFEValidator::SPIFFEValidator(const Envoy::Ssl::CertificateValidationContextC
                                          ProtobufWkt::Struct(),
                                          ProtobufMessage::getStrictValidationVisitor(), message);
 
-  auto size = message.trust_domains().size();
+  if (!config->subjectAltNameMatchers().empty()) {
+    for (const auto& matcher : config->subjectAltNameMatchers()) {
+      subject_alt_name_matchers_.push_back(Matchers::StringMatcherImpl(matcher));
+    }
+  }
+
+  const auto size = message.trust_domains().size();
   trust_bundle_stores_.reserve(size);
   for (auto& domain : message.trust_domains()) {
     if (trust_bundle_stores_.find(domain.name()) != trust_bundle_stores_.end()) {
@@ -152,15 +158,25 @@ int SPIFFEValidator::doVerifyCertChain(X509_STORE_CTX* store_ctx,
     X509_STORE_CTX_set_verify_cb(store_ctx, CertValidatorUtil::ignoreCertificateExpirationCallback);
   }
   auto ret = X509_verify_cert(store_ctx);
-  if (ssl_extended_info) {
-    ssl_extended_info->setCertificateValidationStatus(
-        ret == 1 ? Envoy::Ssl::ClientValidationStatus::Validated
-                 : Envoy::Ssl::ClientValidationStatus::Failed);
-  }
   if (!ret) {
+    if (ssl_extended_info) {
+      ssl_extended_info->setCertificateValidationStatus(Envoy::Ssl::ClientValidationStatus::Failed);
+    }
     stats_.fail_verify_error_.inc();
+    return 0;
   }
-  return ret;
+
+  // Do SAN matching.
+  const bool san_match = subject_alt_name_matchers_.empty() ? true : matchSubjectAltName(leaf_cert);
+  if (!san_match) {
+    stats_.fail_verify_error_.inc();
+  }
+  if (ssl_extended_info) {
+    ssl_extended_info->setCertificateValidationStatus(
+        san_match ? Envoy::Ssl::ClientValidationStatus::Validated
+                  : Envoy::Ssl::ClientValidationStatus::Failed);
+  }
+  return san_match;
 }
 
 X509_STORE* SPIFFEValidator::getTrustBundleStore(X509* leaf_cert) {
@@ -193,15 +209,39 @@ X509_STORE* SPIFFEValidator::getTrustBundleStore(X509* leaf_cert) {
 bool SPIFFEValidator::certificatePrecheck(X509* leaf_cert) {
   // Check basic constrains and key usage.
   // https://github.com/spiffe/spiffe/blob/master/standards/X509-SVID.md#52-leaf-validation
-  auto ext = X509_get_extension_flags(leaf_cert);
+  const auto ext = X509_get_extension_flags(leaf_cert);
   if (ext & EXFLAG_CA) {
     return false;
   }
 
-  auto us = X509_get_key_usage(leaf_cert);
+  const auto us = X509_get_key_usage(leaf_cert);
   return (us & (KU_CRL_SIGN | KU_KEY_CERT_SIGN)) == 0;
 }
 
+bool SPIFFEValidator::matchSubjectAltName(X509& leaf_cert) {
+  bssl::UniquePtr<GENERAL_NAMES> san_names(static_cast<GENERAL_NAMES*>(
+      X509_get_ext_d2i(&leaf_cert, NID_subject_alt_name, nullptr, nullptr)));
+  // We must not have san_names == nullptr here because this function is called after the
+  // SPIFFE cert validation algorithm succeeded, which requires exactly one URI SAN in the leaf
+  // cert.
+  ASSERT(san_names != nullptr,
+         "san_names should have at least one name after SPIFFE cert validation");
+
+  // Only match against URI SAN since SPIFFE specification does not restrict values in other SAN
+  // types. See the discussion: https://github.com/envoyproxy/envoy/issues/15392
+  for (const GENERAL_NAME* general_name : san_names.get()) {
+    if (general_name->type == GEN_URI) {
+      const std::string san = Utility::generalNameAsString(general_name);
+      for (const auto& config_san_matcher : subject_alt_name_matchers_) {
+        if (config_san_matcher.match(san)) {
+          return true;
+        }
+      }
+    }
+  }
+  return false;
+}
+
 std::string SPIFFEValidator::extractTrustDomain(const std::string& san) {
   static const std::string prefix = "spiffe://";
   if (!absl::StartsWith(san, prefix)) {

@@ -62,10 +62,13 @@ class SPIFFEValidator : public CertValidator {
     return trust_bundle_stores_;
   };
 
+  bool matchSubjectAltName(X509& leaf_cert);
+
 private:
   bool allow_expired_certificate_{false};
   std::vector<bssl::UniquePtr<X509>> ca_certs_;
   std::string ca_file_name_;
+  std::vector<Matchers::StringMatcherImpl> subject_alt_name_matchers_{};
   absl::flat_hash_map<std::string, X509StorePtr> trust_bundle_stores_;
 
   SslStats& stats_;

@@ -146,6 +146,11 @@ std::string Utility::generalNameAsString(const GENERAL_NAME* general_name) {
     san.assign(reinterpret_cast<const char*>(ASN1_STRING_data(str)), ASN1_STRING_length(str));
     break;
   }
+  case GEN_EMAIL: {
+    ASN1_STRING* str = general_name->d.rfc822Name;
+    san.assign(reinterpret_cast<const char*>(ASN1_STRING_data(str)), ASN1_STRING_length(str));
+    break;
+  }
   case GEN_IPADD: {
     if (general_name->d.ip->length == 4) {
       sockaddr_in sin;

diff --git a/test/config/utility.cc b/test/config/utility.cc
@@ -1141,6 +1141,10 @@ void ConfigHelper::initializeTls(
           "test/config/integration/certs/server_ecdsa_ocsp_resp.der"));
     }
   }
+  if (!options.san_matchers_.empty()) {
+    *validation_context->mutable_match_subject_alt_names() = {options.san_matchers_.begin(),
+                                                              options.san_matchers_.end()};
+  }
 }
 
 void ConfigHelper::renameListener(const std::string& name) {

diff --git a/test/config/utility.h b/test/config/utility.h
@@ -79,6 +79,12 @@ class ConfigHelper {
       return *this;
     }
 
+    ServerSslOptions&
+    setSanMatchers(std::vector<envoy::type::matcher::v3::StringMatcher> san_matchers) {
+      san_matchers_ = san_matchers;
+      return *this;
+    }
+
     bool allow_expired_certificate_{};
     envoy::config::core::v3::TypedExtensionConfig* custom_validator_config_;
     bool rsa_cert_{true};
@@ -88,6 +94,7 @@ class ConfigHelper {
     bool ocsp_staple_required_{false};
     bool tlsv1_3_{false};
     bool expect_client_ecdsa_cert_{false};
+    std::vector<envoy::type::matcher::v3::StringMatcher> san_matchers_{};
   };
 
   // Set up basic config, using the specified IpVersion for all connections: listeners, upstream,

@@ -17,6 +17,7 @@ void SslSPIFFECertValidatorIntegrationTest::initialize() {
                                   .setTlsV13(true)
                                   .setRsaCertOcspStaple(false)
                                   .setCustomValidatorConfig(custom_validator_config_)
+                                  .setSanMatchers(san_matchers_)
                                   .setAllowExpiredCertificate(allow_expired_cert_));
   HttpIntegrationTest::initialize();
 
@@ -106,6 +107,65 @@ name: envoy.tls.cert_validator.spiffe
   checkVerifyErrorCouter(0);
 }
 
+// clientcert.pem has "spiffe://lyft.com/frontend-team" URI SAN, so this case should be accepted.
+TEST_P(SslSPIFFECertValidatorIntegrationTest, ServerRsaSPIFFEValidatorSANMatch) {
+  auto typed_conf = new envoy::config::core::v3::TypedExtensionConfig();
+  TestUtility::loadFromYaml(TestEnvironment::substitute(R"EOF(
+name: envoy.tls.cert_validator.spiffe
+typed_config:
+  "@type": type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.SPIFFECertValidatorConfig
+  trust_domains:
+    - name: lyft.com
+      trust_bundle:
+        filename: "{{ test_rundir }}/test/config/integration/certs/cacert.pem"
+  )EOF"),
+                            *typed_conf);
+  custom_validator_config_ = typed_conf;
+
+  envoy::type::matcher::v3::StringMatcher matcher;
+  matcher.set_prefix("spiffe://lyft.com/");
+  san_matchers_ = {matcher};
+
+  ConnectionCreationFunction creator = [&]() -> Network::ClientConnectionPtr {
+    return makeSslClientConnection({});
+  };
+  testRouterRequestAndResponseWithBody(1024, 512, false, false, &creator);
+  checkVerifyErrorCouter(0);
+}
+
+// clientcert.pem has "spiffe://lyft.com/frontend-team" URI SAN, so this case should be rejected.
+TEST_P(SslSPIFFECertValidatorIntegrationTest, ServerRsaSPIFFEValidatorSANNotMatch) {
+  auto typed_conf = new envoy::config::core::v3::TypedExtensionConfig();
+  TestUtility::loadFromYaml(TestEnvironment::substitute(R"EOF(
+name: envoy.tls.cert_validator.spiffe
+typed_config:
+  "@type": type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.SPIFFECertValidatorConfig
+  trust_domains:
+    - name: lyft.com
+      trust_bundle:
+        filename: "{{ test_rundir }}/test/config/integration/certs/cacert.pem"
+  )EOF"),
+                            *typed_conf);
+  custom_validator_config_ = typed_conf;
+
+  envoy::type::matcher::v3::StringMatcher matcher;
+  matcher.set_prefix("spiffe://example.com/");
+  // The cert has "DNS.1 = lyft.com" but SPIFFE validator must ignore SAN types other than URI.
+  matcher.set_prefix("www.lyft.com");
+  san_matchers_ = {matcher};
+  initialize();
+  auto conn = makeSslClientConnection({});
+  if (tls_version_ == envoy::extensions::transport_sockets::tls::v3::TlsParameters::TLSv1_2) {
+    auto codec = makeRawHttpConnection(std::move(conn), absl::nullopt);
+    EXPECT_FALSE(codec->connected());
+  } else {
+    auto codec = makeHttpConnection(std::move(conn));
+    ASSERT_TRUE(codec->waitForDisconnect());
+    codec->close();
+  }
+  checkVerifyErrorCouter(1);
+}
+
 // Client certificate has expired and the config does NOT allow expired certificates, so this case
 // should be rejected.
 TEST_P(SslSPIFFECertValidatorIntegrationTest, ServerRsaSPIFFEValidatorExpiredAndRejected) {

@@ -42,6 +42,7 @@ class SslSPIFFECertValidatorIntegrationTest
   bool allow_expired_cert_{};
   envoy::config::core::v3::TypedExtensionConfig* custom_validator_config_{nullptr};
   std::unique_ptr<ContextManager> context_manager_;
+  std::vector<envoy::type::matcher::v3::StringMatcher> san_matchers_;
   const envoy::extensions::transport_sockets::tls::v3::TlsParameters::TlsProtocol tls_version_{
       std::get<1>(GetParam())};
 };