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 489201FF16F for ; Tue, 15 Oct 2024 15:31:09 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 1E545162D5; Tue, 15 Oct 2024 15:31:42 +0200 (CEST) From: Maximiliano Sandoval To: pbs-devel@lists.proxmox.com Date: Tue, 15 Oct 2024 15:31:08 +0200 Message-Id: <20241015133108.345076-1-m.sandoval@proxmox.com> X-Mailer: git-send-email 2.39.5 MIME-Version: 1.0 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.099 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 Subject: [pbs-devel] [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" 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()); } 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() + ); } } -- 2.39.5 _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel