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 0000000000..c55353348a --- /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 d677d20607..df59604583 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 ab3e86f25d..fdd351670a 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 516b6d3915..190301470a 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 9f531929c7..ff66e6cc86 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 0967ef424b..9e26dfeeb6 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 290edfd0c5..a435b1f2b8 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 536b0e04f7..f947a2d0be 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 a9c458e230..6bc1fb6e5f 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 655e0458f4..4b7fb90f4b 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