From 3e8ec8d18ebb019841bef4a1922b8a757b2838f9 Mon Sep 17 00:00:00 2001 From: Derek Menteer <105233703+hashi-derek@users.noreply.github.com> Date: Wed, 31 Jan 2024 12:17:45 -0600 Subject: [PATCH] Fix SAN matching on terminating gateways (#20417) Fixes issue: hashicorp/consul#20360 A regression was introduced in hashicorp/consul#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 --- .changelog/20417.txt | 3 ++ agent/xds/clusters.go | 47 ++++++++++++++----- agent/xds/failover_policy.go | 2 +- .../terminating-gateway-sni.latest.golden | 36 ++++++++++++++ ...ng-gateway-destinations-only.latest.golden | 18 +++++++ .../rbac/v2-default-allow--httpfilter.golden | 2 +- .../rbac/v2-default-deny--httpfilter.golden | 2 +- ...gnore-empty-permissions--httpfilter.golden | 2 +- .../rbac/v2-kitchen-sink--httpfilter.golden | 2 +- agent/xds/xds_protocol_helpers_test.go | 4 +- 10 files changed, 98 insertions(+), 20 deletions(-) create mode 100644 .changelog/20417.txt diff --git a/.changelog/20417.txt b/.changelog/20417.txt new file mode 100644 index 000000000000..c55353348aa9 --- /dev/null +++ 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 index d677d20607e4..df596045839d 100644 --- 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 index ab3e86f25d1d..fdd351670a8f 100644 --- 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 index 516b6d39151f..190301470ad0 100644 --- 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/agent/xds/testdata/clusters/transparent-proxy-terminating-gateway-destinations-only.latest.golden b/agent/xds/testdata/clusters/transparent-proxy-terminating-gateway-destinations-only.latest.golden index 9f531929c7fb..ff66e6cc86ef 100644 --- a/agent/xds/testdata/clusters/transparent-proxy-terminating-gateway-destinations-only.latest.golden +++ b/agent/xds/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 index 0967ef424bce..9e26dfeeb6e6 100644 --- a/agent/xds/testdata/rbac/v2-default-allow--httpfilter.golden +++ b/agent/xds/testdata/rbac/v2-default-allow--httpfilter.golden @@ -1 +1 @@ -{} +{} \ No newline at end of file diff --git a/agent/xds/testdata/rbac/v2-default-deny--httpfilter.golden b/agent/xds/testdata/rbac/v2-default-deny--httpfilter.golden index 290edfd0c507..a435b1f2b870 100644 --- 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": {} } -} +} \ No newline at end of file diff --git a/agent/xds/testdata/rbac/v2-ignore-empty-permissions--httpfilter.golden b/agent/xds/testdata/rbac/v2-ignore-empty-permissions--httpfilter.golden index 536b0e04f75d..f947a2d0be05 100644 --- 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 @@ } } ] -} +} \ No newline at end of file diff --git a/agent/xds/testdata/rbac/v2-kitchen-sink--httpfilter.golden b/agent/xds/testdata/rbac/v2-kitchen-sink--httpfilter.golden index a9c458e230f0..6bc1fb6e5f10 100644 --- a/agent/xds/testdata/rbac/v2-kitchen-sink--httpfilter.golden +++ b/agent/xds/testdata/rbac/v2-kitchen-sink--httpfilter.golden @@ -111,4 +111,4 @@ } } ] -} +} \ No newline at end of file diff --git a/agent/xds/xds_protocol_helpers_test.go b/agent/xds/xds_protocol_helpers_test.go index 655e0458f49e..4b7fb90f4b05 100644 --- 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