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 DEB446C706 for ; Thu, 23 Sep 2021 12:14:11 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id DA68723E88 for ; Thu, 23 Sep 2021 12:13:41 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (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 9FDD023E42 for ; Thu, 23 Sep 2021 12:13:35 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 778AB44A76; Thu, 23 Sep 2021 12:13:35 +0200 (CEST) From: Dietmar Maurer To: pbs-devel@lists.proxmox.com Date: Thu, 23 Sep 2021 12:13:24 +0200 Message-Id: <20210923101329.950146-1-dietmar@proxmox.com> X-Mailer: git-send-email 2.30.2 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.579 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: [pbs-devel] [PATCH proxmox-backup 1/6] src/server/worker_task.rs: Avoid using pbs-api-type::Authid 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: Thu, 23 Sep 2021 10:14:11 -0000 Because we want to move worker_task.rs into proxmox-rest-server crate. --- pbs-api-types/src/upid.rs | 13 +++++++------ src/api2/admin/datastore.rs | 6 +++--- src/api2/backup/environment.rs | 2 +- src/api2/backup/mod.rs | 2 +- src/api2/config/acme.rs | 6 +++--- src/api2/config/datastore.rs | 4 ++-- src/api2/node/apt.rs | 4 ++-- src/api2/node/certificates.rs | 6 +++--- src/api2/node/disks/directory.rs | 4 ++-- src/api2/node/disks/mod.rs | 4 ++-- src/api2/node/disks/zfs.rs | 4 ++-- src/api2/node/mod.rs | 2 +- src/api2/node/network.rs | 2 +- src/api2/node/services.rs | 2 +- src/api2/node/tasks.rs | 15 +++++++++------ src/api2/pull.rs | 4 ++-- src/api2/reader/mod.rs | 2 +- src/api2/tape/backup.rs | 4 ++-- src/api2/tape/drive.rs | 2 +- src/api2/tape/restore.rs | 2 +- src/bin/proxmox-backup-proxy.rs | 2 +- src/server/gc_job.rs | 2 +- src/server/prune_job.rs | 2 +- src/server/verify_job.rs | 2 +- src/server/worker_task.rs | 8 ++++---- 25 files changed, 55 insertions(+), 51 deletions(-) diff --git a/pbs-api-types/src/upid.rs b/pbs-api-types/src/upid.rs index ba23a646..29135bca 100644 --- a/pbs-api-types/src/upid.rs +++ b/pbs-api-types/src/upid.rs @@ -8,8 +8,6 @@ use proxmox::api::schema::{ApiStringFormat, ApiType, Schema, StringSchema, Array use proxmox::const_regex; use proxmox::sys::linux::procfs; -use crate::Authid; - /// Unique Process/Task Identifier /// /// We use this to uniquely identify worker task. UPIDs have a short @@ -37,7 +35,7 @@ pub struct UPID { /// Worker ID (arbitrary ASCII string) pub worker_id: Option, /// The authenticated entity who started the task - pub auth_id: Authid, + pub auth_id: String, /// The node name. pub node: String, } @@ -71,7 +69,7 @@ impl UPID { pub fn new( worker_type: &str, worker_id: Option, - auth_id: Authid, + auth_id: String, ) -> Result { let pid = unsafe { libc::getpid() }; @@ -82,6 +80,10 @@ impl UPID { bail!("illegal characters in worker type '{}'", worker_type); } + if auth_id.contains(bad) { + bail!("illegal characters in auth_id '{}'", auth_id); + } + static WORKER_TASK_NEXT_ID: AtomicUsize = AtomicUsize::new(0); let task_id = WORKER_TASK_NEXT_ID.fetch_add(1, Ordering::SeqCst); @@ -184,7 +186,7 @@ pub struct TaskListItem { /// Worker ID (arbitrary ASCII string) pub worker_id: Option, /// The authenticated entity who started the task - pub user: Authid, + pub user: String, /// The task end time (Epoch) #[serde(skip_serializing_if="Option::is_none")] pub endtime: Option, @@ -200,4 +202,3 @@ pub const NODE_TASKS_LIST_TASKS_RETURN_TYPE: ReturnType = ReturnType { &TaskListItem::API_SCHEMA, ).schema(), }; - diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs index 0b14dfbf..fbb93f35 100644 --- a/src/api2/admin/datastore.rs +++ b/src/api2/admin/datastore.rs @@ -722,7 +722,7 @@ pub fn verify( let upid_str = WorkerTask::new_thread( worker_type, Some(worker_id), - auth_id.clone(), + auth_id.to_string(), to_stdout, move |worker| { let verify_worker = crate::backup::VerifyWorker::new(worker.clone(), datastore); @@ -862,7 +862,7 @@ pub fn prune( // We use a WorkerTask just to have a task log, but run synchrounously - let worker = WorkerTask::new("prune", Some(worker_id), auth_id, true)?; + let worker = WorkerTask::new("prune", Some(worker_id), auth_id.to_string(), true)?; if keep_all { worker.log("No prune selection - keeping all files."); @@ -957,7 +957,7 @@ pub fn prune_datastore( let upid_str = WorkerTask::new_thread( "prune", Some(store.clone()), - auth_id.clone(), + auth_id.to_string(), to_stdout, move |worker| crate::server::prune_datastore( worker.clone(), diff --git a/src/api2/backup/environment.rs b/src/api2/backup/environment.rs index 129ebd2b..306f91ee 100644 --- a/src/api2/backup/environment.rs +++ b/src/api2/backup/environment.rs @@ -525,7 +525,7 @@ impl BackupEnvironment { WorkerTask::new_thread( "verify", Some(worker_id), - self.auth_id.clone(), + self.auth_id.to_string(), false, move |worker| { worker.log("Automatically verifying newly added snapshot"); diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs index 8f51f314..c14f19a4 100644 --- a/src/api2/backup/mod.rs +++ b/src/api2/backup/mod.rs @@ -166,7 +166,7 @@ async move { if !is_new { bail!("backup directory already exists."); } - WorkerTask::spawn(worker_type, Some(worker_id), auth_id.clone(), true, move |worker| { + WorkerTask::spawn(worker_type, Some(worker_id), auth_id.to_string(), true, move |worker| { let mut env = BackupEnvironment::new( env_type, auth_id, worker.clone(), datastore, backup_dir); diff --git a/src/api2/config/acme.rs b/src/api2/config/acme.rs index 593b79a3..564cafae 100644 --- a/src/api2/config/acme.rs +++ b/src/api2/config/acme.rs @@ -215,7 +215,7 @@ fn register_account( WorkerTask::spawn( "acme-register", Some(name.to_string()), - auth_id, + auth_id.to_string(), true, move |worker| async move { let mut client = AcmeClient::new(directory); @@ -275,7 +275,7 @@ pub fn update_account( WorkerTask::spawn( "acme-update", Some(name.to_string()), - auth_id, + auth_id.to_string(), true, move |_worker| async move { let data = match contact { @@ -320,7 +320,7 @@ pub fn deactivate_account( WorkerTask::spawn( "acme-deactivate", Some(name.to_string()), - auth_id, + auth_id.to_string(), true, move |worker| async move { match AcmeClient::load(&name) diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs index c6036fc3..0e6529f8 100644 --- a/src/api2/config/datastore.rs +++ b/src/api2/config/datastore.rs @@ -119,9 +119,9 @@ pub fn create_datastore( WorkerTask::new_thread( "create-datastore", Some(config.name.to_string()), - auth_id, + auth_id.to_string(), to_stdout, - move |worker| do_create_datastore(lock, section_config, config, Some(&worker)), + move |worker| do_create_datastore(lock, section_config, config, Some(&worker)), ) } diff --git a/src/api2/node/apt.rs b/src/api2/node/apt.rs index 8f4bc691..f02920c0 100644 --- a/src/api2/node/apt.rs +++ b/src/api2/node/apt.rs @@ -14,7 +14,7 @@ use proxmox_apt::repositories::{ use proxmox_http::ProxyConfig; use pbs_api_types::{ - Authid, APTUpdateInfo, NODE_SCHEMA, PROXMOX_CONFIG_DIGEST_SCHEMA, UPID_SCHEMA, + APTUpdateInfo, NODE_SCHEMA, PROXMOX_CONFIG_DIGEST_SCHEMA, UPID_SCHEMA, PRIV_SYS_AUDIT, PRIV_SYS_MODIFY, }; @@ -154,7 +154,7 @@ pub fn apt_update_database( rpcenv: &mut dyn RpcEnvironment, ) -> Result { - let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?; + let auth_id = rpcenv.get_auth_id().unwrap(); let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI; let upid_str = WorkerTask::new_thread("aptupdate", None, auth_id, to_stdout, move |worker| { diff --git a/src/api2/node/certificates.rs b/src/api2/node/certificates.rs index 82fa028d..7b31861e 100644 --- a/src/api2/node/certificates.rs +++ b/src/api2/node/certificates.rs @@ -11,7 +11,7 @@ use proxmox::api::router::SubdirMap; use proxmox::api::{api, Permission, Router, RpcEnvironment}; use proxmox::list_subdirs_api_method; -use pbs_api_types::{Authid, NODE_SCHEMA, PRIV_SYS_MODIFY}; +use pbs_api_types::{NODE_SCHEMA, PRIV_SYS_MODIFY}; use pbs_buildcfg::configdir; use pbs_tools::cert; @@ -530,7 +530,7 @@ fn spawn_certificate_worker( let (node_config, _digest) = crate::config::node::config()?; - let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?; + let auth_id = rpcenv.get_auth_id().unwrap(); WorkerTask::spawn(name, None, auth_id, true, move |worker| async move { if let Some(cert) = order_certificate(worker, &node_config).await? { @@ -559,7 +559,7 @@ pub fn revoke_acme_cert(rpcenv: &mut dyn RpcEnvironment) -> Result, rpcenv: &mut dyn RpcEnvironment) -> Resu let upid = WorkerTask::spawn( "termproxy", None, - auth_id, + auth_id.to_string(), false, move |worker| async move { // move inside the worker so that it survives and does not close the port diff --git a/src/api2/node/network.rs b/src/api2/node/network.rs index 351fd11c..0fde9f2a 100644 --- a/src/api2/node/network.rs +++ b/src/api2/node/network.rs @@ -703,7 +703,7 @@ pub async fn reload_network_config( let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?; - let upid_str = WorkerTask::spawn("srvreload", Some(String::from("networking")), auth_id, true, |_worker| async { + let upid_str = WorkerTask::spawn("srvreload", Some(String::from("networking")), auth_id.to_string(), true, |_worker| async { let _ = std::fs::rename(network::NETWORK_INTERFACES_NEW_FILENAME, network::NETWORK_INTERFACES_FILENAME); diff --git a/src/api2/node/services.rs b/src/api2/node/services.rs index 25edd1b6..6c757f43 100644 --- a/src/api2/node/services.rs +++ b/src/api2/node/services.rs @@ -195,7 +195,7 @@ fn run_service_command(service: &str, cmd: &str, auth_id: Authid) -> Result bool { } fn check_task_access(auth_id: &Authid, upid: &UPID) -> Result<(), Error> { - let task_auth_id = &upid.auth_id; - if auth_id == task_auth_id + let task_auth_id: Authid = upid.auth_id.parse()?; + 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(()) @@ -200,6 +200,8 @@ async fn get_task_status( let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?; check_task_access(&auth_id, &upid)?; + let task_auth_id: Authid = upid.auth_id.parse()?; + let mut result = json!({ "upid": param["upid"], "node": upid.node, @@ -208,11 +210,11 @@ async fn get_task_status( "starttime": upid.starttime, "type": upid.worker_type, "id": upid.worker_id, - "user": upid.auth_id.user(), + "user": task_auth_id.user(), }); - if upid.auth_id.is_token() { - result["tokenid"] = Value::from(upid.auth_id.tokenname().unwrap().as_str()); + if task_auth_id.is_token() { + result["tokenid"] = Value::from(task_auth_id.tokenname().unwrap().as_str()); } if crate::server::worker_is_active(&upid).await? { @@ -344,10 +346,11 @@ fn stop_task( let upid = extract_upid(¶m)?; - let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?; + let auth_id = rpcenv.get_auth_id().unwrap(); if auth_id != upid.auth_id { let user_info = CachedUserInfo::new()?; + let auth_id: Authid = auth_id.parse()?; user_info.check_privs(&auth_id, &["system", "tasks"], PRIV_SYS_MODIFY, false)?; } diff --git a/src/api2/pull.rs b/src/api2/pull.rs index 1eb86ea3..e631920f 100644 --- a/src/api2/pull.rs +++ b/src/api2/pull.rs @@ -81,7 +81,7 @@ pub fn do_sync_job( let upid_str = WorkerTask::spawn( &worker_type, Some(job_id.clone()), - auth_id.clone(), + auth_id.to_string(), to_stdout, move |worker| async move { @@ -183,7 +183,7 @@ async fn pull ( let (client, src_repo, tgt_store) = get_pull_parameters(&store, &remote, &remote_store).await?; // fixme: set to_stdout to false? - let upid_str = WorkerTask::spawn("sync", Some(store.clone()), auth_id.clone(), true, move |worker| async move { + let upid_str = WorkerTask::spawn("sync", Some(store.clone()), auth_id.to_string(), true, move |worker| async move { worker.log(format!("sync datastore '{}' start", store)); diff --git a/src/api2/reader/mod.rs b/src/api2/reader/mod.rs index 821e83c4..fada952c 100644 --- a/src/api2/reader/mod.rs +++ b/src/api2/reader/mod.rs @@ -143,7 +143,7 @@ fn upgrade_to_backup_reader_protocol( let worker_id = format!("{}:{}/{}/{:08X}", store, backup_type, backup_id, backup_dir.backup_time()); - WorkerTask::spawn("reader", Some(worker_id), auth_id.clone(), true, move |worker| async move { + WorkerTask::spawn("reader", Some(worker_id), auth_id.to_string(), true, move |worker| async move { let _guard = _guard; let mut env = ReaderEnvironment::new( diff --git a/src/api2/tape/backup.rs b/src/api2/tape/backup.rs index 6b533820..fadbfa3d 100644 --- a/src/api2/tape/backup.rs +++ b/src/api2/tape/backup.rs @@ -195,7 +195,7 @@ pub fn do_tape_backup_job( let upid_str = WorkerTask::new_thread( &worker_type, Some(job_id.clone()), - auth_id.clone(), + auth_id.to_string(), to_stdout, move |worker| { job.start(&worker.upid().to_string())?; @@ -376,7 +376,7 @@ pub fn backup( let upid_str = WorkerTask::new_thread( "tape-backup", Some(job_id), - auth_id, + auth_id.to_string(), to_stdout, move |worker| { let _drive_lock = drive_lock; // keep lock guard diff --git a/src/api2/tape/drive.rs b/src/api2/tape/drive.rs index 58f49f43..10aa6842 100644 --- a/src/api2/tape/drive.rs +++ b/src/api2/tape/drive.rs @@ -87,7 +87,7 @@ where let (config, _digest) = pbs_config::drive::config()?; let lock_guard = lock_tape_device(&config, &drive)?; - let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?; + let auth_id = rpcenv.get_auth_id().unwrap(); let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI; WorkerTask::new_thread(worker_type, job_id, auth_id, to_stdout, move |worker| { diff --git a/src/api2/tape/restore.rs b/src/api2/tape/restore.rs index 4ab60e8f..7739d1a4 100644 --- a/src/api2/tape/restore.rs +++ b/src/api2/tape/restore.rs @@ -275,7 +275,7 @@ pub fn restore( let upid_str = WorkerTask::new_thread( "tape-restore", Some(taskid), - auth_id.clone(), + auth_id.to_string(), to_stdout, move |worker| { let _drive_lock = drive_lock; // keep lock guard diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs index 6734525b..518054bf 100644 --- a/src/bin/proxmox-backup-proxy.rs +++ b/src/bin/proxmox-backup-proxy.rs @@ -745,7 +745,7 @@ async fn schedule_task_log_rotate() { if let Err(err) = WorkerTask::new_thread( worker_type, None, - Authid::root_auth_id().clone(), + Authid::root_auth_id().to_string(), false, move |worker| { job.start(&worker.upid().to_string())?; diff --git a/src/server/gc_job.rs b/src/server/gc_job.rs index 665b9631..317f4a36 100644 --- a/src/server/gc_job.rs +++ b/src/server/gc_job.rs @@ -26,7 +26,7 @@ pub fn do_garbage_collection_job( let upid_str = WorkerTask::new_thread( &worker_type, Some(store.clone()), - auth_id.clone(), + auth_id.to_string(), to_stdout, move |worker| { job.start(&worker.upid().to_string())?; diff --git a/src/server/prune_job.rs b/src/server/prune_job.rs index f298a74c..8d971a1c 100644 --- a/src/server/prune_job.rs +++ b/src/server/prune_job.rs @@ -105,7 +105,7 @@ pub fn do_prune_job( let upid_str = WorkerTask::new_thread( &worker_type, Some(job.jobname().to_string()), - auth_id.clone(), + auth_id.to_string(), false, move |worker| { job.start(&worker.upid().to_string())?; diff --git a/src/server/verify_job.rs b/src/server/verify_job.rs index 7f6d73ff..6005b706 100644 --- a/src/server/verify_job.rs +++ b/src/server/verify_job.rs @@ -36,7 +36,7 @@ pub fn do_verification_job( let upid_str = WorkerTask::new_thread( &worker_type, Some(job_id.clone()), - auth_id.clone(), + auth_id.to_string(), to_stdout, move |worker| { job.start(&worker.upid().to_string())?; diff --git a/src/server/worker_task.rs b/src/server/worker_task.rs index a74b18e1..92ab50d7 100644 --- a/src/server/worker_task.rs +++ b/src/server/worker_task.rs @@ -18,7 +18,7 @@ use proxmox::tools::fs::{create_path, replace_file, CreateOptions}; use pbs_buildcfg; use pbs_tools::logrotate::{LogRotate, LogRotateFiles}; -use pbs_api_types::{Authid, UPID}; +use pbs_api_types::UPID; use pbs_config::{open_backup_lockfile, BackupLockGuard}; use proxmox_rest_server::{CommandoSocket, FileLogger, FileLogOptions}; @@ -589,7 +589,7 @@ struct WorkerTaskData { impl WorkerTask { - pub fn new(worker_type: &str, worker_id: Option, auth_id: Authid, to_stdout: bool) -> Result, Error> { + pub fn new(worker_type: &str, worker_id: Option, auth_id: String, to_stdout: bool) -> Result, Error> { let upid = UPID::new(worker_type, worker_id, auth_id)?; let task_id = upid.task_id; @@ -640,7 +640,7 @@ impl WorkerTask { pub fn spawn( worker_type: &str, worker_id: Option, - auth_id: Authid, + auth_id: String, to_stdout: bool, f: F, ) -> Result @@ -662,7 +662,7 @@ impl WorkerTask { pub fn new_thread( worker_type: &str, worker_id: Option, - auth_id: Authid, + auth_id: String, to_stdout: bool, f: F, ) -> Result -- 2.30.2