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) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 4E4FD693E8 for ; Mon, 18 Jan 2021 09:36:01 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 39C6BFDC5 for ; Mon, 18 Jan 2021 09:35:31 +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) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 9D17AFDBA for ; Mon, 18 Jan 2021 09:35:30 +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 68880457CD for ; Mon, 18 Jan 2021 09:35:30 +0100 (CET) Date: Mon, 18 Jan 2021 09:35:21 +0100 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox Backup Server development discussion , Thomas Lamprecht References: <20210115104855.3992674-1-f.gruenbichler@proxmox.com> <20210115104855.3992674-3-f.gruenbichler@proxmox.com> In-Reply-To: MIME-Version: 1.0 User-Agent: astroid/0.15.0 (https://github.com/astroidmail/astroid) Message-Id: <1610958820.g99tz0jcme.astroid@nora.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.027 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 Subject: Re: [pbs-devel] [PATCH proxmox-backup 3/3] pull: only remove owned groups 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: Mon, 18 Jan 2021 08:36:01 -0000 On January 18, 2021 6:57 am, Thomas Lamprecht wrote: > On 15.01.21 11:48, Fabian Gr=C3=BCnbichler wrote: >> we also only create/add snapshots to owned groups when syncing, so >> removing groups with different ownership is a rather confusing >> side-effect.. >>=20 >> Signed-off-by: Fabian Gr=C3=BCnbichler >> --- >>=20 >> Notes: >> came up in the forum, the restricted behaviour is better for mixed u= sage as >> sync target and regular datastore, or sync target for multiple sourc= es with >> different owners.. >> =20 >> datastores just used as sync target for a single job should still be= have the >> same (they have a single owner), datastores used as sync target for = multiple >> jobs with the same owner should still not use remove_vanished.. we'd= need to >> keep track of the sync origin inside the group for that to work.. >>=20 >> src/client/pull.rs | 23 +++++++++++++++++------ >> 1 file changed, 17 insertions(+), 6 deletions(-) >>=20 >> diff --git a/src/client/pull.rs b/src/client/pull.rs >> index 15514374..33a6c0f1 100644 >> --- a/src/client/pull.rs >> +++ b/src/client/pull.rs >> @@ -590,11 +590,15 @@ pub async fn pull_store( >> =20 >> let mut errors =3D false; >> =20 >> - let mut new_groups =3D std::collections::HashSet::new(); >> + let mut remote_groups =3D std::collections::HashSet::new(); >> for item in list.iter() { >> - new_groups.insert(BackupGroup::new(&item.backup_type, &item.bac= kup_id)); >> + remote_groups.insert(BackupGroup::new(&item.backup_type, &item.= backup_id)); >> } >> =20 >> + let correct_owner =3D |owner: &Authid, auth_id: &Authid| -> bool { >> + owner =3D=3D auth_id || (owner.is_token() && &Authid::from(owne= r.user().clone()) =3D=3D auth_id) >> + }; >> + >> let mut progress =3D StoreProgress::new(list.len() as u64); >> =20 >> for (done, item) in list.into_iter().enumerate() { >> @@ -617,7 +621,7 @@ pub async fn pull_store( >> }; >> =20 >> // permission check >> - if auth_id !=3D owner { >> + if !correct_owner(&owner, &auth_id) { >=20 > this is now also changed to include token owned groups, or? As the `corre= ct_owner` closure > checks not only the replaced (negated) auth_id =3D=3D owner but also an e= xplicit token check? >=20 > (did not looked to much at code out of context, just FYI) yes. in practice it probably won't happen too often (i.e., switching=20 from token-owned sync job to corresponding-user-owned), but those are=20 the semantics we have for ownership checks when doing backups, so it=20 makes sense to also use them here IMHO. >> // only the owner is allowed to create additional snapshots >> worker.log(format!( >> "sync group {}/{} failed - owner check failed ({} !=3D = {})", >> @@ -645,9 +649,16 @@ pub async fn pull_store( >> =20 >> if delete { >> let result: Result<(), Error> =3D proxmox::try_block!({ >> - let local_groups =3D BackupInfo::list_backup_groups(&tgt_st= ore.base_path())?; >> - for local_group in local_groups { >> - if new_groups.contains(&local_group) { >> + let local_owned_groups: Vec =3D >> + BackupInfo::list_backup_groups(&tgt_store.base_path())? >> + .into_iter() >> + .filter(|group| match tgt_store.get_owner(&group) { >> + Ok(owner) =3D> correct_owner(&owner, &auth_id), >> + Err(_) =3D> false, >> + }) >> + .collect(); >> + for local_group in local_owned_groups { >> + if remote_groups.contains(&local_group) { >> continue; >> } >> worker.log(format!( >>=20 >=20 >=20 >=20 =