[pbs-devel] [PATCH pbs 1/2] pbs-datastore: paperkey: Add padding to utf8 qrcodes

[pbs-devel] [PATCH pbs 1/2] pbs-datastore: paperkey: Add padding to utf8 qrcodes

[Maximiliano Sandoval]

Many QR code scanners, including ZBar for example, cannot read qr codes
properly when there is not padding around them.

Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---
 pbs-datastore/src/paperkey.rs | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/pbs-datastore/src/paperkey.rs b/pbs-datastore/src/paperkey.rs
index 14b62264..6d8cd914 100644
--- a/pbs-datastore/src/paperkey.rs
+++ b/pbs-datastore/src/paperkey.rs
@@ -226,8 +226,12 @@ fn paperkey_text<W: Write>(
 }
 
 fn generate_qr_code(output_type: &str, lines: &[String]) -> Result<Vec<u8>, Error> {
+    let padding = match output_type {
+        "utf8" | "utf8i" => "-m2",
+        _ => "-m0",
+    };
     let mut child = Command::new("qrencode")
-        .args(["-t", output_type, "-m0", "-s1", "-lm", "--output", "-"])
+        .args(["-t", output_type, padding, "-s1", "-lm", "--output", "-"])
         .stdin(Stdio::piped())
         .stdout(Stdio::piped())
         .spawn()?;
-- 
2.39.2

[pbs-devel] [PATCH pbs 2/2] fix #4734: manager: add tfa {list, delete, add} commands

[Maximiliano Sandoval]

Adds the commands

    proxmox-backup-manager tfa list <userid>
    proxmox-backup-manager tfa delete <userid> <id>
    proxmox-backup-manager tfa add <userid> <type>

The rationale for not listing these commands under
proxmox-backup-manager user is that in this way we match the API better.

The command `tfa add` will present the user with a scannable QR code and
a prompt to enter the associated 6 digit token for validation.

The function api2::access::list_user_tfa needed an additional `returns`
annotation, otherwise one gets the following error

    unable to format result: got unexpected data (expected null).

Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---
 pbs-datastore/src/paperkey.rs         |   2 +-
 src/api2/access/tfa.rs                |  11 +-
 src/bin/proxmox-backup-manager.rs     |   1 +
 src/bin/proxmox_backup_manager/mod.rs |   2 +
 src/bin/proxmox_backup_manager/tfa.rs | 181 ++++++++++++++++++++++++++
 src/config/tfa.rs                     |  48 +++++++
 6 files changed, 241 insertions(+), 4 deletions(-)
 create mode 100644 src/bin/proxmox_backup_manager/tfa.rs

diff --git a/pbs-datastore/src/paperkey.rs b/pbs-datastore/src/paperkey.rs
index 6d8cd914..11dc8fe3 100644
--- a/pbs-datastore/src/paperkey.rs
+++ b/pbs-datastore/src/paperkey.rs
@@ -225,7 +225,7 @@ fn paperkey_text<W: Write>(
     Ok(())
 }
 
-fn generate_qr_code(output_type: &str, lines: &[String]) -> Result<Vec<u8>, Error> {
+pub fn generate_qr_code(output_type: &str, lines: &[String]) -> Result<Vec<u8>, Error> {
     let padding = match output_type {
         "utf8" | "utf8i" => "-m2",
         _ => "-m0",
diff --git a/src/api2/access/tfa.rs b/src/api2/access/tfa.rs
index 82816d21..595c9433 100644
--- a/src/api2/access/tfa.rs
+++ b/src/api2/access/tfa.rs
@@ -54,6 +54,11 @@ async fn tfa_update_auth(
     input: {
         properties: { userid: { type: Userid } },
     },
+    returns: {
+        description: "The list of TFA entries.",
+        type: Array,
+        items: { type: methods::TypedTfaInfo }
+    },
     access: {
         permission: &Permission::Or(&[
             &Permission::Privilege(&["access", "users"], PRIV_PERMISSIONS_MODIFY, false),
@@ -62,7 +67,7 @@ async fn tfa_update_auth(
     },
 )]
 /// Add a TOTP secret to the user.
-fn list_user_tfa(userid: Userid) -> Result<Vec<methods::TypedTfaInfo>, Error> {
+pub fn list_user_tfa(userid: Userid) -> Result<Vec<methods::TypedTfaInfo>, Error> {
     let _lock = crate::config::tfa::read_lock()?;
 
     methods::list_user_tfa(&crate::config::tfa::read()?, userid.as_str())
@@ -115,7 +120,7 @@ fn get_tfa_entry(userid: Userid, id: String) -> Result<methods::TypedTfaInfo, Er
     },
 )]
 /// Delete a single TFA entry.
-async fn delete_tfa(
+pub async fn delete_tfa(
     userid: Userid,
     id: String,
     password: Option<String>,
@@ -208,7 +213,7 @@ fn list_tfa(rpcenv: &mut dyn RpcEnvironment) -> Result<Vec<methods::TfaUser>, Er
 )]
 /// Add a TFA entry to the user.
 #[allow(clippy::too_many_arguments)]
-async fn add_tfa_entry(
+pub async fn add_tfa_entry(
     userid: Userid,
     description: Option<String>,
     totp: Option<String>,
diff --git a/src/bin/proxmox-backup-manager.rs b/src/bin/proxmox-backup-manager.rs
index b4cb6cb3..a8c1046c 100644
--- a/src/bin/proxmox-backup-manager.rs
+++ b/src/bin/proxmox-backup-manager.rs
@@ -440,6 +440,7 @@ async fn run() -> Result<(), Error> {
         .insert("network", network_commands())
         .insert("node", node_commands())
         .insert("user", user_commands())
+        .insert("tfa", tfa_commands())
         .insert("openid", openid_commands())
         .insert("remote", remote_commands())
         .insert("traffic-control", traffic_control_commands())
diff --git a/src/bin/proxmox_backup_manager/mod.rs b/src/bin/proxmox_backup_manager/mod.rs
index 8a1c140c..4a2e6189 100644
--- a/src/bin/proxmox_backup_manager/mod.rs
+++ b/src/bin/proxmox_backup_manager/mod.rs
@@ -32,3 +32,5 @@ mod openid;
 pub use openid::*;
 mod traffic_control;
 pub use traffic_control::*;
+mod tfa;
+pub use tfa::*;
diff --git a/src/bin/proxmox_backup_manager/tfa.rs b/src/bin/proxmox_backup_manager/tfa.rs
new file mode 100644
index 00000000..9a1bd073
--- /dev/null
+++ b/src/bin/proxmox_backup_manager/tfa.rs
@@ -0,0 +1,181 @@
+use std::io::Write;
+use std::str::FromStr;
+
+use anyhow::{bail, Error};
+use serde_json::{json, Value};
+
+use pbs_api_types::Userid;
+use proxmox_backup::api2;
+use proxmox_router::{cli::*, ApiHandler, RpcEnvironment};
+use proxmox_schema::{api, api_types::PASSWORD_SCHEMA};
+use proxmox_sys::linux::tty;
+use proxmox_tfa::{api::methods, totp::Totp};
+
+fn generate_totp_uri(issuer: &str, account_name: &str) -> Result<String, Error> {
+    // Generate a CSPRNG binary value of 160 bits, the recomended size
+    // from [rfc-4226](https://www.rfc-editor.org/rfc/rfc4226#section-4)
+    //
+    // > The length of the shared secret MUST be at least 128 bits.
+    // > This document RECOMMENDs a shared secret length of 160 bits.
+    //
+    // The generated secret is not guaranteed to be a valid UTF-8
+    // sequence
+    //
+    // See https://github.com/constantoine/totp-rs/blob/e4e055dedddc012cd691dc7156ee707843dd8105/src/secret.rs#L166
+    let secret = proxmox_sys::linux::random_data(20)?;
+
+    let totp = Totp::builder()
+        .secret(secret)
+        .issuer(issuer.to_string())
+        .account_name(account_name.to_string())
+        .build();
+
+    Ok(totp.to_uri()?)
+}
+
+#[api(
+    input: {
+        properties: {
+            "output-format": {
+                schema: OUTPUT_FORMAT,
+                optional: true,
+            },
+            userid: {
+                type: Userid,
+            }
+        },
+    }
+)]
+/// List configured users.
+fn list_user_tfa(param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Error> {
+    let output_format = get_output_format(&param);
+
+    let info = &api2::access::tfa::API_METHOD_LIST_USER_TFA;
+    let mut data = match info.handler {
+        ApiHandler::Sync(handler) => (handler)(param, info, rpcenv)?,
+        _ => unreachable!(),
+    };
+
+    let options = default_table_format_options()
+        .column(ColumnConfig::new("id"))
+        .column(ColumnConfig::new("type"))
+        .column(ColumnConfig::new("description"))
+        .column(ColumnConfig::new("created").renderer(pbs_tools::format::render_epoch));
+
+    format_and_print_result_full(&mut data, &info.returns, &output_format, &options);
+
+    Ok(Value::Null)
+}
+
+#[api(
+    input: {
+        properties: {
+            "output-format": {
+                schema: OUTPUT_FORMAT,
+                optional: true,
+            },
+            userid: {
+                type: Userid,
+            },
+            "type": {
+                type: methods::TfaType,
+            },
+            description: {
+                description: "A description to distinguish multiple entries from one another",
+                type: String,
+                max_length: 255,
+                optional: true,
+            },
+            password: {
+                schema: PASSWORD_SCHEMA,
+                optional: true,
+            },
+        },
+    },
+    returns: { type: methods::TfaUpdateInfo },
+)]
+/// Add a tfa entry for a user
+async fn add_tfa_entry(param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Error> {
+    let output_format = get_output_format(&param);
+
+    let info = &api2::access::tfa::API_METHOD_ADD_TFA_ENTRY;
+
+    let mut param = param;
+
+    match param
+        .get("type")
+        .and_then(|v| v.as_str())
+        .and_then(|s| methods::TfaType::from_str(s).ok())
+    {
+        Some(methods::TfaType::Totp) => {
+            if !tty::stdin_isatty() {
+                bail!("unable to generate qr code - no tty");
+            }
+            if param.get("description").is_none() {
+                bail!("Error: 'description' is required for new totp entries");
+            }
+
+            let userid = param
+                .get("userid")
+                .and_then(|v| v.as_str())
+                .unwrap_or("User");
+
+            let uri = generate_totp_uri("Proxmox", userid)?;
+            param["totp"] = json!(&uri);
+
+            // We use utf8 as it displays the codes as black on white which is
+            // readed by most qr scanners.
+            let qrcode = pbs_datastore::paperkey::generate_qr_code("utf8", &[uri])?;
+            let stdout = std::io::stdout();
+
+            let mut writer = stdout.lock();
+
+            writer.write(&qrcode)?;
+            writer.write(b"Scan QR code in a TOTP app and enter an authentication code here\n")?;
+
+            let value = String::from_utf8(tty::read_password("Verify Code: ")?)?;
+
+            param["value"] = json!(value);
+        }
+        Some(methods::TfaType::Recovery) => (),
+        Some(methods::TfaType::Webauthn) => bail!("WebAuthn is not supported from cli"),
+        Some(methods::TfaType::U2f) => bail!("U2F is not supported from cli"),
+        Some(methods::TfaType::Yubico) => bail!("Yubico is not supported from cli"),
+        None => unreachable!(),
+    }
+    let mut data = match info.handler {
+        ApiHandler::Async(handler) => (handler)(param, info, rpcenv).await?,
+        _ => unreachable!(),
+    };
+
+    let options = default_table_format_options().column(ColumnConfig::new("id"));
+
+    format_and_print_result_full(&mut data, &info.returns, &output_format, &options);
+
+    Ok(Value::Null)
+}
+
+pub fn tfa_commands() -> CommandLineInterface {
+    let cmd_def = CliCommandMap::new()
+        .insert(
+            "list",
+            CliCommand::new(&API_METHOD_LIST_USER_TFA)
+                .arg_param(&["userid"])
+                .completion_cb("userid", pbs_config::user::complete_userid),
+        )
+        .insert(
+            "add",
+            CliCommand::new(&API_METHOD_ADD_TFA_ENTRY)
+                .arg_param(&["userid", "type"])
+                .completion_cb("userid", pbs_config::user::complete_userid),
+        )
+        .insert(
+            "delete",
+            CliCommand::new(&api2::access::tfa::API_METHOD_DELETE_TFA)
+                .arg_param(&["userid", "id"])
+                .completion_cb("userid", pbs_config::user::complete_userid)
+                .completion_cb("id", proxmox_backup::config::tfa::complete_tfa_id),
+        );
+
+    cmd_def.into()
+}
diff --git a/src/config/tfa.rs b/src/config/tfa.rs
index 6b1312bb..bc5a2f85 100644
--- a/src/config/tfa.rs
+++ b/src/config/tfa.rs
@@ -1,3 +1,4 @@
+use std::collections::HashMap;
 use std::fs::File;
 use std::io::{self, Read, Seek, SeekFrom};
 use std::os::unix::fs::OpenOptionsExt;
@@ -302,3 +303,50 @@ impl proxmox_tfa::api::UserChallengeAccess for TfaUserChallengeData {
         TfaUserChallengeData::save(self)
     }
 }
+
+// shell completion helper
+pub fn complete_tfa_id(_arg: &str, param: &HashMap<String, String>) -> Vec<String> {
+    let mut results = Vec::new();
+
+    let json = match read() {
+        Ok(json) => json,
+        Err(_err) => return results,
+    };
+    let user = match param
+        .get("userid")
+        .and_then(|user_name| json.users.get(user_name))
+    {
+        Some(user) => user,
+        None => return results,
+    };
+
+    results.extend(
+        user.totp
+            .iter()
+            .map(|token| token.info.id.clone())
+            .collect::<Vec<_>>(),
+    );
+    results.extend(
+        user.u2f
+            .iter()
+            .map(|token| token.info.id.clone())
+            .collect::<Vec<_>>(),
+    );
+    results.extend(
+        user.webauthn
+            .iter()
+            .map(|token| token.info.id.clone())
+            .collect::<Vec<_>>(),
+    );
+    results.extend(
+        user.yubico
+            .iter()
+            .map(|token| token.info.id.clone())
+            .collect::<Vec<_>>(),
+    );
+    if user.recovery.is_some() {
+        results.push("recovery".to_string());
+    };
+
+    results
+}
-- 
2.39.2

Re: [pbs-devel] [PATCH pbs 2/2] fix #4734: manager: add tfa {list, delete, add} commands

[Wolfgang Bumiller]

On Tue, Jun 13, 2023 at 12:47:30PM +0200, Maximiliano Sandoval wrote:
> Adds the commands
> 
>     proxmox-backup-manager tfa list <userid>
>     proxmox-backup-manager tfa delete <userid> <id>
>     proxmox-backup-manager tfa add <userid> <type>

I'm not a fan of having an 'add' command available here. Especially if
TOTP is the only supported variant. (And it's way too inconvenient to
show the QR code or type off the otpauth uri.)
(I'd rather at some point add WA support there but I don't think it
makes much sense to do this over the CLI really...)

IMO `list` and `delete` should be enough. `unlock` will also come later
on.

> 
> The rationale for not listing these commands under
> proxmox-backup-manager user is that in this way we match the API better.

We have `pveum user tfa <...>` though and we're managing something
that's part of the user, so I think it would still make sense?

> 
> The command `tfa add` will present the user with a scannable QR code and
> a prompt to enter the associated 6 digit token for validation.

What if I'm logged in via SSH?

> 
> The function api2::access::list_user_tfa needed an additional `returns`
> annotation, otherwise one gets the following error
> 
>     unable to format result: got unexpected data (expected null).
> 

return schema fixups are always good ;-)

> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> ---
>  pbs-datastore/src/paperkey.rs         |   2 +-
>  src/api2/access/tfa.rs                |  11 +-
>  src/bin/proxmox-backup-manager.rs     |   1 +
>  src/bin/proxmox_backup_manager/mod.rs |   2 +
>  src/bin/proxmox_backup_manager/tfa.rs | 181 ++++++++++++++++++++++++++
>  src/config/tfa.rs                     |  48 +++++++
>  6 files changed, 241 insertions(+), 4 deletions(-)
>  create mode 100644 src/bin/proxmox_backup_manager/tfa.rs
> 
(...)
> diff --git a/src/config/tfa.rs b/src/config/tfa.rs
> index 6b1312bb..bc5a2f85 100644
> --- a/src/config/tfa.rs
> +++ b/src/config/tfa.rs
> @@ -1,3 +1,4 @@
> +use std::collections::HashMap;
>  use std::fs::File;
>  use std::io::{self, Read, Seek, SeekFrom};
>  use std::os::unix::fs::OpenOptionsExt;
> @@ -302,3 +303,50 @@ impl proxmox_tfa::api::UserChallengeAccess for TfaUserChallengeData {
>          TfaUserChallengeData::save(self)
>      }
>  }
> +
> +// shell completion helper
> +pub fn complete_tfa_id(_arg: &str, param: &HashMap<String, String>) -> Vec<String> {
> +    let mut results = Vec::new();
> +
> +    let json = match read() {

^ This is a `TfaConfig` instance, not json data, please don't call it
json for no reason ;-)
(The rest of the file just calls it `data`.)

> +        Ok(json) => json,
> +        Err(_err) => return results,
> +    };
> +    let user = match param
> +        .get("userid")
> +        .and_then(|user_name| json.users.get(user_name))
> +    {
> +        Some(user) => user,
> +        None => return results,
> +    };
> +
> +    results.extend(
> +        user.totp
> +            .iter()
> +            .map(|token| token.info.id.clone())
> +            .collect::<Vec<_>>(),
> +    );
> +    results.extend(
> +        user.u2f
> +            .iter()
> +            .map(|token| token.info.id.clone())
> +            .collect::<Vec<_>>(),
> +    );
> +    results.extend(
> +        user.webauthn
> +            .iter()
> +            .map(|token| token.info.id.clone())
> +            .collect::<Vec<_>>(),
> +    );
> +    results.extend(
> +        user.yubico
> +            .iter()
> +            .map(|token| token.info.id.clone())
> +            .collect::<Vec<_>>(),
> +    );
> +    if user.recovery.is_some() {
> +        results.push("recovery".to_string());
> +    };
> +
> +    results
> +}
> -- 
> 2.39.2