feat(admin): challenge TOTP only when the login IP changes

[t0ma5]

Summary

• Optional authenticator-app 2FA for oc-admin. A TOTP (or backup code) is required only when the sign-in IP differs from the last verified address; the same IP still signs in with password + captcha alone.
• Enrollment lives on Your Profile (manual key + otpauth:// link, backup codes shown once). Remember-me from a new IP takes the same challenge path. Changing the password clears the stored IP so the next login must 2FA.
• New t_admin_2fa table (migration 0029 + struct.sql). Secrets are encrypted with the install signing key. 2FA attempts use LoginThrottle under a separate admin-2fa context. If the table is missing (upgrade not run yet), lookups fail open so the admin who has to start the upgrade is not locked out.

Test plan

• Fresh install / db:upgrade creates t_admin_2fa
• Users → Your Profile → Set up authenticator → confirm 6-digit code → save backup codes
• Same-IP login: password only (no code prompt)
• Different IP (or after a password change): code prompt, then dashboard
• Remember-me cookie from a new IP: code prompt; cancel drops the cookie
• Wrong codes are throttled; a backup code works once
• Disable 2FA from the same profile with a current TOTP or backup code
• php tests/security-totp.php (RFC 4226 / window pins)

NOTE: I have been running a heavily customized version of Osclass for 12 years volunteersbase.com - Thank you for keeping the project alive! Here is my humble contribution, hope you can find it useful :)

[navjottomer]

Thanks for putting this together — the TOTP implementation itself is clean, the throttle is correctly wired into the second factor under its own admin-2fa key, and needsChallenge() fails closed when either IP is empty. A few things need resolving before this can merge, and one design question I would like to settle explicitly rather than by default.

1. The encryption dependency no longer exists on develop

AdminTotp imports OpensslCryptor\Cryptor. That package was removed from develop yesterday in 695a97b5d as an unused dependency.

This is easy to miss because the branch still carries its own copy of the vendor directory, so it works when you check the branch out. Test-merging into develop is what surfaces it — after the merge, class_exists("OpensslCryptor\Cryptor") is false, and both enrolment and TOTP login would fatal.

2. The secret cannot be stored in the column the migration creates

This one is independent of the above, and is currently masked by it, so fixing only the fatal will not surface it.

encrypt() calls Cryptor::Encrypt($plain, $key, 0). The third argument is the output format, and 0 is FORMAT_RAW — raw binary, under the library default of aes-256-ctr. The migration then declares:

s_secret VARCHAR(255) NOT NULL DEFAULT ''

on a utf8mb4 table. Random ciphertext is effectively never valid UTF-8, so the insert fails:

ERROR 1366 (22007): Incorrect string value: '\x8F:\xC9\xD1\xE0\007F...'
  for column `shopclass`.`zz_totp_probe`.`s_secret`

On an install without strict mode this is worse than an error — the value is silently mangled on write and the admin is locked out of their own panel immediately after enrolling, with no way back in short of database surgery.

Suggested fix covering both points: use openssl_encrypt() with aes-256-gcm directly and base64 the result into the text column. hSecurity.php has the pattern from the alert-token work — 12-byte IV, 16-byte tag, prefixed to the ciphertext. That also gets you authentication, which CTR does not provide; for a stored second-factor secret, tamper-evidence is worth having.

3. Migration numbering and schema drift

The new migration is 0029_admin_2fa.php, but develop has used 0029 through 0035 (0029_billing_entitlements … 0035_billing_listing_limit). MigrationRunner keys on filename so both would still run, but the ordering is wrong — please renumber to 0036.

Related: t_admin_sec does not appear in installer/struct.sql at all, so the table is created only by the migration. Fresh installs and upgraded installs would end up with different schemas. It needs to be in both.

4. The design premise — worth deciding on purpose

The mechanics are sound; my question is about the security property being chosen.

Only one address is remembered, and currentIp() reads REMOTE_ADDR only. On any install behind Cloudflare or a reverse proxy that does not restore the client address, every admin resolves to the same proxy IP. After the first challenge, s_last_ip matches forever, nobody is challenged again — and an attacker who has the password and arrives through the same proxy is never challenged either. The docblock notes the proxy caveat as it affects the accuracy of the value, but not this consequence, which is that 2FA quietly becomes a no-op on a very common deployment shape.

Second, there is no replay tracking. Totp::verify() is a stateless window check with no record of the last consumed slice, so an observed code stays reusable for the whole ±window.

Neither is a coding mistake — they follow from the feature as designed. But "TOTP only when the IP changes" is a materially weaker guarantee than what people generally assume 2FA gives them, and I would rather it be a stated, documented decision than an implicit one.

Options, roughly in order of preference:

• Always challenge, with an explicit opt-in "remember this device" cookie (signed, bounded lifetime) as the thing that suppresses it. That keeps the ergonomics you are after while making the exemption a deliberate per-device act rather than a property of the network.
• Keep the IP gate, but make the trusted-IP list explicit and admin-managed rather than last-seen-wins, and refuse to treat an address as trusted when the request arrives through a proxy the install has not been told about.
• Keep it as-is, and document plainly in both the admin UI and the docblock what it does and does not protect against.

Happy to discuss — I do not want to send you rewriting things if you had a threat model in mind that I have not accounted for. Points 1 to 3 are the blocking ones. Also note the branch is currently conflicting with develop, mostly in CHANGELOG.md.

[t0ma5]

Thanks — 1 to 3 are done on the branch.

Crypto / column: OpensslCryptor is gone. Secrets are openssl_encrypt AES-256-GCM (12-byte IV, 16-byte tag) and base64 into the VARCHAR, same layout as the alert-token work on develop. That also gives a MAC, which CTR did not.

Migration / schema: file is 0036_admin_2fa.php. t_admin_2fa is in struct.sql (it was already on this branch; I added i_last_totp_step there too). Fresh install and upgrade now share the table.

Design (your point 4): I am keeping the IP gate on purpose, not as an accident. The feature we wanted is “password-only on the usual desk, code when the address moves” — not a prompt on every login. I agree that is a weaker property than people assume when they hear 2FA, so it is now spelled out in the class docblock and on Your Profile, including the Cloudflare/proxy hole: if REMOTE_ADDR is the edge, the first success stamps that address and later password theft from the same edge is never challenged. A remember-this-device cookie would be a better exemption; I did not rewrite the gate in this pass because that is a different product. Happy to do that as a follow-up if you would rather merge always-challenge + signed cookie than ship the documented IP gate.

What I did add in this pass: reject a TOTP time-step that was already consumed in the window (replay inside ±1 step).

Also still conflicting with develop on CHANGELOG; merge from this side is ready once you want it pushed.

[navjottomer]

I am keep this pending till 6.2.0 release, once that is out we will integrate this. it will also give us time to properly test this feature.