From c99c468eb7bfbdefe67aa0004f8875b0c6fe2bdd Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Tue, 21 Jul 2026 18:30:54 +0200 Subject: [PATCH] Share TOTP code verification between user and admin 2FA Extract a single `verify_totp()` helper in `two_factor::authenticator` that performs the time-window check, and use it from both `validate_totp_code` (user 2FA) and the admin-page TOTP check instead of duplicating the loop. Each caller keeps its own last-used storage (DB vs. in-memory) and drift policy. The user path now compares in constant time, and a unit test for the helper is added. --- src/api/admin.rs | 24 +++-- src/api/core/two_factor/authenticator.rs | 108 ++++++++++++++++++----- 2 files changed, 94 insertions(+), 38 deletions(-) diff --git a/src/api/admin.rs b/src/api/admin.rs index bcaf694c..afb95bb2 100644 --- a/src/api/admin.rs +++ b/src/api/admin.rs @@ -271,7 +271,7 @@ fn validate_totp(code: Option<&str>) -> bool { } fn check_totp_code(secret: &str, code: &str) -> bool { - use totp_lite::{Sha1, totp_custom}; + use two_factor::authenticator::{TotpValidation, verify_totp}; if code.len() != 6 || !code.chars().all(char::is_numeric) { return false; @@ -284,22 +284,18 @@ fn check_totp_code(secret: &str, code: &str) -> bool { let current_timestamp = chrono::Utc::now().timestamp(); let mut last_used = ADMIN_TOTP_LAST_USED.lock().expect("admin TOTP mutex poisoned"); - // Allow one step of time drift in either direction, like the user 2FA in `two_factor::authenticator` does - for step in -1..=1i64 { - let time_step = current_timestamp / 30 + step; - let time = (current_timestamp + step * 30).cast_unsigned(); - let generated = totp_custom::(30, 6, &decoded_secret, time); - - if crate::crypto::ct_eq(&generated, code) { - if time_step <= *last_used { - warn!("This admin TOTP code has already been used"); - return false; - } + // Reuse the shared verifier; allow one step of time drift in either direction, like the user 2FA does. + match verify_totp(&decoded_secret, code, current_timestamp, *last_used, 1) { + TotpValidation::Accepted(time_step) => { *last_used = time_step; - return true; + true + } + TotpValidation::Reused => { + warn!("This admin TOTP code has already been used"); + false } + TotpValidation::Rejected => false, } - false } #[get("/totp")] diff --git a/src/api/core/two_factor/authenticator.rs b/src/api/core/two_factor/authenticator.rs index 692e8248..270946d6 100644 --- a/src/api/core/two_factor/authenticator.rs +++ b/src/api/core/two_factor/authenticator.rs @@ -112,6 +112,41 @@ pub async fn validate_totp_code_str( validate_totp_code(user_id, totp_code, secret, ip, conn).await } +/// The outcome of checking a TOTP `code` against the allowed time window. +pub(crate) enum TotpValidation { + /// The code is valid. Contains the accepted time step, which must be stored as the + /// new "last used" value so the same code cannot be reused. + Accepted(i64), + /// The code matched a time step that has already been used (replay protection). + Reused, + /// The code did not match any step within the allowed window. + Rejected, +} + +/// Verifies a 6-digit TOTP `code` against the already base32-decoded `secret`, allowing +/// `steps` (>= 0) time steps of ±30 seconds drift in either direction and rejecting any +/// time step that is not newer than `last_used`. The comparison is constant-time. +/// Shared by the user 2FA flow and the admin-page 2FA. +pub(crate) fn verify_totp(secret: &[u8], code: &str, timestamp: i64, last_used: i64, steps: i64) -> TotpValidation { + use totp_lite::{Sha1, totp_custom}; + + for step in -steps..=steps { + let time_step = timestamp / 30i64 + step; + // The generator needs the time as an u64; we only ever deal with times >= 0. + let time: u64 = (timestamp + step * 30i64).cast_unsigned(); + let generated = totp_custom::(30, 6, secret, time); + + if crypto::ct_eq(&generated, code) { + return if time_step > last_used { + TotpValidation::Accepted(time_step) + } else { + TotpValidation::Reused + }; + } + } + TotpValidation::Rejected +} + pub async fn validate_totp_code( user_id: &UserId, totp_code: &str, @@ -119,8 +154,6 @@ pub async fn validate_totp_code( ip: &ClientIp, conn: &DbConn, ) -> EmptyResult { - use totp_lite::{Sha1, totp_custom}; - let Ok(decoded_secret) = BASE32.decode(secret.as_bytes()) else { err!("Invalid TOTP secret") }; @@ -140,17 +173,10 @@ pub async fn validate_totp_code( let current_time = chrono::Utc::now(); let current_timestamp = current_time.timestamp(); - for step in -steps..=steps { - let time_step = current_timestamp / 30i64 + step; - - // We need to calculate the time offsite and cast it as an u64. - // Since we only have times into the future and the totp generator needs an u64 instead of the default i64. - let time: u64 = (current_timestamp + step * 30i64).cast_unsigned(); - let generated = totp_custom::(30, 6, &decoded_secret, time); - - // Check the given code equals the generated and if the time_step is larger then the one last used. - if generated == totp_code && time_step > twofactor.last_used { - // If the step does not equals 0 the time is drifted either server or client side. + match verify_totp(&decoded_secret, totp_code, current_timestamp, twofactor.last_used, steps) { + TotpValidation::Accepted(time_step) => { + // If the accepted step is not the current one the time is drifted either server or client side. + let step = time_step - current_timestamp / 30i64; if step != 0 { warn!("TOTP Time drift detected. The step offset is {step}"); } @@ -159,25 +185,27 @@ pub async fn validate_totp_code( // This will also save a newly created twofactor if the code is correct. twofactor.last_used = time_step; twofactor.save(conn).await?; - return Ok(()); - } else if generated == totp_code && time_step <= twofactor.last_used { + Ok(()) + } + TotpValidation::Reused => { warn!("This TOTP or a TOTP code within {steps} steps back or forward has already been used!"); err!( format!("Invalid TOTP code! Server time: {} IP: {}", current_time.format("%F %T UTC"), ip.ip), ErrorEvent { event: EventType::UserFailedLogIn2fa } - ); + ) } - } - - // Else no valid code received, deny access - err!( - format!("Invalid TOTP code! Server time: {} IP: {}", current_time.format("%F %T UTC"), ip.ip), - ErrorEvent { - event: EventType::UserFailedLogIn2fa + // Else no valid code received, deny access + TotpValidation::Rejected => { + err!( + format!("Invalid TOTP code! Server time: {} IP: {}", current_time.format("%F %T UTC"), ip.ip), + ErrorEvent { + event: EventType::UserFailedLogIn2fa + } + ) } - ); + } } #[derive(Debug, Deserialize)] @@ -217,3 +245,35 @@ async fn disable_authenticator(data: Json, headers: He "object": "twoFactorProvider" }))) } + +#[cfg(test)] +mod tests { + use super::{TotpValidation, verify_totp}; + use data_encoding::BASE32; + use totp_lite::{Sha1, totp_custom}; + + fn code_at(secret: &[u8], timestamp: i64) -> String { + totp_custom::(30, 6, secret, timestamp.cast_unsigned()) + } + + #[test] + fn totp_accepts_rejects_and_detects_reuse() { + let secret = BASE32.decode(b"JBSWY3DPEHPK3PXP").unwrap(); + let ts = 1_700_000_000i64; + let step = ts / 30; + let current = code_at(&secret, ts); + + // A fresh current code is accepted and reports the current time step. + assert!(matches!(verify_totp(&secret, ¤t, ts, 0, 1), TotpValidation::Accepted(s) if s == step)); + // The same code is rejected as reused once its step has been recorded. + assert!(matches!(verify_totp(&secret, ¤t, ts, step, 1), TotpValidation::Reused)); + // A wrong code is rejected. + assert!(matches!(verify_totp(&secret, "000000", ts, 0, 1), TotpValidation::Rejected)); + + // The previous 30s window is accepted with one step of drift... + let previous = code_at(&secret, ts - 30); + assert!(matches!(verify_totp(&secret, &previous, ts, 0, 1), TotpValidation::Accepted(_))); + // ...but rejected when drift is disabled (steps = 0). + assert!(matches!(verify_totp(&secret, &previous, ts, 0, 0), TotpValidation::Rejected)); + } +}