From 1f97ad6bac2c52a72b952d9ea17059939acb5135 Mon Sep 17 00:00:00 2001 From: Hai Huang Date: Tue, 18 Aug 2026 11:53:23 -0400 Subject: [PATCH 1/4] feat(operator): Add inboundInterception=transparent (drops port stealing) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds an opt-in inbound shape for proxy-sidecar / lite that stops stealing the agent's port. When inboundInterception is "transparent", proxy-init installs a PREROUTING REDIRECT and AuthBridge recovers each connection's real destination via SO_ORIGINAL_DST, so the agent keeps the port it already binds. That removes the three problems with port stealing, all of which the existing code acknowledges: - The relocated port (originalPort+1) is declared in the pod spec and directly reachable, so any pod could reach the agent without JWT validation. The pod is the granularity Kubernetes NetworkPolicy and ztunnel both enforce at, so this was the boundary that mattered and it was open. - Relocation depends on the agent honoring PORT. pod_mutator.go concedes that agents which hardcode their listen port "won't be affected" — in practice they collide with AuthBridge on the stolen port and the pod never starts. - Only Ports[0] of the first container with ports was relocated, so a second declared port was never proxied. Default is "reverse-proxy" (port stealing). Transparent costs a privileged proxy-init container, so it must be chosen deliberately; with the field unset every existing path is byte-identical. The switch is the namespace authbridge-runtime-config ConfigMap (inboundInterception), resolved namespace > cluster default, with a proxy.allowedInboundInterception allowlist so a platform admin can forbid it (no NET_ADMIN) or mandate it. Two fallbacks are deliberately biased toward the unprivileged shape: - An unrecognized value falls back to reverse-proxy, so a typo cannot silently grant a NET_ADMIN init container. - transparent + egressEnforcement=none falls back to reverse-proxy. The two features share one proxy-init container; without it nothing would REDIRECT to the inbound listener and inbound would be silently unenforced — worse than port stealing, which at least validates Service-routed traffic. Deliberately NOT adding an AgentRuntime spec field. The obvious move was to mirror spec.egressEnforcement, but that field is dead surface: the pod mutator has no access to the CR and resolves everything from the namespace ConfigMap, so nothing reads spec.egressEnforcement (spec.mtlsMode reaches the pod only as a rollout-triggering annotation, not as mutator input). An enum-validated spec field that silently does nothing is worse than no field, and plumbing CR -> mutator properly is a separate change that would have to fix mtlsMode and egressEnforcement too. Wiring details: - The per-agent ConfigMap gets inbound_interception + transparent_inbound_addr and must NOT get reverse_proxy_addr / reverse_proxy_backend: authbridge's config validation rejects that combination, so emitting both would crash-loop the pod. - proxy-init gets INBOUND_TRANSPARENT_PORT and POD_IP (downward API). POD_IP is not optional — the init script uses it as the DNAT target for the Istio ambient inbound path, which arrives through OUTPUT rather than PREROUTING, and refuses to start without it rather than install PREROUTING-only rules that wave all mesh traffic through. - SIDECAR_PORTS_EXCLUDE carries the RESOLVED forward-proxy port, not the script's 8081 default: findFreePort may have moved it, and an unexempted forward-proxy port would be swallowed by the inbound REDIRECT. - The sidecar declares its inbound port as "transparent-in" rather than "reverse-proxy", so which shape is running is visible in the pod spec. - Config validation rejects transparentInboundPort == transparentPort. Both are listeners in one container, so a shared value fails the second bind at pod start, long after admission succeeded. Drive-by lint cleanups in touched code: extracted the duplicated resolution-source strings ("cluster-default", "default-invalid-fallback") into constants alongside the existing sourceNamespaceConfigMap, and dropped an unused parameter from the new private sidecar builder. make test: 21 packages green. No CRD changes (none needed). Refs: rossoctl/cortex#330 Assisted-By: Claude (Anthropic AI) Signed-off-by: Hai Huang --- charts/operator/values.yaml | 15 + operator/internal/webhook/config/defaults.go | 9 +- .../config/transparent_inbound_test.go | 107 +++++ operator/internal/webhook/config/types.go | 35 ++ .../internal/webhook/injector/constants.go | 32 ++ .../webhook/injector/container_builder.go | 81 +++- .../webhook/injector/namespace_config.go | 19 + .../internal/webhook/injector/pod_mutator.go | 166 +++++++- .../injector/transparent_inbound_test.go | 375 ++++++++++++++++++ 9 files changed, 814 insertions(+), 25 deletions(-) create mode 100644 operator/internal/webhook/config/transparent_inbound_test.go create mode 100644 operator/internal/webhook/injector/transparent_inbound_test.go diff --git a/charts/operator/values.yaml b/charts/operator/values.yaml index e266c949..b0ec3a17 100644 --- a/charts/operator/values.yaml +++ b/charts/operator/values.yaml @@ -262,6 +262,11 @@ defaults: # always-on enforce-redirect egress guard (proxy-sidecar / lite). MUST match # the authbridge listener.transparent_proxy_addr (default :8082). transparentPort: 8082 + # INBOUND transparent listener port — the PREROUTING REDIRECT target when a + # workload selects inboundInterception: transparent. MUST match the authbridge + # listener.transparent_inbound_addr (preset default :8083), and must differ + # from transparentPort (they are separate listeners in the same container). + transparentInboundPort: 8083 # Cluster DNS is kept direct by proxy-init itself (it reads the pod's # /etc/resolv.conf nameservers), so there is no in-cluster CIDR knob to set — # works on Kind / OpenShift / EKS / NodeLocal-DNSCache with no per-cluster config. @@ -280,6 +285,16 @@ defaults: allowedEgressEnforcement: - enforce-redirect - none + # Which inbound interception mechanisms workloads in this cluster may select. + # A resolved value outside this list falls back to the FIRST entry, so order + # matters. Transparent inbound needs a privileged proxy-init container + # (NET_ADMIN), so an admin may want to forbid or mandate it: + # ["reverse-proxy"] — port stealing only, no NET_ADMIN + # ["transparent"] — hard inbound boundary required + # ["reverse-proxy", "transparent"] — workloads choose (default) + allowedInboundInterception: + - reverse-proxy + - transparent # Resource defaults (conservative for dev) # Note: requests must be <= limits diff --git a/operator/internal/webhook/config/defaults.go b/operator/internal/webhook/config/defaults.go index 87cb0d9e..c54eb3bf 100644 --- a/operator/internal/webhook/config/defaults.go +++ b/operator/internal/webhook/config/defaults.go @@ -53,13 +53,20 @@ func CompiledDefaults() *PlatformConfig { // Transparent listener port — must match the authbridge proxy-sidecar // preset (listener.transparent_proxy_addr default :8082). TransparentPort: 8082, + // Inbound transparent listener. Matches the authbridge proxy-sidecar + // preset (listener.transparent_inbound_addr default :8083); 8080/8081/8082 + // are the reverse, forward and transparent-egress listeners. + TransparentInboundPort: 8083, // Empty by default: proxy-init auto-detects the iptables backend from // /proc/modules. Set (e.g. "iptables") to force a backend per-platform. IptablesCmd: "", // Both modes allowed by default. Set to ["none"] on platforms // where iptables is unavailable (ROSA HCP, managed OpenShift), // or ["enforce-redirect"] to prevent opt-out. - AllowedEgressEnforcement: []string{"enforce-redirect", "none"}, + // Both allowed by default: transparent inbound is opt-in per workload, + // and a platform admin can narrow this to forbid or mandate it. + AllowedInboundInterception: []string{"reverse-proxy", "transparent"}, + AllowedEgressEnforcement: []string{"enforce-redirect", "none"}, }, Resources: ResourcesConfig{ EnvoyProxy: corev1.ResourceRequirements{ diff --git a/operator/internal/webhook/config/transparent_inbound_test.go b/operator/internal/webhook/config/transparent_inbound_test.go new file mode 100644 index 00000000..b4ed2b28 --- /dev/null +++ b/operator/internal/webhook/config/transparent_inbound_test.go @@ -0,0 +1,107 @@ +package config + +import "testing" + +// TestValidate_TransparentInboundPort covers the misconfigurations that would +// otherwise surface as a pod that passes admission and then fails to start. +func TestValidate_TransparentInboundPort(t *testing.T) { + tests := []struct { + name string + mutate func(*PlatformConfig) + wantErr bool + }{ + { + name: "defaults are valid", + mutate: func(*PlatformConfig) {}, + }, + { + name: "below the privileged range", + mutate: func(c *PlatformConfig) { c.Proxy.TransparentInboundPort = 80 }, + wantErr: true, + }, + { + name: "above the port range", + mutate: func(c *PlatformConfig) { c.Proxy.TransparentInboundPort = 70000 }, + wantErr: true, + }, + { + name: "unset (zero) is rejected rather than silently defaulted", + mutate: func(c *PlatformConfig) { c.Proxy.TransparentInboundPort = 0 }, + wantErr: true, + }, + { + // Both listeners live in one container, so a shared value makes the + // second bind fail at pod start — long after admission succeeded. + name: "colliding with the egress transparent port", + mutate: func(c *PlatformConfig) { + c.Proxy.TransparentInboundPort = c.Proxy.TransparentPort + }, + wantErr: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + cfg := CompiledDefaults() + tc.mutate(cfg) + err := cfg.Validate() + if tc.wantErr && err == nil { + t.Fatal("expected a validation error, got nil") + } + if !tc.wantErr && err != nil { + t.Fatalf("unexpected validation error: %v", err) + } + }) + } +} + +func TestValidate_AllowedInboundInterception(t *testing.T) { + tests := []struct { + name string + allowed []string + wantErr bool + }{ + {name: "both", allowed: []string{"reverse-proxy", "transparent"}}, + {name: "forbid transparent", allowed: []string{"reverse-proxy"}}, + {name: "mandate transparent", allowed: []string{"transparent"}}, + { + // An empty list would make the fallback index panic, and "allow + // nothing" has no sensible meaning. + name: "empty is rejected", allowed: []string{}, wantErr: true, + }, + {name: "unknown value is rejected", allowed: []string{"reverse-proxy", "tranparent"}, wantErr: true}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + cfg := CompiledDefaults() + cfg.Proxy.AllowedInboundInterception = tc.allowed + err := cfg.Validate() + if tc.wantErr && err == nil { + t.Fatal("expected a validation error, got nil") + } + if !tc.wantErr && err != nil { + t.Fatalf("unexpected validation error: %v", err) + } + }) + } +} + +// TestCompiledDefaults_TransparentInboundDefaults pins the values that must stay +// in lockstep with authbridge's proxy-sidecar preset; a drift here silently +// redirects inbound traffic to a dead port. +func TestCompiledDefaults_TransparentInboundDefaults(t *testing.T) { + cfg := CompiledDefaults() + if cfg.Proxy.TransparentInboundPort != 8083 { + t.Errorf("TransparentInboundPort = %d, want 8083 (authbridge preset transparent_inbound_addr)", cfg.Proxy.TransparentInboundPort) + } + if len(cfg.Proxy.AllowedInboundInterception) != 2 { + t.Errorf("AllowedInboundInterception = %v, want both mechanisms allowed by default", cfg.Proxy.AllowedInboundInterception) + } + // Order matters: the first entry is the fallback when a workload requests a + // value outside the list, and the no-privilege shape must win. + if cfg.Proxy.AllowedInboundInterception[0] != "reverse-proxy" { + t.Errorf("AllowedInboundInterception[0] = %q, want reverse-proxy (the fallback must not grant NET_ADMIN)", + cfg.Proxy.AllowedInboundInterception[0]) + } +} diff --git a/operator/internal/webhook/config/types.go b/operator/internal/webhook/config/types.go index 9735b06b..7190644c 100644 --- a/operator/internal/webhook/config/types.go +++ b/operator/internal/webhook/config/types.go @@ -64,6 +64,20 @@ type ProxyConfig struct { // where auto-detection is wrong or undesired. IptablesCmd string `json:"iptablesCmd" yaml:"iptablesCmd"` + // TransparentInboundPort is the INBOUND transparent listener port — the + // PREROUTING REDIRECT target when inboundInterception is "transparent". + // MUST match the authbridge listener.transparent_inbound_addr (preset + // default :8083); a mismatch redirects inbound traffic to a dead port. + TransparentInboundPort int32 `json:"transparentInboundPort" yaml:"transparentInboundPort"` + + // AllowedInboundInterception restricts which inboundInterception values + // workloads in this cluster may select, mirroring + // AllowedEgressEnforcement. Transparent inbound requires a privileged + // proxy-init container, so a platform admin may want to forbid it + // (["reverse-proxy"]) or mandate it (["transparent"]). A resolved value + // outside the list falls back to the list's first entry. + AllowedInboundInterception []string `json:"allowedInboundInterception,omitempty" yaml:"allowedInboundInterception,omitempty"` + // AllowedEgressEnforcement restricts which egressEnforcement values // workloads (AgentRuntime CR / namespace ConfigMap) may select. // The webhook rejects resolved values not in this list, falling back @@ -108,6 +122,10 @@ func (c *PlatformConfig) DeepCopy() *PlatformConfig { copy(result.TokenExchange.DefaultScopes, c.TokenExchange.DefaultScopes) } + if c.Proxy.AllowedInboundInterception != nil { + result.Proxy.AllowedInboundInterception = make([]string, len(c.Proxy.AllowedInboundInterception)) + copy(result.Proxy.AllowedInboundInterception, c.Proxy.AllowedInboundInterception) + } if c.Proxy.AllowedEgressEnforcement != nil { result.Proxy.AllowedEgressEnforcement = make([]string, len(c.Proxy.AllowedEgressEnforcement)) copy(result.Proxy.AllowedEgressEnforcement, c.Proxy.AllowedEgressEnforcement) @@ -152,6 +170,15 @@ func (c *PlatformConfig) Validate() error { if c.Proxy.TransparentPort < 1024 || c.Proxy.TransparentPort > 65535 { return fmt.Errorf("proxy.transparentPort must be between 1024 and 65535") } + if c.Proxy.TransparentInboundPort < 1024 || c.Proxy.TransparentInboundPort > 65535 { + return fmt.Errorf("proxy.transparentInboundPort must be between 1024 and 65535") + } + // The two transparent listeners are separate sockets in one container; a + // shared value would make the second bind fail at pod start, after admission + // has already succeeded. Catch it at operator startup instead. + if c.Proxy.TransparentInboundPort == c.Proxy.TransparentPort { + return fmt.Errorf("proxy.transparentInboundPort (%d) must differ from proxy.transparentPort — they are distinct listeners in the same container", c.Proxy.TransparentInboundPort) + } // The enforce-redirect guard exempts this UID (--uid-owner) and the proxy // container runs as it; it must be a real non-root user. if c.Proxy.UID < 1 { @@ -166,6 +193,14 @@ func (c *PlatformConfig) Validate() error { default: return fmt.Errorf("proxy.iptablesCmd %q is not a recognized backend (want one of: \"\" (auto-detect), iptables, iptables-nft, iptables-legacy)", c.Proxy.IptablesCmd) } + if len(c.Proxy.AllowedInboundInterception) == 0 { + return fmt.Errorf("proxy.allowedInboundInterception must not be empty (set [\"reverse-proxy\"] to forbid transparent inbound, [\"transparent\"] to require it, or both to allow workload choice)") + } + for _, mode := range c.Proxy.AllowedInboundInterception { + if mode != "reverse-proxy" && mode != "transparent" { + return fmt.Errorf("proxy.allowedInboundInterception contains invalid value %q (allowed: reverse-proxy, transparent)", mode) + } + } if len(c.Proxy.AllowedEgressEnforcement) == 0 { return fmt.Errorf("proxy.allowedEgressEnforcement must not be empty (set [\"enforce-redirect\"] to require enforcement, [\"none\"] to disable it, or both to allow workload choice)") } diff --git a/operator/internal/webhook/injector/constants.go b/operator/internal/webhook/injector/constants.go index 66a0d605..c458b8f8 100644 --- a/operator/internal/webhook/injector/constants.go +++ b/operator/internal/webhook/injector/constants.go @@ -65,6 +65,38 @@ const ( EgressEnforcementNone = "none" ) +// AuthBridge's fixed (non-negotiable) listener ports in proxy-sidecar / lite. +// Unlike the reverse/forward/transparent ports these are not configurable, so +// the operator must exempt them from the transparent inbound REDIRECT by number. +// Gating AuthBridgeHealthPort in particular would put kubelet probes behind JWT +// validation and crash-loop the pod. +const ( + authBridgeHealthPort = 9091 // /healthz + authBridgeStatsPort = 9093 // stats + config inspection + /reload/status + authBridgeSessionAPIPort = 9094 // session events API (consumed by abctl) +) + +// Inbound interception mechanisms for the proxy-sidecar / lite paths. These +// select HOW inbound traffic reaches AuthBridge's inbound pipeline — they are +// not two levels of the same knob, but two different deployment shapes. +const ( + // InboundInterceptionReverseProxy is the default: port stealing. AuthBridge + // binds the agent's original port and the agent is relocated to a free one + // via the PORT env var, so the Service needs no patching. Requires no + // privileges, but leaves the relocated port reachable directly (a pod-to-pod + // bypass of JWT validation), only covers the first declared container port, + // and silently fails for agents that hardcode their listen port. + InboundInterceptionReverseProxy = "reverse-proxy" + + // InboundInterceptionTransparent installs a PREROUTING REDIRECT via + // proxy-init and lets AuthBridge recover each connection's real destination + // via SO_ORIGINAL_DST. The agent keeps its own port — no relocation, no PORT + // env var, no second port to discover — and every port it listens on is + // covered. Costs a privileged proxy-init container (NET_ADMIN) and is + // Linux-only, so it is opt-in. + InboundInterceptionTransparent = "transparent" +) + // mTLS modes for the proxy-sidecar / lite paths. Selected via the // namespace `authbridge-runtime-config` ConfigMap's `mtls.mode` field, // then MTLSModeDisabled. envoy-sidecar mode is incompatible with mTLS diff --git a/operator/internal/webhook/injector/container_builder.go b/operator/internal/webhook/injector/container_builder.go index 74bc607b..1af862d3 100644 --- a/operator/internal/webhook/injector/container_builder.go +++ b/operator/internal/webhook/injector/container_builder.go @@ -234,6 +234,28 @@ func (b *ContainerBuilder) BuildProxySidecarContainer(spireEnabled bool) corev1. // agentBackendPort: where the agent actually listens (moved to a free port) // forwardProxyPort: where the forward proxy listens (HTTP_PROXY target) func (b *ContainerBuilder) BuildProxySidecarContainerWithPorts(spireEnabled bool, image string, reverseProxyPort, agentBackendPort, forwardProxyPort int32) corev1.Container { + // agentBackendPort is not rendered into the container: it is where the agent + // was relocated to, which the sidecar learns from its ConfigMap + // (reverse_proxy_backend), not from a declared port. + _ = agentBackendPort + return b.buildProxySidecarContainer(spireEnabled, image, "reverse-proxy", reverseProxyPort, forwardProxyPort) +} + +// BuildProxySidecarContainerTransparent builds the sidecar for +// inboundInterception "transparent": AuthBridge binds its own inbound listener +// on transparentInboundPort and iptables REDIRECTs to it, so the agent keeps its +// own port and there is no relocated backend port to declare. +// +// The declared port is named "transparent-in" rather than "reverse-proxy" so +// `kubectl describe pod` reflects which inbound shape is actually running — +// otherwise the two are indistinguishable from the pod spec. +func (b *ContainerBuilder) BuildProxySidecarContainerTransparent(spireEnabled bool, image string, transparentInboundPort, forwardProxyPort int32) corev1.Container { + // No backend port: with transparent interception the forwarding target is + // resolved per connection from SO_ORIGINAL_DST, not configured. + return b.buildProxySidecarContainer(spireEnabled, image, "transparent-in", transparentInboundPort, forwardProxyPort) +} + +func (b *ContainerBuilder) buildProxySidecarContainer(spireEnabled bool, image, inboundPortName string, inboundPort, forwardProxyPort int32) corev1.Container { volumeMounts := []corev1.VolumeMount{ { Name: "shared-data", @@ -287,8 +309,8 @@ func (b *ContainerBuilder) BuildProxySidecarContainerWithPorts(spireEnabled bool }, Ports: []corev1.ContainerPort{ { - Name: "reverse-proxy", - ContainerPort: reverseProxyPort, + Name: inboundPortName, + ContainerPort: inboundPort, Protocol: corev1.ProtocolTCP, }, { @@ -465,6 +487,35 @@ const mandatoryOutboundExclude = "8080" // TRANSPARENT_PORT; the exclude args do not apply. Cluster DNS is kept // direct by the init script reading the pod's resolv.conf nameservers. func (b *ContainerBuilder) BuildProxyInitContainer(mode ProxyInitMode, outboundPortsExclude, inboundPortsExclude string) corev1.Container { + return b.BuildProxyInitContainerWithInbound(mode, outboundPortsExclude, inboundPortsExclude, ProxyInitInbound{}) +} + +// ProxyInitInbound carries the transparent-inbound additions to proxy-init's +// enforce-redirect mode. The zero value leaves inbound interception off, which +// is what BuildProxyInitContainer passes — so existing callers keep egress-only +// behavior with no change. +type ProxyInitInbound struct { + // Port is the PREROUTING REDIRECT target (INBOUND_TRANSPARENT_PORT). Zero + // means inbound interception is off. + Port int32 + + // SidecarPortsExclude is the comma-separated list of AuthBridge's own + // listeners to exempt from the inbound REDIRECT. The operator supplies the + // real forward-proxy port here, which may not be the script's default 8081 + // when findFreePort had to move it. + SidecarPortsExclude string + + // PortsExclude is the operator/user app-port exemption list + // (kagenti.io/inbound-ports-exclude), for app ports that must not be + // validated — e.g. an oauth-proxy doing its own authentication. + PortsExclude string +} + +// BuildProxyInitContainerWithInbound is BuildProxyInitContainer plus the +// transparent-inbound environment. Split out rather than folded into the +// existing signature so the redirect-mode and egress-only callers stay +// untouched. +func (b *ContainerBuilder) BuildProxyInitContainerWithInbound(mode ProxyInitMode, outboundPortsExclude, inboundPortsExclude string, inbound ProxyInitInbound) corev1.Container { var env []corev1.EnvVar switch mode { case ProxyInitModeEnforceRedirect: @@ -479,10 +530,34 @@ func (b *ContainerBuilder) BuildProxyInitContainer(mode ProxyInitMode, outboundP {Name: "PROXY_UID", Value: fmt.Sprintf("%d", b.cfg.Proxy.UID)}, {Name: "TRANSPARENT_PORT", Value: fmt.Sprintf("%d", b.cfg.Proxy.TransparentPort)}, } + // Transparent inbound (opt-in). POD_IP is REQUIRED alongside the port: + // the init script uses it as the DNAT target for the Istio ambient inbound + // path, which arrives through OUTPUT rather than PREROUTING. The script + // refuses to start without it rather than install PREROUTING-only rules + // that would wave all mesh traffic through unvalidated. + if inbound.Port > 0 { + env = append(env, + corev1.EnvVar{Name: "INBOUND_TRANSPARENT_PORT", Value: fmt.Sprintf("%d", inbound.Port)}, + corev1.EnvVar{ + Name: "POD_IP", + ValueFrom: &corev1.EnvVarSource{ + FieldRef: &corev1.ObjectFieldSelector{FieldPath: "status.podIP"}, + }, + }, + ) + if inbound.SidecarPortsExclude != "" { + env = append(env, corev1.EnvVar{Name: "SIDECAR_PORTS_EXCLUDE", Value: inbound.SidecarPortsExclude}) + } + if v := buildPortExcludeValue(inbound.PortsExclude, "inbound-ports-exclude"); v != "" { + env = append(env, corev1.EnvVar{Name: "INBOUND_PORTS_EXCLUDE", Value: v}) + } + } builderLog.Info("building ProxyInit Container", "mode", "enforce-redirect", "proxyUID", b.cfg.Proxy.UID, - "transparentPort", b.cfg.Proxy.TransparentPort) + "transparentPort", b.cfg.Proxy.TransparentPort, + "inboundTransparentPort", inbound.Port, + "sidecarPortsExclude", inbound.SidecarPortsExclude) case ProxyInitModeRedirect: outboundValue := buildOutboundExcludeValue(outboundPortsExclude) inboundValue := buildPortExcludeValue(inboundPortsExclude, "inbound-ports-exclude") diff --git a/operator/internal/webhook/injector/namespace_config.go b/operator/internal/webhook/injector/namespace_config.go index 3d263044..00d81332 100644 --- a/operator/internal/webhook/injector/namespace_config.go +++ b/operator/internal/webhook/injector/namespace_config.go @@ -144,6 +144,25 @@ func ExtractMode(authbridgeYAML string) string { return top.Mode } +// ExtractInboundInterception parses an authbridge-runtime-config config.yaml +// string and returns the value of its top-level `inboundInterception:` key. +// Returns "" if the YAML is empty, malformed, or has no `inboundInterception` +// key, so the caller falls through to the next resolution layer. +func ExtractInboundInterception(authbridgeYAML string) string { + if authbridgeYAML == "" { + return "" + } + var top struct { + InboundInterception string `json:"inboundInterception"` + } + if err := yaml.Unmarshal([]byte(authbridgeYAML), &top); err != nil { + nsConfigLog.Info("WARN: failed to parse authbridge-runtime-config config.yaml for inboundInterception; falling back to next resolution layer", + "error", err) + return "" + } + return top.InboundInterception +} + // ExtractEgressEnforcement parses an authbridge-runtime-config config.yaml // string and returns the value of its top-level `egressEnforcement:` key. // Returns "" if the YAML is empty, malformed, or has no `egressEnforcement` diff --git a/operator/internal/webhook/injector/pod_mutator.go b/operator/internal/webhook/injector/pod_mutator.go index 8f07928f..7c0479ec 100644 --- a/operator/internal/webhook/injector/pod_mutator.go +++ b/operator/internal/webhook/injector/pod_mutator.go @@ -69,6 +69,10 @@ const ( InboundPortsExcludeAnnotation = "rossoctl.io/inbound-ports-exclude" sourceNamespaceConfigMap = "namespace-configmap" + // Resolution-source labels shared by the mode / egress / inbound resolvers, + // so the log strings stay identical across them. + sourceClusterDefault = "cluster-default" + sourceInvalidFallback = "default-invalid-fallback" // RossoctlTypeLabel is the label key that identifies the workload type RossoctlTypeLabel = "rossoctl.io/type" @@ -321,7 +325,7 @@ func (m *PodMutator) InjectAuthBridge(ctx context.Context, podSpec *corev1.PodSp } if egressEnforcement == "" { egressEnforcement = EgressEnforcementEnforceRedirect - egressEnforcementSource = "cluster-default" + egressEnforcementSource = sourceClusterDefault } switch egressEnforcement { case EgressEnforcementEnforceRedirect, EgressEnforcementNone: @@ -331,7 +335,7 @@ func (m *PodMutator) InjectAuthBridge(ctx context.Context, podSpec *corev1.PodSp "namespace", namespace, "crName", crName, "unrecognized", egressEnforcement, "source", egressEnforcementSource) egressEnforcement = EgressEnforcementEnforceRedirect - egressEnforcementSource = "default-invalid-fallback" + egressEnforcementSource = sourceInvalidFallback } // Validate against the platform's allowed list. If the resolved value // is not permitted, fall back to the first allowed value (fail closed @@ -350,6 +354,61 @@ func (m *PodMutator) InjectAuthBridge(ctx context.Context, podSpec *corev1.PodSp "namespace", namespace, "crName", crName, "mode", egressEnforcement, "source", egressEnforcementSource) + // ======================================== + // Resolve inboundInterception (namespace > reverse-proxy) + // ======================================== + // + // Selects the inbound deployment shape in proxy-sidecar / lite modes: + // "reverse-proxy" (default) steals the agent's port; "transparent" installs a + // PREROUTING REDIRECT and lets AuthBridge recover the real destination. + // envoy-sidecar ignores this — Envoy already intercepts inbound. + inboundInterception := "" + inboundInterceptionSource := "" + if ii := ExtractInboundInterception(nsConfig.AuthBridgeRuntimeYAML); ii != "" { + inboundInterception = ii + inboundInterceptionSource = sourceNamespaceConfigMap + } + if inboundInterception == "" { + inboundInterception = InboundInterceptionReverseProxy + inboundInterceptionSource = sourceClusterDefault + } + switch inboundInterception { + case InboundInterceptionReverseProxy, InboundInterceptionTransparent: + // recognized, keep as-is + default: + // Fall back to the shape that needs no privileges: an unrecognized value + // must not silently grant a NET_ADMIN init container. + mutatorLog.Info("WARN: unrecognized inboundInterception; defaulting to reverse-proxy", + "namespace", namespace, "crName", crName, + "unrecognized", inboundInterception, "source", inboundInterceptionSource) + inboundInterception = InboundInterceptionReverseProxy + inboundInterceptionSource = sourceInvalidFallback + } + allowedInbound := currentConfig.Proxy.AllowedInboundInterception + if len(allowedInbound) > 0 && !slices.Contains(allowedInbound, inboundInterception) { + mutatorLog.Info("WARN: inboundInterception value not in platform allowedInboundInterception; overriding", + "namespace", namespace, "crName", crName, + "requested", inboundInterception, "allowed", allowedInbound, + "overrideTo", allowedInbound[0]) + inboundInterception = allowedInbound[0] + inboundInterceptionSource = "platform-policy-override" + } + // Transparent inbound and the egress guard share one proxy-init container. + // With egressEnforcement "none" there is no init container to carry the + // PREROUTING rules, so nothing would REDIRECT to the inbound listener and it + // would bind a port that never receives traffic — inbound silently unenforced. + // Fall back to port stealing, which at least validates Service-routed traffic. + if inboundInterception == InboundInterceptionTransparent && egressEnforcement != EgressEnforcementEnforceRedirect { + mutatorLog.Info("WARN: inboundInterception=transparent requires egressEnforcement=enforce-redirect (shared proxy-init); falling back to reverse-proxy", + "namespace", namespace, "crName", crName, + "egressEnforcement", egressEnforcement) + inboundInterception = InboundInterceptionReverseProxy + inboundInterceptionSource = "requires-enforce-redirect-fallback" + } + mutatorLog.Info("resolved inbound interception", + "namespace", namespace, "crName", crName, + "mechanism", inboundInterception, "source", inboundInterceptionSource) + // ======================================== // Resolve TLS bridge posture (CR > namespace > "disabled") // ======================================== @@ -537,21 +596,49 @@ func (m *PodMutator) InjectAuthBridge(ctx context.Context, podSpec *corev1.PodSp // Reserve the original agent port for the reverse proxy usedPorts[originalAgentPort] = true - newAgentPort, err := findFreePort(originalAgentPort + 1) - if err != nil { - return false, fmt.Errorf("proxy-sidecar port assignment: %w", err) + transparentInbound := inboundInterception == InboundInterceptionTransparent + + // The inbound transparent listener needs its own port reserved so + // findFreePort can't hand it to the forward proxy. + transparentInboundPort := builder.cfg.Proxy.TransparentInboundPort + if transparentInbound { + usedPorts[transparentInboundPort] = true + } + + // newAgentPort exists only for the port-stealing shape. Transparent + // interception leaves the agent exactly where it is, so there is nothing + // to relocate and no relocated port for anyone to reach around us. + var newAgentPort int32 + if !transparentInbound { + p, perr := findFreePort(originalAgentPort + 1) + if perr != nil { + return false, fmt.Errorf("proxy-sidecar port assignment: %w", perr) + } + newAgentPort = p } forwardProxyPort, err := findFreePort(8081) if err != nil { return false, fmt.Errorf("proxy-sidecar port assignment: %w", err) } - // Move the agent to the free port. - // Most agent frameworks (Python/uvicorn, Node/express, FastAPI) read the - // PORT env var to determine the bind address. Go agents that hardcode their - // listen port won't be affected by this env var — they must use PORT or - // be configured via their own config mechanism. - if agentContainer != nil { + if transparentInbound { + // No port mutation and no PORT env var: iptables REDIRECTs inbound + // traffic to AuthBridge, which recovers the port the client actually + // addressed via SO_ORIGINAL_DST and forwards there over loopback. That + // removes the two failure modes of port stealing — an agent that + // hardcodes its listen port (which would collide with AuthBridge) and + // the unvalidated relocated port — and covers every port the agent + // listens on, not just the first declared one. + mutatorLog.Info("proxy-sidecar transparent inbound (no port stealing)", + "agentPort", originalAgentPort, + "transparentInboundPort", transparentInboundPort, + "forwardProxyPort", forwardProxyPort) + } else if agentContainer != nil { + // Move the agent to the free port. + // Most agent frameworks (Python/uvicorn, Node/express, FastAPI) read the + // PORT env var to determine the bind address. Go agents that hardcode their + // listen port won't be affected by this env var — they must use PORT or + // be configured via their own config mechanism. agentContainer.Ports[0].ContainerPort = newAgentPort setOrAddEnv(agentContainer, "PORT", fmt.Sprintf("%d", newAgentPort)) mutatorLog.Info("proxy-sidecar port stealing", @@ -562,13 +649,24 @@ func (m *PodMutator) InjectAuthBridge(ctx context.Context, podSpec *corev1.PodSp } // Create per-agent ConfigMap with proxy-sidecar listener addresses + // Transparent interception derives the backend per connection, so it must + // NOT get reverse_proxy_addr / reverse_proxy_backend: authbridge's config + // validation rejects a reverse_proxy_addr alongside inbound_interception + // transparent, and binding it would either be dead or collide with the + // agent's own port. + listenerOverrides := map[string]string{ + "forward_proxy_addr": fmt.Sprintf(":%d", forwardProxyPort), + } + if transparentInbound { + listenerOverrides["inbound_interception"] = InboundInterceptionTransparent + listenerOverrides["transparent_inbound_addr"] = fmt.Sprintf(":%d", transparentInboundPort) + } else { + listenerOverrides["reverse_proxy_addr"] = fmt.Sprintf(":%d", originalAgentPort) + listenerOverrides["reverse_proxy_backend"] = fmt.Sprintf("http://127.0.0.1:%d", newAgentPort) + } perAgentCMName, err := m.ensurePerAgentConfigMap(ctx, namespace, crName, ModeProxySidecar, nsConfig.AuthBridgeRuntimeYAML, nsConfig, - map[string]string{ - "reverse_proxy_addr": fmt.Sprintf(":%d", originalAgentPort), - "reverse_proxy_backend": fmt.Sprintf("http://127.0.0.1:%d", newAgentPort), - "forward_proxy_addr": fmt.Sprintf(":%d", forwardProxyPort), - }, + listenerOverrides, mtlsMode, tlsBridgeMode, spireEnabled, agentRuntime) if err != nil { return false, fmt.Errorf("proxy-sidecar per-agent ConfigMap: %w", err) @@ -576,14 +674,24 @@ func (m *PodMutator) InjectAuthBridge(ctx context.Context, podSpec *corev1.PodSp // Inject authbridge-proxy container listening on the original agent port if !containerExists(podSpec.Containers, AuthBridgeProxyContainerName) { - podSpec.Containers = append(podSpec.Containers, - builder.BuildProxySidecarContainerWithPorts( + var sidecar corev1.Container + if transparentInbound { + sidecar = builder.BuildProxySidecarContainerTransparent( + spireEnabled, + proxyImage, + transparentInboundPort, // iptables REDIRECTs inbound here + forwardProxyPort, // forward proxy listens here + ) + } else { + sidecar = builder.BuildProxySidecarContainerWithPorts( spireEnabled, proxyImage, originalAgentPort, // reverse proxy listens here newAgentPort, // forwards to agent here forwardProxyPort, // forward proxy listens here - )) + ) + } + podSpec.Containers = append(podSpec.Containers, sidecar) } // Set MTLS_MODE env var on the authbridge container so it knows the @@ -615,10 +723,26 @@ func (m *PodMutator) InjectAuthBridge(ctx context.Context, podSpec *corev1.PodSp // this setting. if egressEnforcement == EgressEnforcementEnforceRedirect { if !containerExists(podSpec.InitContainers, ProxyInitContainerName) { + // Transparent inbound rides on the same proxy-init container as the + // egress guard; its PREROUTING rules are installed only when + // INBOUND_TRANSPARENT_PORT is set. SIDECAR_PORTS_EXCLUDE carries the + // RESOLVED forward-proxy port, which findFreePort may have moved off + // 8081 — the script's default would then leave it unexempted and the + // inbound REDIRECT would swallow the forward proxy's own port. + var inbound ProxyInitInbound + if transparentInbound { + inbound = ProxyInitInbound{ + Port: transparentInboundPort, + SidecarPortsExclude: fmt.Sprintf("%d,%d,%d,%d", + forwardProxyPort, authBridgeHealthPort, authBridgeStatsPort, authBridgeSessionAPIPort), + PortsExclude: annotations[InboundPortsExcludeAnnotation], + } + } podSpec.InitContainers = append(podSpec.InitContainers, - builder.BuildProxyInitContainer(ProxyInitModeEnforceRedirect, "", "")) + builder.BuildProxyInitContainerWithInbound(ProxyInitModeEnforceRedirect, "", "", inbound)) mutatorLog.Info("proxy-sidecar egress enforcement enabled (enforce-redirect)", - "namespace", namespace, "crName", crName) + "namespace", namespace, "crName", crName, + "inboundInterception", inboundInterception) } } else { mutatorLog.Info("proxy-sidecar egress enforcement disabled (cooperative mode)", diff --git a/operator/internal/webhook/injector/transparent_inbound_test.go b/operator/internal/webhook/injector/transparent_inbound_test.go new file mode 100644 index 00000000..955773dd --- /dev/null +++ b/operator/internal/webhook/injector/transparent_inbound_test.go @@ -0,0 +1,375 @@ +package injector + +import ( + "context" + "fmt" + "strings" + "testing" + + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + "github.com/rossoctl/operator/internal/webhook/config" +) + +// inboundCM builds a namespace authbridge-runtime-config pinning the inbound +// mechanism (and optionally the egress posture, which transparent depends on). +func inboundCM(inbound, egress string) *corev1.ConfigMap { + yaml := "mode: " + ModeProxySidecar + "\n" + if inbound != "" { + yaml += "inboundInterception: " + inbound + "\n" + } + if egress != "" { + yaml += "egressEnforcement: " + egress + "\n" + } + return &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: AuthBridgeRuntimeConfigMapName, Namespace: "test-ns"}, + Data: map[string]string{"config.yaml": yaml}, + } +} + +func newTestMutatorWithAllowedInbound(allowed []string, objs ...client.Object) *PodMutator { + scheme := runtime.NewScheme() + _ = corev1.AddToScheme(scheme) + _ = appsv1.AddToScheme(scheme) + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objs...).Build() + return &PodMutator{ + Client: fakeClient, + APIReader: fakeClient, + GetPlatformConfig: func() *config.PlatformConfig { + cfg := config.CompiledDefaults() + cfg.Proxy.AllowedInboundInterception = allowed + return cfg + }, + GetFeatureGates: config.DefaultFeatureGates, + } +} + +func agentPodSpec() *corev1.PodSpec { + return &corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "agent", + Image: "my-agent:latest", + Ports: []corev1.ContainerPort{{ContainerPort: 8000}}, + }}, + } +} + +func containerByName(spec *corev1.PodSpec, name string) *corev1.Container { + for i := range spec.Containers { + if spec.Containers[i].Name == name { + return &spec.Containers[i] + } + } + for i := range spec.InitContainers { + if spec.InitContainers[i].Name == name { + return &spec.InitContainers[i] + } + } + return nil +} + +func envVarOf(c *corev1.Container, name string) (corev1.EnvVar, bool) { + for _, e := range c.Env { + if e.Name == name { + return e, true + } + } + return corev1.EnvVar{}, false +} + +// TestTransparentInbound_DoesNotStealAgentPort is the central behavioral claim: +// transparent interception leaves the agent exactly where it is. That removes +// both failure modes of port stealing — an agent that hardcodes its listen port +// (which collides with AuthBridge) and the relocated port that another pod can +// reach without JWT validation. +func TestTransparentInbound_DoesNotStealAgentPort(t *testing.T) { + m := newTestMutator(inboundCM(InboundInterceptionTransparent, "")) + ctx := context.Background() + podSpec := agentPodSpec() + + injected, err := m.InjectAuthBridge(ctx, podSpec, "test-ns", "my-agent", "Deployment", + map[string]string{RossoctlTypeLabel: RossoctlTypeAgent}, nil) + if err != nil { + t.Fatalf("InjectAuthBridge() error: %v", err) + } + if !injected { + t.Fatal("expected injection") + } + + agent := containerByName(podSpec, "agent") + if agent == nil { + t.Fatal("agent container disappeared") + } + if got := agent.Ports[0].ContainerPort; got != 8000 { + t.Errorf("agent port = %d, want 8000 unchanged (transparent must not relocate the agent)", got) + } + if _, ok := envVarOf(agent, "PORT"); ok { + t.Error("transparent inbound must not inject PORT — the agent keeps its own port") + } +} + +// TestReverseProxyDefault_StillStealsPort locks the opt-in contract: with the +// field unset, behavior must be byte-identical to before this feature existed. +func TestReverseProxyDefault_StillStealsPort(t *testing.T) { + m := newTestMutator() + ctx := context.Background() + podSpec := agentPodSpec() + + if _, err := m.InjectAuthBridge(ctx, podSpec, "test-ns", "my-agent", "Deployment", + map[string]string{RossoctlTypeLabel: RossoctlTypeAgent}, nil); err != nil { + t.Fatalf("InjectAuthBridge() error: %v", err) + } + + agent := containerByName(podSpec, "agent") + if got := agent.Ports[0].ContainerPort; got == 8000 { + t.Error("default (reverse-proxy) must still relocate the agent off its original port") + } + if _, ok := envVarOf(agent, "PORT"); !ok { + t.Error("default (reverse-proxy) must still inject PORT") + } +} + +// TestTransparentInbound_SidecarDeclaresTransparentPort checks the sidecar binds +// the REDIRECT target and no longer claims the agent's port. +func TestTransparentInbound_SidecarDeclaresTransparentPort(t *testing.T) { + m := newTestMutator(inboundCM(InboundInterceptionTransparent, "")) + ctx := context.Background() + podSpec := agentPodSpec() + + if _, err := m.InjectAuthBridge(ctx, podSpec, "test-ns", "my-agent", "Deployment", + map[string]string{RossoctlTypeLabel: RossoctlTypeAgent}, nil); err != nil { + t.Fatalf("InjectAuthBridge() error: %v", err) + } + + proxy := containerByName(podSpec, AuthBridgeProxyContainerName) + if proxy == nil { + t.Fatal("authbridge-proxy container not injected") + } + names := make([]string, 0, len(proxy.Ports)) + found := false + for _, p := range proxy.Ports { + names = append(names, fmt.Sprintf("%s=%d", p.Name, p.ContainerPort)) + if p.ContainerPort == 8083 { + found = true + if p.Name != "transparent-in" { + t.Errorf("inbound port name = %q, want transparent-in (so the shape is visible in the pod spec)", p.Name) + } + } + if p.ContainerPort == 8000 { + t.Error("sidecar must not claim the agent's port under transparent interception") + } + } + if !found { + t.Errorf("sidecar does not declare the transparent inbound port 8083; got %v", names) + } +} + +// TestTransparentInbound_ConfigMapOmitsReverseProxyKeys matters because +// authbridge's own config validation REJECTS a reverse_proxy_addr alongside +// inbound_interception: transparent — emitting both would crash-loop the pod. +func TestTransparentInbound_ConfigMapOmitsReverseProxyKeys(t *testing.T) { + m := newTestMutator(inboundCM(InboundInterceptionTransparent, "")) + ctx := context.Background() + + if _, err := m.InjectAuthBridge(ctx, agentPodSpec(), "test-ns", "my-agent", "Deployment", + map[string]string{RossoctlTypeLabel: RossoctlTypeAgent}, nil); err != nil { + t.Fatalf("InjectAuthBridge() error: %v", err) + } + + cm := fetchConfigMap(t, m, "test-ns", perAgentConfigMapName("my-agent")) + cfg := parseConfigYAML(t, cm) + listener, ok := cfg["listener"].(map[string]interface{}) + if !ok { + t.Fatalf("listener block missing or not a map: %T", cfg["listener"]) + } + + if listener["inbound_interception"] != InboundInterceptionTransparent { + t.Errorf("inbound_interception = %v, want %s", listener["inbound_interception"], InboundInterceptionTransparent) + } + if listener["transparent_inbound_addr"] != ":8083" { + t.Errorf("transparent_inbound_addr = %v, want :8083", listener["transparent_inbound_addr"]) + } + for _, key := range []string{"reverse_proxy_addr", "reverse_proxy_backend"} { + if v, present := listener[key]; present { + t.Errorf("%s must be absent under transparent interception (authbridge rejects it), got %v", key, v) + } + } + // Egress is unaffected. + if listener["forward_proxy_addr"] == nil { + t.Error("forward_proxy_addr must still be set — transparent inbound changes only ingress") + } +} + +// TestTransparentInbound_ProxyInitEnv covers the two env vars without which the +// feature is either inert or half-enforced: the REDIRECT target, and POD_IP for +// the Istio ambient path that never traverses PREROUTING. +func TestTransparentInbound_ProxyInitEnv(t *testing.T) { + m := newTestMutator(inboundCM(InboundInterceptionTransparent, "")) + ctx := context.Background() + podSpec := agentPodSpec() + + if _, err := m.InjectAuthBridge(ctx, podSpec, "test-ns", "my-agent", "Deployment", + map[string]string{RossoctlTypeLabel: RossoctlTypeAgent}, nil); err != nil { + t.Fatalf("InjectAuthBridge() error: %v", err) + } + + init := containerByName(podSpec, ProxyInitContainerName) + if init == nil { + t.Fatal("proxy-init not injected — transparent inbound has nothing to REDIRECT with") + } + + port, ok := envVarOf(init, "INBOUND_TRANSPARENT_PORT") + if !ok || port.Value != "8083" { + t.Errorf("INBOUND_TRANSPARENT_PORT = %q (present=%v), want 8083", port.Value, ok) + } + + podIP, ok := envVarOf(init, "POD_IP") + if !ok { + t.Fatal("POD_IP missing: without it proxy-init cannot capture the ambient inbound path and refuses to start") + } + if podIP.ValueFrom == nil || podIP.ValueFrom.FieldRef == nil || + podIP.ValueFrom.FieldRef.FieldPath != "status.podIP" { + t.Errorf("POD_IP must come from the downward API status.podIP, got %+v", podIP.ValueFrom) + } + + // SIDECAR_PORTS_EXCLUDE must carry the RESOLVED forward-proxy port. If the + // script's 8081 default were used and findFreePort had moved it, the inbound + // REDIRECT would swallow the forward proxy's own port. + excl, ok := envVarOf(init, "SIDECAR_PORTS_EXCLUDE") + if !ok { + t.Fatal("SIDECAR_PORTS_EXCLUDE missing — health/stats/session ports would be JWT-gated") + } + for _, want := range []string{"9091", "9093", "9094"} { + if !strings.Contains(excl.Value, want) { + t.Errorf("SIDECAR_PORTS_EXCLUDE=%q missing %s", excl.Value, want) + } + } + + proxy := containerByName(podSpec, AuthBridgeProxyContainerName) + var fwd int32 + for _, p := range proxy.Ports { + if p.Name == "forward-proxy" { + fwd = p.ContainerPort + } + } + if fwd == 0 { + t.Fatal("could not determine the resolved forward-proxy port") + } + if !strings.Contains(excl.Value, fmt.Sprintf("%d", fwd)) { + t.Errorf("SIDECAR_PORTS_EXCLUDE=%q must contain the resolved forward-proxy port %d", excl.Value, fwd) + } +} + +// TestTransparentInbound_HonorsInboundPortsExcludeAnnotation covers the escape +// hatch for app ports that must not be validated (e.g. an oauth-proxy doing its +// own auth). +func TestTransparentInbound_HonorsInboundPortsExcludeAnnotation(t *testing.T) { + m := newTestMutator(inboundCM(InboundInterceptionTransparent, "")) + ctx := context.Background() + podSpec := agentPodSpec() + + annotations := map[string]string{InboundPortsExcludeAnnotation: "8443"} + if _, err := m.InjectAuthBridge(ctx, podSpec, "test-ns", "my-agent", "Deployment", + map[string]string{RossoctlTypeLabel: RossoctlTypeAgent}, annotations); err != nil { + t.Fatalf("InjectAuthBridge() error: %v", err) + } + + init := containerByName(podSpec, ProxyInitContainerName) + excl, ok := envVarOf(init, "INBOUND_PORTS_EXCLUDE") + if !ok || !strings.Contains(excl.Value, "8443") { + t.Errorf("INBOUND_PORTS_EXCLUDE = %q (present=%v), want it to contain 8443", excl.Value, ok) + } +} + +// TestTransparentInbound_RequiresEnforceRedirect: the two features share one +// proxy-init container. With egressEnforcement "none" there is no init container +// to carry the PREROUTING rules, so the listener would bind a port nothing +// redirects to — inbound silently unenforced. Fall back to port stealing, which +// at least validates Service-routed traffic. +func TestTransparentInbound_RequiresEnforceRedirect(t *testing.T) { + m := newTestMutator(inboundCM(InboundInterceptionTransparent, EgressEnforcementNone)) + ctx := context.Background() + podSpec := agentPodSpec() + + if _, err := m.InjectAuthBridge(ctx, podSpec, "test-ns", "my-agent", "Deployment", + map[string]string{RossoctlTypeLabel: RossoctlTypeAgent}, nil); err != nil { + t.Fatalf("InjectAuthBridge() error: %v", err) + } + + agent := containerByName(podSpec, "agent") + if got := agent.Ports[0].ContainerPort; got == 8000 { + t.Error("expected fallback to port stealing when egressEnforcement=none (no proxy-init to REDIRECT with)") + } + cm := fetchConfigMap(t, m, "test-ns", perAgentConfigMapName("my-agent")) + listener := parseConfigYAML(t, cm)["listener"].(map[string]interface{}) + if _, present := listener["transparent_inbound_addr"]; present { + t.Error("must not configure a transparent inbound listener that nothing can REDIRECT to") + } +} + +// TestTransparentInbound_PlatformPolicyCanForbid: transparent inbound grants a +// NET_ADMIN init container, so an admin must be able to forbid it cluster-wide. +func TestTransparentInbound_PlatformPolicyCanForbid(t *testing.T) { + m := newTestMutatorWithAllowedInbound( + []string{InboundInterceptionReverseProxy}, + inboundCM(InboundInterceptionTransparent, ""), + ) + ctx := context.Background() + podSpec := agentPodSpec() + + if _, err := m.InjectAuthBridge(ctx, podSpec, "test-ns", "my-agent", "Deployment", + map[string]string{RossoctlTypeLabel: RossoctlTypeAgent}, nil); err != nil { + t.Fatalf("InjectAuthBridge() error: %v", err) + } + + agent := containerByName(podSpec, "agent") + if got := agent.Ports[0].ContainerPort; got == 8000 { + t.Error("platform policy forbidding transparent must force the port-stealing shape") + } +} + +// TestTransparentInbound_UnrecognizedValueFallsBackToNoPrivilege: an unknown +// value must not silently grant a privileged init container. +func TestTransparentInbound_UnrecognizedValueFallsBackToNoPrivilege(t *testing.T) { + m := newTestMutator(inboundCM("tranparent", "")) // plausible typo (dropped letter) + ctx := context.Background() + podSpec := agentPodSpec() + + if _, err := m.InjectAuthBridge(ctx, podSpec, "test-ns", "my-agent", "Deployment", + map[string]string{RossoctlTypeLabel: RossoctlTypeAgent}, nil); err != nil { + t.Fatalf("InjectAuthBridge() error: %v", err) + } + + init := containerByName(podSpec, ProxyInitContainerName) + if init != nil { + if _, ok := envVarOf(init, "INBOUND_TRANSPARENT_PORT"); ok { + t.Error("an unrecognized inboundInterception must not enable inbound capture") + } + } + agent := containerByName(podSpec, "agent") + if got := agent.Ports[0].ContainerPort; got == 8000 { + t.Error("unrecognized value must fall back to the port-stealing default") + } +} + +// TestExtractInboundInterception covers the namespace-config parse layer, +// including malformed YAML falling through rather than erroring. +func TestExtractInboundInterception(t *testing.T) { + for _, tc := range []struct{ name, yaml, want string }{ + {"present", "mode: proxy-sidecar\ninboundInterception: transparent\n", "transparent"}, + {"absent", "mode: proxy-sidecar\n", ""}, + {"empty", "", ""}, + {"malformed falls through", "mode: [unclosed\n", ""}, + } { + t.Run(tc.name, func(t *testing.T) { + if got := ExtractInboundInterception(tc.yaml); got != tc.want { + t.Errorf("ExtractInboundInterception(%q) = %q, want %q", tc.yaml, got, tc.want) + } + }) + } +} From 9e02256c3ed2f3e5011e52f4d0fe399ebe7c334e Mon Sep 17 00:00:00 2001 From: Hai Huang Date: Tue, 18 Aug 2026 15:37:32 -0400 Subject: [PATCH 2/4] fix(operator): Inject POD_IPS so ambient inbound covers both families MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit proxy-init's ambient DNAT target must match the address family of the traffic. POD_IP is the pod's PRIMARY address (usually v4), so on a dual-stack pod the other family's HBONE delivery passed unvalidated while that family's PREROUTING rules were installed — half-enforcement, which the POD_IP guard refuses to ship in the equivalent case. Adds POD_IPS from the Downward API (status.podIPs) alongside POD_IP. proxy-init falls back to POD_IP when absent, so an older init image still works for its own family. Refs: rossoctl/cortex#330 Assisted-By: Claude (Anthropic AI) Signed-off-by: Hai Huang --- .../internal/webhook/injector/container_builder.go | 12 ++++++++++++ .../webhook/injector/transparent_inbound_test.go | 12 ++++++++++++ 2 files changed, 24 insertions(+) diff --git a/operator/internal/webhook/injector/container_builder.go b/operator/internal/webhook/injector/container_builder.go index 1af862d3..32ee753c 100644 --- a/operator/internal/webhook/injector/container_builder.go +++ b/operator/internal/webhook/injector/container_builder.go @@ -544,6 +544,18 @@ func (b *ContainerBuilder) BuildProxyInitContainerWithInbound(mode ProxyInitMode FieldRef: &corev1.ObjectFieldSelector{FieldPath: "status.podIP"}, }, }, + // POD_IPS carries BOTH families on a dual-stack pod. POD_IP alone is + // the primary address (usually v4), and the ambient DNAT target must + // match the family of the traffic — so keying only off POD_IP leaves + // the other family's HBONE delivery passing unvalidated while its + // PREROUTING rules are installed. proxy-init falls back to POD_IP + // when this is absent. + corev1.EnvVar{ + Name: "POD_IPS", + ValueFrom: &corev1.EnvVarSource{ + FieldRef: &corev1.ObjectFieldSelector{FieldPath: "status.podIPs"}, + }, + }, ) if inbound.SidecarPortsExclude != "" { env = append(env, corev1.EnvVar{Name: "SIDECAR_PORTS_EXCLUDE", Value: inbound.SidecarPortsExclude}) diff --git a/operator/internal/webhook/injector/transparent_inbound_test.go b/operator/internal/webhook/injector/transparent_inbound_test.go index 955773dd..8d92418a 100644 --- a/operator/internal/webhook/injector/transparent_inbound_test.go +++ b/operator/internal/webhook/injector/transparent_inbound_test.go @@ -237,6 +237,18 @@ func TestTransparentInbound_ProxyInitEnv(t *testing.T) { t.Errorf("POD_IP must come from the downward API status.podIP, got %+v", podIP.ValueFrom) } + // POD_IPS covers the dual-stack case: with only POD_IP (the primary address) + // the other family's ambient delivery would pass unvalidated while its + // PREROUTING rules were installed. + podIPs, ok := envVarOf(init, "POD_IPS") + if !ok { + t.Fatal("POD_IPS missing — IPv6 ambient inbound is uncaptured on a dual-stack pod") + } + if podIPs.ValueFrom == nil || podIPs.ValueFrom.FieldRef == nil || + podIPs.ValueFrom.FieldRef.FieldPath != "status.podIPs" { + t.Errorf("POD_IPS must come from the downward API status.podIPs, got %+v", podIPs.ValueFrom) + } + // SIDECAR_PORTS_EXCLUDE must carry the RESOLVED forward-proxy port. If the // script's 8081 default were used and findFreePort had moved it, the inbound // REDIRECT would swallow the forward proxy's own port. From eee000e798b8d4d054e8443bf2ad846a51c64bbb Mon Sep 17 00:00:00 2001 From: Hai Huang Date: Wed, 19 Aug 2026 10:02:18 -0400 Subject: [PATCH 3/4] fix(operator): Guard agent ports against the sidecar's own listeners MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found that transparent mode could produce a crash-looping pod the user cannot diagnose. usedPorts recorded the inbound port as reserved but never checked whether the agent had already DECLARED it. Under port stealing such a collision was accidentally survivable — Ports[0] got relocated off the conflicting port — and transparent mode deliberately removes that, so an agent declaring 8083 left two processes binding one port in a shared netns: admission succeeds, then the sidecar dies on "address already in use" in a container the user never wrote. Same failure class Validate() already guards for the two sidecar listeners, but applied to the agent, which is the case this mode newly creates. Two related holes closed at once: - The transparent EGRESS port (8082) was never reserved either — on main or here — so an agent declaring 8081 could push findFreePort's forward proxy onto a listener that is always on in proxy-sidecar mode. All sidecar-owned ports (8082, 8083, 9091, 9093, 9094) are now reserved unconditionally, so port assignment is also stable if a namespace later flips mechanism. - Reservation is separated from collision: containerPort is informational, so under reverse-proxy nothing binds 8083 and an agent declaring it is harmless. Only ports the sidecar actually binds in the resolved mechanism trigger the guard. Falls back to port stealing rather than rejecting admission. Rejecting would block pods that declare a colliding port informationally without ever binding it — which works today — and for a first-port collision the relocation genuinely resolves it. The warning names the port, the listener that owns it, and the effective mechanism, since the earlier "resolved inbound interception" line is no longer true at that point. Also from review: - The inboundInterception resolver moved to AFTER mode resolution, so an envoy-sidecar or waypoint namespace carrying the field no longer logs a resolved mechanism that is then silently ignored; it logs that the value is not applicable instead. (The reviewer's stated consequence — that authbridge would refuse — does not hold: the operator writes listener.inbound_interception only in the proxy-sidecar branch, so those modes never see it. The misleading log was the real issue.) - Dropped BuildProxySidecarContainerWithPorts' unused agentBackendPort parameter instead of annotating it with `_ =`, which did nothing the compiler needed. A pre-existing test expected the forward proxy on 8082 when 8081 was taken. That expectation encoded the bug above — 8082 is the transparent egress listener — so it is now 8084, with a comment on why, plus an assertion that the port is none of the sidecar's. New tests cover each sidecar port colliding (all five), a non-colliding port staying transparent, and the forward proxy never landing on a sidecar port in either mechanism. The gap the reviewer noted is real: all ten existing tests used ContainerPort 8000. Rebased onto a12afd0 (was CONFLICTING). make test: 21 packages green. Refs: rossoctl/cortex#330 Assisted-By: Claude (Anthropic AI) Signed-off-by: Hai Huang --- .../webhook/injector/container_builder.go | 12 +- .../internal/webhook/injector/pod_mutator.go | 193 ++++++++++++------ .../webhook/injector/pod_mutator_test.go | 18 +- .../injector/transparent_inbound_test.go | 116 +++++++++++ 4 files changed, 268 insertions(+), 71 deletions(-) diff --git a/operator/internal/webhook/injector/container_builder.go b/operator/internal/webhook/injector/container_builder.go index 32ee753c..62d17b0a 100644 --- a/operator/internal/webhook/injector/container_builder.go +++ b/operator/internal/webhook/injector/container_builder.go @@ -221,7 +221,7 @@ func spireSocketDir(socketPath string) string { // The app uses HTTP_PROXY env vars to route outbound traffic through the forward proxy. // Inbound traffic goes through the reverse proxy. func (b *ContainerBuilder) BuildProxySidecarContainer(spireEnabled bool) corev1.Container { - return b.BuildProxySidecarContainerWithPorts(spireEnabled, b.cfg.Images.AuthBridge, 8080, 8000, 8081) + return b.BuildProxySidecarContainerWithPorts(spireEnabled, b.cfg.Images.AuthBridge, 8080, 8081) } // BuildProxySidecarContainerWithPorts creates a proxy-sidecar container with dynamic ports. @@ -231,13 +231,11 @@ func (b *ContainerBuilder) BuildProxySidecarContainer(spireEnabled bool) corev1. // on the same ports; only the plugin set compiled into the binary differs. // // reverseProxyPort: where the reverse proxy listens (takes over the agent's original port) -// agentBackendPort: where the agent actually listens (moved to a free port) // forwardProxyPort: where the forward proxy listens (HTTP_PROXY target) -func (b *ContainerBuilder) BuildProxySidecarContainerWithPorts(spireEnabled bool, image string, reverseProxyPort, agentBackendPort, forwardProxyPort int32) corev1.Container { - // agentBackendPort is not rendered into the container: it is where the agent - // was relocated to, which the sidecar learns from its ConfigMap - // (reverse_proxy_backend), not from a declared port. - _ = agentBackendPort +func (b *ContainerBuilder) BuildProxySidecarContainerWithPorts(spireEnabled bool, image string, reverseProxyPort, forwardProxyPort int32) corev1.Container { + // No agent-backend port parameter: where the agent was relocated to is + // something the sidecar learns from its ConfigMap (reverse_proxy_backend), not + // from a declared container port. return b.buildProxySidecarContainer(spireEnabled, image, "reverse-proxy", reverseProxyPort, forwardProxyPort) } diff --git a/operator/internal/webhook/injector/pod_mutator.go b/operator/internal/webhook/injector/pod_mutator.go index 7c0479ec..c193c26b 100644 --- a/operator/internal/webhook/injector/pod_mutator.go +++ b/operator/internal/webhook/injector/pod_mutator.go @@ -354,61 +354,6 @@ func (m *PodMutator) InjectAuthBridge(ctx context.Context, podSpec *corev1.PodSp "namespace", namespace, "crName", crName, "mode", egressEnforcement, "source", egressEnforcementSource) - // ======================================== - // Resolve inboundInterception (namespace > reverse-proxy) - // ======================================== - // - // Selects the inbound deployment shape in proxy-sidecar / lite modes: - // "reverse-proxy" (default) steals the agent's port; "transparent" installs a - // PREROUTING REDIRECT and lets AuthBridge recover the real destination. - // envoy-sidecar ignores this — Envoy already intercepts inbound. - inboundInterception := "" - inboundInterceptionSource := "" - if ii := ExtractInboundInterception(nsConfig.AuthBridgeRuntimeYAML); ii != "" { - inboundInterception = ii - inboundInterceptionSource = sourceNamespaceConfigMap - } - if inboundInterception == "" { - inboundInterception = InboundInterceptionReverseProxy - inboundInterceptionSource = sourceClusterDefault - } - switch inboundInterception { - case InboundInterceptionReverseProxy, InboundInterceptionTransparent: - // recognized, keep as-is - default: - // Fall back to the shape that needs no privileges: an unrecognized value - // must not silently grant a NET_ADMIN init container. - mutatorLog.Info("WARN: unrecognized inboundInterception; defaulting to reverse-proxy", - "namespace", namespace, "crName", crName, - "unrecognized", inboundInterception, "source", inboundInterceptionSource) - inboundInterception = InboundInterceptionReverseProxy - inboundInterceptionSource = sourceInvalidFallback - } - allowedInbound := currentConfig.Proxy.AllowedInboundInterception - if len(allowedInbound) > 0 && !slices.Contains(allowedInbound, inboundInterception) { - mutatorLog.Info("WARN: inboundInterception value not in platform allowedInboundInterception; overriding", - "namespace", namespace, "crName", crName, - "requested", inboundInterception, "allowed", allowedInbound, - "overrideTo", allowedInbound[0]) - inboundInterception = allowedInbound[0] - inboundInterceptionSource = "platform-policy-override" - } - // Transparent inbound and the egress guard share one proxy-init container. - // With egressEnforcement "none" there is no init container to carry the - // PREROUTING rules, so nothing would REDIRECT to the inbound listener and it - // would bind a port that never receives traffic — inbound silently unenforced. - // Fall back to port stealing, which at least validates Service-routed traffic. - if inboundInterception == InboundInterceptionTransparent && egressEnforcement != EgressEnforcementEnforceRedirect { - mutatorLog.Info("WARN: inboundInterception=transparent requires egressEnforcement=enforce-redirect (shared proxy-init); falling back to reverse-proxy", - "namespace", namespace, "crName", crName, - "egressEnforcement", egressEnforcement) - inboundInterception = InboundInterceptionReverseProxy - inboundInterceptionSource = "requires-enforce-redirect-fallback" - } - mutatorLog.Info("resolved inbound interception", - "namespace", namespace, "crName", crName, - "mechanism", inboundInterception, "source", inboundInterceptionSource) - // ======================================== // Resolve TLS bridge posture (CR > namespace > "disabled") // ======================================== @@ -511,6 +456,71 @@ func (m *PodMutator) InjectAuthBridge(ctx context.Context, podSpec *corev1.PodSp "namespace", namespace, "crName", crName, "mode", authBridgeMode, "source", modeSource) + // ======================================== + // Resolve inboundInterception (namespace > reverse-proxy) + // ======================================== + // + // Selects the inbound deployment shape in proxy-sidecar / lite modes: + // "reverse-proxy" (default) steals the agent's port; "transparent" installs a + // PREROUTING REDIRECT and lets AuthBridge recover the real destination. + // envoy-sidecar ignores this — Envoy already intercepts inbound. + inboundInterception := "" + inboundInterceptionSource := "" + if ii := ExtractInboundInterception(nsConfig.AuthBridgeRuntimeYAML); ii != "" { + inboundInterception = ii + inboundInterceptionSource = sourceNamespaceConfigMap + } + if inboundInterception == "" { + inboundInterception = InboundInterceptionReverseProxy + inboundInterceptionSource = sourceClusterDefault + } + switch inboundInterception { + case InboundInterceptionReverseProxy, InboundInterceptionTransparent: + // recognized, keep as-is + default: + // Fall back to the shape that needs no privileges: an unrecognized value + // must not silently grant a NET_ADMIN init container. + mutatorLog.Info("WARN: unrecognized inboundInterception; defaulting to reverse-proxy", + "namespace", namespace, "crName", crName, + "unrecognized", inboundInterception, "source", inboundInterceptionSource) + inboundInterception = InboundInterceptionReverseProxy + inboundInterceptionSource = sourceInvalidFallback + } + allowedInbound := currentConfig.Proxy.AllowedInboundInterception + if len(allowedInbound) > 0 && !slices.Contains(allowedInbound, inboundInterception) { + mutatorLog.Info("WARN: inboundInterception value not in platform allowedInboundInterception; overriding", + "namespace", namespace, "crName", crName, + "requested", inboundInterception, "allowed", allowedInbound, + "overrideTo", allowedInbound[0]) + inboundInterception = allowedInbound[0] + inboundInterceptionSource = "platform-policy-override" + } + // Transparent inbound and the egress guard share one proxy-init container. + // With egressEnforcement "none" there is no init container to carry the + // PREROUTING rules, so nothing would REDIRECT to the inbound listener and it + // would bind a port that never receives traffic — inbound silently unenforced. + // Fall back to port stealing, which at least validates Service-routed traffic. + if inboundInterception == InboundInterceptionTransparent && egressEnforcement != EgressEnforcementEnforceRedirect { + mutatorLog.Info("WARN: inboundInterception=transparent requires egressEnforcement=enforce-redirect (shared proxy-init); falling back to reverse-proxy", + "namespace", namespace, "crName", crName, + "egressEnforcement", egressEnforcement) + inboundInterception = InboundInterceptionReverseProxy + inboundInterceptionSource = "requires-enforce-redirect-fallback" + } + // Only proxy-sidecar / lite consult this. Saying so keeps the log honest: the + // resolver runs before the mode branch, so an envoy-sidecar or waypoint + // namespace carrying inboundInterception would otherwise read as though the + // value took effect. + if authBridgeMode == ModeProxySidecar || authBridgeMode == ModeLite { + mutatorLog.Info("resolved inbound interception", + "namespace", namespace, "crName", crName, + "mechanism", inboundInterception, "source", inboundInterceptionSource) + } else if inboundInterceptionSource == sourceNamespaceConfigMap { + mutatorLog.Info("ignoring inboundInterception: not applicable to this mode", + "namespace", namespace, "crName", crName, + "mode", authBridgeMode, "requested", inboundInterception) + } + if authBridgeMode == ModeWaypoint { mutatorLog.Info("waypoint mode — skipping sidecar injection (waypoint is a standalone deployment)", "namespace", namespace, "crName", crName) @@ -597,12 +607,76 @@ func (m *PodMutator) InjectAuthBridge(ctx context.Context, podSpec *corev1.PodSp usedPorts[originalAgentPort] = true transparentInbound := inboundInterception == InboundInterceptionTransparent - - // The inbound transparent listener needs its own port reserved so - // findFreePort can't hand it to the forward proxy. transparentInboundPort := builder.cfg.Proxy.TransparentInboundPort + + // Ports the sidecar owns. Reserved unconditionally so findFreePort can never + // hand one to the forward proxy: the transparent EGRESS port in particular + // was previously unreserved, so an agent declaring 8081 could push the + // forward proxy onto 8082 and collide with a listener that is always on in + // this mode. The inbound port is reserved even under reverse-proxy, so port + // assignment stays stable if a namespace later flips mechanism. + sidecarPorts := map[int32]string{ + builder.cfg.Proxy.TransparentPort: "transparent egress listener", + transparentInboundPort: "transparent inbound listener", + authBridgeHealthPort: "health endpoint", + authBridgeStatsPort: "stats endpoint", + authBridgeSessionAPIPort: "session-events API", + } + for p := range sidecarPorts { + usedPorts[p] = true + } + + // Which of those are actually BOUND depends on the mechanism: under + // reverse-proxy nothing listens on the inbound port, so an agent declaring + // it is harmless and must not trigger the fallback below. + boundBySidecar := make(map[int32]string, len(sidecarPorts)) + for port, owner := range sidecarPorts { + if port == transparentInboundPort && !transparentInbound { + continue + } + boundBySidecar[port] = owner + } + + // Reserving is not enough for the agent's OWN ports: containerPort is + // informational, so the operator cannot move a port the agent actually + // binds. Under port stealing a collision was accidentally survivable — + // Ports[0] got relocated off the conflicting port. Transparent mode + // deliberately removes that, so an agent declaring the sidecar's inbound + // port would leave two processes binding one port in a shared netns: + // admission succeeds, then the sidecar dies on "address already in use" in + // a container the user never wrote. + // + // Fall back to port stealing rather than reject. Rejecting would block + // pods that declare a colliding port informationally without ever binding + // it — which works today — and for a first-port collision the relocation + // genuinely resolves it. The warning is the actionable part. if transparentInbound { - usedPorts[transparentInboundPort] = true + for i := range podSpec.Containers { + c := &podSpec.Containers[i] + if c.Name == AuthBridgeProxyContainerName { + continue + } + for _, cp := range c.Ports { + if owner, clash := boundBySidecar[cp.ContainerPort]; clash { + // States the EFFECTIVE mechanism: the "resolved inbound + // interception" line above already logged transparent, and + // this is the point where that stops being true. + mutatorLog.Info("WARN: container declares a port the authbridge sidecar binds; overriding inbound interception to reverse-proxy to avoid a bind collision", + "namespace", namespace, "crName", crName, + "container", c.Name, "port", cp.ContainerPort, + "conflictsWith", owner, + "effectiveMechanism", InboundInterceptionReverseProxy, + "source", "sidecar-port-conflict-fallback") + transparentInbound = false + inboundInterception = InboundInterceptionReverseProxy + break + } + } + if !transparentInbound { + break + } + } + } // newAgentPort exists only for the port-stealing shape. Transparent @@ -687,7 +761,6 @@ func (m *PodMutator) InjectAuthBridge(ctx context.Context, podSpec *corev1.PodSp spireEnabled, proxyImage, originalAgentPort, // reverse proxy listens here - newAgentPort, // forwards to agent here forwardProxyPort, // forward proxy listens here ) } diff --git a/operator/internal/webhook/injector/pod_mutator_test.go b/operator/internal/webhook/injector/pod_mutator_test.go index 38559de2..eee6bd31 100644 --- a/operator/internal/webhook/injector/pod_mutator_test.go +++ b/operator/internal/webhook/injector/pod_mutator_test.go @@ -1106,8 +1106,18 @@ func TestInjectAuthBridge_ProxySidecarMode_ForwardProxyCollision(t *testing.T) { if p.ContainerPort == 8081 { t.Error("forward-proxy should not be 8081 (collision with agent metrics)") } - if p.ContainerPort != 8082 { - t.Errorf("forward-proxy port = %d, want 8082", p.ContainerPort) + // 8084, not 8082: the sidecar's own ports are now reserved, so + // findFreePort skips the transparent egress listener (8082) and + // the transparent inbound listener (8083). This expectation used + // to be 8082, which would have put the forward proxy on top of a + // listener that is always on in proxy-sidecar mode. + if p.ContainerPort != 8084 { + t.Errorf("forward-proxy port = %d, want 8084", p.ContainerPort) + } + for _, owned := range []int32{8082, 8083, 9091, 9093, 9094} { + if p.ContainerPort == owned { + t.Errorf("forward-proxy assigned %d, a port the sidecar binds", owned) + } } } } @@ -1122,8 +1132,8 @@ func TestInjectAuthBridge_ProxySidecarMode_ForwardProxyCollision(t *testing.T) { if env.Value == "http://127.0.0.1:8081" { t.Error("HTTP_PROXY should not use 8081 (collides with agent metrics)") } - if env.Value != "http://127.0.0.1:8082" { - t.Errorf("HTTP_PROXY = %q, want http://127.0.0.1:8082", env.Value) + if env.Value != "http://127.0.0.1:8084" { + t.Errorf("HTTP_PROXY = %q, want http://127.0.0.1:8084", env.Value) } } } diff --git a/operator/internal/webhook/injector/transparent_inbound_test.go b/operator/internal/webhook/injector/transparent_inbound_test.go index 8d92418a..0d2d2ece 100644 --- a/operator/internal/webhook/injector/transparent_inbound_test.go +++ b/operator/internal/webhook/injector/transparent_inbound_test.go @@ -385,3 +385,119 @@ func TestExtractInboundInterception(t *testing.T) { }) } } + +// agentPodSpecOnPort is agentPodSpec with a caller-chosen container port, for the +// collision cases. The existing tests all use 8000, which is why the sidecar-port +// conflict went uncovered. +func agentPodSpecOnPort(port int32) *corev1.PodSpec { + return &corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "agent", + Image: "my-agent:latest", + Ports: []corev1.ContainerPort{{ContainerPort: port}}, + }}, + } +} + +// TestTransparentInbound_AgentPortCollidesWithSidecar covers the failure mode +// transparent interception newly creates. Under port stealing a collision was +// accidentally survivable — Ports[0] got relocated off the conflicting port. +// Transparent mode deliberately does not relocate, so an agent declaring a port +// the sidecar binds would leave two processes on one port in a shared netns: +// admission succeeds, then the sidecar dies on "address already in use" in a +// container the user never wrote. +// +// containerPort is informational, so the operator cannot move a port the agent +// actually binds. Falling back to port stealing is the survivable answer. +func TestTransparentInbound_AgentPortCollidesWithSidecar(t *testing.T) { + // Each of these is a port the sidecar itself binds. + for _, tc := range []struct { + name string + port int32 + }{ + {"transparent inbound listener", 8083}, + {"transparent egress listener", 8082}, + {"health endpoint", 9091}, + {"stats endpoint", 9093}, + {"session-events API", 9094}, + } { + t.Run(tc.name, func(t *testing.T) { + m := newTestMutator(inboundCM(InboundInterceptionTransparent, "")) + podSpec := agentPodSpecOnPort(tc.port) + + if _, err := m.InjectAuthBridge(context.Background(), podSpec, "test-ns", "my-agent", + "Deployment", map[string]string{RossoctlTypeLabel: RossoctlTypeAgent}, nil); err != nil { + t.Fatalf("InjectAuthBridge() error: %v", err) + } + + proxy := containerByName(podSpec, AuthBridgeProxyContainerName) + if proxy == nil { + t.Fatal("authbridge-proxy not injected") + } + // Fell back to port stealing: the agent is relocated, so nothing + // double-binds. + agent := containerByName(podSpec, "agent") + if got := agent.Ports[0].ContainerPort; got == tc.port { + t.Errorf("agent still on %d; expected relocation after falling back to reverse-proxy", tc.port) + } + // And the sidecar must not be running the transparent shape. + for _, p := range proxy.Ports { + if p.Name == "transparent-in" { + t.Errorf("sidecar still declares transparent-in after a collision on %d", tc.port) + } + } + init := containerByName(podSpec, ProxyInitContainerName) + if init != nil { + if _, ok := envVarOf(init, "INBOUND_TRANSPARENT_PORT"); ok { + t.Errorf("inbound capture still armed after a collision on %d", tc.port) + } + } + }) + } +} + +// TestTransparentInbound_NonCollidingPortStillTransparent guards the guard: it +// must not fire for an ordinary agent port. +func TestTransparentInbound_NonCollidingPortStillTransparent(t *testing.T) { + m := newTestMutator(inboundCM(InboundInterceptionTransparent, "")) + podSpec := agentPodSpecOnPort(8000) + + if _, err := m.InjectAuthBridge(context.Background(), podSpec, "test-ns", "my-agent", + "Deployment", map[string]string{RossoctlTypeLabel: RossoctlTypeAgent}, nil); err != nil { + t.Fatalf("InjectAuthBridge() error: %v", err) + } + agent := containerByName(podSpec, "agent") + if got := agent.Ports[0].ContainerPort; got != 8000 { + t.Errorf("agent relocated to %d; a non-colliding port must stay transparent", got) + } +} + +// TestForwardProxyNeverLandsOnSidecarPort covers the pre-existing hole the +// reservation also closes: the transparent EGRESS port was never reserved, so an +// agent declaring 8081 could push findFreePort's forward proxy onto 8082 and +// collide with a listener that is always on in proxy-sidecar mode. +func TestForwardProxyNeverLandsOnSidecarPort(t *testing.T) { + for _, mechanism := range []string{InboundInterceptionReverseProxy, InboundInterceptionTransparent} { + t.Run(mechanism, func(t *testing.T) { + m := newTestMutator(inboundCM(mechanism, "")) + // 8081 declared by the agent forces findFreePort to look further. + podSpec := agentPodSpecOnPort(8081) + + if _, err := m.InjectAuthBridge(context.Background(), podSpec, "test-ns", "my-agent", + "Deployment", map[string]string{RossoctlTypeLabel: RossoctlTypeAgent}, nil); err != nil { + t.Fatalf("InjectAuthBridge() error: %v", err) + } + proxy := containerByName(podSpec, AuthBridgeProxyContainerName) + for _, p := range proxy.Ports { + if p.Name != "forward-proxy" { + continue + } + for _, owned := range []int32{8082, 8083, 9091, 9093, 9094} { + if p.ContainerPort == owned { + t.Errorf("forward proxy assigned %d, which the sidecar already binds", owned) + } + } + } + }) + } +} From 1cabcaa15aec5a0adb8684d3ef93cbdf5133249b Mon Sep 17 00:00:00 2001 From: Hai Huang Date: Wed, 19 Aug 2026 10:58:06 -0400 Subject: [PATCH 4/4] fix(operator): Don't bind a transparent listener nothing redirects to MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review (cwiklik) flagged a re-injection divergence. The proxy-init injection is skipped when a container of that name already exists, but the sidecar container and the per-agent ConfigMap are configured unconditionally. So a pod arriving with a hand-authored proxy-init that lacks inbound capture would get authbridge binding :8083 and a ConfigMap saying transparent, while nothing installed the PREROUTING rules — inbound silently unenforced, with no error anywhere. That is the one outcome this mode exists to prevent, so it is now checked rather than left to reasoning about reachability. The check sits in the pre-flight block alongside the port-collision guard, before any artifact is built, so the ConfigMap, sidecar and proxy-init env cannot disagree: an existing proxy-init without INBOUND_TRANSPARENT_PORT falls back to reverse-proxy with a warning naming the effective mechanism and how to fix it. An existing proxy-init that IS armed is consistent and stays transparent. Narrow in practice — the webhook is CREATE-only on pods and a fresh pod comes from an unmutated Deployment template, so this needs a hand-written container — but the failure being silent is what makes it worth a guard rather than a note. Also documents why SIDECAR_PORTS_EXCLUDE omits 8082/8083, which the same review had to guess at. It carries only the ports the init script cannot know: the forward-proxy port (findFreePort may have moved it off 8081) and the health/stats/session constants. The two transparent ports are exempted unconditionally inside emit_inbound_exemptions, on BOTH the PREROUTING and ambient hooks, since the script already has them as TRANSPARENT_PORT / INBOUND_TRANSPARENT_PORT — listing them again would only duplicate RETURN rules. The asymmetry with 8081 is therefore intentional, and cortex#776's harness asserts the 8082 exemption directly. make test: 21 packages green. Refs: rossoctl/cortex#330 Assisted-By: Claude (Anthropic AI) Signed-off-by: Hai Huang --- .../api/v1alpha1/zz_generated.deepcopy.go | 65 +++++++++++++++++- .../internal/webhook/injector/pod_mutator.go | 44 ++++++++++++ .../injector/transparent_inbound_test.go | 67 +++++++++++++++++++ 3 files changed, 175 insertions(+), 1 deletion(-) diff --git a/operator/api/v1alpha1/zz_generated.deepcopy.go b/operator/api/v1alpha1/zz_generated.deepcopy.go index 9d1b5559..7c3a495e 100644 --- a/operator/api/v1alpha1/zz_generated.deepcopy.go +++ b/operator/api/v1alpha1/zz_generated.deepcopy.go @@ -324,7 +324,7 @@ func (in *AgentRuntime) DeepCopyInto(out *AgentRuntime) { *out = *in out.TypeMeta = in.TypeMeta in.ObjectMeta.DeepCopyInto(&out.ObjectMeta) - out.Spec = in.Spec + in.Spec.DeepCopyInto(&out.Spec) in.Status.DeepCopyInto(&out.Status) } @@ -382,6 +382,11 @@ func (in *AgentRuntimeList) DeepCopyObject() runtime.Object { func (in *AgentRuntimeSpec) DeepCopyInto(out *AgentRuntimeSpec) { *out = *in out.TargetRef = in.TargetRef + if in.Auth != nil { + in, out := &in.Auth, &out.Auth + *out = new(AuthConfig) + (*in).DeepCopyInto(*out) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new AgentRuntimeSpec. @@ -468,6 +473,28 @@ func (in *AgentSkill) DeepCopy() *AgentSkill { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *AuthConfig) DeepCopyInto(out *AuthConfig) { + *out = *in + if in.Outbound != nil { + in, out := &in.Outbound, &out.Outbound + *out = make([]OutboundRoute, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new AuthConfig. +func (in *AuthConfig) DeepCopy() *AuthConfig { + if in == nil { + return nil + } + out := new(AuthConfig) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *AuthorizationPolicy) DeepCopyInto(out *AuthorizationPolicy) { *out = *in @@ -632,6 +659,27 @@ func (in *IdentityBinding) DeepCopy() *IdentityBinding { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *OutboundRoute) DeepCopyInto(out *OutboundRoute) { + *out = *in + out.Destination = in.Destination + if in.Audiences != nil { + in, out := &in.Audiences, &out.Audiences + *out = make([]string, len(*in)) + copy(*out, *in) + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new OutboundRoute. +func (in *OutboundRoute) DeepCopy() *OutboundRoute { + if in == nil { + return nil + } + out := new(OutboundRoute) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *PolicyEntry) DeepCopyInto(out *PolicyEntry) { *out = *in @@ -647,6 +695,21 @@ func (in *PolicyEntry) DeepCopy() *PolicyEntry { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *RouteMatch) DeepCopyInto(out *RouteMatch) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new RouteMatch. +func (in *RouteMatch) DeepCopy() *RouteMatch { + if in == nil { + return nil + } + out := new(RouteMatch) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *SignatureHeader) DeepCopyInto(out *SignatureHeader) { *out = *in diff --git a/operator/internal/webhook/injector/pod_mutator.go b/operator/internal/webhook/injector/pod_mutator.go index c193c26b..7a41df5b 100644 --- a/operator/internal/webhook/injector/pod_mutator.go +++ b/operator/internal/webhook/injector/pod_mutator.go @@ -626,6 +626,41 @@ func (m *PodMutator) InjectAuthBridge(ctx context.Context, podSpec *corev1.PodSp usedPorts[p] = true } + // A proxy-init container already in the INCOMING spec is not one we built, + // so we cannot assume it carries the inbound env — and the injection below + // is skipped when the container exists. Left alone that yields the one + // outcome this mode exists to prevent: the sidecar binds the inbound + // listener and the ConfigMap says transparent, but nothing REDIRECTs to it, + // so inbound is silently unenforced with no error anywhere. + // + // Reachable only via a hand-authored proxy-init (the webhook is CREATE-only + // on pods, and a fresh pod comes from an unmutated template), but the + // failure is silent, so check rather than rely on that. + if transparentInbound { + for i := range podSpec.InitContainers { + if podSpec.InitContainers[i].Name != ProxyInitContainerName { + continue + } + armed := false + for _, e := range podSpec.InitContainers[i].Env { + if e.Name == "INBOUND_TRANSPARENT_PORT" && e.Value != "" { + armed = true + break + } + } + if !armed { + mutatorLog.Info("WARN: pod already carries a proxy-init container without inbound capture; overriding inbound interception to reverse-proxy rather than bind a listener nothing redirects to", + "namespace", namespace, "crName", crName, + "effectiveMechanism", InboundInterceptionReverseProxy, + "source", "preexisting-proxy-init-fallback", + "hint", "remove the hand-added proxy-init container to use transparent interception") + transparentInbound = false + inboundInterception = InboundInterceptionReverseProxy + } + break + } + } + // Which of those are actually BOUND depends on the mechanism: under // reverse-proxy nothing listens on the inbound port, so an agent declaring // it is harmless and must not trigger the fallback below. @@ -806,6 +841,15 @@ func (m *PodMutator) InjectAuthBridge(ctx context.Context, podSpec *corev1.PodSp if transparentInbound { inbound = ProxyInitInbound{ Port: transparentInboundPort, + // Only the ports the SCRIPT cannot know. The forward-proxy port + // is here because findFreePort may have moved it off 8081; the + // health/stats/session ports because they are operator-side + // constants. The two transparent ports (8082 egress, 8083 + // inbound) are deliberately absent: init-iptables.sh exempts + // both unconditionally in emit_inbound_exemptions, on BOTH the + // PREROUTING and ambient hooks, since it already knows them from + // TRANSPARENT_PORT / INBOUND_TRANSPARENT_PORT. Listing them + // again would just duplicate RETURN rules. SidecarPortsExclude: fmt.Sprintf("%d,%d,%d,%d", forwardProxyPort, authBridgeHealthPort, authBridgeStatsPort, authBridgeSessionAPIPort), PortsExclude: annotations[InboundPortsExcludeAnnotation], diff --git a/operator/internal/webhook/injector/transparent_inbound_test.go b/operator/internal/webhook/injector/transparent_inbound_test.go index 0d2d2ece..0c873382 100644 --- a/operator/internal/webhook/injector/transparent_inbound_test.go +++ b/operator/internal/webhook/injector/transparent_inbound_test.go @@ -501,3 +501,70 @@ func TestForwardProxyNeverLandsOnSidecarPort(t *testing.T) { }) } } + +// TestTransparentInbound_PreexistingProxyInitFallsBack covers the divergence a +// reviewer flagged: the proxy-init injection is skipped when a container of that +// name already exists, so a hand-authored one without the inbound env would leave +// the sidecar binding :8083 and the ConfigMap saying transparent while nothing +// REDIRECTs to it — inbound silently unenforced, with no error anywhere. +func TestTransparentInbound_PreexistingProxyInitFallsBack(t *testing.T) { + m := newTestMutator(inboundCM(InboundInterceptionTransparent, "")) + podSpec := agentPodSpec() + // A proxy-init the operator did not build: right name, no inbound capture. + podSpec.InitContainers = append(podSpec.InitContainers, corev1.Container{ + Name: ProxyInitContainerName, + Image: "someone-elses/proxy-init:hand-written", + Env: []corev1.EnvVar{{Name: "MODE", Value: "enforce-redirect"}}, + }) + + if _, err := m.InjectAuthBridge(context.Background(), podSpec, "test-ns", "my-agent", + "Deployment", map[string]string{RossoctlTypeLabel: RossoctlTypeAgent}, nil); err != nil { + t.Fatalf("InjectAuthBridge() error: %v", err) + } + + // Must NOT advertise a transparent listener that nothing feeds. + proxy := containerByName(podSpec, AuthBridgeProxyContainerName) + if proxy == nil { + t.Fatal("authbridge-proxy not injected") + } + for _, p := range proxy.Ports { + if p.Name == "transparent-in" { + t.Error("sidecar declares transparent-in despite no inbound capture being installed") + } + } + cm := fetchConfigMap(t, m, "test-ns", perAgentConfigMapName("my-agent")) + listener := parseConfigYAML(t, cm)["listener"].(map[string]interface{}) + if _, present := listener["transparent_inbound_addr"]; present { + t.Error("ConfigMap still configures a transparent inbound listener nothing redirects to") + } + if listener["reverse_proxy_backend"] == nil { + t.Error("expected the reverse-proxy fallback to be fully configured") + } +} + +// TestTransparentInbound_PreexistingArmedProxyInitStaysTransparent guards the +// guard: an existing proxy-init that DOES carry inbound capture is consistent, so +// the fallback must not fire. +func TestTransparentInbound_PreexistingArmedProxyInitStaysTransparent(t *testing.T) { + m := newTestMutator(inboundCM(InboundInterceptionTransparent, "")) + podSpec := agentPodSpec() + podSpec.InitContainers = append(podSpec.InitContainers, corev1.Container{ + Name: ProxyInitContainerName, + Image: "ghcr.io/rossoctl/cortex/proxy-init:latest", + Env: []corev1.EnvVar{ + {Name: "MODE", Value: "enforce-redirect"}, + {Name: "INBOUND_TRANSPARENT_PORT", Value: "8083"}, + }, + }) + + if _, err := m.InjectAuthBridge(context.Background(), podSpec, "test-ns", "my-agent", + "Deployment", map[string]string{RossoctlTypeLabel: RossoctlTypeAgent}, nil); err != nil { + t.Fatalf("InjectAuthBridge() error: %v", err) + } + + cm := fetchConfigMap(t, m, "test-ns", perAgentConfigMapName("my-agent")) + listener := parseConfigYAML(t, cm)["listener"].(map[string]interface{}) + if listener["transparent_inbound_addr"] != ":8083" { + t.Errorf("transparent_inbound_addr = %v; an already-armed proxy-init is consistent and must stay transparent", listener["transparent_inbound_addr"]) + } +}