From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id BD7EA1FF15C for ; Wed, 16 Oct 2024 09:02:34 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id A381630692; Wed, 16 Oct 2024 09:03:07 +0200 (CEST) Date: Wed, 16 Oct 2024 09:03:00 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox Backup Server development discussion References: <20241015133108.345076-1-m.sandoval@proxmox.com> In-Reply-To: <20241015133108.345076-1-m.sandoval@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid) Message-Id: <1729061622.yvcb60ha1s.astroid@yuna.none> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.048 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 RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record 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] Subject: [pbs-devel] NACK: [PATCH backup] check only user part of backup owner for comparisons 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: , Reply-To: Proxmox Backup Server development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" this is wrong, and I already told you that before you wrote the patch? quoting myself: > different tokens should be handed out to different systems, and system > A should not be able to access backups made by system B unless it is > highly privileged the existing check is intentional and correct. there are other issues surrounding ownership that we might want to tackle: - (optionally?) remove owner when removing last snapshot in a group[0] - list groups without snapshots on the UI (requires restructuring the whole datastore browser, which might be nice anyway for performance reasons - e.g., we could have a streaming browser or a "browser session" similar to the backup/reader ones) - multiple owners [0]: the two issues you linked On October 15, 2024 3:31 pm, Maximiliano Sandoval wrote: > Prevents the errors: > > INFO: Error: backup owner check failed (user@domain!first != user@domain!second) > > when making backups of a guest using different tokens. This can for > example happen when the user has to generate a new token. > > Signed-off-by: Maximiliano Sandoval > --- > > I am not entirely sure if this proposed behavior is a desirable one. The docs > [1] don't mention any token. > > I run into this issue multiple times a week and bugs like [2, 3] make it > (changing the owner of a dozen backups) more cumbersome that it already is. > > [1] https://pbs.proxmox.com/docs/backup-client.html#changing-the-owner-of-a-backup-group > [2] https://bugzilla.proxmox.com/show_bug.cgi?id=5621 > [3] https://bugzilla.proxmox.com/show_bug.cgi?id=3336 > > > pbs-datastore/src/datastore.rs | 62 +++++++++++++++++++++++++++++++--- > src/api2/backup/mod.rs | 4 +-- > src/api2/reader/mod.rs | 10 ++++-- > 3 files changed, 66 insertions(+), 10 deletions(-) > > diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs > index d0f3c53ac..e84139eb5 100644 > --- a/pbs-datastore/src/datastore.rs > +++ b/pbs-datastore/src/datastore.rs > @@ -35,13 +35,15 @@ use crate::DataBlob; > static DATASTORE_MAP: LazyLock>>> = > LazyLock::new(|| Mutex::new(HashMap::new())); > > -/// checks if auth_id is owner, or, if owner is a token, if > -/// auth_id is the user of the token > +/// checks if auth_id's user matches the owner's user. > pub fn check_backup_owner(owner: &Authid, auth_id: &Authid) -> Result<(), Error> { > - let correct_owner = > - owner == auth_id || (owner.is_token() && &Authid::from(owner.user().clone()) == auth_id); > + let correct_owner = owner.user() == auth_id.user(); > if !correct_owner { > - bail!("backup owner check failed ({} != {})", auth_id, owner); > + bail!( > + "backup owner check failed ({} != {})", > + auth_id.user(), > + owner.user() > + ); > } > Ok(()) > } > @@ -1466,3 +1468,53 @@ impl DataStore { > Ok(()) > } > } > + > +#[cfg(test)] > +mod test { > + use std::str::FromStr; > + > + use pbs_api_types::{Tokenname, Userid}; > + > + use super::*; > + > + #[test] > + fn test_check_backup_owner() { > + let user_a = Userid::from_str("user-a@some-realm").unwrap(); > + let user_b = Userid::from_str("user-b@some-realm").unwrap(); > + let token_a = Tokenname::try_from("token-a".to_string()).unwrap(); > + let token_b = Tokenname::try_from("token-b".to_string()).unwrap(); > + > + // Same users. > + assert!(check_backup_owner( > + &Authid::from((user_a.clone(), Some(token_a.clone()))), > + &Authid::from((user_a.clone(), Some(token_b.clone()))) > + ) > + .is_ok()); > + assert!(check_backup_owner( > + &Authid::from((user_a.clone(), Some(token_a.clone()))), > + &Authid::from((user_a.clone(), None)) > + ) > + .is_ok()); > + assert!(check_backup_owner( > + &Authid::from((user_a.clone(), None)), > + &Authid::from((user_a.clone(), None)) > + ) > + .is_ok()); > + > + // Different users. > + assert!(check_backup_owner( > + &Authid::from((user_a.clone(), Some(token_a.clone()))), > + &Authid::from((user_b.clone(), Some(token_b))) > + ) > + .is_err()); > + assert!(check_backup_owner( > + &Authid::from((user_a.clone(), Some(token_a))), > + &Authid::from((user_b.clone(), None)) > + ) > + .is_err()); > + assert!( > + check_backup_owner(&Authid::from((user_a, None)), &Authid::from((user_b, None))) > + .is_err() > + ); > + } > +} > diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs > index ea0d0292e..8e4cd297f 100644 > --- a/src/api2/backup/mod.rs > +++ b/src/api2/backup/mod.rs > @@ -149,10 +149,10 @@ fn upgrade_to_backup_protocol( > > // permission check > let correct_owner = > - owner == auth_id || (owner.is_token() && Authid::from(owner.user().clone()) == auth_id); > + owner.user() == auth_id.user(); > if !correct_owner && worker_type != "benchmark" { > // only the owner is allowed to create additional snapshots > - bail!("backup owner check failed ({} != {})", auth_id, owner); > + bail!("backup owner check failed ({} != {})", auth_id.user(), owner.user()); but this > } > > let last_backup = { > diff --git a/src/api2/reader/mod.rs b/src/api2/reader/mod.rs > index 23051653e..2d192a979 100644 > --- a/src/api2/reader/mod.rs > +++ b/src/api2/reader/mod.rs > @@ -119,10 +119,14 @@ fn upgrade_to_backup_reader_protocol( > let backup_dir = datastore.backup_dir(backup_ns, backup_dir)?; > if !priv_read { > let owner = backup_dir.get_owner()?; > - let correct_owner = owner == auth_id > - || (owner.is_token() && Authid::from(owner.user().clone()) == auth_id); > + let correct_owner = owner.user() == auth_id.user(); > if !correct_owner { > - bail!("backup owner check failed!"); > + // only the owner is allowed to create additional snapshots > + bail!( > + "backup owner check failed ({} != {})", > + auth_id.user(), > + owner.user() > + ); and this could be replaced with a call to the helper, so that there's a single place where that is implemented? > } > } > > -- > 2.39.5 > > > > _______________________________________________ > pbs-devel mailing list > pbs-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel > > > _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel