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

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

This is missing a commit message explaining the rationale.

Am 26.11.24 um 13:07 schrieb Fabian Grünbichler:
>> @@ -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?

Mounting is always a bit of an involved operation as it can result in
IO hangs, just requiring audit on the store seems IMO rather to low of a
requirement. The Audit privs are not for things that alter the system state,
but rather for pure observation. Sys.Modify might not be ideal but IMO
definitively better than the status quo, "Datastore.Modify" might be fine too
though.

>>      },
>>  )]
>>  /// 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

here the status quo requires "Datastore.Modify", which is better, but if we
go for Sys.Modify above I'd not have any objection to use it also here.


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