From bc7323ee2aaca4cf022fd82d72ff719769e16456 Mon Sep 17 00:00:00 2001 From: Kang Ming Date: Wed, 4 May 2022 18:19:12 -0700 Subject: [PATCH] fix: add reuse interval for token refresh (#466) * add reuse interval to config * add test for refresh token reuse detection * fix: add reuse interval to refresh token grant * add tests for reuse interval * refactor token test * ignore reuse interval if revoked token is last token * add test case * refactor query to get child token * remove unnecessary check in refresh token grant * update readme * update example env file --- README.md | 9 +++++ api/token.go | 67 ++++++++++++++++++------------- api/token_test.go | 89 +++++++++++++++++++++++++++++++++++++++++ conf/configuration.go | 1 + example.env | 4 +- models/refresh_token.go | 21 ++++++++-- 6 files changed, 159 insertions(+), 32 deletions(-) diff --git a/README.md b/README.md index 322b8ef52..b0342ce7c 100644 --- a/README.md +++ b/README.md @@ -66,6 +66,15 @@ Rate limit the number of emails sent per hr on the following endpoints: `/signup Minimum password length, defaults to 6. +`GOTRUE_SECURITY_REFRESH_TOKEN_ROTATION_ENABLED` - `bool` + +If refresh token rotation is enabled, gotrue will automatically detect malicious attempts to reuse a revoked refresh token. When a malicious attempt is detected, gotrue immediately revokes all tokens that descended from the offending token. + +`GOTRUE_SECURITY_REFRESH_TOKEN_REUSE_INTERVAL` - `string` + +This setting is only applicable if `GOTRUE_SECURITY_REFRESH_TOKEN_ROTATION_ENABLED` is enabled. The reuse interval for a refresh token allows for exchanging the refresh token multiple times during the interval to support concurrency or offline issues. During the reuse interval, gotrue will not consider using a revoked token as a malicious attempt and will simply return the child refresh token. + +Only the previous revoked token can be reused. Using an old refresh token way before the current valid refresh token will trigger the reuse detection. ### API ```properties diff --git a/api/token.go b/api/token.go index edd86b837..1ce341bee 100644 --- a/api/token.go +++ b/api/token.go @@ -273,41 +273,50 @@ func (a *API) RefreshTokenGrant(ctx context.Context, w http.ResponseWriter, r *h return oauthError("invalid_grant", "Invalid Refresh Token") } - if !(config.External.Email.Enabled && config.External.Phone.Enabled) { - providers, err := models.FindProvidersByUser(a.db, user) - if err != nil { - return internalServerError(err.Error()) - } - for _, provider := range providers { - if provider == "email" && !config.External.Email.Enabled { - return badRequestError("Email logins are disabled") + var newToken *models.RefreshToken + if token.Revoked { + a.clearCookieTokens(config, w) + err = a.db.Transaction(func(tx *storage.Connection) error { + validToken, terr := models.GetValidChildToken(tx, token) + if terr != nil { + if errors.Is(terr, models.RefreshTokenNotFoundError{}) { + // revoked token has no descendants + return nil + } + return terr } - if provider == "phone" && !config.External.Phone.Enabled { - return badRequestError("Phone logins are disabled") + // check if token is the last previous revoked token + if validToken.Parent == storage.NullString(token.Token) { + refreshTokenReuseWindow := token.UpdatedAt.Add(time.Second * time.Duration(config.Security.RefreshTokenReuseInterval)) + if time.Now().Before(refreshTokenReuseWindow) { + newToken = validToken + } } + return nil + }) + if err != nil { + return internalServerError("Error validating reuse interval").WithInternalError(err) } - } - if token.Revoked { - a.clearCookieTokens(config, w) - if config.Security.RefreshTokenRotationEnabled { - // Revoke all tokens in token family - err = a.db.Transaction(func(tx *storage.Connection) error { - var terr error - if terr = models.RevokeTokenFamily(tx, token); terr != nil { - return terr + if newToken == nil { + if config.Security.RefreshTokenRotationEnabled { + // Revoke all tokens in token family + err = a.db.Transaction(func(tx *storage.Connection) error { + var terr error + if terr = models.RevokeTokenFamily(tx, token); terr != nil { + return terr + } + return nil + }) + if err != nil { + return internalServerError(err.Error()) } - return nil - }) - if err != nil { - return internalServerError(err.Error()) } + return oauthError("invalid_grant", "Invalid Refresh Token").WithInternalMessage("Possible abuse attempt: %v", r) } - return oauthError("invalid_grant", "Invalid Refresh Token").WithInternalMessage("Possible abuse attempt: %v", r) } var tokenString string - var newToken *models.RefreshToken var newTokenResponse *AccessTokenResponse err = a.db.Transaction(func(tx *storage.Connection) error { @@ -316,9 +325,11 @@ func (a *API) RefreshTokenGrant(ctx context.Context, w http.ResponseWriter, r *h return terr } - newToken, terr = models.GrantRefreshTokenSwap(tx, user, token) - if terr != nil { - return internalServerError(terr.Error()) + if newToken == nil { + newToken, terr = models.GrantRefreshTokenSwap(tx, user, token) + if terr != nil { + return internalServerError(terr.Error()) + } } tokenString, terr = generateAccessToken(user, time.Second*time.Duration(config.JWT.Exp), config.JWT.Secret) diff --git a/api/token_test.go b/api/token_test.go index eaa5dcb5f..763cb49da 100644 --- a/api/token_test.go +++ b/api/token_test.go @@ -150,6 +150,95 @@ func (ts *TokenTestSuite) TestTokenRefreshTokenGrantFailure() { assert.Equal(ts.T(), http.StatusBadRequest, w.Code) } +func (ts *TokenTestSuite) TestTokenRefreshTokenRotation() { + u, err := models.NewUser(ts.instanceID, "foo@example.com", "password", ts.Config.JWT.Aud, nil) + require.NoError(ts.T(), err, "Error creating test user model") + t := time.Now() + u.EmailConfirmedAt = &t + require.NoError(ts.T(), ts.API.db.Create(u), "Error saving foo user") + first, err := models.GrantAuthenticatedUser(ts.API.db, u) + require.NoError(ts.T(), err) + second, err := models.GrantRefreshTokenSwap(ts.API.db, u, first) + require.NoError(ts.T(), err) + third, err := models.GrantRefreshTokenSwap(ts.API.db, u, second) + require.NoError(ts.T(), err) + + cases := []struct { + desc string + refreshTokenRotationEnabled bool + reuseInterval int + refreshToken string + expectedCode int + expectedBody map[string]interface{} + }{ + { + "Valid refresh within reuse interval", + true, + 30, + second.Token, + http.StatusOK, + map[string]interface{}{ + "refresh_token": third.Token, + }, + }, + { + "Invalid refresh, first token is not the previous revoked token", + true, + 0, + first.Token, + http.StatusBadRequest, + map[string]interface{}{ + "error": "invalid_grant", + "error_description": "Invalid Refresh Token", + }, + }, + { + "Invalid refresh, revoked third token", + true, + 0, + second.Token, + http.StatusBadRequest, + map[string]interface{}{ + "error": "invalid_grant", + "error_description": "Invalid Refresh Token", + }, + }, + { + "Invalid refresh, third token revoked by previous case", + true, + 30, + third.Token, + http.StatusBadRequest, + map[string]interface{}{ + "error": "invalid_grant", + "error_description": "Invalid Refresh Token", + }, + }, + } + + for _, c := range cases { + ts.Run(c.desc, func() { + ts.Config.Security.RefreshTokenRotationEnabled = c.refreshTokenRotationEnabled + ts.Config.Security.RefreshTokenReuseInterval = c.reuseInterval + var buffer bytes.Buffer + require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{ + "refresh_token": c.refreshToken, + })) + req := httptest.NewRequest(http.MethodPost, "http://localhost/token?grant_type=refresh_token", &buffer) + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + ts.API.handler.ServeHTTP(w, req) + assert.Equal(ts.T(), c.expectedCode, w.Code) + + data := make(map[string]interface{}) + require.NoError(ts.T(), json.NewDecoder(w.Body).Decode(&data)) + for k, v := range c.expectedBody { + require.Equal(ts.T(), v, data[k]) + } + }) + } +} + func (ts *TokenTestSuite) createBannedUser() *models.User { u, err := models.NewUser(ts.instanceID, "banned@example.com", "password", ts.Config.JWT.Aud, nil) require.NoError(ts.T(), err, "Error creating test user model") diff --git a/conf/configuration.go b/conf/configuration.go index c99e840e6..b3a817e57 100644 --- a/conf/configuration.go +++ b/conf/configuration.go @@ -181,6 +181,7 @@ type CaptchaConfiguration struct { type SecurityConfiguration struct { Captcha CaptchaConfiguration `json:"captcha"` RefreshTokenRotationEnabled bool `json:"refresh_token_rotation_enabled" split_words:"true" default:"true"` + RefreshTokenReuseInterval int `json:"refresh_token_reuse_interval" split_words:"true"` UpdatePasswordRequireReauthentication bool `json:"update_password_require_reauthentication" split_words:"true"` } diff --git a/example.env b/example.env index e38ef11a4..be0e63122 100644 --- a/example.env +++ b/example.env @@ -190,7 +190,9 @@ GOTRUE_EXTERNAL_SAML_SIGNING_KEY="" # Additional Security config GOTRUE_LOG_LEVEL="debug" -GOTRUE_REFRESH_TOKEN_ROTATION_ENABLED="false" +GOTRUE_SECURITY_REFRESH_TOKEN_ROTATION_ENABLED="false" +GOTRUE_SECURITY_REFRESH_TOKEN_REUSE_INTERVAL="0" +GOTRUE_SECURITY_UPDATE_PASSWORD_REQUIRE_REAUTHENTICATION="false" GOTRUE_OPERATOR_TOKEN="unused-operator-token" GOTRUE_RATE_LIMIT_HEADER="X-Forwarded-For" GOTRUE_RATE_LIMIT_EMAIL_SENT="100" diff --git a/models/refresh_token.go b/models/refresh_token.go index 054806e4e..2af42c505 100644 --- a/models/refresh_token.go +++ b/models/refresh_token.go @@ -1,6 +1,7 @@ package models import ( + "database/sql" "time" "github.com/gobuffalo/pop/v5" @@ -57,19 +58,33 @@ func GrantRefreshTokenSwap(tx *storage.Connection, user *User, token *RefreshTok // RevokeTokenFamily revokes all refresh tokens that descended from the provided token. func RevokeTokenFamily(tx *storage.Connection, token *RefreshToken) error { + tablename := (&pop.Model{Value: RefreshToken{}}).TableName() err := tx.RawQuery(` with recursive token_family as ( - select id, user_id, token, revoked, parent from refresh_tokens where parent = ? + select id, user_id, token, revoked, parent from `+tablename+` where parent = ? union - select r.id, r.user_id, r.token, r.revoked, r.parent from `+(&pop.Model{Value: RefreshToken{}}).TableName()+` r inner join token_family t on t.token = r.parent + select r.id, r.user_id, r.token, r.revoked, r.parent from `+tablename+` r inner join token_family t on t.token = r.parent ) - update `+(&pop.Model{Value: RefreshToken{}}).TableName()+` r set revoked = true from token_family where token_family.id = r.id;`, token.Token).Exec() + update `+tablename+` r set revoked = true from token_family where token_family.id = r.id;`, token.Token).Exec() if err != nil { return err } return nil } +// GetValidChildToken returns the child token of the token provided if the child is not revoked. +func GetValidChildToken(tx *storage.Connection, token *RefreshToken) (*RefreshToken, error) { + refreshToken := &RefreshToken{} + err := tx.Q().Where("parent = ? and revoked = false", token.Token).First(refreshToken) + if err != nil { + if errors.Cause(err) == sql.ErrNoRows { + return nil, RefreshTokenNotFoundError{} + } + return nil, err + } + return refreshToken, nil +} + // Logout deletes all refresh tokens for a user. func Logout(tx *storage.Connection, instanceID uuid.UUID, id uuid.UUID) error { return tx.RawQuery("DELETE FROM "+(&pop.Model{Value: RefreshToken{}}).TableName()+" WHERE instance_id = ? AND user_id = ?", instanceID, id).Exec()