* [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* [pbs-devel] NACK: [PATCH backup] check only user part of backup owner for comparisons
2024-10-15 13:31 [pbs-devel] [PATCH backup] check only user part of backup owner for comparisons Maximiliano Sandoval
@ 2024-10-16 7:03 ` Fabian Grünbichler
0 siblings, 0 replies; 2+ messages in thread
From: Fabian Grünbichler @ 2024-10-16 7:03 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
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 <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());
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2024-10-16 7:02 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-10-15 13:31 [pbs-devel] [PATCH backup] check only user part of backup owner for comparisons Maximiliano Sandoval
2024-10-16 7:03 ` [pbs-devel] NACK: " Fabian Grünbichler
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox