feat(auth): rfc 0029 green — dex end-to-end acceptance and status flip - #426
Conversation
…s 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>
|
Warning Review limit reached
Next review available in: 39 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 (3)
📝 WalkthroughWalkthroughAdds a Docker-based Dex OIDC end-to-end integration test replacing a stub, exercising ingest/query/MCP flows, token expiry, and audit denial telemetry. Adds a reqwest dev-dependency, a new ChangesDex OIDC End-to-End Acceptance Test
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Pull request overview
Advances RFC 0029 to green by adding a CI-gated, real-Dex end-to-end acceptance test (testcontainers) that mints client-credentials tokens and validates ingest/query/MCP behavior and “no JWT material” surfaces, plus wiring a dedicated CI job to run the ignored test.
Changes:
- Flip RFC 0029 status to
greenand document the Dex image-digest pin rationale. - Implement RFC0029.7 as an ignored, Docker-required Dex acceptance test in
ourios-serverintegration tests. - Add a required GitHub Actions job (
dex oidc (testcontainers)) to run the exact ignored test in CI, and addreqwestas a dev-dependency for minting/polling.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/rfcs/0029-oidc-bearer-layer.md | Status flip to green; adds Dex image pin note for RFC0029.7 acceptance. |
| crates/ourios-server/tests/it/rfc0029_oidc.rs | Adds RFC0029.7 real-Dex container scenario exercising mint/ingest/query/MCP/expiry/audit checks. |
| crates/ourios-server/Cargo.toml | Adds reqwest (dev-dep) for Dex readiness polling and token minting in RFC0029.7. |
| Cargo.lock | Locks new dependency graph including reqwest. |
| .github/workflows/ci.yml | Adds required dex-oidc job running the ignored RFC0029.7 test by exact name. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/ourios-server/tests/it/rfc0029_oidc.rs (1)
1038-1052: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a signal API over shelling out to
kill.Sending SIGTERM by invoking the external
killbinary works on the Linux CI runner but ties the test tokillbeing onPATH. A crate likenix::sys::signal::kill(orlibc::kill) would send the signal in-process without spawning a subprocess.♻️ Alternative using a signal crate
- let pid = child.id().expect("child pid").to_string(); - std::process::Command::new("kill") - .args(["-TERM", &pid]) - .status() - .expect("send SIGTERM"); + let pid = child.id().expect("child pid") as i32; + nix::sys::signal::kill( + nix::unistd::Pid::from_raw(pid), + nix::sys::signal::Signal::SIGTERM, + ) + .expect("send SIGTERM");🤖 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 1038 - 1052, The test is shelling out to the external kill binary to send SIGTERM, which makes it depend on PATH and a subprocess. Update the graceful shutdown logic in the rfc0029_oidc test to send the signal in-process using a signal API such as nix::sys::signal::kill or libc::kill, targeting the child PID from child.id(), and keep the existing timeout/wait/drain flow unchanged..github/workflows/ci.yml (1)
144-152: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSet
persist-credentials: falseon the new job's checkout.Static analysis flags credential persistence via this checkout step. Since this job runs
cargo test(compiling third-party dependencies/build scripts) right after checkout, leaving the GitHub token persisted in.git/configis unnecessary exposure — nothing in this job pushes back to the repo.🔒️ Proposed fix
- uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 + with: + persist-credentials: false - uses: dtolnay/rust-toolchain@3c5f7ea28cd621ae0bf5283f0e981fb97b8a7af9 # master (pinned); channel via toolchain input🤖 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 @.github/workflows/ci.yml around lines 144 - 152, The new dex-oidc job’s checkout step is leaving GitHub credentials persisted in the repo config. Update the actions/checkout step in the dex-oidc job to set persist-credentials to false so the token is not stored in .git/config; keep the rest of the job steps unchanged.Source: Linters/SAST tools
🤖 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.
Inline comments:
In `@docs/rfcs/0029-oidc-bearer-layer.md`:
- Around line 269-278: The new “Image note” blockquote in the RFC 0029 document
is accidentally absorbing the following paragraph because the note and the next
text run together without a proper separation. Update the markdown around the
Image note block so the blockquote ends cleanly and the “The RFC 0026 §5 suite
re-runs…” paragraph starts as its own paragraph, following the same blank-line
separation pattern used earlier in the document.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 144-152: The new dex-oidc job’s checkout step is leaving GitHub
credentials persisted in the repo config. Update the actions/checkout step in
the dex-oidc job to set persist-credentials to false so the token is not stored
in .git/config; keep the rest of the job steps unchanged.
In `@crates/ourios-server/tests/it/rfc0029_oidc.rs`:
- Around line 1038-1052: The test is shelling out to the external kill binary to
send SIGTERM, which makes it depend on PATH and a subprocess. Update the
graceful shutdown logic in the rfc0029_oidc test to send the signal in-process
using a signal API such as nix::sys::signal::kill or libc::kill, targeting the
child PID from child.id(), and keep the existing timeout/wait/drain flow
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d06cb55a-9251-42cf-9bad-4dec48e5f249
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.github/workflows/ci.ymlcrates/ourios-server/Cargo.tomlcrates/ourios-server/tests/it/rfc0029_oidc.rsdocs/rfcs/0029-oidc-bearer-layer.md
…ent-credentials Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rm; 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>
What
RFC 0029 →
green. The final §5 arm (.7) goes live: a real Dex container (testcontainers, CI-gated exactly likes3 integration (localstack)) mints client-credentials tokens that the served binary verifies against Dex's real JWKS — the OTel Collectoroauth2clientflow, verbatim.scope=openid profile groups; tenant list fromstaticClients[].clientCredentialsClaims.groups(tenant_claim: groups), name label from the client display name (name_claim: name)PERMISSION_DENIEDingest_deniedcarrying thename_claimvalueThe image decision (called out for review)
The client-credentials grant and
clientCredentialsClaimsare merged upstream (dexidp/dex#4691) but not in any Dex release — v2.45.1 predates both. The job pins Dexmasterby image digest (reproducible; comment + RFC §6 note record the bump path to v2.46 when it ships). The alternative was deviating the scenario to the password-grant mock connector, which would not test the machine-to-machine path the RFC §3 names as the Collector flow.Status flip
red → green: .1 (config, #420/#422) · .2/.6 (verifier, #423) · .3/.4/.5 (bindings, #424/#425) · .7 (here, run by this PR's own requireddex oidc (testcontainers)job).Full local gate: 978 / 0, clippy
-D warnings, rustdoc, fmt, cargo-deny. The.7arm itself is validated by this PR's CI (no Docker locally, by design — the localstack precedent).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation