From d438d5b7628ad6a595dee7b808fa0280a438ba45 Mon Sep 17 00:00:00 2001 From: hc-github-team-consul-core Date: Wed, 31 Jan 2024 12:33:19 -0600 Subject: [PATCH] Backport of Fix SAN matching on terminating gateways into release/1.16.x (#20418) 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 Co-authored-by: Derek Menteer <105233703+hashi-derek@users.noreply.github.com> --- .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 +++++++ agent/xds/xds_protocol_helpers_test.go | 2 +- 6 files changed, 93 insertions(+), 15 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 30ebcd7bf497..c090240d4c05 100644 --- a/agent/xds/clusters.go +++ b/agent/xds/clusters.go @@ -479,7 +479,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) } @@ -894,7 +894,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) } } @@ -923,7 +923,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) } } @@ -1242,7 +1242,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) } @@ -1345,7 +1345,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) } @@ -1625,7 +1625,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") } @@ -1636,16 +1636,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 5edcae914d52..68a891381927 100644 --- a/agent/xds/failover_policy.go +++ b/agent/xds/failover_policy.go @@ -130,7 +130,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 bcbe5463b135..3113c3a93fd9 100644 --- a/agent/xds/testdata/clusters/terminating-gateway-sni.latest.golden +++ b/agent/xds/testdata/clusters/terminating-gateway-sni.latest.golden @@ -56,6 +56,24 @@ "matcher": { "exact": "bar.com" } + }, + { + "sanType": "DNS", + "matcher": { + "exact": "bar.com" + } + }, + { + "sanType": "EMAIL", + "matcher": { + "exact": "bar.com" + } + }, + { + "sanType": "IP_ADDRESS", + "matcher": { + "exact": "bar.com" + } } ] } @@ -152,6 +170,24 @@ "matcher": { "exact": "foo.com" } + }, + { + "sanType": "DNS", + "matcher": { + "exact": "foo.com" + } + }, + { + "sanType": "EMAIL", + "matcher": { + "exact": "foo.com" + } + }, + { + "sanType": "IP_ADDRESS", + "matcher": { + "exact": "foo.com" + } } ] } 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 38d4b1348f28..24faafb2be2f 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 @@ -176,6 +176,24 @@ "matcher": { "exact": "api.test.com" } + }, + { + "sanType": "DNS", + "matcher": { + "exact": "api.test.com" + } + }, + { + "sanType": "EMAIL", + "matcher": { + "exact": "api.test.com" + } + }, + { + "sanType": "IP_ADDRESS", + "matcher": { + "exact": "api.test.com" + } } ] } diff --git a/agent/xds/xds_protocol_helpers_test.go b/agent/xds/xds_protocol_helpers_test.go index 958a27474c44..4005cd17cf31 100644 --- a/agent/xds/xds_protocol_helpers_test.go +++ b/agent/xds/xds_protocol_helpers_test.go @@ -290,7 +290,7 @@ func xdsNewTransportSocket( }, } if len(spiffeID) > 0 { - require.NoError(t, injectSANMatcher(commonTLSContext, spiffeID...)) + require.NoError(t, injectSANMatcher(commonTLSContext, false, spiffeID...)) } var tlsContext proto.Message