diff --git a/src/api/admin.rs b/src/api/admin.rs index 9321a40d..018941e1 100644 --- a/src/api/admin.rs +++ b/src/api/admin.rs @@ -251,9 +251,8 @@ fn validate_token(token: &str) -> bool { } } -// The active-secret fingerprint and its last accepted TOTP time step, kept in memory since there is no database -// record for the admin. Keeping the fingerprint resets replay protection when the configured secret changes. -// Restarting Vaultwarden resets it, which at worst allows a code from the previous 30 seconds to be reused once. +// Last accepted TOTP time step, keyed by a fingerprint of the active secret so replacing the secret resets +// replay protection. Kept in memory (the admin has no DB record); a restart allows one code reuse at worst. static ADMIN_TOTP_LAST_USED: Mutex> = Mutex::new(None); fn configured_admin_totp_secret() -> Option { diff --git a/src/api/core/two_factor/authenticator.rs b/src/api/core/two_factor/authenticator.rs index 270946d6..1b154c67 100644 --- a/src/api/core/two_factor/authenticator.rs +++ b/src/api/core/two_factor/authenticator.rs @@ -114,19 +114,15 @@ pub async fn validate_totp_code_str( /// 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. + /// Valid; holds the accepted time step, which must be stored as the new "last used" value. 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. +/// Verifies a 6-digit TOTP `code` against the base32-decoded `secret`, allowing `steps` time steps of +/// ±30s drift and rejecting any step not newer than `last_used`. The comparison is constant-time. pub(crate) fn verify_totp(secret: &[u8], code: &str, timestamp: i64, last_used: i64, steps: i64) -> TotpValidation { use totp_lite::{Sha1, totp_custom}; diff --git a/src/config.rs b/src/config.rs index 1a971e12..a7c4cbb1 100644 --- a/src/config.rs +++ b/src/config.rs @@ -960,10 +960,8 @@ make_config! { } impl ConfigBuilder { - /// Applies the write-only admin form semantics for the TOTP secret: - /// - an omitted value keeps the value currently stored in `config.json`; - /// - an empty value removes the stored override; - /// - a non-empty value replaces it. + /// Write-only admin form semantics for the TOTP secret: an omitted value keeps the value stored in + /// `config.json`, an empty value removes the stored override and a non-empty value replaces it. fn prepare_admin_update(&mut self, current_user_config: &Self) { self.admin_totp_secret = match self.admin_totp_secret.take() { None => current_user_config.admin_totp_secret.clone(), @@ -1529,8 +1527,7 @@ impl Config { // TODO: Remove values that are defaults, above only checks those set by env and not the defaults let mut builder = other; - // Remove values that are not editable. Preserve an omitted write-only TOTP value, - // while still allowing an explicit empty value to remove the saved override. + // Remove values that are not editable if ignore_non_editable { let current_user_config = self.inner.read().unwrap()._usr.clone(); builder.prepare_admin_update(¤t_user_config); @@ -1919,6 +1916,13 @@ mod tests { const ENV_TOTP_SECRET: &str = "JBSWY3DPEHPK3PXPJBSWY3DPEHPK3PXP"; const USER_TOTP_SECRET: &str = "KRSXG5DSNFXGOIDBKRSXG5DSNFXGOIDB"; + fn builder(secret: &str) -> ConfigBuilder { + ConfigBuilder { + admin_totp_secret: Some(secret.to_owned()), + ..Default::default() + } + } + fn config_with(env: ConfigBuilder, usr: ConfigBuilder) -> Config { let mut overrides = Vec::new(); let config = env.merge(&usr, false, &mut overrides).build(); @@ -1935,6 +1939,10 @@ mod tests { } } + fn env_and_usr_config() -> Config { + config_with(builder(ENV_TOTP_SECRET), builder(USER_TOTP_SECRET)) + } + fn admin_totp_element(config: &Config) -> serde_json::Value { config .prepare_json() @@ -1947,37 +1955,14 @@ mod tests { .clone() } - #[test] - fn config_file_admin_totp_secret_overrides_environment() { - let env = ConfigBuilder { - admin_totp_secret: Some(ENV_TOTP_SECRET.to_owned()), - ..Default::default() - }; - let usr = ConfigBuilder { - admin_totp_secret: Some(USER_TOTP_SECRET.to_owned()), - ..Default::default() - }; - let config = config_with(env, usr); - - assert_eq!(config.admin_totp_secret().as_deref(), Some(USER_TOTP_SECRET)); - assert!(config.inner.read().unwrap()._overrides.contains(&"ADMIN_TOTP_SECRET")); - } - #[test] fn admin_totp_secret_is_write_only_in_settings_json() { - let env = ConfigBuilder { - admin_totp_secret: Some(ENV_TOTP_SECRET.to_owned()), - ..Default::default() - }; - let usr = ConfigBuilder { - admin_totp_secret: Some(USER_TOTP_SECRET.to_owned()), - ..Default::default() - }; - let config = config_with(env, usr); - let json = config.prepare_json(); - let serialized = json.to_string(); + let config = env_and_usr_config(); + let serialized = config.prepare_json().to_string(); let support_json = config.get_support_json().to_string(); + assert_eq!(config.admin_totp_secret().as_deref(), Some(USER_TOTP_SECRET)); + assert!(config.inner.read().unwrap()._overrides.contains(&"ADMIN_TOTP_SECRET")); assert!(!serialized.contains(ENV_TOTP_SECRET)); assert!(!serialized.contains(USER_TOTP_SECRET)); assert!(!support_json.contains(ENV_TOTP_SECRET)); @@ -1991,17 +1976,8 @@ mod tests { assert_eq!(element["user_configured"], true); assert_eq!(element["overridden"], true); - let empty_override = config_with( - ConfigBuilder { - admin_totp_secret: Some(ENV_TOTP_SECRET.to_owned()), - ..Default::default() - }, - ConfigBuilder { - admin_totp_secret: Some(String::new()), - ..Default::default() - }, - ); - let element = admin_totp_element(&empty_override); + // An explicitly emptied override still overrides the environment value. + let element = admin_totp_element(&config_with(builder(ENV_TOTP_SECRET), builder(""))); assert_eq!(element["configured"], false); assert_eq!(element["user_configured"], true); assert_eq!(element["overridden"], true); @@ -2009,15 +1985,7 @@ mod tests { #[test] fn admin_settings_template_never_renders_admin_totp_secret() { - let env = ConfigBuilder { - admin_totp_secret: Some(ENV_TOTP_SECRET.to_owned()), - ..Default::default() - }; - let usr = ConfigBuilder { - admin_totp_secret: Some(USER_TOTP_SECRET.to_owned()), - ..Default::default() - }; - let config = config_with(env, usr); + let config = env_and_usr_config(); let mut handlebars = Handlebars::new(); handlebars.set_strict_mode(true); @@ -2043,18 +2011,15 @@ mod tests { assert!(!rendered.contains(USER_TOTP_SECRET)); assert!(rendered.contains("id=\"input_admin_totp_secret\"")); assert!(rendered.contains("placeholder=\"Configured — enter a new value to replace it\"")); - assert!(rendered.contains("data-bs-target=\"#adminTotpQrDialog\"")); assert!(rendered.contains("id=\"adminTotpQrCode\"")); assert!(rendered.contains("/vw_static/qrcode-generator-2.0.4.js")); + // The QR dialog must not be nested inside the settings form. assert!(rendered.find("").unwrap() < rendered.find("id=\"adminTotpQrDialog\"").unwrap()); } #[test] fn admin_totp_secret_form_supports_keep_set_and_clear() { - let current = ConfigBuilder { - admin_totp_secret: Some(USER_TOTP_SECRET.to_owned()), - ..Default::default() - }; + let current = builder(USER_TOTP_SECRET); let mut keep = ConfigBuilder::default(); keep.prepare_admin_update(¤t); @@ -2064,27 +2029,18 @@ mod tests { no_saved_override.prepare_admin_update(&ConfigBuilder::default()); assert_eq!(no_saved_override.admin_totp_secret, None); - let mut replace = ConfigBuilder { - admin_totp_secret: Some(format!(" {} ", ENV_TOTP_SECRET.to_lowercase())), - ..Default::default() - }; + let mut replace = builder(&format!(" {} ", ENV_TOTP_SECRET.to_lowercase())); replace.prepare_admin_update(¤t); assert_eq!(replace.admin_totp_secret.as_deref(), Some(ENV_TOTP_SECRET)); - let mut clear = ConfigBuilder { - admin_totp_secret: Some(" ".to_owned()), - ..Default::default() - }; + let mut clear = builder(" "); clear.prepare_admin_update(¤t); assert_eq!(clear.admin_totp_secret, None); - let env = ConfigBuilder { - admin_totp_secret: Some(ENV_TOTP_SECRET.to_owned()), - ..Default::default() - }; + // Clearing the saved override falls back to the environment value. let mut overrides = Vec::new(); assert_eq!( - env.merge(&clear, false, &mut overrides).build().admin_totp_secret.as_deref(), + builder(ENV_TOTP_SECRET).merge(&clear, false, &mut overrides).build().admin_totp_secret.as_deref(), Some(ENV_TOTP_SECRET) ); }