jball-resetdata opened a new pull request, #13682:
URL: https://github.com/apache/cloudstack/pull/13682
### Description
This PR hardens CloudStack's native TOTP two-factor authentication provider,
which
previously accepted only the exact current 30-second time step and had
several gaps
relative to RFC 6238 / OWASP guidance. All changes preserve compatibility
with
existing enrolments (see testing notes).
**What changes functionally:**
1. **Verification window (fixes clock-skew login failures).** `check2FA` now
accepts
the current time step plus a configurable number of steps on each side,
via a new
domain-scoped setting `user.2fa.totp.window.steps` (default `1`; set `0`
to restore
the previous strict, current-step-only behaviour). Previously any clock
drift
between the management server and the user's authenticator caused
verification to
fail — see #9515, where an NTP issue broke 2FA login and was only ever
mitigated
with a clearer error message, not a real tolerance.
2. **Replay protection.** A given TOTP code can no longer be reused within
(or across)
its validity window. The last accepted time step is recorded per user
(`user.last_used_2fa_step`) and a code whose step is not strictly newer
is rejected.
The check-and-set is a single atomic conditional `UPDATE`, so two
concurrent
verifications of the same code cannot both succeed.
3. **Encryption of the TOTP secret at rest.** `user.key_for_2fa` was stored
in
plaintext; it is now `@Encrypt`-annotated (matching how `secret_key` on
the same
entity is handled). A DB upgrade encrypts any existing plaintext secrets
in place.
The migration is a no-op when DB encryption is disabled and idempotent
when
enabled, so it is safe to re-run.
4. **One-time backup (recovery) codes.** New API
`generateUserTwoFactorAuthenticationBackupCodes` issues a set of 10
single-use
codes (returned once). Codes are stored **hashed** (SHA-256), never
plaintext or
reversibly encrypted. Verification is provider-agnostic: if the configured
provider's code does not match for an already-verified user, a matching
unused
backup code is accepted and consumed (constant-time comparison). Backup
codes are
not accepted during the initial 2FA setup verification, and are cleared
when 2FA is
disabled. (A UI to surface generation/entry is intended as a follow-up;
the feature
is fully usable via the API and is independent of items 1–3.)
Also: comparisons now use constant-time `MessageDigest.isEqual`, and the
TOTP code is
computed directly per RFC 6238 (HMAC-SHA1, 6 digits, 30 s) because the
bundled
`de.taimos:totp` library only verifies against the current step (its
step-parameter
methods are private). The output is identical to what the library produced,
so
existing enrolments verify unchanged.
<!-- Fixes: #9515 -->
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing
functionality to change)
- [x] New feature (non-breaking change which adds functionality)
- [x] Bug fix (non-breaking change which fixes an issue)
- [x] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
- [ ] Build/CI
- [ ] Test (unit or integration test code)
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [x] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [x] Major
- [ ] Minor
- [ ] Trivial
### How Has This Been Tested?
Unit tests added for each area (22 new assertions across three test classes):
- `TotpUserTwoFactorAuthenticatorTest` — window accept/reject, strict mode
(`window=0`),
constant-time verification, replay rejection incl. "same code cannot be
used twice",
invalid/blank input.
- `TwoFactorAuthenticationBackupCodesTest` — generation, one-time
consumption,
invalid-code rejection, input-format tolerance, null-safety.
- `Upgrade42210to42220Test` — version chain and plaintext-vs-encrypted
detection
(100 freshly generated secrets + encrypted-looking blobs).
Build is clean with `0` Checkstyle violations across the changed modules
(`api`, `engine/schema`, `server`, the TOTP plugin).
**Backward-compatibility of enrolled users** was verified by computing codes
with the
previous `de.taimos:totp` library and the new in-tree RFC 6238
implementation for the
same secret and step — output is byte-identical, so existing authenticator
apps keep
working after upgrade.
#### How did you try to break this feature and the system with this change?
- **Upgrade with DB encryption enabled + a pre-existing enrolled 2FA user**
— the
highest-risk path (a broken migration here would lock out existing users,
since the
new `@Encrypt` read path can't decrypt a plaintext secret). Confirmed the
migration
detects plaintext by Base32 shape and encrypts it, so the user still logs
in
post-upgrade; and that it no-ops cleanly when encryption is disabled.
- **Concurrent verification of the same code** — replaced the original
read-then-write
with an atomic conditional `UPDATE` so a race cannot let the same code
pass twice.
- **Clock skew** — a code from the previous/next step is accepted with the
default
window and rejected with `window=0`.
- **Backup-code reuse** — a consumed code is removed and cannot be used
again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
