fix: stop 421 for own hostnames with no route

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},
 			},
 		}},
 	}

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),

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 <<EOF | kubectl -n "$MD_NS" create -f -
+              apiVersion: gateway.networking.k8s.io/v1
+              kind: Gateway
+              metadata:
+                name: md-site-c
+              spec:
+                gatewayClassName: ${GATEWAY_CLASS}
+                listeners:
+                  - name: https
+                    protocol: HTTPS
+                    port: 443
+                    hostname: ${SITE_C}
+                    allowedRoutes:
+                      namespaces:
+                        from: Same
+                    tls:
+                      mode: Terminate
+                      certificateRefs:
+                        - kind: Secret
+                          name: md-shared-tls
+              EOF
               for site in a b; do
                 host=$SITE_A; [ "$site" = b ] && host=$SITE_B
                 cat <<EOF | kubectl -n "$MD_NS" create -f -
@@ -203,6 +233,19 @@ spec:
                 (listeners[?name == 'https'] | [0]):
                   (conditions[?type == 'Programmed']):
                     - status: "True"
+        - assert:
+            timeout: 120s
+            cluster: nso-downstream
+            resource:
+              apiVersion: gateway.networking.k8s.io/v1
+              kind: Gateway
+              metadata:
+                name: md-site-c
+                namespace: (join('-', ['md', $namespace]))
+              status:
+                (listeners[?name == 'https'] | [0]):
+                  (conditions[?type == 'Programmed']):
+                    - status: "True"
       catch:
         - script:
             cluster: nso-downstream
@@ -309,6 +352,37 @@ spec:
               echo "ERROR: misdirected request was not answered 421"
               exit 1
 
+    - name: A hostname with no route on its own connection is answered 404
+      description: |
+        TLS is negotiated for site C and the request names site C, which has no
+        route. A retry would land on the same connection, so this stays the
+        empty 404 it was before the catch-all.
+      try:
+        - script:
+            timeout: 300s
+            cluster: nso-downstream
+            env:
+              - name: OWNING_GC
+                value: ($downstreamGatewayClass)
+              - name: SITE_C
+                value: ($siteC)
+            content: |
+              set -eu
+              IP=$(kubectl get svc -n datum-downstream-gateway \
+                -l gateway.envoyproxy.io/owning-gatewayclass=$OWNING_GC \
+                -o jsonpath='{.items[0].spec.clusterIP}')
+              for i in $(seq 1 24); do
+                got=$(kubectl -n default exec conntest -- \
+                  curl -ksS -o /dev/null -w '%{http_code} %{size_download}' --max-time 10 \
+                  --resolve "${SITE_C}:443:${IP}" "https://${SITE_C}/" 2>/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