fix(proxy): always reject a proxy password that mismatches auth_token - #73
Conversation
A companion configured with actor_token_from delegates its entire Proxy-Authorization check to the STS: delegateAuth's hasBasicProxyAuth only required some non-empty password, never that it equaled the instance's own auth_token. Confirmed live: box A sent a request through box B's companion using A's own username and token; B answered 200 and served it as A, because the STS validates a token against whoever it belongs to, not against which instance received it. That behavior is correct for a shared instance serving many callers with distinct per-user passwords, but wrong for an instance meant to serve exactly one caller holding its own static token. Add proxy.reject_mismatched_auth (default false). When true, a presented password that does not equal the resolved auth_token/auth_token_env is rejected with 407, constant-time compared, before delegateAuth or any credential resolver runs. A request with no Proxy-Authorization at all is unaffected, so callers that never send proxy credentials keep working. The option requires auth_token or auth_token_env to be set and has no effect under a contextResolver, where tokens are never a single shared value. Every existing config keeps its current behavior unchanged.
📦 Previous Review (superseded by newer review)Claude finished @andybons's task in 6m 52s —— View job Claude is reviewing this PR
|
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation and tests are complete; the remaining documentation nit is non-blocking.
Review effort: Lite
Findings: None
What changed in this PR
Adds opt-in proxy password validation against the configured static token to prevent cross-instance credential use.
Changes:
- Adds constant-time mismatch rejection and configuration validation.
- Wires the option into proxy authentication.
- Adds regression tests and documentation updates.
| File | Description |
|---|---|
proxy/proxy.go |
Implements mismatch rejection. |
proxy/proxy_test.go |
Tests authentication scenarios. |
gatekeeper.go |
Wires configuration into the proxy. |
docs/content/reference/02-config-file.md |
Documents the option. |
docs/content/guides/06-token-exchange.md |
Explains token-exchange usage. |
config.go |
Adds configuration and validation. |
config_test.go |
Tests YAML parsing and defaults. |
config_authtoken_test.go |
Tests token validation requirements. |
CHANGELOG.md |
Records the feature. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The owner rejected the opt-in design: reject_mismatched_auth implied the old behavior (accept any non-empty password under delegateAuth) was a legitimate default rather than a bug. Removed the config option entirely. Behavior change: whenever auth_token or auth_token_env resolves to a non-empty token, a request presenting a Proxy-Authorization password that does not constant-time-equal it now always gets 407, before delegateAuth or any credential resolver runs. This flips TestProxy_DelegateAuthSkipsStaticCheck (renamed TestProxy_DelegateAuthRejectsMismatchedStaticToken) and TestHTTPSTokenExchangeActorTokenWithAuthToken (renamed TestHTTPSTokenExchangeActorTokenWithAuthTokenMismatchRejected), which both pinned the old "delegateAuth skips the static check" behavior as correct. A config that sets auth_token together with a different, STS-validated actor password now gets 407; a deployment that needs that must leave auth_token and auth_token_env unset. Absent credentials are unaffected, and delegateAuth with no auth_token configured keeps requiring, and forwarding to the token exchange, any non-empty password, matching or not, exactly as before. contextResolver (daemon) mode is unaffected and unchanged: authToken is never a single shared secret there, so this check does not apply — each caller's own token is looked up individually. Checked every other reader of the proxy password: - proxy/postgres.go's authenticate already applies this exact rule unconditionally (constant-time compare against authToken whenever it's set, no delegateAuth-equivalent bypass ever existed there) — already consistent, no change needed. - gatekeeper_tokenexchange.go's extractProxyAuthCredentials reads the Basic password only inside a CredentialResolver, which ServeHTTP invokes after this check has already run — so it now only ever sees a password that already passed, when auth_token is set. - The remaining Proxy-Authorization references (proxy/mcp.go, proxy/relay.go, proxy.go's outbound header stripping) only delete the header before forwarding upstream; they make no auth decision. Docs: docs/content/reference/02-config-file.md folds the rule into proxy.auth_token and states the contextResolver exception; docs/content/guides/06-token-exchange.md states the auth_token interaction plainly. CHANGELOG.md's v0.25.0 entry moves to Changed.
andybons
changed the title
📦 Previous Review (superseded by newer review)Claude finished @andybons's task in 5m 59s —— View job Claude finished the code review
SummaryI reviewed I traced the new upfront check in Two lower-severity, non-blocking findings, posted inline where the diff allows:
No security, race, or error-handling issues found beyond the above. Security-sensitive properties (constant-time comparison, credentials never logged, exemption of |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Second Copilot review round on #73. gatekeeper_test.go: TestHTTPSTokenExchangeActorTokenWithAuthTokenMismatchRejected only asserted the backend never saw the request, which does not prove the resolver (and its STS round trip) never ran. Add an stsCalls counter, following the existing pattern in TestHTTPSTokenExchangeOutrankedByStatic, and assert it stays zero. Checked before the err==nil Fatal so a regression that lets the request through still reports the resolver having run, instead of being masked by the Fatal's early return. Red-verified by disabling the ServeHTTP check: the new assertion failed with "STS was called 1 times, want 0" before failing on the missing error, then passed again once restored. proxy/proxy.go: the upfront mismatch check duplicated checkAuth's extractProxyToken + subtle.ConstantTimeCompare inline. checkAuth now returns (ok, present bool) so both call sites share one comparison: the upfront check rejects when present && !ok, and the later `else if p.authToken != ""` branch — previously split into a `!checkAuth(r)` reject case and a separate success case purely to mirror checkAuth's single bool — collapses into one branch that only checks `present`, since the upfront check has already handled every present-and-mismatched case by the time execution reaches it.
📦 Previous Review (superseded by newer review)Claude finished @andybons's task in 13m 24s —— View job Claude finished the review
|
Third Copilot review round on #73, five threads. SECURITY: handleRelay is reached directly via the NO_PROXY bypass in ServeHTTP, before the CONNECT/plain-HTTP auth chain the previous commit fixed. A direct /relay/{name} request carrying another instance's still-valid Proxy-Authorization reached getCredentialsForRequest, and the STS behind a token-exchange credential, exactly like the CONNECT path already fixed. handleRelay now calls the same mismatch check before doing any relay work. Red-verified with a new table test in proxy/relay_test.go (TestRelay_MismatchedAuthTokenRejectedBeforeResolver): before the fix, a mismatched password returned 200 and the resolver was called once; after, it returns 407 and the resolver is never called, while a matching password and absent credentials both still reach the resolver unchanged. Maintainability: removed the standalone pre-check block ahead of the auth chain. checkAuth now returns (ok, present bool), and a new rejectMismatchedAuth(w, r) helper wraps it into the single "write 407 and return true" comparison every enforcing site needs. The plain authToken branch, the delegateAuth branch, and handleRelay each call it directly — one helper, one comparison, called once per request, with no ordering dependency on a block ahead of the chain. Both call sites now write the same body text, "Proxy authentication required", for an absent password and a mismatched one, so a prober cannot distinguish whether auth_token is configured from the 407 alone. The WARN log still names only the mismatch, never the presented value. CHANGELOG.md's v0.25.0 entry: corrected the closing sentence, which read as if unauthenticated callers are now allowed somewhere — the change is that no *new* rejection reason is added for absent credentials, not that absence is newly tolerated; delegateAuth and a configured auth_token both already rejected absent credentials before this PR. Also notes the relay path is covered by the same rule. Docs: docs/token-exchange-endpoint.md and docs/content/reference/03-credential-sources.md both still claimed actor_token_from replaces or skips the static auth_token check outright; corrected to state the auth_token-set/auth_token-unset split. docs/content/reference/02-config-file.md now also names the relay path explicitly. Grepped the repo for the same claim elsewhere; docs/content/guides/06-token-exchange.md and skills/configuring-gatekeeper/references/credential-sources.md already stated it correctly from the prior round, and CHANGELOG.md's historical v0.5.0 entry describes what shipped at the time and is left alone.
📦 Previous Review (superseded by newer review)Claude finished @andybons's task in 2m 31s —— View job Claude finished the code review
SummaryThis push ( Fixed:
Still open (not addressed by any of the three follow-up commits):
Minor, non-blocking: in the I did not find any new security, race, or logic issues in |
Claude review on #73, two items. gatekeeper.go: the comment above the actor_token_from loop still said gatekeeper "skips" the static authToken check, which stopped being true once rejectMismatchedAuth started enforcing it under delegateAuth. Reworded to one line matching SetDelegateAuth's updated doc in proxy/proxy.go. proxy/proxy.go: the plain authToken branch called p.checkAuth(r) twice — once inside rejectMismatchedAuth, once again right after to check presence. rejectMismatchedAuth now returns (rejected, present bool), computing checkAuth exactly once; the authToken branch uses the returned present instead of calling checkAuth again, and the delegateAuth and relay call sites ignore present since they already have their own presence check (hasBasicProxyAuth) or don't need one.
|
Claude finished @andybons's task in 8m 38s —— View job Claude finished the review
SummaryThis is a review of the current state ( Core fix verified correct. I dispatched the One residual finding, posted inline on Minor, non-blocking observations from the automated passes (not inline-commented, since they're stylistic/efficiency rather than correctness, and this PR has already been through several rounds of nit-fixing):
I wasn't able to run |
| // bypassing the CONNECT/plain-HTTP auth chain entirely, so it enforces | ||
| // authToken itself before any resolver or STS call. Absent credentials | ||
| // are unaffected: relay traffic requires no Proxy-Authorization at all. | ||
| if rejected, _ := p.rejectMismatchedAuth(w, r); rejected { |
There was a problem hiding this comment.
Minor, non-blocking: rejectMismatchedAuth only branches on p.authToken — it never checks p.contextResolver. The main ServeHTTP chain (proxy/proxy.go:2058) treats contextResolver != nil and the authToken/delegateAuth chain as mutually exclusive (daemon mode looks up each caller's own token via contextResolver and never consults authToken at all), but handleRelay's dispatch (proxy/proxy.go:2031) is gated only on len(p.relays) > 0, with no contextResolver == nil guard.
Today this is safe in practice because the only real consumer (moat) never calls both SetAuthToken and SetContextResolver on the same Proxy, so p.authToken == "" whenever contextResolver is set, and rejectMismatchedAuth short-circuits harmlessly. But since AGENTS.md notes this is a general-purpose library "meant to be imported," nothing in the type enforces that exclusivity — a future consumer that configures both (e.g. a static authToken as a fallback alongside daemon-mode contextResolver, while also registering relays) would find /relay/ requests rejected against the static authToken even though the same per-caller password would be accepted by the main proxy path via contextResolver. Worth either asserting the exclusivity where both are set, or having rejectMismatchedAuth/handleRelay skip the authToken compare when contextResolver != nil, consistent with how ServeHTTP treats the two modes everywhere else.
Summary
A gatekeeper companion configured with
actor_token_from(RFC 8693 token exchange) used to delegate its entire proxy-auth check to the STS.delegateAuth'shasBasicProxyAuthonly required that some non-empty password was presented, never that it equaled the instance's ownauth_token.Confirmed live on boxes production: box A sent a request through box B's companion using A's own username and token.
B's companion answered 200 and served it as A, its log showing
user_id=A, because the STS validates a token against whoever it actually belongs to, not against which instance received it.An earlier version of this PR added an opt-in
proxy.reject_mismatched_authconfig flag, off by default. The owner rejected that direction: accepting a mismatched password is a bug, not a legitimate default behavior that should require opting out of. This PR now makes the correct behavior unconditional and removes the flag.Behavior change
Whenever
auth_tokenorauth_token_envresolves to a non-empty token, a request presenting aProxy-Authorizationpassword that does not constant-time-equal it now always gets407, beforedelegateAuthor any credential resolver ever runs.A config that sets
auth_tokentogether withactor_token_fromand expects a different, STS-validated password to still pass will now see that password rejected.A deployment that needs distinct per-caller passwords through one shared instance must leave
auth_tokenandauth_token_envboth unset; with neither set,actor_token_fromkeeps requiring, and forwarding to the STS, any non-empty password exactly as before.A request with no
Proxy-Authorizationat all is unaffected, so callers that never send proxy credentials keep working.contextResolver(moat's daemon mode) is unaffected:auth_tokenis never a single shared secret there, since each caller's own token is looked up individually, so this check does not apply.Other consumers of the proxy password
proxy/postgres.go'sauthenticatealready applies this exact rule unconditionally: a constant-time compare againstauthTokenwhenever it's set, with nodelegateAuth-equivalent bypass ever existing on the Postgres data-plane listener. Already consistent, no change needed.gatekeeper_tokenexchange.go'sextractProxyAuthCredentialsreads the Basic password only inside aCredentialResolver, whichServeHTTPinvokes after this new check has already run. It now only ever sees a password that already passed, wheneverauth_tokenis set.Proxy-Authorizationreferences (proxy/mcp.go,proxy/relay.go, the outbound header stripping inproxy.go) only delete the header before forwarding upstream; none of them make an authentication decision.Tests
TestProxy_DelegateAuthSkipsStaticCheck(proxy package) is renamedTestProxy_DelegateAuthRejectsMismatchedStaticTokenand now asserts407instead of200— a genuine, intentional behavior change.TestHTTPSTokenExchangeActorTokenWithAuthToken(top-level package, full end-to-end TLS+STS integration test) is renamedTestHTTPSTokenExchangeActorTokenWithAuthTokenMismatchRejectedand now asserts rejection instead of a successful exchange, also confirming the backend never receives the request.TestProxy_MismatchedProxyPasswordRejectedkeeps the table: mismatched → 407, matching → pass, absent → unchanged,delegateAuthwith noauth_token→ the non-empty password still passes through.Docs
docs/content/reference/02-config-file.mdfolds the rule intoproxy.auth_token/auth_token_envand states thecontextResolverexception plainly.docs/content/guides/06-token-exchange.mdstates theauth_tokeninteraction withactor_token_fromplainly, replacing the removed config-flag cross-reference.CHANGELOG.md's v0.25.0 entry moves from Added to Changed, describing this as a behavior change with the config-migration note.Test plan
ServeHTTPcheck neutered,TestProxy_DelegateAuthRejectsMismatchedStaticToken,TestProxy_MismatchedProxyPasswordRejected/mismatched_password_from_another_box_is_rejected, andTestHTTPSTokenExchangeActorTokenWithAuthTokenMismatchRejectedall failed for the named reason (status = 200, want 407).go build ./...,go vet ./...,gofmt -l .(clean),go test -race -timeout 600s ./...,go run ./cmd/skill-lintall pass.