WIP: Introduce general web secret

[wxiaoguang]

Follow #29205
Fix #30923

⚠️ BREAKING ⚠️

No action is required for most users, Gitea will generate a proper secret when upgrading.

---

[oauth2].JWT_SECRET and [server].LFS_JWT_SECRET are removed, Gitea will generate a new [security].GENERAL_WEB_SECRET automatically.

If you'd like to keep the existing LFS signed tokens work, you could also use the old LFS_JWT_SECRET as GENERAL_WEB_SECRET.

If there are multiple processes (containers) for one Gitea instance, the config option GENERAL_WEB_SECRET should be the same in all config files.

[silverwind]

Will this write the new secret into the config file or how does it work? Likely we can not write anything to config files in scenarios like when the config is from env vars only.

[wxiaoguang]

Will this write the new secret into the config file or how does it work? Likely we can not write anything to config files in scenarios like when the config is from env vars only..

There is no different from the old INTERNAL_TOKEN / JWT_SECRET (see their code)

• If there is a value from env, then the env value should have been processed ahead and it won't be generated again.

What this PR does is only merging the JWT_SECRET / LFS_JWT_SECRET into one, and make the new GENERAL_WEB_SECRET independent from any other feature.

[silverwind]

Name it WEB_SECRET instead of GENERAL_WEB_SECRET? The prefix seems pointless to me.

[wxiaoguang]

Name it WEB_SECRET instead of GENERAL_WEB_SECRET? The prefix seems pointless to me.

I think general is important here. In the future maybe we'd like to introduce other "web secrets", the prefix makes it easier to distinguish them.

---

There was already a bad example: JWT_SECRET vs LFS_JWT_SECRET .

[KN4CK3R]

As I have written in https://gitea.com/gitea/gitea-security/issues/30 I think we should use a simple plain format in the readme. The base64 value can't be written without tool support. Alowing a simple value like 546dztvuG(/&T%ZTRFCB can be "generated" by smashing on your keyboard. We can hash/transform this value to create a well defined we need for the secret generation.

[techknowlogick]

@wxiaoguang I removed backport as this is breaking, and we have already a release candidate

[wxiaoguang]

@wxiaoguang I removed backport as this is breaking, and we have already a release candidate

I think it should be backported because it is not quite breaking and it indeed is a bug fix.

[wxiaoguang]

As I have written in https://gitea.com/gitea/gitea-security/issues/30 I think we should use a simple plain format in the readme. The base64 value can't be written without tool support. Alowing a simple value like 546dztvuG(/&T%ZTRFCB can be "generated" by smashing on your keyboard. We can hash/transform this value to create a well defined we need for the secret generation.

I could agree. But for this case, the value is automatically generated, so I think end users do not need to manually touch it?

[KN4CK3R]

Nothing stops us from generating just a random string without underlaying format.

[wxiaoguang]

Nothing stops us from generating just a random string without underlaying format.

Then another question, should we stop end users if they use SECRET=123456 ?

[CirnoT]

@wxiaoguang I removed backport as this is breaking, and we have already a release candidate

I think it should be backported because it is not quite breaking and it indeed is a bug fix.

So you're saying that invalidating all OAuth2 tokens is not a breaking change and can go into patch release? I think your users might have a different idea about that.

Also, this PR begs the question - why? The PR explicitly lowers security of any instance by consolidating two separate secret tokens used for different purposes into one. It is now impossible to update one token while keeping other intact, which I would consider a valid use-case. The proposed changes neither simplify code nor do anything beyond being a change for a sake of change, which raises some eyebrows considering you're touching and refactoring security-related code.

[wxiaoguang]

Thank you for the questions. I will try to answer them by my understanding, correct me if I am wrong.

I am not a native speaker so please understand that I could only explain my understanding directly.

So you're saying that invalidating all OAuth2 tokens is not a breaking change and can go into patch release? I think your users might have a different idea about that.

I have marked it as "breaking", while I think it it acceptable for 1.22 because:

1. The 1.22 release is still in RC period, it could have necessary breaking changes, it is not a "patch release"
2. The tokens are invalided, users could re-grant the access tokens and it doesn't really block the daily usage.

I could agree that the change is not friendly and thank you for pointing out, maybe we need to make it more friendly to end users, eg: support rotating (multiple secrets) or keep using the old secret.

Also, this PR begs the question - why? The PR explicitly lowers security of any instance by consolidating two separate secret tokens used for different purposes into one. It is now impossible to update one token while keeping other intact, which I would consider a valid use-case. The proposed changes neither simplify code nor do anything beyond being a change for a sake of change.

In my mind, I don't think it "lowers". There are pretty many uses the for "general web secrets", see the comments: not only JWT, but also CSRF token, email validation, etc. Do you prefer to introduce dozens of "secrets" for them to "harden"? TBH I do not see the necessity at the moment.

Ideally in the future Gitea should support "secret" rotating and manage these secrets automatically, then every concern could be resolved, I think this could be considered as the first step: to reduce unnecessary "secrets".

which raises some eyebrows considering you're touching and refactoring security-related code.

This PR is not really security-related. The real security-related one is #29205 , it has been discussed by many maintainers. It is a quick patch and has been backported to 1.21, but it does have a problem: if oauth2 is disabled, then no persistent secrets between restarts and instances. (I know 29205 is not ideal either, but indeed it seems that there is no other choice)

If you are interested in the security related topic, feel free to contact me privately.

[wxiaoguang]

I proposed another "quick fix" : Always load or generate oauth2 jwt secret #30942 , without changing the existing logic too much.

Maybe we can take that one and leave enough time for the "secret" refactoring.

(I guess there is no easy&non-breaking fix for the legacy secret problems ...... TBH I don't like these "quick fixes" either, but the real world is that, we have to move on .....)

[CirnoT]

There are pretty many uses the for "general web secrets", see the comments: not only JWT, but also CSRF token, email validation, etc. Do you prefer to introduce dozens of "secrets" for them to "harden"?

I think it would be valid to introduce new "general" secret that is used for internal tokens while leaving existing OAuth2 intact as separate. This would make this change only partially breaking and ready for 1.23 as while it would touch LFS-related stuff, same advice as you given out previously (to reuse old LFS secret) would be valid.