Re: [pbs-devel] [PATCH v5 proxmox-backup 17/31] api: config: extend modify access check by sync direction

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

On October 18, 2024 10:42 am, Christian Ebner wrote:
> Add the sync direction as additional parameter for the priv helper to
> check for the required permissions in pull and push direction.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
> changes since version 4:
> - no changes
> 
> changes since version 3:
> - not present in previous version
> 
>  src/api2/admin/sync.rs  |   4 +-
>  src/api2/config/sync.rs | 136 +++++++++++++++++++++++++++++-----------
>  2 files changed, 100 insertions(+), 40 deletions(-)
> 
> diff --git a/src/api2/admin/sync.rs b/src/api2/admin/sync.rs
> index 7a4e38942..f2c0f0e85 100644
> --- a/src/api2/admin/sync.rs
> +++ b/src/api2/admin/sync.rs
> @@ -122,8 +122,8 @@ pub fn run_sync_job(
>      let sync_direction = sync_direction.unwrap_or_default();
>      let sync_job: SyncJobConfig = config.lookup(sync_direction.as_config_type_str(), &id)?;
>  
> -    if !check_sync_job_modify_access(&user_info, &auth_id, &sync_job) {
> -        bail!("permission check failed");
> +    if !check_sync_job_modify_access(&user_info, &auth_id, &sync_job, sync_direction) {
> +        bail!("permission check failed, '{auth_id}' is missing access");
>      }
>  
>      let job = Job::new("syncjob", &id)?;
> diff --git a/src/api2/config/sync.rs b/src/api2/config/sync.rs
> index e0d96afe5..cffcf429f 100644
> --- a/src/api2/config/sync.rs
> +++ b/src/api2/config/sync.rs
> @@ -9,8 +9,9 @@ use proxmox_schema::{api, param_bail};
>  
>  use pbs_api_types::{
>      Authid, SyncJobConfig, SyncJobConfigUpdater, JOB_ID_SCHEMA, PRIV_DATASTORE_AUDIT,
> -    PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_MODIFY, PRIV_DATASTORE_PRUNE, PRIV_REMOTE_AUDIT,
> -    PRIV_REMOTE_READ, PROXMOX_CONFIG_DIGEST_SCHEMA,
> +    PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_MODIFY, PRIV_DATASTORE_PRUNE, PRIV_DATASTORE_READ,
> +    PRIV_REMOTE_AUDIT, PRIV_REMOTE_DATASTORE_BACKUP, PRIV_REMOTE_DATASTORE_MODIFY,
> +    PRIV_REMOTE_DATASTORE_PRUNE, PRIV_REMOTE_READ, PROXMOX_CONFIG_DIGEST_SCHEMA,
>  };
>  use pbs_config::sync;
>  
> @@ -63,36 +64,77 @@ fn is_correct_owner(auth_id: &Authid, job: &SyncJobConfig) -> bool {
>      }
>  }
>  
> -/// checks whether user can run the corresponding pull job
> +/// checks whether user can run the corresponding sync job, depending on sync direction
>  ///
> -/// namespace creation/deletion ACL and backup group ownership checks happen in the pull code directly.
> +/// namespace creation/deletion ACL and backup group ownership checks happen in the pull/push code
> +/// directly.
>  /// remote side checks/filters remote datastore/namespace/group access.
>  pub fn check_sync_job_modify_access(
>      user_info: &CachedUserInfo,
>      auth_id: &Authid,
>      job: &SyncJobConfig,
> +    sync_direction: SyncDirection,
>  ) -> bool {
> -    let ns_anchor_privs = user_info.lookup_privs(auth_id, &job.acl_path());
> -    if ns_anchor_privs & PRIV_DATASTORE_BACKUP == 0 || ns_anchor_privs & PRIV_DATASTORE_AUDIT == 0 {
> -        return false;
> -    }
> +    match sync_direction {
> +        SyncDirection::Pull => {
> +            let ns_anchor_privs = user_info.lookup_privs(auth_id, &job.acl_path());
> +            if ns_anchor_privs & PRIV_DATASTORE_BACKUP == 0
> +                || ns_anchor_privs & PRIV_DATASTORE_AUDIT == 0
> +            {
> +                return false;
> +            }
> +
> +            if let Some(true) = job.remove_vanished {
> +                if ns_anchor_privs & PRIV_DATASTORE_PRUNE == 0 {
> +                    return false;
> +                }
> +            }
> +
> +            // same permission as changing ownership after syncing
> +            if !is_correct_owner(auth_id, job) && ns_anchor_privs & PRIV_DATASTORE_MODIFY == 0 {
> +                return false;
> +            }
>  
> -    if let Some(true) = job.remove_vanished {
> -        if ns_anchor_privs & PRIV_DATASTORE_PRUNE == 0 {
> -            return false;
> +            if let Some(remote) = &job.remote {
> +                let remote_privs =
> +                    user_info.lookup_privs(auth_id, &["remote", remote, &job.remote_store]);
> +                return remote_privs & PRIV_REMOTE_READ != 0;
> +            }
> +            true
>          }
> -    }
> +        SyncDirection::Push => {
> +            // Remote must always be present for sync in push direction, fail otherwise
> +            let target_privs = if let Some(target_acl_path) = job.remote_acl_path() {
> +                user_info.lookup_privs(auth_id, &target_acl_path)
> +            } else {
> +                return false;
> +            };
>  
> -    // same permission as changing ownership after syncing
> -    if !is_correct_owner(auth_id, job) && ns_anchor_privs & PRIV_DATASTORE_MODIFY == 0 {
> -        return false;
> -    }
> +            // check user is allowed to create backups on remote datastore
> +            if target_privs & PRIV_REMOTE_DATASTORE_BACKUP == 0 {
> +                return false;
> +            }
>  
> -    if let Some(remote) = &job.remote {
> -        let remote_privs = user_info.lookup_privs(auth_id, &["remote", remote, &job.remote_store]);
> -        return remote_privs & PRIV_REMOTE_READ != 0;
> +            if let Some(true) = job.remove_vanished {
> +                // check user is allowed to prune backup snapshots on remote datastore
> +                if target_privs & PRIV_REMOTE_DATASTORE_PRUNE == 0 {
> +                    return false;
> +                }
> +            }
> +
> +            // check user is not the owner of the sync job, but has remote datastore modify permissions
> +            if !is_correct_owner(auth_id, job) && target_privs & PRIV_REMOTE_DATASTORE_MODIFY == 0 {
> +                return false;
> +            }
> +
> +            // check user is allowed to read from (local) source datastore/namespace
> +            let source_privs = user_info.lookup_privs(auth_id, &job.acl_path());
> +            if source_privs & PRIV_DATASTORE_AUDIT == 0 {
> +                return false;
> +            }
> +            source_privs & PRIV_DATASTORE_READ != 0

wouldn't PRIV_DATASTORE_BACKUP be enough here? the user doesn't need to
be able to read the whole datastore, just their own backups? of course,
READ implies BACKUP, so you could check for either here..

> +        }
>      }
> -    true
>  }


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel