Skip to content

fix(auth): select OIDC signing key by kid - #1415

Open
seonghobae wants to merge 5 commits into
developfrom
fix/oidc-kid-selection
Open

fix(auth): select OIDC signing key by kid#1415
seonghobae wants to merge 5 commits into
developfrom
fix/oidc-kid-selection

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Summary

  • select exactly one cached OIDC signing key by the unverified kid header before signature verification
  • fail closed for unknown or duplicate key ids and signature failures
  • verify that a mismatched kid never invokes JWT decoding, even with decoy cached keys

Verification

  • uv run --project backend --group dev pytest -q backend/tests/test_auth_real.py
  • ruff check backend/api/auth.py backend/tests/test_auth_real.py

This is intentionally isolated from unrelated PR #1355: Strix reported the unchanged auth surface while its provider fallback later failed on NVIDIA NIM rate limiting and GitHub Models brownout.

Summary by CodeRabbit

  • Bug Fixes
    • Improved OIDC session validation by matching tokens to their specific signing key before verification.
    • Rejects sessions when the signing key is missing or ambiguous.
    • Prevents verification attempts with unrelated cached keys, strengthening authentication security.

Open in Devin Review

@coderabbitai

coderabbitai Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@seonghobae, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 10 minutes

Limit 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.

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 4bc0cfcb-f96d-432f-81ca-870f653931ab

📥 Commits

Reviewing files that changed from the base of the PR and between e22abc8 and e0a1f16.

📒 Files selected for processing (2)
  • backend/api/auth.py
  • backend/tests/test_auth_real.py

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: aeedb834-a329-4f24-a28d-1b408175fd7a

📥 Commits

Reviewing files that changed from the base of the PR and between c9bfba2 and e22abc8.

📒 Files selected for processing (2)
  • backend/api/auth.py
  • backend/tests/test_auth_real.py

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

OIDC session decoding now requires one cached signing key with a matching token kid. Unknown or duplicate key IDs are rejected before JWT decoding. The authentication test verifies that decoding is skipped for an unknown key ID.

Changes

OIDC key verification

Layer / File(s) Summary
Select the signing key before decoding
backend/api/auth.py, backend/tests/test_auth_real.py
The verifier selects exactly one cached key that matches the token kid before JWT decoding. The test adds a decoy key and confirms that decoding is skipped for an unknown key ID.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: selecting the OIDC signing key by its kid.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/oidc-kid-selection

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Please perform an independent review for exact current head 3448274. Review only the current diff and current-head checks; do not dismiss reviews, bypass branch protection, update the base, or merge.

@seonghobae
seonghobae enabled auto-merge (squash) August 19, 2026 13:59
@seonghobae

seonghobae commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

Current-head review request for PR #1415.

HEAD: 3448274

The OIDC verifier now selects exactly one cached signing key by the unverified kid before signature verification, rejects unknown/ambiguous key ids without calling decode, and preserves the RS256/issuer/audience/critical-header checks. Focused auth tests: 90 passed. Current hosted source/security checks are terminal and passing; the metadata gate is stale review state.

Review this exact HEAD against current checks and active ruleset. Do not reuse approvals or failures from an earlier SHA; provide concrete findings with path:line evidence, regression verification, and a merge recommendation.

@opencode-agent opencode-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 success with required evidence or explicit no-source not-applicable evidence.

  • Regression test: Keep the approval branch checking needs.coverage-evidence.result == success before 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 head 3448274b080cffb4c59e67f76f128f9d61c77e89.

  • Head SHA: 3448274b080cffb4c59e67f76f128f9d61c77e89

  • Workflow run: 32255885315

  • 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["Backend (2 files)"]
  S1 --> I1["API and service runtime"]
  I1 --> R1["Review risk: Backend (2 files)"]
  R1 --> V1["backend tests"]
Loading

@opencode-agent

Copy link
Copy Markdown
Contributor

OpenCode Review Overview

  • Head SHA: 3448274b080cffb4c59e67f76f128f9d61c77e89
  • Workflow run: 32255885315
  • Workflow attempt: 1
  • Gate result: REQUEST_CHANGES (approval step)

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 success with required evidence or explicit no-source not-applicable evidence.

  • Regression test: Keep the approval branch checking needs.coverage-evidence.result == success before 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 head 3448274b080cffb4c59e67f76f128f9d61c77e89.

  • Head SHA: 3448274b080cffb4c59e67f76f128f9d61c77e89

  • Workflow run: 32255885315

  • 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["Backend (2 files)"]
  S1 --> I1["API and service runtime"]
  I1 --> R1["Review risk: Backend (2 files)"]
  R1 --> V1["backend tests"]
