Skip to content

fix(auth): bind internal JWTs to principal secret generation - #5053

Merged
flyrain merged 20 commits into
apache:mainfrom
vigneshio:fix/jwt-bind-credentials-epoch
Sep 4, 2026
Merged

fix(auth): bind internal JWTs to principal secret generation#5053
flyrain merged 20 commits into
apache:mainfrom
vigneshio:fix/jwt-bind-credentials-epoch

Conversation

@vigneshio

@vigneshio vigneshio commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Bind internal JWT access tokens to their issuing credential generation via a polaris-cv claim (no secret material in the token). Verification honors the current and secondary generations, matching the existing dual-secret rotation grace.

Single rotations leave active tokens valid; resets (and a second rotation) invalidate prior generations. Token exchange preserves the subject token's generation. Claim-less tokens still verify until expiry for upgrade compatibility, but exchange rejects them. Tokens for deleted principals are rejected because verification requires loadable secrets. Persistence failures while loading secrets during verify return service unavailable rather than an authentication failure. No schema migration is needed.

@github-project-automation github-project-automation Bot moved this to PRs In Progress in Basic Kanban Board Jul 13, 2026
@vigneshio
vigneshio force-pushed the fix/jwt-bind-credentials-epoch branch from 5925d16 to 1a98a23 Compare July 16, 2026 13:35

@dimas-b dimas-b 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.

Thanks for your diligence, @vigneshio ! The general idea is very reasonable. Some implementation comments below.

@dimas-b
dimas-b requested review from adutra and snazy July 16, 2026 22:47
@vigneshio
vigneshio force-pushed the fix/jwt-bind-credentials-epoch branch from 1a98a23 to 083f99e Compare July 17, 2026 08:02

@dimas-b dimas-b 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.

partial review

@vigneshio
vigneshio force-pushed the fix/jwt-bind-credentials-epoch branch from bba3287 to a66743e Compare July 20, 2026 20:56
@vigneshio
vigneshio force-pushed the fix/jwt-bind-credentials-epoch branch from 0f31cd0 to e082112 Compare July 24, 2026 15:01
@vigneshio
vigneshio force-pushed the fix/jwt-bind-credentials-epoch branch from 6158068 to 5377243 Compare July 26, 2026 11:43
@jbonofre
jbonofre self-requested a review July 27, 2026 16:56
@vigneshio
vigneshio force-pushed the fix/jwt-bind-credentials-epoch branch from 5377243 to de40af7 Compare July 28, 2026 10:08
adutra
adutra previously approved these changes Jul 28, 2026
@github-project-automation github-project-automation Bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Jul 28, 2026
@flyrain
flyrain requested a review from adnanhemani July 28, 2026 22:15

@flyrain flyrain 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.

Thanks @vigneshio for the PR. I have replied in the private mailing list. I think it's valid to introduce an explicit invalidation operation, but I'm not convinced that we need to invalidate access tokens for principal credential rotating and resetting.

@vigneshio

Copy link
Copy Markdown
Contributor Author

Thanks @vigneshio for the PR. I have replied in the private mailing list. I think it's valid to introduce an explicit invalidation operation, but I'm not convinced that we need to invalidate access tokens for principal credential rotating and resetting.

Thanks for the review @flyrain replied in the private thread as well.. PTAL.. The description is now updated to match the actual semantics!!.

@vigneshio
vigneshio force-pushed the fix/jwt-bind-credentials-epoch branch from 29c2bf5 to 000eb97 Compare August 8, 2026 07:47

@dimas-b dimas-b 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.

LGTM overall 👍 Some more minor comments.

@vigneshio
vigneshio force-pushed the fix/jwt-bind-credentials-epoch branch from 000eb97 to a48b81e Compare August 12, 2026 17:32
dimas-b
dimas-b previously approved these changes Aug 12, 2026

@dimas-b dimas-b 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.

Thanks, @vigneshio !

@vigneshio
vigneshio requested review from adutra and flyrain August 19, 2026 10:59
Drop an unrelated CHANGELOG line. Map metastore load failures during
token verify to service unavailable instead of NotAuthorized/invalid_client.
Reject claim-less legacy tokens on exchange so they cannot be re-minted
onto the current credentials generation; bearer verify until expiry is
unchanged for upgrade compatibility.
Propagate ServiceUnavailableException from token verify in
InternalAuthenticationMechanism so INTERNAL and MIXED auth return 503
instead of treating metastore failures as bad credentials or falling
through to external auth. Add unit and HTTP-level coverage.
Shorten the polaris-cv CHANGELOG entry to user-facing behavior only, and
use a finite await timeout in InternalAuthenticationMechanismTest.
flyrain
flyrain previously approved these changes Sep 4, 2026

@flyingImer flyingImer left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The latest exchange-only rewrite looks sound; I don’t see a code blocker. I have two wording nits so the public contract and rolling-upgrade behavior match the implementation, plus one small stale-test cleanup. Once those are addressed, this looks ready to merge.

Comment thread CHANGELOG.md Outdated
@flyrain

flyrain commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Given we have 3 approvals on the PR, I'd propose to merge it once CI is happy.

@flyrain flyrain closed this Sep 4, 2026
@github-project-automation github-project-automation Bot moved this from Ready to merge to Done in Basic Kanban Board Sep 4, 2026
@flyrain flyrain reopened this Sep 4, 2026
@github-project-automation github-project-automation Bot moved this from Done to PRs In Progress in Basic Kanban Board Sep 4, 2026
@vigneshio

Copy link
Copy Markdown
Contributor Author

Good points @flyingImer updated the wording/tests to match the exchange-only contract...

PTAL.. @dimas-b @flyrain before we merge.

The latest exchange-only rewrite looks sound; I don’t see a code blocker. I have two wording nits so the public contract and rolling-upgrade behavior match the implementation, plus one small stale-test cleanup. Once those are addressed, this looks ready to merge.

@github-project-automation github-project-automation Bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Sep 4, 2026
@flyrain
flyrain merged commit 8775ef4 into apache:main Sep 4, 2026
24 checks passed
@github-project-automation github-project-automation Bot moved this from Ready to merge to Done in Basic Kanban Board Sep 4, 2026
@flyrain

flyrain commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Thanks for working on it, @vigneshio! Appreciated your patience and persistence! Thanks everyone for the review.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants