[pbs-devel] [PATCH v3 proxmox-backup 00/33] fix #3044: push datastore to remote target

[Fabian Grünbichler]

> Christian Ebner <c.ebner at proxmox.com> hat am 11.10.2024 09:12 CEST geschrieben:  
> Thanks for tackling the review for this patches, let me look into your 
> comments and come back to you with further questions when they arise 
> during reworking of the series.

I'll try to get around to the missing parts today!

> I think the main confusion here stems from different interpretation on 
> what the `local_user` should be (as in which users permissions to 
> check). I try to clarify inline:
> 
> On 10/10/24 16:48, Fabian Grünbichler wrote:
> > left some comments on individual patches (those that I got around to
> > anyway, which is roughly up to patch #20), the permissions are still not
> > quite right, but since those changes are spread over a few patches, I'll
> > leave the comment for that here in one place (existing pull priv checks
> > should remain as they are, the following *only* applies to push based
> > syncing, except maybe the first bit):
> > 
> > UI/UX issues:
> > 
> > - I can create a sync job without having DatastoreAudit, but then I
> >    don't see it afterwards (this affects pull and push)
> 
> This is a bit strange, but will see to fix it for both.

I guess we either need to check for Audit as well when creating/.. a sync job, or we should drop the Audit requirement when listing.. maybe a user with just Datastore.Backup should at least see the sync jobs with themselves as owner/local_user (provided they have access to the remote, of course!)? they could after all do the same thing as non-sync pull invocation as well..

> > usage of helpers and logic in helpers:
> > 
> > - I can see other people's push jobs (where local_user/owner != auth_id)
> 
> Local user is the one who's permissions decide which source 
> snapshots/groups/namespaces are visible. I did not intend that to be 
> used to define permissions for the job itself. The intention was to use 
> the authenticated user for that exclusively.
> 
> > -- I can't modify them or create such jobs (unless I am highly privileged)
> 
> As you can set the sync job's local user (defining which sources can be 
> read) this has to be highly privileged.

yes, this part is okay I'd say (although it does not map 1:1 to the pull check, so it might make sense to distangle those).

if I can either read the whole datastore, or change arbitrary ownership, then I can also set local_user. I think currently only the second part is implemented (since for pull-based syncing, that makes sense for the question "am I allowed to set the owner to somebody else).
 
> > -- I can execute them (even if I am not highly privileged!)
> 
> So that a highly privileged user can create a job for you, but you are 
> only allowed to execute it.

but for pull based syncing, the checks are the same for modification and execution. I don't think it makes much sense to allow less-privileged users to execute sync jobs that they would not be able to set up (among other things - the sync job might not have a schedule and not be intended to be run at that point in time!).

> > the check_sync_job_remote_datastore_backup_access helper is wrong (it
> > doesn't account for auth_id vs local_user/owner at all). also, it is not
> > called when modifying a sync job or creating one, just when executing it
> > manually, which is probably also wrong. it also has a logic bug (missing
> > "not" when preparing the remote ACL path).
> 
> Okay, I need to look closer at this again, but I guess there is once 
> again a difference in interpretation of what the local user is/should 
> be. So that needs clarification first.

see above :) IMHO, modifying a sync job and running it should use the same helpers/checks. the latter should disallow running a sync job with a different local_user, unless the the authenticated user can either read all groups anyway, or  change their ownership in arbitrary fashion, in which case the user could do that and run the equivalent sync job as themselves anyway.

> > privileges:
> > 
> > - for pull-syncing, creating/removing namespaces needs PRIV_DATASTORE_MODIFY
> > - for push-syncing, creating namespaces needs PRIV_REMOTE_DATASTORE_MODIFY
> > - for push-syncing, removing namespaces needs PRIV_REMOTE_DATASTORE_PRUNE(!)
> > - manual push requires PRIV_REMOTE_DATASTORE_MODIFY (instead of
> >    PRIV_REMOTE_DATASTORE_BACKUP)
> 
> Okay, will double check and adapt.
> 
>
> > related code style nit:
> > 
> > since job_user is required for pushing (in
> > `check_ns_remote_datastore_privs`), it might make sense to not allow
> > creation of PushParameters without it set, e.g. by changing the TryFrom
> > impl to convert from (SyncJobConfig, AuthId) instead of just the
> > config.. or by using a custom helper.
> > 
> 
> OK, have a look at this as well.
> 
> Thanks!