Use pbkdf2 instead of md5, add salt in totp

[lunny]

Updated the two-factor secret storage to use PBKDF2-derived AES keys with per-user salts, added a secret_algo marker and migration (including backfilling existing rows to md5), and implemented on-login re-encryption of legacy secrets. Added migration tests for the new columns and unit tests covering PBKDF2 setup and legacy secret upgrade behavior.

---

Generated by Coding Agent with Codex 5.2

[Copilot]

Pull request overview

This PR strengthens TOTP secret protection by introducing per-user salting and PBKDF2-derived encryption keys, while keeping backward compatibility by upgrading legacy (md5-derived) encrypted secrets on successful validation.

Changes:

• Add secret_salt / secret_algo fields to TwoFactor and a migration (v330) to add columns and mark existing rows as legacy (md5).
• Update ValidateTOTP to support legacy decryption and opportunistically re-encrypt secrets with PBKDF2+salt (returning an upgraded flag).
• Update auth flows/tests to use the new (ok, upgraded, err) return signature and cover upgrade behavior.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
services/auth/basic.go	Persists upgraded 2FA secret on successful OTP validation for basic auth flow.
routers/web/auth/password.go	Updates TOTP validation call signature in password reset flow.
routers/web/auth/2fa.go	Updates TOTP validation call signature in web 2FA sign-in flow.
models/auth/twofactor.go	Adds salt/algo fields; switches key derivation to PBKDF2; implements legacy upgrade on validate.
models/auth/twofactor_test.go	Adds tests for PBKDF2 secrets and legacy upgrade behavior.
models/migrations/v1_26/v330.go	Adds migration to introduce secret_salt / secret_algo columns and set legacy algo.
models/migrations/v1_26/v330_test.go	Adds migration test asserting new columns exist.
models/migrations/migrations.go	Registers migration 330.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

When rotateSecretSalt() fails during a legacy-secret upgrade, the function returns ok (which is true in this branch) alongside a non-nil error. Returning false for ok when err != nil avoids ambiguous results and prevents future callers from accidentally accepting a passcode if they mishandle the error.

Suggested change

	return ok, false, err
	}
	key, _ = t.getEncryptionKey()
	secretBytes, err = secret.AesEncrypt(key, []byte(secretStr))
	if err != nil {
	return ok, false, err
	return false, false, err
	}
	key, _ = t.getEncryptionKey()
	secretBytes, err = secret.AesEncrypt(key, []byte(secretStr))
	if err != nil {
	return false, false, err

[Copilot]

Similarly, if re-encryption of the legacy secret fails, this returns ok (true) with a non-nil error. Prefer returning false for ok whenever err is non-nil to keep the API contract consistent and reduce the risk of misuse by future callers.

Suggested change

	return ok, false, err
	}
	key, _ = t.getEncryptionKey()
	secretBytes, err = secret.AesEncrypt(key, []byte(secretStr))
	if err != nil {
	return ok, false, err
	return false, false, err
	}
	key, _ = t.getEncryptionKey()
	secretBytes, err = secret.AesEncrypt(key, []byte(secretStr))
	if err != nil {
	return false, false, err

[silverwind]

PBKDF2-derived AES keys

How "secure" is this? Never heard of PBKDF2 personally. Maybe consider argon2:

[wxiaoguang]

The code reads very strange.

There are many calls to ValidateTOTP, but why only one call does UpdateTwoFactor ?

Shouldn't ValidateTOTP fully handle the "upgrade"?

The test is also very strange, it never really tested the data is upgraded in database, never tested that after upgrade the TOTP can still succeed by reading from database.

And the check t.SecretAlgo == "" || t.SecretAlgo == "md5" || t.SecretSalt == "" is also very strange, why not just simply treat "empty" as legacy "md5"? Why need to use string const "md5"?

---

Conclusion: AI slop

[bircni]

Superseded by #39540 — rewritten on current main with upgrade handled inside ValidateAndConsumeTOTP, empty secret_salt as the legacy marker (no secret_algo/md5 string), and DB round-trip coverage.

Assisted-by: Composer