-
Notifications
You must be signed in to change notification settings - Fork 386
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add hashed refresh tokens #663
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm so we're just hashing the token in the refresh_tokens
table? what happens when the user tries to use the refresh token in /token?grant_type=refresh_token
?
will we hash the refresh_token sent by the user to check if it matches the hashed_token
in the db?
how do we also determine whether the refresh_token
is an existing one (no corresponding hash in the db) or a newly issued one (has a corresponding hash in the db)?
It's hashed and then looked up in the database. func FindUserWithRefreshToken(tx *storage.Connection, token string) (*User, *RefreshToken, error) {
hashedToken := crypto.HashSHA224Base64(token)
refreshToken := &RefreshToken{}
if err := tx.Where("token = ? OR token = ?", hashedToken, token).First(refreshToken); err != nil { For backward compatibility the non-hashed version is also looked up. Note that since these strings are random and 128 bits or more, it is impossible to have the same value appearing in hashed or unhashed.
(Yes.)
Well there is an interesting property. If you know the original value of the token (i.e. what the user has) and you successfully find it in the database, then you have access to both the hashed and original values. Thus, it does not matter if a token is one of the old ones or a new one. If you really needed to distinguish, you can always compare |
6dadf79
to
091725b
Compare
if err != nil { | ||
return internalServerError("Error validating reuse interval").WithInternalError(err) | ||
} | ||
if time.Now().Before(refreshTokenReuseWindow) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully, this logic can be tested with a scenario I've encountered on the client side with supabase-js v2.
More details can be found here: https://github.com/supabase/supabase/issues/8959
Here's the gist, there's a multiTab situation, where the user's session is destroyed because there's a refresh timer conflict. Hopefully, the details provided give a better understanding of the problem
@hf Couldn't an attacker just send the hashed token (from DB) as the non-hashed token (in Request) and mint new tokens with it? |
Yes, that is a good catch (tho only valid in the backward compatibility period)... it's an easy fix though if the hashed tokens are stored with some prefix in the database. For example,
Since it's practically impossible to have an already existing token with the value of I'll amend the code to include the prefix. Obviously this only works if any Fixed and added test cases for it. |
091725b
to
d28a53d
Compare
d28a53d
to
d1605c0
Compare
Old PR. |
Today refresh tokens are stored raw in the database. Should the database be stolen, leaked or misused by an external or internal party, it would be nearly trivial to issue a valid access token for any user.
This PR stores the refresh tokens (the
token
column inrefresh_tokens
) as a SHA256/224 hash. It is thus impossible to impersonate a user using this value, but it is possible to verify that the user has a valid token.For backward compatibility purposes, a token lookup occurs using both the hashed and raw value of the token; but all new tokens will be using the hashed and secure form. To prevent hashed tokens being used instead of real tokens, they are stored with a
H:
prefix in the database, and backward-compatible lookups are done only after stripping the same prefix before hashing.Furthermore, the old behavior with the reuse window of refresh tokens is now cleaned up. A new refresh token is generated if a revoked refresh token is issued within the revocation period. (This only happens if the browser / client has not synced up the refresh tokens they use for requests -- in a multi-tab or concurrent request environment.)