feat(auth): rfc 0029 green (verifier) — local OIDC JWT verification - #423
Merged
Conversation
|
Warning Review limit reached
Next review available in: 42 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)
📝 WalkthroughWalkthroughThis PR adds a feature-gated OIDC JWT verifier in ourios-core, implements discovery, JWKS caching and refresh, and claim-based identity resolution. It also introduces ChangesOIDC Verification and Clock Skew Configuration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant OidcVerifier
participant IdentityProvider
Client->>OidcVerifier: verify(token)
OidcVerifier->>OidcVerifier: parse header, check alg/kid
alt kid unseen in cache
OidcVerifier->>IdentityProvider: refresh_keys (fetch JWKS)
IdentityProvider-->>OidcVerifier: updated JWKS
end
OidcVerifier->>OidcVerifier: try_decode (validate signature/claims)
OidcVerifier->>OidcVerifier: resolve_identity (name, tenants)
OidcVerifier-->>Client: VerifiedIdentity or None
sequenceDiagram
participant Server
participant OidcVerifier
participant IdentityProvider
Server->>OidcVerifier: discover(config)
OidcVerifier->>IdentityProvider: GET /.well-known/openid-configuration
IdentityProvider-->>OidcVerifier: discovery document
OidcVerifier->>IdentityProvider: GET jwks_uri
IdentityProvider-->>OidcVerifier: JWKS
OidcVerifier->>OidcVerifier: validate issuer, cache keys
OidcVerifier-->>Server: OidcVerifier instance or DiscoveryError
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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds the RFC 0029 “verifier slice” to ourios-core behind the oidc feature, enabling local OIDC JWT verification via cached JWKS (discovery at construction; optional refresh on rotation), and threads a configurable clock_skew_secs from server config into the validated core auth config.
Changes:
- Introduce
ourios_core::auth::oidcwithOidcVerifier(discovery + JWKS cache + local JWT verification) and RFC-matrix tests. - Add
auth.oidc.clock_skew_secsplumbing from server config →OidcSpec→ validatedOidcConfig. - Add feature-gated dependencies in
ourios-corefor OIDC verification (jsonwebtoken/reqwest/tokio/serde) plus test-only deps for the fixture issuer.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/ourios-server/src/config/file.rs | Adds auth.oidc.clock_skew_secs to file config and env-substitution. |
| crates/ourios-server/src/auth.rs | Passes clock_skew_secs into OidcSpec when building core auth config. |
| crates/ourios-core/src/auth/oidc.rs | Implements OIDC discovery + JWKS caching + local JWT verification, with RFC tests. |
| crates/ourios-core/src/auth/mod.rs | Extends OidcSpec/OidcConfig with clock skew and exposes the oidc module behind feature flag. |
| crates/ourios-core/Cargo.toml | Adds feature-gated OIDC deps and test deps for local issuer fixture. |
| Cargo.lock | Locks new transitive dependencies from the added crates/features. |
Comments suppressed due to low confidence (1)
crates/ourios-core/src/auth/mod.rs:200
- The error string for an invalid
auth.oidc.clock_skew_secsvalue contains a large run of spaces (likely from a wrapped literal). This will surface as a confusing message in startup validation output.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The core `auth::oidc` module behind the `oidc` feature (RFC 0029 §3.2): - `OidcVerifier::discover` fetches the issuer's discovery document + JWKS once at construction (issuer equality enforced against the discovery `issuer` field); `verify` is local thereafter — signature against the cached keys, `iss`/`aud`/`exp`/`nbf` with the configured clock skew, claims resolved onto the RFC 0026 `(name, tenants)` binding via `tenant_claim`/`name_claim`. - JWKS rotation: an unseen `kid` triggers one throttled refetch (REFRESH_MIN_INTERVAL); keys outside the §3.2 asymmetric allow-list are skipped at cache time, each decode pins its cached key's algorithm, so `alg: none` and HMAC — including the public-key-as-HMAC-secret downgrade — never verify. - Every rejection is the same undifferentiated `None` (§3.2 no-oracle); no token, claims, or key material reaches any error/Debug surface. Two bugs caught by the first complete test run: - jsonwebtoken 10 only ships a CryptoProvider behind the umbrella `rust_crypto` feature — the granular `rsa`+`p256` selection compiled key types but panicked on every decode. The umbrella feature is now used; the allow-list stays enforced by the verifier (cache filter + per-key pin + per-decode Validation), proven by the matrix's HMAC downgrade arm against compiled-in HMAC. - `Validation.validate_nbf` defaults to off, so a token with `nbf` 300 s in the future verified. Enabled (validates when present; required set stays `exp`/`iss`/`aud`). Evidence: RFC0029.2 verification matrix (happy path + expired / premature-nbf / wrong-aud / wrong-iss / corrupted-signature / alg-none / HMAC-downgrade / non-JWT arms), RFC0029.6 rotation (new-kid refetch + removed-kid rejection), discovery issuer-mismatch/unusable-JWKS rejection — all against a local fixture issuer. Full gate: 968 passed / 0 failed, clippy pedantic, rustdoc, fmt. The ingest/query bindings (tower layer, `enforcement_store()` retirement) and the Dex testcontainers arm (.7) follow as the next slices; RFC status stays `red` until they land. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jensholdgaard
force-pushed
the
rfc0029-green-verifier
branch
from
July 7, 2026 14:26
7f47e3b to
523ed73
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
crates/ourios-core/src/auth/mod.rs:200
- The
clock_skew_secsparse error message contains a long run of extra spaces ("number of seconds"), which will render oddly for users and makes the message harder to read. Collapse this to a single space.
… PS family Review round on the verifier: - try_decode now returns a three-way DecodeAttempt; only UnknownKid may trigger the throttled JWKS refetch — validation failures are terminal, so invalid tokens can no longer drive issuer traffic (pinned by a fixture-issuer fetch counter in the matrix test). - last_refresh is backdated one interval at construction so the FIRST unseen-kid miss refreshes immediately; the rotation test now runs under the real production throttle instead of injecting zero. - PS384/PS512 join the allow-list and the JWKS cache mapping — the "PS* completes the RSA family" comment is now true. - The unreachable-issuer arm uses a just-closed loopback port instead of an external hostname (no DNS/egress dependency in CI). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
crates/ourios-core/src/auth/mod.rs:200
- The
clock_skew_secsparse error message includes a large run of whitespace ("number of seconds"), which makes the startup validation error harder to read/search. Consider using a normal wrapped string literal (like the other auth errors) so the message is stable and clean.
crates/ourios-core/src/auth/mod.rs:512 clock_skew_secsis a new validated/defaulted field onOidcConfig, but the existingoidc_config_requires_its_fields_and_defaults_name_claimtest doesn’t assert the default (60s) or otherwise exercise the new behavior. Adding a small assertion helps prevent accidental changes to the default/skew wiring.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
crates/ourios-core/src/auth/mod.rs:200
- Error message for invalid auth.oidc.clock_skew_secs contains a large run of whitespace ("number of seconds"), which will render oddly and is harder to read/search. It looks unintentional and should be normalized.
crates/ourios-core/src/auth/mod.rs:201 - clock_skew_secs parsing/defaulting is newly added behavior, but the existing OIDC config tests don’t cover (1) defaulting to 60 when unset, (2) trimming behavior, or (3) rejecting invalid/non-integer/negative values. Adding a couple focused unit tests here would prevent regressions in this security-sensitive validation.
An RSA JWK published without alg (the common issuer shape) now verifies any allow-listed RSA-family algorithm (RS*/PS*) instead of silently pinning RS256 and rejecting the rest; an explicit alg still pins exactly that algorithm, and EC keys stay curve-determined. Cross-family re-typing remains impossible. Pinned by a fixture test (checked-in RSA PEM — debug-build keygen is too slow per test): RS256/384/512 verify against the alg-less JWK, an ES256 header selecting the RSA key rejects. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
crates/ourios-core/src/auth/mod.rs:200
- The error message for an invalid
auth.oidc.clock_skew_secsvalue contains a large run of extra spaces ("number of seconds"), which makes the startup/config error harder to read and looks unintentional.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
crates/ourios-core/src/auth/mod.rs:198
- The error message for an invalid
auth.oidc.clock_skew_secsvalue contains an unintended run of whitespace, which will show up in user-facing validation output.
…tted PEM A checked-in private key trips secret scanners and contradicts the crate's own fixture policy (the P-256 comment: "no committed private-key fixtures for scanners to flag"). The RSA-family fixture is now generated once per test process (OnceLock; 1024-bit — the tests exercise signature shape, not strength, so unoptimized keygen stays fast) via an explicit `rsa` dev-dependency (already in the tree through jsonwebtoken's rust_crypto backend). The unconditional dev-dep surfaces RUSTSEC-2023-0071 (Marvin) in cargo-deny's default graph; accepted in deny.toml with the usage argument: the advisory concerns private-key-operation timing, and Ourios only ever *verifies* against public JWKS keys — no RSA private-key operation exists outside throwaway test fixtures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
crates/ourios-core/src/auth/mod.rs:200
- The error message for an invalid
auth.oidc.clock_skew_secsvalue contains a large run of whitespace ("number of seconds"), which will render poorly and looks like an accidental formatting artifact. This also makes the message harder to grep/read.
… agree with its curve An empty refreshed set is an issuer glitch, not a total withdrawal — wiping the cache would fail all verification until the next refresh. An EC JWK whose explicit alg disagrees with its curve is skipped as malformed (the comment previously claimed this; now the code does it). Both pinned by tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
crates/ourios-core/src/auth/mod.rs:200
- The error message for an invalid
auth.oidc.clock_skew_secscontains a large run of extra spaces ("number of seconds"), which will surface to users and looks unintentional. Tighten the wording/spacing so the message is readable and consistent with nearby errors.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
crates/ourios-core/src/auth/oidc.rs (1)
272-286: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDrop the refresh timestamp lock before awaiting the JWKS fetch.
Line 279 keeps the
last_refreshwrite guard alive across the network call on Line 286, so a slow issuer blocks all other unknown-kidrefresh checks behind this lock. Keep the throttle update scoped, then fetch outside the lock.Proposed refactor
- let mut last = self.last_refresh.write().await; - // Re-check under the write lock: a concurrent refresher may have - // just fetched, and one fetch per interval is the whole point. - if last.elapsed() < self.refresh_min_interval { - return None; - } - *last = Instant::now(); + { + let mut last = self.last_refresh.write().await; + // Re-check under the write lock: a concurrent refresher may have + // just fetched, and one fetch per interval is the whole point. + if last.elapsed() < self.refresh_min_interval { + return None; + } + *last = Instant::now(); + } let jwks: JwkSet = fetch_json(&self.http, &self.jwks_uri).await.ok()?;🤖 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/oidc.rs` around lines 272 - 286, The refresh_keys method is holding the last_refresh write guard across the fetch_json JWKS network call, which blocks other refresh attempts while the request is in flight. Update the throttle timestamp while the lock is held, then drop the guard before awaiting fetch_json on self.http and self.jwks_uri; keep the re-check logic in refresh_keys so only one refresh per interval still proceeds.
🤖 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 `@crates/ourios-core/src/auth/oidc.rs`:
- Around line 828-831: The `config.issuer` assignment in `oidc.rs` is
immediately overwritten before being read, which causes an unused-assignment
warning. Remove the first `config.issuer = Some(issuer.clone())` assignment and
keep the later assignment using the `empty_issuer` value in the OIDC setup logic
around `serve_issuer` and `config.issuer`.
- Around line 709-735: The jwk_without_alg_accepts_its_family_only test in
OidcVerifier should also cover the RSA-PSS family, not just RS256/RS384/RS512.
Extend the existing algorithm loop to include PS256, PS384, and PS512 when
minting tokens with the alg-less RSA JWK, so the test exercises the full
documented RSA key family behavior.
- Around line 325-372: The key caching logic in cache_keys currently accepts
RSA/EC JWKs based only on shape and algorithm, so it can cache keys not meant
for JWT verification. Before calling DecodingKey::from_jwk, inspect each JWK’s
use and key_ops fields and skip any key whose declared usage is incompatible
with signature verification, such as use set to enc or key_ops missing verify.
Keep the existing algorithm filtering in place, and apply the new usage check
alongside the jwk.common / AlgorithmParameters handling in cache_keys.
---
Nitpick comments:
In `@crates/ourios-core/src/auth/oidc.rs`:
- Around line 272-286: The refresh_keys method is holding the last_refresh write
guard across the fetch_json JWKS network call, which blocks other refresh
attempts while the request is in flight. Update the throttle timestamp while the
lock is held, then drop the guard before awaiting fetch_json on self.http and
self.jwks_uri; keep the re-check logic in refresh_keys so only one refresh per
interval still proceeds.
🪄 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: a4f5be1f-0f3b-4f20-bb18-b1a6f579bb80
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
crates/ourios-core/Cargo.tomlcrates/ourios-core/src/auth/mod.rscrates/ourios-core/src/auth/oidc.rscrates/ourios-server/src/auth.rscrates/ourios-server/src/config/file.rsdeny.toml
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/ourios-server/src/auth.rs
- crates/ourios-core/Cargo.toml
- crates/ourios-server/src/config/file.rs
- crates/ourios-core/src/auth/mod.rs
…ssignment Keys declaring use: enc or key_ops without verify never cache (RFC 7517 gating, pinned by test); the alg-less family test now exercises PS256/384/512 too (fixture bumped to 2048 bits — PS512's PSS salt+hash exceeds 1024); the discovery test's overwritten issuer assignment is gone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
crates/ourios-core/src/auth/mod.rs:200
- The error message for an invalid
auth.oidc.clock_skew_secscontains a large run of extra spaces (likely an accidental formatting artifact), which makes the message harder to read and inconsistent with nearby validation errors.
…to production fetch_json streams via chunk() and rejects past the bound instead of buffering an arbitrary-size discovery/JWKS response (pinned by test); the RUSTSEC-2023-0071 note now acknowledges test-only signing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
crates/ourios-core/src/auth/mod.rs:200
- The error message for an invalid
auth.oidc.clock_skew_secscontains an unintended long run of spaces before "seconds", which makes the message look corrupted and harder to read/search for.
This was referenced Jul 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
RFC 0029 verifier slice — the core
auth::oidcmodule behind theoidcfeature: local OIDC JWT verification resolving onto the unchanged RFC 0026(name, tenants)binding. Follows the config slice (#422); the ingest/query bindings and the Dex testcontainers arm (.7) are the next slices, so the RFC staysredfor now.OidcVerifier::discoverfetches the discovery document + JWKS at construction (discoveryissuerequality enforced);verifynever leaves the process — cached-key signature check,iss/aud/exp/nbfwith configured clock skew,tenant_claim/name_claim→ the RFC 0026 binding.kid→ one throttled JWKS refetch; removed keys stop verifying.Validationgets exactly that algorithm.alg: noneand HMAC (incl. the public-key-as-HMAC-secret downgrade) never verify.None; no token/claims/key material in any error orDebugsurface.Two bugs the first complete test run caught
rsa+p256features compile key types but noCryptoProvider— every decode panicked. Switched to the umbrellarust_cryptofeature; the compiled-in HMAC stays unusable through the verifier (the matrix's downgrade arm now proves this against real compiled-in HMAC).validate_nbfdefaults off: a token withnbf300 s in the future verified — a §3.2 violation. Enabled (validates when the claim is present; required set unchanged:exp/iss/aud).§5 evidence
nbf/ wrong-aud/ wrong-iss/ corrupted-signature /alg: none/ HMAC-downgrade / non-JWT — all the sameNone, against a local fixture issuerkidrefetch verifies; removed-kidrejectsInvariants
§3.7 tenancy: verified claims resolve through the same
TenantSetmachinery as static tokens — no new tenant-scoping path. Supply chain:jsonwebtoken(RustCrypto stack) + the already-presentreqwest/rustls; no new TLS stack.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Chores