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

[Christian Ebner <c.ebner@proxmox.com>]

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 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(&section_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