switch tfa api to use proxmox-tfa::api

Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>

Cargo.toml

@@ -86,7 +86,6 @@ udev = "0.4"
 url = "2.1"
 #valgrind_request = { git = "https://github.com/edef1c/libvalgrind_request", version = "1.1.0", optional = true }
 walkdir = "2"
-webauthn-rs = "0.2.5"
 xdg = "2.2"
 nom = "5.1"
 crossbeam-channel = "0.5"
@@ -104,7 +103,7 @@ proxmox-lang = "1"
 proxmox-router = { version = "1.1", features = [ "cli" ] }
 proxmox-schema = { version = "1", features = [ "api-macro" ] }
 proxmox-section-config = "1"
-proxmox-tfa = { version = "1", features = [ "u2f" ] }
+proxmox-tfa = { version = "1.3", features = [ "api", "api-types" ] }
 proxmox-time = "1"
 proxmox-uuid = "1"
 proxmox-shared-memory = "0.1.1"
@@ -130,6 +129,8 @@ pbs-tape = { path = "pbs-tape" }
 [patch.crates-io]
 #proxmox = { path = "../proxmox/proxmox" }
 #proxmox-http = { path = "../proxmox/proxmox-http" }
+#proxmox-tfa = { path = "../proxmox/proxmox-tfa" }
+#proxmox-schema = { path = "../proxmox/proxmox-schema" }
 #pxar = { path = "../pxar" }
 
 [features]

src/api2/access/tfa.rs

@@ -1,16 +1,17 @@
 //! Two Factor Authentication
 
-use anyhow::{bail, format_err, Error};
+use anyhow::Error;
-use serde::{Deserialize, Serialize};
 
-use proxmox_router::{http_bail, http_err, Router, RpcEnvironment, Permission};
+use proxmox_router::{http_bail, http_err, Permission, Router, RpcEnvironment};
 use proxmox_schema::api;
-use proxmox_tfa::totp::Totp;
+use proxmox_tfa::api::methods;
-
-use pbs_api_types::{Authid, Userid, User, PASSWORD_SCHEMA, PRIV_PERMISSIONS_MODIFY, PRIV_SYS_AUDIT};
 
+use pbs_api_types::{
+    Authid, User, Userid, PASSWORD_SCHEMA, PRIV_PERMISSIONS_MODIFY, PRIV_SYS_AUDIT,
+};
 use pbs_config::CachedUserInfo;
-use crate::config::tfa::{TfaInfo, TfaUserData};
+
+use crate::config::tfa::UserAccess;
 
 /// Perform first-factor (password) authentication only. Ignore password for the root user.
 /// Otherwise check the current user's password.
@@ -36,10 +37,7 @@ fn tfa_update_auth(
     if must_exist && authid.user() != userid {
         let (config, _digest) = pbs_config::user::config()?;
 
-        if config
+        if config.lookup::<User>("user", userid.as_str()).is_err() {
-            .lookup::<User>("user", userid.as_str())
-            .is_err()
-        {
             http_bail!(UNAUTHORIZED, "user '{}' does not exists.", userid);
         }
     }
@@ -47,97 +45,6 @@ fn tfa_update_auth(
     Ok(())
 }
 
-#[api]
-/// A TFA entry type.
-#[derive(Deserialize, Serialize)]
-#[serde(rename_all = "lowercase")]
-enum TfaType {
-    /// A TOTP entry type.
-    Totp,
-    /// A U2F token entry.
-    U2f,
-    /// A Webauthn token entry.
-    Webauthn,
-    /// Recovery tokens.
-    Recovery,
-}
-
-#[api(
-    properties: {
-        type: { type: TfaType },
-        info: { type: TfaInfo },
-    },
-)]
-/// A TFA entry for a user.
-#[derive(Deserialize, Serialize)]
-#[serde(deny_unknown_fields)]
-struct TypedTfaInfo {
-    #[serde(rename = "type")]
-    pub ty: TfaType,
-
-    #[serde(flatten)]
-    pub info: TfaInfo,
-}
-
-fn to_data(data: TfaUserData) -> Vec<TypedTfaInfo> {
-    let mut out = Vec::with_capacity(
-        data.totp.len()
-            + data.u2f.len()
-            + data.webauthn.len()
-            + if data.recovery().is_some() { 1 } else { 0 },
-    );
-    if let Some(recovery) = data.recovery() {
-        out.push(TypedTfaInfo {
-            ty: TfaType::Recovery,
-            info: TfaInfo::recovery(recovery.created),
-        })
-    }
-    for entry in data.totp {
-        out.push(TypedTfaInfo {
-            ty: TfaType::Totp,
-            info: entry.info,
-        });
-    }
-    for entry in data.webauthn {
-        out.push(TypedTfaInfo {
-            ty: TfaType::Webauthn,
-            info: entry.info,
-        });
-    }
-    for entry in data.u2f {
-        out.push(TypedTfaInfo {
-            ty: TfaType::U2f,
-            info: entry.info,
-        });
-    }
-    out
-}
-
-/// Iterate through tuples of `(type, index, id)`.
-fn tfa_id_iter(data: &TfaUserData) -> impl Iterator<Item = (TfaType, usize, &str)> {
-    data.totp
-        .iter()
-        .enumerate()
-        .map(|(i, entry)| (TfaType::Totp, i, entry.info.id.as_str()))
-        .chain(
-            data.webauthn
-                .iter()
-                .enumerate()
-                .map(|(i, entry)| (TfaType::Webauthn, i, entry.info.id.as_str())),
-        )
-        .chain(
-            data.u2f
-                .iter()
-                .enumerate()
-                .map(|(i, entry)| (TfaType::U2f, i, entry.info.id.as_str())),
-        )
-        .chain(
-            data.recovery
-                .iter()
-                .map(|_| (TfaType::Recovery, 0, "recovery")),
-        )
-}
-
 #[api(
     protected: true,
     input: {
@@ -151,13 +58,10 @@ fn tfa_id_iter(data: &TfaUserData) -> impl Iterator<Item = (TfaType, usize, &str
     },
 )]
 /// Add a TOTP secret to the user.
-fn list_user_tfa(userid: Userid) -> Result<Vec<TypedTfaInfo>, Error> {
+fn list_user_tfa(userid: Userid) -> Result<Vec<methods::TypedTfaInfo>, Error> {
     let _lock = crate::config::tfa::read_lock()?;
 
-    Ok(match crate::config::tfa::read()?.users.remove(&userid) {
+    methods::list_user_tfa(&crate::config::tfa::read()?, userid.as_str())
-        Some(data) => to_data(data),
-        None => Vec::new(),
-    })
 }
 
 #[api(
@@ -176,47 +80,13 @@ fn list_user_tfa(userid: Userid) -> Result<Vec<TypedTfaInfo>, Error> {
     },
 )]
 /// Get a single TFA entry.
