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:

• Only load SECRET_KEY and INTERNAL_TOKEN if they exist.
• Never write the config file if the keys do not exist, which was only a fallback for Gitea upgraded from < 1.5

[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)

[wxiaoguang]

@techknowlogick no objection from my side.

My thought is that CreateOrAppendToCustomConf shouldn't be called inside loadSecret. All secrets should be prepared (generated) carefully before loading them, then the config file can be put in read-only mode.

Since it's security related, let more maintainers review.

[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]

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

[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.

• Revert: auto generate INTERNAL_TOKEN #21608

[wxiaoguang]

Another bug:

• Fix token generation when using INTERNAL_TOKEN_URI #21669

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