fix(auth): require issued-at in Keyverse OIDC sessions - #1321
fix(auth): require issued-at in Keyverse OIDC sessions#1321seonghobae wants to merge 22 commits into
Conversation
|
@opencode-agent review\n\nPlease review exact head 8db9575 for the Keyverse OIDC trust-boundary change, including the required iat claim and deny-first authorization behavior. |
📝 WalkthroughWalkthroughOIDC authentication now requires verified ChangesAuthentication hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🔵 Low · up to The change strengthens OIDC session validation and HTTP invalid-session throttling without a demonstrated runtime regression; one documentation statement should be corrected to describe both authentication modes. The PR is mergeable with explicit owner awareness and follow-up on that bounded documentation issue. Sequence Diagram(s)sequenceDiagram
participant HTTPClient
participant get_auth_context
participant build_auth_context
participant verify_signed_session
HTTPClient->>get_auth_context: Send bearer authorization and ASGI Request
get_auth_context->>build_auth_context: Pass token and server-observed peer scope
build_auth_context->>verify_signed_session: Verify token with exact-token and peer buckets
verify_signed_session-->>build_auth_context: Return authenticated context or failure
build_auth_context-->>get_auth_context: Return AuthContext or authentication error
Possibly related issues
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 (2)
ARCHITECTURE.md (1)
257-259: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify configured-audience matching.
The text says “exact configured issuer/client audience.”
backend/api/auth.py:468-508checks exact issuer equality but acceptssettings.OIDC_CLIENT_IDas a member of_session_audience_claim(payload). State the audience-membership rule explicitly, or change the implementation and tests if single-value equality is required.🤖 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 `@ARCHITECTURE.md` around lines 257 - 259, Clarify the OIDC audience requirement in the architecture text to match the implementation: issuer must match exactly, while the configured OIDC client ID must be present in the session audience claim, including multi-valued audiences. Update the wording near the signed session envelope description without changing implementation behavior.docs/operations/auth-key-management.md (1)
124-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd citations for the OIDC trust-boundary claims.
This section defines security requirements for issuer, audience,
iat,exp, and critical headers, but provides no supporting citation or summary. Add the relevant OIDC/JWT standards and applicable security literature. A formal OIDC security analysis discusses related validation requirements and attack variants. (arxiv.org)As per coding guidelines: “Substantive feature or process PRs should cite relevant academic literature and commit PDFs when redistribution is permitted; otherwise provide citations, links, and summaries.”
🤖 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 `@docs/operations/auth-key-management.md` around lines 124 - 130, Add citations and brief summaries for the OIDC/JWT trust-boundary requirements described in this section, covering issuer, audience, NumericDate claims, critical headers, and related validation attacks. Reference the relevant OIDC and JWT standards plus the cited formal OIDC security analysis, and include links or committed PDFs according to the repository’s citation guidelines.Source: Coding guidelines
🤖 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 `@ARCHITECTURE.md`:
- Around line 257-259: Clarify the OIDC audience requirement in the architecture
text to match the implementation: issuer must match exactly, while the
configured OIDC client ID must be present in the session audience claim,
including multi-valued audiences. Update the wording near the signed session
envelope description without changing implementation behavior.
In `@docs/operations/auth-key-management.md`:
- Around line 124-130: Add citations and brief summaries for the OIDC/JWT
trust-boundary requirements described in this section, covering issuer,
audience, NumericDate claims, critical headers, and related validation attacks.
Reference the relevant OIDC and JWT standards plus the cited formal OIDC
security analysis, and include links or committed PDFs according to the
repository’s citation guidelines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 55acf52b-6de0-4682-a6c2-21709134f523
📒 Files selected for processing (5)
ARCHITECTURE.mdCHANGELOG.mdbackend/api/auth.pybackend/tests/test_auth_real.pydocs/operations/auth-key-management.md
|
PR governance metadata gate is not ready for
|
|
@opencode-agent review Please review exact current head |
|
@coderabbitai review Please review exact current head |
|
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
This branch contains valuable OIDC |
|
@coderabbitai review |
|
Dismissed as superseded by the current branch: the requested changes targeted backend/services/text_safety.py, which is no longer part of this PR's exact current diff and the associated inline thread is resolved/outdated. Current-head review evidence remains required independently.
|
@coderabbitai review |
|
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current head57f42180dc29b5bec3578eed06a625a6fb9793b3. -
Head SHA:
57f42180dc29b5bec3578eed06a625a6fb9793b3 -
Workflow run: 32099577912
-
Workflow attempt: 1
Coverage evidence
Coverage evidence job did not run or did not publish coverage evidence.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file: ARCHITECTURE.md"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file: ARCHITECTURE.md"]
R1 --> V1["required checks"]
Evidence --> S2["Backend (5 files)"]
S2 --> I2["API and service runtime"]
I2 --> R2["Review risk: Backend (5 files)"]
R2 --> V2["backend tests"]
Evidence --> S3["Docs (2 files)"]
S3 --> I3["operator or user guidance"]
I3 --> R3["Review risk: Docs (2 files)"]
R3 --> V3["docs review"]
OpenCode Review Overview
Pull request overviewOpenCode cannot approve yet because required coverage evidence did not pass. Review outcome1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
Coverage evidenceCoverage evidence job did not run or did not publish coverage evidence. Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file: ARCHITECTURE.md"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file: ARCHITECTURE.md"]
R1 --> V1["required checks"]
Evidence --> S2["Backend (5 files)"]
S2 --> I2["API and service runtime"]
I2 --> R2["Review risk: Backend (5 files)"]
R2 --> V2["backend tests"]
Evidence --> S3["Docs (2 files)"]
S3 --> I3["operator or user guidance"]
I3 --> R3["Review risk: Docs (2 files)"]
R3 --> V3["docs review"]
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current head091e9f85c561b2d43e050484dcfab4fa1807fd41. -
Head SHA:
091e9f85c561b2d43e050484dcfab4fa1807fd41 -
Workflow run: 32420955159
-
Workflow attempt: 1
Coverage evidence
Coverage evidence job did not run or did not publish coverage evidence.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file: ARCHITECTURE.md"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file: ARCHITECTURE.md"]
R1 --> V1["required checks"]
Evidence --> S2["Backend (5 files)"]
S2 --> I2["API and service runtime"]
I2 --> R2["Review risk: Backend (5 files)"]
R2 --> V2["backend tests"]
Evidence --> S3["Docs (2 files)"]
S3 --> I3["operator or user guidance"]
I3 --> R3["Review risk: Docs (2 files)"]
R3 --> V3["docs review"]
Verified: coverage-evidence and other non-metadata-gate required checks on the current head now report pass (gh pr checks). The coverage-evidence failure this review cited has since cleared on rerun. Dismissing as superseded.
opencode-agent
Bot
added
area: auth
|
Caution Review failedAn error occurred during the review process. Please try again later. 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 |
|
Warning Review limit reachedNext included review available in 59 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
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 |
1 participant
Summary
This current-base security lane combines three bounded, independently testable hardening changes:
iatNumericDate alongsideiss,aud, andexpbefore Naruon accepts OIDC identity claims._fetch_messages()opens the TLS connection so alternate call paths cannot bypass the existing outbound destination policy.The HTTP aggregate scope is derived only from the ASGI
request.client.hostboundary. Application code deliberately ignoresForwardedandX-Forwarded-For, so caller-controlled forwarding headers cannot create new throttle identities. The peer budget is looser than the exact-token budget to reduce collateral lockout behind NAT/reverse proxies, remains bounded by the existing expiry/capacity store, and is not reset by a successful bearer token. Direct non-HTTPbuild_auth_context()calls retain the exact-token-only contract.Test-first evidence
The RED regression for #1348 established that distinct invalid bearer tokens from one observed HTTP peer could previously reach JWT decoding independently. Production implementation then added the peer-scoped budget and focused regressions for:
Forwarded/X-Forwarded-Forvalues not changing the application scope;The IMAP hardening was also introduced test-first: commit
ffb2363192eb6004dea6154faea3df34bc767172adds the regression that rejects an explicitly supplied unvalidated fetch destination, and current head2790a7edff5f5ed29a6a7aedcd398bc0d5ef7c06applies the narrow final-use validation fix.OIDC trust boundary
audclaim, including multi-valued audiences;iatandexpNumericDate lifetime checks;The standards and abuse-control rationale are recorded in
docs/doctoring/http-session-throttling.mdanddocs/operations/auth-key-management.md, including RFC 7519, OpenID Connect Core 1.0, NIST SP 800-63B-4, and the formal OpenID Connect security analysis by Fett, Küsters, and Schmitz (2017), with APA 7 references.Current-base scope
develop@bc98789521d21271e84789888413c182aa111b4d.2790a7edff5f5ed29a6a7aedcd398bc0d5ef7c06.develop:ARCHITECTURE.mdbackend/api/auth.pybackend/services/imap_worker.pybackend/tests/test_auth_http_rate_limit.pybackend/tests/test_auth_real.pybackend/tests/test_imap_worker.pydocs/doctoring/http-session-throttling.mddocs/operations/auth-key-management.mdExact-head verification
For exact head
2790a7edff5f5ed29a6a7aedcd398bc0d5ef7c06, applicable required and quality/security checks have completed successfully, includingopencode-review,backend (Python 3.14),frontend,dependency-review,trivy-fs,osv-scan,coverage-source-tree,coverage-evidence,security,scorecard,strix,noema-review, CodeQL/Semgrep analysis, and container validation. Conditional deployment/publish/cancellation jobs that are skipped are not treated as passing evidence. The separate advisory GitHub Advanced SecurityTrivycode-scanning result is neutral because a base-branch configuration is not present on this PR head; it is not substituted for the required successfultrivy-fscontext.Merge gate
Do not merge until the unchanged exact current head satisfies every live protected-branch rule and receives the required qualifying independent approval after the last push. The live repository currently has only
@seonghobaeas collaborator/CODEOWNER, so no qualifying independent reviewer path exists yet; #1371 tracks that governance prerequisite. No self-approval, bypass, or ruleset weakening is acceptable.Related: #1348, #1371, and ContextualWisdomLab/keyverse ADR-0008.