Re: [pbs-devel] [PATCH proxmox-backup 2/5] api: add Sys.Modify on /system/disks as permission to endpoints handling removable datastores

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

On November 26, 2024 12:43 pm, Hannes Laimer wrote:
> Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>  src/api2/admin/datastore.rs  | 12 +++++++++---
>  src/api2/config/datastore.rs | 12 +++++++++---
>  2 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index 1c939bc20..cae7eb89c 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -45,7 +45,7 @@ use pbs_api_types::{
>      BACKUP_TYPE_SCHEMA, CATALOG_NAME, CLIENT_LOG_BLOB_NAME, DATASTORE_SCHEMA,
>      IGNORE_VERIFIED_BACKUPS_SCHEMA, MANIFEST_BLOB_NAME, MAX_NAMESPACE_DEPTH, NS_MAX_DEPTH_SCHEMA,
>      PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_MODIFY, PRIV_DATASTORE_PRUNE,
> -    PRIV_DATASTORE_READ, PRIV_DATASTORE_VERIFY, UPID, UPID_SCHEMA,
> +    PRIV_DATASTORE_READ, PRIV_DATASTORE_VERIFY, PRIV_SYS_MODIFY, UPID, UPID_SCHEMA,
>      VERIFICATION_OUTDATED_AFTER_SCHEMA,
>  };
>  use pbs_client::pxar::{create_tar, create_zip};
> @@ -2512,7 +2512,10 @@ pub fn do_mount_device(datastore: DataStoreConfig) -> Result<(), Error> {
>          schema: UPID_SCHEMA,
>      },
>      access: {
> -        permission: &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_AUDIT, false),
> +        permission: &Permission::And(&[
> +            &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_AUDIT, false),
> +            &Permission::Privilege(&["system", "disks"], PRIV_SYS_MODIFY, false)
> +        ]),

I am not 100% sure this part should require Sys.Modify.. somebody needs
to have set up the datastore already, just mounting seems benign in that
case?

>      },
>  )]
>  /// Mount removable datastore.
> @@ -2625,7 +2628,10 @@ fn do_unmount_device(
>          schema: UPID_SCHEMA,
>      },
>      access: {
> -        permission: &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_MODIFY, true),
> +        permission: &Permission::And(&[
> +            &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_MODIFY, true),
> +            &Permission::Privilege(&["system", "disks"], PRIV_SYS_MODIFY, false)
> +        ]),

same logic would apply here..

>      }
>  )]
>  /// Unmount a removable device that is associated with the datastore
> diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
> index 121222c40..359b676a5 100644
> --- a/src/api2/config/datastore.rs
> +++ b/src/api2/config/datastore.rs
> @@ -14,7 +14,7 @@ use proxmox_uuid::Uuid;
>  use pbs_api_types::{
>      Authid, DataStoreConfig, DataStoreConfigUpdater, DatastoreNotify, DatastoreTuning, KeepOptions,
>      MaintenanceMode, PruneJobConfig, PruneJobOptions, SyncDirection, DATASTORE_SCHEMA,
> -    PRIV_DATASTORE_ALLOCATE, PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_MODIFY,
> +    PRIV_DATASTORE_ALLOCATE, PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_MODIFY, PRIV_SYS_MODIFY,
>      PROXMOX_CONFIG_DIGEST_SCHEMA, UPID_SCHEMA,
>  };
>  use pbs_config::BackupLockGuard;
> @@ -173,7 +173,10 @@ pub(crate) fn do_create_datastore(
>          },
>      },
>      access: {
> -        permission: &Permission::Privilege(&["datastore"], PRIV_DATASTORE_ALLOCATE, false),
> +        permission: &Permission::And(&[
> +            &Permission::Privilege(&["datastore"], PRIV_DATASTORE_ALLOCATE, false),
> +            &Permission::Privilege(&["system", "disks"], PRIV_SYS_MODIFY, false)
> +        ]),

this now affects regular datastores as well, it should probably be
inside the API handler and conditionalized on backing_device being set?

>      },
>  )]
>  /// Create new datastore config.
> @@ -551,7 +554,10 @@ pub fn update_datastore(
>          },
>      },
>      access: {
> -        permission: &Permission::Privilege(&["datastore", "{name}"], PRIV_DATASTORE_ALLOCATE, false),
> +        permission: &Permission::And(&[
> +            &Permission::Privilege(&["datastore", "{name}"], PRIV_DATASTORE_ALLOCATE, false),
> +            &Permission::Privilege(&["system", "disks"], PRIV_SYS_MODIFY, false)
> +        ]),

and this is not needed at all, since path and backing_device are fixed
after creation?

>      },
>      returns: {
>          schema: UPID_SCHEMA,
> -- 
> 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