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

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

[Lorenz Stechauner]

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");
+	} elsif ($perms->{'Pool.Allocate'}) {
+	    log_warn("Custom role '$role' contains permission 'Pool.Allocate' - to ensure same behavior add 'Pool.Audit' to this role after the upgrade");
+	} else {
+	    log_pass("Custom role '$role' contains no permissions that need to be updated");
+	}
+    }
 }
 
 __PACKAGE__->register_method ({
-- 
2.20.1

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

[Fabian Grünbichler]

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:

read raw file
look for lines starting with 'role:'
split line on ':'
split_list third field
do checks like in this patch 
  (split third field is privilege list, second field is role name)

obviously, this might warn about some roles that otherwise fail parsing 
with the real parser (e.g., invalid name), but that isn't really a 
problem for the purpose that pve6to7 has ;)

> +	} elsif ($perms->{'Pool.Allocate'}) {
> +	    log_warn("Custom role '$role' contains permission 'Pool.Allocate' - to ensure same behavior add 'Pool.Audit' to this role after the upgrade");
> +	} else {
> +	    log_pass("Custom role '$role' contains no permissions that need to be updated");
> +	}
> +    }
>  }
>  
>  __PACKAGE__->register_method ({
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

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

[Thomas Lamprecht]

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?