diff --git a/internal/extensionserver/mutate/misdirected.go b/internal/extensionserver/mutate/misdirected.go index e29cbc5f..722f75ba 100644 --- a/internal/extensionserver/mutate/misdirected.go +++ b/internal/extensionserver/mutate/misdirected.go @@ -2,6 +2,7 @@ package mutate import ( "slices" + "strings" listenerv3 "github.com/envoyproxy/go-control-plane/envoy/config/listener/v3" routev3 "github.com/envoyproxy/go-control-plane/envoy/config/route/v3" @@ -12,41 +13,66 @@ import ( const ( misdirectedVirtualHostName = "datum-misdirected-request" misdirectedStatus = 421 + unroutedVirtualHostName = "datum-unrouted-hostname" + unroutedStatus = 404 ) // GuardMisdirectedRequests answers 421 instead of 404 when a browser reuses one -// site's connection for another, so it retries on a connection of its own. +// site's connection for another, so it retries on a connection of its own. A +// hostname the connection was opened for still answers 404, since a retry would +// land on the same connection, unless another filter chain on the listener +// claims it more specifically. func GuardMisdirectedRequests(listeners []*listenerv3.Listener, routes []*routev3.RouteConfiguration) int { - secure, plaintext := sets.New[string](), sets.New[string]() + serverNames := map[string]sets.Set[string]{} + listenerNames := map[string][]sets.Set[string]{} + unguarded := sets.New[string]() for _, l := range listeners { chains := append([]*listenerv3.FilterChain{}, l.GetFilterChains()...) if dfc := l.GetDefaultFilterChain(); dfc != nil { chains = append(chains, dfc) } + onListener := sets.New[string]() for _, fc := range chains { - for _, name := range rdsRouteConfigNames(fc) { - if fc.GetTransportSocket() != nil { - secure.Insert(name) - } else { - plaintext.Insert(name) + onListener.Insert(fc.GetFilterChainMatch().GetServerNames()...) + } + for _, fc := range chains { + names := fc.GetFilterChainMatch().GetServerNames() + for _, rcName := range rdsRouteConfigNames(fc) { + if fc.GetTransportSocket() == nil || len(names) == 0 { + unguarded.Insert(rcName) + continue } + addNames(serverNames, rcName, names...) + listenerNames[rcName] = append(listenerNames[rcName], onListener) } } } guarded := 0 for _, rc := range routes { - name := rc.GetName() + names, ok := serverNames[rc.GetName()] // A second "*" domain would make Envoy reject the whole update. - if !secure.Has(name) || plaintext.Has(name) || hasCatchAllDomain(rc) { + if !ok || unguarded.Has(rc.GetName()) || hasCatchAllDomain(rc) { continue } - rc.VirtualHosts = append(rc.VirtualHosts, misdirectedVirtualHost()) + misdirected := []string{"*"} + if unrouted := unroutedNames(rc, names); len(unrouted) > 0 { + misdirected = append(misdirected, claimedElsewhere(rc, unrouted, names, listenerNames[rc.GetName()])...) + rc.VirtualHosts = append(rc.VirtualHosts, directResponseVirtualHost(unroutedVirtualHostName, unrouted, unroutedStatus)) + } + rc.VirtualHosts = append(rc.VirtualHosts, directResponseVirtualHost(misdirectedVirtualHostName, misdirected, misdirectedStatus)) guarded++ } return guarded } +func addNames(m map[string]sets.Set[string], key string, names ...string) { + if m[key] == nil { + m[key] = sets.New[string]() + } + m[key].Insert(names...) +} + func rdsRouteConfigNames(fc *listenerv3.FilterChain) []string { var names []string for _, f := range fc.GetFilters() { @@ -73,17 +99,60 @@ func hasCatchAllDomain(rc *routev3.RouteConfiguration) bool { return false } -func misdirectedVirtualHost() *routev3.VirtualHost { +func unroutedNames(rc *routev3.RouteConfiguration, names sets.Set[string]) []string { + var domains []string + for _, vh := range rc.GetVirtualHosts() { + domains = append(domains, vh.GetDomains()...) + } + var unrouted []string + for _, name := range sets.List(names) { + if !slices.ContainsFunc(domains, func(d string) bool { return domainMatches(d, name) }) { + unrouted = append(unrouted, name) + } + } + return unrouted +} + +func claimedElsewhere(rc *routev3.RouteConfiguration, unrouted []string, own sets.Set[string], listenerNames []sets.Set[string]) []string { + var wildcards []string + for _, name := range unrouted { + if strings.HasPrefix(name, "*") { + wildcards = append(wildcards, name) + } + } + if len(wildcards) == 0 { + return nil + } + claimed := sets.New[string]() + for _, names := range listenerNames { + for name := range names { + if !own.Has(name) && slices.ContainsFunc(wildcards, func(w string) bool { return domainMatches(w, name) }) { + claimed.Insert(name) + } + } + } + return unroutedNames(rc, claimed) +} + +func domainMatches(domain, name string) bool { + if domain == name { + return true + } + suffix, wildcard := strings.CutPrefix(domain, "*") + return wildcard && name != suffix && strings.HasSuffix(name, suffix) +} + +func directResponseVirtualHost(name string, domains []string, status uint32) *routev3.VirtualHost { return &routev3.VirtualHost{ - Name: misdirectedVirtualHostName, - Domains: []string{"*"}, + Name: name, + Domains: domains, Routes: []*routev3.Route{{ - Name: misdirectedVirtualHostName, + Name: name, Match: &routev3.RouteMatch{ PathSpecifier: &routev3.RouteMatch_Prefix{Prefix: "/"}, }, Action: &routev3.Route_DirectResponse{ - DirectResponse: &routev3.DirectResponseAction{Status: misdirectedStatus}, + DirectResponse: &routev3.DirectResponseAction{Status: status}, }, }}, } diff --git a/internal/extensionserver/mutate/misdirected_test.go b/internal/extensionserver/mutate/misdirected_test.go index 69bf9a16..664e4565 100644 --- a/internal/extensionserver/mutate/misdirected_test.go +++ b/internal/extensionserver/mutate/misdirected_test.go @@ -48,17 +48,30 @@ func findVirtualHost(rc *routev3.RouteConfiguration, name string) *routev3.Virtu func TestGuardMisdirectedRequests(t *testing.T) { quicChain := withRDS(t, &listenerv3.FilterChain{ - Name: "quic-b", - TransportSocket: &corev3.TransportSocket{Name: "envoy.transport_sockets.quic"}, + Name: "quic-b", + FilterChainMatch: &listenerv3.FilterChainMatch{ServerNames: []string{"b.example.net"}}, + TransportSocket: &corev3.TransportSocket{Name: "envoy.transport_sockets.quic"}, }, "https-b") + multiChain := tlsChain(t, "m1.example.net", "secret-m") + multiChain.FilterChainMatch.ServerNames = []string{"m1.example.net", "m2.example.net"} + anySNIChain := tlsChain(t, "unused", "secret-any-sni") + anySNIChain.FilterChainMatch = nil listeners := []*listenerv3.Listener{ { Name: "tcp-443", FilterChains: []*listenerv3.FilterChain{ withRDS(t, tlsChain(t, "a.example.net", "secret-a"), "https-a"), + withRDS(t, tlsChain(t, "empty.example.net", "secret-empty"), "no-routes"), + withRDS(t, tlsChain(t, "*.w.example.net", "secret-w"), "wildcard-listener"), + withRDS(t, tlsChain(t, "x.w.example.net", "secret-x"), "under-wildcard"), + withRDS(t, tlsChain(t, "app.w.example.net", "secret-app"), "routed-under-wildcard"), + withRDS(t, multiChain, "multi"), + withRDS(t, tlsChain(t, "app.c.example.net", "secret-c"), "wildcard-route"), withRDS(t, tlsChain(t, "shared.example.net", "secret-shared"), "shared"), withRDS(t, tlsChain(t, "any.example.net", "secret-any"), "has-catch-all"), + withRDS(t, tlsChain(t, "s.example.net", "secret-s"), "any-sni"), + withRDS(t, anySNIChain, "any-sni"), }, }, {Name: "udp-443", FilterChains: []*listenerv3.FilterChain{quicChain}}, @@ -74,24 +87,57 @@ func TestGuardMisdirectedRequests(t *testing.T) { routes := []*routev3.RouteConfiguration{ siteRouteConfig("https-a", "a.example.net"), siteRouteConfig("https-b", "b.example.net"), + {Name: "no-routes"}, + siteRouteConfig("wildcard-listener", "app.w.example.net"), + siteRouteConfig("under-wildcard", "x.w.example.net"), + siteRouteConfig("routed-under-wildcard", "app.w.example.net"), + siteRouteConfig("multi", "m1.example.net"), + siteRouteConfig("wildcard-route", "*.c.example.net"), siteRouteConfig("shared", "shared.example.net"), catchAll, + siteRouteConfig("any-sni", "s.example.net"), siteRouteConfig("http-80", "plain.example.net"), } - require.Equal(t, 2, GuardMisdirectedRequests(listeners, routes)) - - for _, rc := range routes[:2] { - vh := findVirtualHost(rc, misdirectedVirtualHostName) - require.NotNil(t, vh, "%s is only reached over TLS or QUIC and must answer 421", rc.GetName()) - assert.Equal(t, []string{"*"}, vh.GetDomains()) - assert.EqualValues(t, misdirectedStatus, vh.GetRoutes()[0].GetDirectResponse().GetStatus()) - assert.Equal(t, rc.GetVirtualHosts()[0].GetName(), rc.GetVirtualHosts()[0].GetDomains()[0], - "the site's own virtual host stays first and unchanged") + type guard struct{ unrouted, misdirected []string } + want := map[string]guard{ + "https-a": {}, + "https-b": {}, + "no-routes": {unrouted: []string{"empty.example.net"}}, + "wildcard-listener": { + unrouted: []string{"*.w.example.net"}, + misdirected: []string{"*", "x.w.example.net"}, + }, + "under-wildcard": {}, + "routed-under-wildcard": {}, + "multi": {unrouted: []string{"m2.example.net"}}, + "wildcard-route": {}, } - for _, rc := range routes[2:] { - assert.Nil(t, findVirtualHost(rc, misdirectedVirtualHostName), - "%s is reached in plaintext or already has a catch-all and must be left alone", rc.GetName()) + + require.Equal(t, len(want), GuardMisdirectedRequests(listeners, routes)) + + for _, rc := range routes { + w, guarded := want[rc.GetName()] + misdirected := findVirtualHost(rc, misdirectedVirtualHostName) + if !guarded { + assert.Nil(t, misdirected, "%s is reached in plaintext, from any SNI or already has a catch-all", rc.GetName()) + continue + } + require.NotNil(t, misdirected, "%s is only reached over TLS or QUIC and must answer 421", rc.GetName()) + if w.misdirected == nil { + w.misdirected = []string{"*"} + } + assert.Equal(t, w.misdirected, misdirected.GetDomains(), rc.GetName()) + assert.EqualValues(t, misdirectedStatus, misdirected.GetRoutes()[0].GetDirectResponse().GetStatus()) + + vh := findVirtualHost(rc, unroutedVirtualHostName) + if w.unrouted == nil { + assert.Nil(t, vh, "%s has a virtual host for every server name", rc.GetName()) + continue + } + require.NotNil(t, vh, "%s must answer 404 for its own unrouted hostnames", rc.GetName()) + assert.Equal(t, w.unrouted, vh.GetDomains(), rc.GetName()) + assert.EqualValues(t, unroutedStatus, vh.GetRoutes()[0].GetDirectResponse().GetStatus()) } assert.Zero(t, GuardMisdirectedRequests(listeners, routes), diff --git a/test/e2e-edge/misdirected-request/chainsaw-test.yaml b/test/e2e-edge/misdirected-request/chainsaw-test.yaml index 7c0cbe20..908ad367 100644 --- a/test/e2e-edge/misdirected-request/chainsaw-test.yaml +++ b/test/e2e-edge/misdirected-request/chainsaw-test.yaml @@ -13,6 +13,10 @@ # them. HTTP/1.1 exercises the same route configuration a reused HTTP/2 or # HTTP/3 connection does: TLS is negotiated for site A, the request names site B. # +# A third gateway has an HTTPS listener and no route. A request for its own +# hostname on its own connection stays a 404, since a retry would land on the +# same connection. +# # The gateways are created on the downstream cluster directly, as in # atomic-reject-isolation; the namespace carries the gateway-watch label at # creation. @@ -30,6 +34,8 @@ spec: value: misdirect-a.e2e.env.datum.net - name: siteB value: misdirect-b.e2e.env.datum.net + - name: siteC + value: misdirect-c.e2e.env.datum.net steps: - name: Two tenants' gateways sharing one certificate try: @@ -73,6 +79,7 @@ spec: dnsNames: - ($siteA) - ($siteB) + - ($siteC) issuerRef: name: md-selfsigned kind: Issuer @@ -132,8 +139,31 @@ spec: value: ($siteA) - name: SITE_B value: ($siteB) + - name: SITE_C + value: ($siteC) content: | set -eu + cat </dev/null || true) + echo "attempt ${i}: SNI and Host ${SITE_C} -> ${got}" + if [ "${got}" = "404 0" ]; then echo "OK: own unrouted hostname answered an empty 404"; exit 0; fi + case "${got}" in 421*) echo "ERROR: own unrouted hostname answered 421"; exit 1 ;; esac + sleep 5 + done + echo "ERROR: own unrouted hostname was not answered an empty 404" + exit 1 + - name: The proxy runs the configuration the extension server built description: | The 421 is only the guard's if the proxy carries what the extension