fix(sandbox-router-extproc): close validation gaps + lock down dst-host trust

Mirror the parity fixes that just landed on PR kubernetes-sigs#838, in the shapes
that fit the ext_proc design. Together with the per-PR ingress strip,
this closes the four-finding security review from @aditya-shantanu.

1. X-Sandbox-ID DNS-label validation. The Python router runs ID
   through ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ (max 63 chars) so a caller
   can't interpolate extra DNS components into "<id>.<ns>.svc.
   <cluster-domain>" via the DNS-form fallback. We validated namespace
   and port but not ID. Now applied to both ID and namespace through
   a shared validDNSLabel helper.

2. Tightened namespace validation. The previous validNamespace allowed
   uppercase, leading/trailing hyphens, and unbounded length. K8s
   itself rejects all three, so the practical risk was just routing
   to FQDNs that can't exist — but tightening to the same DNS-1123
   rule used for ID keeps the validation surface uniform and matches
   Python exactly. validNamespace removed in favor of validDNSLabel.

3. Authorization header strip. Always add "authorization" to the
   HeaderMutation.RemoveHeaders so the sandbox never sees the
   caller's bearer credential. Without this, a sandbox could
   impersonate the caller against the K8s API or any other
   Bearer-protected service. Matches the Python router and the same
   fix on kubernetes-sigs#838.

4. x-envoy-original-dst-host ingress strip (defense in depth). A new
   envoy.filters.http.header_mutation filter runs BEFORE ext_proc in
   the HCM chain and removes any client-supplied
   x-envoy-original-dst-host. ext_proc still always sets it via
   HeaderMutation, so after the filter chain the value reaching the
   ORIGINAL_DST cluster is provably the one ext_proc wrote — or
   absent if a future route disables ext_proc, in which case the
   cluster fails closed with 503 rather than dispatching to whatever
   the client asked for. Without this, the security of the data path
   would rest on "ext_proc is enabled on every route", which the
   existing /healthz route already demonstrates is not a load-
   bearing assumption.

Tests: TestHandle_InvalidIDRejected covers the six classes of
DNS-injection / traversal inputs (dot, slash, underscore, uppercase,
leading hyphen, trailing hyphen). TestHandle_AlwaysStripsAuthorization
asserts the RemoveHeaders mutation contains "authorization" on both
upgrade and non-upgrade paths. TestValidDNSLabel replaces the old
TestValidNamespace with the stricter table (accepts 1abc per RFC 1123,
rejects MY-NS, -x, x-, length > 63).

README documents the new validation surface, the headers we strip
before forwarding (Authorization, Origin-on-upgrade, and the
listener-level x-envoy-original-dst-host strip), and the rationale
for each.

Author: mastersingh24

sandbox-router/README.md

