Allow specifying SECRET_KEY_URI, similar to INTERNAL_TOKEN_URI

[clarfonthey]

This is a decent part of the work needed for #16603. While it does not allow setting all secrets via URI, it refactors the existing INTERNAL_TOKEN_URI code to add a new SECRET_KEY_URI setting. In theory, this code could be modified further to allow additional settings.

This modifies the existing internal token loading and the new secret key loading such that:

• Specifying both INTERNAL_TOKEN and INTERNAL_TOKEN_URI is an error, instead of just preferring the URI. (Technically breaking, but probably no one with a proper configuration is doing this.)
• The install lock isn't required to regenerate the internal token or the secret key; it will always be regenerated if it's absent.

[lunny]

We need a more abstract layer to read secrets from ini file, envs, docker secrets, k8s secrets.

[techknowlogick]

We need a more abstract layer to read secrets from ini file, envs, docker secrets, k8s secrets.

This would allow that in the future, as we could add support for new URI scheme (eg, http://, s3://, vault://, etc..)

[clarfonthey]

Yeah, the biggest barrier I'd see to using this for other kinds of secrets is that there are secrets which work in pairs which would have to be generated and supplied in pairs, and this doesn't really allow for that. Secrets which must always be supplied and cannot be generated (e.g. minio credentials) could just always return an error in the generator, however.

Some more work is needed still, but this is still progress IMHO.

[lunny]

We need a more abstract layer to read secrets from ini file, envs, docker secrets, k8s secrets.

This would allow that in the future, as we could add support for new URI scheme (eg, http://, s3://, vault://, etc..)

Yes. I mean function loadSecret's parameters couldn't support that. It has to be changed now or future.

[clarfonthey]

I'm not quite sure what you mean by it not being supported by the current signature.

All the signature requires is:

1. What key in the config could directly contain the secret
2. What key in the config could contain a URI locating the secret
3. A function to generate the secret, if it is not found in the config directly or at a given URI

With this formulation, we simply need to change the underlying URI logic to support more kinds of URIs, and we gain access to the ability to parse secrets at those locations automatically. For example, if we supported s3:// URIs in the loadSecret function, then automatically, we'd gain the ability to load secrets directly from S3, and the ability to generate and store the secrets in S3 if not found at the given URI.

[techknowlogick]

It would certainly need to be changed in the future (for the user/pass signature), but this brings existing functionality to another component, PLUS makes it more generic way of calling the function. As such I'm very happy to give a LGTM (and bot will accept this my approval). I think perhaps even the next place this could even go, now that it's much more generic is the database credentials (I suspect with this PR, the next one wouldn't be much to change to add that in)

[clarfonthey]

I agree that the default shouldn't be to put secrets into the config file, although at least for now, I'm just matching the existing behaviour.

Although, we could probably modify the default to generate a separate file instead, using that file for the secret. This has both the benefit of working with a read-only config file (we can assume a default URI) and also avoids storing the secret in the file. Longer-term we can also default the install process to doing this as well, but that will have to happen in another PR.

[zeripath]

OK I think there's a simple solution that will allow docker secrets etc to just work immediately.

We just allow people to specify a secondary ini file to read from. In the case of docker secrets you would put an ini file with the secrets in it e.g.: /run/secrets/gitea.ini

Then we add an additional option to the ini, ADDITIONAL_CONFIGURATION_FILE, which will be used when reading the ini file to read another ini file to overlay.

APP_NAME = Gitea: Git with a cup of tea
RUN_USER = gitea
RUN_MODE = dev
ADDITIONAL_CONFIGURATION_FILE = /run/secrets/gitea.ini

...

[clarfonthey]

@zeripath, two things. One, that feature is tracked in #4860 and separate to this. Two, I'm not sure how loading a config file is more appropriate than just loading the secrets from a separate file; AFAIK, docker secrets are just mounted files that you can read from, and I would imagine that having separated secret files would be better.

[wxiaoguang]

@clarfonthey I made some small changes, please help to review whether it's correct. I will vote my L-G-T-M.

[clarfonthey]

Thank you @wxiaoguang, it looks good to me!

[wxiaoguang]

Sorry for bothering, after some tests, I think we need a new big change 😂

The reason is: the code in: loadFromConf->loadSecret shouldn't write the config file unconditionally, because sometimes then loadFromConf is called when the config file doesn't exist.

The old code checked InstallLock to make sure the config file already existed and it was for a installed instance, and as a fallback for Gitea upgraded from < 1.5

To make things clear, the new code we can just make loadSecret only load secrets. Because: since 1.5 (very old), the InternalToken should be always there then InstallLock is true, no fallback (generate a new token) is needed anymore

(made some force-pushes to fix doc and lint, make all new changes in the same commit)

[wxiaoguang]

If no more changes, I think it could be merged in 2 days.

[codecov-commenter]

Codecov Report

Merging #19663 (febf582) into main (08609d4) will increase coverage by 47.07%. The diff coverage is 21.21%.

@@            Coverage Diff            @@
##           main   #19663       +/-   ##
=========================================
+ Coverage      0   47.07%   +47.07%     
=========================================
  Files         0     1017     +1017     
  Lines         0   138549   +138549     
=========================================
+ Hits          0    65224    +65224     
- Misses        0    65360    +65360     
- Partials      0     7965     +7965     

Impacted Files	Coverage Δ
cmd/web.go	0.00% <0.00%> (ø)
modules/setting/lfs.go	65.71% <0.00%> (ø)
routers/private/internal.go	77.35% <0.00%> (ø)
services/auth/source/oauth2/jwtsigningkey.go	27.03% <0.00%> (ø)
modules/setting/setting.go	50.48% <28.00%> (ø)
services/pull/merge.go	50.13% <0.00%> (ø)
services/webhook/telegram.go	80.53% <0.00%> (ø)
modules/storage/helper.go	20.58% <0.00%> (ø)
modules/process/manager.go	97.11% <0.00%> (ø)
routers/web/admin/packages.go	0.00% <0.00%> (ø)
... and 1012 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[clarfonthey]

Great work @wxiaoguang, thank you!

[wxiaoguang]

Update: not sure whether we should still support auto-generating INTERNAL_TOKEN if it's empty when INSTALL_LOCK=true.

A user in discord reported that when they deploy a cluster, they depend on the auto-generating behavior.

Although I think it's not good to do the auto-generate, not sure whether we should revert the behavior.

• https://github.com/go-gitea/gitea/pull/21608

[wxiaoguang]

Another bug:

• https://github.com/go-gitea/gitea/pull/21669

That case was not found before ......