-fn get_tfa_entry(userid: Userid, id: String) -> Result<TypedTfaInfo, Error> {
+fn get_tfa_entry(userid: Userid, id: String) -> Result<methods::TypedTfaInfo, Error> {
     let _lock = crate::config::tfa::read_lock()?;
 
-    if let Some(user_data) = crate::config::tfa::read()?.users.remove(&userid) {
+    match methods::get_tfa_entry(&crate::config::tfa::read()?, userid.as_str(), &id) {
-        match {
+        Some(entry) => Ok(entry),
-            // scope to prevent the temporary iter from borrowing across the whole match
+        None => http_bail!(NOT_FOUND, "no such tfa entry: {}/{}", userid, id),
-            let entry = tfa_id_iter(&user_data).find(|(_ty, _index, entry_id)| id == *entry_id);
-            entry.map(|(ty, index, _)| (ty, index))
-        } {
-            Some((TfaType::Recovery, _)) => {
-                if let Some(recovery) = user_data.recovery() {
-                    return Ok(TypedTfaInfo {
-                        ty: TfaType::Recovery,
-                        info: TfaInfo::recovery(recovery.created),
-                    });
-                }
-            }
-            Some((TfaType::Totp, index)) => {
-                return Ok(TypedTfaInfo {
-                    ty: TfaType::Totp,
-                    // `into_iter().nth()` to *move* out of it
-                    info: user_data.totp.into_iter().nth(index).unwrap().info,
-                });
-            }
-            Some((TfaType::Webauthn, index)) => {
-                return Ok(TypedTfaInfo {
-                    ty: TfaType::Webauthn,
-                    info: user_data.webauthn.into_iter().nth(index).unwrap().info,
-                });
-            }
-            Some((TfaType::U2f, index)) => {
-                return Ok(TypedTfaInfo {
-                    ty: TfaType::U2f,
-                    info: user_data.u2f.into_iter().nth(index).unwrap().info,
-                });
-            }
-            None => (),
-        }
     }
-
-    http_bail!(NOT_FOUND, "no such tfa entry: {}/{}", userid, id);
 }
 
 #[api(
@@ -253,25 +123,11 @@ fn delete_tfa(
 
     let mut data = crate::config::tfa::read()?;
 
-    let user_data = data
+    match methods::delete_tfa(&mut data, userid.as_str(), &id) {
-        .users
+        Ok(_) => (),
-        .get_mut(&userid)
+        Err(methods::EntryNotFound) => {
-        .ok_or_else(|| http_err!(NOT_FOUND, "no such entry: {}/{}", userid, id))?;
+            http_bail!(NOT_FOUND, "no such tfa entry: {}/{}", userid, id)
-
+        }
-    match {
-        // scope to prevent the temporary iter from borrowing across the whole match
-        let entry = tfa_id_iter(&user_data).find(|(_, _, entry_id)| id == *entry_id);
-        entry.map(|(ty, index, _)| (ty, index))
-    } {
-        Some((TfaType::Recovery, _)) => user_data.recovery = None,
-        Some((TfaType::Totp, index)) => drop(user_data.totp.remove(index)),
-        Some((TfaType::Webauthn, index)) => drop(user_data.webauthn.remove(index)),
-        Some((TfaType::U2f, index)) => drop(user_data.u2f.remove(index)),
-        None => http_bail!(NOT_FOUND, "no such tfa entry: {}/{}", userid, id),
-    }
-
-    if user_data.is_empty() {
-        data.users.remove(&userid);
     }
 
     crate::config::tfa::write(&data)?;
@@ -279,26 +135,6 @@ fn delete_tfa(
     Ok(())
 }
 
-#[api(
-    properties: {
-        "userid": { type: Userid },
-        "entries": {
-            type: Array,
-            items: { type: TypedTfaInfo },
-        },
-    },
-)]
-#[derive(Deserialize, Serialize)]
-#[serde(deny_unknown_fields)]
-/// Over the API we only provide the descriptions for TFA data.
-struct TfaUser {
-    /// The user this entry belongs to.
-    userid: Userid,
-
-    /// TFA entries.
-    entries: Vec<TypedTfaInfo>,
-}
-
 #[api(
     protected: true,
     input: {
@@ -311,11 +147,11 @@ struct TfaUser {
     returns: {
         description: "The list tuples of user and TFA entries.",
         type: Array,
-        items: { type: TfaUser }
+        items: { type: methods::TfaUser }
     },
 )]
 /// List user TFA configuration.
-fn list_tfa(rpcenv: &mut dyn RpcEnvironment) -> Result<Vec<TfaUser>, Error> {
+fn list_tfa(rpcenv: &mut dyn RpcEnvironment) -> Result<Vec<methods::TfaUser>, Error> {
     let authid: Authid = rpcenv.get_auth_id().unwrap().parse()?;
     let user_info = CachedUserInfo::new()?;
 
@@ -323,62 +159,8 @@ fn list_tfa(rpcenv: &mut dyn RpcEnvironment) -> Result<Vec<TfaUser>, Error> {
     let top_level_allowed = (top_level_privs & PRIV_SYS_AUDIT) != 0;
 
     let _lock = crate::config::tfa::read_lock()?;
-    let tfa_data = crate::config::tfa::read()?.users;
+    let tfa_data = crate::config::tfa::read()?;
-
+    methods::list_tfa(&tfa_data, authid.user().as_str(), top_level_allowed)
-    let mut out = Vec::<TfaUser>::new();
-    if top_level_allowed {
-        for (user, data) in tfa_data {
-            out.push(TfaUser {
-                userid: user,
-                entries: to_data(data),
-            });
-        }
-    } else if let Some(data) = { tfa_data }.remove(authid.user()) {
-        out.push(TfaUser {
-            userid: authid.into(),
-            entries: to_data(data),
-        });
-    }
-
-    Ok(out)
-}
-
-#[api(
-    properties: {
-        recovery: {
-            description: "A list of recovery codes as integers.",
-            type: Array,
-            items: {
-                type: Integer,
-                description: "A one-time usable recovery code entry.",
-            },
-        },
-    },
-)]
-/// The result returned when adding TFA entries to a user.
-#[derive(Default, Serialize)]
-struct TfaUpdateInfo {
-    /// The id if a newly added TFA entry.
-    id: Option<String>,
-
-    /// When adding u2f entries, this contains a challenge the user must respond to in order to
-    /// finish the registration.
-    #[serde(skip_serializing_if = "Option::is_none")]
-    challenge: Option<String>,
-
-    /// When adding recovery codes, this contains the list of codes to be displayed to the user
-    /// this one time.
-    #[serde(skip_serializing_if = "Vec::is_empty", default)]
-    recovery: Vec<String>,
-}
-
-impl TfaUpdateInfo {
-    fn id(id: String) -> Self {
-        Self {
-            id: Some(id),
-            ..Default::default()
-        }
-    }
 }
 
 #[api(
@@ -392,7 +174,7 @@ impl TfaUpdateInfo {
                 max_length: 255,
                 optional: true,
             },
-            "type": { type: TfaType },
+            "type": { type: methods::TfaType },
             totp: {
                 description: "A totp URI.",
                 optional: true,
@@ -412,7 +194,7 @@ impl TfaUpdateInfo {
             },
         },
     },
-    returns: { type: TfaUpdateInfo },
+    returns: { type: methods::TfaUpdateInfo },
     access: {
         permission: &Permission::Or(&[
             &Permission::Privilege(&["access", "users"], PRIV_PERMISSIONS_MODIFY, false),
@@ -429,90 +211,26 @@ fn add_tfa_entry(
     value: Option<String>,
     challenge: Option<String>,
     password: Option<String>,
-    r#type: TfaType,
+    r#type: methods::TfaType,
     rpcenv: &mut dyn RpcEnvironment,
-) -> Result<TfaUpdateInfo, Error> {
+) -> Result<methods::TfaUpdateInfo, Error> {
     tfa_update_auth(rpcenv, &userid, password, true)?;
 
-    let need_description =
+    let _lock = crate::config::tfa::write_lock()?;
-        move || description.ok_or_else(|| format_err!("'description' is required for new entries"));
 
-    match r#type {
+    let mut data = crate::config::tfa::read()?;
-        TfaType::Totp => match (totp, value) {
+    let out = methods::add_tfa_entry(
-            (Some(totp), Some(value)) => {
+        &mut data,
-                if challenge.is_some() {
+        UserAccess,
-                    bail!("'challenge' parameter is invalid for 'totp' entries");
+        userid.as_str(),
-                }
+        description,
-                let description = need_description()?;
+        totp,
-
+        value,
-                let totp: Totp = totp.parse()?;
+        challenge,
-                if totp
+        r#type,
-                    .verify(&value, std::time::SystemTime::now(), -1..=1)?
+    )?;
-                    .is_none()
+    crate::config::tfa::write(&data)?;
-                {
+    Ok(out)
-                    bail!("failed to verify TOTP challenge");
-                }
-                crate::config::tfa::add_totp(&userid, description, totp).map(TfaUpdateInfo::id)
-            }
-            _ => bail!("'totp' type requires both 'totp' and 'value' parameters"),
-        },
-        TfaType::Webauthn => {
-            if totp.is_some() {
-                bail!("'totp' parameter is invalid for 'totp' entries");
-            }
-
-            match challenge {
-                None => crate::config::tfa::add_webauthn_registration(&userid, need_description()?)
-                    .map(|c| TfaUpdateInfo {
-                        challenge: Some(c),
-                        ..Default::default()
-                    }),
-                Some(challenge) => {
-                    let value = value.ok_or_else(|| {
-                        format_err!(
-                            "missing 'value' parameter (webauthn challenge response missing)"
-                        )
-                    })?;
-                    crate::config::tfa::finish_webauthn_registration(&userid, &challenge, &value)
-                        .map(TfaUpdateInfo::id)
-                }
-            }
-        }
-        TfaType::U2f => {
-            if totp.is_some() {
-                bail!("'totp' parameter is invalid for 'totp' entries");
-            }
-
-            match challenge {
-                None => crate::config::tfa::add_u2f_registration(&userid, need_description()?).map(
-                    |c| TfaUpdateInfo {
-                        challenge: Some(c),
-                        ..Default::default()
-                    },
-                ),
-                Some(challenge) => {
-                    let value = value.ok_or_else(|| {
-                        format_err!("missing 'value' parameter (u2f challenge response missing)")
-                    })?;
-                    crate::config::tfa::finish_u2f_registration(&userid, &challenge, &value)
-                        .map(TfaUpdateInfo::id)
-                }
-            }
-        }
-        TfaType::Recovery => {
-            if totp.or(value).or(challenge).is_some() {
-                bail!("generating recovery tokens does not allow additional parameters");
-            }
-
-            let recovery = crate::config::tfa::add_recovery(&userid)?;
-
-            Ok(TfaUpdateInfo {
-                id: Some("recovery".to_string()),
-                recovery,
-                ..Default::default()
-            })
-        }
-    }
 }
 
 #[api(
@@ -560,21 +278,10 @@ fn update_tfa_entry(
     let _lock = crate::config::tfa::write_lock()?;
 
     let mut data = crate::config::tfa::read()?;
-
+    match methods::update_tfa_entry(&mut data, userid.as_str(), &id, description, enable) {
-    let mut entry = data
+        Ok(()) => (),
-        .users
+        Err(methods::EntryNotFound) => http_bail!(NOT_FOUND, "no such entry: {}/{}", userid, id),
-        .get_mut(&userid)
-        .and_then(|user| user.find_entry_mut(&id))
-        .ok_or_else(|| http_err!(NOT_FOUND, "no such entry: {}/{}", userid, id))?;
-
-    if let Some(description) = description {
-        entry.description = description;
     }
-
-    if let Some(enable) = enable {
-        entry.enable = enable;
-    }
-
     crate::config::tfa::write(&data)?;
     Ok(())
 }

src/api2/access/user.rs

@@ -374,7 +374,8 @@ pub fn delete_user(userid: Userid, digest: Option<String>) -> Result<(), Error>
     }
 
     match crate::config::tfa::read().and_then(|mut cfg| {
-        let _: bool = cfg.remove_user(&userid);
+        let _: proxmox_tfa::api::NeedsSaving =
+            cfg.remove_user(crate::config::tfa::UserAccess, userid.as_str())?;
         crate::config::tfa::write(&cfg)
     }) {
         Ok(()) => (),

src/api2/config/access/tfa.rs

@@ -73,7 +73,10 @@ pub fn update_webauthn_config(
     if let Some(wa) = &mut tfa.webauthn {
         if let Some(ref digest) = digest {
             let digest = proxmox::tools::hex_to_digest(digest)?;
-            crate::tools::detect_modified_configuration_file(&digest, &wa.digest()?)?;
+            crate::tools::detect_modified_configuration_file(
+                &digest,
+                &crate::config::tfa::webauthn_config_digest(&wa)?,
+            )?;
         }
         if let Some(ref rp) = webauthn.rp { wa.rp = rp.clone(); }
         if let Some(ref origin) = webauthn.rp { wa.origin = origin.clone(); }