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 3BF581FF15E for ; Fri, 18 Oct 2024 10:43:23 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D6FE21E742; Fri, 18 Oct 2024 10:43:57 +0200 (CEST) From: Christian Ebner To: pbs-devel@lists.proxmox.com Date: Fri, 18 Oct 2024 10:42:28 +0200 Message-Id: <20241018084242.144010-18-c.ebner@proxmox.com> X-Mailer: git-send-email 2.39.5 In-Reply-To: <20241018084242.144010-1-c.ebner@proxmox.com> References: <20241018084242.144010-1-c.ebner@proxmox.com> MIME-Version: 1.0 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.029 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: [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" 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 + } } - true } #[api( @@ -166,10 +208,11 @@ pub fn create_sync_job( ) -> Result<(), Error> { let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?; let user_info = CachedUserInfo::new()?; + let sync_direction = sync_direction.unwrap_or_default(); let _lock = sync::lock_config()?; - if !check_sync_job_modify_access(&user_info, &auth_id, &config) { + if !check_sync_job_modify_access(&user_info, &auth_id, &config, sync_direction) { bail!("permission check failed"); } @@ -192,7 +235,6 @@ pub fn create_sync_job( param_bail!("id", "job '{}' already exists.", config.id); } - let sync_direction = sync_direction.unwrap_or_default(); section_config.set_data(&config.id, sync_direction.as_config_type_str(), &config)?; sync::save_config(§ion_config)?; @@ -455,7 +497,7 @@ pub fn update_sync_job( } } - if !check_sync_job_modify_access(&user_info, &auth_id, &data) { + if !check_sync_job_modify_access(&user_info, &auth_id, &data, sync_direction) { bail!("permission check failed"); } @@ -514,7 +556,7 @@ pub fn delete_sync_job( let sync_direction = sync_direction.unwrap_or_default(); match config.lookup(sync_direction.as_config_type_str(), &id) { Ok(job) => { - if !check_sync_job_modify_access(&user_info, &auth_id, &job) { + if !check_sync_job_modify_access(&user_info, &auth_id, &job, sync_direction) { bail!("permission check failed"); } config.sections.remove(&id); @@ -598,7 +640,12 @@ acl:1:/remote/remote1/remotestore1:write@pbs:RemoteSyncOperator &job, SyncDirection::Pull, )); - assert!(check_sync_job_modify_access(&user_info, root_auth_id, &job)); + assert!(check_sync_job_modify_access( + &user_info, + root_auth_id, + &job, + SyncDirection::Pull, + )); // user without permissions must fail assert!(!check_sync_job_read_access( @@ -610,7 +657,8 @@ acl:1:/remote/remote1/remotestore1:write@pbs:RemoteSyncOperator assert!(!check_sync_job_modify_access( &user_info, &no_perm_auth_id, - &job + &job, + SyncDirection::Pull, )); // reading without proper read permissions on either remote or local must fail @@ -645,7 +693,8 @@ acl:1:/remote/remote1/remotestore1:write@pbs:RemoteSyncOperator assert!(!check_sync_job_modify_access( &user_info, &write_auth_id, - &job + &job, + SyncDirection::Pull, )); // writing without proper write permissions on local end must fail @@ -657,7 +706,8 @@ acl:1:/remote/remote1/remotestore1:write@pbs:RemoteSyncOperator assert!(!check_sync_job_modify_access( &user_info, &write_auth_id, - &job + &job, + SyncDirection::Pull, )); // reset remote to one where users have access @@ -674,19 +724,22 @@ acl:1:/remote/remote1/remotestore1:write@pbs:RemoteSyncOperator assert!(!check_sync_job_modify_access( &user_info, &read_auth_id, - &job + &job, + SyncDirection::Pull, )); job.owner = None; assert!(!check_sync_job_modify_access( &user_info, &read_auth_id, - &job + &job, + SyncDirection::Pull, )); job.owner = Some(write_auth_id.clone()); assert!(!check_sync_job_modify_access( &user_info, &read_auth_id, - &job + &job, + SyncDirection::Pull, )); // user with simple write permission can modify/run @@ -699,7 +752,8 @@ acl:1:/remote/remote1/remotestore1:write@pbs:RemoteSyncOperator assert!(check_sync_job_modify_access( &user_info, &write_auth_id, - &job + &job, + SyncDirection::Pull, )); // but can't modify/run with deletion @@ -707,7 +761,8 @@ acl:1:/remote/remote1/remotestore1:write@pbs:RemoteSyncOperator assert!(!check_sync_job_modify_access( &user_info, &write_auth_id, - &job + &job, + SyncDirection::Pull, )); // unless they have Datastore.Prune as well @@ -715,7 +770,8 @@ acl:1:/remote/remote1/remotestore1:write@pbs:RemoteSyncOperator assert!(check_sync_job_modify_access( &user_info, &write_auth_id, - &job + &job, + SyncDirection::Pull, )); // changing owner is not possible @@ -723,7 +779,8 @@ acl:1:/remote/remote1/remotestore1:write@pbs:RemoteSyncOperator assert!(!check_sync_job_modify_access( &user_info, &write_auth_id, - &job + &job, + SyncDirection::Pull, )); // also not to the default 'root@pam' @@ -731,7 +788,8 @@ acl:1:/remote/remote1/remotestore1:write@pbs:RemoteSyncOperator assert!(!check_sync_job_modify_access( &user_info, &write_auth_id, - &job + &job, + SyncDirection::Pull, )); // unless they have Datastore.Modify as well @@ -740,13 +798,15 @@ acl:1:/remote/remote1/remotestore1:write@pbs:RemoteSyncOperator assert!(check_sync_job_modify_access( &user_info, &write_auth_id, - &job + &job, + SyncDirection::Pull, )); job.owner = None; assert!(check_sync_job_modify_access( &user_info, &write_auth_id, - &job + &job, + SyncDirection::Pull, )); Ok(()) -- 2.39.5 _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel