chore: do not refresh tokens that have already failed refreshing #15608
Conversation
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
Once a token fails a refresh, discard the refresh token, and never try again.
Emyrk
commented
Nov 20, 2024
Comment on lines
+1511
to
+1528
| var oauthErr *oauth2.RetrieveError | ||
| if errors.As(err, &oauthErr) { | ||
| if oauthErr.Response.StatusCode != 0 { | ||
| status = oauthErr.Response.StatusCode | ||
| } | ||
|
|
8000
||
| rw.Header().Set("Content-Type", "application/x-www-form-urlencoded; charset=utf-8") | ||
| form := url.Values{ | ||
| "error": {oauthErr.ErrorCode}, | ||
| "error_description": {oauthErr.ErrorDescription}, | ||
| "error_uri": {oauthErr.ErrorURI}, | ||
| } | ||
| rw.WriteHeader(status) | ||
| _, _ = rw.Write([]byte(form.Encode())) | ||
| return | ||
| } | ||
|
|
||
| http.Error(rw, err.Error(), status) |
There was a problem hiding this comment.
Added this to the fake IDP to construct proper oauth errors. This allows me to send a status code 200 with an oauth error. This is how github does it, which feels kinda strange, but wanted to make sure we support that behavior.
johnstcn
reviewed
Nov 20, 2024
johnstcn
reviewed
Nov 20, 2024
| // | ||
| 8000 | // Notes: Provider responses are not uniform. Here are some examples: | |
| // Github | ||
| // - Returns a 200 with Code "bad_refresh_token" and Description "The refresh token passed is incorrect or expired." |
johnstcn
approved these changes
Nov 20, 2024
coadler
pushed a commit
that referenced
this pull request
Dec 3, 2024
…with dbcrypt (#15721) #15608 introduced a buggy behaviour with dbcrypt enabled. When clearing an oauth refresh token, we had been setting the value to the empty string. The database encryption package considers decrypting an empty string to be an error, as an empty encrypted string value will still have a nonce associated with it and thus not actually be empty when stored at rest. Instead of 'deleting' the refresh token, 'update' it to be the empty string. This plays nicely with dbcrypt. It also adds a 'utility test' in the dbcrypt package to help encrypt a value. This was useful when manually fixing users affected by this bug on our dogfood instance.
stirby
pushed a commit
that referenced
this pull request
Dec 3, 2024
…with dbcrypt (#15721) #15608 introduced a buggy behaviour with dbcrypt enabled. When clearing an oauth refresh token, we had been setting the value to the empty string. The database encryption package considers decrypting an empty string to be an error, as an empty encrypted string value will still have a nonce associated with it and thus not actually be empty when stored at rest. Instead of 'deleting' the refresh token, 'update' it to be the empty string. This plays nicely with dbcrypt. It also adds a 'utility test' in the dbcrypt package to help encrypt a value. This was useful when manually fixing users affected by this bug on our dogfood instance. (cherry picked from commit e744cde)
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.
For #14982
What this does
Once a token refresh fails, we remove the
oauth_refresh_tokenfrom the database. This will prevent the token from hitting the IDP for subsequent refresh attempts.Without this change, a bad script can cause a failing token to hit a remote IDP repeatedly with each
gitoperation. With this change, after the first hit, subsequent hits will fail locally, and never contact the IDP.The solution in both cases is to authenticate the external auth link. So the resolution is the same as before.
Note
When thinking about this originally, I added a column to cache the refresh error. That way I could persist the original error on subsequent calls. This method requires 2 columns, one for the error, the other for the
(access_token, refresh_token)pair.It adds more complexity, and in the end, the errors are all very generic "Refresh token is invalid" type errors. Which is no more actionable then our error
token expired, refreshing is either disabled or refreshing failed and will not be retried.So this solution requires no db schema changes, and prevents refresh token spam.