From 507ec03ffdc98f7af86af7210c782cafd95ed917 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sat, 17 Feb 2024 12:49:06 +0800 Subject: [PATCH 1/5] refactor --- modules/base/tool.go | 2 +- modules/context/context.go | 3 +- modules/setting/lfs.go | 21 ++++++------ modules/setting/oauth2.go | 32 ++++++++++++++---- modules/setting/oauth2_test.go | 34 ++++++++++++++++++++ services/actions/auth.go | 4 +-- services/actions/auth_test.go | 2 +- services/auth/source/oauth2/jwtsigningkey.go | 9 +----- services/packages/auth.go | 4 +-- 9 files changed, 78 insertions(+), 33 deletions(-) create mode 100644 modules/setting/oauth2_test.go diff --git a/modules/base/tool.go b/modules/base/tool.go index e9f4dfa27947..19fb2c451fdf 100644 --- a/modules/base/tool.go +++ b/modules/base/tool.go @@ -115,7 +115,7 @@ func CreateTimeLimitCode(data string, minutes int, startInf any) string { // create sha1 encode string sh := sha1.New() - _, _ = sh.Write([]byte(fmt.Sprintf("%s%s%s%s%d", data, setting.SecretKey, startStr, endStr, minutes))) + _, _ = sh.Write([]byte(fmt.Sprintf("%s%s%s%s%d", data, hex.EncodeToString(setting.GetGeneralTokenSigningSecret()), startStr, endStr, minutes))) encoded := hex.EncodeToString(sh.Sum(nil)) code := fmt.Sprintf("%s%06d%s", startStr, minutes, encoded) diff --git a/modules/context/context.go b/modules/context/context.go index 4d367b3242cb..66732eaa8aa6 100644 --- a/modules/context/context.go +++ b/modules/context/context.go @@ -6,6 +6,7 @@ package context import ( "context" + "encoding/hex" "fmt" "html/template" "io" @@ -124,7 +125,7 @@ func NewWebContext(base *Base, render Render, session session.Store) *Context { func Contexter() func(next http.Handler) http.Handler { rnd := templates.HTMLRenderer() csrfOpts := CsrfOptions{ - Secret: setting.SecretKey, + Secret: hex.EncodeToString(setting.GetGeneralTokenSigningSecret()), Cookie: setting.CSRFCookieName, SetCookie: true, Secure: setting.SessionConfig.Secure, diff --git a/modules/setting/lfs.go b/modules/setting/lfs.go index 22a75f60084f..2034ef782c22 100644 --- a/modules/setting/lfs.go +++ b/modules/setting/lfs.go @@ -12,12 +12,11 @@ import ( // LFS represents the configuration for Git LFS var LFS = struct { - StartServer bool `ini:"LFS_START_SERVER"` - JWTSecretBase64 string `ini:"LFS_JWT_SECRET"` - JWTSecretBytes []byte `ini:"-"` - HTTPAuthExpiry time.Duration `ini:"LFS_HTTP_AUTH_EXPIRY"` - MaxFileSize int64 `ini:"LFS_MAX_FILE_SIZE"` - LocksPagingNum int `ini:"LFS_LOCKS_PAGING_NUM"` + StartServer bool `ini:"LFS_START_SERVER"` + JWTSecretBytes []byte `ini:"-"` + HTTPAuthExpiry time.Duration `ini:"LFS_HTTP_AUTH_EXPIRY"` + MaxFileSize int64 `ini:"LFS_MAX_FILE_SIZE"` + LocksPagingNum int `ini:"LFS_LOCKS_PAGING_NUM"` Storage *Storage }{} @@ -59,10 +58,10 @@ func loadLFSFrom(rootCfg ConfigProvider) error { return nil } - LFS.JWTSecretBase64 = loadSecret(rootCfg.Section("server"), "LFS_JWT_SECRET_URI", "LFS_JWT_SECRET") - LFS.JWTSecretBytes, err = generate.DecodeJwtSecretBase64(LFS.JWTSecretBase64) + jwtSecretBase64 := loadSecret(rootCfg.Section("server"), "LFS_JWT_SECRET_URI", "LFS_JWT_SECRET") + LFS.JWTSecretBytes, err = generate.DecodeJwtSecretBase64(jwtSecretBase64) if err != nil { - LFS.JWTSecretBytes, LFS.JWTSecretBase64, err = generate.NewJwtSecretWithBase64() + LFS.JWTSecretBytes, jwtSecretBase64, err = generate.NewJwtSecretWithBase64() if err != nil { return fmt.Errorf("error generating JWT Secret for custom config: %v", err) } @@ -72,8 +71,8 @@ func loadLFSFrom(rootCfg ConfigProvider) error { if err != nil { return fmt.Errorf("error saving JWT Secret for custom config: %v", err) } - rootCfg.Section("server").Key("LFS_JWT_SECRET").SetValue(LFS.JWTSecretBase64) - saveCfg.Section("server").Key("LFS_JWT_SECRET").SetValue(LFS.JWTSecretBase64) + rootCfg.Section("server").Key("LFS_JWT_SECRET").SetValue(jwtSecretBase64) + saveCfg.Section("server").Key("LFS_JWT_SECRET").SetValue(jwtSecretBase64) if err := saveCfg.Save(); err != nil { return fmt.Errorf("error saving JWT Secret for custom config: %v", err) } diff --git a/modules/setting/oauth2.go b/modules/setting/oauth2.go index e16e1670249c..af7476636271 100644 --- a/modules/setting/oauth2.go +++ b/modules/setting/oauth2.go @@ -4,8 +4,10 @@ package setting import ( + "fmt" "math" "path/filepath" + "sync/atomic" "code.gitea.io/gitea/modules/generate" "code.gitea.io/gitea/modules/log" @@ -96,7 +98,6 @@ var OAuth2 = struct { RefreshTokenExpirationTime int64 InvalidateRefreshTokens bool JWTSigningAlgorithm string `ini:"JWT_SIGNING_ALGORITHM"` - JWTSecretBase64 string `ini:"JWT_SECRET"` JWTSigningPrivateKeyFile string `ini:"JWT_SIGNING_PRIVATE_KEY_FILE"` MaxTokenLength int DefaultApplications []string @@ -128,28 +129,45 @@ func loadOAuth2From(rootCfg ConfigProvider) { return } - OAuth2.JWTSecretBase64 = loadSecret(sec, "JWT_SECRET_URI", "JWT_SECRET") + jwtSecretBase64 := loadSecret(sec, "JWT_SECRET_URI", "JWT_SECRET") if !filepath.IsAbs(OAuth2.JWTSigningPrivateKeyFile) { OAuth2.JWTSigningPrivateKeyFile = filepath.Join(AppDataPath, OAuth2.JWTSigningPrivateKeyFile) } if InstallLock { - if _, err := generate.DecodeJwtSecretBase64(OAuth2.JWTSecretBase64); err != nil { - _, OAuth2.JWTSecretBase64, err = generate.NewJwtSecretWithBase64() + jwtSecretBytes, err := generate.DecodeJwtSecretBase64(jwtSecretBase64) + if err != nil { + jwtSecretBytes, jwtSecretBase64, err = generate.NewJwtSecretWithBase64() if err != nil { log.Fatal("error generating JWT secret: %v", err) } - saveCfg, err := rootCfg.PrepareSaving() if err != nil { log.Fatal("save oauth2.JWT_SECRET failed: %v", err) } - rootCfg.Section("oauth2").Key("JWT_SECRET").SetValue(OAuth2.JWTSecretBase64) - saveCfg.Section("oauth2").Key("JWT_SECRET").SetValue(OAuth2.JWTSecretBase64) + rootCfg.Section("oauth2").Key("JWT_SECRET").SetValue(jwtSecretBase64) + saveCfg.Section("oauth2").Key("JWT_SECRET").SetValue(jwtSecretBase64) if err := saveCfg.Save(); err != nil { log.Fatal("save oauth2.JWT_SECRET failed: %v", err) } } + generalSigningSecret.Store(&jwtSecretBytes) + } +} + +var generalSigningSecret atomic.Pointer[[]byte] + +func GetGeneralTokenSigningSecret() []byte { + old := generalSigningSecret.Load() + if old == nil || len(*old) == 0 { + jwtSecret, _, err := generate.NewJwtSecretWithBase64() + if err != nil { + panic(fmt.Errorf("unable to generate general JWT secret: %w", err)) + } + if generalSigningSecret.CompareAndSwap(old, &jwtSecret) { + log.Warn("OAuth2 is not enabled, unable to use a persistent signing secret, a new one is generated, which is not persistent between restarts and cluster nodes") + } } + return *generalSigningSecret.Load() } diff --git a/modules/setting/oauth2_test.go b/modules/setting/oauth2_test.go new file mode 100644 index 000000000000..d822198619db --- /dev/null +++ b/modules/setting/oauth2_test.go @@ -0,0 +1,34 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package setting + +import ( + "testing" + + "code.gitea.io/gitea/modules/generate" + "code.gitea.io/gitea/modules/test" + + "github.com/stretchr/testify/assert" +) + +func TestGetGeneralSigningSecret(t *testing.T) { + // when there is no general signing secret, it should be generated, and keep the same value + assert.Nil(t, generalSigningSecret.Load()) + s1 := GetGeneralTokenSigningSecret() + assert.NotNil(t, s1) + s2 := GetGeneralTokenSigningSecret() + assert.Equal(t, s1, s2) + + // the config value should always override any pre-generated value + cfg, _ := NewConfigProviderFromData(` +[oauth2] +JWT_SECRET = BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB +`) + defer test.MockVariableValue(&InstallLock, true)() + loadOAuth2From(cfg) + actual := GetGeneralTokenSigningSecret() + expected, _ := generate.DecodeJwtSecretBase64("BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB") + assert.Len(t, actual, 32) + assert.EqualValues(t, expected, actual) +} diff --git a/services/actions/auth.go b/services/actions/auth.go index 53e68f0b71bf..e0f9a9015dcd 100644 --- a/services/actions/auth.go +++ b/services/actions/auth.go @@ -38,7 +38,7 @@ func CreateAuthorizationToken(taskID, runID, jobID int64) (string, error) { } token := jwt.NewWithClaims(jwt.SigningMethodHS256, claims) - tokenString, err := token.SignedString([]byte(setting.SecretKey)) + tokenString, err := token.SignedString(setting.GetGeneralTokenSigningSecret()) if err != nil { return "", err } @@ -62,7 +62,7 @@ func ParseAuthorizationToken(req *http.Request) (int64, error) { if _, ok := t.Method.(*jwt.SigningMethodHMAC); !ok { return nil, fmt.Errorf("unexpected signing method: %v", t.Header["alg"]) } - return []byte(setting.SecretKey), nil + return setting.GetGeneralTokenSigningSecret(), nil }) if err != nil { return 0, err diff --git a/services/actions/auth_test.go b/services/actions/auth_test.go index f6288ccd5ad3..1f62f17f52a8 100644 --- a/services/actions/auth_test.go +++ b/services/actions/auth_test.go @@ -20,7 +20,7 @@ func TestCreateAuthorizationToken(t *testing.T) { assert.NotEqual(t, "", token) claims := jwt.MapClaims{} _, err = jwt.ParseWithClaims(token, claims, func(t *jwt.Token) (interface{}, error) { - return []byte(setting.SecretKey), nil + return setting.GetGeneralTokenSigningSecret(), nil }) assert.Nil(t, err) scp, ok := claims["scp"] diff --git a/services/auth/source/oauth2/jwtsigningkey.go b/services/auth/source/oauth2/jwtsigningkey.go index 2afe557b0d62..070fffe60f7f 100644 --- a/services/auth/source/oauth2/jwtsigningkey.go +++ b/services/auth/source/oauth2/jwtsigningkey.go @@ -18,7 +18,6 @@ import ( "path/filepath" "strings" - "code.gitea.io/gitea/modules/generate" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/util" @@ -301,7 +300,7 @@ func InitSigningKey() error { case "HS384": fallthrough case "HS512": - key, err = loadSymmetricKey() + key = setting.GetGeneralTokenSigningSecret() case "RS256": fallthrough case "RS384": @@ -334,12 +333,6 @@ func InitSigningKey() error { return nil } -// loadSymmetricKey checks if the configured secret is valid. -// If it is not valid, it will return an error. -func loadSymmetricKey() (any, error) { - return generate.DecodeJwtSecretBase64(setting.OAuth2.JWTSecretBase64) -} - // loadOrCreateAsymmetricKey checks if the configured private key exists. // If it does not exist a new random key gets generated and saved on the configured path. func loadOrCreateAsymmetricKey() (any, error) { diff --git a/services/packages/auth.go b/services/packages/auth.go index 2f78b26f506e..8263c28bed02 100644 --- a/services/packages/auth.go +++ b/services/packages/auth.go @@ -33,7 +33,7 @@ func CreateAuthorizationToken(u *user_model.User) (string, error) { } token := jwt.NewWithClaims(jwt.SigningMethodHS256, claims) - tokenString, err := token.SignedString([]byte(setting.SecretKey)) + tokenString, err := token.SignedString(setting.GetGeneralTokenSigningSecret()) if err != nil { return "", err } @@ -57,7 +57,7 @@ func ParseAuthorizationToken(req *http.Request) (int64, error) { if _, ok := t.Method.(*jwt.SigningMethodHMAC); !ok { return nil, fmt.Errorf("unexpected signing method: %v", t.Header["alg"]) } - return []byte(setting.SecretKey), nil + return setting.GetGeneralTokenSigningSecret(), nil }) if err != nil { return 0, err From ef40ef35f920e1605cc91a0c2ebec180c3cddd36 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sat, 17 Feb 2024 13:07:52 +0800 Subject: [PATCH 2/5] optmize --- modules/setting/oauth2.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/modules/setting/oauth2.go b/modules/setting/oauth2.go index af7476636271..40f75dc23843 100644 --- a/modules/setting/oauth2.go +++ b/modules/setting/oauth2.go @@ -167,7 +167,9 @@ func GetGeneralTokenSigningSecret() []byte { } if generalSigningSecret.CompareAndSwap(old, &jwtSecret) { log.Warn("OAuth2 is not enabled, unable to use a persistent signing secret, a new one is generated, which is not persistent between restarts and cluster nodes") + return jwtSecret } + return *generalSigningSecret.Load() } - return *generalSigningSecret.Load() + return *old } From a1e2a0831f3df7de3ed6e56f1a092cd875e3b635 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sat, 17 Feb 2024 17:30:30 +0800 Subject: [PATCH 3/5] use log.Fatal instead of panic --- modules/setting/oauth2.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/modules/setting/oauth2.go b/modules/setting/oauth2.go index 40f75dc23843..7dc8e9afe639 100644 --- a/modules/setting/oauth2.go +++ b/modules/setting/oauth2.go @@ -4,7 +4,6 @@ package setting import ( - "fmt" "math" "path/filepath" "sync/atomic" @@ -163,7 +162,7 @@ func GetGeneralTokenSigningSecret() []byte { if old == nil || len(*old) == 0 { jwtSecret, _, err := generate.NewJwtSecretWithBase64() if err != nil { - panic(fmt.Errorf("unable to generate general JWT secret: %w", err)) + log.Fatal("Unable to generate general JWT secret: %s", err.Error()) } if generalSigningSecret.CompareAndSwap(old, &jwtSecret) { log.Warn("OAuth2 is not enabled, unable to use a persistent signing secret, a new one is generated, which is not persistent between restarts and cluster nodes") From a9b376ba9ce697538c09c57c7153e396a1acf041 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Sat, 17 Feb 2024 21:48:49 +0100 Subject: [PATCH 4/5] code comment --- modules/setting/oauth2.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/modules/setting/oauth2.go b/modules/setting/oauth2.go index 7dc8e9afe639..334847ca0da7 100644 --- a/modules/setting/oauth2.go +++ b/modules/setting/oauth2.go @@ -155,6 +155,8 @@ func loadOAuth2From(rootCfg ConfigProvider) { } } +// generalSigningSecret is used as container for a []byte value +// instead of an additional mutex, we use CompareAndSwap func to change the value thread save var generalSigningSecret atomic.Pointer[[]byte] func GetGeneralTokenSigningSecret() []byte { From fa2d841265cabf199285424486034ebddc0c66df Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Sun, 18 Feb 2024 18:13:29 +0100 Subject: [PATCH 5/5] Update modules/setting/oauth2.go Co-authored-by: wxiaoguang --- modules/setting/oauth2.go | 1 + 1 file changed, 1 insertion(+) diff --git a/modules/setting/oauth2.go b/modules/setting/oauth2.go index 334847ca0da7..4d3bfd3eb60a 100644 --- a/modules/setting/oauth2.go +++ b/modules/setting/oauth2.go @@ -167,6 +167,7 @@ func GetGeneralTokenSigningSecret() []byte { log.Fatal("Unable to generate general JWT secret: %s", err.Error()) } if generalSigningSecret.CompareAndSwap(old, &jwtSecret) { + // FIXME: in main branch, the signing token should be refactored (eg: one unique for LFS/OAuth2/etc ...) log.Warn("OAuth2 is not enabled, unable to use a persistent signing secret, a new one is generated, which is not persistent between restarts and cluster nodes") return jwtSecret }