@@ -38,10 +38,12 @@ Envoy expects these headers from the client (preserved from the Python router fo
 
 | Header | Required? | Default | Notes |
 |---|---|---|---|
-| `X-Sandbox-ID` | yes | — | Sandbox name. Used for DNS-form fallback when UID is missing or uncached. |
+| `X-Sandbox-ID` | yes | — | Sandbox name. Used for DNS-form fallback when UID is missing or uncached. Must be a valid DNS-1123 label (`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`, max 63 chars). |
 | `X-Sandbox-UID` | recommended | — | Sandbox CR UID. Primary key for cache lookup; non-guessable. |
-| `X-Sandbox-Namespace` | no | `default` | ASCII letters/digits/hyphens, ≥1 alphanumeric. |
-| `X-Sandbox-Port` | no | `8888` | Numeric. |
+| `X-Sandbox-Namespace` | no | `default` | Same DNS-1123 label check as ID. |
+| `X-Sandbox-Port` | no | `8888` | Integer in `[1, 65535]`. |
+
+DNS-label validation on both ID and namespace prevents DNS injection (e.g. `foo.evil.com` interpolating extra components into the upstream FQDN) and traversal-style inputs (e.g. `foo/bar`). Matches the Python router's `_is_valid_dns_label` check.
 
 Routing precedence:
 
@@ -51,6 +53,14 @@ Routing precedence:
 
 The ext_proc handler returns Python-router-compatible JSON error bodies (`{"detail":"..."}`) for validation failures so existing clients keep working.
 
+### Headers stripped before forwarding
+
+The `HeaderMutation` returned to Envoy always removes:
+
+- **`Authorization`** — identity-checking authorizers in front of ext_proc (TokenReview, JWT, etc.) consume the bearer credential. Forwarding it to the sandbox would let the sandbox impersonate the caller against the K8s API or any other Bearer-protected service. Matches the Python router, which strips `Authorization` right next to `Host`.
+- **`x-envoy-original-dst-host`** on the listener (defense in depth) — an `envoy.filters.http.header_mutation` filter runs *before* ext_proc and removes any client-supplied value, so ext_proc is provably the only writer of that header. Without this, a future route that disables ext_proc and uses the ORIGINAL_DST cluster would dispatch to whatever the client asked for.
+- **`Origin` on upgrade requests** — see the WebSockets section below.
+
 ### WebSockets and X-Forwarded-* headers
 
 Two small carve-outs make browser-facing backends (vscode-server, Jupyter, anything that holds an Origin/Host CSRF check) work without changes:

sandbox-router/deploy/envoy/configmap.yaml

@@ -93,6 +93,22 @@ data:
                       timeout: 180s
 
               http_filters:
+              # Strip any inbound x-envoy-original-dst-host BEFORE
+              # ext_proc runs. ext_proc always sets this header via
+              # HeaderMutation, so after this filter chain the value
+              # in the header is provably the one ext_proc wrote (or
+              # absent if ext_proc was bypassed for the route, in
+              # which case the ORIGINAL_DST cluster won't find a
+              # target and returns 503 — fail-closed). Defense-in-
+              # depth: keeps the security of the data path from
+              # resting on "ext_proc is enabled on every route".
+              - name: envoy.filters.http.header_mutation
+                typed_config:
+                  "@type": type.googleapis.com/envoy.extensions.filters.http.header_mutation.v3.HeaderMutation
+                  mutations:
+                    request_mutations:
+                    - remove: x-envoy-original-dst-host
+
               # ext_proc must run before the router filter so we get the
               # header mutation applied + route cache cleared before the
               # routing decision is made.

sandbox-router/internal/extproc/server.go

@@ -208,7 +208,15 @@ func (s *Server) onRequestHeaders(_ context.Context, hdrs *extprocv3.HttpHeaders
 	if r.id == "" {
 		return immediate(400, `{"detail":"X-Sandbox-ID header is required."}`)
 	}
-	if !validNamespace(r.namespace) {
+	// DNS-label validation on ID + namespace prevents DNS injection
+	// (e.g. id="foo.evil.com" interpolating extra components into
+	// "<id>.<ns>.svc.<cluster-domain>") and traversal-style inputs
+	// (e.g. id="foo/bar"). Matches the Python router's
+	// _is_valid_dns_label check and the same fix on PR #838.
+	if !validDNSLabel(r.id) {
+		return immediate(400, `{"detail":"Invalid sandbox ID format."}`)
+	}
+	if !validDNSLabel(r.namespace) {
 		return immediate(400, `{"detail":"Invalid namespace format."}`)
 	}
 	// TCP port range is [1, 65535]. Reject anything outside it (and the
@@ -246,6 +254,17 @@ func (s *Server) onRequestHeaders(_ context.Context, hdrs *extprocv3.HttpHeaders
 			},
 			AppendAction: corev3.HeaderValueOption_OVERWRITE_IF_EXISTS_OR_ADD,
 		}},
+		// Always strip Authorization on the way to the upstream.
+		// Identity-checking authorizers (TokenReview, JWT, etc.) sit in
+		// front of ext_proc and consume the bearer credential there;
+		// forwarding it to the sandbox would let the sandbox
+		// impersonate the caller against the K8s API or any other
+		// Bearer-protected service. Envoy normalizes header keys to
+		// lowercase, so "authorization" is the key to remove. Matches
+		// the same fix on the from-scratch Go router (PR #838) and the
+		// Python router, which strips Authorization right next to Host
+		// before forwarding.
+		RemoveHeaders: []string{"authorization"},
 	}
 	if r.upgrade {
 		// Strip Origin on upgrade so WebSocket backends that validate
@@ -369,29 +388,37 @@ func headerString(h *corev3.HeaderValue) string {
 	return h.Value
 }
 
-// validNamespace mirrors the Python router's namespace check:
+// validDNSLabel reports whether s is a syntactically valid DNS-1123
+// label. Mirrors the Python router's
 //
-//	namespace.replace("-", "").isalnum()
+//	^[a-z0-9](?:[-a-z0-9]*[a-z0-9])?$
 //
-// At least one ASCII alphanumeric, only ASCII letters/digits/hyphens
-// otherwise. Empty input and hyphen-only input both rejected.
-func validNamespace(s string) bool {
-	if s == "" {
+// regex with a 63-character cap. Applied to both X-Sandbox-ID and
+// X-Sandbox-Namespace before either gets interpolated into the DNS-
+// form upstream FQDN, so callers can't inject extra DNS components or
+// traversal sequences ("foo.evil.com", "foo/bar", etc.). Stricter
+// than the previous validNamespace: rejects uppercase, leading/
+// trailing hyphens, and anything over 63 chars — same shape K8s
+// itself enforces on Pod and Namespace names.
+func validDNSLabel(s string) bool {
+	if s == "" || len(s) > 63 {
 		return false
 	}
-	hasAlphanum := false
 	for i := 0; i < len(s); i++ {
 		c := s[i]
 		switch {
-		case c >= '0' && c <= '9', c >= 'a' && c <= 'z', c >= 'A' && c <= 'Z':
-			hasAlphanum = true
+		case c >= '0' && c <= '9':
+		case c >= 'a' && c <= 'z':
 		case c == '-':
-			// allowed
+			// Hyphen is allowed only in the interior — not first or last.
+			if i == 0 || i == len(s)-1 {
+				return false
+			}
 		default:
 			return false
 		}
 	}
-	return hasAlphanum
+	return true
 }
 
 // joinHostPort formats "host:port", bracketing IPv6 literals per

sandbox-router/internal/extproc/server_test.go

@@ -198,6 +198,35 @@ func TestHandle_InvalidNamespaceRejected(t *testing.T) {
 	}
 }
 
+func TestHandle_InvalidIDRejected(t *testing.T) {
+	// Mirrors the Python router's DNS-label check on sandbox_id. Without
+	// this, a caller could interpolate extra DNS components into the
+	// upstream FQDN via the DNS form ("<id>.<ns>.svc.<cluster-domain>").
+	cases := map[string]string{
+		"dot in id (would inject extra DNS components)": "foo.evil.com",
+		"slash in id (traversal flavor)":                "foo/bar",
+		"underscore in id":                              "foo_bar",
+		"uppercase":                                     "FooBar",
+		"leading hyphen":                                "-foo",
+		"trailing hyphen":                               "foo-",
+	}
+	s, _ := newServer(t)
+	for name, id := range cases {
+		t.Run(name, func(t *testing.T) {
+			resp := s.handle(context.Background(), reqHeaders(map[string]string{
+				HeaderSandboxID: id,
+			}))
+			ir := resp.GetImmediateResponse()
+			if ir == nil || ir.Status.Code != 400 {
+				t.Fatalf("id=%q: expected 400 immediate; got: %+v", id, resp)
+			}
+			if !strings.Contains(string(ir.Body), "Invalid sandbox ID") {
+				t.Errorf("id=%q: body should mention Invalid sandbox ID: %s", id, ir.Body)
+			}
+		})
+	}
+}
+
 func TestHandle_InvalidPortRejected(t *testing.T) {
 	// Port must fall in [1, 65535]. Anything else gets rejected with a
 	// 400 immediate before we hand the value to net.JoinHostPort, so a
@@ -350,26 +379,42 @@ func TestHandle_UnknownPhaseReturnsNil(t *testing.T) {
 	}
 }
 
-func TestValidNamespace(t *testing.T) {
+func TestValidDNSLabel(t *testing.T) {
+	// Stricter than the previous validNamespace: rejects uppercase,
+	// leading/trailing hyphens, and labels > 63 chars. Same shape as
+	// RFC 1123 / K8s' own DNS-label rules and the Python router's
+	// _is_valid_dns_label regex.
 	cases := map[string]bool{
-		"default": true,
-		"prod":    true,
-		"my-ns":   true,
-		"my-ns-1": true,
-		"MY-NS":   true,
-		"a":       true,
-		"":        false,
-		"-":       false,
-		"---":     false,
-		"my_ns":   false,
-		"my.ns":   false,
-		" ns":     false,
-		"ns ":     false,
+		// accepted
+		"default":               true,
+		"prod":                  true,
+		"my-ns":                 true,
+		"my-ns-1":               true,
+		"a":                     true,
+		"abc123":                true,
+		"1abc":                  true, // leading digit OK in RFC 1123 (vs the older 952)
+		strings.Repeat("a", 63): true,
+
+		// rejected — character class
+		"MY-NS":   false, // uppercase
+		"my_ns":   false, // underscore
+		"my.ns":   false, // dot would inject extra DNS components
+		"foo/bar": false, // slash, traversal flavor
+		" ns":     false, // leading space
+		"ns ":     false, // trailing space
 		"bad!":    false,
+
+		// rejected — structure
+		"":                      false, // empty
+		"-":                     false, // single hyphen
+		"---":                   false, // hyphens only
+		"-x":                    false, // leading hyphen
+		"x-":                    false, // trailing hyphen
+		strings.Repeat("a", 64): false, // exceeds 63-char cap
 	}
 	for in, want := range cases {
-		if got := validNamespace(in); got != want {
-			t.Errorf("validNamespace(%q) = %v, want %v", in, got, want)
+		if got := validDNSLabel(in); got != want {
+			t.Errorf("validDNSLabel(%q) = %v, want %v", in, got, want)
 		}
 	}
 }
@@ -509,6 +554,45 @@ func TestHandle_NonUpgradePreservesOrigin(t *testing.T) {
 	}
 }
 
+// TestHandle_AlwaysStripsAuthorization is the regression test for the
+// privilege-escalation hazard the Python router avoids by stripping
+// Authorization. When the router is fronted by an identity-checking
+// authorizer (TokenReview, JWT, etc.) the caller's bearer credential
+// is meant for the router, not the sandbox. Forwarding it would let
+// the sandbox impersonate the caller against the K8s API or any
+// other Bearer-protected service. The strip applies to both upgrade
+// and non-upgrade paths, so we assert it on each.
+func TestHandle_AlwaysStripsAuthorization(t *testing.T) {
+	s, stub := newServer(t)
+	uid := types.UID("auth-uid")
+	stub[uid] = cache.Entry{PodIP: "10.0.0.9"}
+
+	for _, name := range []string{"non-upgrade", "upgrade"} {
+		t.Run(name, func(t *testing.T) {
+			hdrs := map[string]string{
+				HeaderSandboxID:  "alpha",
+				HeaderSandboxUID: string(uid),
+				"Authorization":  "Bearer should-not-leak",
+			}
+			if name == "upgrade" {
+				hdrs["Connection"] = "Upgrade"
+				hdrs["Upgrade"] = "websocket"
+			}
+			resp := s.handle(context.Background(), reqHeaders(hdrs))
+			found := false
+			for _, h := range removeHeadersFrom(resp) {
+				if h == "authorization" {
+					found = true
+				}
+			}
+			if !found {
+				t.Fatalf("%s: expected RemoveHeaders to contain \"authorization\"; got %v",
+					name, removeHeadersFrom(resp))
+			}
+		})
+	}
+}
+
 func TestReadHeaders_UpgradeDetection(t *testing.T) {
 	// Locks in the predicate: an upgrade is recognized iff BOTH
 	// Connection contains an upgrade token AND Upgrade is non-empty.