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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

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)

usage of helpers and logic in helpers:

- I can see other people's push jobs (where local_user/owner != auth_id)
-- I can't modify them or create such jobs (unless I am highly privileged)
-- I can execute them (even if I am not highly privileged!)

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

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)

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.

On September 12, 2024 4:32 pm, Christian Ebner wrote:
> This patch series implements the functionality to extend the current
> sync jobs in pull direction by an additional push direction, allowing
> to push contents of a local source datastore to a remote target.
> 
> The series implements this by using the REST API of the remote target
> for fetching, creating and/or deleting namespaces, groups and backups,
> and reuses the clients backup writer functionality to create snapshots
> by writing a manifeset on the remote target and sync the fixed index,
> dynamic index or blobs contained in the source manifest to the remote,
> preserving also encryption information.
> 
> Thanks to Fabian for further feedback to the previous version of the
> patches, especially regarding users and ACLs.
> 
> Most notable changes since version 2 of the patch series include:
> - Add checks and extend roles and privs to allow for restricting a local
>   users access to remote datastore operations. In order to perform a
>   full sync in push direction, including permissions for namespace
>   creation and deleting contents with remove vansished, a acl.cfg looks
>   like below:
>   ```
>   acl:1:/datastore/datastore:syncoperator@pbs:DatastoreAudit
>   acl:1:/remote:syncoperator@pbs:RemoteSyncOperator
>   acl:1:/remote/local/pushme:syncoperator@pbs:RemoteDatastoreModify,RemoteDatastorePrune,RemoteSyncPushOperator
>   ```
>   Based on further feedback, privs might get further grouped or an
>   additional role containing most of these can be created.
> - Drop patch introducing `no-timestamp-check` flag for backup client, as pointed
>   out by Fabian this is not needed, as only backups newer than the currently
>   last available will be pushed.
> - Fix read snapshots from source by using the correct namespace.
> - Rename PullParameters `owner` to more fitting `local_user`.
> - Fix typos in remote sync push operator comment.
> - Fix comments not matching the functionality for the cli implementations.
> 
> The patch series is structured as follows in this version:
> - patch 1 is a cleanup patch fixing typos in api documentation.
> - patches 2 to 7 are patches restructuring the current code so that
>   functionality of the current pull implementation can be reused for
>   the push implementation as well.
> - patch 8 extens the backup writers functionality to be able to push
>   snapshots to the target.
> - patches 9 to 11 are once again preparatory patches for shared
>   implementation of sync jobs in pull and push direction.
> - patches 12 to 14 define the required permission acls and roles.
> - patch 15 implements almost all of the logic required for the push,
>   including pushing of the datastore, namespace, groups and snapshots,
>   taking into account also filters and additional sync flags.
> - patch 16 extends the current sync job configuration by a new config
>   type `sync-push` allowing to configure sync jobs in push direction
>   while limiting possible misconfiguration errors.
> - patches 17 to 28 expose the new sync job direction via the API, CLI
>   and WebUI.
> - patches 29 to 33 finally are followup patches, changing the return
>   type for the backup group and namespace delete REST API endpoints
>   to return statistics on the deleted snapshots, groups and namespaces,
>   which are then used to include this information in the task log.
>   As this is an API breaking change, the patches are kept independent
>   from the other patches.
> 
> Link to issue on bugtracker:
> https://bugzilla.proxmox.com/show_bug.cgi?id=3044
> 
> Christian Ebner (33):
>   api: datastore: add missing whitespace in description
>   server: sync: move sync related stats to common module
>   server: sync: move reader trait to common sync module
>   server: sync: move source to common sync module
>   client: backup writer: bundle upload stats counters
>   client: backup writer: factor out merged chunk stream upload
>   client: backup writer: add chunk count and duration stats
>   client: backup writer: allow push uploading index and chunks
>   server: sync: move skip info/reason to common sync module
>   server: sync: make skip reason message more genenric
>   server: sync: factor out namespace depth check into sync module
>   config: acl: mention optional namespace acl path component
>   config: acl: allow namespace components for remote datastores
>   api types: define remote permissions and roles for push sync
>   fix #3044: server: implement push support for sync operations
>   config: jobs: add `sync-push` config type for push sync jobs
>   api: push: implement endpoint for sync in push direction
>   api: sync: move sync job invocation to server sync module
>   api: sync jobs: expose optional `sync-direction` parameter
>   api: sync: add permission checks for push sync jobs
>   bin: manager: add datastore push cli command
>   ui: group filter: allow to set namespace for local datastore
>   ui: sync edit: source group filters based on sync direction
>   ui: add view with separate grids for pull and push sync jobs
>   ui: sync job: adapt edit window to be used for pull and push
>   ui: sync: pass sync-direction to allow removing push jobs
>   ui: sync view: do not use data model proxy for store
>   ui: sync view: set sync direction when invoking run task via api
>   datastore: move `BackupGroupDeleteStats` to api types
>   api types: implement api type for `BackupGroupDeleteStats`
>   datastore: increment deleted group counter when removing group
>   api: datastore/namespace: return backup groups delete stats on remove
>   server: sync job: use delete stats provided by the api
> 
>  pbs-api-types/src/acl.rs             |  32 +
>  pbs-api-types/src/datastore.rs       |  64 ++
>  pbs-api-types/src/jobs.rs            |  52 ++
>  pbs-client/src/backup_writer.rs      | 228 +++++--
>  pbs-config/src/acl.rs                |   7 +-
>  pbs-config/src/sync.rs               |  11 +-
>  pbs-datastore/src/backup_info.rs     |  34 +-
>  pbs-datastore/src/datastore.rs       |  27 +-
>  src/api2/admin/datastore.rs          |  24 +-
>  src/api2/admin/namespace.rs          |  20 +-
>  src/api2/admin/sync.rs               |  45 +-
>  src/api2/config/datastore.rs         |  22 +-
>  src/api2/config/notifications/mod.rs |  15 +-
>  src/api2/config/sync.rs              |  84 ++-
>  src/api2/mod.rs                      |   2 +
>  src/api2/pull.rs                     | 108 ----
>  src/api2/push.rs                     | 182 ++++++
>  src/bin/proxmox-backup-manager.rs    | 216 +++++--
>  src/bin/proxmox-backup-proxy.rs      |  25 +-
>  src/server/mod.rs                    |   3 +
>  src/server/pull.rs                   | 658 ++------------------
>  src/server/push.rs                   | 883 +++++++++++++++++++++++++++
>  src/server/sync.rs                   | 700 +++++++++++++++++++++
>  www/Makefile                         |   1 +
>  www/config/SyncPullPushView.js       |  60 ++
>  www/config/SyncView.js               |  47 +-
>  www/datastore/DataStoreList.js       |   2 +-
>  www/datastore/Panel.js               |   2 +-
>  www/form/GroupFilter.js              |  18 +-
>  www/window/SyncJobEdit.js            |  45 +-
>  30 files changed, 2706 insertions(+), 911 deletions(-)
>  create mode 100644 src/api2/push.rs
>  create mode 100644 src/server/push.rs
>  create mode 100644 src/server/sync.rs
>  create mode 100644 www/config/SyncPullPushView.js
> 
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> 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