feat(auth): rfc 0029 green (query/mcp binding) — one resolver on every surface - #425
Conversation
…y surface
The §3.3 binding completes across the read side: the querier's
/v1/query handler, the MCP transport-level bearer gate, and the
per-tool tenant check all resolve through the same async AuthResolver
the ingest listeners use — static store first, then OIDC — and the
transitional `enforcement_store()` bridge is retired exactly as its
own doc promised (RFC 0026 gates now consume the full auth config).
- querier.rs / mcp.rs: `Option<Arc<TokenStore>>` → `AuthResolver`
throughout; `check_tenant` is async (an OIDC unseen-kid miss may
refetch); open mode = `resolver.is_open()`.
- main.rs: both network roles build their resolver via the renamed
`auth_resolver` (OIDC discovery at startup; failure names auth.oidc).
The querier role therefore now requires a reachable issuer for an
oidc-configured startup — the .1 served test gained the fixture
issuer accordingly.
- ourios-core: `AuthConfig::enforcement_store` removed with its tests
migrated to direct `static_tokens` assertions (the empty-store
oidc-only semantics are superseded by resolver enforcement, asserted
on the wire by the served arms).
§5: RFC0029.3/.4/.5 are live against the served binary —
.3: tenant_claim [a,b] drives RFC 0026 verbatim (in-set ingest acks,
out-of-set whole-batch PERMISSION_DENIED, query 401→400→403 order);
.4: wildcard ingests/queries arbitrary tenants; .5: one config with
both halves — the static bearer and a JWT authenticate via their own
paths, each confined to its own binding on ingest and query (the
static token exercising the RFC 0020 ${env:…} secret-hygiene rule).
.2/.6 stubs flipped to discharged markers naming their core oracles.
Only .7 (Dex) remains.
Full gate: 978 passed / 0 failed, clippy -D warnings, rustdoc, fmt,
cargo-deny.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe ChangesAuthResolver migration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/ourios-core/src/auth/mod.rs (1)
562-568: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale reference to the removed enforcement bridge.
The bridge was deleted, but this test's doc comment still claims it validates that "the oidc-only enforcement bridge rejects every bearer rather than opening the gates" — behavior the test no longer exercises (it only asserts
static_tokens/oidcshape). The_oidc_only_bridgesuffix in the test name is likewise now misleading. Consider trimming the comment and renaming to reflect that enforcement is now a resolver/serving property.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ourios-core/src/auth/mod.rs` around lines 562 - 568, The test name and doc comment in auth_config_rules_and_oidc_only_bridge still refer to the removed enforcement bridge, but the test now only checks config shape for static_tokens and oidc. Update the documentation to remove the claim about “the oidc-only enforcement bridge rejects every bearer” and rename the test to reflect its current scope as a resolver/serving rules check, using the auth_config_rules_and_oidc_only_bridge symbol as the locator.crates/ourios-server/tests/it/rfc0029_oidc.rs (2)
580-582: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Content-Length: 16is hand-tied to the literal body.The header hardcodes
16while the bodytemplate_id == 1is pushed in the same call. Any future edit to the query string desynchronizes the length, which will stall the read or trip a request error rather than fail loudly. Deriving the length from the body removes the footgun.♻️ Compute Content-Length from the body
- request.push_str( - "Content-Type: text/plain\r\nContent-Length: 16\r\nConnection: close\r\n\r\ntemplate_id == 1", - ); + let body = "template_id == 1"; + write!( + request, + "Content-Type: text/plain\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{body}", + body.len(), + ) + .expect("write body");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ourios-server/tests/it/rfc0029_oidc.rs` around lines 580 - 582, The request in the RFC0029 OIDC test hardcodes Content-Length to 16 while appending the body in the same call, so the header can drift from the actual payload. Update the construction around the request.push_str call to derive Content-Length from the body string instead of a literal, using the same body value that contains template_id == 1 so future edits stay in sync.
533-534: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNulling child stderr blinds the 15s startup wait.
If the child fails to announce its roles, the only diagnostic is the
panic!("role announcements never appeared")after a 15s timeout — stderr is discarded.rfc0029_1_oidc_only_starts_and_enforcesdeliberately buffers stderr for exactly this reason. Consider capturing stderr here (at least on the timeout path) so spawn failures across the threeclaim_bindingtests are debuggable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ourios-server/tests/it/rfc0029_oidc.rs` around lines 533 - 534, The child process in the `claim_binding` test setup is discarding stderr, which makes the 15s startup timeout impossible to diagnose. Update the test harness around the child spawn/role-announcement wait to capture stderr like `rfc0029_1_oidc_only_starts_and_enforces` does, and surface that buffered output on the timeout path or spawn failure. Keep the fix localized to the `claim_binding`-related startup helper so failures in all three tests remain debuggable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/ourios-core/src/auth/mod.rs`:
- Around line 562-568: The test name and doc comment in
auth_config_rules_and_oidc_only_bridge still refer to the removed enforcement
bridge, but the test now only checks config shape for static_tokens and oidc.
Update the documentation to remove the claim about “the oidc-only enforcement
bridge rejects every bearer” and rename the test to reflect its current scope as
a resolver/serving rules check, using the auth_config_rules_and_oidc_only_bridge
symbol as the locator.
In `@crates/ourios-server/tests/it/rfc0029_oidc.rs`:
- Around line 580-582: The request in the RFC0029 OIDC test hardcodes
Content-Length to 16 while appending the body in the same call, so the header
can drift from the actual payload. Update the construction around the
request.push_str call to derive Content-Length from the body string instead of a
literal, using the same body value that contains template_id == 1 so future
edits stay in sync.
- Around line 533-534: The child process in the `claim_binding` test setup is
discarding stderr, which makes the 15s startup timeout impossible to diagnose.
Update the test harness around the child spawn/role-announcement wait to capture
stderr like `rfc0029_1_oidc_only_starts_and_enforces` does, and surface that
buffered output on the timeout path or spawn failure. Keep the fix localized to
the `claim_binding`-related startup helper so failures in all three tests remain
debuggable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f5ee407a-cccf-4963-bcf3-c64e7c8d9776
📒 Files selected for processing (7)
crates/ourios-core/src/auth/mod.rscrates/ourios-server/src/main.rscrates/ourios-server/src/mcp.rscrates/ourios-server/src/querier.rscrates/ourios-server/tests/it/rfc0026_auth.rscrates/ourios-server/tests/it/rfc0027_mcp.rscrates/ourios-server/tests/it/rfc0029_oidc.rs
There was a problem hiding this comment.
Pull request overview
This PR completes RFC 0029 “slice 3b” by wiring the querier /v1/query and MCP surfaces onto the same async AuthResolver used by ingest, and removes the transitional AuthConfig::enforcement_store() bridge from core/server configuration.
Changes:
- Replaces querier/MCP auth plumbing from
Option<Arc<TokenStore>>+authenticate_bearer(...)toAuthResolver+ asyncauthenticate(...)(including async per-tool tenant checks for MCP). - Updates server startup to construct an
AuthResolver(including OIDC discovery) for network roles, and removes theenforcement_store()bridge. - Expands RFC 0029 served-binary integration tests to cover claim-binding enforcement and wildcard/coexistence behavior across ingest + query.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/ourios-server/src/querier.rs | Switches query endpoint auth from static store lookup to async AuthResolver authentication. |
| crates/ourios-server/src/mcp.rs | Updates MCP bearer gating and per-tool tenant enforcement to use async AuthResolver. |
| crates/ourios-server/src/main.rs | Retires enforcement_store() usage and builds role auth via auth_resolver() at startup. |
| crates/ourios-core/src/auth/mod.rs | Removes AuthConfig::enforcement_store() and updates related docs/tests. |
| crates/ourios-server/tests/it/rfc0029_oidc.rs | Adds served-binary tests for RFC 0029 claim binding/wildcard/coexistence on query. |
| crates/ourios-server/tests/it/rfc0027_mcp.rs | Adjusts MCP router tests for new AuthResolver parameter type. |
| crates/ourios-server/tests/it/rfc0026_auth.rs | Adjusts query auth tests for new AuthResolver parameter type. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ments auth_resolver returns None when no network role is enabled and is built exactly once otherwise — one OIDC discovery, one shared JWKS cache/throttle across receiver + querier. QuerierConfig/router/ require_bearer docs describe the resolver, not the retired store. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…hed binding require_bearer caches the resolved AuthBinding on the request; the per-tool tenant check reads it from the forwarded parts (sync again), failing closed when absent — no second verification, no second possible JWKS fetch per tool call. spawn_with_auth inherits the child's stderr so startup failures aren't bare timeouts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
#426) * feat(auth): rfc 0029 green — dex end-to-end acceptance (.7) and status flip The last §5 arm: a real Dex container (testcontainers, CI-gated like the localstack job) mints client-credentials tokens the served binary verifies against Dex's real JWKS. - The static client carries the tenant list via clientCredentialsClaims.groups (tenant_claim: groups) and the name label via its display name (name_claim: name, scope profile) — the OTel Collector oauth2client flow verbatim. - Arms: startup discovery against Dex; in-claim gRPC ingest acks; cross-tenant batch PERMISSION_DENIED; in-claim query 200; MCP 401 bearer-less / success with the Dex bearer; real-TTL expiry (8 s tokens, zero skew) collapses to the undifferentiated 401; SIGTERM flushes the audit sink and the read-back ingest_denied event carries the name_claim value; no JWT material in error bodies or the log surface. - Image: Dex `master` pinned by digest — the client-credentials grant and clientCredentialsClaims are merged upstream (dexidp/dex#4691) but post-v2.45.1; the RFC §6 note + ci.yml comment record the bump path to v2.46. New required `dex oidc (testcontainers)` CI job runs the arm by exact name. - reqwest (rustls + json) joins the server dev-deps for minting and readiness polling — the same stack the verifier itself uses. RFC 0029 status red → green: .1–.6 discharged across #420–#425, .7 lands here and runs in this PR's own required CI job. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(auth): surface dex container logs on readiness timeout Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(auth): dex needs one connector — enablePasswordDB, inert for client-credentials Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(auth): dex arm hardening — ttl headroom, port retry, async sigterm; ci creds + rfc quote 20s tokens with the expiry wait driven by the response's expires_in; container start retries a fresh port on the reserve race; SIGTERM via tokio Command with an asserted status; persist-credentials: false on the dex job checkout; the RFC image note no longer swallows the following paragraph. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
What
RFC 0029 query/MCP binding (slice 3b) — the read side joins the ingest side (#424) on one async
AuthResolver, and the transitionalenforcement_store()bridge retires exactly as its own doc comment promised./v1/query+ MCP (transport bearer gate and per-tool tenant check):Option<Arc<TokenStore>>→AuthResolverthroughout;check_tenantis now async (an OIDC unseen-kidmiss may refetch the JWKS).auth_resolver— OIDC discovery failure is a startup error namingauth.oidc. The.1served test gained a fixture issuer since an oidc-configured querier now also discovers at startup.AuthConfig::enforcement_storeremoved from ourios-core; its tests migrated to directstatic_tokensassertions (the oidc-only empty-store semantics are superseded by resolver enforcement, asserted on the wire).§5 — .3/.4/.5 live on the served binary
tenant_claim ["a","b"]: in-set ingest acks; out-of-set whole-batchPERMISSION_DENIED; query enforces 401 → 400 → 403 in order, in-set 200["*"]ingests + queries alpha/beta/entirely-new-tenantacme, JWT confined toglobex, side by side on ingest and query — each authenticating via its own path (and the static token exercising the RFC 0020${env:…}secret-hygiene rule); static-only / oidc-only / open-mode arms cited to their standing testsRemaining before the status flip: the Dex testcontainers arm (.7).
Full gate: 978 / 0, clippy
-D warnings, rustdoc, fmt, cargo-deny.Invariants
§3.7: the OIDC binding flows through the same tenant checks on every surface — no new path. H6: nothing OIDC-specific leaks through the query DSL surface; rejections keep their RFC 0026 shapes (401/400/403, undifferentiated bodies).
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes