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

[Christian Ebner <c.ebner@proxmox.com>]

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 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.

> 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.

> -- 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.

> 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.
> 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!



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel