[pbs-devel] [PATCH proxmox-backup 0/8] permission improvements

[pbs-devel] [PATCH proxmox-backup 0/8] permission improvements

[Fabian Grünbichler]

this series cleans up
- get/set_notes permissions
- unused PRIV_REMOTE_PRUNE

reworks verification permissions:
- add a new PRIV_DATASTORE_VERIFY that allows verifying whole datastores
- allows unprivileged users to verify their part of a datastore in bulk
- allows non-superusers to setup and view verification jobs (if they are
  privileged enough)

reworks pulls/syncs:
- allow setting an owner
- allow non-superusers to setup and view sync jobs (if they are
  privileged enough)

Fabian Grünbichler (8):
  privs: allow reading notes with Datastore.Audit
  privs: use Datastore.Modify|Backup to set backup notes
  verify: introduce & use new Datastore.Verify privilege
  verify jobs: add permissions
  sync: add owner
  sync: allow sync for non-superusers
  privs: remove PRIV_REMOVE_PRUNE
  privs: add some more comments explaining privileges

 src/api2/admin/datastore.rs |  34 ++++++--
 src/api2/admin/sync.rs      |  30 ++++++-
 src/api2/config/remote.rs   |  15 +++-
 src/api2/config/sync.rs     | 152 +++++++++++++++++++++++++++++++++---
 src/api2/config/verify.rs   |  41 +++++++++-
 src/api2/pull.rs            |   5 +-
 src/backup/verify.rs        |  29 ++++++-
 src/config/acl.rs           |  24 ++++--
 src/config/sync.rs          |  28 ++++++-
 src/server/verify_job.rs    |   2 +-
 www/config/SyncView.js      |  14 +++-
 www/window/SyncJobEdit.js   |  20 +++++
 12 files changed, 352 insertions(+), 42 deletions(-)

-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 1/8] privs: allow reading notes with Datastore.Audit

[Fabian Grünbichler]

they are returned when reading the manifest, which just requires
Datastore.Audit as well. Datastore.Read is for reading backup contents,
not metadata.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/api2/admin/datastore.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 2e41c28a..41e270a1 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -1562,7 +1562,7 @@ fn get_rrd_stats(
         },
     },
     access: {
-        permission: &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_READ | PRIV_DATASTORE_BACKUP, true),
+        permission: &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_AUDIT | PRIV_DATASTORE_BACKUP, true),
     },
 )]
 /// Get "notes" for a specific backup
@@ -1578,7 +1578,7 @@ fn get_notes(
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
     let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?;
 
-    check_priv_or_backup_owner(&datastore, backup_dir.group(), &auth_id, PRIV_DATASTORE_READ)?;
+    check_priv_or_backup_owner(&datastore, backup_dir.group(), &auth_id, PRIV_DATASTORE_AUDIT)?;
 
     let (manifest, _) = datastore.load_manifest(&backup_dir)?;
 
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 2/8] privs: use Datastore.Modify|Backup to set backup notes

[Fabian Grünbichler]

Datastore.Backup is limited to owned groups, as usual.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/api2/admin/datastore.rs | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 41e270a1..b9412ba6 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -1610,7 +1610,9 @@ fn get_notes(
         },
     },
     access: {
-        permission: &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_MODIFY, true),
+        permission: &Permission::Privilege(&["datastore", "{store}"],
+                                           PRIV_DATASTORE_MODIFY | PRIV_DATASTORE_BACKUP,
+                                           true),
     },
 )]
 /// Set "notes" for a specific backup
@@ -1627,7 +1629,7 @@ fn set_notes(
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
     let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?;
 
-    check_priv_or_backup_owner(&datastore, backup_dir.group(), &auth_id, PRIV_DATASTORE_READ)?;
+    check_priv_or_backup_owner(&datastore, backup_dir.group(), &auth_id, PRIV_DATASTORE_MODIFY)?;
 
     datastore.update_manifest(&backup_dir,|manifest| {
         manifest.unprotected["notes"] = notes.into();
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 3/8] verify: introduce & use new Datastore.Verify privilege

[Fabian Grünbichler]

for verifying a whole datastore. Datastore.Backup now allows verifying
only backups owned by the triggering user.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    this will need a rebase post-token, sending anyway for review of the idea itself.

 src/api2/admin/datastore.rs | 24 ++++++++++++++++++++----
 src/backup/verify.rs        | 29 ++++++++++++++++++++++++++++-
 src/config/acl.rs           |  5 ++++-
 src/server/verify_job.rs    |  2 +-
 4 files changed, 53 insertions(+), 7 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index b9412ba6..220f06ae 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -42,6 +42,7 @@ use crate::config::acl::{
     PRIV_DATASTORE_READ,
     PRIV_DATASTORE_PRUNE,
     PRIV_DATASTORE_BACKUP,
+    PRIV_DATASTORE_VERIFY,
 };
 
 fn check_priv_or_backup_owner(
@@ -537,7 +538,7 @@ pub fn status(
         schema: UPID_SCHEMA,
     },
     access: {
-        permission: &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_READ | PRIV_DATASTORE_BACKUP, true), // fixme
+        permission: &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_VERIFY | PRIV_DATASTORE_BACKUP, true),
     },
 )]
 /// Verify backups.
