... from #11194 / 0034e55965
Revert a test code change left over from an intermediate development step which is not needed, because the LFS JWT config is tested in lfs.TestAuthenticate()
Fixes#12263
## Checklist
The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. All work and communication must conform to Forgejo's [AI Agreement](https://codeberg.org/forgejo/governance/src/branch/main/AIAgreement.md). There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org).
### Tests for Go changes
(can be removed for JavaScript changes)
- I added test coverage for Go changes...
- [X] in their respective `*_test.go` for unit tests.
- [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server.
- I ran...
- [X] `make pr-go` before pushing
### Documentation
- [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change.
- [X] I did not document these changes and I do not expect someone else to do it.
### Release notes
- [ ] This change will be noticed by a Forgejo user or admin (feature, bug fix, performance, etc.). I suggest to include a release note for this change.
- [X] This change is not visible to a Forgejo user or admin (refactor, dependency upgrade, etc.). I think there is no need to add a release note for this change.
*The decision if the pull request will be shown in the release notes is up to the mergers / release team.*
The content of the `release-notes/<pull request number>.md` file will serve as the basis for the release notes. If the file does not exist, the title of the pull request will be used instead.
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12281
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
## 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>
This is based on https://code.forgejo.org/go-chi/session/pulls/80.
The remainder of this message is largely copied from there:
For interoperability with reverse proxies and CDNs, setting a session
cookie for no good reason (login is a good reason) is a PITA, because it
makes caching of content for anonymous (not logged-in) users very hard,
requiring all kinds of special casing and error prone workarounds.
In particular in an age of exploitative AI bot crawling, being able to
serve content for anonymous users from a fast, efficient page cache is
an important option.
This patch lays a foundation by using an option added to go-chi/session
to not create session cookies always, but rather only when the
respective session is non-empty.
Test cases are included there and omitted here.
This is to have a simple and consistent make target for developers to locally run the checks which the CI will also run. The goal is to avoid wasting review cycles on CI failures.
To have a single source of truth, the CI is adjusted to call the same make target. Additional checks should no longer be added to the CI workflow, but rather to the makefile.
The pull request template is adjusted to remind of running this make target.
CI output is improved by using a simple bash script which uses sed to add `##[group]` tags to "make --debug=b" output and filter out messages which usually do not contribute to understanding. While this approach has limits and depends on the specific debug output format of GNU make, it avoids adjusting the makefile itself for additional CI beautification, contributing to maintainability.
- no tests needed (this is purely a build change)
- docs: hint added to PR template
- no release notes needed
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11053
Reviewed-by: Michael Kriese <michael.kriese@gmx.de>
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>
The virtual session code creates an in-memory session, and only upon release does it copy it to the actual session store. This makes a lot of sense to avoid operations on session stores with potentially high cost for I/O.
This commit removes a weird hack used in this code: virtual sessions were always created with an _old_uid=0 key/value pair, which was taken into account when checking if the session needed to be persisted.
As I could not find _any_ use of _old_uid in the code base, this looks like something worth removing.
The first ever mention of _old_uid is b33f7f792b and even there it is part of a
newly added file with no additional information. So likely code copied over from another project?
- no tests to add, remove or change
- not relevant for documentation
- not relevant for release notes
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11277
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>
This slightly simplifies calling code by centralizing the common 3-liner to create a JWT from claims, signed by a key.
But more importantly, it reduces the risk of `key.PreProcessToken()` being forgotten, which will become relevant in upcoming PRs:
`key.PreProcessToken()` adds the key id to the JWT header, which is important to efficiently validate tokens when multiple validation keys are supported (that is not the case yet)
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11067
Co-authored-by: Nils Goroll <nils.goroll@uplex.de>
Co-committed-by: Nils Goroll <nils.goroll@uplex.de>
This patch only moves code around and splits out two functions from
loadOrCreateAsymmetricKey(). The diff is best viewed with -b to ignore
white space (indentation) changes.
This is done for two reasons:
- For future additions, we will need loadAsymmetricKey() only, without
the create
- The doubly nested immediately invoked closure construction was not
exactly helping clarity
golang-jwt/jwt already has a GetSigningMethod() function which we should
use to ensure that our signing methods are actually registered.
Yet we should also keep our own check against a set of allowed methods
such that we do not accidentally accept methods which we are not
prepared to support.
The module calling `log.Fatal()` (which terminates the process) prevents the calling function to enrich the error message with vital information allowing the user to track down problematic configuration directives. Also this was impeding unit tests.
One such case is where the path to the specified key can not be created, as demonstrated in the test case. Here the error message is:
```
Error while loading or creating JWT key: Error generating private key ...: mkdir ...: permission denied
```
`log.Fatal()` is kept for `f.Close()` errors which indicate much more severe but very rare underlying issues. Handling these would require broader changes.
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11066
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>
This PR is in preparation of, but independent of, an upcoming suggestion for a feature addition:
* The first commit moves a tiny bit of logic into a separate function to prepare for extension of that logic, avoiding duplication
* The second commit moves checking for disabled registrations earlier, which, I think, has merits in terms of performance and resilience (hopefully not significant, but who knows?)
* The third commit adds simple unit tests for SignUp() and SignUpPost() to avoid the long-ish roundtrip over integration tests
* The forth commit introduces `ctx.Data["DisableRegistrationReason"]` for the signup template to use as the reason printed if `.DisableRegistration` to prepare for other reasons to be added
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/10915
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>
For the previous code with the Page attribute present in
ListCursorOptions for page 1, github would not return an "After" cursor,
such that the request for page 2 would request what effectively is the
content of page 1 a second time.
This would lead to an attempt to insert the same issues twice.
Note that this is not the only reason why this can happen with the
current code base.
We fix this particular issue by not using the Page attribute so github
does return an "After" cursor.
Fixes#10794
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/10798
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>
This is a successor to #10805, which simply did not work. It is also much simpler and basically a one line change to enable an existing feature in [go-github](https://github.com/google/go-github).
Fixes#10845
With this fix and #10798 in place, a migration of a repo with ~3K issues and ~1.3k pull requests finally completed successfully.
## Patch
We use SleepUntilPrimaryRateLimitResetWhenRateLimited to instruct the go-github code to wait until the retry time and retry the request when the primary rate limit gets hit.
## Test case
TestGitHubDownloadRepo() has been modified such that 403 rate limit errors are injected every 7 requests with a retry time of one second, resulting in the rate limit condition being hit twice with the current tests. The test case confirms that the migration code itself is in fact unaffected by the rate limit being hit.
## Scope
This change does not affect secondary rate limits.
If the server is restarted during the wait for the rate limit refresh, the migration likely still fails when retried, because inserts for already present database objects will be attempted.
This approach effectively puts the task's goroutine to sleep until the retry time, which implies that the respective resources stay allocated.
A better approach might be to add the necessary infrastructure to support restarts of migration tasks at a later time, but this is much more involved, because the migration state would need to be saved and/or re-created based on already pulled data. This would also require adding support for database upserts.
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/10846
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>
The two commits in this PR fix test cases which did not work as intended, because `require.Errorf()` was used where `require.ErrorContains()` was intended.
The former formats the error output by the test itself when failing, while the latter is to expect the tested error to contain some expected string.
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11035
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>
Related to #11035, this test case used `require.Errorf()` where it could use `require.Error()` to make it clear that this is not a case of confusion with `require.ErrorContains()`
The other cases in the same function already use `require.Error()`, so this clearly looks like a consistency glitch.
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11037
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>
Found while working on https://codeberg.org/forgejo/forgejo/pulls/10798#issuecomment-10083846: The symptom was that the go-github client never returned a `resp.After`, so I tracked down the root cause, which was that, with the mocked http server ...
Mocked headers never reached the calling client, because w.WriteHeader()
was called before the headers were set in the response.
Fix by moving w.WriteHeader() to the right place just before w.Write(),
which writes the body.
Test added which fails without the fix and succeeds with it.
### Tests
- I added test coverage for Go changes...
- [X] in their respective `*_test.go` for unit tests.
- [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server.
### Documentation
- [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change.
- [X] I did not document these changes and I do not expect someone else to do it.
### Release notes
- [ ] This change will be noticed by a Forgejo user or admin (feature, bug fix, performance, etc.). I suggest to include a release note for this change.
- [X] This change is not visible to a Forgejo user or admin (refactor, dependency upgrade, etc.). I think there is no need to add a release note for this change.
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11007
Reviewed-by: Michael Kriese <michael.kriese@gmx.de>
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
Co-authored-by: Nils Goroll <nils.goroll@uplex.de>
Co-committed-by: Nils Goroll <nils.goroll@uplex.de>
Using node head built from source, "make frontend" failed with this confusing message:
```
$ make node-check
/bin/sh: 1: printf: 0-pre: not completely converted
Forgejo requires Node.js 20.0.0 or greater and npm to build. You can get it at https://nodejs.org/en/download/
make: *** [Makefile:327: node-check] Error 1
```
The reason is that from this output
```
$ node -v
v26.0.0-pre
```
`0-pre` could not be converted to the printf `"%d"` format.
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/10790
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
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>
