fix(auth): validate iat claim when verifying access tokens - #1807
Open
cipherprofessor wants to merge 1 commit into
Open
cipherprofessor wants to merge 1 commit into
cipherprofessor wants to merge 1 commit into
Conversation
APIKeyTokenVerifier.Verify() called jwt.ParseWithClaims without jwt.WithIssuedAt(), so the iat claim was never validated. A token correctly signed with the API secret but carrying a far-future iat, with no nbf claim, verified successfully immediately regardless of how far in the future it claimed to have been issued. First-party SDKs always set nbf (see AccessToken.ToJWT), which the parser already validates by default when present, so this only mattered for hand-rolled or third-party-minted tokens that omit nbf. Adds jwt.WithIssuedAt() alongside the existing jwt.WithExpirationRequired() (added in livekit#1706 for a related but distinct gap). Confirmed against the vendored golang-jwt/v5 v5.3.1 source that this does not require iat to be present -- verifyIssuedAt is called with required=false, so a token that omits iat entirely is completely unaffected, and existing SDK-minted and hand-rolled test tokens without an iat claim continue to verify exactly as before. A token that omits both iat and nbf remains unaffected by this check either way, since iat is still not required -- that's an intentional, narrower boundary matching this issue's own scope, not something missed. webhook/verifier.go's Receive() calls the same APIKeyTokenVerifier.Verify, so it inherits this fix with no separate change needed; confirmed there's no parallel jwt.ParseWithClaims call anywhere else in the repo. Added a regression test modeling a hand-rolled token with iat 2 hours in the future and no nbf claim, following the existing "token without exp is rejected" sibling test's style. Confirmed it fails against pre-fix code and passes after. go build, go vet, gofmt, and the full auth package test suite are all clean. One pre-existing, unrelated test failure exists in the webhook package (TestURLNotifierFilter/none, a flaky require.Eventually-style timeout) -- confirmed identical on unmodified main via stash-and-rerun. Three pre-existing gofmt violations exist elsewhere in the repo (auth/grants.go, tools/tools.go, utils/jwtutil/jwtutil.go) -- confirmed identical on unmodified main, untouched by this diff. Fixes livekit#1710
🦋 Changeset detectedLatest commit: ff09d63 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
Fixes #1710
Problem
APIKeyTokenVerifier.Verify()callsjwt.ParseWithClaimswithoutjwt.WithIssuedAt(), so theiatclaim is never validated. A token correctly signed with the API secret but carrying a far-futureiat, with nonbfclaim, verifies successfully immediately — regardless of how far in the future it claims to have been issued.First-party SDKs always set
nbf(seeAccessToken.ToJWT), which the parser already validates by default when present, so this only matters for hand-rolled or third-party-minted tokens that omitnbf.Fix
Adds
jwt.WithIssuedAt()alongside the existingjwt.WithExpirationRequired()(added in #1706 for a related but distinct gap — a missingexpclaim).Backward compatibility, verified against the actual vendored
golang-jwt/v5v5.3.1 source (not assumed):WithIssuedAt()callsverifyIssuedAtwithrequired=false, so a token that omitsiatentirely is completely unaffected — no breaking change for any minter that doesn't set it. Every existing SDK-minted and hand-rolled test token without aniatclaim continues to verify exactly as before.Known, intentional scope boundary: a token that omits both
iatandnbfremains unaffected by this check either way, sinceiatis still not required — only validated when present. That matches this issue's own scope (a token that setsiatbut omitsnbf), not something missed. Makingiatmandatory would be a materially bigger, more debatable change (RFC 7519 treatsiatas informational, unlikeexp) that could reasonably break minters that omit it — happy to discuss if that's wanted, but didn't want to bundle it into this fix unasked.webhook/verifier.go'sReceive()calls the sameAPIKeyTokenVerifier.Verify, so it inherits this fix automatically — confirmed there's no paralleljwt.ParseWithClaimscall anywhere else in the repo that would need a matching change.Testing
Added a regression test modeling a hand-rolled token with
iat2 hours in the future and nonbfclaim, following the existing"token without exp is rejected"sibling test's style and conventions. Confirmed it fails against pre-fix code and passes after (TDD red/green).go build,go vet,gofmt, and the fullauthpackage test suite are all clean. Two pre-existing, unrelated issues found while validating, both confirmed identical on unmodifiedmainvia stash-and-rerun, neither touched by this diff:TestURLNotifierFilter/nonein thewebhookpackage fails with arequire.Eventuallytimeout (looks flaky/environment-dependent).gofmtflags 3 files elsewhere in the repo (auth/grants.go,tools/tools.go,utils/jwtutil/jwtutil.go).Included a changeset per the repo's convention.