From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id E7B6FA2C89 for ; Tue, 20 Jun 2023 13:54:48 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id C527C348A0 for ; Tue, 20 Jun 2023 13:54:48 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Tue, 20 Jun 2023 13:54:48 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id EA5DF41C98 for ; Tue, 20 Jun 2023 13:54:47 +0200 (CEST) Date: Tue, 20 Jun 2023 13:54:46 +0200 From: Wolfgang Bumiller To: Maximiliano Sandoval Cc: pbs-devel@lists.proxmox.com Message-ID: References: <20230613104730.120991-1-m.sandoval@proxmox.com> <20230613104730.120991-2-m.sandoval@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230613104730.120991-2-m.sandoval@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.124 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [mod.rs, tfa.rs, paperkey.rs, proxmox-backup-manager.rs] Subject: Re: [pbs-devel] [PATCH pbs 2/2] fix #4734: manager: add tfa {list, delete, add} commands X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 20 Jun 2023 11:54:49 -0000 On Tue, Jun 13, 2023 at 12:47:30PM +0200, Maximiliano Sandoval wrote: > Adds the commands > > proxmox-backup-manager tfa list > proxmox-backup-manager tfa delete > proxmox-backup-manager tfa add 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 > --- > 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) -> Vec { > + 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::>(), > + ); > + results.extend( > + user.u2f > + .iter() > + .map(|token| token.info.id.clone()) > + .collect::>(), > + ); > + results.extend( > + user.webauthn > + .iter() > + .map(|token| token.info.id.clone()) > + .collect::>(), > + ); > + results.extend( > + user.yubico > + .iter() > + .map(|token| token.info.id.clone()) > + .collect::>(), > + ); > + if user.recovery.is_some() { > + results.push("recovery".to_string()); > + }; > + > + results > +} > -- > 2.39.2