From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 623FE66564 for ; Fri, 6 Nov 2020 11:23:54 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 5ADD21FF52 for ; Fri, 6 Nov 2020 11:23:24 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 99F591FF48 for ; Fri, 6 Nov 2020 11:23:23 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 663EE437AF for ; Fri, 6 Nov 2020 11:23:23 +0100 (CET) From: =?UTF-8?q?Fabian=20Gr=C3=BCnbichler?= To: pbs-devel@lists.proxmox.com Date: Fri, 6 Nov 2020 11:23:09 +0100 Message-Id: <20201106102309.1458959-4-f.gruenbichler@proxmox.com> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20201106102309.1458959-1-f.gruenbichler@proxmox.com> References: <20201106102309.1458959-1-f.gruenbichler@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.024 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [mod.rs, tasks.rs, pull.rs] Subject: [pbs-devel] [PATCH proxmox-backup 3/3] tasks: allow access to job tasks X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 06 Nov 2020 10:23:54 -0000 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 --- 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, ) -> Result { - 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