[pbs-devel] [PATCH backup] check only user part of backup owner for comparisons

* [pbs-devel] [PATCH backup] check only user part of backup owner for comparisons
@ 2024-10-15 13:31 Maximiliano Sandoval
  2024-10-16  7:03 ` [pbs-devel] NACK: " Fabian Grünbichler
  0 siblings, 1 reply; 2+ messages in thread
From: Maximiliano Sandoval @ 2024-10-15 13:31 UTC (permalink / raw)
  To: 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 <m.sandoval@proxmox.com>
---

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<Mutex<HashMap<String, Arc<DataStoreImpl>>>> =
     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


^ permalink raw reply	[flat|nested] 2+ messages in thread