- Notifications
You must be signed in to change notification settings - Fork911
chore: do not refresh tokens that have already failed refreshing#15608
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Once a token fails a refresh, discard the refresh token, and nevertry again.
| var oauthErr *oauth2.RetrieveError | ||
| if errors.As(err, &oauthErr) { | ||
| if oauthErr.Response.StatusCode != 0 { | ||
| status = oauthErr.Response.StatusCode | ||
| } | ||
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Uh oh!
There was an error while loading.Please reload this page.
| // | ||
| // 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." |
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.
thisisfine.jpg
johnstcn left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Tested out on personal deployment after cherry-picking on top of 2.17.2, appears to work fine for me
78f9f43 intomainUh oh!
There was an error while loading.Please reload this page.
…with dbcrypt (#15721)#15608 introduced a buggy behaviourwith dbcrypt enabled.When clearing an oauth refresh token, we had been setting the value tothe empty string.The database encryption package considers decrypting an empty string tobe an error, as an empty encrypted string value will still have a nonceassociated with it and thus not actually be empty when stored at rest.Instead of 'deleting' the refresh token, 'update' it to be the emptystring.This plays nicely with dbcrypt.It also adds a 'utility test' in the dbcrypt package to help encrypt avalue. This was useful when manually fixing users affected by this bugon our dogfood instance.
…with dbcrypt (#15721)#15608 introduced a buggy behaviourwith dbcrypt enabled.When clearing an oauth refresh token, we had been setting the value tothe empty string.The database encryption package considers decrypting an empty string tobe an error, as an empty encrypted string value will still have a nonceassociated with it and thus not actually be empty when stored at rest.Instead of 'deleting' the refresh token, 'update' it to be the emptystring.This plays nicely with dbcrypt.It also adds a 'utility test' in the dbcrypt package to help encrypt avalue. This was useful when manually fixing users affected by this bugon our dogfood instance.(cherry picked from commite744cde)
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.