fix(auth): encrypt secrets with AES-256-GCM and an Argon2id-derived key

[bircni]

Fixes https://github.com/go-gitea/gitea/security/advisories/GHSA-673g-8f9j-6q25 (supersedes #36966). Every value encrypted with SECRET_KEY used an unstretched hash of it as the AES key, md5 for TOTP secrets and sha256 for webhook auth headers, Actions secrets, LDAP bind passwords and migration credentials. Any one of them allowed fast offline brute-force of a weak SECRET_KEY, so hardening only TOTP secrets would not help.

• Derive one Argon2id key from SECRET_KEY once per process
• Encrypt new values with AES-256-GCM instead of unauthenticated AES-CFB, prefixed with v2:
• Route TOTP secrets through the shared EncryptSecret/DecryptSecret
• Add a migration that re-encrypts all existing values
• Keep reading legacy sha256 and md5 TOTP values that the migration could not convert, e.g. already queued migration tasks or a run under a wrong SECRET_KEY

[silverwind]

This needs more work, it does not actually fix the advisory. Working on it.

[silverwind]

Current fix works, but has a migration, I'm checking if that can be avoided.

[silverwind]

Diff is now as small as it can get. There is no solution without the migration because weak encrypted values must be rewritten in the database, or else someone with a database dump could break the weak crypto in it.

[silverwind]

Legacy fallback could be dropped again if we consider "running migration under wrong SECRET_KEY" to be too much of an edge case, but I kept it in.

[wxiaoguang]

Since it is a large migration, can we introduce the preparation for the key rotation?

Move all encrypted contents into a single table and manage them together. Then, to rotate, only need to update that table.

• table: two_factor.secret = "ref:abcdefg"
• table: encrypted_content: ref_id="abcdefg", encrypted_value="v2:{base64 encrypted content}"

Pros: easy to rotate the master key
Cons: how to delete the encrypted_content when a two_factor record is deleted.

(IIRC there was a discussion in discord, cc @TheFox0x7 )

---

I am neutral for the decisions, so this comment is just a question or suggestion, no blocker.

[silverwind]

Sounds doable. I think I will also drop this MD5 fallback after all to keep the codebase clean from legacy mechanisms that could trigger security scanners needlessly.

[wxiaoguang]

Sounds doable. I think I will also drop this MD5 fallback after all to keep the codebase clean from legacy mechanisms that could trigger security scanners needlessly.

I haven't thought about the problem carefully, so not sure whether we should do that (single encrypt content table) or not. It really depends on how Gitea will manage the encrypted content in the future. If we'd like to do more, we need a careful and complete design first.

Or, just use the current good enough approach, leave more discussions to the future.

[wxiaoguang]

A quick idea: without introducing a single encrypted content table, maybe we can move ReencryptSecrets to a new standalone package. Make it can use old secrets to decrypt the contents, and use a new secret to encrypt the contents. For this PR's migration, old secret is the one from config file, new secret is the derived one (also, versioned)

Then, we can introduce a cli sub-command to call ReencryptSecrets.

This seems a minimal change, won't make anything more complicated, and should be good enough for the future.

[TheFox0x7]

I redid my research into that (I'll recheck with previous looks when I'll get a chance) and I think the one common table would not be a good approach. For it to work properly you need FKs to lock deletion if someone holds a reference and while it's nice and simple to migrate there's not much use in it apart from that - unless you'd be planning to enable multiple references... so a user could define a secret once and reuse that somewhere else from a dropdown or such.

Plain columns are probably simpler to use and maintain - no joins, no table, no possibly broken references if something goes wrong. It's what grafana and gitlab use so I'd assume it's a sane pick for this. Downside being the spread between tables but it's not like this changes much.

[techknowlogick]

Does this also handle this case: #16832 ?

[wxiaoguang]

Plain columns are probably simpler to use and maintain - no joins, no table, no possibly broken references if something goes wrong. It's what grafana and gitlab use so I'd assume it's a sane pick for this. Downside being the spread between tables but it's not like this changes much.

Agree, I proposed maybe we can move ReencryptSecrets to a new standalone package, then it can be reused later.

[wxiaoguang]

Any one of them allowed fast offline brute-force of a weak SECRET_KEY .... Add a migration that re-encrypts all existing values

By the way, I think this PR doesn't really help the situation.

The current situation is: most instances are using the default SECRET_KEY. So, even if you use a derived key to re-encrypt the values, the derived key is still the same for most instances (no need to brute-force)