[pbs-devel] [PATCH proxmox-backup 3/3] tasks: allow access to job tasks

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

if the user/token could have either configured/manually executed the
task, but it was either executed via the schedule (backup@pam) or
another user/token.

without this change, semi-privileged users (that cannot read all tasks
globally, but are DatastoreAdmin) could schedule jobs, but not read
their logs once the schedule executes them. it also makes sense for
multiple such users to see eachothers manually executed jobs, as long as
the privilege level on the datastore (or remote/remote_store/local
store) itself is sufficient.

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

Notes:
    especially the sync job is now rather long with the auto-generated ID, not sure
    whether we want to extract this information for better display on the GUI side
    as well? :-/
    
    group/snapshot prune and group/snapshot verification are not included in this
    relaxation of required privileges, but they could be if we want a user with
    'Datastore.Verify' to be able to see ALL verification tasks on a datastore (or
    'Datastore.Modify' -> prune tasks), instead of just the store-level job ones.
    
    Datastore.Audit might be a candidate for read-only access to a
    store's job tasks, but I did not want to make this any more
    complicated than it already is..

 src/api2/node/tasks.rs   | 74 ++++++++++++++++++++++++++++++++++++++--
 src/api2/pull.rs         |  8 +++--
 src/api2/types/mod.rs    |  5 +++
 src/server/verify_job.rs |  6 ++--
 4 files changed, 86 insertions(+), 7 deletions(-)

diff --git a/src/api2/node/tasks.rs b/src/api2/node/tasks.rs
index a638b04f..8523d41b 100644
--- a/src/api2/node/tasks.rs
+++ b/src/api2/node/tasks.rs
@@ -1,7 +1,7 @@
 use std::fs::File;
 use std::io::{BufRead, BufReader};
 
-use anyhow::{Error};
+use anyhow::{bail, Error};
 use serde_json::{json, Value};
 
 use proxmox::api::{api, Router, RpcEnvironment, Permission};
@@ -9,19 +9,87 @@ use proxmox::api::router::SubdirMap;
 use proxmox::{identity, list_subdirs_api_method, sortable};
 
 use crate::tools;
+
 use crate::api2::types::*;
+use crate::api2::pull::check_pull_privs;
+
 use crate::server::{self, UPID, TaskState, TaskListInfoIterator};
-use crate::config::acl::{PRIV_SYS_AUDIT, PRIV_SYS_MODIFY};
+use crate::config::acl::{
+    PRIV_DATASTORE_MODIFY,
+    PRIV_DATASTORE_VERIFY,
+    PRIV_SYS_AUDIT,
+    PRIV_SYS_MODIFY,
+};
 use crate::config::cached_user_info::CachedUserInfo;
 
+// matches respective job execution privileges
+fn check_job_privs(auth_id: &Authid, user_info: &CachedUserInfo, upid: &UPID) -> Result<(), Error> {
+    match (upid.worker_type.as_str(), &upid.worker_id) {
+        ("verificationjob", Some(workerid)) => {
+            if let Some(captures) = VERIFICATION_JOB_WORKER_ID_REGEX.captures(&workerid) {
+                if let Some(store) = captures.get(1) {
+                    return user_info.check_privs(&auth_id,
+                                                 &["datastore", store.as_str()],
+                                                 PRIV_DATASTORE_VERIFY,
+                                                 true);
+                }
+            }
+        },
+        ("syncjob", Some(workerid)) => {
+            if let Some(captures) = SYNC_JOB_WORKER_ID_REGEX.captures(&workerid) {
+                let remote = captures.get(1);
+                let remote_store = captures.get(2);
+                let local_store = captures.get(3);
+
+                if let (Some(remote), Some(remote_store), Some(local_store)) =
+                    (remote, remote_store, local_store) {
+
+                    return check_pull_privs(&auth_id,
+                                            local_store.as_str(),
+                                            remote.as_str(),
+                                            remote_store.as_str(),
+                                            false);
+                }
+            }
+        },
+        ("garbage_collection", Some(workerid)) => {
+            return user_info.check_privs(&auth_id,
+                                         &["datastore", &workerid],
+                                         PRIV_DATASTORE_MODIFY,
+                                         true)
+        },
+        ("prune", Some(workerid)) => {
+            return user_info.check_privs(&auth_id,
+                                         &["datastore",
+                                         &workerid],
+                                         PRIV_DATASTORE_MODIFY,
+                                         true);
+        },
+        _ => bail!("not a scheduled job task"),
+    };
+
+    bail!("not a scheduled job task");
+}
+
 fn check_task_access(auth_id: &Authid, upid: &UPID) -> Result<(), Error> {
     let task_auth_id = &upid.auth_id;
     if auth_id == task_auth_id
         || (task_auth_id.is_token() && &Authid::from(task_auth_id.user().clone()) == auth_id) {
+        // task owner can always read
         Ok(())
     } else {
         let user_info = CachedUserInfo::new()?;
-        user_info.check_privs(auth_id, &["system", "tasks"], PRIV_SYS_AUDIT, false)
+
+        let task_privs = user_info.lookup_privs(auth_id, &["system", "tasks"]);
+        if task_privs & PRIV_SYS_AUDIT != 0 {
+            // allowed to read all tasks in general
+            Ok(())
+        } else if check_job_privs(&auth_id, &user_info, upid).is_ok() {
+            // job which the user/token could have configured/manually executed
+            Ok(())
+        } else {
+            bail!("task access not allowed");
+        }
     }
 }
 
diff --git a/src/api2/pull.rs b/src/api2/pull.rs
index 7ac6308e..ff59abe1 100644
--- a/src/api2/pull.rs
+++ b/src/api2/pull.rs
@@ -64,14 +64,18 @@ pub fn do_sync_job(
     schedule: Option<String>,
 ) -> Result<String, Error> {
 
-    let job_id = job.jobname().to_string();
+    let job_id = format!("{}:{}:{}:{}",
+                         sync_job.remote,
+                         sync_job.remote_store,
+                         sync_job.store,
+                         job.jobname());
     let worker_type = job.jobtype().to_string();
 
     let (email, notify) = crate::server::lookup_datastore_notify_settings(&sync_job.store);
 
     let upid_str = WorkerTask::spawn(
         &worker_type,
-        Some(job.jobname().to_string()),
+        Some(job_id.clone()),
         auth_id.clone(),
         false,
         move |worker| async move {
diff --git a/src/api2/types/mod.rs b/src/api2/types/mod.rs
index b1dac2f8..28c6a622 100644
--- a/src/api2/types/mod.rs
+++ b/src/api2/types/mod.rs
@@ -59,6 +59,11 @@ const_regex!{
     /// any identifier command line tools work with.
     pub PROXMOX_SAFE_ID_REGEX = concat!(r"^", PROXMOX_SAFE_ID_REGEX_STR!(), r"$");
 
+    /// Regex for verification jobs 'DATASTORE:ACTUAL_JOB_ID'
+    pub VERIFICATION_JOB_WORKER_ID_REGEX = concat!(r"^(", PROXMOX_SAFE_ID_REGEX_STR!(), r"):");
+    /// Regex for sync jobs 'REMOTE:REMOTE_DATASTORE:LOCAL_DATASTORE:ACTUAL_JOB_ID'
+    pub SYNC_JOB_WORKER_ID_REGEX = concat!(r"^(", PROXMOX_SAFE_ID_REGEX_STR!(), r"):(", PROXMOX_SAFE_ID_REGEX_STR!(), r"):(", PROXMOX_SAFE_ID_REGEX_STR!(), r"):");
+
     pub SINGLE_LINE_COMMENT_REGEX = r"^[[:^cntrl:]]*$";
 
     pub HOSTNAME_REGEX = r"^(?:[a-zA-Z0-9](?:[a-zA-Z0-9\-]*[a-zA-Z0-9])?)$";
diff --git a/src/server/verify_job.rs b/src/server/verify_job.rs
index 9cea3fee..bcf57f8a 100644
--- a/src/server/verify_job.rs
+++ b/src/server/verify_job.rs
@@ -50,11 +50,13 @@ pub fn do_verification_job(
 
     let (email, notify) = crate::server::lookup_datastore_notify_settings(&verification_job.store);
 
-    let job_id = job.jobname().to_string();
+    let job_id = format!("{}:{}",
+                         &verification_job.store,
+                         job.jobname());
     let worker_type = job.jobtype().to_string();
     let upid_str = WorkerTask::new_thread(
         &worker_type,
-        Some(job.jobname().to_string()),
+        Some(job_id.clone()),
         auth_id.clone(),
         false,
         move |worker| {
-- 
2.20.1