From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id B441E1FF166 for ; Fri, 25 Oct 2024 12:17:26 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 34A6E1D93B; Fri, 25 Oct 2024 12:17:27 +0200 (CEST) Date: Fri, 25 Oct 2024 12:17:20 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox Backup Server development discussion References: <20241018084242.144010-1-c.ebner@proxmox.com> <20241018084242.144010-18-c.ebner@proxmox.com> In-Reply-To: <20241018084242.144010-18-c.ebner@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid) Message-Id: <1729851395.wkxyk3ock5.astroid@yuna.none> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.049 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH v5 proxmox-backup 17/31] api: config: extend modify access check by sync direction X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Backup Server development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" 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 > --- > 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