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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

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