@@ -553,6 +554,7 @@ pub fn verify(
 ) -> Result<Value, Error> {
     let datastore = DataStore::lookup_datastore(&store)?;
 
+    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
     let worker_id;
 
     let mut backup_dir = None;
@@ -563,12 +565,18 @@ pub fn verify(
         (Some(backup_type), Some(backup_id), Some(backup_time)) => {
             worker_id = format!("{}:{}/{}/{:08X}", store, backup_type, backup_id, backup_time);
             let dir = BackupDir::new(backup_type, backup_id, backup_time)?;
+
+            check_priv_or_backup_owner(&datastore, dir.group(), &auth_id, PRIV_DATASTORE_VERIFY)?;
+
             backup_dir = Some(dir);
             worker_type = "verify_snapshot";
         }
         (Some(backup_type), Some(backup_id), None) => {
             worker_id = format!("{}:{}/{}", store, backup_type, backup_id);
             let group = BackupGroup::new(backup_type, backup_id);
+
+            check_priv_or_backup_owner(&datastore, &group, &auth_id, PRIV_DATASTORE_VERIFY)?;
+
             backup_group = Some(group);
             worker_type = "verify_group";
         }
@@ -578,13 +586,12 @@ pub fn verify(
         _ => bail!("parameters do not specify a backup group or snapshot"),
     }
 
-    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
     let to_stdout = if rpcenv.env_type() == RpcEnvironmentType::CLI { true } else { false };
 
     let upid_str = WorkerTask::new_thread(
         worker_type,
         Some(worker_id.clone()),
-        auth_id,
+        auth_id.clone(),
         to_stdout,
         move |worker| {
             let verified_chunks = Arc::new(Mutex::new(HashSet::with_capacity(1024*16)));
@@ -617,7 +624,16 @@ pub fn verify(
                 )?;
                 failed_dirs
             } else {
-                verify_all_backups(datastore, worker.clone(), worker.upid(), None)?
+                let privs = CachedUserInfo::new()?
+                    .lookup_privs(&auth_id, &["datastore", &store]);
+
+                let owner = if privs & PRIV_DATASTORE_VERIFY == 0 {
+                    Some(auth_id)
+                } else {
+                    None
+                };
+
+                verify_all_backups(datastore, worker.clone(), worker.upid(), owner, None)?
             };
             if failed_dirs.len() > 0 {
                 worker.log("Failed to verify following snapshots:");
diff --git a/src/backup/verify.rs b/src/backup/verify.rs
index bb39f7d8..e0e28ee9 100644
--- a/src/backup/verify.rs
+++ b/src/backup/verify.rs
@@ -482,7 +482,7 @@ pub fn verify_backup_group(
     Ok((count, errors))
 }
 
-/// Verify all backups inside a datastore
+/// Verify all (owned) backups inside a datastore
 ///
 /// Errors are logged to the worker log.
 ///
@@ -493,14 +493,41 @@ pub fn verify_all_backups(
     datastore: Arc<DataStore>,
     worker: Arc<dyn TaskState + Send + Sync>,
     upid: &UPID,
+    owner: Option<Authid>,
     filter: Option<&dyn Fn(&BackupManifest) -> bool>,
 ) -> Result<Vec<String>, Error> {
     let mut errors = Vec::new();
 
+    if let Some(owner) = &owner {
+        task_log!(
+            worker,
+            "verify datastore {} - limiting to backups owned by {}",
+            datastore.name(),
+            owner
+        );
+    }
+
+    let filter_by_owner = |group: &BackupGroup| {
+        if let Some(owner) = &owner {
+            match datastore.get_owner(group) {
+                Ok(ref group_owner) => {
+                    group_owner == owner
+                        || (group_owner.is_token()
+                            && !owner.is_token()
+                            && group_owner.user() == owner.user())
+                },
+                Err(_) => false,
+            }
+        } else {
+            true
+        }
+    };
+
     let mut list = match BackupGroup::list_groups(&datastore.base_path()) {
         Ok(list) => list
             .into_iter()
             .filter(|group| !(group.backup_type() == "host" && group.backup_id() == "benchmark"))
+            .filter(filter_by_owner)
             .collect::<Vec<BackupGroup>>(),
         Err(err) => {
             task_log!(
diff --git a/src/config/acl.rs b/src/config/acl.rs
index f82d5903..7345adea 100644
--- a/src/config/acl.rs
+++ b/src/config/acl.rs
@@ -30,6 +30,7 @@ constnamedbitmap! {
         PRIV_DATASTORE_ALLOCATE("Datastore.Allocate");
         PRIV_DATASTORE_MODIFY("Datastore.Modify");
         PRIV_DATASTORE_READ("Datastore.Read");
+        PRIV_DATASTORE_VERIFY("Datastore.Verify");
 
         /// Datastore.Backup also requires backup ownership
         PRIV_DATASTORE_BACKUP("Datastore.Backup");
@@ -64,12 +65,14 @@ pub const ROLE_DATASTORE_ADMIN: u64 =
 PRIV_DATASTORE_AUDIT |
 PRIV_DATASTORE_MODIFY |
 PRIV_DATASTORE_READ |
+PRIV_DATASTORE_VERIFY |
 PRIV_DATASTORE_BACKUP |
 PRIV_DATASTORE_PRUNE;
 
-/// Datastore.Reader can read datastore content an do restore
+/// Datastore.Reader can read/verify datastore content and do restore
 pub const ROLE_DATASTORE_READER: u64 =
 PRIV_DATASTORE_AUDIT |
+PRIV_DATASTORE_VERIFY |
 PRIV_DATASTORE_READ;
 
 /// Datastore.Backup can do backup and restore, but no prune.
diff --git a/src/server/verify_job.rs b/src/server/verify_job.rs
index a8f532ac..c98cd5b2 100644
--- a/src/server/verify_job.rs
+++ b/src/server/verify_job.rs
@@ -65,7 +65,7 @@ pub fn do_verification_job(
                 task_log!(worker,"task triggered by schedule '{}'", event_str);
             }
 
-            let result = verify_all_backups(datastore, worker.clone(), worker.upid(), Some(&filter));
+            let result = verify_all_backups(datastore, worker.clone(), worker.upid(), None, Some(&filter));
             let job_result = match result {
                 Ok(ref errors) if errors.is_empty() => Ok(()),
                 Ok(_) => Err(format_err!("verification failed - please check the log for details")),
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 4/8] verify jobs: add permissions

[Fabian Grünbichler]

equivalent to verifying a whole datastore, except for reading job
(entries), which is accessible to regular Datastore.Audit/Backup users
as well.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    modifying the config could also be guarded by Datastore.Modify, but I like the
    idea of scheduling an action and doing it now requiring the same permissions..

 src/api2/config/verify.rs | 41 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/src/api2/config/verify.rs b/src/api2/config/verify.rs
index 32d83d17..0c5a6460 100644
--- a/src/api2/config/verify.rs
+++ b/src/api2/config/verify.rs
@@ -2,10 +2,17 @@ use anyhow::{bail, Error};
 use serde_json::Value;
 use ::serde::{Deserialize, Serialize};
 
-use proxmox::api::{api, Router, RpcEnvironment};
+use proxmox::api::{api, Permission, Router, RpcEnvironment};
 use proxmox::tools::fs::open_file_locked;
 
 use crate::api2::types::*;
+
+use crate::config::acl::{
+    PRIV_DATASTORE_AUDIT,
+    PRIV_DATASTORE_BACKUP,
+    PRIV_DATASTORE_VERIFY,
+};
+
 use crate::config::verify::{self, VerificationJobConfig};
 
 #[api(
@@ -17,6 +24,12 @@ use crate::config::verify::{self, VerificationJobConfig};
         type: Array,
         items: { type: verify::VerificationJobConfig },
     },
+    access: {
+        permission: &Permission::Privilege(
+            &["datastore", "{store}"],
+            PRIV_DATASTORE_AUDIT | PRIV_DATASTORE_BACKUP | PRIV_DATASTORE_VERIFY,
+            true),
+    },
 )]
 /// List all verification jobs
 pub fn list_verification_jobs(
@@ -61,7 +74,13 @@ pub fn list_verification_jobs(
                 schema: VERIFICATION_SCHEDULE_SCHEMA,
             },
         }
-    }
+    },
+    access: {
+        permission: &Permission::Privilege(
+            &["datastore", "{store}"],
+            PRIV_DATASTORE_VERIFY,
+            true),
+    },
 )]
 /// Create a new verification job.
 pub fn create_verification_job(param: Value) -> Result<(), Error> {
@@ -97,6 +116,12 @@ pub fn create_verification_job(param: Value) -> Result<(), Error> {
         description: "The verification job configuration.",
         type: verify::VerificationJobConfig,
     },
+    access: {
+        permission: &Permission::Privilege(
+            &["datastore", "{store}"],
+            PRIV_DATASTORE_AUDIT | PRIV_DATASTORE_BACKUP | PRIV_DATASTORE_VERIFY,
+            true),
+    },
 )]
 /// Read a verification job configuration.
 pub fn read_verification_job(
@@ -167,6 +192,12 @@ pub enum DeletableProperty {
             },
         },
     },
