[pbs-devel] [PATCH v5 proxmox-backup 17/31] api: config: extend modify access check by sync direction
Christian Ebner
c.ebner at proxmox.com
Fri Oct 18 10:42:28 CEST 2024
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 at 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
+ }
}
- 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at pbs:RemoteSyncOperator
assert!(!check_sync_job_modify_access(
&user_info,
&write_auth_id,
- &job
+ &job,
+ SyncDirection::Pull,
));
// also not to the default 'root at pam'
@@ -731,7 +788,8 @@ acl:1:/remote/remote1/remotestore1:write at 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 at 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
More information about the pbs-devel
mailing list