2023-06-14 11:42:38 +08:00
// Copyright 2023 The Gitea Authors. All rights reserved.
// SPDX-License-Identifier: MIT
package setting
import (
"path/filepath"
"testing"
2026-01-15 03:39:00 +01:00
"forgejo.org/modules/test"
2023-06-14 11:42:38 +08:00
"github.com/stretchr/testify/assert"
Restrict `[actions].DEFAULT_ACTIONS_URL` to only `github` or `self` (#25581)
Resolve #24789
## :warning: BREAKING :warning:
Before this, `DEFAULT_ACTIONS_URL` cound be set to any custom URLs like
`https://gitea.com` or `http://your-git-server,https://gitea.com`, and
the default value was `https://gitea.com`.
But now, `DEFAULT_ACTIONS_URL` supports only
`github`(`https://github.com`) or `self`(the root url of current Gitea
instance), and the default value is `github`.
If it has configured with a URL, an error log will be displayed and it
will fallback to `github`.
Actually, what we really want to do is always make it
`https://github.com`, however, this may not be acceptable for some
instances of internal use, so there's extra support for `self`, but no
more, even `https://gitea.com`.
Please note that `uses: https://xxx/yyy/zzz` always works and it does
exactly what it is supposed to do.
Although it's breaking, I belive it should be backported to `v1.20` due
to some security issues.
Follow-up on the runner side:
- https://gitea.com/gitea/act_runner/pulls/262
- https://gitea.com/gitea/act/pulls/70
2023-06-30 15:26:36 +08:00
"github.com/stretchr/testify/require"
2023-06-14 11:42:38 +08:00
)
func Test_getStorageInheritNameSectionTypeForActions ( t * testing . T ) {
iniStr := `
[ storage ]
STORAGE_TYPE = minio
`
cfg , err := NewConfigProviderFromData ( iniStr )
2024-07-30 19:41:10 +00:00
require . NoError ( t , err )
require . NoError ( t , loadActionsFrom ( cfg ) )
2023-06-14 11:42:38 +08:00
assert . EqualValues ( t , "minio" , Actions . LogStorage . Type )
2025-03-28 22:22:21 +00:00
assert . Equal ( t , "actions_log/" , Actions . LogStorage . MinioConfig . BasePath )
2023-06-14 11:42:38 +08:00
assert . EqualValues ( t , "minio" , Actions . ArtifactStorage . Type )
2025-03-28 22:22:21 +00:00
assert . Equal ( t , "actions_artifacts/" , Actions . ArtifactStorage . MinioConfig . BasePath )
2023-06-14 11:42:38 +08:00
iniStr = `
[ storage . actions_log ]
STORAGE_TYPE = minio
`
cfg , err = NewConfigProviderFromData ( iniStr )
2024-07-30 19:41:10 +00:00
require . NoError ( t , err )
require . NoError ( t , loadActionsFrom ( cfg ) )
2023-06-14 11:42:38 +08:00
assert . EqualValues ( t , "minio" , Actions . LogStorage . Type )
2025-03-28 22:22:21 +00:00
assert . Equal ( t , "actions_log/" , Actions . LogStorage . MinioConfig . BasePath )
2023-06-14 11:42:38 +08:00
assert . EqualValues ( t , "local" , Actions . ArtifactStorage . Type )
2025-03-28 22:22:21 +00:00
assert . Equal ( t , "actions_artifacts" , filepath . Base ( Actions . ArtifactStorage . Path ) )
2023-06-14 11:42:38 +08:00
iniStr = `
[ storage . actions_log ]
STORAGE_TYPE = my_storage
[ storage . my_storage ]
STORAGE_TYPE = minio
`
cfg , err = NewConfigProviderFromData ( iniStr )
2024-07-30 19:41:10 +00:00
require . NoError ( t , err )
require . NoError ( t , loadActionsFrom ( cfg ) )
2023-06-14 11:42:38 +08:00
assert . EqualValues ( t , "minio" , Actions . LogStorage . Type )
2025-03-28 22:22:21 +00:00
assert . Equal ( t , "actions_log/" , Actions . LogStorage . MinioConfig . BasePath )
2023-06-14 11:42:38 +08:00
assert . EqualValues ( t , "local" , Actions . ArtifactStorage . Type )
2025-03-28 22:22:21 +00:00
assert . Equal ( t , "actions_artifacts" , filepath . Base ( Actions . ArtifactStorage . Path ) )
2023-06-14 11:42:38 +08:00
iniStr = `
[ storage . actions_artifacts ]
STORAGE_TYPE = my_storage
[ storage . my_storage ]
STORAGE_TYPE = minio
`
cfg , err = NewConfigProviderFromData ( iniStr )
2024-07-30 19:41:10 +00:00
require . NoError ( t , err )
require . NoError ( t , loadActionsFrom ( cfg ) )
2023-06-14 11:42:38 +08:00
assert . EqualValues ( t , "local" , Actions . LogStorage . Type )
2025-03-28 22:22:21 +00:00
assert . Equal ( t , "actions_log" , filepath . Base ( Actions . LogStorage . Path ) )
2023-06-14 11:42:38 +08:00
assert . EqualValues ( t , "minio" , Actions . ArtifactStorage . Type )
2025-03-28 22:22:21 +00:00
assert . Equal ( t , "actions_artifacts/" , Actions . ArtifactStorage . MinioConfig . BasePath )
2023-06-14 11:42:38 +08:00
iniStr = `
[ storage . actions_artifacts ]
STORAGE_TYPE = my_storage
[ storage . my_storage ]
STORAGE_TYPE = minio
`
cfg , err = NewConfigProviderFromData ( iniStr )
2024-07-30 19:41:10 +00:00
require . NoError ( t , err )
require . NoError ( t , loadActionsFrom ( cfg ) )
2023-06-14 11:42:38 +08:00
assert . EqualValues ( t , "local" , Actions . LogStorage . Type )
2025-03-28 22:22:21 +00:00
assert . Equal ( t , "actions_log" , filepath . Base ( Actions . LogStorage . Path ) )
2023-06-14 11:42:38 +08:00
assert . EqualValues ( t , "minio" , Actions . ArtifactStorage . Type )
2025-03-28 22:22:21 +00:00
assert . Equal ( t , "actions_artifacts/" , Actions . ArtifactStorage . MinioConfig . BasePath )
2023-06-14 11:42:38 +08:00
iniStr = ` `
cfg , err = NewConfigProviderFromData ( iniStr )
2024-07-30 19:41:10 +00:00
require . NoError ( t , err )
require . NoError ( t , loadActionsFrom ( cfg ) )
2023-06-14 11:42:38 +08:00
assert . EqualValues ( t , "local" , Actions . LogStorage . Type )
2025-03-28 22:22:21 +00:00
assert . Equal ( t , "actions_log" , filepath . Base ( Actions . LogStorage . Path ) )
2023-06-14 11:42:38 +08:00
assert . EqualValues ( t , "local" , Actions . ArtifactStorage . Type )
2025-03-28 22:22:21 +00:00
assert . Equal ( t , "actions_artifacts" , filepath . Base ( Actions . ArtifactStorage . Path ) )
2023-06-14 11:42:38 +08:00
}
Restrict `[actions].DEFAULT_ACTIONS_URL` to only `github` or `self` (#25581)
Resolve #24789
## :warning: BREAKING :warning:
Before this, `DEFAULT_ACTIONS_URL` cound be set to any custom URLs like
`https://gitea.com` or `http://your-git-server,https://gitea.com`, and
the default value was `https://gitea.com`.
But now, `DEFAULT_ACTIONS_URL` supports only
`github`(`https://github.com`) or `self`(the root url of current Gitea
instance), and the default value is `github`.
If it has configured with a URL, an error log will be displayed and it
will fallback to `github`.
Actually, what we really want to do is always make it
`https://github.com`, however, this may not be acceptable for some
instances of internal use, so there's extra support for `self`, but no
more, even `https://gitea.com`.
Please note that `uses: https://xxx/yyy/zzz` always works and it does
exactly what it is supposed to do.
Although it's breaking, I belive it should be backported to `v1.20` due
to some security issues.
Follow-up on the runner side:
- https://gitea.com/gitea/act_runner/pulls/262
- https://gitea.com/gitea/act/pulls/70
2023-06-30 15:26:36 +08:00
func Test_getDefaultActionsURLForActions ( t * testing . T ) {
oldActions := Actions
oldAppURL := AppURL
defer func ( ) {
Actions = oldActions
AppURL = oldAppURL
} ( )
AppURL = "http://test_get_default_actions_url_for_actions:3000/"
tests := [ ] struct {
name string
iniStr string
wantURL string
} {
{
name : "default" ,
iniStr : `
[ actions ]
` ,
2024-12-18 12:03:41 +01:00
wantURL : "https://data.forgejo.org" ,
Restrict `[actions].DEFAULT_ACTIONS_URL` to only `github` or `self` (#25581)
Resolve #24789
## :warning: BREAKING :warning:
Before this, `DEFAULT_ACTIONS_URL` cound be set to any custom URLs like
`https://gitea.com` or `http://your-git-server,https://gitea.com`, and
the default value was `https://gitea.com`.
But now, `DEFAULT_ACTIONS_URL` supports only
`github`(`https://github.com`) or `self`(the root url of current Gitea
instance), and the default value is `github`.
If it has configured with a URL, an error log will be displayed and it
will fallback to `github`.
Actually, what we really want to do is always make it
`https://github.com`, however, this may not be acceptable for some
instances of internal use, so there's extra support for `self`, but no
more, even `https://gitea.com`.
Please note that `uses: https://xxx/yyy/zzz` always works and it does
exactly what it is supposed to do.
Although it's breaking, I belive it should be backported to `v1.20` due
to some security issues.
Follow-up on the runner side:
- https://gitea.com/gitea/act_runner/pulls/262
- https://gitea.com/gitea/act/pulls/70
2023-06-30 15:26:36 +08:00
} ,
{
name : "github" ,
iniStr : `
[ actions ]
DEFAULT_ACTIONS_URL = github
` ,
wantURL : "https://github.com" ,
} ,
{
name : "self" ,
iniStr : `
[ actions ]
DEFAULT_ACTIONS_URL = self
` ,
wantURL : "http://test_get_default_actions_url_for_actions:3000" ,
} ,
{
name : "custom urls" ,
iniStr : `
[ actions ]
2023-07-03 09:45:12 +02:00
DEFAULT_ACTIONS_URL = https : //example.com
Restrict `[actions].DEFAULT_ACTIONS_URL` to only `github` or `self` (#25581)
Resolve #24789
## :warning: BREAKING :warning:
Before this, `DEFAULT_ACTIONS_URL` cound be set to any custom URLs like
`https://gitea.com` or `http://your-git-server,https://gitea.com`, and
the default value was `https://gitea.com`.
But now, `DEFAULT_ACTIONS_URL` supports only
`github`(`https://github.com`) or `self`(the root url of current Gitea
instance), and the default value is `github`.
If it has configured with a URL, an error log will be displayed and it
will fallback to `github`.
Actually, what we really want to do is always make it
`https://github.com`, however, this may not be acceptable for some
instances of internal use, so there's extra support for `self`, but no
more, even `https://gitea.com`.
Please note that `uses: https://xxx/yyy/zzz` always works and it does
exactly what it is supposed to do.
Although it's breaking, I belive it should be backported to `v1.20` due
to some security issues.
Follow-up on the runner side:
- https://gitea.com/gitea/act_runner/pulls/262
- https://gitea.com/gitea/act/pulls/70
2023-06-30 15:26:36 +08:00
` ,
2023-07-03 09:45:12 +02:00
wantURL : "https://example.com" ,
Restrict `[actions].DEFAULT_ACTIONS_URL` to only `github` or `self` (#25581)
Resolve #24789
## :warning: BREAKING :warning:
Before this, `DEFAULT_ACTIONS_URL` cound be set to any custom URLs like
`https://gitea.com` or `http://your-git-server,https://gitea.com`, and
the default value was `https://gitea.com`.
But now, `DEFAULT_ACTIONS_URL` supports only
`github`(`https://github.com`) or `self`(the root url of current Gitea
instance), and the default value is `github`.
If it has configured with a URL, an error log will be displayed and it
will fallback to `github`.
Actually, what we really want to do is always make it
`https://github.com`, however, this may not be acceptable for some
instances of internal use, so there's extra support for `self`, but no
more, even `https://gitea.com`.
Please note that `uses: https://xxx/yyy/zzz` always works and it does
exactly what it is supposed to do.
Although it's breaking, I belive it should be backported to `v1.20` due
to some security issues.
Follow-up on the runner side:
- https://gitea.com/gitea/act_runner/pulls/262
- https://gitea.com/gitea/act/pulls/70
2023-06-30 15:26:36 +08:00
} ,
}
for _ , tt := range tests {
t . Run ( tt . name , func ( t * testing . T ) {
cfg , err := NewConfigProviderFromData ( tt . iniStr )
require . NoError ( t , err )
2024-07-30 19:41:10 +00:00
require . NoError ( t , loadActionsFrom ( cfg ) )
2025-03-28 22:22:21 +00:00
assert . Equal ( t , tt . wantURL , Actions . DefaultActionsURL . URL ( ) )
Restrict `[actions].DEFAULT_ACTIONS_URL` to only `github` or `self` (#25581)
Resolve #24789
## :warning: BREAKING :warning:
Before this, `DEFAULT_ACTIONS_URL` cound be set to any custom URLs like
`https://gitea.com` or `http://your-git-server,https://gitea.com`, and
the default value was `https://gitea.com`.
But now, `DEFAULT_ACTIONS_URL` supports only
`github`(`https://github.com`) or `self`(the root url of current Gitea
instance), and the default value is `github`.
If it has configured with a URL, an error log will be displayed and it
will fallback to `github`.
Actually, what we really want to do is always make it
`https://github.com`, however, this may not be acceptable for some
instances of internal use, so there's extra support for `self`, but no
more, even `https://gitea.com`.
Please note that `uses: https://xxx/yyy/zzz` always works and it does
exactly what it is supposed to do.
Although it's breaking, I belive it should be backported to `v1.20` due
to some security issues.
Follow-up on the runner side:
- https://gitea.com/gitea/act_runner/pulls/262
- https://gitea.com/gitea/act/pulls/70
2023-06-30 15:26:36 +08:00
} )
}
}
2026-01-15 03:39:00 +01:00
func Test_getIDTokenSettingsForActions ( t * testing . T ) {
defer test . MockVariableValue ( & AppDataPath , "/home/app/data" ) ( )
oldActions := Actions
oldAppURL := AppURL
defer func ( ) {
Actions = oldActions
AppURL = oldAppURL
} ( )
iniStr := `
[ actions ]
`
cfg , err := NewConfigProviderFromData ( iniStr )
require . NoError ( t , err )
require . NoError ( t , loadActionsFrom ( cfg ) )
chore: unify signing key configuration across modules (#11194)
## Context
the three commits in this series are the first step towards the goal of removing the special casing around `JWT_SECRET`, which is used for various modules via `GetGeneralTokenSigningSecret()`. Ultimately, I want to work towards enabling seamless migration away from general use of the common secret. To enable this, we need proper secret/key rotation support, that is, we need to allow for configuration of additional secrets/keys which are accepted for token validation, but not used to issue tokens.
I have this _Verifier_ support basically implemented, but this PR is not it.
This PR contains cleanup refactoring which I worked on before writing the _Verifier_ support, because I noticed that the existing secret/key handling across modules was inconsistent and required duplicated code.
I am submitting this part now to allow for incremental review of not too large a diff, and because these commits remained unchanged during two weeks since I moved on the the next task.
## The problem being addressed
Configuration of JWT signing secrets/keys was inconsistent:
Under `[oauth2]` the full configuration set was supported:
- `JWT_SIGNING_ALGORITHM` configured the algorithm
- `JWT_SECRET` configured a literal secret for symmetric algorithms
- `JWT_SECRET_URI` configured a `file:` uri of a secret for symmetric algorithms
- `JWT_SIGNING_PRIVATE_KEY_FILE` configured a file for asymmetric algorithms
For `[server]`, the LFS module only supported `LFS_JWT_SECRET`, and the signing method was hardcoded to `HS256`
For `[actions]`, only asymmetric signing methods were supported via `ID_TOKEN_SIGNING_ALGORITHM` and `ID_TOKEN_SIGNING_PRIVATE_KEY_FILE`.
## ini unification
The proposed code centralizes ini parsing to always support the following ini keys:
- `[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]` is specific to the module and chosen to support the existing ini keys
Centralizing this code and unifying the ini keys will come handy for at least the following reasons:
- consistent behavior across modules is easier to understand
- less duplicated code
- easier to expand later, which is my main motivation
## implementation notes
as might be apparent by the _take3_ branch name, this is the third iteration of this patch series. The main reason why I abandoned the other two is that I first tried to move all the key initialization into the code called from settings.go when the ini file is parsed. But that lead to a lot of friction with test cases, because private key files which are configured, but do not exist will get created and hence require a writable `AppDataPath` and additional clean up.
To avoid a lot of noise and complications in test cases, I kept the existing two stage process, where
- the settings component creates missing symmetric signing keys and writes them to the .ini
- the settings component creates a simple configuration struct
- which is then used from the module init to create the actual key, which also includes creating a private key file if asymmetric crypto is configured and the key file does not exist.
I would have wished this patch was a net negative in terms of LOCs, but I hope it contributes to clarity and many added lines are in test cases.
## Commits
Because sometimes PRs are merged as squashes with the PR text remaining, I am repeating here the individual messages of the individual commits for future reference:
### Refactor signing key initalization and oauth2 use of it
This commit is the first in a series towards the goal of addressing the
FIXME comment in modules/setting/oauth2.go to remove
GeneralTokenSigningSecret
To do it properly, the task also requires addition of signing secret/key
rotation: We ultimately want to be able to change a signing key, but
continue to accept the previous one. This is particularly relevant to
offer a path from GeneralTokenSigningSecret aka JWT_SECRET to new,
specific component key configuration, where it should be possible to add
the former JWT_SECRET as a key accepted for verification to enable a
seamless transition.
This perspective, in turn, calls for refactoring of the existing secret
initialization code to centralize the common functions of parsing
signing key related configuration directives: The oauth2 module
currently is the only component accepting symmetric and asymmetric keys,
with the limitation of the symmetric key being also the
GeneralTokenSigningSecret. Other components either enforce HS256 or
public key algorithms.
We should really give the choice of algorithm selection and avoid code
duplication in other places, so this commit
- generalizes setting parsing into a configuration struct: A prefix can
be provided, with which the common configuration directives are
processed:
- [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
- which is then accepted by jwtx.InitSigningKey() to create an actual
signing key
The reasons for the two stage process are explained in a long-ish
comment in modules/setting/security.go. In short, other options would
either violate sensible module boundaries or cause too much friction.
These other options have actually been tried, this is take 3 of the
proposed changes.
### Refactor services/lfs: Change token code to use SigningKey
This now also enables use of token algorithms other than HS256.
In this case, signing key initialization also happens during settings
initialization, because LFS is also used in CLI commands.
### Refactor api/actions to use new signingkey API
This now also enables use of symmetric token algorithms.
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11194
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Co-authored-by: Nils Goroll <nils.goroll@uplex.de>
Co-committed-by: Nils Goroll <nils.goroll@uplex.de>
2026-04-21 19:39:33 +02:00
assert . Equal ( t , "RS256" , Actions . KeyCfg . Signing . Algorithm )
assert . Equal ( t , "/home/app/data/actions_id_token/private.pem" , * Actions . KeyCfg . Signing . PrivateKeyPath )
2026-01-15 03:39:00 +01:00
assert . EqualValues ( t , 3600 , Actions . IDTokenExpirationTime )
iniStr = `
[ actions ]
ID_TOKEN_SIGNING_ALGORITHM = ES256
ID_TOKEN_SIGNING_PRIVATE_KEY_FILE = / test / test . pem
ID_TOKEN_EXPIRATION_TIME = 120
`
cfg , err = NewConfigProviderFromData ( iniStr )
require . NoError ( t , err )
require . NoError ( t , loadActionsFrom ( cfg ) )
chore: unify signing key configuration across modules (#11194)
## Context
the three commits in this series are the first step towards the goal of removing the special casing around `JWT_SECRET`, which is used for various modules via `GetGeneralTokenSigningSecret()`. Ultimately, I want to work towards enabling seamless migration away from general use of the common secret. To enable this, we need proper secret/key rotation support, that is, we need to allow for configuration of additional secrets/keys which are accepted for token validation, but not used to issue tokens.
I have this _Verifier_ support basically implemented, but this PR is not it.
This PR contains cleanup refactoring which I worked on before writing the _Verifier_ support, because I noticed that the existing secret/key handling across modules was inconsistent and required duplicated code.
I am submitting this part now to allow for incremental review of not too large a diff, and because these commits remained unchanged during two weeks since I moved on the the next task.
## The problem being addressed
Configuration of JWT signing secrets/keys was inconsistent:
Under `[oauth2]` the full configuration set was supported:
- `JWT_SIGNING_ALGORITHM` configured the algorithm
- `JWT_SECRET` configured a literal secret for symmetric algorithms
- `JWT_SECRET_URI` configured a `file:` uri of a secret for symmetric algorithms
- `JWT_SIGNING_PRIVATE_KEY_FILE` configured a file for asymmetric algorithms
For `[server]`, the LFS module only supported `LFS_JWT_SECRET`, and the signing method was hardcoded to `HS256`
For `[actions]`, only asymmetric signing methods were supported via `ID_TOKEN_SIGNING_ALGORITHM` and `ID_TOKEN_SIGNING_PRIVATE_KEY_FILE`.
## ini unification
The proposed code centralizes ini parsing to always support the following ini keys:
- `[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]` is specific to the module and chosen to support the existing ini keys
Centralizing this code and unifying the ini keys will come handy for at least the following reasons:
- consistent behavior across modules is easier to understand
- less duplicated code
- easier to expand later, which is my main motivation
## implementation notes
as might be apparent by the _take3_ branch name, this is the third iteration of this patch series. The main reason why I abandoned the other two is that I first tried to move all the key initialization into the code called from settings.go when the ini file is parsed. But that lead to a lot of friction with test cases, because private key files which are configured, but do not exist will get created and hence require a writable `AppDataPath` and additional clean up.
To avoid a lot of noise and complications in test cases, I kept the existing two stage process, where
- the settings component creates missing symmetric signing keys and writes them to the .ini
- the settings component creates a simple configuration struct
- which is then used from the module init to create the actual key, which also includes creating a private key file if asymmetric crypto is configured and the key file does not exist.
I would have wished this patch was a net negative in terms of LOCs, but I hope it contributes to clarity and many added lines are in test cases.
## Commits
Because sometimes PRs are merged as squashes with the PR text remaining, I am repeating here the individual messages of the individual commits for future reference:
### Refactor signing key initalization and oauth2 use of it
This commit is the first in a series towards the goal of addressing the
FIXME comment in modules/setting/oauth2.go to remove
GeneralTokenSigningSecret
To do it properly, the task also requires addition of signing secret/key
rotation: We ultimately want to be able to change a signing key, but
continue to accept the previous one. This is particularly relevant to
offer a path from GeneralTokenSigningSecret aka JWT_SECRET to new,
specific component key configuration, where it should be possible to add
the former JWT_SECRET as a key accepted for verification to enable a
seamless transition.
This perspective, in turn, calls for refactoring of the existing secret
initialization code to centralize the common functions of parsing
signing key related configuration directives: The oauth2 module
currently is the only component accepting symmetric and asymmetric keys,
with the limitation of the symmetric key being also the
GeneralTokenSigningSecret. Other components either enforce HS256 or
public key algorithms.
We should really give the choice of algorithm selection and avoid code
duplication in other places, so this commit
- generalizes setting parsing into a configuration struct: A prefix can
be provided, with which the common configuration directives are
processed:
- [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
- which is then accepted by jwtx.InitSigningKey() to create an actual
signing key
The reasons for the two stage process are explained in a long-ish
comment in modules/setting/security.go. In short, other options would
either violate sensible module boundaries or cause too much friction.
These other options have actually been tried, this is take 3 of the
proposed changes.
### Refactor services/lfs: Change token code to use SigningKey
This now also enables use of token algorithms other than HS256.
In this case, signing key initialization also happens during settings
initialization, because LFS is also used in CLI commands.
### Refactor api/actions to use new signingkey API
This now also enables use of symmetric token algorithms.
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11194
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Co-authored-by: Nils Goroll <nils.goroll@uplex.de>
Co-committed-by: Nils Goroll <nils.goroll@uplex.de>
2026-04-21 19:39:33 +02:00
assert . Equal ( t , "ES256" , Actions . KeyCfg . Signing . Algorithm )
assert . Equal ( t , "/test/test.pem" , * Actions . KeyCfg . Signing . PrivateKeyPath )
2026-01-15 03:39:00 +01:00
assert . EqualValues ( t , 120 , Actions . IDTokenExpirationTime )
iniStr = `
[ actions ]
ID_TOKEN_SIGNING_ALGORITHM = EdDSA
ID_TOKEN_SIGNING_PRIVATE_KEY_FILE = . / test / test . pem
ID_TOKEN_EXPIRATION_TIME = 123
`
cfg , err = NewConfigProviderFromData ( iniStr )
require . NoError ( t , err )
require . NoError ( t , loadActionsFrom ( cfg ) )
chore: unify signing key configuration across modules (#11194)
## Context
the three commits in this series are the first step towards the goal of removing the special casing around `JWT_SECRET`, which is used for various modules via `GetGeneralTokenSigningSecret()`. Ultimately, I want to work towards enabling seamless migration away from general use of the common secret. To enable this, we need proper secret/key rotation support, that is, we need to allow for configuration of additional secrets/keys which are accepted for token validation, but not used to issue tokens.
I have this _Verifier_ support basically implemented, but this PR is not it.
This PR contains cleanup refactoring which I worked on before writing the _Verifier_ support, because I noticed that the existing secret/key handling across modules was inconsistent and required duplicated code.
I am submitting this part now to allow for incremental review of not too large a diff, and because these commits remained unchanged during two weeks since I moved on the the next task.
## The problem being addressed
Configuration of JWT signing secrets/keys was inconsistent:
Under `[oauth2]` the full configuration set was supported:
- `JWT_SIGNING_ALGORITHM` configured the algorithm
- `JWT_SECRET` configured a literal secret for symmetric algorithms
- `JWT_SECRET_URI` configured a `file:` uri of a secret for symmetric algorithms
- `JWT_SIGNING_PRIVATE_KEY_FILE` configured a file for asymmetric algorithms
For `[server]`, the LFS module only supported `LFS_JWT_SECRET`, and the signing method was hardcoded to `HS256`
For `[actions]`, only asymmetric signing methods were supported via `ID_TOKEN_SIGNING_ALGORITHM` and `ID_TOKEN_SIGNING_PRIVATE_KEY_FILE`.
## ini unification
The proposed code centralizes ini parsing to always support the following ini keys:
- `[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]` is specific to the module and chosen to support the existing ini keys
Centralizing this code and unifying the ini keys will come handy for at least the following reasons:
- consistent behavior across modules is easier to understand
- less duplicated code
- easier to expand later, which is my main motivation
## implementation notes
as might be apparent by the _take3_ branch name, this is the third iteration of this patch series. The main reason why I abandoned the other two is that I first tried to move all the key initialization into the code called from settings.go when the ini file is parsed. But that lead to a lot of friction with test cases, because private key files which are configured, but do not exist will get created and hence require a writable `AppDataPath` and additional clean up.
To avoid a lot of noise and complications in test cases, I kept the existing two stage process, where
- the settings component creates missing symmetric signing keys and writes them to the .ini
- the settings component creates a simple configuration struct
- which is then used from the module init to create the actual key, which also includes creating a private key file if asymmetric crypto is configured and the key file does not exist.
I would have wished this patch was a net negative in terms of LOCs, but I hope it contributes to clarity and many added lines are in test cases.
## Commits
Because sometimes PRs are merged as squashes with the PR text remaining, I am repeating here the individual messages of the individual commits for future reference:
### Refactor signing key initalization and oauth2 use of it
This commit is the first in a series towards the goal of addressing the
FIXME comment in modules/setting/oauth2.go to remove
GeneralTokenSigningSecret
To do it properly, the task also requires addition of signing secret/key
rotation: We ultimately want to be able to change a signing key, but
continue to accept the previous one. This is particularly relevant to
offer a path from GeneralTokenSigningSecret aka JWT_SECRET to new,
specific component key configuration, where it should be possible to add
the former JWT_SECRET as a key accepted for verification to enable a
seamless transition.
This perspective, in turn, calls for refactoring of the existing secret
initialization code to centralize the common functions of parsing
signing key related configuration directives: The oauth2 module
currently is the only component accepting symmetric and asymmetric keys,
with the limitation of the symmetric key being also the
GeneralTokenSigningSecret. Other components either enforce HS256 or
public key algorithms.
We should really give the choice of algorithm selection and avoid code
duplication in other places, so this commit
- generalizes setting parsing into a configuration struct: A prefix can
be provided, with which the common configuration directives are
processed:
- [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
- which is then accepted by jwtx.InitSigningKey() to create an actual
signing key
The reasons for the two stage process are explained in a long-ish
comment in modules/setting/security.go. In short, other options would
either violate sensible module boundaries or cause too much friction.
These other options have actually been tried, this is take 3 of the
proposed changes.
### Refactor services/lfs: Change token code to use SigningKey
This now also enables use of token algorithms other than HS256.
In this case, signing key initialization also happens during settings
initialization, because LFS is also used in CLI commands.
### Refactor api/actions to use new signingkey API
This now also enables use of symmetric token algorithms.
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11194
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Co-authored-by: Nils Goroll <nils.goroll@uplex.de>
Co-committed-by: Nils Goroll <nils.goroll@uplex.de>
2026-04-21 19:39:33 +02:00
assert . Equal ( t , "EdDSA" , Actions . KeyCfg . Signing . Algorithm )
assert . Equal ( t , "/home/app/data/test/test.pem" , * Actions . KeyCfg . Signing . PrivateKeyPath )
2026-01-15 03:39:00 +01:00
assert . EqualValues ( t , 123 , Actions . IDTokenExpirationTime )
iniStr = `
[ actions ]
ID_TOKEN_SIGNING_ALGORITHM = HS256
`
cfg , err = NewConfigProviderFromData ( iniStr )
require . NoError ( t , err )
err = loadActionsFrom ( cfg )
chore: unify signing key configuration across modules (#11194)
## Context
the three commits in this series are the first step towards the goal of removing the special casing around `JWT_SECRET`, which is used for various modules via `GetGeneralTokenSigningSecret()`. Ultimately, I want to work towards enabling seamless migration away from general use of the common secret. To enable this, we need proper secret/key rotation support, that is, we need to allow for configuration of additional secrets/keys which are accepted for token validation, but not used to issue tokens.
I have this _Verifier_ support basically implemented, but this PR is not it.
This PR contains cleanup refactoring which I worked on before writing the _Verifier_ support, because I noticed that the existing secret/key handling across modules was inconsistent and required duplicated code.
I am submitting this part now to allow for incremental review of not too large a diff, and because these commits remained unchanged during two weeks since I moved on the the next task.
## The problem being addressed
Configuration of JWT signing secrets/keys was inconsistent:
Under `[oauth2]` the full configuration set was supported:
- `JWT_SIGNING_ALGORITHM` configured the algorithm
- `JWT_SECRET` configured a literal secret for symmetric algorithms
- `JWT_SECRET_URI` configured a `file:` uri of a secret for symmetric algorithms
- `JWT_SIGNING_PRIVATE_KEY_FILE` configured a file for asymmetric algorithms
For `[server]`, the LFS module only supported `LFS_JWT_SECRET`, and the signing method was hardcoded to `HS256`
For `[actions]`, only asymmetric signing methods were supported via `ID_TOKEN_SIGNING_ALGORITHM` and `ID_TOKEN_SIGNING_PRIVATE_KEY_FILE`.
## ini unification
The proposed code centralizes ini parsing to always support the following ini keys:
- `[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]` is specific to the module and chosen to support the existing ini keys
Centralizing this code and unifying the ini keys will come handy for at least the following reasons:
- consistent behavior across modules is easier to understand
- less duplicated code
- easier to expand later, which is my main motivation
## implementation notes
as might be apparent by the _take3_ branch name, this is the third iteration of this patch series. The main reason why I abandoned the other two is that I first tried to move all the key initialization into the code called from settings.go when the ini file is parsed. But that lead to a lot of friction with test cases, because private key files which are configured, but do not exist will get created and hence require a writable `AppDataPath` and additional clean up.
To avoid a lot of noise and complications in test cases, I kept the existing two stage process, where
- the settings component creates missing symmetric signing keys and writes them to the .ini
- the settings component creates a simple configuration struct
- which is then used from the module init to create the actual key, which also includes creating a private key file if asymmetric crypto is configured and the key file does not exist.
I would have wished this patch was a net negative in terms of LOCs, but I hope it contributes to clarity and many added lines are in test cases.
## Commits
Because sometimes PRs are merged as squashes with the PR text remaining, I am repeating here the individual messages of the individual commits for future reference:
### Refactor signing key initalization and oauth2 use of it
This commit is the first in a series towards the goal of addressing the
FIXME comment in modules/setting/oauth2.go to remove
GeneralTokenSigningSecret
To do it properly, the task also requires addition of signing secret/key
rotation: We ultimately want to be able to change a signing key, but
continue to accept the previous one. This is particularly relevant to
offer a path from GeneralTokenSigningSecret aka JWT_SECRET to new,
specific component key configuration, where it should be possible to add
the former JWT_SECRET as a key accepted for verification to enable a
seamless transition.
This perspective, in turn, calls for refactoring of the existing secret
initialization code to centralize the common functions of parsing
signing key related configuration directives: The oauth2 module
currently is the only component accepting symmetric and asymmetric keys,
with the limitation of the symmetric key being also the
GeneralTokenSigningSecret. Other components either enforce HS256 or
public key algorithms.
We should really give the choice of algorithm selection and avoid code
duplication in other places, so this commit
- generalizes setting parsing into a configuration struct: A prefix can
be provided, with which the common configuration directives are
processed:
- [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
- which is then accepted by jwtx.InitSigningKey() to create an actual
signing key
The reasons for the two stage process are explained in a long-ish
comment in modules/setting/security.go. In short, other options would
either violate sensible module boundaries or cause too much friction.
These other options have actually been tried, this is take 3 of the
proposed changes.
### Refactor services/lfs: Change token code to use SigningKey
This now also enables use of token algorithms other than HS256.
In this case, signing key initialization also happens during settings
initialization, because LFS is also used in CLI commands.
### Refactor api/actions to use new signingkey API
This now also enables use of symmetric token algorithms.
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11194
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Co-authored-by: Nils Goroll <nils.goroll@uplex.de>
Co-committed-by: Nils Goroll <nils.goroll@uplex.de>
2026-04-21 19:39:33 +02:00
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" )
2026-01-15 03:39:00 +01:00
}