Make refresh token rotation atomic#321
Open
damilolaedwards wants to merge 1 commit into
Open
Conversation
The refresh handler read the session under a brief lock, released it, then validated and rotated separately. Two concurrent requests presenting the same refresh token could both pass the initial lookup before either one rotated, so a single token could fork into two independently valid refresh families instead of being single use. The same gap let one request read session fields through a pointer into the shared session map while another request mutated that same object in place, a data race on the login and org fields. Copy every field needed out of the session while the lock is held instead of keeping the pointer around, and make rotation check that the presented token is still live and consume it in the same critical section as the delete and insert. A token already rotated by another request is now rejected as invalid rather than rotated again.
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
1 participant
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.
Summary
The refresh handler read the session under a brief lock, released it, then
validated and rotated separately. Two concurrent requests presenting the
same refresh token could both pass the initial lookup before either one
rotated, so a single token could fork into two independently valid refresh
families instead of being single use. That defeats org-based revocation:
removing someone from the allowed org only kills one of the two forked
chains.
The same gap let one request read session fields through a pointer into
the shared session map while another request mutated that object in
place, a data race on the login and org fields.
Copies every field needed out of the session while the lock is held
instead of keeping the pointer around, and makes rotation check that the
presented token is still live and consume it in the same critical section
as the delete and insert. A token already rotated by another request is
now rejected as invalid rather than rotated again.
Test plan
go test -race ./pkg/auth/...requests and confirms exactly one succeeds and exactly one live
session remains afterward; confirmed it fails on the old code
go build ./...,go vet ./...