Skip to content

fix(proxy): default deny provider operations - #439

Merged
hbrodin merged 10 commits into
trailofbits:mainfrom
0xalpharush:security/openai-default-deny
Sep 3, 2026
Merged

fix(proxy): default deny provider operations#439
hbrodin merged 10 commits into
trailofbits:mainfrom
0xalpharush:security/openai-default-deny

Conversation

@0xalpharush

@0xalpharush 0xalpharush commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Summary

  • default-deny credential-proxy operations after capability-token authentication and before any upstream connection
  • allow only POST /v1/responses for OpenAI
  • allow only POST /v1/messages and POST /v1/messages/count_tokens for Anthropic
  • reject every other method, path, cross-provider route, trailing-slash variant, and unknown upstream profile locally with 403

Why

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, not OpenAI; therefore Codex does not currently use its OpenAI-specific POST /v1/responses/compact behavior.

Review follow-ups

  • removed the unneeded OpenAI token-count endpoint rather than creating a stored-object token-count oracle
  • corrected the VM integration probe to use the allowed Responses operation instead of the now-denied GET /v1/models
  • added exact-path, cross-provider, unknown-profile, and authentication-to-policy coverage
  • documented the allowlist, body-reference limitation, and agent compatibility boundary

Testing

  • cargo +1.94.1 fmt --all -- --check
  • cargo +1.94.1 test -p coop-proxy
  • cargo +1.94.1 clippy -p coop-proxy --all-targets -- -D warnings
  • bash -n tests/integration.sh

@0xalpharush
0xalpharush marked this pull request as ready for review August 31, 2026 22:26

@hbrodin hbrodin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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-28 describes 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-149 describes 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 - the OpenAI operation policy banner also labels the Anthropic tests and unknown_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/hello warm-up probe and its GET /v1/models?limit=1000 discovery call now 403. Anthropic's gateway protocol names both as rejectable without breaking anything, and coop never sets CLAUDE_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/compact is gated on is_openai() (provider name "OpenAI") and coop names its provider "coop credential proxy". Worth a sentence above operation_allowed recording that dependency.

.cargo/mutants.toml needs no change - coop-proxy is outside the sweep as a non-default workspace member.

Comment thread coop-proxy/src/proxy.rs Outdated
forward(upstream_req, ctx, permit).await
}

/// Whether the fixed upstream permits this method/path pair.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Comment thread coop-proxy/src/proxy.rs Outdated
(upstream_host, uri.path()),
(
"api.openai.com",
"/v1/responses" | "/v1/responses/input_tokens"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Comment thread coop-proxy/tests/gate.rs
//! 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,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Comment thread coop-proxy/tests/gate.rs
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:?}");

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@hbrodin
hbrodin merged commit 199a95b into trailofbits:main Sep 3, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants