From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 088CC1FF161 for ; Wed, 6 Nov 2024 16:11:10 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 4E4BE1157C; Wed, 6 Nov 2024 16:11:20 +0100 (CET) MIME-Version: 1.0 In-Reply-To: <20241031121519.434337-16-c.ebner@proxmox.com> References: <20241031121519.434337-1-c.ebner@proxmox.com> <20241031121519.434337-16-c.ebner@proxmox.com> From: Fabian =?utf-8?q?Gr=C3=BCnbichler?= To: Christian Ebner , pbs-devel@lists.proxmox.com Date: Wed, 06 Nov 2024 16:10:39 +0100 Message-ID: <173090583969.79072.15737271044931374423@yuna.proxmox.com> User-Agent: alot/0.10 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.047 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. 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. [push.rs, proxmox.com, datastore.read, mod.rs] Subject: Re: [pbs-devel] [PATCH v6 proxmox-backup 15/29] api: push: implement endpoint for sync in push direction 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: , Reply-To: Proxmox Backup Server development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" Quoting Christian Ebner (2024-10-31 13:15:05) > Expose the sync job in push direction via a dedicated API endpoint, > analogous to the pull direction. > > Signed-off-by: Christian Ebner > --- > changes since version 5: > - Avoid double deserialization for backup namespaces > - Drop TryFrom<&SyncJobConfig> for PushParameters impl, as constructing > them requires an api call to fetch the remote api version now > > src/api2/mod.rs | 2 + > src/api2/push.rs | 183 +++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 185 insertions(+) > create mode 100644 src/api2/push.rs > > diff --git a/src/api2/mod.rs b/src/api2/mod.rs > index a83e4c205..03596326b 100644 > --- a/src/api2/mod.rs > +++ b/src/api2/mod.rs > @@ -12,6 +12,7 @@ pub mod helpers; > pub mod node; > pub mod ping; > pub mod pull; > +pub mod push; > pub mod reader; > pub mod status; > pub mod tape; > @@ -29,6 +30,7 @@ const SUBDIRS: SubdirMap = &sorted!([ > ("nodes", &node::ROUTER), > ("ping", &ping::ROUTER), > ("pull", &pull::ROUTER), > + ("push", &push::ROUTER), > ("reader", &reader::ROUTER), > ("status", &status::ROUTER), > ("tape", &tape::ROUTER), > diff --git a/src/api2/push.rs b/src/api2/push.rs > new file mode 100644 > index 000000000..28f4417d1 > --- /dev/null > +++ b/src/api2/push.rs > @@ -0,0 +1,183 @@ > +use anyhow::{format_err, Error}; > +use futures::{future::FutureExt, select}; > +use tracing::info; > + > +use pbs_api_types::{ > + Authid, BackupNamespace, GroupFilter, RateLimitConfig, DATASTORE_SCHEMA, > + GROUP_FILTER_LIST_SCHEMA, NS_MAX_DEPTH_REDUCED_SCHEMA, PRIV_DATASTORE_READ, > + PRIV_REMOTE_DATASTORE_BACKUP, PRIV_REMOTE_DATASTORE_PRUNE, REMOTE_ID_SCHEMA, > + REMOVE_VANISHED_BACKUPS_SCHEMA, TRANSFER_LAST_SCHEMA, > +}; > +use proxmox_rest_server::WorkerTask; > +use proxmox_router::{Permission, Router, RpcEnvironment}; > +use proxmox_schema::api; > + > +use pbs_config::CachedUserInfo; > + > +use crate::server::push::{push_store, PushParameters}; > + > +/// Check if the provided user is allowed to read from the local source and act on the remote > +/// target for pushing content > +pub fn check_push_privs( not used anywhere except here, could be private? > + auth_id: &Authid, > + store: &str, > + namespace: &BackupNamespace, > + remote: &str, > + remote_store: &str, > + remote_ns: Option<&BackupNamespace>, since we don't actually need to support not setting the root namespace, the Option here can go away.. > + delete: bool, > +) -> Result<(), Error> { > + let user_info = CachedUserInfo::new()?; > + > + let target_acl_path = match remote_ns { > + Some(ns) => ns.remote_acl_path(remote, remote_store), > + None => vec!["remote", remote, remote_store], > + }; which makes this simpler > + > + // Check user is allowed to backup to remote/// > + user_info.check_privs( > + auth_id, > + &target_acl_path, > + PRIV_REMOTE_DATASTORE_BACKUP, > + false, > + )?; > + > + if delete { > + // Check user is allowed to prune remote datastore > + user_info.check_privs( > + auth_id, > + &target_acl_path, > + PRIV_REMOTE_DATASTORE_PRUNE, > + false, > + )?; > + } > + > + // Check user is allowed to read source datastore > + user_info.check_privs( > + auth_id, > + &namespace.acl_path(store), > + PRIV_DATASTORE_READ, isn't this too restrictive? should be PRIV_DATASTORE_BACKUP *or* READ? the push task will then filter the local namespaces/backup groups/.. by what the user is allowed to see.. > + false, > + )?; > + > + Ok(()) > +} > + > +#[api( > + input: { > + properties: { > + store: { > + schema: DATASTORE_SCHEMA, > + }, > + ns: { > + type: BackupNamespace, > + optional: true, > + }, > + remote: { > + schema: REMOTE_ID_SCHEMA, > + }, > + "remote-store": { > + schema: DATASTORE_SCHEMA, > + }, > + "remote-ns": { > + type: BackupNamespace, > + optional: true, > + }, > + "remove-vanished": { > + schema: REMOVE_VANISHED_BACKUPS_SCHEMA, > + optional: true, > + }, > + "max-depth": { > + schema: NS_MAX_DEPTH_REDUCED_SCHEMA, > + optional: true, > + }, > + "group-filter": { > + schema: GROUP_FILTER_LIST_SCHEMA, > + optional: true, > + }, > + limit: { > + type: RateLimitConfig, > + flatten: true, > + }, > + "transfer-last": { > + schema: TRANSFER_LAST_SCHEMA, > + optional: true, > + }, > + }, > + }, > + access: { > + description: r###"The user needs Remote.Backup privilege on '/remote/{remote}/{remote-store}' > +and needs to own the backup group. Datastore.Read is required on '/datastore/{store}'. > +The delete flag additionally requires the Remote.Prune privilege on '/remote/{remote}/{remote-store}'. this is partly wrong and/or weirdly phrased ;) maybe something like The user needs (at least) Remote.DatastoreBackup on '/remote/{remote}/{remote-store}[/{remote-ns}]', and either Datastore.Backup or Datastore.Read on '/datastore/{store}[/{ns}]'. The 'remove-vanished' parameter might require additional privileges. > +"###, > + permission: &Permission::Anybody, > + }, > +)] > +/// Push store to other repository > +#[allow(clippy::too_many_arguments)] > +async fn push( > + store: String, > + ns: Option, > + remote: String, > + remote_store: String, > + remote_ns: Option, > + remove_vanished: Option, > + max_depth: Option, > + group_filter: Option>, > + limit: RateLimitConfig, > + transfer_last: Option, > + rpcenv: &mut dyn RpcEnvironment, > +) -> Result { > + let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?; > + let delete = remove_vanished.unwrap_or(false); > + let ns = ns.unwrap_or_default(); this could also be done for remote_ns > + > + check_push_privs( > + &auth_id, > + &store, > + &ns, > + &remote, > + &remote_store, > + remote_ns.as_ref(), > + delete, > + )?; > + > + let push_params = PushParameters::new( > + &store, > + ns, > + &remote, > + &remote_store, > + remote_ns.unwrap_or_default(), since we unwrap it here anyway ;) > + auth_id.clone(), > + remove_vanished, > + max_depth, > + group_filter, > + limit, > + transfer_last, > + ) > + .await?; > + > + let upid_str = WorkerTask::spawn( > + "sync", > + Some(store.clone()), > + auth_id.to_string(), > + true, > + move |worker| async move { > + info!("push datastore '{store}' to '{remote}/{remote_store}'"); this is a bit redundant (and incomplete), the push output will contain this correctly extended with namespace information.. > + > + let push_future = push_store(push_params); > + (select! { > + success = push_future.fuse() => success, > + abort = worker.abort_future().map(|_| Err(format_err!("push aborted"))) => abort, > + })?; > + > + info!("push datastore '{store}' end"); same here > + > + Ok(()) > + }, > + )?; > + > + Ok(upid_str) > +} > + > +pub const ROUTER: Router = Router::new().post(&API_METHOD_PUSH); > -- > 2.39.5 > > > > _______________________________________________ > pbs-devel mailing list > pbs-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel > > _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel