Re: [pve-devel] [PATCH v2 manager] pve6to7: add check for pool permissions

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

On 16.06.21 14:45, Fabian Grünbichler wrote:
> On June 16, 2021 2:16 pm, Lorenz Stechauner wrote:
>> the two checks make sure that:
>> * no user defined role 'PVEPoolUser' exists
>> * the user gets a hint for roles only containing Pool.Allocate and
>>     not Pool.Audit
>>
>> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
>> ---
>> changes to v1:
>> * rebased on master
>>
>>  PVE/CLI/pve6to7.pm | 25 +++++++++++++++++++++++++
>>  1 file changed, 25 insertions(+)
>>
>> diff --git a/PVE/CLI/pve6to7.pm b/PVE/CLI/pve6to7.pm
>> index 90f92a55..b391d006 100644
>> --- a/PVE/CLI/pve6to7.pm
>> +++ b/PVE/CLI/pve6to7.pm
>> @@ -9,6 +9,7 @@ use PVE::API2::LXC;
>>  use PVE::API2::Qemu;
>>  use PVE::API2::Certificates;
>>  
>> +use PVE::AccessControl;
>>  use PVE::Ceph::Tools;
>>  use PVE::Cluster;
>>  use PVE::Corosync;
>> @@ -693,6 +694,30 @@ sub check_misc {
>>  
>>      check_backup_retention_settings();
>>      check_cifs_credential_location();
>> +
>> +    log_info("Check custom roles");
>> +    my $usercfg = PVE::Cluster::cfs_read_file("user.cfg");
>> +    foreach my $role (sort keys %{$usercfg->{roles}}) {
>> +	if (PVE::AccessControl::role_is_special($role)) {
>> +	    next;
>> +	}
>> +
>> +	if ($role eq "PVEPoolUser") {
>> +	    # the user created a custom role named PVEPoolUser
>> +	    log_fail("Custom role '$role' has a restricted name - a built-in role 'PVEPoolUser' will be available with the upgrade");
>> +	} else {
>> +	    log_pass("Custom role '$role' has no restricted name");
>> +	}
>> +
>> +	my $perms = $usercfg->{roles}->{$role};
>> +	if ($perms->{'Pool.Allocate'} && $perms->{'Pool.Audit'}) {
>> +	    log_pass("Custom role '$role' contains updated pool permissions");
> 
> that does not work for PVE 6.x, where Pool.Audit is not yet a valid 
> privilege, so gets dropped on parsing user.cfg ;)
> 
> so either we add it as valid privilege (without using it for anything) 
> in a new stable-6 branch, or we switch to lower-level parsing/checks 
> here.. the file format is pretty simple, so the following should 
> probably work for the purposes of the check:

did not try, but in theory you could also just mock the relevant subs in the pve 6
case to add that as valid priv there for this script only?