Re: [pve-devel] [PATCH access-control v3 1/1] PVE/AccessControl: add Hardware.* privileges and /hardware/ paths

[Dominik Csapak <d.csapak@proxmox.com>]

On 11/9/22 13:05, Fabian Grünbichler wrote:
> On September 20, 2022 2:50 pm, Dominik Csapak wrote:
>> so that we can assign privileges on hardware level
>>
>> this will generate a new role (PVEHardwareAdmin)
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>>   src/PVE/AccessControl.pm  | 13 +++++++++++++
>>   src/PVE/RPCEnvironment.pm |  3 ++-
>>   2 files changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
>> index c32dcc3..9cbc376 100644
>> --- a/src/PVE/AccessControl.pm
>> +++ b/src/PVE/AccessControl.pm
>> @@ -1080,6 +1080,17 @@ my $privgroups = {
>>   	    'Pool.Audit',
>>   	],
>>       },
>> +    Hardware => {
>> +	root => [
>> +	    'Hardware.Configure', # create/edit mappings
>> +	],
>> +	admin => [
>> +	    'Hardware.Use',
>> +	],
>> +	audit => [
>> +	    'Hardware.Audit',
>> +	],
>> +    },
> 
> I guess the rationale here was that currently hardware is root only, so having
> 
> admin => Configure,
> user => Use,
> audit => Audit,
> 
> would mean the existing PVEAdmin roles would gain something that was previously
> root only?
> 
> note that the current patch still means that for the "Administrator" role
> anyway, since that gets *all* defined privileges.. (which might also be
> something worthy of calling out somewhere?)

yes the idea was that existing roles don't get that privilege, but
i did not really find a way to add the privilige and not give it to the 
administrator role

and yes putting it in the admin bucket would mean that pveadmin gets it too

> 
> it still might make sense to put Hardware.Use into the user category for
> consistency's sake? also not sure whether it would be worth it to re-think
> "Configure" (a bit more explicit) vs. "Modify" (consistent with existing
> schema)..

Modify is fine by me, i didn't choose it because we don't actually 
'modify' the hardware, but yes we modify the hardware configuration

putting the use in the user bracket has the side effect that
there would be a 'PVEHardwareAdmin' and 'PVEHardwareUser' role
with the same privileges, which i did find weird to have

this way we only get the PVEHardwareAdmin that can use/see the
devices

if there is a way to only have 'user' role without the admin one
please do tell ;)

> 
>>   };
>>   
>>   my $valid_privs = {};
>> @@ -1209,6 +1220,8 @@ sub check_path {
>>   	|/storage/[[:alnum:]\.\-\_]+
>>   	|/vms
>>   	|/vms/[1-9][0-9]{2,}
>> +	|/hardware
>> +	|/hardware/[[:alnum:]\.\-\_]+
>>       )$!xs;
>>   }
>>   
>> diff --git a/src/PVE/RPCEnvironment.pm b/src/PVE/RPCEnvironment.pm
>> index 0ee2346..bcf911b 100644
>> --- a/src/PVE/RPCEnvironment.pm
>> +++ b/src/PVE/RPCEnvironment.pm
>> @@ -187,10 +187,11 @@ sub compute_api_permission {
>>   	nodes => qr/Sys\.|Permissions\.Modify/,
>>   	sdn => qr/SDN\.|Permissions\.Modify/,
>>   	dc => qr/Sys\.Audit|SDN\./,
>> +	hardware => qr/Hardware\.|Permissiions\.Modify/,
> 
> typo "Permissiions"
> 
>>       };
>>       map { $res->{$_} = {} } keys %$priv_re_map;
>>   
>> -    my $required_paths = ['/', '/nodes', '/access/groups', '/vms', '/storage', '/sdn'];
>> +    my $required_paths = ['/', '/nodes', '/access/groups', '/vms', '/storage', '/sdn', '/hardware'];
>>   
>>       my $checked_paths = {};
>>       foreach my $path (@$required_paths, keys %{$usercfg->{acl}}) {
>> -- 
>> 2.30.2
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
>