Loading

@github-actions

github-actions Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

PR governance metadata gate update for 7ac160d70318f7e6da853a40c858b58ed68d659b: no current blocking failures remain.

PR governance metadata gate is waiting on current-head requirements; see the latest check for pending reasons.

@opencode-agent
opencode-agent Bot disabled auto-merge August 19, 2026 22:51
@seonghobae
seonghobae enabled auto-merge (squash) August 20, 2026 09:08
@opencode-agent
opencode-agent Bot disabled auto-merge August 20, 2026 10:30
@seonghobae
seonghobae enabled auto-merge (squash) August 20, 2026 12:09
coderabbitai[bot]
coderabbitai Bot previously approved these changes Aug 20, 2026
@seonghobae

Copy link
Copy Markdown
Contributor Author

Revalidated current head e22abc8900eee241c0a4f1d3b23cc1d25cf4b22c; the stale OpenCode coverage finding referenced prior head 3448274b and is not current-head evidence. Current OIDC KID selection behavior passes the focused auth/OIDC contract tests (18 passed, 75 deselected). No additional source change was necessary; current-head review and required Checks were requested again.

@seonghobae

seonghobae commented Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Current head ada12ff295e379189f784010dca2d8bbabcd2f4a adds a shared trust-boundary fix in _reject_signed_session_admin_payload: surrounding whitespace is rejected before role normalization can convert a non-admin-looking claim into system_admin, platform_admin, tenant_admin, or organization_admin. HMAC and OIDC regression cases are covered; local evidence is 1794 passed, 32 skipped, Ruff clean, compileall clean. Auto-merge remains protected and scheduler validation is requested for this exact head.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Open in Devin Review

@seonghobae
seonghobae force-pushed the fix/oidc-kid-selection branch from ada12ff to e0a1f16 Compare August 21, 2026 02:09
@seonghobae
seonghobae dismissed opencode-agent[bot]’s stale review August 22, 2026 07:43

Stale review: cited a coverage-evidence/required-check failure on an earlier commit; current head has been verified (gh pr checks) to pass coverage-evidence and all other non-metadata-gate required checks, with no current-head review from this reviewer. Dismissing as superseded per AGENTS.md stale-review guidance.

@opencode-agent opencode-agent Bot added area: auth Authentication, authorization, identity, or tenant isolation priority: medium Normal-priority or P2 work status: needs-review Open pull request requiring current-head review or checks type: bug Defect or incorrect behavior labels Aug 22, 2026
@coderabbitai

coderabbitai Bot commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Caution

Review failed

An 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai

coderabbitai Bot commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

Next included review available in 59 minutes.

View limit details

Limit 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.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: f57453bd-f9b3-45e0-8655-1c81d5c85e03

📥 Commits

Reviewing files that changed from the base of the PR and between e22abc8 and 7ac160d.

📒 Files selected for processing (2)
  • backend/api/auth.py
  • backend/tests/test_auth_real.py

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Exact-head maintenance evidence

  • Repository: ContextualWisdomLab/naruon
  • Pull request: #1415
  • Exact head SHA: 994c6d40bb8a5a1de82e2f137300ea620bcdf933
  • Exact base SHA: e5e99b4e3bb081b92c602358878856536030e2ca
  • Source review: OIDC RS256 verification now selects exactly one cached JWKS key by the token header kid before signature verification; unknown, duplicate, or missing key identities fail closed. The shared role boundary also rejects whitespace-padded administrator claims.
  • Verification: uv run pytest -q backend/tests/test_auth_real.py (98 passed); targeted Ruff; Python compile; git diff --check.
  • Review disposition: current source and security tests cover the exact key-selection and padded-admin-role boundaries; no unresolved current substantive thread was found.
  • Hosted state: required Checks remain the authority; protected auto-merge is enabled, but no hosted pass or merge is claimed.
  • Decision: WAIT_AND_REMEDIATE until exact-head required Checks and lawful review evidence complete. No bypass, direct push, or force push.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: auth Authentication, authorization, identity, or tenant isolation priority: medium Normal-priority or P2 work status: needs-review Open pull request requiring current-head review or checks type: bug Defect or incorrect behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant