[pbs-devel] [PATCH proxmox-backup 0/3] job privileges

[pbs-devel] [PATCH proxmox-backup 0/3] job privileges

[Fabian Grünbichler]

this series reworks some more job privileges:

the first is a follow-up fix for verification job config privileges,
the second extends it to the corresponding 'admin/' API endpoints,
and the third is a usability improvement to allow semi-privileged users
to view/access job tasks which don't belong to them

some commit messages might need to be adapted if this gets applied after
backup@pam was replaced with root@pam in the scheduler

Fabian Grünbichler (3):
  verify: fix unprivileged verification jobs
  verify: allow unprivileged access to admin API
  tasks: allow access to job tasks

 src/api2/admin/verify.rs  | 33 ++++++++++++--
 src/api2/config/verify.rs | 92 +++++++++++++++++++++++++++------------
 src/api2/node/tasks.rs    | 74 +++++++++++++++++++++++++++++--
 src/api2/pull.rs          |  8 +++-
 src/api2/types/mod.rs     |  5 +++
 src/server/verify_job.rs  |  6 ++-
 6 files changed, 179 insertions(+), 39 deletions(-)

-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 1/3] verify: fix unprivileged verification jobs

[Fabian Grünbichler]

since the store is not a path parameter, we need to do manual instead of
schema checks. also dropping Datastore.Backup here

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

diff --git a/src/api2/config/verify.rs b/src/api2/config/verify.rs
index 0c5a6460..2c03e33e 100644
--- a/src/api2/config/verify.rs
+++ b/src/api2/config/verify.rs
@@ -9,10 +9,11 @@ use crate::api2::types::*;
 
 use crate::config::acl::{
     PRIV_DATASTORE_AUDIT,
-    PRIV_DATASTORE_BACKUP,
     PRIV_DATASTORE_VERIFY,
 };
 
+use crate::config::cached_user_info::CachedUserInfo;
+
 use crate::config::verify::{self, VerificationJobConfig};
 
 #[api(
@@ -25,10 +26,8 @@ use crate::config::verify::{self, VerificationJobConfig};
         items: { type: verify::VerificationJobConfig },
     },
     access: {
-        permission: &Permission::Privilege(
-            &["datastore", "{store}"],
-            PRIV_DATASTORE_AUDIT | PRIV_DATASTORE_BACKUP | PRIV_DATASTORE_VERIFY,
-            true),
+        permission: &Permission::Anybody,
+        description: "Requires Datastore.Audit or Datastore.Verify on datastore.",
     },
 )]
 /// List all verification jobs
@@ -36,11 +35,22 @@ pub fn list_verification_jobs(
     _param: Value,
     mut rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<Vec<VerificationJobConfig>, Error> {
+    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
+    let user_info = CachedUserInfo::new()?;
+
+    let required_privs = PRIV_DATASTORE_AUDIT | PRIV_DATASTORE_VERIFY;
 
     let (config, digest) = verify::config()?;
 
     let list = config.convert_to_typed_array("verification")?;
 
+    let list = list.into_iter()
+        .filter(|job: &VerificationJobConfig| {
+            let privs = user_info.lookup_privs(&auth_id, &["datastore", &job.store]);
+
+            privs & required_privs != 00
+        }).collect();
+
     rpcenv["digest"] = proxmox::tools::digest_to_hex(&digest).into();
 
     Ok(list)
@@ -76,19 +86,24 @@ pub fn list_verification_jobs(
         }
     },
     access: {
-        permission: &Permission::Privilege(
-            &["datastore", "{store}"],
-            PRIV_DATASTORE_VERIFY,
-            true),
+        permission: &Permission::Anybody,
+        description: "Requires Datastore.Verify on job's datastore.",
     },
 )]
 /// Create a new verification job.
-pub fn create_verification_job(param: Value) -> Result<(), Error> {
-
-    let _lock = open_file_locked(verify::VERIFICATION_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
+pub fn create_verification_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 verification_job: verify::VerificationJobConfig = serde_json::from_value(param.clone())?;
 
+    user_info.check_privs(&auth_id, &["datastore", &verification_job.store], PRIV_DATASTORE_VERIFY, false)?;
+
+    let _lock = open_file_locked(verify::VERIFICATION_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
+
     let (mut config, _digest) = verify::config()?;
 
     if let Some(_) = config.sections.get(&verification_job.id) {
@@ -117,10 +132,8 @@ pub fn create_verification_job(param: Value) -> Result<(), Error> {
         type: verify::VerificationJobConfig,
     },
     access: {
-        permission: &Permission::Privilege(
-            &["datastore", "{store}"],
-            PRIV_DATASTORE_AUDIT | PRIV_DATASTORE_BACKUP | PRIV_DATASTORE_VERIFY,
-            true),
+        permission: &Permission::Anybody,
+        description: "Requires Datastore.Audit or Datastore.Verify on job's datastore.",
     },
 )]
 /// Read a verification job configuration.
@@ -128,9 +141,16 @@ pub fn read_verification_job(
     id: String,
     mut rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<VerificationJobConfig, Error> {
+    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
+    let user_info = CachedUserInfo::new()?;
+
     let (config, digest) = verify::config()?;
 
-    let verification_job = config.lookup("verification", &id)?;
+    let verification_job: verify::VerificationJobConfig = config.lookup("verification", &id)?;
+
+    let required_privs = PRIV_DATASTORE_AUDIT | PRIV_DATASTORE_VERIFY;
+    user_info.check_privs(&auth_id, &["datastore", &verification_job.store], required_privs, true)?;
+
     rpcenv["digest"] = proxmox::tools::digest_to_hex(&digest).into();
 
     Ok(verification_job)
@@ -193,10 +213,8 @@ pub enum DeletableProperty {
         },
     },
     access: {
-        permission: &Permission::Privilege(
-            &["datastore", "{store}"],
-            PRIV_DATASTORE_VERIFY,
-            true),
+        permission: &Permission::Anybody,
+        description: "Requires Datastore.Verify on job's datastore.",
     },
 )]
 /// Update verification job config.
@@ -209,7 +227,10 @@ pub fn update_verification_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(verify::VERIFICATION_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
 
@@ -223,7 +244,10 @@ pub fn update_verification_job(
 
     let mut data: verify::VerificationJobConfig = config.lookup("verification", &id)?;
 
-     if let Some(delete) = delete {
+    // check existing store
+    user_info.check_privs(&auth_id, &["datastore", &data.store], PRIV_DATASTORE_VERIFY, true)?;
+
+    if let Some(delete) = delete {
         for delete_prop in delete {
             match delete_prop {
                 DeletableProperty::IgnoreVerified => { data.ignore_verified = None; },
@@ -243,7 +267,12 @@ pub fn update_verification_job(
         }
     }
 
-    if let Some(store) = store { data.store = store; }
+    if let Some(store) = store {
+        // check new store
+        user_info.check_privs(&auth_id, &["datastore", &store], PRIV_DATASTORE_VERIFY, true)?;
+        data.store = store;
+    }
+
 
     if ignore_verified.is_some() { data.ignore_verified = ignore_verified; }
     if outdated_after.is_some() { data.outdated_after = outdated_after; }
@@ -270,19 +299,26 @@ pub fn update_verification_job(
         },
     },
     access: {
-        permission: &Permission::Privilege(
-            &["datastore", "{store}"],
-            PRIV_DATASTORE_VERIFY,
-            true),
+        permission: &Permission::Anybody,
+        description: "Requires Datastore.Verify on job's datastore.",
     },
 )]
 /// Remove a verification job configuration
-pub fn delete_verification_job(id: String, digest: Option<String>) -> Result<(), Error> {
+pub fn delete_verification_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(verify::VERIFICATION_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
 
     let (mut config, expected_digest) = verify::config()?;
 
+    let job: verify::VerificationJobConfig = config.lookup("verification", &id)?;
+    user_info.check_privs(&auth_id, &["datastore", &job.store], PRIV_DATASTORE_VERIFY, true)?;
+
     if let Some(ref digest) = digest {
         let digest = proxmox::tools::hex_to_digest(digest)?;
         crate::tools::detect_modified_configuration_file(&digest, &expected_digest)?;
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 2/3] verify: allow unprivileged access to admin API

[Fabian Grünbichler]

which is the one used by the GUI.

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

diff --git a/src/api2/admin/verify.rs b/src/api2/admin/verify.rs
index 02c1eef4..6c345cd8 100644
--- a/src/api2/admin/verify.rs
+++ b/src/api2/admin/verify.rs
@@ -2,11 +2,16 @@ use anyhow::{format_err, Error};
 
 use proxmox::api::router::SubdirMap;
 use proxmox::{list_subdirs_api_method, sortable};
-use proxmox::api::{api, ApiMethod, Router, RpcEnvironment};
+use proxmox::api::{api, ApiMethod, Permission, Router, RpcEnvironment};
 
 use crate::api2::types::*;
 use crate::server::do_verification_job;
 use crate::server::jobstate::{Job, JobState};
+use crate::config::acl::{
+    PRIV_DATASTORE_AUDIT,
+    PRIV_DATASTORE_VERIFY,
+};
+use crate::config::cached_user_info::CachedUserInfo;
 use crate::config::verify;
 use crate::config::verify::{VerificationJobConfig, VerificationJobStatus};
 use serde_json::Value;
@@ -23,10 +28,14 @@ use crate::server::UPID;
         },
     },
     returns: {
-        description: "List configured jobs and their status.",
+        description: "List configured jobs and their status (filtered by access)",
         type: Array,
         items: { type: verify::VerificationJobStatus },
     },
+    access: {
+        permission: &Permission::Anybody,
+        description: "Requires Datastore.Audit or Datastore.Verify on datastore.",
+    },
 )]
 /// List all verification jobs
 pub fn list_verification_jobs(
@@ -34,6 +43,10 @@ pub fn list_verification_jobs(
     _param: Value,
     mut rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<Vec<VerificationJobStatus>, Error> {
+    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
+    let user_info = CachedUserInfo::new()?;
+
+    let required_privs = PRIV_DATASTORE_AUDIT | PRIV_DATASTORE_VERIFY;
 
     let (config, digest) = verify::config()?;
 
@@ -41,6 +54,11 @@ pub fn list_verification_jobs(
         .convert_to_typed_array("verification")?
         .into_iter()
         .filter(|job: &VerificationJobStatus| {
+            let privs = user_info.lookup_privs(&auth_id, &["datastore", &job.store]);
+            if privs & required_privs == 0 {
+                return false;
+            }
+
             if let Some(store) = &store {
                 &job.store == store
             } else {
@@ -90,7 +108,11 @@ pub fn list_verification_jobs(
                 schema: JOB_ID_SCHEMA,
             }
         }
-    }
+    },
+    access: {
+        permission: &Permission::Anybody,
+        description: "Requires Datastore.Verify on job's datastore.",
+    },
 )]
 /// Runs a verification job manually.
 fn run_verification_job(
@@ -98,10 +120,13 @@ fn run_verification_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) = verify::config()?;
     let verification_job: VerificationJobConfig = config.lookup("verification", &id)?;
 
-    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
+    user_info.check_privs(&auth_id, &["datastore", &verification_job.store], PRIV_DATASTORE_VERIFY, true)?;
 
     let job = Job::new("verificationjob", &id)?;
 
-- 
2.20.1

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

[Fabian Grünbichler]

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

[pbs-devel] applied: [PATCH proxmox-backup 0/3] job privileges

[Dietmar Maurer]

applied

> On 11/06/2020 11:23 AM Fabian Grünbichler <f.gruenbichler@proxmox.com> wrote:
> 
>  
> this series reworks some more job privileges:
> 
> the first is a follow-up fix for verification job config privileges,
> the second extends it to the corresponding 'admin/' API endpoints,
> and the third is a usability improvement to allow semi-privileged users
> to view/access job tasks which don't belong to them
> 
> some commit messages might need to be adapted if this gets applied after
> backup@pam was replaced with root@pam in the scheduler
> 
> Fabian Grünbichler (3):
>   verify: fix unprivileged verification jobs
>   verify: allow unprivileged access to admin API
>   tasks: allow access to job tasks
> 
>  src/api2/admin/verify.rs  | 33 ++++++++++++--
>  src/api2/config/verify.rs | 92 +++++++++++++++++++++++++++------------
>  src/api2/node/tasks.rs    | 74 +++++++++++++++++++++++++++++--
>  src/api2/pull.rs          |  8 +++-
>  src/api2/types/mod.rs     |  5 +++
>  src/server/verify_job.rs  |  6 ++-
>  6 files changed, 179 insertions(+), 39 deletions(-)
> 
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] applied: applied: [PATCH proxmox-backup 0/3] job privileges

[Dietmar Maurer]

applied

Re: [pbs-devel] applied: applied: [PATCH proxmox-backup 0/3] job privileges

[Dietmar Maurer]

sorry, please ignore this mail!