+    access: {
+        permission: &Permission::Privilege(
+            &["datastore", "{store}"],
+            PRIV_DATASTORE_VERIFY,
+            true),
+    },
 )]
 /// Update verification job config.
 pub fn update_verification_job(
@@ -238,6 +269,12 @@ pub fn update_verification_job(
             },
         },
     },
+    access: {
+        permission: &Permission::Privilege(
+            &["datastore", "{store}"],
+            PRIV_DATASTORE_VERIFY,
+            true),
+    },
 )]
 /// Remove a verification job configuration
 pub fn delete_verification_job(id: String, digest: Option<String>) -> Result<(), Error> {
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 5/8] fix #2864: add owner option to sync

[Fabian Grünbichler]

instead of hard-coding 'backup@pam'. this allows a bit more flexibility
(e.g., syncing to a datastore that can directly be used as restore
source) without overly complicating things.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/api2/config/sync.rs   | 16 ++++++++++++++--
 src/api2/pull.rs          |  5 ++---
 src/config/sync.rs        | 12 ++++++++++++
 www/config/SyncView.js    | 14 +++++++++++++-
 www/window/SyncJobEdit.js | 20 ++++++++++++++++++++
 5 files changed, 61 insertions(+), 6 deletions(-)

diff --git a/src/api2/config/sync.rs b/src/api2/config/sync.rs
index e5baa7da..758b4a02 100644
--- a/src/api2/config/sync.rs
+++ b/src/api2/config/sync.rs
@@ -45,6 +45,10 @@ pub fn list_sync_jobs(
             store: {
                 schema: DATASTORE_SCHEMA,
             },
+            owner: {
+                type: Authid,
+                optional: true,
+            },
             remote: {
                 schema: REMOTE_ID_SCHEMA,
             },
@@ -120,6 +124,8 @@ pub fn read_sync_job(
 #[allow(non_camel_case_types)]
 /// Deletable property name
 pub enum DeletableProperty {
+    /// Delete the owner property.
+    owner,
     /// Delete the comment property.
     comment,
     /// Delete the job schedule.
@@ -139,6 +145,10 @@ pub enum DeletableProperty {
                 schema: DATASTORE_SCHEMA,
                 optional: true,
             },
+            owner: {
+                type: Authid,
+                optional: true,
+            },
             remote: {
                 schema: REMOTE_ID_SCHEMA,
                 optional: true,
@@ -178,6 +188,7 @@ pub enum DeletableProperty {
 pub fn update_sync_job(
     id: String,
     store: Option<String>,
+    owner: Option<Authid>,
     remote: Option<String>,
     remote_store: Option<String>,
     remove_vanished: Option<bool>,
@@ -202,6 +213,7 @@ pub fn update_sync_job(
      if let Some(delete) = delete {
         for delete_prop in delete {
             match delete_prop {
+                DeletableProperty::owner => { data.owner = None; },
                 DeletableProperty::comment => { data.comment = None; },
                 DeletableProperty::schedule => { data.schedule = None; },
                 DeletableProperty::remove_vanished => { data.remove_vanished = None; },
@@ -221,8 +233,8 @@ pub fn update_sync_job(
     if let Some(store) = store { data.store = store; }
     if let Some(remote) = remote { data.remote = remote; }
     if let Some(remote_store) = remote_store { data.remote_store = remote_store; }
-        
-    
+    if let Some(owner) = owner { data.owner = owner; }
+
     if schedule.is_some() { data.schedule = schedule; }
     if remove_vanished.is_some() { data.remove_vanished = remove_vanished; }
 
diff --git a/src/api2/pull.rs b/src/api2/pull.rs
index 45c0b1b1..8491ab00 100644
--- a/src/api2/pull.rs
+++ b/src/api2/pull.rs
@@ -92,6 +92,7 @@ pub fn do_sync_job(
             let worker_future = async move {
 
                 let delete = sync_job.remove_vanished.unwrap_or(true);
+                let sync_owner = sync_job.owner.unwrap_or(Authid::backup_auth_id().clone());
                 let (client, src_repo, tgt_store) = get_pull_parameters(&sync_job.store, &sync_job.remote, &sync_job.remote_store).await?;
 
                 worker.log(format!("Starting datastore sync job '{}'", job_id));
@@ -101,9 +102,7 @@ pub fn do_sync_job(
                 worker.log(format!("Sync datastore '{}' from '{}/{}'",
                         sync_job.store, sync_job.remote, sync_job.remote_store));
 
-                let backup_auth_id = Authid::backup_auth_id();
-
-                crate::client::pull::pull_store(&worker, &client, &src_repo, tgt_store.clone(), delete, backup_auth_id.clone()).await?;
+                crate::client::pull::pull_store(&worker, &client, &src_repo, tgt_store.clone(), delete, sync_owner).await?;
 
                 worker.log(format!("sync job '{}' end", &job_id));
 
diff --git a/src/config/sync.rs b/src/config/sync.rs
index 92ccb6fe..d007570c 100644
--- a/src/config/sync.rs
+++ b/src/config/sync.rs
@@ -30,6 +30,10 @@ lazy_static! {
         store: {
            schema: DATASTORE_SCHEMA,
         },
+        "owner": {
+            type: Authid,
+            optional: true,
+        },
         remote: {
             schema: REMOTE_ID_SCHEMA,
         },
@@ -56,6 +60,8 @@ lazy_static! {
 pub struct SyncJobConfig {
     pub id: String,
     pub store: String,
+    #[serde(skip_serializing_if="Option::is_none")]
+    pub owner: Option<Authid>,
     pub remote: String,
     pub remote_store: String,
     #[serde(skip_serializing_if="Option::is_none")]
@@ -75,6 +81,10 @@ pub struct SyncJobConfig {
         store: {
            schema: DATASTORE_SCHEMA,
         },
+        owner: {
+            type: Authid,
+            optional: true,
+        },
         remote: {
             schema: REMOTE_ID_SCHEMA,
         },
@@ -121,6 +131,8 @@ pub struct SyncJobConfig {
 pub struct SyncJobStatus {
     pub id: String,
     pub store: String,
+    #[serde(skip_serializing_if="Option::is_none")]
+    pub owner: Option<Authid>,
     pub remote: String,
     pub remote_store: String,
     #[serde(skip_serializing_if="Option::is_none")]
diff --git a/www/config/SyncView.js b/www/config/SyncView.js
index 0f68555b..88cb0045 100644
--- a/www/config/SyncView.js
+++ b/www/config/SyncView.js
@@ -1,7 +1,7 @@
 Ext.define('pbs-sync-jobs-status', {
     extend: 'Ext.data.Model',
     fields: [
-	'id', 'remote', 'remote-store', 'store', 'schedule',
+	'id', 'owner', 'remote', 'remote-store', 'store', 'schedule',
 	'next-run', 'last-run-upid', 'last-run-state', 'last-run-endtime',
 	{
 	    name: 'duration',
@@ -151,6 +151,11 @@ Ext.define('PBS.config.SyncJobView', {
 	    return Proxmox.Utils.render_timestamp(value);
 	},
 
+	render_optional_owner: function(value, metadata, record) {
+	    if (!value) return '-';
+	    return Ext.String.htmlEncode(value, metadata, record);
+	},
+
 	startStore: function() { this.getView().getStore().rstore.startUpdate(); },
 	stopStore: function() { this.getView().getStore().rstore.stopUpdate(); },
 
@@ -249,6 +254,13 @@ Ext.define('PBS.config.SyncJobView', {
 	    flex: 2,
 	    sortable: true,
 	},
+	{
+	    header: gettext('Owner'),
+	    dataIndex: 'owner',
+	    renderer: 'render_optional_owner',
+	    flex: 2,
+	    sortable: true,
+	},
 	{
 	    header: gettext('Schedule'),
 	    dataIndex: 'schedule',
diff --git a/www/window/SyncJobEdit.js b/www/window/SyncJobEdit.js
index 85dfb7d0..4edc8cc5 100644
--- a/www/window/SyncJobEdit.js
+++ b/www/window/SyncJobEdit.js
@@ -62,6 +62,26 @@ Ext.define('PBS.window.SyncJobEdit', {
 	],
 
 	column2: [
+	    {
+		fieldLabel: gettext('Owner'),
+		xtype: 'pbsUserSelector',
+		name: 'owner',
+		allowBlank: true,
+		emptyText: 'backup@pam',
+		getSubmitData: function() {
+		    let me = this;
+		    let name = me.getName();
+		    let val = me.getSubmitValue();
+
+		    let data = {};
+		    if (val === null || val === "") {
+			data.delete = name;
+		    } else {
+			data[name] = val;
+		    }
+		    return data;
+		},
+	    },
 	    {
 		fieldLabel: gettext('Remove vanished'),
 		xtype: 'proxmoxcheckbox',
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 6/8] sync: allow sync for non-superusers

[Fabian Grünbichler]

by requiring
- Datastore.Backup permission for target datastore
- Remote.Read permission for source remote/datastore
- Datastore.Prune if vanished snapshots should be removed
- Datastore.Modify if another user should own the freshly synced
snapshots

reading a sync job entry only requires knowing about both the source
remote and the target datastore.

note that this does not affect the Authid used to authenticate with the
remote, which of course also needs permissions to access the source
datastore.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/api2/admin/sync.rs    |  30 +++++++--
 src/api2/config/remote.rs |  15 ++++-
 src/api2/config/sync.rs   | 138 +++++++++++++++++++++++++++++++++++---
 src/config/sync.rs        |  16 ++++-
 4 files changed, 182 insertions(+), 17 deletions(-)

diff --git a/src/api2/admin/sync.rs b/src/api2/admin/sync.rs
index f7032a3a..c9b9e145 100644
--- a/src/api2/admin/sync.rs
+++ b/src/api2/admin/sync.rs
@@ -1,12 +1,15 @@
-use anyhow::{format_err, Error};
+use anyhow::{bail, format_err, Error};
 use serde_json::Value;
 
-use proxmox::api::{api, ApiMethod, Router, RpcEnvironment};
+use proxmox::api::{api, ApiMethod, Permission, Router, RpcEnvironment};
 use proxmox::api::router::SubdirMap;
 use proxmox::{list_subdirs_api_method, sortable};
 
 use crate::api2::types::*;
 use crate::api2::pull::do_sync_job;
+use crate::api2::config::sync::{check_sync_job_modify_access, check_sync_job_read_access};
+
+use crate::config::cached_user_info::CachedUserInfo;
 use crate::config::sync::{self, SyncJobStatus, SyncJobConfig};
 use crate::server::UPID;
 use crate::server::jobstate::{Job, JobState};
@@ -27,6 +30,10 @@ use crate::tools::systemd::time::{
         type: Array,
         items: { type: sync::SyncJobStatus },
     },
+    access: {
+        description: "Limited to sync jobs where user has Datastore.Audit on target datastore, and Remote.Audit on source remote.",
+        permission: &Permission::Anybody,
+    },
 )]
 /// List all sync jobs
 pub fn list_sync_jobs(
@@ -35,6 +42,9 @@ pub fn list_sync_jobs(
     mut rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<Vec<SyncJobStatus>, Error> {
 
+    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
+    let user_info = CachedUserInfo::new()?;
+
     let (config, digest) = sync::config()?;
 
     let mut list: Vec<SyncJobStatus> = config
@@ -46,6 +56,10 @@ pub fn list_sync_jobs(
             } else {
                 true
             }
+        })
+        .filter(|job: &SyncJobStatus| {
+            let as_config: SyncJobConfig = job.clone().into();
+            check_sync_job_read_access(&user_info, &auth_id, &as_config)
         }).collect();
 
     for job in &mut list {
@@ -89,7 +103,11 @@ pub fn list_sync_jobs(
                 schema: JOB_ID_SCHEMA,
             }
         }
-    }
+    },
+    access: {
+        description: "User needs Datastore.Backup on target datastore, and Remote.Read on source remote. Additionally, remove_vanished requires Datastore.Prune, and any owner other than the user themselves requires Datastore.Modify",
+        permission: &Permission::Anybody,
+    },
 )]
 /// Runs the sync jobs manually.
 fn run_sync_job(
@@ -97,11 +115,15 @@ fn run_sync_job(
     _info: &ApiMethod,
     rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<String, Error> {
+    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
+    let user_info = CachedUserInfo::new()?;
 
     let (config, _digest) = sync::config()?;
     let sync_job: SyncJobConfig = config.lookup("sync", &id)?;
 
-    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
+    if !check_sync_job_modify_access(&user_info, &auth_id, &sync_job) {
+        bail!("permission check failed");
+    }
 
     let job = Job::new("syncjob", &id)?;
 
diff --git a/src/api2/config/remote.rs b/src/api2/config/remote.rs
index 96869695..ffbba1d2 100644
--- a/src/api2/config/remote.rs
+++ b/src/api2/config/remote.rs
@@ -6,6 +6,7 @@ use proxmox::api::{api, ApiMethod, Router, RpcEnvironment, Permission};
 use proxmox::tools::fs::open_file_locked;
 
 use crate::api2::types::*;
+use crate::config::cached_user_info::CachedUserInfo;
 use crate::config::remote;
 use crate::config::acl::{PRIV_REMOTE_AUDIT, PRIV_REMOTE_MODIFY};
 
@@ -22,7 +23,8 @@ use crate::config::acl::{PRIV_REMOTE_AUDIT, PRIV_REMOTE_MODIFY};
         },
     },
     access: {
-        permission: &Permission::Privilege(&["remote"], PRIV_REMOTE_AUDIT, false),
+        description: "List configured remotes filtered by Remote.Audit privileges",
+        permission: &Permission::Anybody,
     },
 )]
 /// List all remotes
@@ -31,16 +33,25 @@ pub fn list_remotes(
     _info: &ApiMethod,
     mut rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<Vec<remote::Remote>, Error> {
+    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
+    let user_info = CachedUserInfo::new()?;
 
     let (config, digest) = remote::config()?;
 
     let mut list: Vec<remote::Remote> = config.convert_to_typed_array("remote")?;
-
     // don't return password in api
     for remote in &mut list {
         remote.password = "".to_string();
     }
 
+    let list = list
+        .into_iter()
+        .filter(|remote| {
+            let privs = user_info.lookup_privs(&auth_id, &["remote", &remote.name]);
+            privs & PRIV_REMOTE_AUDIT != 0
+        })
+        .collect();
+
     rpcenv["digest"] = proxmox::tools::digest_to_hex(&digest).into();
     Ok(list)
 }
diff --git a/src/api2/config/sync.rs b/src/api2/config/sync.rs
index 758b4a02..0d67b033 100644
--- a/src/api2/config/sync.rs
+++ b/src/api2/config/sync.rs
@@ -2,13 +2,72 @@ use anyhow::{bail, Error};
 use serde_json::Value;
 use ::serde::{Deserialize, Serialize};
 
-use proxmox::api::{api, Router, RpcEnvironment};
+use proxmox::api::{api, Permission, Router, RpcEnvironment};
 use proxmox::tools::fs::open_file_locked;
 
 use crate::api2::types::*;
+
+use crate::config::acl::{
+    PRIV_DATASTORE_AUDIT,
+    PRIV_DATASTORE_BACKUP,
+    PRIV_DATASTORE_MODIFY,
+    PRIV_DATASTORE_PRUNE,
+    PRIV_REMOTE_AUDIT,
+    PRIV_REMOTE_READ,
+};
+
+use crate::config::cached_user_info::CachedUserInfo;
 use crate::config::sync::{self, SyncJobConfig};
 
-// fixme: add access permissions
+pub fn check_sync_job_read_access(
+    user_info: &CachedUserInfo,
+    auth_id: &Authid,
+    job: &SyncJobConfig,
+) -> bool {
+    let datastore_privs = user_info.lookup_privs(&auth_id, &["datastore", &job.store]);
+    if datastore_privs & PRIV_DATASTORE_AUDIT == 0 {
+        return false;
+    }
+
+    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,
+    auth_id: &Authid,
+    job: &SyncJobConfig,
+) -> bool {
+    let datastore_privs = user_info.lookup_privs(&auth_id, &["datastore", &job.store]);
+    if datastore_privs & PRIV_DATASTORE_BACKUP == 0 {
+        return false;
+    }
+
+    if let Some(true) = job.remove_vanished {
+        if datastore_privs & PRIV_DATASTORE_PRUNE == 0 {
+            return false;
+        }
+    }
+
+    let correct_owner = match job.owner {
+        Some(ref owner) => {
+            owner == auth_id
+                || (owner.is_token()
+                    && !auth_id.is_token()
+                    && owner.user() == auth_id.user())
+        },
+        // default sync owner
+        None => auth_id == Authid::backup_auth_id(),
+    };
+
+    // same permission as changing ownership after syncing
+    if !correct_owner && datastore_privs & PRIV_DATASTORE_MODIFY == 0 {
+        return false;
+    }
+
+    let remote_privs = user_info.lookup_privs(&auth_id, &["remote", &job.remote, &job.remote_store]);
+    remote_privs & PRIV_REMOTE_READ != 0
+}
 
 #[api(
     input: {
@@ -19,12 +78,18 @@ use crate::config::sync::{self, SyncJobConfig};
         type: Array,
         items: { type: sync::SyncJobConfig },
     },
+    access: {
+        description: "Limited to sync job entries where user has Datastore.Audit on target datastore, and Remote.Audit on source remote.",
+        permission: &Permission::Anybody,
+    },
 )]
 /// List all sync jobs
 pub fn list_sync_jobs(
     _param: Value,
     mut rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<Vec<SyncJobConfig>, Error> {
+    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
+    let user_info = CachedUserInfo::new()?;
 
     let (config, digest) = sync::config()?;
 
@@ -32,7 +97,11 @@ pub fn list_sync_jobs(
 
     rpcenv["digest"] = proxmox::tools::digest_to_hex(&digest).into();
 
-    Ok(list)
+    let list = list
+        .into_iter()
+        .filter(|sync_job| check_sync_job_read_access(&user_info, &auth_id, &sync_job))
+        .collect();
+   Ok(list)
 }
 
 #[api(
@@ -69,13 +138,25 @@ pub fn list_sync_jobs(
             },
         },
     },
+    access: {
+        description: "User needs Datastore.Backup on target datastore, and Remote.Read on source remote. Additionally, remove_vanished requires Datastore.Prune, and any owner other than the user themselves requires Datastore.Modify",
+        permission: &Permission::Anybody,
+    },
 )]
 /// Create a new sync job.
-pub fn create_sync_job(param: Value) -> Result<(), Error> {
+pub fn create_sync_job(
+    param: Value,
+    rpcenv: &mut dyn RpcEnvironment,
+) -> Result<(), Error> {
+    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
+    let user_info = CachedUserInfo::new()?;
 
     let _lock = open_file_locked(sync::SYNC_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
 
     let sync_job: sync::SyncJobConfig = serde_json::from_value(param.clone())?;
+    if !check_sync_job_modify_access(&user_info, &auth_id, &sync_job) {
+        bail!("permission check failed");
+    }
 
     let (mut config, _digest) = sync::config()?;
 
@@ -104,15 +185,26 @@ pub fn create_sync_job(param: Value) -> Result<(), Error> {
         description: "The sync job configuration.",
         type: sync::SyncJobConfig,
     },
+    access: {
+        description: "Limited to sync job entries where user has Datastore.Audit on target datastore, and Remote.Audit on source remote.",
+        permission: &Permission::Anybody,
+    },
 )]
 /// Read a sync job configuration.
 pub fn read_sync_job(
     id: String,
     mut rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<SyncJobConfig, Error> {
+    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
+    let user_info = CachedUserInfo::new()?;
+
     let (config, digest) = sync::config()?;
 
     let sync_job = config.lookup("sync", &id)?;
+    if !check_sync_job_read_access(&user_info, &auth_id, &sync_job) {
+        bail!("permission check failed");
+    }
+
     rpcenv["digest"] = proxmox::tools::digest_to_hex(&digest).into();
 
     Ok(sync_job)
@@ -183,6 +275,10 @@ pub enum DeletableProperty {
             },
         },
     },
+    access: {
+        permission: &Permission::Anybody,
+        description: "User needs Datastore.Backup on target datastore, and Remote.Read on source remote. Additionally, remove_vanished requires Datastore.Prune, and any owner other than the user themselves requires Datastore.Modify",
+    },
 )]
 /// Update sync job config.
 pub fn update_sync_job(
@@ -196,7 +292,10 @@ pub fn update_sync_job(
     schedule: Option<String>,
     delete: Option<Vec<DeletableProperty>>,
     digest: Option<String>,
+    rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<(), Error> {
+    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
+    let user_info = CachedUserInfo::new()?;
 
     let _lock = open_file_locked(sync::SYNC_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
 
@@ -233,11 +332,15 @@ pub fn update_sync_job(
     if let Some(store) = store { data.store = store; }
     if let Some(remote) = remote { data.remote = remote; }
     if let Some(remote_store) = remote_store { data.remote_store = remote_store; }
-    if let Some(owner) = owner { data.owner = owner; }
+    if let Some(owner) = owner { data.owner = Some(owner); }
 
     if schedule.is_some() { data.schedule = schedule; }
     if remove_vanished.is_some() { data.remove_vanished = remove_vanished; }
 
+    if !check_sync_job_modify_access(&user_info, &auth_id, &data) {
+        bail!("permission check failed");
+    }
+
     config.set_data(&id, "sync", &data)?;
 
     sync::save_config(&config)?;
@@ -258,9 +361,19 @@ pub fn update_sync_job(
             },
         },
     },
+    access: {
+        permission: &Permission::Anybody,
+        description: "User needs Datastore.Backup on target datastore, and Remote.Read on source remote. Additionally, remove_vanished requires Datastore.Prune, and any owner other than the user themselves requires Datastore.Modify",
+    },
 )]
 /// Remove a sync job configuration
-pub fn delete_sync_job(id: String, digest: Option<String>) -> Result<(), Error> {
+pub fn delete_sync_job(
+    id: String,
+    digest: Option<String>,
+    rpcenv: &mut dyn RpcEnvironment,
+) -> Result<(), Error> {
+    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
+    let user_info = CachedUserInfo::new()?;
 
     let _lock = open_file_locked(sync::SYNC_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
 
@@ -271,10 +384,15 @@ pub fn delete_sync_job(id: String, digest: Option<String>) -> Result<(), Error>
         crate::tools::detect_modified_configuration_file(&digest, &expected_digest)?;
     }
 
-    match config.sections.get(&id) {
-        Some(_) => { config.sections.remove(&id); },
-        None => bail!("job '{}' does not exist.", id),
-    }
+    match config.lookup("sync", &id) {
+        Ok(job) => {
+            if !check_sync_job_modify_access(&user_info, &auth_id, &job) {
+                bail!("permission check failed");
+            }
+            config.sections.remove(&id);
+        },
+        Err(_) => { bail!("job '{}' does not exist.", id) },
+    };
 
     sync::save_config(&config)?;
 
diff --git a/src/config/sync.rs b/src/config/sync.rs
index d007570c..d2e945a1 100644
--- a/src/config/sync.rs
+++ b/src/config/sync.rs
@@ -21,7 +21,6 @@ lazy_static! {
     static ref CONFIG: SectionConfig = init();
 }
 
-
 #[api(
     properties: {
         id: {
@@ -72,6 +71,21 @@ pub struct SyncJobConfig {
     pub schedule: Option<String>,
 }
 
+impl From<&SyncJobStatus> for SyncJobConfig {
+    fn from(job_status: &SyncJobStatus) -> Self {
+        Self {
+            id: job_status.id.clone(),
+            store: job_status.store.clone(),
+            owner: job_status.owner.clone(),
+            remote: job_status.remote.clone(),
+            remote_store: job_status.remote_store.clone(),
+            remove_vanished: job_status.remove_vanished.clone(),
+            comment: job_status.comment.clone(),
+            schedule: job_status.schedule.clone(),
+        }
+    }
+}
+
 // FIXME: generate duplicate schemas/structs from one listing?
 #[api(
     properties: {
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 7/8] privs: remove PRIV_REMOVE_PRUNE

[Fabian Grünbichler]

it's not used anywhere, and not needed either until the day we might
implement push syncs.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/config/acl.rs | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/src/config/acl.rs b/src/config/acl.rs
index 7345adea..17eb47dc 100644
--- a/src/config/acl.rs
+++ b/src/config/acl.rs
@@ -42,7 +42,6 @@ constnamedbitmap! {
         PRIV_REMOTE_AUDIT("Remote.Audit");
         PRIV_REMOTE_MODIFY("Remote.Modify");
         PRIV_REMOTE_READ("Remote.Read");
-        PRIV_REMOTE_PRUNE("Remote.Prune");
 
         PRIV_SYS_CONSOLE("Sys.Console");
     }
@@ -96,14 +95,12 @@ PRIV_REMOTE_AUDIT;
 pub const ROLE_REMOTE_ADMIN: u64 =
 PRIV_REMOTE_AUDIT |
 PRIV_REMOTE_MODIFY |
-PRIV_REMOTE_READ |
-PRIV_REMOTE_PRUNE;
+PRIV_REMOTE_READ;
 
 /// Remote.SyncOperator can do read and prune on the remote.
 pub const ROLE_REMOTE_SYNC_OPERATOR: u64 =
 PRIV_REMOTE_AUDIT |
-PRIV_REMOTE_READ |
-PRIV_REMOTE_PRUNE;
+PRIV_REMOTE_READ;
 
 pub const ROLE_NAME_NO_ACCESS: &str ="NoAccess";
 
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 8/8] privs: add some more comments explaining privileges

[Fabian Grünbichler]

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/config/acl.rs | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/config/acl.rs b/src/config/acl.rs
index 17eb47dc..8cdce8bf 100644
--- a/src/config/acl.rs
+++ b/src/config/acl.rs
@@ -26,15 +26,23 @@ constnamedbitmap! {
         PRIV_SYS_MODIFY("Sys.Modify");
         PRIV_SYS_POWER_MANAGEMENT("Sys.PowerManagement");
 
+        /// Datastore.Audit allows knowing about a datastore,
+        /// including reading the configuration entry and listing its contents
         PRIV_DATASTORE_AUDIT("Datastore.Audit");
+        /// Datastore.Allocate allows creating or deleting datastores
         PRIV_DATASTORE_ALLOCATE("Datastore.Allocate");
+        /// Datastore.Modify allows modifying a datastore and its contents
         PRIV_DATASTORE_MODIFY("Datastore.Modify");
+        /// Datastore.Read allows reading arbitrary backup contents
         PRIV_DATASTORE_READ("Datastore.Read");
+        /// Allows verifying a datastore
         PRIV_DATASTORE_VERIFY("Datastore.Verify");
 
-        /// Datastore.Backup also requires backup ownership
+        /// Datastore.Backup allows Datastore.Read|Verify and creating new snapshots,
+        /// but also requires backup ownership
         PRIV_DATASTORE_BACKUP("Datastore.Backup");
-        /// Datastore.Prune also requires backup ownership
+        /// Datastore.Prune allows deleting snapshots,
+        /// but also requires backup ownership
         PRIV_DATASTORE_PRUNE("Datastore.Prune");
 
         PRIV_PERMISSIONS_MODIFY("Permissions.Modify");
-- 
2.20.1

[pbs-devel] partially-applied: [PATCH proxmox-backup 0/8] permission improvements

[Thomas Lamprecht]

On 30.10.20 12:36, Fabian Grünbichler wrote:
> this series cleans up
> - get/set_notes permissions
> - unused PRIV_REMOTE_PRUNE
> 
> reworks verification permissions:
> - add a new PRIV_DATASTORE_VERIFY that allows verifying whole datastores
> - allows unprivileged users to verify their part of a datastore in bulk
> - allows non-superusers to setup and view verification jobs (if they are
>   privileged enough)
> 
> reworks pulls/syncs:
> - allow setting an owner
> - allow non-superusers to setup and view sync jobs (if they are
>   privileged enough)
> 
> Fabian Grünbichler (8):
>   privs: allow reading notes with Datastore.Audit
>   privs: use Datastore.Modify|Backup to set backup notes
>   verify: introduce & use new Datastore.Verify privilege
>   verify jobs: add permissions
>   sync: add owner
>   sync: allow sync for non-superusers
>   privs: remove PRIV_REMOVE_PRUNE
>   privs: add some more comments explaining privileges
> 
>  src/api2/admin/datastore.rs |  34 ++++++--
>  src/api2/admin/sync.rs      |  30 ++++++-
>  src/api2/config/remote.rs   |  15 +++-
>  src/api2/config/sync.rs     | 152 +++++++++++++++++++++++++++++++++---
>  src/api2/config/verify.rs   |  41 +++++++++-
>  src/api2/pull.rs            |   5 +-
>  src/backup/verify.rs        |  29 ++++++-
>  src/config/acl.rs           |  24 ++++--
>  src/config/sync.rs          |  28 ++++++-
>  src/server/verify_job.rs    |   2 +-
>  www/config/SyncView.js      |  14 +++-
>  www/window/SyncJobEdit.js   |  20 +++++
>  12 files changed, 352 insertions(+), 42 deletions(-)
> 



applied all but the sync related ones (patch 5/8 and 6/8), need some more thoughts
and another opinion would not hurt there - thanks!

[pbs-devel] applied: [PATCH proxmox-backup 5/8] fix #2864: add owner option to sync

[Dietmar Maurer]

applied

[pbs-devel] applied: [PATCH proxmox-backup 6/8] sync: allow sync for non-superusers

[Dietmar Maurer]

applied, but it would be great to have regression tests for

check_sync_job_read_access() and check_sync_job_modify_access()