TLS: Allow specification of both typed and non-typed san matchers in …

…config (#21170)

Backport of #20529

Signed-off-by: Pradeep Rao <pcrao@google.com>

api/envoy/extensions/transport_sockets/tls/v3/common.proto

@@ -449,8 +449,12 @@ message CertificateValidationContext {
   //   <envoy_v3_api_field_extensions.transport_sockets.tls.v3.CertificateValidationContext.trusted_ca>`.
   repeated SubjectAltNameMatcher match_typed_subject_alt_names = 15;
 
-  // This field is deprecated in favor of ref:`match_typed_subject_alt_names
+  // This field is deprecated in favor of
+  // :ref:`match_typed_subject_alt_names
+  // <envoy_v3_api_field_extensions.transport_sockets.tls.v3.CertificateValidationContext.match_typed_subject_alt_names>`.
+  // Note that if both this field and :ref:`match_typed_subject_alt_names
   // <envoy_v3_api_field_extensions.transport_sockets.tls.v3.CertificateValidationContext.match_typed_subject_alt_names>`
+  // are specified, the former (deprecated field) is ignored.
   repeated type.matcher.v3.StringMatcher match_subject_alt_names = 9
       [deprecated = true, (envoy.annotations.deprecated_at_minor_version) = "3.0"];
 

docs/root/version_history/current.rst

@@ -9,6 +9,8 @@ Minor Behavior Changes
 ----------------------
 *Changes that may cause incompatibilities for some users, but should not for most*
 
+* tls: if both :ref:`match_subject_alt_names <envoy_v3_api_field_extensions.transport_sockets.tls.v3.CertificateValidationContext.match_subject_alt_names>` and :ref:`match_typed_subject_alt_names <envoy_v3_api_field_extensions.transport_sockets.tls.v3.CertificateValidationContext.match_typed_subject_alt_names>` are specified, the former (deprecated) field is ignored. Previously, setting both fields would result in an error.
+
 Bug Fixes
 ---------
 *Changes expected to improve the state of the world and are unlikely to have negative effects*

source/common/ssl/certificate_validation_context_config_impl.cc

@@ -10,6 +10,8 @@
 #include "source/common/common/logger.h"
 #include "source/common/config/datasource.h"
 
+#include "spdlog/spdlog.h"
+
 namespace Envoy {
 namespace Ssl {
 
@@ -56,14 +58,21 @@ CertificateValidationContextConfigImpl::CertificateValidationContextConfigImpl(
 std::vector<envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher>
 CertificateValidationContextConfigImpl::getSubjectAltNameMatchers(
     const envoy::extensions::transport_sockets::tls::v3::CertificateValidationContext& config) {
-  if (!config.match_typed_subject_alt_names().empty() &&
-      !config.match_subject_alt_names().empty()) {
-    throw EnvoyException("SAN-based verification using both match_typed_subject_alt_names and "
-                         "the deprecated match_subject_alt_names is not allowed");
-  }
   std::vector<envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher>
       subject_alt_name_matchers(config.match_typed_subject_alt_names().begin(),
                                 config.match_typed_subject_alt_names().end());
+  // If typed subject alt name matchers are provided in the config, don't check
+  // for the deprecated non-typed field.
+  if (!subject_alt_name_matchers.empty()) {
+    // Warn that we're ignoring the deprecated san matcher field, if both are
+    // specified.
+    if (!config.match_subject_alt_names().empty()) {
+      ENVOY_LOG_MISC(warn,
+                     "Ignoring match_subject_alt_names as match_typed_subject_alt_names is also "
+                     "specified, and the former is deprecated.");
+    }
+    return subject_alt_name_matchers;
+  }
   // Handle deprecated string type san matchers without san type specified, by
   // creating a matcher for each supported type.
   for (const envoy::type::matcher::v3::StringMatcher& matcher : config.match_subject_alt_names()) {

test/extensions/transport_sockets/tls/context_impl_test.cc

@@ -1932,8 +1932,8 @@ TEST_F(ServerContextConfigImplTest, PrivateKeyMethodLoadFailureBothKeyAndMethod)
       "Certificate configuration can't have both private_key and private_key_provider");
 }
 
-// Test that we don't allow specification of both typed and untyped matchers for
-// sans.
+// Test that if both typed and untyped matchers for sans are specified, we
+// ignore the untyped matchers.
 TEST_F(ServerContextConfigImplTest, DeprecatedSanMatcher) {
   envoy::extensions::transport_sockets::tls::v3::DownstreamTlsContext tls_context;
   NiceMock<Ssl::MockContextManager> context_manager;
@@ -1943,22 +1943,42 @@ TEST_F(ServerContextConfigImplTest, DeprecatedSanMatcher) {
   const std::string yaml =
       R"EOF(
       common_tls_context:
+        tls_certificates:
+        - certificate_chain:
+            filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/selfsigned_cert.pem"
+          private_key:
+            filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/selfsigned_key.pem"
         validation_context:
           trusted_ca: { filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ca_cert.pem" }
           allow_expired_certificate: true
           match_typed_subject_alt_names:
           - san_type: DNS
             matcher:
-              exact: "foo.example"
+              exact: "foo1.example"
           match_subject_alt_names:
-            exact: "foo.example"
+            exact: "foo2.example"
       )EOF";
   TestUtility::loadFromYaml(TestEnvironment::substitute(yaml), tls_context);
 
-  EXPECT_THROW_WITH_MESSAGE(
-      ServerContextConfigImpl server_context_config(tls_context, factory_context_), EnvoyException,
-      "SAN-based verification using both match_typed_subject_alt_names and "
-      "the deprecated match_subject_alt_names is not allowed");
+  absl::optional<ServerContextConfigImpl> server_context_config;
+  EXPECT_LOG_CONTAINS("warning",
+                      "Ignoring match_subject_alt_names as match_typed_subject_alt_names is also "
+                      "specified, and the former is deprecated.",
+                      server_context_config.emplace(tls_context, factory_context_));
+  EXPECT_EQ(
+      server_context_config.value().certificateValidationContext()->subjectAltNameMatchers().size(),
+      1);
+  EXPECT_EQ(server_context_config.value()
+                .certificateValidationContext()
+                ->subjectAltNameMatchers()[0]
+                .san_type(),
+            envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::DNS);
+  EXPECT_EQ(server_context_config.value()
+                .certificateValidationContext()
+                ->subjectAltNameMatchers()[0]
+                .matcher()
+                .exact(),
+            "foo1.example");
 }
 
 TEST_F(ServerContextConfigImplTest, Pkcs12LoadFailureBothPkcs12AndMethod) {