* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal