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

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

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