Skip to content

feat: Wire proxy-sidecar egress enforcement (UID fix + enforce-drop)#406

Merged
huang195 merged 3 commits into
mainfrom
feat/proxy-sidecar-egress-enforcement
Jun 5, 2026
Merged

feat: Wire proxy-sidecar egress enforcement (UID fix + enforce-drop)#406
huang195 merged 3 commits into
mainfrom
feat/proxy-sidecar-egress-enforcement

Conversation

@huang195

@huang195 huang195 commented Jun 5, 2026

Copy link
Copy Markdown
Member

Summary

Wires the proxy-sidecar fail-closed egress guard into the operator. proxy-sidecar mode routes egress through AuthBridge via HTTP_PROXY, which is cooperative — an app that ignores HTTP_PROXY (or sets NO_PROXY) egresses directly and bypasses AuthBridge. kagenti-extensions#484 (merged) added MODE=enforce-drop to the shared proxy-init image; this PR injects it (opt-in, default off) plus the prerequisite UID fix.

This is the operator half of the proxy-sidecar egress-enforcement work (extensions#484 was the image half).

What changed

  • UID fix (unconditional): the proxy-sidecar authbridge-proxy container hardcoded RunAsUser: 1001 — the same UID as the app — so the enforce-drop --uid-owner exemption couldn't distinguish proxy from app egress. Now derived from cfg.Proxy.UID (1337), the single source of truth also used for the proxy-init PROXY_UID, matching the envoy builder. Adds RunAsGroup parity and validates Proxy.UID >= 1.
  • Config (PlatformConfig.Proxy): EgressEnforcement (off|enforce, default off) and ClusterCIDRs (default 10.0.0.0/8). Surfaced via the chart's defaults.proxy.
  • Injection: BuildProxyInitContainer gains a mode param — redirect (envoy-sidecar, unchanged) or enforce-drop (emits MODE + CLUSTER_CIDRS, omits the redirect-only POD_IP/exclude vars). The proxy-sidecar branch injects it when EgressEnforcement=="enforce".

Design notes

  • Default off → keeps main shippable. No behavior change until an operator sets enforce. The end goal is enforce-by-default: a later release flips the compiled default off → enforce (one line) once ClusterCIDRs sourcing is validated across distros. The knob is a permanent operational override — nothing to remove, no throwaway gate.
  • envoy-sidecar is unaffected. It already enforces egress structurally via the transparent redirect, so the knob is consulted only on the proxy-sidecar / lite path — no mode-compatibility matrix.
  • Security: the guard rests on UID separation (the agent must not run as, or setuid() to, Proxy.UID). Proxy.UID >= 1 is validated. Before flipping the default to enforce, confirm PSA restricted / SCC enforces non-root + no CAP_SETUID on agents (or add a runAsUser == Proxy.UID admission check).

Rollout notes / dependencies

  • ClusterCIDRs default is Kind-shaped (10.0.0.0/8). OCP/EKS must override (e.g. OCP services 172.30.0.0/16 — outside 10/8 — and pods 10.128.0.0/14) or in-cluster service traffic is dropped. Documented in values.yaml and the field doc.
  • enforce-drop intentionally ignores OUTBOUND_PORTS_EXCLUDE (redirect-only); anything previously bypassed that way must route through the proxy or fall in ClusterCIDRs.
  • Release coordination: the install chart must deploy a proxy-init image containing extensions#484. Image versions are owned by the install chart, not pinned here.

Testing

go build ./... && go test ./internal/webhook/... — all green. New tests: proxy container RunAsUser == Proxy.UID; proxy-init enforce-drop env (MODE/PROXY_UID/CLUSTER_CIDRS, no POD_IP); injection present on enforce / absent on default off / not duplicated. Helm defaults.proxy renders the new fields.

Assisted-By: Claude (Anthropic AI) noreply@anthropic.com

proxy-sidecar mode routes egress through AuthBridge via HTTP_PROXY, which is
cooperative and bypassable. kagenti-extensions#484 added MODE=enforce-drop to
the shared proxy-init image (a fail-closed mangle-OUTPUT guard); this wires it
into the operator, plus the prerequisite UID fix.

- Fix: the proxy-sidecar authbridge-proxy container hardcoded RunAsUser 1001
  (same as the app), so the enforce-drop `--uid-owner` exemption could not
  distinguish proxy from app egress. Derive it from cfg.Proxy.UID (1337) —
  the single source of truth also used for the proxy-init PROXY_UID — matching
  the envoy builder. Add RunAsGroup parity and validate Proxy.UID >= 1.

- Add cluster config (PlatformConfig.Proxy): EgressEnforcement ("off"|"enforce",
  default "off") and ClusterCIDRs (default 10.0.0.0/8, Kind-shaped). Surfaced
  via the operator chart's defaults.proxy. Off by default keeps main shippable;
  a future release flips the compiled default to "enforce" — the knob is a
  permanent override, nothing to remove.

- BuildProxyInitContainer gains a mode param: "redirect" (envoy-sidecar,
  unchanged) or "enforce-drop" (emits MODE + CLUSTER_CIDRS; omits the
  redirect-only POD_IP/exclude vars). The proxy-sidecar branch injects it when
  EgressEnforcement=="enforce"; envoy-sidecar is unaffected.

enforce-drop only applies to proxy-sidecar — envoy-sidecar already enforces
egress structurally via the transparent redirect, so the knob is consulted only
on the proxy-sidecar/lite path (no mode-compatibility matrix needed).

Tests: proxy container RunAsUser==Proxy.UID; proxy-init enforce-drop env;
injection present on enforce / absent on default-off / not duplicated.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
Address review on #406: Validate() checked the egressEnforcement enum but
not ClusterCIDRs. When enforce is set, an empty list silently fell back to
the Kind-shaped 10.0.0.0/8 default in init-iptables.sh, and a malformed
entry crashed the proxy-init container under set -e. Now, when
EgressEnforcement == enforce, require ClusterCIDRs non-empty and
net.ParseCIDR each entry — misconfig fails fast at config load with a clear
message instead of as a pod CrashLoop or a silent wrong default. Off mode
leaves ClusterCIDRs unvalidated.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>

@mrsabath mrsabath left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Default-off opt-in for proxy-sidecar egress enforcement. The UID fix (1001 → cfg.Proxy.UID) is the right cleanup — the prior hardcode had proxy and app sharing a UID, which would have defeated any UID-owner exemption.

Validation in types.go:122-176 is solid: switch covers empty/off/enforce, requires non-empty ClusterCIDRs when enforce, and net.ParseCIDRs each entry. Helm values.yaml documents the OCP/EKS override requirement clearly. Redirect path is byte-identical to pre-PR (verified), so envoy-sidecar mode is unaffected. Proxy-init's existing Privileged: false + NET_ADMIN, NET_RAW capabilities posture is unchanged.

Only meaningful gap is that the new public mode param has no validation — a typo at any future callsite silently turns the fail-closed guard into fail-open. Worth tightening before this rolls out further; not blocking since today's two callsites both use typed constants.

Areas reviewed: Go (webhook config, injector, container builder), tests, Helm values, CI
Commits: 2 (both signed-off, DCO ✅)
CI status: ✅ all required checks passing (E2E still running at review time)

Assisted-By: Claude Code

// the exclude args do not apply (the script ignores them in this mode).
func (b *ContainerBuilder) BuildProxyInitContainer(mode, outboundPortsExclude, inboundPortsExclude string) corev1.Container {
var env []corev1.EnvVar
if mode == ProxyInitModeEnforceDrop {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: if mode == ProxyInitModeEnforceDrop { ... } else { ... } means any value that isn't exactly "enforce-drop" falls through to redirect mode. Today both callsites use typed constants so risk is contained, but BuildProxyInitContainer is exported — a future caller passing "enfore-drop" (typo) silently disables enforcement, which is the worst possible failure mode for a fail-closed guard.

Two cheap options:

// (a) explicit default branch
switch mode {
case ProxyInitModeEnforceDrop:
    // ...
case ProxyInitModeRedirect:
    // ...
default:
    builderLog.Error(nil, "unknown proxy-init mode", "mode", mode)
    return corev1.Container{}
}

// (b) typed mode
type ProxyInitMode string
const (
    ProxyInitModeRedirect    ProxyInitMode = "redirect"
    ProxyInitModeEnforceDrop ProxyInitMode = "enforce-drop"
)

Option (b) lets the compiler catch the typo. A typo in a fail-closed guard turns it into fail-open — 千里之堤,溃于蚁穴 (a thousand-li dike collapses from an ant-hole).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adopted the typed-mode approach and kept an explicit fail-closed default. Worth noting: a named type alone doesn't fully protect — an untyped string literal still implicitly converts to ProxyInitMode at a callsite, so BuildProxyInitContainer("enfore-drop", …) would still compile. The default branch is what actually closes the gap: it logs an error and returns a zero-value container, so a typo'd mode breaks injection visibly instead of degrading to redirect. Fixed in d280093.


// enforce-drop mode emits MODE / PROXY_UID / CLUSTER_CIDRS and none of the
// redirect-only vars (POD_IP, OUTBOUND_PORTS_EXCLUDE).
func TestBuildProxyInitContainer_EnforceDrop(t *testing.T) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: the new test covers enforce-drop env vars, but there's no test that an invalid mode is rejected. Today BuildProxyInitContainer("enfore-drop", "", "") (typo) would silently render as redirect — exactly the failure this PR exists to prevent.

If you keep the string-with-fallthrough approach, add a negative test:

func TestBuildProxyInitContainer_InvalidMode(t *testing.T) {
    builder := newTestBuilder(t)
    c := builder.BuildProxyInitContainer("enfore-drop", "", "")
    // assert: returned zero-value, OR env doesn't contain MODE=enforce-drop
    //         AND doesn't contain redirect-only PROXY_PORT
}

Goes away if you adopt the typed-mode approach in the other comment.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added TestBuildProxyInitContainer_InvalidMode in d280093 — passes ProxyInitMode("enfore-drop") and asserts a zero-value container (empty name/image/env), exercising the new fail-closed default branch.

return fmt.Errorf("proxy.clusterCIDRs must be non-empty when proxy.egressEnforcement is %q (set the cluster's pod+service CIDRs)", EgressEnforcementEnforce)
}
for _, cidr := range c.Proxy.ClusterCIDRs {
if _, _, err := net.ParseCIDR(cidr); err != nil {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: net.ParseCIDR(cidr) per entry catches malformed syntax (good), but doesn't reject overlapping ranges or warn on IPv6 in IPv4-only deployments. Probably intentional — overlapping CIDRs are fine for iptables — but worth a one-line comment so future readers don't assume the validation is exhaustive:

// Syntactic validation only — overlapping ranges and IPv4/IPv6 mixing are
// accepted (iptables handles both); we only reject malformed strings here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the clarifying comment in d280093 — validation is syntactic only; overlapping ranges and v4/v6 mixing are accepted (iptables and the init script split v4/v6 themselves).

Address review feedback on the exported BuildProxyInitContainer API: a
mistyped mode literal at a future callsite would silently fall through to
redirect, shipping a proxy-init with no egress guard — fail-open, the worst
outcome for a fail-closed control.

- Make ProxyInitMode a named type and the mode dispatch an explicit switch
  with a fail-closed default that logs an error and returns a zero-value
  container (injection breaks visibly instead of degrading to redirect).
- Add TestBuildProxyInitContainer_InvalidMode covering the default branch.
- Document that clusterCIDRs validation is syntactic only (overlap / v4-v6
  mixing accepted; iptables and the init script handle both).

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
@huang195
huang195 merged commit 543ba07 into main Jun 5, 2026
15 checks passed
@github-project-automation github-project-automation Bot moved this from New /:ToDo to Done in Rossoctl Issue Prioritization Jun 5, 2026
@huang195
huang195 deleted the feat/proxy-sidecar-egress-enforcement branch June 5, 2026 21:02
odh-devops-app Bot pushed a commit to opendatahub-io/agents-operator that referenced this pull request Jun 6, 2026
…ct), remove flag

PR rossoctl#406 gated proxy-sidecar egress enforcement behind proxy.egressEnforcement
(default off) because the only enforcement available was fail-closed DROP, which
breaks agents that ignore HTTP_PROXY. kagenti-extensions#485 adds the missing
half — a transparent listener + MODE=enforce-redirect that CAPTURES bypass
egress (REDIRECT to the transparent listener) instead of dropping it — so
enforcement no longer breaks anything and can be always-on.

This wires the operator to that:
- constants: add ProxyInitModeEnforceRedirect; keep ProxyInitModeEnforceDrop as
  a documented no-transparent-listener fallback the operator no longer emits.
- container_builder: enforce-redirect branch emits MODE / PROXY_UID /
  CLUSTER_CIDRS / TRANSPARENT_PORT (the new ProxyConfig.TransparentPort, default
  8082, matching the authbridge listener.transparent_proxy_addr preset).
- pod_mutator: the proxy-sidecar / lite branch now ALWAYS injects the
  enforce-redirect proxy-init (no flag check); envoy-sidecar still uses redirect.
- config: REMOVE ProxyConfig.EgressEnforcement (+ its constants, validation, and
  the Helm value). ClusterCIDRs is now validated unconditionally (always consumed
  by the always-on guard), and TransparentPort gets a port-range check.
- Helm values: replace egressEnforcement with transparentPort; update comments.

Tests updated: builder emits enforce-redirect env incl. TRANSPARENT_PORT;
proxy-sidecar always injects proxy-init (enforce-redirect); ClusterCIDRs always
validated; TransparentPort validated; webhook envtest expects proxy-init.

Depends on kagenti-extensions#485 (transparent listener + enforce-redirect mode)
being released in the proxy-init + authbridge images. The install chart owns
image versions; this PR does not pin them. Do not merge until those images ship.

Preconditions for the always-on guarantee (documented, not enforced here):
- UID separation — agents must not run as Proxy.UID (1337) nor hold CAP_SETUID,
  or they could bypass the --uid-owner exemption. Confirm via PSA restricted/SCC
  or an admission check.
- Name-vs-IP policy — captured traffic gates on the recovered SNI/Host name
  (kagenti-extensions#485), which an agent controls independently of the IP, so
  it is reliable against a cooperative/misconfigured agent but not a hostile one.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants