fix(proxy): default deny provider operations - #439
Conversation
hbrodin
left a comment
There was a problem hiding this comment.
Reviewed origin/main...1bacfeb (2 files, +122/-8).
The security core looks sound. I probed Uri::path() against the pinned http 1.4.2 and could not construct a bypass: percent-encoding, dot segments, duplicate and trailing slashes, * and authority-form all fail the exact-match arms in the safe direction. There is also no check/forward parser differential, because origin_form re-serializes path_and_query() from the same Uri the policy inspected, so the bytes checked are the bytes forwarded. Absolute-form targets yield the same path and are still dialed at the config-pinned host. Placement is right too: after the capability check, so the allowlist cannot be probed unauthenticated, and before the permit, so denied requests cannot exhaust the concurrency semaphore. Gates pass at head (fmt, clippy -D warnings, 29 unit + 9 gate tests).
Four inline comments above. The rest sit in files this diff does not touch, so they cannot be anchored inline:
src/proxy.rs:11-13 - "The binary is provider-agnostic - it injects one configured header to one fixed upstream - so a provider is just a different upstream host, port, and auth scheme resolved on the host." The second clause is now false: a third provider also needs an arm in operation_allowed, and until it gets one every request 403s (which this PR's own unknown_upstream_has_no_allowed_operations pins). Since GitHub is a tracked follow-up, this will misdirect whoever picks it up. The same claim repeats in shorter form at line 57.
The allowlist is documented nowhere. Grepping docs/ and README.md for v1/messages, v1/responses, count_tokens and input_tokens returns zero hits. Two places worth a line each:
docs/credential-proxy.md:26-28describes the request path as "verifies that token (constant-time), strips it, injects the real credential ..., and streams the request to the pinned upstream" - no deny step, and nothing anywhere names the four permitted operations.docs/trust-model.md:144-149describes the proxy's guards as the capability token plus the fixed per-provider upstream. Given that file's convention of pairing an invariant with a tripwire ("a change that lets the guest influence the upstream host ... is a finding"), a clause here is what stops the allowlist being quietly widened later. The residual gap your PR body states well - objects still reachable by ID inside the allowed bodies, which are forwarded unexamined - reads naturally in that file's accepted-limitations style.
tests/integration.sh:4240-4262 - test_proxy curls GET /v1/models with the capability token and branches on whether the body contains "capability". That request now returns 403 operation is not allowed by coop-proxy, so the pass "valid capability token passes the gate (forwarded upstream)" branch fires for a request that was never forwarded. The assertion still discriminates a broken auth gate, since a 401 body does contain "capability" - so it is not vacuous - but the label and the five-line comment above it now describe behaviour that can no longer happen. Pointing the token-bearing probe at POST /v1/responses makes both true again. Note this phase is --full-only, so CI would not have caught it.
Smaller things, no reply needed:
coop-proxy/src/proxy.rs:508- theOpenAI operation policybanner also labels the Anthropic tests andunknown_upstream_has_no_allowed_operations, and is 66 columns where the three sibling banners are 64.- Optionally one
POST /v1/messages/row to pin trailing-slash fail-closed, and one cross-provider row (POST api.openai.com /v1/messages) to pin the host-path pairing. Neither is a security boundary - a merged path set would yield upstream 404s, not escalation. - Claude Code's
HEAD /api/hellowarm-up probe and itsGET /v1/models?limit=1000discovery call now 403. Anthropic's gateway protocol names both as rejectable without breaking anything, and coop never setsCLAUDE_CODE_ENABLE_GATEWAY_MODEL_DISCOVERY, so this is not a functional break - just worth a doc line so it is not debugged twice. - Both agents auto-update inside the guest (
scripts/guest/codex.sh:36,scripts/guest/claude-code.sh:30) while the allowlist is a closed set of four literal paths, so a future agent release that adds an endpoint 403s with an opaque body. Codex is safe today by a narrow margin:/responses/compactis gated onis_openai()(provider name"OpenAI") and coop names its provider"coop credential proxy". Worth a sentence aboveoperation_allowedrecording that dependency.
.cargo/mutants.toml needs no change - coop-proxy is outside the sweep as a non-default workspace member.
| forward(upstream_req, ctx, permit).await | ||
| } | ||
|
|
||
| /// Whether the fixed upstream permits this method/path pair. |
There was a problem hiding this comment.
Two things about this doc block.
The summary line says the upstream permits the method/path pair, but this is coop-proxy's own policy: api.openai.com would happily serve POST /v1/organization/admin_api_keys, and refusing it is the entire point of the function. Something like "Whether coop-proxy allows this method/path pair for the fixed upstream" would read truer. The paragraph below already frames it correctly, so this is just the rustdoc summary line - the one sentence that shows up in the module index.
Separately: this file opens with a list headed "The security-critical invariants:" (lines 8-16), enumerating the capability token, the fixed upstream host/scheme, and the credential strip/inject. This PR adds a fourth invariant of exactly that class without adding a bullet, so someone auditing the proxy's guarantees from the file header misses the policy the PR exists to add. It is in this file already, so it is not scope creep.
There was a problem hiding this comment.
Addressed in fbcdcd7: the rustdoc now identifies coop-proxy as the policy owner, and the file-level invariant list includes local default-deny operation filtering.
| (upstream_host, uri.path()), | ||
| ( | ||
| "api.openai.com", | ||
| "/v1/responses" | "/v1/responses/input_tokens" |
There was a problem hiding this comment.
Worth justifying or dropping /v1/responses/input_tokens.
Codex appears to have no code path to it. The only request paths it builds are ResponsesEndpoint::{Responses, Guardian} plus the /responses/compact, /realtime/calls and /memories/trace_summarize constants in codex-rs/core/src/client.rs; token accounting arrives on the inference response as ResponseEvent::Completed { token_usage }. OpenAI's own hardening proxy for Codex allows exactly POST /v1/responses and nothing else.
It is also not free. Per OpenAI's API reference the endpoint accepts conversation - "Items from this conversation are prepended to input_items" - and previous_response_id. That makes it a token-count oracle over stored objects belonging to the key owner: a guest can confirm the existence of, and measure, conversations and responses by ID. It leaks counts rather than contents, so it is weaker than a direct read, but it is the same class of operation this PR names as the thing it closes ("retrieving a stored OpenAI response directly").
If a Codex version is known to call it, a comment saying so would settle it; otherwise dropping the arm keeps the allowlist to what the agents demonstrably need.
There was a problem hiding this comment.
Dropped it in 3d39d5c. Current Codex does not use this endpoint, and keeping the allowlist to demonstrated agent traffic avoids the stored-object token-count oracle.
| //! logic is covered by the unit tests in `src/proxy.rs`, and end-to-end against | ||
| //! a mock upstream by coop's VM integration suite. | ||
| //! upstream is never contacted; a request with a valid token reaches the | ||
| //! operation policy and is denied with 403 for the test-only unknown upstream, |
There was a problem hiding this comment.
The stated cause is not the operative one. request_status sends GET /v1/messages (line 248), so operation_allowed returns false at if method != Method::POST before the (upstream_host, uri.path()) match is evaluated at all - the denial has nothing to do with proxy-test.invalid. As written, adding proxy-test.invalid to the allowlist, or breaking the host arm outright, would leave this test green.
Switching the harness to POST /v1/messages fixes both halves at once: the doc's stated mechanism becomes the one actually exercised, and the host-policy branch gets its first coverage. It still returns 403 and still never dials.
There was a problem hiding this comment.
Addressed in c8f50d0. The gate request is now POST /v1/messages, so a valid capability token reaches the unknown-host policy branch before returning 403.
| let (addr, _child) = spawn_serving().await; | ||
| let status = request_status(addr, Some("Authorization: Bearer the-right-token")).await; | ||
| assert!(status.contains("502"), "expected 502, got: {status:?}"); | ||
| assert!(status.contains("403"), "expected 403, got: {status:?}"); |
There was a problem hiding this comment.
Two notes on retargeting this from 502 to 403.
The 502 was the only automated proof that an authorized request traverses the rest of the path - permit acquisition, build_upstream_headers (credential strip, injection, Host pin), origin_form, and into forward. It now stops two statements into proxy(). build_upstream_headers and origin_form are still directly unit-tested in src/proxy.rs, so what actually loses all coverage is forward, bad_gateway, and the GuardedBody permit wiring. Because the allowlist keys on the two real hostnames, restoring a 502 would mean pointing the harness at a real provider host - live egress, which proxy-test.invalid exists to avoid. That reads as an unavoidable trade-off rather than a defect, so recording it in the module doc seems better than chasing the coverage back.
Two doc-comments above are now stale in the same way the module doc was: RESPONSE_TIMEOUT (line 46) justifies its 20s budget as "Read budget for a test's own request, which waits on the real upstream path", which no caller does any more; and config_json (line 62) says "any request that passes the gate fails closed rather than reaching a real service", but nothing passes to the upstream now.
There was a problem hiding this comment.
Addressed in 1584d91. The gate harness now documents that its unknown-profile refusal prevents it from exercising forward, bad_gateway, and GuardedBody; the timeout and fake-config comments now describe their actual behavior.
Summary
POST /v1/responsesfor OpenAIPOST /v1/messagesandPOST /v1/messages/count_tokensfor Anthropic403Why
This is defense in depth against a compromised guest using the host credential for provider administrative, account/model-discovery, or direct resource-retrieval APIs. It prevents operations such as minting an OpenAI admin key, creating an Anthropic organization invite, retrieving a stored OpenAI response directly, and retrieving Anthropic batch results.
The policy is matched against the pinned upstream host as well as the request method and path, so an operation allowed for one provider cannot be replayed through the other provider profile. It does not replace tenant or object-level authorization: the proxy deliberately streams bodies without parsing them, and an allowed OpenAI Responses request can still reference provider objects by ID when the injected credential is authorized for them.
The route set is intentionally closed. Future agent versions that need a new endpoint fail locally until a deliberate policy, test, and documentation change is reviewed. Current Codex proxy configuration is named
coop credential proxy, notOpenAI; therefore Codex does not currently use its OpenAI-specificPOST /v1/responses/compactbehavior.Review follow-ups
GET /v1/modelsTesting
cargo +1.94.1 fmt --all -- --checkcargo +1.94.1 test -p coop-proxycargo +1.94.1 clippy -p coop-proxy --all-targets -- -D warningsbash -n tests/integration.sh