Fix SAN matching on terminating gateways

Fixes issue: #20360 A regression was introduced in #19954 where the SAN validation matching was reduced from 4 potential types down to just the URI. Terminating gateways will need to match on many fields depending on user configuration, since they make egress calls outside of the cluster. Having more than one matcher behaves like an OR operation, where any match is sufficient to pass the certificate validation. To maintain backwards compatibility with the old untyped `match_subject_alt_names` Envoy behavior, we should match on all 4 enum types. https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/transport_sockets/tls/v3/common.proto#enum-extensions-transport-sockets-tls-v3-subjectaltnamematcher-santype
hashicorp · Jan 31, 2024 · 434103c · 434103c
1 parent 890332c
commit 434103c
Show file tree

Hide file tree

Showing 10 changed files with 98 additions and 20 deletions.
diff --git a/.changelog/20417.txt b/.changelog/20417.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+connect: Fix regression with SAN matching on terminating gateways [GH-20360](https://github.com/hashicorp/consul/issues/20360)
+```
diff --git a/agent/xds/clusters.go b/agent/xds/clusters.go
@@ -481,7 +481,7 @@ func makeMTLSTransportSocket(cfgSnap *proxycfg.ConfigSnapshot, uid proxycfg.Upst
 		cfgSnap.RootPEMs(),
 		makeTLSParametersFromProxyTLSConfig(cfgSnap.MeshConfigTLSOutgoing()),
 	)
-	err := injectSANMatcher(commonTLSContext, spiffeID.URI().String())
+	err := injectSANMatcher(commonTLSContext, false, spiffeID.URI().String())
 	if err != nil {
 		return nil, fmt.Errorf("failed to inject SAN matcher rules for cluster %q: %v", sni, err)
 	}
@@ -875,7 +875,7 @@ func (s *ResourceGenerator) injectGatewayServiceAddons(cfgSnap *proxycfg.ConfigS
 			}
 			if mapping.SNI != "" {
 				tlsContext.Sni = mapping.SNI
-				if err := injectSANMatcher(tlsContext.CommonTlsContext, mapping.SNI); err != nil {
+				if err := injectSANMatcher(tlsContext.CommonTlsContext, true, mapping.SNI); err != nil {
 					return fmt.Errorf("failed to inject SNI matcher into TLS context: %v", err)
 				}
 			}
@@ -904,7 +904,7 @@ func (s *ResourceGenerator) injectGatewayDestinationAddons(cfgSnap *proxycfg.Con
 			}
 			if mapping.SNI != "" {
 				tlsContext.Sni = mapping.SNI
-				if err := injectSANMatcher(tlsContext.CommonTlsContext, mapping.SNI); err != nil {
+				if err := injectSANMatcher(tlsContext.CommonTlsContext, true, mapping.SNI); err != nil {
 					return fmt.Errorf("failed to inject SNI matcher into TLS context: %v", err)
 				}
 			}
@@ -1226,7 +1226,7 @@ func (s *ResourceGenerator) makeUpstreamClusterForPeerService(
 		rootPEMs,
 		makeTLSParametersFromProxyTLSConfig(cfgSnap.MeshConfigTLSOutgoing()),
 	)
-	err = injectSANMatcher(commonTLSContext, peerMeta.SpiffeID...)
+	err = injectSANMatcher(commonTLSContext, false, peerMeta.SpiffeID...)
 	if err != nil {
 		return nil, fmt.Errorf("failed to inject SAN matcher rules for cluster %q: %v", clusterName, err)
 	}
@@ -1329,7 +1329,7 @@ func (s *ResourceGenerator) makeUpstreamClusterForPreparedQuery(upstream structs
 		cfgSnap.RootPEMs(),
 		makeTLSParametersFromProxyTLSConfig(cfgSnap.MeshConfigTLSOutgoing()),
 	)
-	err = injectSANMatcher(commonTLSContext, spiffeIDs...)
+	err = injectSANMatcher(commonTLSContext, false, spiffeIDs...)
 	if err != nil {
 		return nil, fmt.Errorf("failed to inject SAN matcher rules for cluster %q: %v", sni, err)
 	}
@@ -1609,7 +1609,7 @@ func (s *ResourceGenerator) makeExportedUpstreamClustersForMeshGateway(cfgSnap *
 }
 
 // injectSANMatcher updates a TLS context so that it verifies the upstream SAN.
-func injectSANMatcher(tlsContext *envoy_tls_v3.CommonTlsContext, matchStrings ...string) error {
+func injectSANMatcher(tlsContext *envoy_tls_v3.CommonTlsContext, terminatingEgress bool, matchStrings ...string) error {
 	if tlsContext == nil {
 		return fmt.Errorf("invalid type: expected CommonTlsContext_ValidationContext not to be nil")
 	}
@@ -1620,16 +1620,37 @@ func injectSANMatcher(tlsContext *envoy_tls_v3.CommonTlsContext, matchStrings ..
 			tlsContext.ValidationContextType)
 	}
 
+	// All mesh services should match by URI
+	types := []envoy_tls_v3.SubjectAltNameMatcher_SanType{
+		envoy_tls_v3.SubjectAltNameMatcher_URI,
+	}
+	if terminatingEgress {
+		// Terminating gateways will need to match on many fields depending on user configuration,
+		// since they make egress calls outside of the cluster. Having more than one matcher behaves
+		// like an OR operation, where any match is sufficient to pass the certificate validation.
+		// To maintain backwards compatibility with the old untyped `match_subject_alt_names` behavior,
+		// we should match on all 4 enum types.
+		// https://github.com/hashicorp/consul/issues/20360
+		// https://github.com/envoyproxy/envoy/pull/18628/files#diff-cf088136dc052ddf1762fb3c96c0e8de472f3031f288e7e300558e6e72c8e129R69-R75
+		types = []envoy_tls_v3.SubjectAltNameMatcher_SanType{
+			envoy_tls_v3.SubjectAltNameMatcher_URI,
+			envoy_tls_v3.SubjectAltNameMatcher_DNS,
+			envoy_tls_v3.SubjectAltNameMatcher_EMAIL,
+			envoy_tls_v3.SubjectAltNameMatcher_IP_ADDRESS,
+		}
+	}
 	var matchers []*envoy_tls_v3.SubjectAltNameMatcher
 	for _, m := range matchStrings {
-		matchers = append(matchers, &envoy_tls_v3.SubjectAltNameMatcher{
-			SanType: envoy_tls_v3.SubjectAltNameMatcher_URI,
-			Matcher: &envoy_matcher_v3.StringMatcher{
-				MatchPattern: &envoy_matcher_v3.StringMatcher_Exact{
-					Exact: m,
+		for _, t := range types {
+			matchers = append(matchers, &envoy_tls_v3.SubjectAltNameMatcher{
+				SanType: t,
+				Matcher: &envoy_matcher_v3.StringMatcher{
+					MatchPattern: &envoy_matcher_v3.StringMatcher_Exact{
+						Exact: m,
+					},
 				},
-			},
-		})
+			})
+		}
 	}
 
 	validationCtx.ValidationContext.MatchTypedSubjectAltNames = matchers

diff --git a/agent/xds/failover_policy.go b/agent/xds/failover_policy.go
@@ -132,7 +132,7 @@ func (s *ResourceGenerator) mapDiscoChainTargets(cfgSnap *proxycfg.ConfigSnapsho
 			makeTLSParametersFromProxyTLSConfig(cfgSnap.MeshConfigTLSOutgoing()),
 		)
 
-		err := injectSANMatcher(commonTLSContext, spiffeIDs...)
+		err := injectSANMatcher(commonTLSContext, false, spiffeIDs...)
 		if err != nil {
 			return failoverTargets, fmt.Errorf("failed to inject SAN matcher rules for cluster %q: %v", sni, err)
 		}

diff --git a/agent/xds/testdata/clusters/terminating-gateway-sni.latest.golden b/agent/xds/testdata/clusters/terminating-gateway-sni.latest.golden
@@ -52,6 +52,24 @@
                     "exact": "bar.com"
                   },
                   "sanType": "URI"
+                },
+                {
+                  "matcher": {
+                    "exact": "bar.com"
+                  },
+                  "sanType": "DNS"
+                },
+                {
+                  "matcher": {
+                    "exact": "bar.com"
+                  },
+                  "sanType": "EMAIL"
+                },
+                {
+                  "matcher": {
+                    "exact": "bar.com"
+                  },
+                  "sanType": "IP_ADDRESS"
                 }
               ],
               "trustedCa": {
@@ -148,6 +166,24 @@
                     "exact": "foo.com"
                   },
                   "sanType": "URI"
+                },
+                {
+                  "matcher": {
+                    "exact": "foo.com"
+                  },
+                  "sanType": "DNS"
+                },
+                {
+                  "matcher": {
+                    "exact": "foo.com"
+                  },
+                  "sanType": "EMAIL"
+                },
+                {
+                  "matcher": {
+                    "exact": "foo.com"
+                  },
+                  "sanType": "IP_ADDRESS"
                 }
               ],
               "trustedCa": {

diff --git a/...s/testdata/clusters/transparent-proxy-terminating-gateway-destinations-only.latest.golden b/...s/testdata/clusters/transparent-proxy-terminating-gateway-destinations-only.latest.golden
@@ -172,6 +172,24 @@
                     "exact": "api.test.com"
                   },
                   "sanType": "URI"
+                },
+                {
+                  "matcher": {
+                    "exact": "api.test.com"
+                  },
+                  "sanType": "DNS"
+                },
+                {
+                  "matcher": {
+                    "exact": "api.test.com"
+                  },
+                  "sanType": "EMAIL"
+                },
+                {
+                  "matcher": {
+                    "exact": "api.test.com"
+                  },
+                  "sanType": "IP_ADDRESS"
                 }
               ],
               "trustedCa": {

diff --git a/agent/xds/testdata/rbac/v2-default-allow--httpfilter.golden b/agent/xds/testdata/rbac/v2-default-allow--httpfilter.golden
@@ -1 +1 @@
-{}
+{}
diff --git a/agent/xds/testdata/rbac/v2-default-deny--httpfilter.golden b/agent/xds/testdata/rbac/v2-default-deny--httpfilter.golden
@@ -4,4 +4,4 @@
     "@type": "type.googleapis.com/envoy.extensions.filters.http.rbac.v3.RBAC",
     "rules": {}
   }
-}
+}
diff --git a/agent/xds/testdata/rbac/v2-ignore-empty-permissions--httpfilter.golden b/agent/xds/testdata/rbac/v2-ignore-empty-permissions--httpfilter.golden
@@ -17,4 +17,4 @@
       }
     }
   ]
-}
+}
diff --git a/agent/xds/testdata/rbac/v2-kitchen-sink--httpfilter.golden b/agent/xds/testdata/rbac/v2-kitchen-sink--httpfilter.golden
@@ -111,4 +111,4 @@
       }
     }
   ]
-}
+}
diff --git a/agent/xds/xds_protocol_helpers_test.go b/agent/xds/xds_protocol_helpers_test.go
@@ -38,7 +38,7 @@ import (
 	"github.com/hashicorp/consul/agent/structs"
 	"github.com/hashicorp/consul/agent/xds/response"
 	"github.com/hashicorp/consul/envoyextensions/xdscommon"
-	"github.com/hashicorp/consul/internal/mesh/proxy-snapshot"
+	proxysnapshot "github.com/hashicorp/consul/internal/mesh/proxy-snapshot"
 	"github.com/hashicorp/consul/proto-public/pbresource"
 	"github.com/hashicorp/consul/sdk/testutil"
 )
@@ -295,7 +295,7 @@ func xdsNewTransportSocket(
 		},
 	}
 	if len(spiffeID) > 0 {
-		require.NoError(t, injectSANMatcher(commonTLSContext, spiffeID...))
+		require.NoError(t, injectSANMatcher(commonTLSContext, false, spiffeID...))
 	}
 
 	var tlsContext proto.Message
-Original file line number
+Diff line change
@@ Expand Up / @@ -4,4 +4,4 @@ @@
         "@type": "type.googleapis.com/envoy.extensions.filters.http.rbac.v3.RBAC",
         "rules": {}
       }
-    }
+    }