From 54600af00cfe2f454a49fc11a9ca5fbd8b8b808a Mon Sep 17 00:00:00 2001 From: Gusted Date: Sat, 3 Jun 2023 15:21:40 +0200 Subject: [PATCH 1/4] rework long-term authentication - The current architecture is inherently insecure, because you can construct the 'secret' cookie value with values that are available in the database. Thus provides zero protection when a database is dumped/leaked. - This patch implements a new architecture that's inspired from: [Paragonie Initiative](https://paragonie.com/blog/2015/04/secure-authentication-php-with-long-term-persistence#secure-remember-me-cookies). - Integration testing is added to ensure the new mechanism works. - Removes a setting, because it's not used anymore. Refs: https://codeberg.org/forgejo/forgejo/pulls/1562 --- models/auth/auth_token.go | 97 +++++++++++++++ models/user/user.go | 5 + modules/context/context_cookie.go | 50 ++------ modules/setting/security.go | 2 - modules/util/legacy.go | 53 --------- modules/util/legacy_test.go | 20 ---- routers/install/install.go | 13 +- routers/web/auth/auth.go | 59 +++++---- routers/web/auth/oauth.go | 3 +- routers/web/home.go | 3 +- routers/web/user/setting/account.go | 9 ++ routers/web/web.go | 2 +- services/auth/auth.go | 4 - tests/integration/auth_token_test.go | 164 ++++++++++++++++++++++++++ tests/integration/integration_test.go | 8 ++ 15 files changed, 339 insertions(+), 153 deletions(-) create mode 100644 models/auth/auth_token.go create mode 100644 tests/integration/auth_token_test.go diff --git a/models/auth/auth_token.go b/models/auth/auth_token.go new file mode 100644 index 0000000000000..9cbd21504b41e --- /dev/null +++ b/models/auth/auth_token.go @@ -0,0 +1,97 @@ +// Copyright 2023 The Gitea Authors. All rights reserved. +// Copyright 2023 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package auth + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "fmt" + "time" + + "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/modules/timeutil" + "code.gitea.io/gitea/modules/util" +) + +// AuthorizationToken represents a authorization token to a user. +type AuthorizationToken struct { + ID int64 `xorm:"pk autoincr"` + UID int64 `xorm:"INDEX"` + LookupKey string `xorm:"INDEX UNIQUE"` + HashedValidator string + Expiry timeutil.TimeStamp +} + +// TableName provides the real table name. +func (AuthorizationToken) TableName() string { + return "forgejo_auth_token" +} + +func init() { + db.RegisterModel(new(AuthorizationToken)) +} + +// IsExpired returns if the authorization token is expired. +func (authToken *AuthorizationToken) IsExpired() bool { + return authToken.Expiry.AsLocalTime().Before(time.Now()) +} + +// GenerateAuthToken generates a new authentication token for the given user. +// It returns the lookup key and validator values that should be passed to the +// user via a long-term cookie. +func GenerateAuthToken(ctx context.Context, userID int64, expiry timeutil.TimeStamp) (lookupKey, validator string, err error) { + // Request 64 random bytes. The first 32 bytes will be used for the lookupKey + // and the other 32 bytes will be used for the validator. + rBytes, err := util.CryptoRandomBytes(64) + if err != nil { + return "", "", err + } + hexEncoded := hex.EncodeToString(rBytes) + validator, lookupKey = hexEncoded[64:], hexEncoded[:64] + + _, err = db.GetEngine(ctx).Insert(&AuthorizationToken{ + UID: userID, + Expiry: expiry, + LookupKey: lookupKey, + HashedValidator: HashValidator(rBytes[32:]), + }) + return lookupKey, validator, err +} + +// FindAuthToken will find a authorization token via the lookup key. +func FindAuthToken(ctx context.Context, lookupKey string) (*AuthorizationToken, error) { + var authToken AuthorizationToken + has, err := db.GetEngine(ctx).Where("lookup_key = ?", lookupKey).Get(&authToken) + if err != nil { + return nil, err + } else if !has { + return nil, fmt.Errorf("lookup key %q: %w", lookupKey, util.ErrNotExist) + } + return &authToken, nil +} + +// DeleteAuthToken will delete the authorization token. +func DeleteAuthToken(ctx context.Context, authToken *AuthorizationToken) error { + _, err := db.DeleteByBean(ctx, authToken) + return err +} + +// DeleteAuthTokenByUser will delete all authorization tokens for the user. +func DeleteAuthTokenByUser(ctx context.Context, userID int64) error { + if userID == 0 { + return nil + } + + _, err := db.DeleteByBean(ctx, &AuthorizationToken{UID: userID}) + return err +} + +// HashValidator will return a hexified hashed version of the validator. +func HashValidator(validator []byte) string { + h := sha256.New() + h.Write(validator) + return hex.EncodeToString(h.Sum(nil)) +} diff --git a/models/user/user.go b/models/user/user.go index 60aa6b9a6f5f4..e5760e9d28dc7 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -380,6 +380,11 @@ func (u *User) SetPassword(passwd string) (err error) { return nil } + // Invalidate all authentication tokens for this user. + if err := auth.DeleteAuthTokenByUser(db.DefaultContext, u.ID); err != nil { + return err + } + if u.Salt, err = GetUserSalt(); err != nil { return err } diff --git a/modules/context/context_cookie.go b/modules/context/context_cookie.go index 9ce67a5298154..39e3218d1bafd 100644 --- a/modules/context/context_cookie.go +++ b/modules/context/context_cookie.go @@ -4,16 +4,14 @@ package context import ( - "encoding/hex" "net/http" "strings" + auth_model "code.gitea.io/gitea/models/auth" + user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/setting" - "code.gitea.io/gitea/modules/util" + "code.gitea.io/gitea/modules/timeutil" "code.gitea.io/gitea/modules/web/middleware" - - "github.com/minio/sha256-simd" - "golang.org/x/crypto/pbkdf2" ) const CookieNameFlash = "gitea_flash" @@ -46,41 +44,13 @@ func (ctx *Context) GetSiteCookie(name string) string { return middleware.GetSiteCookie(ctx.Req, name) } -// GetSuperSecureCookie returns given cookie value from request header with secret string. -func (ctx *Context) GetSuperSecureCookie(secret, name string) (string, bool) { - val := ctx.GetSiteCookie(name) - return ctx.CookieDecrypt(secret, val) -} - -// CookieDecrypt returns given value from with secret string. -func (ctx *Context) CookieDecrypt(secret, val string) (string, bool) { - if val == "" { - return "", false - } - - text, err := hex.DecodeString(val) +// SetLTACookie will generate a LTA token and add it as an cookie. +func (ctx *Context) SetLTACookie(u *user_model.User) error { + days := 86400 * setting.LogInRememberDays + lookup, validator, err := auth_model.GenerateAuthToken(ctx, u.ID, timeutil.TimeStampNow().Add(int64(days))) if err != nil { - return "", false + return err } - - key := pbkdf2.Key([]byte(secret), []byte(secret), 1000, 16, sha256.New) - text, err = util.AESGCMDecrypt(key, text) - return string(text), err == nil -} - -// SetSuperSecureCookie sets given cookie value to response header with secret string. -func (ctx *Context) SetSuperSecureCookie(secret, name, value string, maxAge int) { - text := ctx.CookieEncrypt(secret, value) - ctx.SetSiteCookie(name, text, maxAge) -} - -// CookieEncrypt encrypts a given value using the provided secret -func (ctx *Context) CookieEncrypt(secret, value string) string { - key := pbkdf2.Key([]byte(secret), []byte(secret), 1000, 16, sha256.New) - text, err := util.AESGCMEncrypt(key, []byte(value)) - if err != nil { - panic("error encrypting cookie: " + err.Error()) - } - - return hex.EncodeToString(text) + ctx.SetSiteCookie(setting.CookieRememberName, lookup+":"+validator, days) + return nil } diff --git a/modules/setting/security.go b/modules/setting/security.go index 90f614d4cd302..92caa05fad174 100644 --- a/modules/setting/security.go +++ b/modules/setting/security.go @@ -19,7 +19,6 @@ var ( SecretKey string InternalToken string // internal access token LogInRememberDays int - CookieUserName string CookieRememberName string ReverseProxyAuthUser string ReverseProxyAuthEmail string @@ -104,7 +103,6 @@ func loadSecurityFrom(rootCfg ConfigProvider) { sec := rootCfg.Section("security") InstallLock = HasInstallLock(rootCfg) LogInRememberDays = sec.Key("LOGIN_REMEMBER_DAYS").MustInt(7) - CookieUserName = sec.Key("COOKIE_USERNAME").MustString("gitea_awesome") SecretKey = loadSecret(sec, "SECRET_KEY_URI", "SECRET_KEY") if SecretKey == "" { // FIXME: https://github.com/go-gitea/gitea/issues/16832 diff --git a/modules/util/legacy.go b/modules/util/legacy.go index 2ea293a2be856..2d4de01949e3e 100644 --- a/modules/util/legacy.go +++ b/modules/util/legacy.go @@ -4,10 +4,6 @@ package util import ( - "crypto/aes" - "crypto/cipher" - "crypto/rand" - "errors" "io" "os" ) @@ -40,52 +36,3 @@ func CopyFile(src, dest string) error { } return os.Chmod(dest, si.Mode()) } - -// AESGCMEncrypt (from legacy package): encrypts plaintext with the given key using AES in GCM mode. should be replaced. -func AESGCMEncrypt(key, plaintext []byte) ([]byte, error) { - block, err := aes.NewCipher(key) - if err != nil { - return nil, err - } - - gcm, err := cipher.NewGCM(block) - if err != nil { - return nil, err - } - - nonce := make([]byte, gcm.NonceSize()) - if _, err := rand.Read(nonce); err != nil { - return nil, err - } - - ciphertext := gcm.Seal(nil, nonce, plaintext, nil) - return append(nonce, ciphertext...), nil -} - -// AESGCMDecrypt (from legacy package): decrypts ciphertext with the given key using AES in GCM mode. should be replaced. -func AESGCMDecrypt(key, ciphertext []byte) ([]byte, error) { - block, err := aes.NewCipher(key) - if err != nil { - return nil, err - } - - gcm, err := cipher.NewGCM(block) - if err != nil { - return nil, err - } - - size := gcm.NonceSize() - if len(ciphertext)-size <= 0 { - return nil, errors.New("ciphertext is empty") - } - - nonce := ciphertext[:size] - ciphertext = ciphertext[size:] - - plainText, err := gcm.Open(nil, nonce, ciphertext, nil) - if err != nil { - return nil, err - } - - return plainText, nil -} diff --git a/modules/util/legacy_test.go b/modules/util/legacy_test.go index e732094c298a1..b7991bd365c08 100644 --- a/modules/util/legacy_test.go +++ b/modules/util/legacy_test.go @@ -4,8 +4,6 @@ package util import ( - "crypto/aes" - "crypto/rand" "fmt" "os" "testing" @@ -37,21 +35,3 @@ func TestCopyFile(t *testing.T) { assert.NoError(t, err) assert.Equal(t, testContent, dstContent) } - -func TestAESGCM(t *testing.T) { - t.Parallel() - - key := make([]byte, aes.BlockSize) - _, err := rand.Read(key) - assert.NoError(t, err) - - plaintext := []byte("this will be encrypted") - - ciphertext, err := AESGCMEncrypt(key, plaintext) - assert.NoError(t, err) - - decrypted, err := AESGCMDecrypt(key, ciphertext) - assert.NoError(t, err) - - assert.Equal(t, plaintext, decrypted) -} diff --git a/routers/install/install.go b/routers/install/install.go index 185e4bf6bf6c6..1dbfafcd14a9c 100644 --- a/routers/install/install.go +++ b/routers/install/install.go @@ -547,18 +547,13 @@ func SubmitInstall(ctx *context.Context) { u, _ = user_model.GetUserByName(ctx, u.Name) } - days := 86400 * setting.LogInRememberDays - ctx.SetSiteCookie(setting.CookieUserName, u.Name, days) - - ctx.SetSuperSecureCookie(base.EncodeMD5(u.Rands+u.Passwd), - setting.CookieRememberName, u.Name, days) - - // Auto-login for admin - if err = ctx.Session.Set("uid", u.ID); err != nil { + if err := ctx.SetLTACookie(u); err != nil { ctx.RenderWithErr(ctx.Tr("install.save_config_failed", err), tplInstall, &form) return } - if err = ctx.Session.Set("uname", u.Name); err != nil { + + // Auto-login for admin + if err = ctx.Session.Set("uid", u.ID); err != nil { ctx.RenderWithErr(ctx.Tr("install.save_config_failed", err), tplInstall, &form) return } diff --git a/routers/web/auth/auth.go b/routers/web/auth/auth.go index 8017602d99026..300afaef40316 100644 --- a/routers/web/auth/auth.go +++ b/routers/web/auth/auth.go @@ -5,6 +5,8 @@ package auth import ( + "crypto/subtle" + "encoding/hex" "errors" "fmt" "net/http" @@ -49,30 +51,51 @@ func AutoSignIn(ctx *context.Context) (bool, error) { return false, nil } - uname := ctx.GetSiteCookie(setting.CookieUserName) - if len(uname) == 0 { + authCookie := ctx.GetSiteCookie(setting.CookieRememberName) + if len(authCookie) == 0 { return false, nil } isSucceed := false defer func() { if !isSucceed { - log.Trace("auto-login cookie cleared: %s", uname) - ctx.DeleteSiteCookie(setting.CookieUserName) + log.Trace("Auto login cookie is cleared: %s", authCookie) ctx.DeleteSiteCookie(setting.CookieRememberName) } }() - u, err := user_model.GetUserByName(ctx, uname) + lookupKey, validator, found := strings.Cut(authCookie, ":") + if !found { + return false, nil + } + + authToken, err := auth.FindAuthToken(ctx, lookupKey) if err != nil { - if !user_model.IsErrUserNotExist(err) { - return false, fmt.Errorf("GetUserByName: %w", err) + if errors.Is(err, util.ErrNotExist) { + return false, nil } + return false, err + } + + if authToken.IsExpired() { + err = auth.DeleteAuthToken(ctx, authToken) + return false, err + } + + rawValidator, err := hex.DecodeString(validator) + if err != nil { + return false, err + } + + if subtle.ConstantTimeCompare([]byte(authToken.HashedValidator), []byte(auth.HashValidator(rawValidator))) == 0 { return false, nil } - if val, ok := ctx.GetSuperSecureCookie( - base.EncodeMD5(u.Rands+u.Passwd), setting.CookieRememberName); !ok || val != u.Name { + u, err := user_model.GetUserByID(ctx, authToken.UID) + if err != nil { + if !user_model.IsErrUserNotExist(err) { + return false, fmt.Errorf("GetUserByName: %w", err) + } return false, nil } @@ -80,8 +103,7 @@ func AutoSignIn(ctx *context.Context) (bool, error) { if err := updateSession(ctx, nil, map[string]any{ // Set session IDs - "uid": u.ID, - "uname": u.Name, + "uid": authToken.UID, }); err != nil { return false, fmt.Errorf("unable to updateSession: %w", err) } @@ -290,10 +312,10 @@ func handleSignIn(ctx *context.Context, u *user_model.User, remember bool) { func handleSignInFull(ctx *context.Context, u *user_model.User, remember, obeyRedirect bool) string { if remember { - days := 86400 * setting.LogInRememberDays - ctx.SetSiteCookie(setting.CookieUserName, u.Name, days) - ctx.SetSuperSecureCookie(base.EncodeMD5(u.Rands+u.Passwd), - setting.CookieRememberName, u.Name, days) + if err := ctx.SetLTACookie(u); err != nil { + ctx.ServerError("GenerateAuthToken", err) + return setting.AppSubURL + "/" + } } if err := updateSession(ctx, []string{ @@ -306,8 +328,7 @@ func handleSignInFull(ctx *context.Context, u *user_model.User, remember, obeyRe "twofaRemember", "linkAccount", }, map[string]any{ - "uid": u.ID, - "uname": u.Name, + "uid": u.ID, }); err != nil { ctx.ServerError("RegenerateSession", err) return setting.AppSubURL + "/" @@ -368,7 +389,6 @@ func getUserName(gothUser *goth.User) string { func HandleSignOut(ctx *context.Context) { _ = ctx.Session.Flush() _ = ctx.Session.Destroy(ctx.Resp, ctx.Req) - ctx.DeleteSiteCookie(setting.CookieUserName) ctx.DeleteSiteCookie(setting.CookieRememberName) ctx.Csrf.DeleteCookie(ctx) middleware.DeleteRedirectToCookie(ctx.Resp) @@ -731,8 +751,7 @@ func handleAccountActivation(ctx *context.Context, user *user_model.User) { log.Trace("User activated: %s", user.Name) if err := updateSession(ctx, nil, map[string]any{ - "uid": user.ID, - "uname": user.Name, + "uid": user.ID, }); err != nil { log.Error("Unable to regenerate session for user: %-v with email: %s: %v", user, user.Email, err) ctx.ServerError("ActivateUserEmail", err) diff --git a/routers/web/auth/oauth.go b/routers/web/auth/oauth.go index ce58cbdef9994..2aba6e1224cdf 100644 --- a/routers/web/auth/oauth.go +++ b/routers/web/auth/oauth.go @@ -1118,8 +1118,7 @@ func handleOAuth2SignIn(ctx *context.Context, source *auth.Source, u *user_model // we can't sign the user in just yet. Instead, redirect them to the 2FA authentication page. if !needs2FA { if err := updateSession(ctx, nil, map[string]any{ - "uid": u.ID, - "uname": u.Name, + "uid": u.ID, }); err != nil { ctx.ServerError("updateSession", err) return diff --git a/routers/web/home.go b/routers/web/home.go index ab3fbde2c9a80..4bcc4adcbfd1b 100644 --- a/routers/web/home.go +++ b/routers/web/home.go @@ -54,8 +54,7 @@ func Home(ctx *context.Context) { } // Check auto-login. - uname := ctx.GetSiteCookie(setting.CookieUserName) - if len(uname) != 0 { + if len(ctx.GetSiteCookie(setting.CookieRememberName)) != 0 { ctx.Redirect(setting.AppSubURL + "/user/login") return } diff --git a/routers/web/user/setting/account.go b/routers/web/user/setting/account.go index 5c14f3ad4b522..f50c19a923550 100644 --- a/routers/web/user/setting/account.go +++ b/routers/web/user/setting/account.go @@ -78,6 +78,15 @@ func AccountPost(ctx *context.Context) { ctx.ServerError("UpdateUser", err) return } + + // Re-generate LTA cookie. + if len(ctx.GetSiteCookie(setting.CookieRememberName)) != 0 { + if err := ctx.SetLTACookie(ctx.Doer); err != nil { + ctx.ServerError("SetLTACookie", err) + return + } + } + log.Trace("User password updated: %s", ctx.Doer.Name) ctx.Flash.Success(ctx.Tr("settings.change_password_success")) } diff --git a/routers/web/web.go b/routers/web/web.go index 215483872670d..960d44db5e919 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -186,7 +186,7 @@ func verifyAuthWithOptions(options *common.VerifyOptions) func(ctx *context.Cont // Redirect to log in page if auto-signin info is provided and has not signed in. if !options.SignOutRequired && !ctx.IsSigned && - len(ctx.GetSiteCookie(setting.CookieUserName)) > 0 { + len(ctx.GetSiteCookie(setting.CookieRememberName)) > 0 { if ctx.Req.URL.Path != "/user/events" { middleware.SetRedirectToCookie(ctx.Resp, setting.AppSubURL+ctx.Req.URL.RequestURI()) } diff --git a/services/auth/auth.go b/services/auth/auth.go index 0c8acac61f053..338e07005d3f5 100644 --- a/services/auth/auth.go +++ b/services/auth/auth.go @@ -72,10 +72,6 @@ func handleSignIn(resp http.ResponseWriter, req *http.Request, sess SessionStore if err != nil { log.Error(fmt.Sprintf("Error setting session: %v", err)) } - err = sess.Set("uname", user.Name) - if err != nil { - log.Error(fmt.Sprintf("Error setting session: %v", err)) - } // Language setting of the user overwrites the one previously set // If the user does not have a locale set, we save the current one. diff --git a/tests/integration/auth_token_test.go b/tests/integration/auth_token_test.go new file mode 100644 index 0000000000000..898a52f62e3ff --- /dev/null +++ b/tests/integration/auth_token_test.go @@ -0,0 +1,164 @@ +// Copyright 2023 The Gitea Authors. All rights reserved. +// Copyright 2023 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package integration + +import ( + "encoding/hex" + "net/http" + "net/url" + "strings" + "testing" + + "code.gitea.io/gitea/models/auth" + "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/timeutil" + "code.gitea.io/gitea/tests" + + "github.com/stretchr/testify/assert" +) + +// GetSessionForLTACookie returns a new session with only the LTA cookie being set. +func GetSessionForLTACookie(t *testing.T, ltaCookie *http.Cookie) *TestSession { + t.Helper() + + ch := http.Header{} + ch.Add("Cookie", ltaCookie.String()) + cr := http.Request{Header: ch} + + session := emptyTestSession(t) + baseURL, err := url.Parse(setting.AppURL) + assert.NoError(t, err) + session.jar.SetCookies(baseURL, cr.Cookies()) + + return session +} + +// GetLTACookieValue returns the value of the LTA cookie. +func GetLTACookieValue(t *testing.T, sess *TestSession) string { + t.Helper() + + rememberCookie := sess.GetCookie(setting.CookieRememberName) + assert.NotNil(t, rememberCookie) + + cookieValue, err := url.QueryUnescape(rememberCookie.Value) + assert.NoError(t, err) + + return cookieValue +} + +// TestSessionCookie checks if the session cookie provides authentication. +func TestSessionCookie(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + sess := loginUser(t, "user1") + assert.NotNil(t, sess.GetCookie(setting.SessionConfig.CookieName)) + + req := NewRequest(t, "GET", "/user/settings") + sess.MakeRequest(t, req, http.StatusOK) +} + +// TestLTACookie checks if the LTA cookie that's returned is valid, exists in the database +// and provides authentication of no session cookie is present. +func TestLTACookie(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + sess := emptyTestSession(t) + + req := NewRequestWithValues(t, "POST", "/user/login", map[string]string{ + "_csrf": GetCSRF(t, sess, "/user/login"), + "user_name": user.Name, + "password": userPassword, + "remember": "true", + }) + sess.MakeRequest(t, req, http.StatusSeeOther) + + // Checks if the database entry exist for the user. + ltaCookieValue := GetLTACookieValue(t, sess) + lookupKey, validator, found := strings.Cut(ltaCookieValue, ":") + assert.True(t, found) + rawValidator, err := hex.DecodeString(validator) + assert.NoError(t, err) + unittest.AssertExistsAndLoadBean(t, &auth.AuthorizationToken{LookupKey: lookupKey, HashedValidator: auth.HashValidator(rawValidator), UID: user.ID}) + + // Check if the LTA cookie it provides authentication. + // If LTA cookie provides authentication /user/login shouldn't return status 200. + session := GetSessionForLTACookie(t, sess.GetCookie(setting.CookieRememberName)) + req = NewRequest(t, "GET", "/user/login") + session.MakeRequest(t, req, http.StatusSeeOther) +} + +// TestLTAPasswordChange checks that LTA doesn't provide authentication when a +// password change has happened and that the new LTA does provide authentication. +func TestLTAPasswordChange(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + + sess := loginUserWithPasswordRemember(t, user.Name, userPassword, true) + oldRememberCookie := sess.GetCookie(setting.CookieRememberName) + assert.NotNil(t, oldRememberCookie) + + // Make a simple password change. + req := NewRequestWithValues(t, "POST", "/user/settings/account", map[string]string{ + "_csrf": GetCSRF(t, sess, "/user/settings/account"), + "old_password": userPassword, + "password": "password2", + "retype": "password2", + }) + sess.MakeRequest(t, req, http.StatusSeeOther) + rememberCookie := sess.GetCookie(setting.CookieRememberName) + assert.NotNil(t, rememberCookie) + + // Check if the password really changed. + assert.NotEqualValues(t, unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}).Passwd, user.Passwd) + + // /user/settings/account should provide with a new LTA cookie, so check for that. + // If LTA cookie provides authentication /user/login shouldn't return status 200. + session := GetSessionForLTACookie(t, rememberCookie) + req = NewRequest(t, "GET", "/user/login") + session.MakeRequest(t, req, http.StatusSeeOther) + + // Check if the old LTA token is invalidated. + session = GetSessionForLTACookie(t, oldRememberCookie) + req = NewRequest(t, "GET", "/user/login") + session.MakeRequest(t, req, http.StatusOK) +} + +// TestLTAExpiry tests that the LTA expiry works. +func TestLTAExpiry(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + + sess := loginUserWithPasswordRemember(t, user.Name, userPassword, true) + + ltaCookieValie := GetLTACookieValue(t, sess) + lookupKey, _, found := strings.Cut(ltaCookieValie, ":") + assert.True(t, found) + + // Ensure it's not expired. + lta := unittest.AssertExistsAndLoadBean(t, &auth.AuthorizationToken{UID: user.ID, LookupKey: lookupKey}) + assert.False(t, lta.IsExpired()) + + // Manually stub LTA's expiry. + _, err := db.GetEngine(db.DefaultContext).ID(lta.ID).Table("forgejo_auth_token").Cols("expiry").Update(&auth.AuthorizationToken{Expiry: timeutil.TimeStampNow()}) + assert.NoError(t, err) + + // Ensure it's expired. + lta = unittest.AssertExistsAndLoadBean(t, &auth.AuthorizationToken{UID: user.ID, LookupKey: lookupKey}) + assert.True(t, lta.IsExpired()) + + // Should return 200 OK, because LTA doesn't provide authorization anymore. + session := GetSessionForLTACookie(t, sess.GetCookie(setting.CookieRememberName)) + req := NewRequest(t, "GET", "/user/login") + session.MakeRequest(t, req, http.StatusOK) + + // Ensure it's deleted. + unittest.AssertNotExistsBean(t, &auth.AuthorizationToken{UID: user.ID, LookupKey: lookupKey}) +} diff --git a/tests/integration/integration_test.go b/tests/integration/integration_test.go index 76a65e3f03727..ac86ace981e3f 100644 --- a/tests/integration/integration_test.go +++ b/tests/integration/integration_test.go @@ -17,6 +17,7 @@ import ( "net/url" "os" "path/filepath" + "strconv" "strings" "sync/atomic" "testing" @@ -236,6 +237,12 @@ func loginUser(t testing.TB, userName string) *TestSession { func loginUserWithPassword(t testing.TB, userName, password string) *TestSession { t.Helper() + + return loginUserWithPasswordRemember(t, userName, password, false) +} + +func loginUserWithPasswordRemember(t testing.TB, userName, password string, rememberMe bool) *TestSession { + t.Helper() req := NewRequest(t, "GET", "/user/login") resp := MakeRequest(t, req, http.StatusOK) @@ -244,6 +251,7 @@ func loginUserWithPassword(t testing.TB, userName, password string) *TestSession "_csrf": doc.GetCSRF(), "user_name": userName, "password": password, + "remember": strconv.FormatBool(rememberMe), }) resp = MakeRequest(t, req, http.StatusSeeOther) From 3bd0635c5e31b44139998adb6d02962f8f35f0b7 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Thu, 5 Oct 2023 11:53:02 +0200 Subject: [PATCH 2/4] add the missing migration file --- models/auth/auth_token.go | 2 +- models/migrations/migrations.go | 2 ++ models/migrations/v1_21/v280.go | 23 +++++++++++++++++++++++ tests/integration/auth_token_test.go | 2 +- 4 files changed, 27 insertions(+), 2 deletions(-) create mode 100644 models/migrations/v1_21/v280.go diff --git a/models/auth/auth_token.go b/models/auth/auth_token.go index 9cbd21504b41e..3647b85cb1cf0 100644 --- a/models/auth/auth_token.go +++ b/models/auth/auth_token.go @@ -27,7 +27,7 @@ type AuthorizationToken struct { // TableName provides the real table name. func (AuthorizationToken) TableName() string { - return "forgejo_auth_token" + return "authorization_token" } func init() { diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index 020043cfc3b3e..96f9f55cab549 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -540,6 +540,8 @@ var migrations = []Migration{ NewMigration("Add Index to comment.dependent_issue_id", v1_21.AddIndexToCommentDependentIssueID), // v279 -> v280 NewMigration("Add Index to action.user_id", v1_21.AddIndexToActionUserID), + // v280 -> v281 + NewMigration("create the authorization_token table", v1_21.CreateAuthorizationTokenTable), } // GetCurrentDBVersion returns the current db version diff --git a/models/migrations/v1_21/v280.go b/models/migrations/v1_21/v280.go new file mode 100644 index 0000000000000..ab85ca8052db7 --- /dev/null +++ b/models/migrations/v1_21/v280.go @@ -0,0 +1,23 @@ +// Copyright 2023 The Gitea Authors. All rights reserved. +// Copyright 2023 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package v1_21 //nolint + +import ( + "code.gitea.io/gitea/modules/timeutil" + + "xorm.io/xorm" +) + +func CreateAuthorizationTokenTable(x *xorm.Engine) error { + type AuthorizationToken struct { + ID int64 `xorm:"pk autoincr"` + UID int64 `xorm:"INDEX"` + LookupKey string `xorm:"INDEX UNIQUE"` + HashedValidator string + Expiry timeutil.TimeStamp + } + + return x.Sync(new(AuthorizationToken)) +} diff --git a/tests/integration/auth_token_test.go b/tests/integration/auth_token_test.go index 898a52f62e3ff..2791906267793 100644 --- a/tests/integration/auth_token_test.go +++ b/tests/integration/auth_token_test.go @@ -147,7 +147,7 @@ func TestLTAExpiry(t *testing.T) { assert.False(t, lta.IsExpired()) // Manually stub LTA's expiry. - _, err := db.GetEngine(db.DefaultContext).ID(lta.ID).Table("forgejo_auth_token").Cols("expiry").Update(&auth.AuthorizationToken{Expiry: timeutil.TimeStampNow()}) + _, err := db.GetEngine(db.DefaultContext).ID(lta.ID).Table("authorization_token").Cols("expiry").Update(&auth.AuthorizationToken{Expiry: timeutil.TimeStampNow()}) assert.NoError(t, err) // Ensure it's expired. From a98e3c61c5835385da1089379af3a270fe0308fa Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Mon, 9 Oct 2023 22:37:34 +0200 Subject: [PATCH 3/4] move over to v1.22 --- models/migrations/migrations.go | 2 +- models/migrations/{v1_21/v280.go => v1_22/v281.go} | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) rename models/migrations/{v1_21/v280.go => v1_22/v281.go} (96%) diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index 36e93a1aed8a1..538fa1ba04c7b 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -547,7 +547,7 @@ var migrations = []Migration{ // v280 -> v281 NewMigration("Rename user themes", v1_22.RenameUserThemes), // v281 -> v282 - NewMigration("create the authorization_token table", v1_21.CreateAuthorizationTokenTable), + NewMigration("create the authorization_token table", v1_22.CreateAuthorizationTokenTable), } // GetCurrentDBVersion returns the current db version diff --git a/models/migrations/v1_21/v280.go b/models/migrations/v1_22/v281.go similarity index 96% rename from models/migrations/v1_21/v280.go rename to models/migrations/v1_22/v281.go index ab85ca8052db7..b9d6a3b89da91 100644 --- a/models/migrations/v1_21/v280.go +++ b/models/migrations/v1_22/v281.go @@ -2,7 +2,7 @@ // Copyright 2023 The Forgejo Authors. All rights reserved. // SPDX-License-Identifier: MIT -package v1_21 //nolint +package v1_22 //nolint import ( "code.gitea.io/gitea/modules/timeutil" From 73573258a5181779b55db99bc60918e3821df729 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Mon, 9 Oct 2023 22:40:29 +0200 Subject: [PATCH 4/4] remove the Forgejo copyright header under protest with permission from the author of the commits, the Forgejo copyright headers are removed. This is done under protest and for the sake of the Gitea users who would otherwise be deprived of an important security fix. --- models/auth/auth_token.go | 1 - models/migrations/v1_22/v281.go | 1 - tests/integration/auth_token_test.go | 1 - 3 files changed, 3 deletions(-) diff --git a/models/auth/auth_token.go b/models/auth/auth_token.go index 3647b85cb1cf0..c76f70de951f3 100644 --- a/models/auth/auth_token.go +++ b/models/auth/auth_token.go @@ -1,5 +1,4 @@ // Copyright 2023 The Gitea Authors. All rights reserved. -// Copyright 2023 The Forgejo Authors. All rights reserved. // SPDX-License-Identifier: MIT package auth diff --git a/models/migrations/v1_22/v281.go b/models/migrations/v1_22/v281.go index b9d6a3b89da91..0ab193370221d 100644 --- a/models/migrations/v1_22/v281.go +++ b/models/migrations/v1_22/v281.go @@ -1,5 +1,4 @@ // Copyright 2023 The Gitea Authors. All rights reserved. -// Copyright 2023 The Forgejo Authors. All rights reserved. // SPDX-License-Identifier: MIT package v1_22 //nolint diff --git a/tests/integration/auth_token_test.go b/tests/integration/auth_token_test.go index 2791906267793..172f7656fa865 100644 --- a/tests/integration/auth_token_test.go +++ b/tests/integration/auth_token_test.go @@ -1,5 +1,4 @@ // Copyright 2023 The Gitea Authors. All rights reserved. -// Copyright 2023 The Forgejo Authors. All rights reserved. // SPDX-License-Identifier: MIT package integration