* [pbs-devel] [PATCH pbs 2/2] fix #4734: manager: add tfa {list, delete, add} commands
2023-06-13 10:47 [pbs-devel] [PATCH pbs 1/2] pbs-datastore: paperkey: Add padding to utf8 qrcodes Maximiliano Sandoval
@ 2023-06-13 10:47 ` Maximiliano Sandoval
2023-06-20 11:54 ` Wolfgang Bumiller
0 siblings, 1 reply; 3+ messages in thread
From: Maximiliano Sandoval @ 2023-06-13 10:47 UTC (permalink / raw)
To: pbs-devel
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(¶m);
+
+ 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(¶m);
+
+ 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
^ permalink raw reply [flat|nested] 3+ messages in thread