Fix: Propagate every plugin header mutation in extproc and forwardproxy - #760
Fix: Propagate every plugin header mutation in extproc and forwardproxy#760JoshSag wants to merge 1 commit into
Conversation
reverseproxy already syncs the pipeline's whole header set onto the forwarded
request, and its comment states the bug it fixed:
Only Authorization used to be forwarded, silently dropping any other
injected header (e.g. static-inject's x-api-key).
extproc and forwardproxy still behave the way that comment describes. This
brings them to parity.
extproc gains a generic withHeaderMutation: diff pctx.Headers against a clone
taken before the pipeline ran, emit the difference as SetHeaders/RemoveHeaders.
It skips ':'-prefixed pseudo-headers, which govern routing, and
Content-Length/Content-Encoding, which the body-rewrite path and the transport
manage — the same exclusions reverseproxy makes. forwardproxy takes the
equivalent block.
Both drop the Authorization special case. Every writer in-tree emits
"Bearer "+token, so extract-and-re-prefix was the identity function on all real
inputs, and it mangled non-Bearer schemes because ExtractBearer returns empty
for them. Removing it takes three lines out of each of the four ext_proc
handlers and drops the auth import from the file. No header is special in any
listener now.
Affected today, with no telemetry involved: static-inject writes a configurable
header name (plugin.go:221) and deletes Authorization (plugin.go:229) — neither
reached the wire, the deletion because the old path only ever set that header.
cpex writes arbitrary pairs (manager_cpex.go:492).
Also here, separable in review: a 4-line authorityOf helper used at five sites.
The inbound ext_proc handlers never set pctx.Host while the outbound ones did,
though pipeline.SessionEvent documents Host for both directions and reverseproxy
always populated it. A 107-line table test covers every handler site and both
header forms.
Six listener-level regression tests come with this, asserting at the
ProcessingResponse layer that a plugin-level test cannot observe. Three use
ordinary header names to pin the general behaviour: an arbitrary header reaches
the wire, a deleted header is removed, pseudo-headers are never emitted.
Out of scope: extauthz (waypoint mode) has the same Authorization-only pattern
at server.go:86-92 and is untouched here.
Signed-off-by: YehoshuaSagron <ysagron@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe ext-proc listener now derives host context consistently and propagates general pipeline header mutations. The forward-proxy listener forwards added, changed, and deleted headers. Tests cover authority fallback, trace headers, arbitrary headers, deletions, and pseudo-headers. ChangesPipeline header propagation
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The change propagates plugin header additions and deletions across the affected listeners and aligns authority handling, with reported validation checks passing; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Client
participant ExtProcListener
participant Pipeline
participant ForwardProxy
participant Upstream
Client->>ExtProcListener: Send request
ExtProcListener->>Pipeline: Process request
Pipeline-->>ExtProcListener: Return mutated headers and body
ExtProcListener->>ForwardProxy: Emit header and body mutations
ForwardProxy->>Upstream: Forward synchronized headers and body
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
huang195
left a comment
huang195
left a comment
There was a problem hiding this comment.
Header-propagation change is correct and worth taking; I verified the parts that could bite:
- forwardproxy (
server.go:325-347) — checked ordering:pctx.Headersis a fullr.Header.Clone()(line 220), the hop-by-hop strip (lines 359-368) runs after the sync soProxy-Authorizationcan't be re-introduced upstream, and thepctx.BodyMutated()block still owns Content-Length. No leak. - Authorization special case removal — confirmed every
pctx.Headerswriter emits"Bearer "+token(jwtvalidation:381,tokenbroker:303,tokenexchange:707), soExtractBearer+ re-prefix was indeed the identity function on real inputs.placeholder_test.gocovers the inbound Authorization path through the handlers, so that path keeps regression coverage. append_action— Envoy's ext_proc reads the deprecatedappendbool (default false →setCopy), permutation_utils.cc:165-177, so omittingAppendActionreplaces rather than appends. Matches the retired helpers' behaviour.- No double-emit — header and body phases are mutually exclusive (
server.go:113-138). - No plugin uses
pctx.Headersas a scratchpad, so generalizing propagation leaks nothing internal; honouringstaticinject'sDel("Authorization")(plugin.go:229) is a security improvement in its own right.
One blocker: the bundled authorityOf change populates inbound Host from a caller-controlled authority, and inbound pctx.Host feeds ibac's un-guarded host-bypass, opa's policy input, and jwtvalidation's per-host audience. That needs to be split out or guarded before merge. Details inline.
Areas reviewed: Go (ext_proc / forward-proxy listeners), tests
Commits: 1 commit, signed off
CI status: all 19 checks passing (CodeRabbit still running)
| Direction: pipeline.Inbound, | ||
| Method: getHeader(headers, ":method"), | ||
| Scheme: getHeader(headers, ":scheme"), | ||
| Host: authorityOf(headers), |
There was a problem hiding this comment.
must-fix — The header-propagation fix is sound, but this hunk (and its twin at line 190) is not the neutral telemetry fix the description claims. On the inbound path :authority/Host is caller-controlled, and pctx.Host is read by decision-making plugins, not just recorded:
plugins/ibac/plugin.go:352—matchesAnyHost(p.bypassHosts, pctx.Host)→pctx.Skip("host_bypass"), with no direction guard.defaultBypassHostsincludes keycloak/spire/otel, plus whateveragent_llm_hostis set to. Today in ext_proc inbound this branch is inert becausepctx.Hostis""; after this change a caller who setsHost: keycloak...skips IBAC judging entirely.plugins/opa/plugin.go:525—"host": pctx.Hostbecomes caller-controlled policy input.plugins/jwtvalidation/plugin.go:393— withaudience_mode: per-host, the expected audience is derived from the caller-supplied authority.
This repo already documents the hazard and guards for it: plugins/cpex/plugin.go:306-310 gates matchesAnyHost behind pctx.Direction == pipeline.Outbound, with the comment "the Host header is attacker-controlled and identity has NOT been pre-validated."
Two ways forward, either is fine: (a) drop the two inbound authorityOf hunks and keep the outbound consolidation (lines 469/511, a pure no-op refactor); or (b) land them together with a direction guard on ibac's host-bypass check. Worth noting reverseproxy already populates inbound Host, so ibac's exposure pre-dates this PR — but this widens it to the ext_proc sidecar path, and that shouldn't ride along in a PR framed as header propagation.
| // headerMapToHTTP copies into pctx.Headers and whose :authority governs routing; | ||
| // and Content-Length / Content-Encoding, managed by withBodyMutation and the | ||
| // transport. | ||
| func withHeaderMutation(resp *extprocv3.ProcessingResponse, pctx *pipeline.Context, orig http.Header) *extprocv3.ProcessingResponse { |
There was a problem hiding this comment.
suggestion — With the Authorization special case retired, replaceTokenResponse (line 888) and replaceTokenBodyResponse (line 863) have no callers left. The five references in placeholder_test.go (lines 14, 31, 103, 106, 132) are comments, not calls, and now describe a path production no longer takes. Deleting both helpers and rewording those comments to name withHeaderMutation keeps the next reader from tracing a dead path.
| // (whose separator is "; ") — no plugin rewrites Cookie today, and | ||
| // one that does must split this out rather than discover it here. | ||
| set = append(set, &corev3.HeaderValueOption{ | ||
| Header: &corev3.HeaderValue{Key: strings.ToLower(k), RawValue: []byte(strings.Join(vv, ","))}, |
There was a problem hiding this comment.
nit — Two edges worth a line of comment or a follow-up:
headerMapToHTTP(line 766) usesh.Set, so a header that arrived on the wire with duplicate entries is already collapsed to its last value inpctx.Headers. Unchanged headers emit nothing so nothing regresses, but for a header a plugin does mutate, the emittedSetHeadersreplaces all wire values with the collapsed one.- A plugin doing
pctx.Headers[k] = nilinstead ofDel(k)lands here rather than in the remove loop, emitting an emptyRawValue— Envoy drops empty values withoutkeep_empty_value, so the effect is right by accident. Treating a zero-length slice as a delete makes it right by construction.
|
|
||
| // TestExtProc_Outbound_DeletedHeaderIsRemoved: a plugin deleting a header | ||
| // must emit RemoveHeaders — the narrow two-name diff could not express this. | ||
| func TestExtProc_Outbound_DeletedHeaderIsRemoved(t *testing.T) { |
There was a problem hiding this comment.
suggestion — All six new cases go through outboundRequest. handleInbound/handleInboundBody took the identical change, and inbound coverage today is only placeholder_test.go's Authorization case — the one header that worked before. One inbound variant of ArbitraryHeaderReachesWire + DeletedHeaderIsRemoved would pin the general behaviour on both paths; the harness already supports it.
| ) | ||
|
|
||
| // hostCapture records the pctx.Host the listener built, so a test can assert | ||
| // what plugins actually see (Host is what SessionEvent.Host and the lineage |
There was a problem hiding this comment.
nit — This comment says Host is what "the lineage plugin's lineage.peer.host fact" is derived from, which is hard to square with "That is an upstream omission rather than anything we need" in the PR body. The grep in the description is scoped to the two production files, so it's accurate as written — but the honest framing matters here, because the motivation is exactly what a reviewer weighs against the inbound-authority risk in my other comment.
3 participants
The problem
A plugin's header write does not reach the wire in two of your three
pipeline-running listeners.
reverseproxyalready gets this right. Its forwarding path syncs the wholepipeline header set onto the outgoing request, and the comment there states the
bug it was fixing (
authlib/listener/reverseproxy/server.go:270-289):extprocandforwardproxystill have the behaviour that comment describes asthe bug: they forward
Authorizationand nothing else. This PR brings them toparity.
Who this affects today, with no lineage involved
Grepping the plugins that write to
pctx.Headers:staticinjectSafeSetHeader(pctx.Headers, target, …),plugin.go:221)staticinjectpctx.Headers.Del("Authorization")(plugin.go:229)cpexmanager_cpex.go:492)tokenexchange,tokenbroker,jwtvalidationAuthorizationSo this is a general correctness fix to your own code. It is the reason the PR
is worth taking on its own merits, independent of anything else we are
proposing.
The change
extproc/server.gogains a genericwithHeaderMutation: it diffspctx.Headersagainst a clone taken before the pipeline ran, and emits thedifference as
SetHeaders/RemoveHeaderson theProcessingResponse. Itskips
:-prefixed pseudo-headers (:authoritygoverns routing) andContent-Length/Content-Encoding(managed by the body-rewrite path and thetransport) — the same exclusions
reverseproxymakes.forwardproxy/server.gotakes the equivalent sync block.Both retire the
Authorizationspecial case. Every upstream writer emits"Bearer " + token, so the dedicated extract-and-re-prefix path was theidentity function on all real inputs — and it mangled non-Bearer schemes, since
ExtractBearerreturns empty for them. Removing it makes the four ext_prochandlers three lines shorter each and drops an
authimport from the file.After this, no header is special in any listener.
A second, smaller fix in the same file: a 4-line
authorityOfhelper(
server.go:755), used at 5 sites. The inbound ext_proc handlers never setpctx.Host, while the outbound ones did — andpipeline.SessionEventdocuments
Hostfor both directions, withreverseproxyalways populating it.That is an upstream omission rather than anything we need; it is separated out
here so you can judge it on its own.
Tests
Six listener-level regression tests, in two new files:
They sit at the listener rather than in a plugin's suite deliberately: the
assertion is about what appears on the
ProcessingResponse— the boundary aplugin-level test structurally cannot observe.
ArbitraryHeaderReachesWire,DeletedHeaderIsRemovedandPseudoHeadersNeverEmittedare the ones that pinthe general behaviour; they use ordinary header names, not ours.
No lineage vocabulary in this diff
This is checkable, and we would rather you check it than take our word. The
production diff carries no lineage vocabulary:
Measured on the branch: no matches — the production diff mentions no telemetry
concept at all. The two new test files do mention traceparent/tracestate:
their regression fixture is a plugin that rewrites those headers, chosen
precisely because header mutations that must survive to the wire are what this
fix is about. Test vocabulary, not listener vocabulary.
Verification
Run under
golang:1.26on this branch, mirroring.github/workflows/ci.yaml:go vet ./...(authlib)go build ./...(authlib)go test -race -cover ./...(authlib)go vet+build+test -raceon bothcmd/authbridge-{envoy,proxy}(GOWORK=off)exclude_plugin_*tags)go mod tidybyte-clean × 3 modulesgofmt -lmainitself, measured on both. This branch adds no gofmt drift and touches none of those filesDeliberately out of scope
extauthz(waypoint mode) is a fourth pipeline-running listener with the sameAuthorization-only pattern (
extauthz/server.go:86-92). We have not touched it:we do not run that mode and cannot test the change end to end. Bringing it to
parity would be a natural follow-up, and the shape of the fix here should
transfer directly.
Assisted-By: Claude (Anthropic AI) noreply@anthropic.com
Summary by CodeRabbit
New Features
Bug Fixes