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

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

Am 09/11/2022 um 13:05 schrieb Fabian Grünbichler:
> 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?)
> 
> 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)..

I'd rather keep it at .Modify IIUC and it will one allow to modify mappings,
IMO its worth to keep this aligned, and actually we could go for 

Sys.HW.Modify
Sys.HW.Use

For what is Hardware.Audit though? (the commit message really needs to contain
more info...)  is it for just seeing a HW mapping? as the underlying device
details on a specific node are already handled by Sys.Audit.

Because if its for HW mappings only I feel like it may not be required initially
we should be able to cover all by Hardware.Modify for managing them and and
Hardware.Use, for being allowed to use a specific one, but just listing could
wait for some actual use case.

Also, maybe call it HWMap.Modify and possibly make it a sub-group of a (new)
Cluster. priv group and /cluster/hw-map acl path.

Cluster.HWMap.Use
Cluster.HWMap.Modify

and /cluster/hw-id/{id}

IMO a bit more clear about what this actually covers, as a general /hardware
one sounds a bit ominous IMO for our users, and we want to have Cluster.Modify
et al. anyway someday for making cluster non-root create/join/editable.

(yes this isn't all that important and a bit close to bike shedding, but we got
to keep this pretty much forever (or have a PITA upgrade) so it's IMO worth
to spent a bit more time picking colors of the shed here ;)