diff --git a/cmd/serv.go b/cmd/serv.go index 0e0551d297..3a92b0e5fb 100644 --- a/cmd/serv.go +++ b/cmd/serv.go @@ -290,10 +290,9 @@ func runServ(ctx context.Context, c *cli.Command) error { Op: lfsVerb, UserID: results.UserID, } - token := jwt.NewWithClaims(jwt.SigningMethodHS256, claims) // Sign and get the complete encoded token as a string using the secret - tokenString, err := token.SignedString(setting.LFS.JWTSecretBytes) + tokenString, err := setting.LFS.SigningKey.JWT(claims) if err != nil { return fail(ctx, "Failed to sign JWT Token", "Failed to sign JWT token: %v", err) } diff --git a/custom/conf/app.example.ini b/custom/conf/app.example.ini index b0f6bd3d44..7a85d1e7ae 100644 --- a/custom/conf/app.example.ini +++ b/custom/conf/app.example.ini @@ -313,6 +313,9 @@ RUN_USER = ; git ;LFS_START_SERVER = false ;; ;; +;; see JWT_* under [oauth2] +;LFS_JWT_SIGNING_ALGORITHM = HS256 +;LFS_JWT_SIGNING_PRIVATE_KEY_FILE = jwt/lfs_private.pem ;; LFS authentication secret, change this yourself ;LFS_JWT_SECRET = ;; @@ -544,6 +547,7 @@ ENABLED = true ;; Private key file path used to sign OAuth2 tokens. The path is relative to APP_DATA_PATH. ;; This setting is only needed if JWT_SIGNING_ALGORITHM is set to RS256, RS384, RS512, ES256, ES384 or ES512. ;; The file must contain a RSA or ECDSA private key in the PKCS8 format. If no key exists a 4096 bit key will be created for you. +;; XXX jwt/ is a misnomer, it should rather be oauth2/, because we use many JWTs ;JWT_SIGNING_PRIVATE_KEY_FILE = jwt/private.pem ;; ;; OAuth2 authentication secret for access and refresh tokens, change this yourself to a unique string. CLI generate option is helpful in this case. https://forgejo.org/docs/latest/admin/command-line/#generate-secret @@ -2790,7 +2794,7 @@ LEVEL = Info ;; server and database workload due to more complex database queries and more frequent server task querying; this ;; feature can be disabled to reduce performance impact ;CONCURRENCY_GROUP_QUEUE_ENABLED = true -;; Algorithm used to sign ID tokens. Valid values: HS256, HS384, HS512, RS256, RS384, RS512, ES256, ES384, ES512, EdDSA. +;; Algorithm used to sign ID tokens. Valid values: RS256, RS384, RS512, ES256, ES384, ES512, EdDSA. ;; RS256 will ensure compatibility with all relying parties. ;; If a different algorithm is chosen, verify that relying parties of interest support the signing algorithm. ;ID_TOKEN_SIGNING_ALGORITHM = RS256 diff --git a/modules/generate/generate.go b/modules/generate/generate.go index 2df808fe9e..0dd5d071de 100644 --- a/modules/generate/generate.go +++ b/modules/generate/generate.go @@ -37,7 +37,7 @@ func DecodeJwtSecret(src string) ([]byte, error) { encoding := base64.RawURLEncoding decoded := make([]byte, encoding.DecodedLen(len(src))+3) if n, err := encoding.Decode(decoded, []byte(src)); err != nil { - return nil, err + return nil, fmt.Errorf("JwtSecret decode failed: %v", err) } else if n != defaultJwtSecretLen { return nil, fmt.Errorf("invalid base64 decoded length: %d, expects: %d", n, defaultJwtSecretLen) } diff --git a/modules/jwtx/signingkey.go b/modules/jwtx/signingkey.go index 2aef70440d..be1115cb15 100644 --- a/modules/jwtx/signingkey.go +++ b/modules/jwtx/signingkey.go @@ -25,6 +25,20 @@ import ( "github.com/golang-jwt/jwt/v5" ) +// The ...KeyCfg types are only used for handover from setting to signingkey +// see comment in setting/security.go + +type SigningKeyCfg struct { + Algorithm string + SecretBytes *[]byte + PrivateKeyPath *string +} + +type KeyCfg struct { + Signing *SigningKeyCfg + // more later +} + // ErrInvalidAlgorithmType represents an invalid algorithm error. type ErrInvalidAlgorithmType struct { Algorithm string @@ -399,50 +413,39 @@ func loadOrCreateAsymmetricKey(keyPath, algorithm string) (any, error) { return loadAsymmetricKey(keyPath) } -// InitSigningKey creates a signing key from settings or creates a random key. -func InitSigningKey(getGeneralTokenSigningSecret func() []byte, keyPath, algorithm string) (SigningKey, error) { +// InitSigningKey creates a signing key from SigningKeyCfg +// cfgP is set to nil to mark that is has been processed +func InitSigningKey(cfgP **SigningKeyCfg) (SigningKey, error) { + cfg := *cfgP + *cfgP = nil var err error var key SigningKey - key, err = InitSymmetricSigningKey(getGeneralTokenSigningSecret, algorithm) - if err != nil { - key, err = InitAsymmetricSigningKey(keyPath, algorithm) - if err != nil { - return nil, err - } + if IsValidSymmetricAlgorithm(cfg.Algorithm) { + key, err = CreateSigningKey(cfg.Algorithm, *cfg.SecretBytes) + } else if IsValidAsymmetricAlgorithm(cfg.Algorithm) { + key, err = InitAsymmetricSigningKey(*cfg.PrivateKeyPath, cfg.Algorithm) + } else { + // should never happen, setting.loadSigningKeyCfg() ensures + err = ErrInvalidAlgorithmType{Algorithm: cfg.Algorithm} } - return key, nil + return key, err } +var ( + ValidSymmetricAlgorighms = []string{"HS256", "HS384", "HS512"} + ValidAsymmetricAlgorithms = []string{"RS256", "RS384", "RS512", "ES256", "ES384", "ES512", "EdDSA"} +) + // IsValidSymmetricAlgorithm checks if the passed in algorithm is a supported symettric algorithm. func IsValidSymmetricAlgorithm(algorithm string) bool { - validAlgs := []string{"HS256", "HS384", "HS512"} - - return slices.Contains(validAlgs, algorithm) -} - -// InitSymmetricSigningKey creates a symmetric signing key from settings. -func InitSymmetricSigningKey(getGeneralTokenSigningSecret func() []byte, algorithm string) (SigningKey, error) { - var err error - - if !IsValidSymmetricAlgorithm(algorithm) { - return nil, fmt.Errorf("invalid algorithm: %s", algorithm) - } - - signingKey, err := CreateSigningKey(algorithm, getGeneralTokenSigningSecret()) - if err != nil { - return nil, err - } - - return signingKey, nil + return slices.Contains(ValidSymmetricAlgorighms, algorithm) } // IsValidAsymmetricAlgorithm checks if the passed in algorithm is a supported asymmetric algorithm. func IsValidAsymmetricAlgorithm(algorithm string) bool { - validAlgs := []string{"RS256", "RS384", "RS512", "ES256", "ES384", "ES512", "EdDSA"} - - return slices.Contains(validAlgs, algorithm) + return slices.Contains(ValidAsymmetricAlgorithms, algorithm) } // InitAsymmetricSigningKey creates an asymmetric signing key from settings or creates a random key. diff --git a/modules/jwtx/signingkey_test.go b/modules/jwtx/signingkey_test.go index eb30473cad..96f6613b84 100644 --- a/modules/jwtx/signingkey_test.go +++ b/modules/jwtx/signingkey_test.go @@ -18,6 +18,36 @@ import ( "github.com/stretchr/testify/require" ) +func testSignVerify(t *testing.T, signKey, verifyKey SigningKey) { + t.Helper() + // test sign and verify + claimsIn := jwt.RegisteredClaims{ + Issuer: "abc", + ID: "0815", + } + token, err := signKey.JWT(claimsIn) + require.NoError(t, err) + require.NotEmpty(t, token) + + var claimsOut jwt.RegisteredClaims + parsed, err := jwt.ParseWithClaims(token, &claimsOut, func(valToken *jwt.Token) (any, error) { + assert.NotNil(t, valToken.Method) + assert.Equal(t, signKey.SigningMethod().Alg(), valToken.Method.Alg()) + assert.Equal(t, verifyKey.SigningMethod().Alg(), valToken.Method.Alg()) + kid, ok := valToken.Header["kid"] + assert.True(t, ok) + assert.NotNil(t, kid) + + return verifyKey.VerifyKey(), nil + }) + require.NoError(t, err) + assert.NotNil(t, parsed) + assert.Equal(t, claimsIn, claimsOut) + assert.Equal(t, &claimsIn, parsed.Claims) +} + +// creates private key +// loads it back from the file func TestLoadOrCreateAsymmetricKey(t *testing.T) { loadKey := func(t *testing.T, keyPath, algorithm string) any { t.Helper() @@ -35,6 +65,21 @@ func TestLoadOrCreateAsymmetricKey(t *testing.T) { return parsedKey } + useKey := func(t *testing.T, keyPath, algorithm string) { + t.Helper() + // duplicates loadKey() to some extent, but uses SigningKey + cfg := &SigningKeyCfg{ + Algorithm: algorithm, + PrivateKeyPath: &keyPath, + } + + key, err := InitSigningKey(&cfg) + require.NoError(t, err) + assert.NotNil(t, key) + assert.Nil(t, cfg) + + testSignVerify(t, key, key) + } t.Run("RSA-2048", func(t *testing.T) { keyPath := filepath.Join(t.TempDir(), "jwt-rsa-2048.priv") algorithm := "RS256" @@ -43,6 +88,9 @@ func TestLoadOrCreateAsymmetricKey(t *testing.T) { rsaPrivateKey := parsedKey.(*rsa.PrivateKey) assert.Equal(t, 2048, rsaPrivateKey.N.BitLen()) + t.Run("Use", func(t *testing.T) { + useKey(t, keyPath, algorithm) + }) t.Run("Load key with differ specified algorithm", func(t *testing.T) { algorithm = "EdDSA" @@ -61,6 +109,9 @@ func TestLoadOrCreateAsymmetricKey(t *testing.T) { rsaPrivateKey := parsedKey.(*rsa.PrivateKey) assert.Equal(t, 3072, rsaPrivateKey.N.BitLen()) + t.Run("Use", func(t *testing.T) { + useKey(t, keyPath, algorithm) + }) }) t.Run("RSA-4096", func(t *testing.T) { @@ -71,6 +122,9 @@ func TestLoadOrCreateAsymmetricKey(t *testing.T) { rsaPrivateKey := parsedKey.(*rsa.PrivateKey) assert.Equal(t, 4096, rsaPrivateKey.N.BitLen()) + t.Run("Use", func(t *testing.T) { + useKey(t, keyPath, algorithm) + }) }) t.Run("ECDSA-256", func(t *testing.T) { @@ -81,6 +135,9 @@ func TestLoadOrCreateAsymmetricKey(t *testing.T) { ecdsaPrivateKey := parsedKey.(*ecdsa.PrivateKey) assert.Equal(t, 256, ecdsaPrivateKey.Params().BitSize) + t.Run("Use", func(t *testing.T) { + useKey(t, keyPath, algorithm) + }) }) t.Run("ECDSA-384", func(t *testing.T) { @@ -91,6 +148,9 @@ func TestLoadOrCreateAsymmetricKey(t *testing.T) { ecdsaPrivateKey := parsedKey.(*ecdsa.PrivateKey) assert.Equal(t, 384, ecdsaPrivateKey.Params().BitSize) + t.Run("Use", func(t *testing.T) { + useKey(t, keyPath, algorithm) + }) }) t.Run("ECDSA-512", func(t *testing.T) { @@ -101,6 +161,9 @@ func TestLoadOrCreateAsymmetricKey(t *testing.T) { ecdsaPrivateKey := parsedKey.(*ecdsa.PrivateKey) assert.Equal(t, 521, ecdsaPrivateKey.Params().BitSize) + t.Run("Use", func(t *testing.T) { + useKey(t, keyPath, algorithm) + }) }) t.Run("EdDSA", func(t *testing.T) { @@ -110,47 +173,12 @@ func TestLoadOrCreateAsymmetricKey(t *testing.T) { parsedKey := loadKey(t, keyPath, algorithm) assert.NotNil(t, parsedKey.(ed25519.PrivateKey)) + t.Run("Use", func(t *testing.T) { + useKey(t, keyPath, algorithm) + }) }) } -type testClaims struct { - Foo string `json:"Foo"` - jwt.RegisteredClaims -} - -func TestJWTHasKid(t *testing.T) { - keyPath := filepath.Join(t.TempDir(), "jwt-rsa-2048.priv") - algorithm := "RS256" - key, err := InitAsymmetricSigningKey(keyPath, algorithm) - require.NoError(t, err) - - claimsIn := testClaims{ - Foo: "bar", - RegisteredClaims: jwt.RegisteredClaims{}, - } - token, err := key.JWT(&claimsIn) - require.NoError(t, err) - - var claimsOut testClaims - parsed, err := jwt.ParseWithClaims(token, &claimsOut, func(valToken *jwt.Token) (any, error) { - assert.NotNil(t, valToken.Method) - assert.Equal(t, key.SigningMethod().Alg(), valToken.Method.Alg()) - kid, ok := valToken.Header["kid"] - assert.True(t, ok) - assert.NotNil(t, kid) - - return key.VerifyKey(), nil - }) - require.NoError(t, err) - assert.NotNil(t, parsed) - assert.Equal(t, "bar", parsed.Claims.(*testClaims).Foo) - assert.Equal(t, "bar", claimsOut.Foo) - // dup to keyFunc above - kid, ok := parsed.Header["kid"] - assert.True(t, ok) - assert.NotNil(t, kid) -} - func TestCannotCreatePrivateKey(t *testing.T) { _, err := InitAsymmetricSigningKey("/dev/directory-does-not-exist-and-you-should-not-have-permission-to-create/privatekey.pem", "RS256") require.Error(t, err) diff --git a/modules/setting/actions.go b/modules/setting/actions.go index 2e0554195f..930e0d0bed 100644 --- a/modules/setting/actions.go +++ b/modules/setting/actions.go @@ -5,7 +5,6 @@ package setting import ( "fmt" - "path/filepath" "strings" "time" @@ -28,17 +27,15 @@ var ( SkipWorkflowStrings []string `ini:"SKIP_WORKFLOW_STRINGS"` LimitDispatchInputs int64 `ini:"LIMIT_DISPATCH_INPUTS"` ConcurrencyGroupQueueEnabled bool `ini:"CONCURRENCY_GROUP_QUEUE_ENABLED"` - IDTokenSigningAlgorithm idTokenAlgorithm `ini:"ID_TOKEN_SIGNING_ALGORITHM"` - IDTokenSigningPrivateKeyFile string `ini:"ID_TOKEN_SIGNING_PRIVATE_KEY_FILE"` IDTokenExpirationTime int64 `ini:"ID_TOKEN_EXPIRATION_TIME"` + + KeyCfg *jwtx.KeyCfg }{ Enabled: true, DefaultActionsURL: defaultActionsURLForgejo, SkipWorkflowStrings: []string{"[skip ci]", "[ci skip]", "[no ci]", "[skip actions]", "[actions skip]"}, LimitDispatchInputs: 100, ConcurrencyGroupQueueEnabled: true, - IDTokenSigningAlgorithm: "RS256", - IDTokenSigningPrivateKeyFile: "actions_id_token/private.pem", IDTokenExpirationTime: 3600, } ) @@ -76,15 +73,9 @@ func (c logCompression) IsZstd() bool { return c == "" || strings.ToLower(string(c)) == "zstd" } -type idTokenAlgorithm string - -func (c idTokenAlgorithm) IsValid() bool { - // Empty string implies RS256 - return jwtx.IsValidAsymmetricAlgorithm(string(c)) || string(c) == "" -} - func loadActionsFrom(rootCfg ConfigProvider) error { - sec := rootCfg.Section("actions") + secName := "actions" + sec := rootCfg.Section(secName) err := sec.MapTo(&Actions) if err != nil { return fmt.Errorf("failed to map Actions settings: %v", err) @@ -120,13 +111,9 @@ func loadActionsFrom(rootCfg ConfigProvider) error { return fmt.Errorf("invalid [actions] LOG_COMPRESSION: %q", Actions.LogCompression) } - if !Actions.IDTokenSigningAlgorithm.IsValid() { - return fmt.Errorf("invalid [actions] ID_TOKEN_SIGNING_ALGORITHM: %q", Actions.IDTokenSigningAlgorithm) + Actions.KeyCfg, err = loadKeyCfg(rootCfg, secName, "ID_TOKEN_", "RS256", "actions_id_token/private.pem", onlyAsymmetric()) + if err != nil { + return err } - - if !filepath.IsAbs(Actions.IDTokenSigningPrivateKeyFile) { - Actions.IDTokenSigningPrivateKeyFile = filepath.Join(AppDataPath, Actions.IDTokenSigningPrivateKeyFile) - } - return nil } diff --git a/modules/setting/actions_test.go b/modules/setting/actions_test.go index 7d32131d32..1bbd4d0251 100644 --- a/modules/setting/actions_test.go +++ b/modules/setting/actions_test.go @@ -4,7 +4,6 @@ package setting import ( - "fmt" "path/filepath" "testing" @@ -176,8 +175,8 @@ func Test_getIDTokenSettingsForActions(t *testing.T) { require.NoError(t, err) require.NoError(t, loadActionsFrom(cfg)) - assert.EqualValues(t, "RS256", Actions.IDTokenSigningAlgorithm) - assert.Equal(t, "/home/app/data/actions_id_token/private.pem", Actions.IDTokenSigningPrivateKeyFile) + assert.Equal(t, "RS256", Actions.KeyCfg.Signing.Algorithm) + assert.Equal(t, "/home/app/data/actions_id_token/private.pem", *Actions.KeyCfg.Signing.PrivateKeyPath) assert.EqualValues(t, 3600, Actions.IDTokenExpirationTime) iniStr = ` @@ -190,8 +189,8 @@ func Test_getIDTokenSettingsForActions(t *testing.T) { require.NoError(t, err) require.NoError(t, loadActionsFrom(cfg)) - assert.EqualValues(t, "ES256", Actions.IDTokenSigningAlgorithm) - assert.Equal(t, "/test/test.pem", Actions.IDTokenSigningPrivateKeyFile) + assert.Equal(t, "ES256", Actions.KeyCfg.Signing.Algorithm) + assert.Equal(t, "/test/test.pem", *Actions.KeyCfg.Signing.PrivateKeyPath) assert.EqualValues(t, 120, Actions.IDTokenExpirationTime) iniStr = ` @@ -204,8 +203,8 @@ func Test_getIDTokenSettingsForActions(t *testing.T) { require.NoError(t, err) require.NoError(t, loadActionsFrom(cfg)) - assert.EqualValues(t, "EdDSA", Actions.IDTokenSigningAlgorithm) - assert.Equal(t, "/home/app/data/test/test.pem", Actions.IDTokenSigningPrivateKeyFile) + assert.Equal(t, "EdDSA", Actions.KeyCfg.Signing.Algorithm) + assert.Equal(t, "/home/app/data/test/test.pem", *Actions.KeyCfg.Signing.PrivateKeyPath) assert.EqualValues(t, 123, Actions.IDTokenExpirationTime) iniStr = ` @@ -215,6 +214,23 @@ func Test_getIDTokenSettingsForActions(t *testing.T) { cfg, err = NewConfigProviderFromData(iniStr) require.NoError(t, err) err = loadActionsFrom(cfg) - require.ErrorContains(t, err, - fmt.Sprintf("invalid [actions] ID_TOKEN_SIGNING_ALGORITHM: %q", Actions.IDTokenSigningAlgorithm)) + require.ErrorContains(t, err, "[actions] Unexpected algorithm: ID_TOKEN_SIGNING_ALGORITHM = HS256, needs to be one of [RS256 RS384 RS512 ES256 ES384 ES512 EdDSA]") + + iniStr = ` + [actions] + ID_TOKEN_SECRET = ABC + ` + cfg, err = NewConfigProviderFromData(iniStr) + require.NoError(t, err) + err = loadActionsFrom(cfg) + require.ErrorContains(t, err, "[actions] Invalid config key: ID_TOKEN_SECRET - must be removed") + + iniStr = ` + [actions] + ID_TOKEN_SECRET_URI = ABC + ` + cfg, err = NewConfigProviderFromData(iniStr) + require.NoError(t, err) + err = loadActionsFrom(cfg) + require.ErrorContains(t, err, "[actions] Invalid config key: ID_TOKEN_SECRET_URI - must be removed") } diff --git a/modules/setting/lfs.go b/modules/setting/lfs.go index 452bfae737..12f90cd092 100644 --- a/modules/setting/lfs.go +++ b/modules/setting/lfs.go @@ -7,7 +7,7 @@ import ( "fmt" "time" - "forgejo.org/modules/generate" + "forgejo.org/modules/jwtx" ) // LFS represents the server-side configuration for Git LFS. @@ -16,13 +16,13 @@ import ( // Could be refactored in the future while keeping backwards compatibility. var LFS = struct { 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"` MaxBatchSize int `ini:"LFS_MAX_BATCH_SIZE"` - Storage *Storage + SigningKey jwtx.SigningKey + Storage *Storage }{} // LFSClient represents configuration for Gitea's LFS clients, for example: mirroring upstream Git LFS @@ -77,20 +77,14 @@ func loadLFSFrom(rootCfg ConfigProvider) error { return nil } - jwtSecretBase64 := loadSecret(rootCfg.Section("server"), "LFS_JWT_SECRET_URI", "LFS_JWT_SECRET") - LFS.JWTSecretBytes, err = generate.DecodeJwtSecret(jwtSecretBase64) - if err != nil { - LFS.JWTSecretBytes, jwtSecretBase64 = generate.NewJwtSecret() - - // Save secret - saveCfg, err := rootCfg.PrepareSaving() - if err != nil { - return fmt.Errorf("error saving JWT Secret for custom config: %v", err) + // TODO: #11024 check nil because settings loaded twice + if LFS.SigningKey == nil { + keyCfg, err := loadKeyCfg(rootCfg, "server", "LFS_JWT_", "HS256", "lfs/private.pem") + if err == nil { + LFS.SigningKey, err = jwtx.InitSigningKey(&keyCfg.Signing) } - 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) + if err != nil { + return fmt.Errorf("lfs key initialization failed: %v", err) } } diff --git a/modules/setting/oauth2.go b/modules/setting/oauth2.go index 9e95e1c6a9..8598b7b24d 100644 --- a/modules/setting/oauth2.go +++ b/modules/setting/oauth2.go @@ -5,10 +5,10 @@ package setting import ( "math" - "path/filepath" "sync/atomic" "forgejo.org/modules/generate" + "forgejo.org/modules/jwtx" "forgejo.org/modules/log" ) @@ -96,18 +96,15 @@ var OAuth2 = struct { AccessTokenExpirationTime int64 RefreshTokenExpirationTime int64 InvalidateRefreshTokens bool - JWTSigningAlgorithm string `ini:"JWT_SIGNING_ALGORITHM"` - JWTSigningPrivateKeyFile string `ini:"JWT_SIGNING_PRIVATE_KEY_FILE"` MaxTokenLength int DefaultApplications []string EnableAdditionalGrantScopes bool + KeyCfg *jwtx.KeyCfg }{ Enabled: true, AccessTokenExpirationTime: 3600, RefreshTokenExpirationTime: 730, InvalidateRefreshTokens: true, - JWTSigningAlgorithm: "RS256", - JWTSigningPrivateKeyFile: "jwt/private.pem", MaxTokenLength: math.MaxInt16, DefaultApplications: []string{"git-credential-oauth", "git-credential-manager", "tea"}, EnableAdditionalGrantScopes: false, @@ -126,30 +123,26 @@ func loadOAuth2From(rootCfg ConfigProvider) { OAuth2.Enabled = sec.Key("ENABLE").MustBool(OAuth2.Enabled) } - if !filepath.IsAbs(OAuth2.JWTSigningPrivateKeyFile) { - OAuth2.JWTSigningPrivateKeyFile = filepath.Join(AppDataPath, OAuth2.JWTSigningPrivateKeyFile) - } - // FIXME: at the moment, no matter oauth2 is enabled or not, it must generate a "oauth2 JWT_SECRET" // Because this secret is also used as GeneralTokenSigningSecret (as a quick not-that-breaking fix for some legacy problems). // Including: CSRF token, account validation token, etc ... // In main branch, the signing token should be refactored (eg: one unique for LFS/OAuth2/etc ...) - jwtSecretBase64 := loadSecret(sec, "JWT_SECRET_URI", "JWT_SECRET") if InstallLock { - jwtSecretBytes, err := generate.DecodeJwtSecret(jwtSecretBase64) + signingKey, err := loadSymmeticSigningKeyCfg(rootCfg, sec, "JWT_") if err != nil { - jwtSecretBytes, jwtSecretBase64 = generate.NewJwtSecret() - saveCfg, err := rootCfg.PrepareSaving() - if err != nil { - log.Fatal("save oauth2.JWT_SECRET failed: %v", err) - } - 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) - } + log.Fatal("%v", err) } - generalSigningSecret.Store(&jwtSecretBytes) + generalSigningSecret.Store(signingKey) + } + + if !OAuth2.Enabled { + return + } + + var err error + OAuth2.KeyCfg, err = loadKeyCfg(rootCfg, "oauth2", "JWT_", "RS256", "jwt/private.pem") + if err != nil { + log.Fatal("oauth2 key initialization failed: %v", err) } } diff --git a/modules/setting/security.go b/modules/setting/security.go index e00a6af0ee..586989101d 100644 --- a/modules/setting/security.go +++ b/modules/setting/security.go @@ -4,12 +4,15 @@ package setting import ( + "fmt" "net/url" "os" + "path/filepath" "strings" "forgejo.org/modules/auth/password/hash" "forgejo.org/modules/generate" + "forgejo.org/modules/jwtx" "forgejo.org/modules/keying" "forgejo.org/modules/log" ) @@ -39,6 +42,31 @@ var ( DisableQueryAuthToken bool ) +/* + * key loading is a two-stage process to avoid complications for unit tests: + * + * For symmetric keys, we want to add a random key to the configuration. We would + * not want to change the configuration after loading has completed to maintain + * isolation. So from this perspective, we would want to initialize keys only + * during setting.load...From() + * + * For asymmetric keys, we want to create a random private key _file_. + * Doing so during the setting load phase, however, creates key files for unit + * tests, because they usually complete the full settings load phase, but do not + * initialize modules which they do not need. Depending on the test case, it + * might not provide a writable AppDataPath, or it would leave private key files + * in the source tree. All of this can be avoided with + * specifically adjusting the ini loaded for unit tests, but adds considerable + * friction. + * + * So to avoid all this, we split key loading in two phases: + * - settings parse the config and save missing symmetric keys + * - module init takes the parsed config, creates missing asymmetric keys and + * creates the actual signingkey objects + * + * jwtx.SigningKeyCfg and jwtx.KeyCfg are used for handover + */ + // loadSecret load the secret from ini by uriKey or verbatimKey, only one of them could be set // If the secret is loaded from uriKey (file), the file should be non-empty, to guarantee the behavior stable and clear. func loadSecret(sec ConfigSection, uriKey, verbatimKey string) string { @@ -53,16 +81,29 @@ func loadSecret(sec ConfigSection, uriKey, verbatimKey string) string { if uri == "" { return verbatim } + verbatim, err := loadSecretFromURI(uri) + if err == nil { + return verbatim + } + log.Fatal("%s: %w", uriKey, err) + // unreached + return "" +} +func loadSecretFromURI(uri string) (string, error) { tempURI, err := url.Parse(uri) if err != nil { - log.Fatal("Failed to parse %s (%s): %v", uriKey, uri, err) + return "", fmt.Errorf("Failed to parse %s: %v", uri, err) } switch tempURI.Scheme { case "file": - buf, err := os.ReadFile(tempURI.RequestURI()) + path := tempURI.RequestURI() + if !filepath.IsAbs(path) { + path = filepath.Join(AppDataPath, path) + } + buf, err := os.ReadFile(path) if err != nil { - log.Fatal("Failed to read %s (%s): %v", uriKey, tempURI.RequestURI(), err) + return "", fmt.Errorf("Failed to read %s: %v", path, err) } val := strings.TrimSpace(string(buf)) if val == "" { @@ -70,17 +111,135 @@ func loadSecret(sec ConfigSection, uriKey, verbatimKey string) string { // For example: if INTERNAL_TOKEN_URI=file:///empty-file, // Then if the token is re-generated during installation and saved to INTERNAL_TOKEN // Then INTERNAL_TOKEN and INTERNAL_TOKEN_URI both exist, that's a fatal error (they shouldn't) - log.Fatal("Failed to read %s (%s): the file is empty", uriKey, tempURI.RequestURI()) + return "", fmt.Errorf("Failed to read %s: the file is empty", path) } - return val + return val, nil // only file URIs are allowed default: - log.Fatal("Unsupported URI-Scheme %q (%q = %q)", tempURI.Scheme, uriKey, uri) - return "" + return "", fmt.Errorf("Unsupported URI-Scheme %q in %q", tempURI.Scheme, uri) } } +// createSymmeticSigningKey creates a new symmetric signing key and saves it to +// the setting named cfgSecret (usually [PFX_]SECRET) in section cfgSection +func createSymmeticSigningKeyCfg(rootCfg ConfigProvider, cfgSection, cfgSecret string) (*[]byte, error) { + jwtSecretBytes, jwtSecretBase64 := generate.NewJwtSecret() + saveCfg, err := rootCfg.PrepareSaving() + if err == nil { + rootCfg.Section(cfgSection).Key(cfgSecret).SetValue(jwtSecretBase64) + saveCfg.Section(cfgSection).Key(cfgSecret).SetValue(jwtSecretBase64) + err = saveCfg.Save() + } + if err != nil { + return nil, fmt.Errorf("save %s.%s failed: %v", cfgSection, cfgSecret, err) + } + return &jwtSecretBytes, nil +} + +// loadSymmeticSigningKey loads a signing key and creates it unless present +// loads from [pfx]SECRET_URI +// loads from or saves to [pfx]SECRET +// in section sec +func loadSymmeticSigningKeyCfg(rootCfg ConfigProvider, sec ConfigSection, pfx string) (*[]byte, error) { + cfgSecretURI := pfx + "SECRET_URI" + cfgSecret := pfx + "SECRET" + + secretBase64 := loadSecret(sec, cfgSecretURI, cfgSecret) + secret, err := generate.DecodeJwtSecret(secretBase64) + if err == nil { + return &secret, nil + } + + log.Info("[%s] %s or %s failed loading: %v - creating new key", sec.Name(), cfgSecret, cfgSecretURI, err) + return createSymmeticSigningKeyCfg(rootCfg, sec.Name(), cfgSecret) +} + +// loadAsymmeticSigningKey loads a signing key from [pfx]SIGNING_PRIVATE_KEY_FILE +// or creates it if it does not exist +func loadAsymmeticSigningKeyPath(sec ConfigSection, pfx, defaultFile string) *string { + cfgFile := pfx + "SIGNING_PRIVATE_KEY_FILE" + keyPath := sec.Key(cfgFile).MustString(defaultFile) + if !filepath.IsAbs(keyPath) { + keyPath = filepath.Join(AppDataPath, keyPath) + } + return &keyPath +} + +type checkKeyCfg func(rootCfg ConfigProvider, cfgSection, pfx string) error + +func onlyAsymmetric() checkKeyCfg { + return func(rootCfg ConfigProvider, cfgSection, pfx string) error { + sec := rootCfg.Section(cfgSection) + cfgAlg := pfx + "SIGNING_ALGORITHM" + + if sec.HasKey(cfgAlg) { + alg := sec.Key(cfgAlg).String() + if !jwtx.IsValidAsymmetricAlgorithm(alg) { + return fmt.Errorf("Unexpected algorithm: %s = %s, needs to be one of %v", + cfgAlg, alg, jwtx.ValidAsymmetricAlgorithms) + } + } + + noCfg := []string{ + pfx + "SECRET_URI", + pfx + "SECRET", + } + + for _, cfg := range noCfg { + if sec.HasKey(cfg) { + return fmt.Errorf("Invalid config key: %s - must be removed", cfg) + } + } + + return nil + } +} + +// loadSigningKey() loads a or creates signing key based on settings in section cfgSection +// [pfx]SIGNING_ALGORITHM determines the algorithm +// [pfx]SECRET is a literal secret for symmetric algorithms +// [pfx]SECRET_URI is the uri of a secret for symmetric algorithms +// [pfx]SIGNING_PRIVATE_KEY_FILE is a file with a private key for asymmetric algorithms +// +// [pfx]SECRET might get written to literally in the config if needed but not present + +func loadSigningKeyCfg(rootCfg ConfigProvider, cfgSection, pfx, defaultAlg, defaultPrivateKeyFile string, checks ...checkKeyCfg) (*jwtx.SigningKeyCfg, error) { + for _, check := range checks { + err := check(rootCfg, cfgSection, pfx) + if err != nil { + return nil, err + } + } + + sec := rootCfg.Section(cfgSection) + cfgAlg := pfx + "SIGNING_ALGORITHM" + + algorithm := sec.Key(cfgAlg).MustString(defaultAlg) + + cfg := jwtx.SigningKeyCfg{Algorithm: algorithm} + var err error + + if jwtx.IsValidSymmetricAlgorithm(algorithm) { + cfg.SecretBytes, err = loadSymmeticSigningKeyCfg(rootCfg, sec, pfx) + } else if jwtx.IsValidAsymmetricAlgorithm(algorithm) { + cfg.PrivateKeyPath = loadAsymmeticSigningKeyPath(sec, pfx, defaultPrivateKeyFile) + } else { + err = fmt.Errorf("invalid algorithm: %s = %s", cfgAlg, algorithm) + } + + return &cfg, err +} + +func loadKeyCfg(rootCfg ConfigProvider, cfgSection, pfx, defaultAlg, defaultPrivateKeyFile string, checks ...checkKeyCfg) (*jwtx.KeyCfg, error) { + signing, err := loadSigningKeyCfg(rootCfg, cfgSection, pfx, defaultAlg, defaultPrivateKeyFile, checks...) + if err != nil { + err = fmt.Errorf("[%s] %v", cfgSection, err) + return nil, err + } + return &jwtx.KeyCfg{Signing: signing}, nil +} + // generateSaveInternalToken generates and saves the internal token to app.ini func generateSaveInternalToken(rootCfg ConfigProvider) { token, err := generate.NewInternalToken() diff --git a/modules/storage/storage.go b/modules/storage/storage.go index db081e0768..fe9222060f 100644 --- a/modules/storage/storage.go +++ b/modules/storage/storage.go @@ -164,6 +164,7 @@ func initLFS() (err error) { LFS = DiscardStorage("LFS isn't enabled") return nil } + log.Info("Initialising LFS storage with type: %s", setting.LFS.Storage.Type) LFS, err = NewStorage(setting.LFS.Storage.Type, setting.LFS.Storage) return err diff --git a/release-notes/11194.md b/release-notes/11194.md new file mode 100644 index 0000000000..1ca37e04c3 --- /dev/null +++ b/release-notes/11194.md @@ -0,0 +1 @@ +Configuration of JSON Web Token (JWT) signing secrets in the `app.ini` file has been unified: `[pfx]SIGNING_ALGORITHM` determines the algorithm, `[pfx]SECRET` is a literal secret for symmetric algorithms, `[pfx]SECRET_URI` is the `file:` uri of a secret for symmetric algorithms `[pfx]SIGNING_PRIVATE_KEY_FILE` is the file with a private key for asymmetric algorithms. For the following configuration keys, the existing behavior is preserved: `[oauth2]` pfx = `JWT_`, `[actions]` pfx = `ID_TOKEN_` only supports asymmetric algorithms. `[server]` pfx = `LFS_JWT_` gained support for asymmetric algorithms. For consistency with `[pfx]SIGNING_PRIVATE_KEY_FILE`, `[pfx]SECRET_URI` now also accepts relative paths. diff --git a/routers/api/actions/oidc.go b/routers/api/actions/oidc.go index d824030ca7..20cc44f36a 100644 --- a/routers/api/actions/oidc.go +++ b/routers/api/actions/oidc.go @@ -48,7 +48,7 @@ type OIDCContext struct { func InitOIDC() error { var err error - jwtSigningKey, err = jwtx.InitAsymmetricSigningKey(setting.Actions.IDTokenSigningPrivateKeyFile, string(setting.Actions.IDTokenSigningAlgorithm)) + jwtSigningKey, err = jwtx.InitSigningKey(&setting.Actions.KeyCfg.Signing) if err != nil { return err } @@ -118,7 +118,7 @@ func OIDCRoutes(prefix string) *web.Route { SubjectTypesSupported: []string{"public"}, ResponseTypesSupported: []string{"id_token"}, ClaimsSupported: claimsSupported, - IDTokenSigningAlgValuesSupported: []string{string(setting.Actions.IDTokenSigningAlgorithm)}, + IDTokenSigningAlgValuesSupported: []string{jwtSigningKey.SigningMethod().Alg()}, ScopesSupported: []string{"openid"}, }, jwks: map[string][]map[string]string{ diff --git a/services/auth/source/oauth2/init.go b/services/auth/source/oauth2/init.go index ac7853222c..6e0714b9b5 100644 --- a/services/auth/source/oauth2/init.go +++ b/services/auth/source/oauth2/init.go @@ -35,7 +35,7 @@ var DefaultSigningKey jwtx.SigningKey // Init initializes the oauth source func Init(ctx context.Context) error { var err error - DefaultSigningKey, err = jwtx.InitSigningKey(setting.GetGeneralTokenSigningSecret, setting.OAuth2.JWTSigningPrivateKeyFile, setting.OAuth2.JWTSigningAlgorithm) + DefaultSigningKey, err = jwtx.InitSigningKey(&setting.OAuth2.KeyCfg.Signing) if err != nil { return err } diff --git a/services/lfs/server.go b/services/lfs/server.go index cc8afc2aa8..ef0df4a0ab 100644 --- a/services/lfs/server.go +++ b/services/lfs/server.go @@ -580,10 +580,11 @@ func authenticate(ctx *context.Context, repository *repo_model.Repository, autho func handleLFSToken(ctx stdCtx.Context, tokenSHA string, target *repo_model.Repository, mode perm.AccessMode) (*user_model.User, error) { token, err := jwt.ParseWithClaims(tokenSHA, &Claims{}, func(t *jwt.Token) (any, error) { - if _, ok := t.Method.(*jwt.SigningMethodHMAC); !ok { + k := setting.LFS.SigningKey + if t.Method != k.SigningMethod() { return nil, fmt.Errorf("unexpected signing method: %v", t.Header["alg"]) } - return setting.LFS.JWTSecretBytes, nil + return k.VerifyKey(), nil }) if err != nil { return nil, errors.New("invalid token") diff --git a/services/lfs/server_test.go b/services/lfs/server_test.go index 13e7e73010..67c93f30bb 100644 --- a/services/lfs/server_test.go +++ b/services/lfs/server_test.go @@ -41,18 +41,23 @@ func getLFSAuthTokenWithBearer(opts authTokenOptions) (string, error) { Op: opts.Op, UserID: opts.UserID, } - token := jwt.NewWithClaims(jwt.SigningMethodHS256, claims) // Sign and get the complete encoded token as a string using the secret - tokenString, err := token.SignedString(setting.LFS.JWTSecretBytes) + tokenString, err := setting.LFS.SigningKey.JWT(claims) if err != nil { return "", fmt.Errorf("failed to sign LFS JWT token: %w", err) } return "Bearer " + tokenString, nil } -func TestAuthenticate(t *testing.T) { +func testAuthenticate(t *testing.T, cfg string) { require.NoError(t, unittest.PrepareTestDatabase()) + var err error + setting.CfgProvider, err = setting.NewConfigProviderFromData(cfg) + require.NoError(t, err, "Config") + setting.LoadCommonSettings() + assert.True(t, setting.LFS.StartServer, "LFS_START_SERVER = true") + assert.NotNil(t, setting.LFS.SigningKey, "SigningKey initialized") repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) token2, _ := getLFSAuthTokenWithBearer(authTokenOptions{Op: "download", UserID: 2, RepoID: 1}) @@ -80,3 +85,30 @@ func TestAuthenticate(t *testing.T) { assert.True(t, authenticate(ctx, repo1, prefixBearer+token2, true, false)) }) } + +type namedCfg struct { + name, cfg string +} + +var iniCommon = `[security] +INSTALL_LOCK = true +INTERNAL_TOKEN = ForgejoForgejoForgejoForgejoForgejoForgejo_ # don't use in prod +[oauth2] +JWT_SECRET = ForgejoForgejoForgejoForgejoForgejoForgejo_ # don't use in prod +[server] +LFS_START_SERVER = true + ` + +var cfgVariants = []namedCfg{ + {name: "HS256_default", cfg: `LFS_JWT_SECRET = ForgejoForgejoForgejoForgejoForgejoForgejo_`}, + {name: "RS256", cfg: `LFS_JWT_SIGNING_ALGORITHM = RS256`}, +} + +func TestAuthenticate(t *testing.T) { + // TODO: #11024 + setting.InstallLock = true + for _, v := range cfgVariants { + cfg := iniCommon + v.cfg + t.Run(v.name, func(t *testing.T) { testAuthenticate(t, cfg) }) + } +} diff --git a/services/repository/lfs_test.go b/services/repository/lfs_test.go index e38c38e29c..0224b71100 100644 --- a/services/repository/lfs_test.go +++ b/services/repository/lfs_test.go @@ -21,11 +21,25 @@ import ( "github.com/stretchr/testify/require" ) +var ini = `[security] +INSTALL_LOCK = true +INTERNAL_TOKEN = ForgejoForgejoForgejoForgejoForgejoForgejo_ # don't use in prod +[oauth2] +JWT_SECRET = ForgejoForgejoForgejoForgejoForgejoForgejo_ # don't use in prod +[server] +LFS_START_SERVER = true +LFS_JWT_SECRET = ForgejoForgejoForgejoForgejoForgejoForgejo_ # don't use in prod + ` + func TestGarbageCollectLFSMetaObjects(t *testing.T) { + var err error + setting.CfgProvider, err = setting.NewConfigProviderFromData(ini) + require.NoError(t, err, "Config") + setting.LoadCommonSettings() + unittest.PrepareTestEnv(t) - setting.LFS.StartServer = true - err := storage.Init() + err = storage.Init() require.NoError(t, err) repo, err := repo_model.GetRepositoryByOwnerAndName(db.DefaultContext, "user2", "lfs")