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

[Hannes Laimer <h.laimer@proxmox.com>]

On 11/26/24 13:07, Fabian Grünbichler wrote:
> 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?
> 

not sure why git diff shows `update_datastore` this is for the delete
endpoint. But I'll chnage that to only check when it is actually
removable(as above)
>>       },
>>       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



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