[pbs-devel] [PATCH proxmox-backup 1/3] user.cfg/user info: add test constructors

[pbs-devel] [PATCH proxmox-backup 1/3] user.cfg/user info: add test constructors

[Fabian Grünbichler]

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/config/cached_user_info.rs | 8 ++++++++
 src/config/user.rs             | 8 ++++++++
 2 files changed, 16 insertions(+)

diff --git a/src/config/cached_user_info.rs b/src/config/cached_user_info.rs
index f56c07a8..518cf050 100644
--- a/src/config/cached_user_info.rs
+++ b/src/config/cached_user_info.rs
@@ -57,6 +57,14 @@ impl CachedUserInfo {
         Ok(config)
     }
 
+    #[cfg(test)]
+    pub(crate) fn test_new(user_cfg: SectionConfigData, acl_tree: AclTree) -> Self {
+        Self {
+            user_cfg: Arc::new(user_cfg),
+            acl_tree: Arc::new(acl_tree),
+        }
+    }
+
     /// Test if a authentication id is enabled and not expired
     pub fn is_active_auth_id(&self, auth_id: &Authid) -> bool {
         let userid = auth_id.user();
diff --git a/src/config/user.rs b/src/config/user.rs
index 5966c96d..3254183b 100644
--- a/src/config/user.rs
+++ b/src/config/user.rs
@@ -241,6 +241,14 @@ pub fn save_config(config: &SectionConfigData) -> Result<(), Error> {
     Ok(())
 }
 
+#[cfg(test)]
+pub(crate) fn test_cfg_from_str(raw: &str) -> Result<(SectionConfigData, [u8;32]), Error> {
+    let cfg = init();
+    let parsed = cfg.parse("test_user_cfg", raw)?;
+
+    Ok((parsed, [0;32]))
+}
+
 // shell completion helper
 pub fn complete_userid(_arg: &str, _param: &HashMap<String, String>) -> Vec<String> {
     match config() {
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 2/3] sync: add access check tests

[Fabian Grünbichler]

should cover all the current scenarios. remote server-side checks can't
be meaningfully unit-tested, but they are simple enough so should
hopefully never break.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/api2/config/sync.rs | 114 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 114 insertions(+)

diff --git a/src/api2/config/sync.rs b/src/api2/config/sync.rs
index 0d67b033..f9810369 100644
--- a/src/api2/config/sync.rs
+++ b/src/api2/config/sync.rs
@@ -32,6 +32,7 @@ pub fn check_sync_job_read_access(
     let remote_privs = user_info.lookup_privs(&auth_id, &["remote", &job.remote]);
     remote_privs & PRIV_REMOTE_AUDIT != 0
 }
+
 // user can run the corresponding pull job
 pub fn check_sync_job_modify_access(
     user_info: &CachedUserInfo,
@@ -410,3 +411,116 @@ pub const ROUTER: Router = Router::new()
     .get(&API_METHOD_LIST_SYNC_JOBS)
     .post(&API_METHOD_CREATE_SYNC_JOB)
     .match_all("id", &ITEM_ROUTER);
+
+
+#[test]
+fn sync_job_access_test() -> Result<(), Error> {
+    let (user_cfg, _) = crate::config::user::test_cfg_from_str(r###"
+user: noperm@pbs
+
+user: read@pbs
+
+user: write@pbs
+
+"###).expect("test user.cfg is not parsable");
+    let acl_tree = crate::config::acl::AclTree::from_raw(r###"
+acl:1:/datastore/localstore1:read@pbs,write@pbs:DatastoreAudit
+acl:1:/datastore/localstore1:write@pbs:DatastoreBackup
+acl:1:/datastore/localstore2:write@pbs:DatastorePowerUser
+acl:1:/datastore/localstore3:write@pbs:DatastoreAdmin
+acl:1:/remote/remote1:read@pbs,write@pbs:RemoteAudit
+acl:1:/remote/remote1/remotestore1:write@pbs:RemoteSyncOperator
+"###).expect("test acl.cfg is not parsable");
+
+    let user_info = CachedUserInfo::test_new(user_cfg, acl_tree);
+
+    let root_auth_id = Authid::root_auth_id();
+
+    let no_perm_auth_id: Authid = "noperm@pbs".parse()?;
+    let read_auth_id: Authid = "read@pbs".parse()?;
+    let write_auth_id: Authid = "write@pbs".parse()?;
+
+    let mut job = SyncJobConfig {
+        id: "regular".to_string(),
+        remote: "remote0".to_string(),
+        remote_store: "remotestore1".to_string(),
+        store: "localstore0".to_string(),
+        owner: Some(write_auth_id.clone()),
+        comment: None,
+        remove_vanished: None,
+        schedule: None,
+    };
+
+    // should work without ACLs
+    assert_eq!(check_sync_job_read_access(&user_info, &root_auth_id, &job), true);
+    assert_eq!(check_sync_job_modify_access(&user_info, &root_auth_id, &job), true);
+
+    // user without permissions must fail
+    assert_eq!(check_sync_job_read_access(&user_info, &no_perm_auth_id, &job), false);
+    assert_eq!(check_sync_job_modify_access(&user_info, &no_perm_auth_id, &job), false);
+
+    // reading without proper read permissions on either remote or local must fail
+    assert_eq!(check_sync_job_read_access(&user_info, &read_auth_id, &job), false);
+
+    // reading without proper read permissions on local end must fail
+    job.remote = "remote1".to_string();
+    assert_eq!(check_sync_job_read_access(&user_info, &read_auth_id, &job), false);
+
+    // reading without proper read permissions on remote end must fail
+    job.remote = "remote0".to_string();
+    job.store = "localstore1".to_string();
+    assert_eq!(check_sync_job_read_access(&user_info, &read_auth_id, &job), false);
+
+    // writing without proper write permissions on either end must fail
+    job.store = "localstore0".to_string();
+    assert_eq!(check_sync_job_modify_access(&user_info, &write_auth_id, &job), false);
+
+    // writing without proper write permissions on local end must fail
+    job.remote = "remote1".to_string();
+
+    // writing without proper write permissions on remote end must fail
+    job.remote = "remote0".to_string();
+    job.store = "localstore1".to_string();
+    assert_eq!(check_sync_job_modify_access(&user_info, &write_auth_id, &job), false);
+
+    // reset remote to one where users have access
+    job.remote = "remote1".to_string();
+
+    // user with read permission can only read, but not modify/run
+    assert_eq!(check_sync_job_read_access(&user_info, &read_auth_id, &job), true);
+    job.owner = Some(read_auth_id.clone());
+    assert_eq!(check_sync_job_modify_access(&user_info, &read_auth_id, &job), false);
+    job.owner = None;
+    assert_eq!(check_sync_job_modify_access(&user_info, &read_auth_id, &job), false);
+    job.owner = Some(write_auth_id.clone());
+    assert_eq!(check_sync_job_modify_access(&user_info, &read_auth_id, &job), false);
+
+    // user with simple write permission can modify/run
+    assert_eq!(check_sync_job_read_access(&user_info, &write_auth_id, &job), true);
+    assert_eq!(check_sync_job_modify_access(&user_info, &write_auth_id, &job), true);
+
+    // but can't modify/run with deletion
+    job.remove_vanished = Some(true);
+    assert_eq!(check_sync_job_modify_access(&user_info, &write_auth_id, &job), false);
+
+    // unless they have Datastore.Prune as well
+    job.store = "localstore2".to_string();
+    assert_eq!(check_sync_job_modify_access(&user_info, &write_auth_id, &job), true);
+
+    // changing owner is not possible
+    job.owner = Some(read_auth_id.clone());
+    assert_eq!(check_sync_job_modify_access(&user_info, &write_auth_id, &job), false);
+
+    // also not to the default 'backup@pam'
+    job.owner = None;
+    assert_eq!(check_sync_job_modify_access(&user_info, &write_auth_id, &job), false);
+
+    // unless they have Datastore.Modify as well
+    job.store = "localstore3".to_string();
+    job.owner = Some(read_auth_id.clone());
+    assert_eq!(check_sync_job_modify_access(&user_info, &write_auth_id, &job), true);
+    job.owner = None;
+    assert_eq!(check_sync_job_modify_access(&user_info, &write_auth_id, &job), true);
+
+    Ok(())
+}
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 3/3] docs: extend managing remotes

[Fabian Grünbichler]

with information about required privileges and limitations

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 docs/managing-remotes.rst | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/docs/managing-remotes.rst b/docs/managing-remotes.rst
index e8495db1..382ca84d 100644
--- a/docs/managing-remotes.rst
+++ b/docs/managing-remotes.rst
@@ -79,4 +79,17 @@ either start it manually on the GUI or provide it with a schedule (see
   └────────────┴───────┴────────┴──────────────┴───────────┴─────────┘
   # proxmox-backup-manager sync-job remove pbs2-local
 
+For setting up sync jobs, the configuring user needs the following permissions:
 
+#. ``Remote.Read`` on the ``/remote/{remote}/{remote-store}`` path
+#. at least ``Datastore.Backup`` on the local target datastore (``/datastore/{store}``)
+
+If the ``remove-vanished`` option is set, ``Datastore.Prune`` is required on
+the local datastore as well. If the ``owner`` option is not set (defaulting to
+``backup@pam``) or set to something other than the configuring user,
+``Datastore.Modify`` is required as well.
+
+.. note:: A sync job can only sync backup groups that the configured remote's
+  user/API token can read. If a remote is configured with a user/API token that
+  only has ``Datastore.Backup`` privileges, only the limited set of accessible
+  snapshots owned by that user/API token can be synced.
-- 
2.20.1

[pbs-devel] applied-series: [PATCH proxmox-backup 1/3] user.cfg/user info: add test constructors

[Thomas Lamprecht]

On 02.11.20 11:48, Fabian Grünbichler wrote:
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
>  src/config/cached_user_info.rs | 8 ++++++++
>  src/config/user.rs             | 8 ++++++++
>  2 files changed, 16 insertions(+)
> 
>

applied series, thanks!