[pve-devel] [PATCH v4 access-control++ 00/18] SuperUser privilege

[pve-devel] [PATCH v4 access-control++ 00/18] SuperUser privilege

[Oguz Bektas]

big thanks to Fabian G. for the earlier reviews :)

v3 was not reviewed but i thought i should rebase it to make it easier.
i also noticed some things that weren't addressed or were
incorrect, so those are hopefully fixed now.

please note that the privilege columns of the role selector in widget-toolkit
is broken (reverting commit 49275c6726e7bf40f6d79e7b62eb4ad490a75119 fixes
that, the custom renderer broke the view but i wasn't able to come up with a
fix besides that -- extjs is pretty weird)

v3->v4
* rebased to latest commits
* fix incorrect check for login prompt in pve-manager for termproxy and friends
* slightly changed warning message when checking propagate
* show warning to user via raise_perm_exc() when changing passwords
* mark shortcut in parse_backup_hints


v2->v3:
* fixed some incorrect checks in qemu-server
* further restrict changing passwords and tfa settings for superusers
* gui fix for lxc features
* slight improvements to docs, added some notes

see the separate patches for further details :)

intentionally left out ceph and cluster-related endpoints for checking SU
privileges:

* addnode
* copy
* create
* delnode
* index
* join

* destroyosd
* createosd

and some other endpoints:
* register_account
* deactivate_account

i left these endpoints alone since we have plans to introduce separate
privileges for cluster-related actions in the future.



 access-control: Oguz Bektas (5):
  add "SuperAdministrator" role with the new "SuperUser" privilege
  RPC env: add SuperUser API permission for GUI capabilities
  api: acl: only allow granting SU privilege if user already has it
  api: roles: only allow modifying roles to add/remove SU if user has SU
    themselves
  api: allow superusers to edit tfa and password settings

 src/PVE/API2/ACL.pm           | 16 ++++++++++++++++
 src/PVE/API2/AccessControl.pm | 24 ++++++++++++++----------
 src/PVE/API2/Role.pm          | 21 +++++++++++++++++++++
 src/PVE/API2/TFA.pm           | 16 +++++++++++++++-
 src/PVE/AccessControl.pm      |  9 ++++++---
 src/PVE/RPCEnvironment.pm     | 29 ++++++++++++++++++++++-------
 6 files changed, 94 insertions(+), 21 deletions(-)

 qemu-server: Oguz Bektas (4):
  api: allow SU privileged users to edit root-only options for VM
    configs
  migration tests: mock $rpcenv->check subroutine
  api: allow superusers to use 'skiplock' option
  parse_backup_hints: add comment for root shortcut and fix typos

 PVE/API2/Qemu.pm             | 133 +++++++++++++++++++++--------------
 PVE/QemuServer.pm            |   6 +-
 test/MigrationTest/QmMock.pm |   5 ++
 3 files changed, 87 insertions(+), 57 deletions(-)

 manager: Oguz Bektas (6):
  api: backup: allow SUs to use 'tmpdir', 'dumpdir' and 'script' options
  api: vzdump: allow SUs to use 'bwlimit' and 'ionice' parameters
  api: always drop to login prompt for non-root users on terminal proxy
    calls
  ui: include "SuperUser" in privilege selector
  ui: lxc features: check for SU instead of 'root@pam'
  ui: adapt sensible 'root@pam' checks to SU

 PVE/API2/Backup.pm                      | 11 +++++++----
 PVE/API2/Nodes.pm                       | 11 ++++++++---
 PVE/API2/VZDump.pm                      |  8 +++++---
 www/manager6/form/PrivilegesSelector.js |  2 +-
 www/manager6/lxc/Options.js             |  8 ++++++--
 www/manager6/lxc/Resources.js           |  6 +++---
 www/manager6/window/Migrate.js          |  4 ++--
 7 files changed, 32 insertions(+), 18 deletions(-)

 container: Oguz Bektas (1):
  fix #2582: api: add checks for 'SuperUser' privilege for root-only
    options

 src/PVE/API2/LXC.pm        | 19 +++++++++----------
 src/PVE/API2/LXC/Config.pm |  2 +-
 src/PVE/API2/LXC/Status.pm | 12 ++++++++----
 src/PVE/LXC.pm             | 21 ++++++++++++---------
 src/PVE/LXC/Create.pm      |  2 +-
 5 files changed, 31 insertions(+), 25 deletions(-)

 storage: Oguz Bektas (1):
  check_volume_access: allow superusers to pass arbitrary fs paths

 PVE/Storage.pm | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

 docs: Oguz Bektas (1):
  pveum: add SU privilege and SA role

 pveum.adoc | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

-- 
2.30.2

[pve-devel] [PATCH v4 access-control 01/18] add "SuperAdministrator" role with the new "SuperUser" privilege

[Oguz Bektas]

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
 src/PVE/AccessControl.pm  | 9 ++++++---
 src/PVE/RPCEnvironment.pm | 2 +-
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
index d0dbabc..aeda951 100644
--- a/src/PVE/AccessControl.pm
+++ b/src/PVE/AccessControl.pm
@@ -1012,6 +1012,7 @@ my $privgroups = {
     },
     Sys => {
 	root => [
+	    'SuperUser',
 	    'Sys.PowerMgmt',
 	    'Sys.Modify', # edit/change node settings
 	],
@@ -1078,7 +1079,8 @@ my $valid_privs = {};
 
 my $special_roles = {
     'NoAccess' => {}, # no privileges
-    'Administrator' => $valid_privs, # all privileges
+    'Administrator' => {}, # populated with all privs except superuser
+    'SuperAdministrator' => $valid_privs, # can do everything
 };
 
 sub create_roles {
@@ -1105,6 +1107,7 @@ sub create_roles {
     }
 
     $special_roles->{"PVETemplateUser"} = { 'VM.Clone' => 1, 'VM.Audit' => 1 };
+    $special_roles->{"Administrator"} =  { map { $_ => 1 } grep { $_ ne 'SuperUser'  } keys %$valid_privs };
 };
 
 create_roles();
@@ -1585,7 +1588,7 @@ sub write_user_config {
 
 	$collect_rolelist_members->($d->{'groups'}, $rolelist_members, '@');
 
-	# no need to save 'root@pam', it is always 'Administrator'
+	# no need to save 'root@pam', it is always 'SuperAdministrator'
 	$collect_rolelist_members->($d->{'users'}, $rolelist_members, '', 'root@pam');
 
 	$collect_rolelist_members->($d->{'tokens'}, $rolelist_members, '');
@@ -1639,7 +1642,7 @@ sub roles {
     # corresponding user has.
     # Use $rpcenv->permission() for any actual permission checks!
 
-    return 'Administrator' if $user eq 'root@pam'; # root can do anything
+    return 'SuperAdministrator' if $user eq 'root@pam'; # root can do anything
 
     if (pve_verify_tokenid($user, 1)) {
 	my $tokenid = $user;
diff --git a/src/PVE/RPCEnvironment.pm b/src/PVE/RPCEnvironment.pm
index ed5625e..f5d2219 100644
--- a/src/PVE/RPCEnvironment.pm
+++ b/src/PVE/RPCEnvironment.pm
@@ -101,7 +101,7 @@ sub permissions {
 
     if ($user eq 'root@pam') { # root can do anything
 	my $cfg = $self->{user_cfg};
-	return { map { $_ => 1 } keys %{$cfg->{roles}->{'Administrator'}} };
+	return { map { $_ => 1 } keys %{$cfg->{roles}->{'SuperAdministrator'}} };
     }
 
     if (PVE::AccessControl::pve_verify_tokenid($user, 1)) {
-- 
2.30.2

[pve-devel] [PATCH v4 access-control 02/18] RPC env: add SuperUser API permission for GUI capabilities

[Oguz Bektas]

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
 src/PVE/RPCEnvironment.pm | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/PVE/RPCEnvironment.pm b/src/PVE/RPCEnvironment.pm
index f5d2219..4c55b25 100644
--- a/src/PVE/RPCEnvironment.pm
+++ b/src/PVE/RPCEnvironment.pm
@@ -139,12 +139,12 @@ sub compute_api_permission {
 
     my $res = {};
     my $priv_re_map = {
-	vms => qr/VM\.|Permissions\.Modify/,
-	access => qr/(User|Group)\.|Permissions\.Modify/,
-	storage => qr/Datastore\.|Permissions\.Modify/,
-	nodes => qr/Sys\.|Permissions\.Modify/,
-	sdn => qr/SDN\.|Permissions\.Modify/,
-	dc => qr/Sys\.Audit|SDN\./,
+	vms => qr/VM\.|Permissions\.Modify|SuperUser/,
+	access => qr/(User|Group)\.|Permissions\.Modify|SuperUser/,
+	storage => qr/Datastore\.|Permissions\.Modify|SuperUser/,
+	nodes => qr/Sys\.|Permissions\.Modify|SuperUser/,
+	sdn => qr/SDN\.|Permissions\.Modify|SuperUser/,
+	dc => qr/Sys\.Audit|SDN\.|SuperUser/,
     };
     map { $res->{$_} = {} } keys %$priv_re_map;
 
-- 
2.30.2

[pve-devel] [PATCH v4 access-control 03/18] api: acl: only allow granting SU privilege if user already has it

[Oguz Bektas]

also check for 'propagate' bit on the target path to verify if the
user can grant SU privileges on there.

Co-authored-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
 src/PVE/API2/ACL.pm | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/src/PVE/API2/ACL.pm b/src/PVE/API2/ACL.pm
index 857c672..f8d4914 100644
--- a/src/PVE/API2/ACL.pm
+++ b/src/PVE/API2/ACL.pm
@@ -134,6 +134,10 @@ __PACKAGE__->register_method ({
     code => sub {
 	my ($param) = @_;
 
+	my $rpcenv = PVE::RPCEnvironment::get();
+	my $authuser = $rpcenv->get_user();
+	my $is_superuser = $rpcenv->check($authuser, $param->{path}, ['SuperUser'], 1);
+
 	if (!($param->{users} || $param->{groups} || $param->{tokens})) {
 	    raise_param_exc({ map { $_ => "either 'users', 'groups' or 'tokens' is required." } qw(users groups tokens) });
 	}
@@ -160,6 +164,18 @@ __PACKAGE__->register_method ({
 		    die "role '$role' does not exist\n"
 			if !$cfg->{roles}->{$role};
 
+		    my $role_privs = $cfg->{roles}->{$role};
+		    my $role_contains_superuser = grep { $_ eq 'SuperUser' } keys %$role_privs;
+		    if ($role_contains_superuser) {
+			die "only superusers can grant/remove this role!\n"
+			    if !$is_superuser;
+
+			my $user_perms = $rpcenv->permissions($authuser, $param->{path});
+			my $has_propagate = $user_perms->{SuperUser}; # check if user has SU with propagate bit on the target path
+			die "cannot grant SU on '$param->{path}' without having 'propagate' bit!\n"
+			    if !$has_propagate;
+		    }
+
 		    foreach my $group (split_list($param->{groups})) {
 
 			die "group '$group' does not exist\n"
-- 
2.30.2

Re: [pve-devel] [PATCH v4 access-control 03/18] api: acl: only allow granting SU privilege if user already has it

[Fabian Grünbichler]

On June 2, 2022 9:24 am, Oguz Bektas wrote:
> also check for 'propagate' bit on the target path to verify if the
> user can grant SU privileges on there.
> 
> Co-authored-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
>  src/PVE/API2/ACL.pm | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/src/PVE/API2/ACL.pm b/src/PVE/API2/ACL.pm
> index 857c672..f8d4914 100644
> --- a/src/PVE/API2/ACL.pm
> +++ b/src/PVE/API2/ACL.pm
> @@ -134,6 +134,10 @@ __PACKAGE__->register_method ({
>      code => sub {
>  	my ($param) = @_;
>  
> +	my $rpcenv = PVE::RPCEnvironment::get();
> +	my $authuser = $rpcenv->get_user();
> +	my $is_superuser = $rpcenv->check($authuser, $param->{path}, ['SuperUser'], 1);
> +
>  	if (!($param->{users} || $param->{groups} || $param->{tokens})) {
>  	    raise_param_exc({ map { $_ => "either 'users', 'groups' or 'tokens' is required." } qw(users groups tokens) });
>  	}
> @@ -160,6 +164,18 @@ __PACKAGE__->register_method ({
>  		    die "role '$role' does not exist\n"
>  			if !$cfg->{roles}->{$role};
>  
> +		    my $role_privs = $cfg->{roles}->{$role};
> +		    my $role_contains_superuser = grep { $_ eq 'SuperUser' } keys %$role_privs;
> +		    if ($role_contains_superuser) {
> +			die "only superusers can grant/remove this role!\n"
> +			    if !$is_superuser;
> +
> +			my $user_perms = $rpcenv->permissions($authuser, $param->{path});
> +			my $has_propagate = $user_perms->{SuperUser}; # check if user has SU with propagate bit on the target path
> +			die "cannot grant SU on '$param->{path}' without having 'propagate' bit!\n"
> +			    if !$has_propagate;

this only needs to be checked if the updated ACL also has the propagate 
bit set. also, it doesn't need to be checked for every role that is 
referenced by the ACL (it's not dependent on the role at all, but just 
on the combination of authid and path and propagate parameter).

> +		    }
> +
>  		    foreach my $group (split_list($param->{groups})) {
>  
>  			die "group '$group' does not exist\n"
> -- 
> 2.30.2
> 
> 

[pve-devel] [PATCH v4 access-control 04/18] api: roles: only allow modifying roles to add/remove SU if user has SU themselves

[Oguz Bektas]

Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
 src/PVE/API2/Role.pm | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/src/PVE/API2/Role.pm b/src/PVE/API2/Role.pm
index 70a92b6..4a09ad6 100644
--- a/src/PVE/API2/Role.pm
+++ b/src/PVE/API2/Role.pm
@@ -4,6 +4,7 @@ use strict;
 use warnings;
 use PVE::Cluster qw (cfs_read_file cfs_write_file);
 use PVE::AccessControl;
+use PVE::Tools qw (split_list);
 use PVE::JSONSchema qw(get_standard_option register_standard_option);
 
 use PVE::SafeSyslog;
@@ -90,11 +91,19 @@ __PACKAGE__->register_method ({
 
 		my $usercfg = cfs_read_file("user.cfg");
 
+		my $rpcenv = PVE::RPCEnvironment::get();
+		my $authuser = $rpcenv->get_user();
+		my $is_superuser = $rpcenv->check($authuser, "/", ['SuperUser'], 1);
+
 		my $role = $param->{roleid};
 
 		die "role '$role' already exists\n"
 		    if $usercfg->{roles}->{$role};
 
+		my $contains_superuser = grep { $_ eq 'SuperUser' } split_list($param->{privs});
+		die "only superusers can add this privilege!\n"
+		    if $contains_superuser && !$is_superuser;
+
 		$usercfg->{roles}->{$role} = {};
 
 		PVE::AccessControl::add_role_privs($role, $usercfg, $param->{privs});
@@ -136,9 +145,21 @@ __PACKAGE__->register_method ({
 
 		my $usercfg = cfs_read_file("user.cfg");
 
+		my $rpcenv = PVE::RPCEnvironment::get();
+		my $authuser = $rpcenv->get_user();
+		my $is_superuser = $rpcenv->check($authuser, "/", ['SuperUser'], 1);
+
 		die "role '$role' does not exist\n"
 		    if !$usercfg->{roles}->{$role};
 
+		my @old_role_privs = keys(%{$usercfg->{roles}->{$role}});
+
+		my $contained_superuser = grep { $_ eq 'SuperUser' } @old_role_privs;
+		my $contains_superuser = grep { $_ eq 'SuperUser' } split_list($param->{privs});
+
+		die "only superusers can add/remove this privilege!\n"
+		    if ($contains_superuser || $contained_superuser) && !$is_superuser;
+
 		$usercfg->{roles}->{$role} = {} if !$param->{append};
 
 		PVE::AccessControl::add_role_privs($role, $usercfg, $param->{privs});
-- 
2.30.2

[pve-devel] [PATCH v4 access-control 05/18] api: allow superusers to edit tfa and password settings

[Oguz Bektas]

- prevent non-SU to change SU passwords
- warning messages on raise_perm_exc()
- log who did the password change
- has_superuser_anywhere helper

Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
 src/PVE/API2/AccessControl.pm | 24 ++++++++++++++----------
 src/PVE/API2/TFA.pm           | 16 +++++++++++++++-
 src/PVE/RPCEnvironment.pm     | 15 +++++++++++++++
 3 files changed, 44 insertions(+), 11 deletions(-)

diff --git a/src/PVE/API2/AccessControl.pm b/src/PVE/API2/AccessControl.pm
index 5d78c6f..2a584ab 100644
--- a/src/PVE/API2/AccessControl.pm
+++ b/src/PVE/API2/AccessControl.pm
@@ -378,23 +378,27 @@ __PACKAGE__->register_method ({
 
 	$rpcenv->check_user_exist($userid);
 
+	my $is_superuser = $rpcenv->check($authuser, "/access", ['SuperUser'], 1);
+
 	if ($authuser eq 'root@pam') {
 	    # OK - root can change anything
-	} else {
-	    if ($authuser eq $userid) {
-		$rpcenv->check_user_enabled($userid);
-		# OK - each user can change its own password
-	    } else {
-		# only root may change root password
-		raise_perm_exc() if $userid eq 'root@pam';
-		# do not allow to change system user passwords
-		raise_perm_exc() if $realm eq 'pam';
+	} elsif ($authuser eq $userid) {
+	    $rpcenv->check_user_enabled($userid);
+	    # OK - each user can change its own password
+	} else { # changing someone else's password
+	    raise_perm_exc("only root\@pam may change their password!\n") if $userid eq 'root@pam';
+	    raise_perm_exc("changing system user passwords is not allowed!\n") if $realm eq 'pam';
+
+	    if (!$is_superuser) {
+		# check if the target user has SU privileges
+		raise_perm_exc("only superusers can change another superuser's password!\n")
+		    if $rpcenv->has_superuser_anywhere($userid);
 	    }
 	}
 
 	PVE::AccessControl::domain_set_password($realm, $ruid, $param->{password});
 
-	PVE::Cluster::log_msg('info', 'root@pam', "changed password for user '$userid'");
+	PVE::Cluster::log_msg('info', "$authuser", "changed password for user '$userid'");
 
 	return undef;
     }});
diff --git a/src/PVE/API2/TFA.pm b/src/PVE/API2/TFA.pm
index bee4dee..c1cdd5e 100644
--- a/src/PVE/API2/TFA.pm
+++ b/src/PVE/API2/TFA.pm
@@ -96,22 +96,36 @@ my $TFA_UPDATE_INFO_SCHEMA = {
 };
 
 # Only root may modify root, regular users need to specify their password.
+# Only users with SU on /access may modify other users with SU anywhere
 #
 # Returns the userid returned from `verify_username`.
 # Or ($userid, $realm) in list context.
 my sub root_permission_check : prototype($$$$) {
     my ($rpcenv, $authuser, $userid, $password) = @_;
 
+    # authuser = the user making the change
+    # userid = the user to be changed
+
+    if ($userid eq 'root@pam') {
+	raise_perm_exc("only root\@pam may edit themselves!\n")
+	    if $authuser ne 'root@pam';
+    }
+
     ($userid, undef, my $realm) = PVE::AccessControl::verify_username($userid);
     $rpcenv->check_user_exist($userid);
 
-    raise_perm_exc() if $userid eq 'root@pam' && $authuser ne 'root@pam';
+    my $is_superuser = $rpcenv->check($authuser, "/access", ['SuperUser'], 1);
 
     # Regular users need to confirm their password to change TFA settings.
     if ($authuser ne 'root@pam') {
 	raise_param_exc({ 'password' => 'password is required to modify TFA data' })
 	    if !defined($password);
 
+	if (!$is_superuser && $authuser ne $userid) {
+	    raise_perm_exc("only superusers can change another superuser's TFA settings!\n")
+		if $rpcenv->has_superuser_anywhere($userid);
+	}
+
 	($authuser, my $auth_username, my $auth_realm) =
 	    PVE::AccessControl::verify_username($authuser);
 
diff --git a/src/PVE/RPCEnvironment.pm b/src/PVE/RPCEnvironment.pm
index 4c55b25..5bcb4ba 100644
--- a/src/PVE/RPCEnvironment.pm
+++ b/src/PVE/RPCEnvironment.pm
@@ -479,6 +479,21 @@ sub check_api2_permissions {
     raise_perm_exc();
 }
 
+sub has_superuser_anywhere {
+    my ($self, $username) = @_;
+
+    return 1 if $username eq 'root@pam';
+
+    # get all ACL paths from config and check for SU privilege
+    my $acls = $self->{user_cfg}->{acl};
+    my @acl_paths = keys(%$acls);
+    @acl_paths = ['/'] if !@acl_paths;
+    foreach my $path (@acl_paths) {
+	return 1 if $self->check($username, $path, ['SuperUser'], 1);
+    }
+    return 0;
+}
+
 sub log_cluster_msg {
     my ($self, $pri, $user, $msg) = @_;
 
-- 
2.30.2

Re: [pve-devel] [PATCH v4 access-control 05/18] api: allow superusers to edit tfa and password settings

[Fabian Grünbichler]

On June 2, 2022 9:24 am, Oguz Bektas wrote:
> - prevent non-SU to change SU passwords
> - warning messages on raise_perm_exc()
> - log who did the password change
> - has_superuser_anywhere helper
> 
> Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
>  src/PVE/API2/AccessControl.pm | 24 ++++++++++++++----------
>  src/PVE/API2/TFA.pm           | 16 +++++++++++++++-
>  src/PVE/RPCEnvironment.pm     | 15 +++++++++++++++
>  3 files changed, 44 insertions(+), 11 deletions(-)
> 
> diff --git a/src/PVE/API2/AccessControl.pm b/src/PVE/API2/AccessControl.pm
> index 5d78c6f..2a584ab 100644
> --- a/src/PVE/API2/AccessControl.pm
> +++ b/src/PVE/API2/AccessControl.pm
> @@ -378,23 +378,27 @@ __PACKAGE__->register_method ({
>  
>  	$rpcenv->check_user_exist($userid);
>  
> +	my $is_superuser = $rpcenv->check($authuser, "/access", ['SuperUser'], 1);
> +
>  	if ($authuser eq 'root@pam') {
>  	    # OK - root can change anything
> -	} else {
> -	    if ($authuser eq $userid) {
> -		$rpcenv->check_user_enabled($userid);
> -		# OK - each user can change its own password
> -	    } else {
> -		# only root may change root password
> -		raise_perm_exc() if $userid eq 'root@pam';
> -		# do not allow to change system user passwords
> -		raise_perm_exc() if $realm eq 'pam';
> +	} elsif ($authuser eq $userid) {
> +	    $rpcenv->check_user_enabled($userid);
> +	    # OK - each user can change its own password
> +	} else { # changing someone else's password
> +	    raise_perm_exc("only root\@pam may change their password!\n") if $userid eq 'root@pam';
> +	    raise_perm_exc("changing system user passwords is not allowed!\n") if $realm eq 'pam';
> +
> +	    if (!$is_superuser) {
> +		# check if the target user has SU privileges
> +		raise_perm_exc("only superusers can change another superuser's password!\n")
> +		    if $rpcenv->has_superuser_anywhere($userid);
>  	    }
>  	}
>  
>  	PVE::AccessControl::domain_set_password($realm, $ruid, $param->{password});
>  
> -	PVE::Cluster::log_msg('info', 'root@pam', "changed password for user '$userid'");
> +	PVE::Cluster::log_msg('info', "$authuser", "changed password for user '$userid'");
>  
>  	return undef;
>      }});
> diff --git a/src/PVE/API2/TFA.pm b/src/PVE/API2/TFA.pm
> index bee4dee..c1cdd5e 100644
> --- a/src/PVE/API2/TFA.pm
> +++ b/src/PVE/API2/TFA.pm
> @@ -96,22 +96,36 @@ my $TFA_UPDATE_INFO_SCHEMA = {
>  };
>  
>  # Only root may modify root, regular users need to specify their password.
> +# Only users with SU on /access may modify other users with SU anywhere
>  #
>  # Returns the userid returned from `verify_username`.
>  # Or ($userid, $realm) in list context.
>  my sub root_permission_check : prototype($$$$) {
>      my ($rpcenv, $authuser, $userid, $password) = @_;
>  
> +    # authuser = the user making the change
> +    # userid = the user to be changed
> +
> +    if ($userid eq 'root@pam') {
> +	raise_perm_exc("only root\@pam may edit themselves!\n")
> +	    if $authuser ne 'root@pam';
> +    }

if ($foo && $bar) {
  baz();
}

or

baz() if $foo && $bar;

also, the escaping can be avoided if you just quote with '' instead of 
"" in the message.

> +
>      ($userid, undef, my $realm) = PVE::AccessControl::verify_username($userid);
>      $rpcenv->check_user_exist($userid);
>  
> -    raise_perm_exc() if $userid eq 'root@pam' && $authuser ne 'root@pam';
> +    my $is_superuser = $rpcenv->check($authuser, "/access", ['SuperUser'], 1);

only used once, inside the next if, please move it there (I told you 
this already?)

>  
>      # Regular users need to confirm their password to change TFA settings.
>      if ($authuser ne 'root@pam') {
>  	raise_param_exc({ 'password' => 'password is required to modify TFA data' })
>  	    if !defined($password);
>  
> +	if (!$is_superuser && $authuser ne $userid) {
> +	    raise_perm_exc("only superusers can change another superuser's TFA settings!\n")
> +		if $rpcenv->has_superuser_anywhere($userid);

could actually be ordered by order of expensiveness here, but it's not 
too important..

if ($authuser ne $userid) {
  my $is_superuser = ...;
  raise_perm_exc..
    if !is_superuser && $rpcenv->has_superuser_anywhere(..);
}

or

raise_perm_exc
  if user_mismatch_check
    && isnt_superuser_check
    && has_superuser_check;

that way each check is only actually done if needed, after the previous 
check made us go down further. the current way is just hard to parse 
with no benefit at all.

> +	}
> +
>  	($authuser, my $auth_username, my $auth_realm) =
>  	    PVE::AccessControl::verify_username($authuser);

the whole code here is rather similar to the check in change_password, 
have you thought about unifying them?

>  
> diff --git a/src/PVE/RPCEnvironment.pm b/src/PVE/RPCEnvironment.pm
> index 4c55b25..5bcb4ba 100644
> --- a/src/PVE/RPCEnvironment.pm
> +++ b/src/PVE/RPCEnvironment.pm
> @@ -479,6 +479,21 @@ sub check_api2_permissions {
>      raise_perm_exc();
>  }
>  
> +sub has_superuser_anywhere {
> +    my ($self, $username) = @_;
> +
> +    return 1 if $username eq 'root@pam';
> +
> +    # get all ACL paths from config and check for SU privilege
> +    my $acls = $self->{user_cfg}->{acl};
> +    my @acl_paths = keys(%$acls);
> +    @acl_paths = ['/'] if !@acl_paths;
> +    foreach my $path (@acl_paths) {
> +	return 1 if $self->check($username, $path, ['SuperUser'], 1);
> +    }
> +    return 0;
> +}

this is basically a reduced $rpcenv->get_effective_permissions with 
early abort. some possible options:
- extend it (add a '$needle' parameter, early return 1 if any path 
  matches that check)
- just use it (and drop the early abort)
- factor out the "get all ACL paths" part for re-using

else we risk missing this helper here if we ever add another layer of 
indirection for ACL path resolution, like we currently have for pools..

> +
>  sub log_cluster_msg {
>      my ($self, $pri, $user, $msg) = @_;
>  
> -- 
> 2.30.2
> 
> 

[pve-devel] [PATCH v4 qemu-server 06/18] api: allow SU privileged users to edit root-only options for VM configs

[Oguz Bektas]

we now allow users with SU privilege to edit real device configurations
for VMs.

they still need the required privilege to edit the corresponding
configuration options (e.g. `VM.Config.HWType`), as well as the SU
privilege.

Co-authored-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
v3->v4:
* added fabian's tag there as well since he helped me a lot with this part


 PVE/API2/Qemu.pm | 62 ++++++++++++++++++++++++------------------------
 1 file changed, 31 insertions(+), 31 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 99b426e..2e75ab6 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -545,16 +545,17 @@ my $cloudinitoptions = {
 my $check_vm_create_serial_perm = sub {
     my ($rpcenv, $authuser, $vmid, $pool, $param) = @_;
 
+    # no need to check permissions for root@pam
     return 1 if $authuser eq 'root@pam';
 
+    my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
+
     foreach my $opt (keys %{$param}) {
 	next if $opt !~ m/^serial\d+$/;
 
-	if ($param->{$opt} eq 'socket') {
-	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.HWType']);
-	} else {
-	    die "only root can set '$opt' config for real devices\n";
-	}
+	$rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.HWType']);
+	die "only superusers can set '$opt' config for real devices\n"
+	    if $param->{$opt} ne 'socket' && !$is_superuser;
     }
 
     return 1;
@@ -563,16 +564,17 @@ my $check_vm_create_serial_perm = sub {
 my $check_vm_create_usb_perm = sub {
     my ($rpcenv, $authuser, $vmid, $pool, $param) = @_;
 
+    # no need to check permissions for root@pam
     return 1 if $authuser eq 'root@pam';
 
+    my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
+
     foreach my $opt (keys %{$param}) {
 	next if $opt !~ m/^usb\d+$/;
 
-	if ($param->{$opt} =~ m/spice/) {
-	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.HWType']);
-	} else {
-	    die "only root can set '$opt' config for real devices\n";
-	}
+	$rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.HWType']);
+	die "only superusers can set '$opt' config for real devices\n"
+	    if $param->{$opt} !~ m/spice/ && !$is_superuser;
     }
 
     return 1;
@@ -581,8 +583,11 @@ my $check_vm_create_usb_perm = sub {
 my $check_vm_modify_config_perm = sub {
     my ($rpcenv, $authuser, $vmid, $pool, $key_list) = @_;
 
+    # no need to check permissions for root@pam
     return 1 if $authuser eq 'root@pam';
 
+    my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
+
     foreach my $opt (@$key_list) {
 	# some checks (e.g., disk, serial port, usb) need to be done somewhere
 	# else, as there the permission can be value dependend
@@ -618,7 +623,8 @@ my $check_vm_modify_config_perm = sub {
 	} else {
 	    # catches hostpci\d+, args, lock, etc.
 	    # new options will be checked here
-	    die "only root can set '$opt' config\n";
+	    die "only superusers can set '$opt' config\n"
+		if !$is_superuser;
 	}
     }
 
@@ -1328,6 +1334,8 @@ my $update_vm_api  = sub {
 	push @paramarr, "-$key", $value;
     }
 
+    my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
+
     my $skiplock = extract_param($param, 'skiplock');
     raise_param_exc({ skiplock => "Only root may use this option." })
 	if $skiplock && $authuser ne 'root@pam';
@@ -1544,19 +1552,15 @@ my $update_vm_api  = sub {
 		    PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
 		    PVE::QemuConfig->write_config($vmid, $conf);
 		} elsif ($opt =~ m/^serial\d+$/) {
-		    if ($val eq 'socket') {
-			$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
-		    } elsif ($authuser ne 'root@pam') {
-			die "only root can delete '$opt' config for real devices\n";
-		    }
+		    $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
+		    die "only superusers can delete '$opt' config for real devices\n"
+			if $val ne 'socket' && !$is_superuser;
 		    PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
 		    PVE::QemuConfig->write_config($vmid, $conf);
 		} elsif ($opt =~ m/^usb\d+$/) {
-		    if ($val =~ m/spice/) {
-			$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
-		    } elsif ($authuser ne 'root@pam') {
-			die "only root can delete '$opt' config for real devices\n";
-		    }
+		    $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
+		    die "only superusers can delete '$opt' config for real devices\n"
+			if $val !~ m/spice/ && !$is_superuser;
 		    PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
 		    PVE::QemuConfig->write_config($vmid, $conf);
 		} else {
@@ -1606,18 +1610,14 @@ my $update_vm_api  = sub {
 			}
 		    }
 		} elsif ($opt =~ m/^serial\d+/) {
-		    if ((!defined($conf->{$opt}) || $conf->{$opt} eq 'socket') && $param->{$opt} eq 'socket') {
-			$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
-		    } elsif ($authuser ne 'root@pam') {
-			die "only root can modify '$opt' config for real devices\n";
-		    }
+		    $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']); # always check hw config
+		    die "only superusers can modify '$opt' config for real devices\n"
+			if !$is_superuser && ((defined($conf->{$opt}) && $conf->{$opt} ne 'socket') || $param->{$opt} ne 'socket');
 		    $conf->{pending}->{$opt} = $param->{$opt};
 		} elsif ($opt =~ m/^usb\d+/) {
-		    if ((!defined($conf->{$opt}) || $conf->{$opt} =~ m/spice/) && $param->{$opt} =~ m/spice/) {
-			$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
-		    } elsif ($authuser ne 'root@pam') {
-			die "only root can modify '$opt' config for real devices\n";
-		    }
+		    $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
+		    die "only superusers can modify '$opt' config for real devices\n"
+			if !$is_superuser && ((defined($conf->{$opt}) && $conf->{$opt} !~ m/spice/) || $param->{$opt} !~ m/spice/);
 		    $conf->{pending}->{$opt} = $param->{$opt};
 		} else {
 		    $conf->{pending}->{$opt} = $param->{$opt};
-- 
2.30.2

Re: [pve-devel] [PATCH v4 qemu-server 06/18] api: allow SU privileged users to edit root-only options for VM configs

[Fabian Grünbichler]

On June 2, 2022 9:24 am, Oguz Bektas wrote:
> we now allow users with SU privilege to edit real device configurations
> for VMs.
> 
> they still need the required privilege to edit the corresponding
> configuration options (e.g. `VM.Config.HWType`), as well as the SU
> privilege.
> 
> Co-authored-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
> v3->v4:
> * added fabian's tag there as well since he helped me a lot with this part
> 
> 
>  PVE/API2/Qemu.pm | 62 ++++++++++++++++++++++++------------------------
>  1 file changed, 31 insertions(+), 31 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 99b426e..2e75ab6 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -545,16 +545,17 @@ my $cloudinitoptions = {
>  my $check_vm_create_serial_perm = sub {
>      my ($rpcenv, $authuser, $vmid, $pool, $param) = @_;
>  
> +    # no need to check permissions for root@pam
>      return 1 if $authuser eq 'root@pam';
>  
> +    my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);

this could switch to check_vm_perm if we want to extend the pool-based 
ACL to SU as well for VM creation. the vmid is not added to the pool yet 
at that point, so just checking /vms/$vmid doesn't work. but likely 
anybody who has superuser on a full pool and needs it to create 
superuser-restricted VMs already has superuser globally anyway, so not 
that important.

> +
>      foreach my $opt (keys %{$param}) {
>  	next if $opt !~ m/^serial\d+$/;
>  
> -	if ($param->{$opt} eq 'socket') {
> -	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.HWType']);
> -	} else {
> -	    die "only root can set '$opt' config for real devices\n";
> -	}
> +	$rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.HWType']);
> +	die "only superusers can set '$opt' config for real devices\n"
> +	    if $param->{$opt} ne 'socket' && !$is_superuser;
>      }
>  
>      return 1;
> @@ -563,16 +564,17 @@ my $check_vm_create_serial_perm = sub {
>  my $check_vm_create_usb_perm = sub {
>      my ($rpcenv, $authuser, $vmid, $pool, $param) = @_;
>  
> +    # no need to check permissions for root@pam
>      return 1 if $authuser eq 'root@pam';
>  
> +    my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);

same here

> +
>      foreach my $opt (keys %{$param}) {
>  	next if $opt !~ m/^usb\d+$/;
>  
> -	if ($param->{$opt} =~ m/spice/) {
> -	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.HWType']);
> -	} else {
> -	    die "only root can set '$opt' config for real devices\n";
> -	}
> +	$rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.HWType']);
> +	die "only superusers can set '$opt' config for real devices\n"
> +	    if $param->{$opt} !~ m/spice/ && !$is_superuser;
>      }
>  
>      return 1;
> @@ -581,8 +583,11 @@ my $check_vm_create_usb_perm = sub {
>  my $check_vm_modify_config_perm = sub {
>      my ($rpcenv, $authuser, $vmid, $pool, $key_list) = @_;
>  
> +    # no need to check permissions for root@pam
>      return 1 if $authuser eq 'root@pam';
>  
> +    my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);

and here

> +
>      foreach my $opt (@$key_list) {
>  	# some checks (e.g., disk, serial port, usb) need to be done somewhere
>  	# else, as there the permission can be value dependend
> @@ -618,7 +623,8 @@ my $check_vm_modify_config_perm = sub {
>  	} else {
>  	    # catches hostpci\d+, args, lock, etc.
>  	    # new options will be checked here
> -	    die "only root can set '$opt' config\n";
> +	    die "only superusers can set '$opt' config\n"
> +		if !$is_superuser;
>  	}
>      }
>  
> @@ -1328,6 +1334,8 @@ my $update_vm_api  = sub {
>  	push @paramarr, "-$key", $value;
>      }
>  
> +    my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);

but not here ;)

> +
>      my $skiplock = extract_param($param, 'skiplock');
>      raise_param_exc({ skiplock => "Only root may use this option." })
>  	if $skiplock && $authuser ne 'root@pam';
> @@ -1544,19 +1552,15 @@ my $update_vm_api  = sub {
>  		    PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
>  		    PVE::QemuConfig->write_config($vmid, $conf);
>  		} elsif ($opt =~ m/^serial\d+$/) {
> -		    if ($val eq 'socket') {
> -			$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
> -		    } elsif ($authuser ne 'root@pam') {
> -			die "only root can delete '$opt' config for real devices\n";
> -		    }
> +		    $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
> +		    die "only superusers can delete '$opt' config for real devices\n"
> +			if $val ne 'socket' && !$is_superuser;
>  		    PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
>  		    PVE::QemuConfig->write_config($vmid, $conf);
>  		} elsif ($opt =~ m/^usb\d+$/) {
> -		    if ($val =~ m/spice/) {
> -			$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
> -		    } elsif ($authuser ne 'root@pam') {
> -			die "only root can delete '$opt' config for real devices\n";
> -		    }
> +		    $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
> +		    die "only superusers can delete '$opt' config for real devices\n"
> +			if $val !~ m/spice/ && !$is_superuser;
>  		    PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
>  		    PVE::QemuConfig->write_config($vmid, $conf);
>  		} else {
> @@ -1606,18 +1610,14 @@ my $update_vm_api  = sub {
>  			}
>  		    }
>  		} elsif ($opt =~ m/^serial\d+/) {
> -		    if ((!defined($conf->{$opt}) || $conf->{$opt} eq 'socket') && $param->{$opt} eq 'socket') {
> -			$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
> -		    } elsif ($authuser ne 'root@pam') {
> -			die "only root can modify '$opt' config for real devices\n";
> -		    }
> +		    $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']); # always check hw config

nit: that comment is not needed

> +		    die "only superusers can modify '$opt' config for real devices\n"
> +			if !$is_superuser && ((defined($conf->{$opt}) && $conf->{$opt} ne 'socket') || $param->{$opt} ne 'socket');
>  		    $conf->{pending}->{$opt} = $param->{$opt};
>  		} elsif ($opt =~ m/^usb\d+/) {
> -		    if ((!defined($conf->{$opt}) || $conf->{$opt} =~ m/spice/) && $param->{$opt} =~ m/spice/) {
> -			$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
> -		    } elsif ($authuser ne 'root@pam') {
> -			die "only root can modify '$opt' config for real devices\n";
> -		    }
> +		    $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
> +		    die "only superusers can modify '$opt' config for real devices\n"
> +			if !$is_superuser && ((defined($conf->{$opt}) && $conf->{$opt} !~ m/spice/) || $param->{$opt} !~ m/spice/);
>  		    $conf->{pending}->{$opt} = $param->{$opt};
>  		} else {
>  		    $conf->{pending}->{$opt} = $param->{$opt};
> -- 
> 2.30.2
> 
> 

[pve-devel] [PATCH v4 qemu-server 07/18] migration tests: mock $rpcenv->check subroutine

[Oguz Bektas]

missing mock routine is causes the tests to fail at build time
when $rpcenv->check is called.

previous assumption was returning 'root@pam' to get_user() so we can
do the same here.

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
 test/MigrationTest/QmMock.pm | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/test/MigrationTest/QmMock.pm b/test/MigrationTest/QmMock.pm
index 2d5d5c6..eb6ea53 100644
--- a/test/MigrationTest/QmMock.pm
+++ b/test/MigrationTest/QmMock.pm
@@ -40,6 +40,11 @@ sub fork_worker {
     return '123456';
 }
 
+sub check {
+    my ($self, $authuser, $path, $priv, $noerr) = @_;
+    return 1 if $authuser eq 'root@pam';
+}
+
 # mocked modules
 
 my $inotify_module = Test::MockModule->new("PVE::INotify");
-- 
2.30.2

[pve-devel] [PATCH v4 qemu-server 08/18] api: allow superusers to use 'skiplock' option

[Oguz Bektas]

also mark the intentionally root-only migration related options
in param descriptions and leave a reminder comment.

Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
 PVE/API2/Qemu.pm | 71 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 48 insertions(+), 23 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 2e75ab6..198e736 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -1337,8 +1337,8 @@ my $update_vm_api  = sub {
     my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
 
     my $skiplock = extract_param($param, 'skiplock');
-    raise_param_exc({ skiplock => "Only root may use this option." })
-	if $skiplock && $authuser ne 'root@pam';
+    raise_param_exc({ skiplock => "Only superusers may use this option." })
+	if $skiplock && !$is_superuser;
 
     my $delete_str = extract_param($param, 'delete');
 
@@ -1864,9 +1864,11 @@ __PACKAGE__->register_method({
 	my $authuser = $rpcenv->get_user();
 	my $vmid = $param->{vmid};
 
+	my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
+
 	my $skiplock = $param->{skiplock};
-	raise_param_exc({ skiplock => "Only root may use this option." })
-	    if $skiplock && $authuser ne 'root@pam';
+	raise_param_exc({ skiplock => "Only superusers may use this option." })
+	    if $skiplock && !$is_superuser;
 
 	my $early_checks = sub {
 	    # test if VM exists
@@ -2474,25 +2476,27 @@ __PACKAGE__->register_method({
 	    migration_type => {
 		type => 'string',
 		enum => ['secure', 'insecure'],
-		description => "Migration traffic is encrypted using an SSH " .
+		description => "Migration-internal parameter. Migration traffic is encrypted using an SSH " .
 		  "tunnel by default. On secure, completely private networks " .
 		  "this can be disabled to increase performance.",
 		optional => 1,
 	    },
 	    migration_network => {
 		type => 'string', format => 'CIDR',
-		description => "CIDR of the (sub) network that is used for migration.",
+		description => "Migration-internal parameter. CIDR of the (sub)network " .
+		    "that is used for migration.",
 		optional => 1,
 	    },
 	    machine => get_standard_option('pve-qemu-machine'),
 	    'force-cpu' => {
-		description => "Override QEMU's -cpu argument with the given string.",
+		description => "Migration-internal parameter. Override QEMU's" .
+		    "-cpu argument with the given string.",
 		type => 'string',
 		optional => 1,
 	    },
 	    targetstorage => get_standard_option('pve-targetstorage'),
 	    timeout => {
-		description => "Wait maximal timeout seconds.",
+		description => "Migration-internal parameter. Wait maximal timeout seconds.",
 		type => 'integer',
 		minimum => 0,
 		default => 'max(30, vm memory in GiB)',
@@ -2514,6 +2518,14 @@ __PACKAGE__->register_method({
 	my $timeout = extract_param($param, 'timeout');
 	my $machine = extract_param($param, 'machine');
 
+	my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
+
+	my $skiplock = extract_param($param, 'skiplock');
+	raise_param_exc({ skiplock => "Only superusers may use this option." })
+	    if $skiplock && !$is_superuser;
+
+	# since they are only used for migration-internal flows,
+	# these parameters are still intentionally limited to root@pam
 	my $get_root_param = sub {
 	    my $value = extract_param($param, $_[0]);
 	    raise_param_exc({ "$_[0]" => "Only root may use this option." })
@@ -2522,7 +2534,6 @@ __PACKAGE__->register_method({
 	};
 
 	my $stateuri = $get_root_param->('stateuri');
-	my $skiplock = $get_root_param->('skiplock');
 	my $migratedfrom = $get_root_param->('migratedfrom');
 	my $migration_type = $get_root_param->('migration_type');
 	my $migration_network = $get_root_param->('migration_network');
@@ -2662,9 +2673,11 @@ __PACKAGE__->register_method({
 	my $node = extract_param($param, 'node');
 	my $vmid = extract_param($param, 'vmid');
 
+	my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
+
 	my $skiplock = extract_param($param, 'skiplock');
-	raise_param_exc({ skiplock => "Only root may use this option." })
-	    if $skiplock && $authuser ne 'root@pam';
+	raise_param_exc({ skiplock => "Only superusers may use this option." })
+	    if $skiplock && !$is_superuser;
 
 	my $keepActive = extract_param($param, 'keepActive');
 	raise_param_exc({ keepActive => "Only root may use this option." })
@@ -2739,9 +2752,11 @@ __PACKAGE__->register_method({
 
 	my $vmid = extract_param($param, 'vmid');
 
+	my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
+
 	my $skiplock = extract_param($param, 'skiplock');
-	raise_param_exc({ skiplock => "Only root may use this option." })
-	    if $skiplock && $authuser ne 'root@pam';
+	raise_param_exc({ skiplock => "Only superusers may use this option." })
+	    if $skiplock && !$is_superuser;
 
 	die "VM $vmid not running\n" if !PVE::QemuServer::check_running($vmid);
 
@@ -2806,9 +2821,11 @@ __PACKAGE__->register_method({
 	my $node = extract_param($param, 'node');
 	my $vmid = extract_param($param, 'vmid');
 
+	my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
+
 	my $skiplock = extract_param($param, 'skiplock');
-	raise_param_exc({ skiplock => "Only root may use this option." })
-	    if $skiplock && $authuser ne 'root@pam';
+	raise_param_exc({ skiplock => "Only superusers may use this option." })
+	    if $skiplock && !$is_superuser;
 
 	my $keepActive = extract_param($param, 'keepActive');
 	raise_param_exc({ keepActive => "Only root may use this option." })
@@ -2965,9 +2982,11 @@ __PACKAGE__->register_method({
 
 	my $statestorage = extract_param($param, 'statestorage');
 
+	my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
+
 	my $skiplock = extract_param($param, 'skiplock');
-	raise_param_exc({ skiplock => "Only root may use this option." })
-	    if $skiplock && $authuser ne 'root@pam';
+	raise_param_exc({ skiplock => "Only superusers may use this option." })
+	    if $skiplock && !$is_superuser;
 
 	die "VM $vmid not running\n" if !PVE::QemuServer::check_running($vmid);
 
@@ -3037,9 +3056,11 @@ __PACKAGE__->register_method({
 
 	my $vmid = extract_param($param, 'vmid');
 
+	my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
+
 	my $skiplock = extract_param($param, 'skiplock');
-	raise_param_exc({ skiplock => "Only root may use this option." })
-	    if $skiplock && $authuser ne 'root@pam';
+	raise_param_exc({ skiplock => "Only superusers may use this option." })
+	    if $skiplock && !$is_superuser;
 
 	my $nocheck = extract_param($param, 'nocheck');
 	raise_param_exc({ nocheck => "Only root may use this option." })
@@ -3109,9 +3130,11 @@ __PACKAGE__->register_method({
 
 	my $vmid = extract_param($param, 'vmid');
 
+	my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
+
 	my $skiplock = extract_param($param, 'skiplock');
-	raise_param_exc({ skiplock => "Only root may use this option." })
-	    if $skiplock && $authuser ne 'root@pam';
+	raise_param_exc({ skiplock => "Only superusers may use this option." })
+	    if $skiplock && !$is_superuser;
 
 	PVE::QemuServer::vm_sendkey($vmid, $skiplock, $param->{key});
 
@@ -4372,9 +4395,11 @@ __PACKAGE__->register_method({
 
 	my $sizestr = extract_param($param, 'size');
 
+	my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
+
 	my $skiplock = extract_param($param, 'skiplock');
-        raise_param_exc({ skiplock => "Only root may use this option." })
-            if $skiplock && $authuser ne 'root@pam';
+        raise_param_exc({ skiplock => "Only superusers may use this option." })
+            if $skiplock && !$is_superuser;
 
         my $storecfg = PVE::Storage::config();
 
-- 
2.30.2

Re: [pve-devel] [PATCH v4 qemu-server 08/18] api: allow superusers to use 'skiplock' option

[Fabian Grünbichler]

On June 2, 2022 9:24 am, Oguz Bektas wrote:
> also mark the intentionally root-only migration related options
> in param descriptions and leave a reminder comment.

how are these changes related? please split them up into two patches (or 
merge the comment part into the other path that adds similar comments)

> 
> Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
>  PVE/API2/Qemu.pm | 71 ++++++++++++++++++++++++++++++++----------------
>  1 file changed, 48 insertions(+), 23 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 2e75ab6..198e736 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -1337,8 +1337,8 @@ my $update_vm_api  = sub {
>      my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
>  
>      my $skiplock = extract_param($param, 'skiplock');
> -    raise_param_exc({ skiplock => "Only root may use this option." })
> -	if $skiplock && $authuser ne 'root@pam';
> +    raise_param_exc({ skiplock => "Only superusers may use this option." })
> +	if $skiplock && !$is_superuser;
>  
>      my $delete_str = extract_param($param, 'delete');
>  
> @@ -1864,9 +1864,11 @@ __PACKAGE__->register_method({
>  	my $authuser = $rpcenv->get_user();
>  	my $vmid = $param->{vmid};
>  
> +	my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
> +
>  	my $skiplock = $param->{skiplock};
> -	raise_param_exc({ skiplock => "Only root may use this option." })
> -	    if $skiplock && $authuser ne 'root@pam';
> +	raise_param_exc({ skiplock => "Only superusers may use this option." })
> +	    if $skiplock && !$is_superuser;
>  
>  	my $early_checks = sub {
>  	    # test if VM exists
> @@ -2474,25 +2476,27 @@ __PACKAGE__->register_method({
>  	    migration_type => {
>  		type => 'string',
>  		enum => ['secure', 'insecure'],
> -		description => "Migration traffic is encrypted using an SSH " .
> +		description => "Migration-internal parameter. Migration traffic is encrypted using an SSH " .
>  		  "tunnel by default. On secure, completely private networks " .
>  		  "this can be disabled to increase performance.",
>  		optional => 1,
>  	    },
>  	    migration_network => {
>  		type => 'string', format => 'CIDR',
> -		description => "CIDR of the (sub) network that is used for migration.",
> +		description => "Migration-internal parameter. CIDR of the (sub)network " .
> +		    "that is used for migration.",
>  		optional => 1,
>  	    },
>  	    machine => get_standard_option('pve-qemu-machine'),
>  	    'force-cpu' => {
> -		description => "Override QEMU's -cpu argument with the given string.",
> +		description => "Migration-internal parameter. Override QEMU's" .
> +		    "-cpu argument with the given string.",
>  		type => 'string',
>  		optional => 1,
>  	    },
>  	    targetstorage => get_standard_option('pve-targetstorage'),
>  	    timeout => {
> -		description => "Wait maximal timeout seconds.",
> +		description => "Migration-internal parameter. Wait maximal timeout seconds.",
>  		type => 'integer',
>  		minimum => 0,
>  		default => 'max(30, vm memory in GiB)',
> @@ -2514,6 +2518,14 @@ __PACKAGE__->register_method({
>  	my $timeout = extract_param($param, 'timeout');
>  	my $machine = extract_param($param, 'machine');
>  
> +	my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
> +
> +	my $skiplock = extract_param($param, 'skiplock');
> +	raise_param_exc({ skiplock => "Only superusers may use this option." })
> +	    if $skiplock && !$is_superuser;
> +
> +	# since they are only used for migration-internal flows,
> +	# these parameters are still intentionally limited to root@pam
>  	my $get_root_param = sub {
>  	    my $value = extract_param($param, $_[0]);
>  	    raise_param_exc({ "$_[0]" => "Only root may use this option." })
> @@ -2522,7 +2534,6 @@ __PACKAGE__->register_method({
>  	};
>  
>  	my $stateuri = $get_root_param->('stateuri');
> -	my $skiplock = $get_root_param->('skiplock');
>  	my $migratedfrom = $get_root_param->('migratedfrom');
>  	my $migration_type = $get_root_param->('migration_type');
>  	my $migration_network = $get_root_param->('migration_network');
> @@ -2662,9 +2673,11 @@ __PACKAGE__->register_method({
>  	my $node = extract_param($param, 'node');
>  	my $vmid = extract_param($param, 'vmid');
>  
> +	my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
> +
>  	my $skiplock = extract_param($param, 'skiplock');
> -	raise_param_exc({ skiplock => "Only root may use this option." })
> -	    if $skiplock && $authuser ne 'root@pam';
> +	raise_param_exc({ skiplock => "Only superusers may use this option." })
> +	    if $skiplock && !$is_superuser;
>  
>  	my $keepActive = extract_param($param, 'keepActive');
>  	raise_param_exc({ keepActive => "Only root may use this option." })
> @@ -2739,9 +2752,11 @@ __PACKAGE__->register_method({
>  
>  	my $vmid = extract_param($param, 'vmid');
>  
> +	my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
> +
>  	my $skiplock = extract_param($param, 'skiplock');
> -	raise_param_exc({ skiplock => "Only root may use this option." })
> -	    if $skiplock && $authuser ne 'root@pam';
> +	raise_param_exc({ skiplock => "Only superusers may use this option." })
> +	    if $skiplock && !$is_superuser;
>  
>  	die "VM $vmid not running\n" if !PVE::QemuServer::check_running($vmid);
>  
> @@ -2806,9 +2821,11 @@ __PACKAGE__->register_method({
>  	my $node = extract_param($param, 'node');
>  	my $vmid = extract_param($param, 'vmid');
>  
> +	my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
> +
>  	my $skiplock = extract_param($param, 'skiplock');
> -	raise_param_exc({ skiplock => "Only root may use this option." })
> -	    if $skiplock && $authuser ne 'root@pam';
> +	raise_param_exc({ skiplock => "Only superusers may use this option." })
> +	    if $skiplock && !$is_superuser;
>  
>  	my $keepActive = extract_param($param, 'keepActive');
>  	raise_param_exc({ keepActive => "Only root may use this option." })
> @@ -2965,9 +2982,11 @@ __PACKAGE__->register_method({
>  
>  	my $statestorage = extract_param($param, 'statestorage');
>  
> +	my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
> +
>  	my $skiplock = extract_param($param, 'skiplock');
> -	raise_param_exc({ skiplock => "Only root may use this option." })
> -	    if $skiplock && $authuser ne 'root@pam';
> +	raise_param_exc({ skiplock => "Only superusers may use this option." })
> +	    if $skiplock && !$is_superuser;
>  
>  	die "VM $vmid not running\n" if !PVE::QemuServer::check_running($vmid);
>  
> @@ -3037,9 +3056,11 @@ __PACKAGE__->register_method({
>  
>  	my $vmid = extract_param($param, 'vmid');
>  
> +	my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
> +
>  	my $skiplock = extract_param($param, 'skiplock');
> -	raise_param_exc({ skiplock => "Only root may use this option." })
> -	    if $skiplock && $authuser ne 'root@pam';
> +	raise_param_exc({ skiplock => "Only superusers may use this option." })
> +	    if $skiplock && !$is_superuser;
>  
>  	my $nocheck = extract_param($param, 'nocheck');
>  	raise_param_exc({ nocheck => "Only root may use this option." })
> @@ -3109,9 +3130,11 @@ __PACKAGE__->register_method({
>  
>  	my $vmid = extract_param($param, 'vmid');
>  
> +	my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
> +
>  	my $skiplock = extract_param($param, 'skiplock');
> -	raise_param_exc({ skiplock => "Only root may use this option." })
> -	    if $skiplock && $authuser ne 'root@pam';
> +	raise_param_exc({ skiplock => "Only superusers may use this option." })
> +	    if $skiplock && !$is_superuser;
>  
>  	PVE::QemuServer::vm_sendkey($vmid, $skiplock, $param->{key});
>  
> @@ -4372,9 +4395,11 @@ __PACKAGE__->register_method({
>  
>  	my $sizestr = extract_param($param, 'size');
>  
> +	my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
> +
>  	my $skiplock = extract_param($param, 'skiplock');
> -        raise_param_exc({ skiplock => "Only root may use this option." })
> -            if $skiplock && $authuser ne 'root@pam';
> +        raise_param_exc({ skiplock => "Only superusers may use this option." })
> +            if $skiplock && !$is_superuser;
>  
>          my $storecfg = PVE::Storage::config();
>  
> -- 
> 2.30.2
> 
> 

[pve-devel] [PATCH v4 qemu-server 09/18] parse_backup_hints: add comment for root shortcut and fix typos

[Oguz Bektas]

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
 PVE/QemuServer.pm | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index e9aa248..05b2f29 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -6290,11 +6290,11 @@ my $restore_cleanup_oldconf = sub {
 
 # Helper to parse vzdump backup device hints
 #
-# $rpcenv: Environment, used to ckeck storage permissions
+# $rpcenv: Environment, used to check storage permissions
 # $user: User ID, to check storage permissions
 # $storecfg: Storage configuration
 # $fh: the file handle for reading the configuration
-# $devinfo: should contain device sizes for all backu-up'ed devices
+# $devinfo: should contain device sizes for all backed up devices
 # $options: backup options (pool, default storage)
 #
 # Return: $virtdev_hash, updates $devinfo (add devname, virtdev, format, storeid)
@@ -6306,7 +6306,7 @@ my $parse_backup_hints = sub {
 	die "Content type 'images' is not available on storage '$storeid'\n"
 	    if !$scfg->{content}->{images};
 	$rpcenv->check($user, "/storage/$storeid", ['Datastore.AllocateSpace'])
-	    if $user ne 'root@pam';
+	    if $user ne 'root@pam'; # shortcut for root@pam
     };
 
     my $virtdev_hash = {};
-- 
2.30.2

[pve-devel] [PATCH v4 manager 10/18] api: backup: allow SUs to use 'tmpdir', 'dumpdir' and 'script' options

[Oguz Bektas]

previously limited to root@pam; we can allow SUs to use these options if
they have the privilege on the whole API path.

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
 PVE/API2/Backup.pm | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/PVE/API2/Backup.pm b/PVE/API2/Backup.pm
index 5d36789a..286996b5 100644
--- a/PVE/API2/Backup.pm
+++ b/PVE/API2/Backup.pm
@@ -41,10 +41,13 @@ my $vzdump_job_id_prop = {
 
 my $assert_param_permission = sub {
     my ($param, $user) = @_;
-    return if $user eq 'root@pam'; # always OK
+    return if $user eq 'root@pam'; # root@pam always OK
+
+    my $rpcenv = PVE::RPCEnvironment::get();
+    return if $rpcenv->check($user, "/", ['SuperUser'], 1); # SuperUser on /, always OK
 
     for my $key (qw(tmpdir dumpdir script)) {
-	raise_param_exc({ $key => "Only root may set this option."}) if exists $param->{$key};
+	raise_param_exc({ $key => "Only superusers may set this option."}) if exists $param->{$key};
     }
 };
 
@@ -143,7 +146,7 @@ __PACKAGE__->register_method({
     description => "Create new vzdump backup job.",
     permissions => {
 	check => ['perm', '/', ['Sys.Modify']],
-	description => "The 'tmpdir', 'dumpdir' and 'script' parameters are additionally restricted to the 'root\@pam' user.",
+	description => "The 'tmpdir', 'dumpdir' and 'script' parameters are additionally restricted to superusers.",
     },
     parameters => {
     	additionalProperties => 0,
@@ -345,7 +348,7 @@ __PACKAGE__->register_method({
     description => "Update vzdump backup job definition.",
     permissions => {
 	check => ['perm', '/', ['Sys.Modify']],
-	description => "The 'tmpdir', 'dumpdir' and 'script' parameters are additionally restricted to the 'root\@pam' user.",
+	description => "The 'tmpdir', 'dumpdir' and 'script' parameters are additionally restricted to superusers.",
     },
     parameters => {
     	additionalProperties => 0,
-- 
2.30.2

[pve-devel] [PATCH v4 manager 11/18] api: vzdump: allow SUs to use 'bwlimit' and 'ionice' parameters

[Oguz Bektas]

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
 PVE/API2/VZDump.pm | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/PVE/API2/VZDump.pm b/PVE/API2/VZDump.pm
index 13b6cd46..1c9c3631 100644
--- a/PVE/API2/VZDump.pm
+++ b/PVE/API2/VZDump.pm
@@ -27,7 +27,7 @@ __PACKAGE__->register_method ({
     permissions => {
 	description => "The user needs 'VM.Backup' permissions on any VM, and 'Datastore.AllocateSpace'"
 	    ." on the backup storage. The 'maxfiles', 'prune-backups', 'tmpdir', 'dumpdir', 'script',"
-	    ." 'bwlimit' and 'ionice' parameters are restricted to the 'root\@pam' user.",
+	    ." 'bwlimit' and 'ionice' parameters are restricted to the superusers.",
 	user => 'all',
     },
     protected => 1,
@@ -52,6 +52,8 @@ __PACKAGE__->register_method ({
 
 	my $nodename = PVE::INotify::nodename();
 
+	my $is_superuser = $rpcenv->check($user, "/", ['SuperUser'], 1);
+
 	if ($rpcenv->{type} ne 'cli') {
 	    raise_param_exc({ node => "option is only allowed on the command line interface."})
 		if $param->{node} && $param->{node} ne $nodename;
@@ -61,8 +63,8 @@ __PACKAGE__->register_method ({
 	}
 
 	foreach my $key (qw(maxfiles prune-backups tmpdir dumpdir script bwlimit ionice)) {
-	    raise_param_exc({ $key => "Only root may set this option."})
-		if defined($param->{$key}) && ($user ne 'root@pam');
+	    raise_param_exc({ $key => "Only superusers may set this option."})
+		if defined($param->{$key}) && !$is_superuser;
 	}
 
 	PVE::VZDump::verify_vzdump_parameters($param, 1);
-- 
2.30.2

[pve-devel] [PATCH v4 manager 12/18] api: always drop to login prompt for non-root users on terminal proxy calls

[Oguz Bektas]

we should still drop to a login prompt on our spice/vnc/termproxy for
SUs.

also updated a comment about missing superuser role.

Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
v3->v4:
* changed wrong condition check (eq 'login' vs. ne 'login')


 PVE/API2/Nodes.pm | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
index 655493a3..a43768a7 100644
--- a/PVE/API2/Nodes.pm
+++ b/PVE/API2/Nodes.pm
@@ -870,7 +870,7 @@ sub get_shell_command  {
 	    $cmd = [ '/bin/login', '-f', 'root' ];
 	}
     } else {
-	# non-root must always login for now, we do not have a superuser role!
+	# non-root must always login, even with SU privilege
 	$cmd = [ '/bin/login' ];
     }
     return $cmd;
@@ -960,7 +960,7 @@ __PACKAGE__->register_method ({
 
 	raise_perm_exc("realm != pam") if $realm ne 'pam';
 
-	if (defined($param->{cmd}) && $param->{cmd} eq 'upgrade' && $user ne 'root@pam') {
+	if (defined($param->{cmd}) && $param->{cmd} ne 'login' && $user ne 'root@pam') {
 	    raise_perm_exc('user != root@pam');
 	}
 
@@ -1077,8 +1077,13 @@ __PACKAGE__->register_method ({
 
 	my $rpcenv = PVE::RPCEnvironment::get();
 	my ($user, undef, $realm) = PVE::AccessControl::verify_username($rpcenv->get_user());
+
 	raise_perm_exc("realm $realm != pam") if $realm ne 'pam';
 
+	if (defined($param->{cmd}) && $param->{cmd} ne 'login' && $user ne 'root@pam') {
+	    raise_perm_exc('user != root@pam');
+	}
+
 	my $node = $param->{node};
 	my $authpath = "/nodes/$node";
 	my $ticket = PVE::AccessControl::assemble_vnc_ticket($user, $authpath);
@@ -1208,7 +1213,7 @@ __PACKAGE__->register_method ({
 
 	raise_perm_exc("realm != pam") if $realm ne 'pam';
 
-	if (defined($param->{cmd}) && $param->{cmd} eq 'upgrade' && $user ne 'root@pam') {
+	if (defined($param->{cmd}) && $param->{cmd} ne 'login' && $user ne 'root@pam') {
 	    raise_perm_exc('user != root@pam');
 	}
 
-- 
2.30.2

[pve-devel] [PATCH v4 manager 13/18] ui: include "SuperUser" in privilege selector

[Oguz Bektas]

only the 'SuperAdministrator' role has full privileges including SU,
that's why we use that role to generate all the privileges.

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
 www/manager6/form/PrivilegesSelector.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/www/manager6/form/PrivilegesSelector.js b/www/manager6/form/PrivilegesSelector.js
index a2427c36..5494e7c2 100644
--- a/www/manager6/form/PrivilegesSelector.js
+++ b/www/manager6/form/PrivilegesSelector.js
@@ -10,7 +10,7 @@ Ext.define('PVE.form.PrivilegesSelector', {
 	me.callParent();
 
 	Proxmox.Utils.API2Request({
-	    url: '/access/roles/Administrator',
+	    url: '/access/roles/SuperAdministrator',
 	    method: 'GET',
 	    success: function(response, options) {
 		let data = Object.keys(response.result.data).map(key => [key, key]);
-- 
2.30.2

[pve-devel] [PATCH v4 manager 14/18] ui: lxc features: check for SU instead of 'root@pam'

[Oguz Bektas]

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
 www/manager6/lxc/Options.js | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/www/manager6/lxc/Options.js b/www/manager6/lxc/Options.js
index d0a53fc7..7aa18447 100644
--- a/www/manager6/lxc/Options.js
+++ b/www/manager6/lxc/Options.js
@@ -175,9 +175,13 @@ Ext.define('PVE.lxc.Options', {
 
 	    if (key === 'features') {
 		let unprivileged = me.getStore().getById('unprivileged').data.value;
-		let root = Proxmox.UserName === 'root@pam';
+		let superuser = caps.vms['SuperUser']; // eslint-disable-line
 		let vmalloc = caps.vms['VM.Allocate'];
-		edit_btn.setDisabled(!(root || (vmalloc && unprivileged)));
+		let disabled = !(superuser || (vmalloc && unprivileged));
+		edit_btn.setDisabled(disabled);
+		if (disabled) {
+		    rowdef.editor = undefined;
+		}
 	    } else {
 		edit_btn.setDisabled(!rowdef.editor);
 	    }
-- 
2.30.2

[pve-devel] [PATCH v4 manager 15/18] ui: adapt sensible 'root@pam' checks to SU

[Oguz Bektas]

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
* left off ceph since we drop to a /bin/login shell anyways

 www/manager6/lxc/Resources.js  | 6 +++---
 www/manager6/window/Migrate.js | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/www/manager6/lxc/Resources.js b/www/manager6/lxc/Resources.js
index 4b2ae95e..f82d056d 100644
--- a/www/manager6/lxc/Resources.js
+++ b/www/manager6/lxc/Resources.js
@@ -306,9 +306,9 @@ Ext.define('PVE.lxc.RessourceView', {
 	    let isUnusedDisk = key.match(/^unused\d+/);
 	    let isUsedDisk = isDisk && !isUnusedDisk;
 
-	    let noedit = isDelete || !rowdef.editor;
-	    if (!noedit && Proxmox.UserName !== 'root@pam' && key.match(/^mp\d+$/)) {
-		let mp = PVE.Parser.parseLxcMountPoint(value);
+	    var noedit = rec.data.delete || !rowdef.editor;
+	    if (!noedit && !caps.vms['SuperUser'] && key.match(/^mp\d+$/)) { // eslint-disable-line
+		var mp = PVE.Parser.parseLxcMountPoint(value);
 		if (mp.type !== 'volume') {
 		    noedit = true;
 		}
diff --git a/www/manager6/window/Migrate.js b/www/manager6/window/Migrate.js
index 1c23abb3..597e3b0b 100644
--- a/www/manager6/window/Migrate.js
+++ b/www/manager6/window/Migrate.js
@@ -52,8 +52,8 @@ Ext.define('PVE.window.Migrate', {
 		    }
 	    },
 	    setLocalResourceCheckboxHidden: function(get) {
-		if (get('running') || !get('migration.hasLocalResources') ||
-		    Proxmox.UserName !== 'root@pam') {
+		let caps = Ext.state.Manager.get('GuiCap');
+		if (get('running') || !get('migration.hasLocalResources') || !caps.vms['SuperUser']) { // eslint-disable-line
 		    return true;
 		} else {
 		    return false;
-- 
2.30.2

Re: [pve-devel] [PATCH v4 manager 15/18] ui: adapt sensible 'root@pam' checks to SU

[Fabian Grünbichler]

On June 2, 2022 9:24 am, Oguz Bektas wrote:
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
> * left off ceph since we drop to a /bin/login shell anyways
> 
>  www/manager6/lxc/Resources.js  | 6 +++---
>  www/manager6/window/Migrate.js | 4 ++--
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/www/manager6/lxc/Resources.js b/www/manager6/lxc/Resources.js
> index 4b2ae95e..f82d056d 100644
> --- a/www/manager6/lxc/Resources.js
> +++ b/www/manager6/lxc/Resources.js
> @@ -306,9 +306,9 @@ Ext.define('PVE.lxc.RessourceView', {
>  	    let isUnusedDisk = key.match(/^unused\d+/);
>  	    let isUsedDisk = isDisk && !isUnusedDisk;
>  
> -	    let noedit = isDelete || !rowdef.editor;
> -	    if (!noedit && Proxmox.UserName !== 'root@pam' && key.match(/^mp\d+$/)) {
> -		let mp = PVE.Parser.parseLxcMountPoint(value);
> +	    var noedit = rec.data.delete || !rowdef.editor;

nit: why not keep the isDelete like in the rest of the code above?

> +	    if (!noedit && !caps.vms['SuperUser'] && key.match(/^mp\d+$/)) { // eslint-disable-line
> +		var mp = PVE.Parser.parseLxcMountPoint(value);
>  		if (mp.type !== 'volume') {
>  		    noedit = true;
>  		}
> diff --git a/www/manager6/window/Migrate.js b/www/manager6/window/Migrate.js
> index 1c23abb3..597e3b0b 100644
> --- a/www/manager6/window/Migrate.js
> +++ b/www/manager6/window/Migrate.js
> @@ -52,8 +52,8 @@ Ext.define('PVE.window.Migrate', {
>  		    }
>  	    },
>  	    setLocalResourceCheckboxHidden: function(get) {
> -		if (get('running') || !get('migration.hasLocalResources') ||
> -		    Proxmox.UserName !== 'root@pam') {
> +		let caps = Ext.state.Manager.get('GuiCap');
> +		if (get('running') || !get('migration.hasLocalResources') || !caps.vms['SuperUser']) { // eslint-disable-line
>  		    return true;
>  		} else {
>  		    return false;
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

[pve-devel] [PATCH v4 container 16/18] fix #2582: api: add checks for 'SuperUser' privilege for root-only options

[Oguz Bektas]

this way we can allow regular users to act as superuser on specific
paths by giving them the (new) builtin 'SuperAdministrator' role or a
custom role with the 'SuperUser' privilege

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
 src/PVE/API2/LXC.pm        | 19 +++++++++----------
 src/PVE/API2/LXC/Config.pm |  2 +-
 src/PVE/API2/LXC/Status.pm | 12 ++++++++----
 src/PVE/LXC.pm             | 21 ++++++++++++---------
 src/PVE/LXC/Create.pm      |  2 +-
 5 files changed, 31 insertions(+), 25 deletions(-)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 64724cb..e42bd7a 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -258,7 +258,7 @@ __PACKAGE__->register_method({
 	    $skip_fw_config_restore = 1;
 
 	    # error out if a user tries to change from unprivileged to privileged
-	    # explicit change is checked here, implicit is checked down below or happening in root-only paths
+	    # explicit change is checked here, implicit is checked down below or happening in superuser-only paths
 	    my $conf = PVE::LXC::Config->load_config($vmid);
 	    if ($conf->{unprivileged} && defined($unprivileged) && !$unprivileged) {
 		raise_perm_exc("cannot change from unprivileged to privileged without VM.Allocate");
@@ -312,7 +312,7 @@ __PACKAGE__->register_method({
 
 	my $conf = {};
 
-	my $is_root = $authuser eq 'root@pam';
+	my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
 
 	my $no_disk_param = {};
 	my $mp_param = {};
@@ -347,8 +347,8 @@ __PACKAGE__->register_method({
 	    my $mp = $mountpoint->{mp};
 
 	    if ($mountpoint->{type} ne 'volume') { # bind or device
-		die "Only root can pass arbitrary filesystem paths.\n"
-		    if !$is_root;
+		die "Only superusers can pass arbitrary filesystem paths.\n"
+		    if !$is_superuser;
 	    } else {
 		my ($sid, $volname) = PVE::Storage::parse_volume_id($volid);
 		&$check_and_activate_storage($sid);
@@ -384,11 +384,11 @@ __PACKAGE__->register_method({
 
 			$was_template = delete $orig_conf->{template};
 
-			# When we're root call 'restore_configuration' with restricted=0,
+			# call 'restore_configuration' with restricted=0 when we have superuser,
 			# causing it to restore the raw lxc entries, among which there may be
 			# 'lxc.idmap' entries. We need to make sure that the extracted contents
 			# of the container match up with the restored configuration afterwards:
-			$conf->{lxc} = $orig_conf->{lxc} if $is_root;
+			$conf->{lxc} = $orig_conf->{lxc} if $is_superuser;
 
 			$conf->{unprivileged} = $orig_conf->{unprivileged}
 			    if !defined($unprivileged) && defined($orig_conf->{unprivileged});
@@ -422,8 +422,7 @@ __PACKAGE__->register_method({
 				my $type = $mountpoint->{type};
 				die "restoring rootfs to $type mount is only possible by specifying -rootfs manually!\n"
 				    if ($ms eq 'rootfs');
-				die "restoring '$ms' to $type mount is only possible for root\n"
-				    if !$is_root;
+				die "restoring '$ms' to $type mount is only possible for superusers\n" if !$is_superuser;
 
 				if ($mountpoint->{backup}) {
 				    warn "WARNING - unsupported configuration!\n";
@@ -464,7 +463,7 @@ __PACKAGE__->register_method({
 
 		    if ($restore) {
 			print "merging backed-up and given configuration..\n";
-			PVE::LXC::Create::restore_configuration($vmid, $storage_cfg, $archive, $rootdir, $conf, !$is_root, $unique, $skip_fw_config_restore);
+			PVE::LXC::Create::restore_configuration($vmid, $storage_cfg, $archive, $rootdir, $conf, !$is_superuser, $unique, $skip_fw_config_restore);
 			my $lxc_setup = PVE::LXC::Setup->new($conf, $rootdir);
 			$lxc_setup->template_fixup($conf);
 		    } else {
@@ -2215,7 +2214,7 @@ __PACKAGE__->register_method({
 	    raise_param_exc({ 'target-vmid' => $msg, 'storage' => $msg });
 	} elsif ($target_vmid) {
 	    $rpcenv->check_vm_perm($authuser, $target_vmid, undef, ['VM.Config.Disk'])
-		if $authuser ne 'root@pam';
+		if $authuser ne 'root@pam'; # no need to check for root@pam
 
 	    my (undef, undef, $drive) = $load_and_check_reassign_configs->();
 	    my $storeid = PVE::Storage::parse_volume_id($drive->{volume});
diff --git a/src/PVE/API2/LXC/Config.pm b/src/PVE/API2/LXC/Config.pm
index 1fec048..6278b8a 100644
--- a/src/PVE/API2/LXC/Config.pm
+++ b/src/PVE/API2/LXC/Config.pm
@@ -99,7 +99,7 @@ __PACKAGE__->register_method({
     description => "Set container options.",
     permissions => {
 	check => ['perm', '/vms/{vmid}', $vm_config_perm_list, any => 1],
-	description => 'non-volume mount points in rootfs and mp[n] are restricted to root@pam',
+	description => 'non-volume mount points in rootfs and mp[n] are restricted to superusers',
     },
     parameters => {
 	additionalProperties => 0,
diff --git a/src/PVE/API2/LXC/Status.pm b/src/PVE/API2/LXC/Status.pm
index f7e3128..ecd77e5 100644
--- a/src/PVE/API2/LXC/Status.pm
+++ b/src/PVE/API2/LXC/Status.pm
@@ -150,9 +150,11 @@ __PACKAGE__->register_method({
 	my $node = extract_param($param, 'node');
 	my $vmid = extract_param($param, 'vmid');
 
+	my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
+
 	my $skiplock = extract_param($param, 'skiplock');
-	raise_param_exc({ skiplock => "Only root may use this option." })
-	    if $skiplock && $authuser ne 'root@pam';
+	raise_param_exc({ skiplock => "Only superusers may use this option." })
+	    if $skiplock && !$is_superuser;
 
 	die "CT $vmid already running\n" if PVE::LXC::check_running($vmid);
 
@@ -234,9 +236,11 @@ __PACKAGE__->register_method({
 	my $node = extract_param($param, 'node');
 	my $vmid = extract_param($param, 'vmid');
 
+	my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
+
 	my $skiplock = extract_param($param, 'skiplock');
-	raise_param_exc({ skiplock => "Only root may use this option." })
-	    if $skiplock && $authuser ne 'root@pam';
+	raise_param_exc({ skiplock => "Only superusers may use this option." })
+	    if $skiplock && !$is_superuser;
 
 	die "CT $vmid not running\n" if !PVE::LXC::check_running($vmid);
 
diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index fe63087..c93b314 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -1254,7 +1254,10 @@ sub template_create {
 sub check_ct_modify_config_perm {
     my ($rpcenv, $authuser, $vmid, $pool, $oldconf, $newconf, $delete, $unprivileged) = @_;
 
-    return 1 if $authuser eq 'root@pam';
+    return 1 if $authuser eq 'root@pam'; # early exit for root@pam
+
+    my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
+
     my $storage_cfg = PVE::Storage::config();
 
     my $check = sub {
@@ -1265,8 +1268,8 @@ sub check_ct_modify_config_perm {
 	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.Disk']);
 	    return if $delete;
 	    my $data = PVE::LXC::Config->parse_volume($opt, $newconf->{$opt});
-	    raise_perm_exc("mount point type $data->{type} is only allowed for root\@pam")
-		if $data->{type} ne 'volume';
+	    raise_perm_exc("mount point type $data->{type} is only allowed for superusers")
+		if $data->{type} ne 'volume' && !$is_superuser;
 	    my $volid = $data->{volume};
 	    if ($volid =~ $NEW_DISK_RE) {
 		my $sid = $1;
@@ -1287,8 +1290,8 @@ sub check_ct_modify_config_perm {
 		 $opt eq 'searchdomain' || $opt eq 'hostname') {
 	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.Network']);
 	} elsif ($opt eq 'features') {
-	    raise_perm_exc("changing feature flags for privileged container is only allowed for root\@pam")
-		if !$unprivileged;
+	    raise_perm_exc("changing feature flags for privileged container is only allowed for superusers")
+		if !$unprivileged && !$is_superuser;
 
 	    my $nesting_changed = 0;
 	    my $other_changed = 0;
@@ -1326,13 +1329,13 @@ sub check_ct_modify_config_perm {
 		    $other_changed = 1;
 		}
 	    }
-	    raise_perm_exc("changing feature flags (except nesting) is only allowed for root\@pam")
-		if $other_changed;
+	    raise_perm_exc("changing feature flags (except nesting) is only allowed for superusers")
+		if $other_changed && !$is_superuser;
 	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Allocate'])
 		if $nesting_changed;
 	} elsif ($opt eq 'hookscript') {
-	    # For now this is restricted to root@pam
-	    raise_perm_exc("changing the hookscript is only allowed for root\@pam");
+	    raise_perm_exc("changing the hookscript is only allowed for superusers")
+		if !$is_superuser;
 	} else {
 	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.Options']);
 	}
diff --git a/src/PVE/LXC/Create.pm b/src/PVE/LXC/Create.pm
index 460df57..4602678 100644
--- a/src/PVE/LXC/Create.pm
+++ b/src/PVE/LXC/Create.pm
@@ -331,7 +331,7 @@ sub sanitize_and_merge_config {
 	if ($key eq 'lxc' && $restricted) {
 	    my $lxc_list = $oldconf->{'lxc'};
 
-	    my $msg = "skipping custom lxc options, restore manually as root:\n";
+	    my $msg = "skipping custom lxc options, restore manually as superuser:\n";
 	    $msg .= "--------------------------------\n";
 	    foreach my $lxc_opt (@$lxc_list) {
 		$msg .= "$lxc_opt->[0]: $lxc_opt->[1]\n"
-- 
2.30.2

[pve-devel] [PATCH v4 storage 17/18] check_volume_access: allow superusers to pass arbitrary fs paths

[Oguz Bektas]

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
 PVE/Storage.pm | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index 72458cf..f6da63d 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -510,6 +510,11 @@ sub parse_volume_id {
 sub check_volume_access {
     my ($rpcenv, $user, $cfg, $vmid, $volid, $type) = @_;
 
+    return if $user eq 'root@pam'; # root@pam always OK
+
+    # SU on '/' needed for passing arbitrary filesystem paths
+    my $is_superuser = $rpcenv->check($user, "/", ['SuperUser'], 1);
+
     my ($sid, $volname) = parse_volume_id($volid, 1);
     if ($sid) {
 	my ($vtype, undef, $ownervm) = parse_volname($cfg, $volid);
@@ -535,8 +540,8 @@ sub check_volume_access {
 	    die "missing privileges to access $volid\n";
 	}
     } else {
-	die "Only root can pass arbitrary filesystem paths."
-	    if $user ne 'root@pam';
+	die "Only superusers can pass arbitrary filesystem paths."
+	    if !$is_superuser;
     }
 
     return undef;
-- 
2.30.2

[pve-devel] [PATCH v4 docs 18/18] pveum: add SU privilege and SA role

[Oguz Bektas]

with some warnings about imposed restrictions and the danger of giving
this role/privilege to untrusted users.

Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
 pveum.adoc | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/pveum.adoc b/pveum.adoc
index 840067e..8067984 100644
--- a/pveum.adoc
+++ b/pveum.adoc
@@ -705,7 +705,11 @@ Roles
 A role is simply a list of privileges. Proxmox VE comes with a number
 of predefined roles, which satisfy most requirements.
 
-* `Administrator`: has full privileges
+* `SuperAdministrator`: has **full** privileges including `SuperUser`
+* `Administrator`: has all privileges **except** `SuperUser`
+
+NOTE: `SuperAdministrator` role is equivalent to 'root@pam'! Do not give this role to untrusted users.
+
 * `NoAccess`: has no privileges (used to forbid access)
 * `PVEAdmin`: can do most tasks, but has no rights to modify system settings (`Sys.PowerMgmt`, `Sys.Modify`, `Realm.Allocate`)
 * `PVEAuditor`: has read only access
@@ -748,6 +752,14 @@ We currently support the following privileges:
 
 Node / System related privileges::
 
+* `SuperUser`: modify root-only configuration options (warning! **do
+not give this privilege to untrusted users**)
+
+NOTE: `SuperUser` privilege by itself does not equal the access level of 'root@pam'.
+
+NOTE: Certain actions on users with the `SuperUser` privilege are restricted to others
+with `SuperUser`, i.e. changing their password or two-factor-authentication settings
+
 * `Permissions.Modify`: modify access permissions
 * `Sys.PowerMgmt`: node power management (start, stop, reset, shutdown, ...)
 * `Sys.Console`: console access to node
-- 
2.30.2

Re: [pve-devel] [PATCH v4 docs 18/18] pveum: add SU privilege and SA role

[Fabian Grünbichler]

On June 2, 2022 9:24 am, Oguz Bektas wrote:
> with some warnings about imposed restrictions and the danger of giving
> this role/privilege to untrusted users.

this should probably have a warning about giving whole groups SuperUser 
privileges, since anybody able to add users to that group (which does 
not require SU) can give themselves SU that way. unfortunately groups 
are not a proper entity that we can query privs for, so this is hard to 
check/guard against reliably/in a future proof fashion.

something like this maybe?

 Be careful to restrict access to groups with `SuperUser` privileges - 
 anybody who can modify such a group can give themselves `SuperUser` 
 access, without the group modification itself requiring it!

> 
> Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
>  pveum.adoc | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/pveum.adoc b/pveum.adoc
> index 840067e..8067984 100644
> --- a/pveum.adoc
> +++ b/pveum.adoc
> @@ -705,7 +705,11 @@ Roles
>  A role is simply a list of privileges. Proxmox VE comes with a number
>  of predefined roles, which satisfy most requirements.
>  
> -* `Administrator`: has full privileges
> +* `SuperAdministrator`: has **full** privileges including `SuperUser`
> +* `Administrator`: has all privileges **except** `SuperUser`
> +
> +NOTE: `SuperAdministrator` role is equivalent to 'root@pam'! Do not give this role to untrusted users.

should be warning likely?

> +
>  * `NoAccess`: has no privileges (used to forbid access)
>  * `PVEAdmin`: can do most tasks, but has no rights to modify system settings (`Sys.PowerMgmt`, `Sys.Modify`, `Realm.Allocate`)
>  * `PVEAuditor`: has read only access
> @@ -748,6 +752,14 @@ We currently support the following privileges:
>  
>  Node / System related privileges::
>  
> +* `SuperUser`: modify root-only configuration options (warning! **do
> +not give this privilege to untrusted users**)

should be a proper warning? and, as discussed, `SuperUser` should be its 
own section (also, the warnings/notes would look weird otherwise/break 
formatting).

> +
> +NOTE: `SuperUser` privilege by itself does not equal the access level of 'root@pam'.
> +
> +NOTE: Certain actions on users with the `SuperUser` privilege are restricted to others
> +with `SuperUser`, i.e. changing their password or two-factor-authentication settings
> +
>  * `Permissions.Modify`: modify access permissions
>  * `Sys.PowerMgmt`: node power management (start, stop, reset, shutdown, ...)
>  * `Sys.Console`: console access to node
> -- 
> 2.30.2
> 
> 

Re: [pve-devel] [PATCH v4 access-control++ 00/18] SuperUser privilege

[Fabian Grünbichler]

On June 2, 2022 9:24 am, Oguz Bektas wrote:
> big thanks to Fabian G. for the earlier reviews :)
> 
> v3 was not reviewed but i thought i should rebase it to make it easier.
> i also noticed some things that weren't addressed or were
> incorrect, so those are hopefully fixed now.

sorry for the long delay!

> please note that the privilege columns of the role selector in widget-toolkit
> is broken (reverting commit 49275c6726e7bf40f6d79e7b62eb4ad490a75119 fixes
> that, the custom renderer broke the view but i wasn't able to come up with a
> fix besides that -- extjs is pretty weird)

is this fixed nowadays?

> 
> v3->v4
> * rebased to latest commits
> * fix incorrect check for login prompt in pve-manager for termproxy and friends
> * slightly changed warning message when checking propagate
> * show warning to user via raise_perm_exc() when changing passwords
> * mark shortcut in parse_backup_hints
> 
> 
> v2->v3:
> * fixed some incorrect checks in qemu-server
> * further restrict changing passwords and tfa settings for superusers
> * gui fix for lxc features
> * slight improvements to docs, added some notes
> 
> see the separate patches for further details :)
> 
> intentionally left out ceph and cluster-related endpoints for checking SU
> privileges:
> 
> * addnode
> * copy
> * create
> * delnode
> * index
> * join
> 

I can guess that addnode,delnode,create and join are the respective 
cluster endpoints (although for them the full API path or module would 
obviously make sense as well!)

copy and index could be anything?

> * destroyosd
> * createosd
> 

couldn't these also move to Sys.Modify like the rest of disk-management?

> and some other endpoints:
> * register_account
> * deactivate_account
> 

these are ACME I guess? why would they need to be SU only?

> i left these endpoints alone since we have plans to introduce separate
> privileges for cluster-related actions in the future.

some indication which parts you actively tested would be nice as well.

please don't forget to re-check the API tree and other code for any 
root@pam-only stuff that might have been added in the meantime!

>  access-control: Oguz Bektas (5):
>   add "SuperAdministrator" role with the new "SuperUser" privilege
>   RPC env: add SuperUser API permission for GUI capabilities
>   api: acl: only allow granting SU privilege if user already has it
>   api: roles: only allow modifying roles to add/remove SU if user has SU
>     themselves
>   api: allow superusers to edit tfa and password settings
> 
>  src/PVE/API2/ACL.pm           | 16 ++++++++++++++++
>  src/PVE/API2/AccessControl.pm | 24 ++++++++++++++----------
>  src/PVE/API2/Role.pm          | 21 +++++++++++++++++++++
>  src/PVE/API2/TFA.pm           | 16 +++++++++++++++-
>  src/PVE/AccessControl.pm      |  9 ++++++---
>  src/PVE/RPCEnvironment.pm     | 29 ++++++++++++++++++++++-------
>  6 files changed, 94 insertions(+), 21 deletions(-)
> 
>  qemu-server: Oguz Bektas (4):
>   api: allow SU privileged users to edit root-only options for VM
>     configs
>   migration tests: mock $rpcenv->check subroutine
>   api: allow superusers to use 'skiplock' option
>   parse_backup_hints: add comment for root shortcut and fix typos
> 
>  PVE/API2/Qemu.pm             | 133 +++++++++++++++++++++--------------
>  PVE/QemuServer.pm            |   6 +-
>  test/MigrationTest/QmMock.pm |   5 ++
>  3 files changed, 87 insertions(+), 57 deletions(-)
> 
>  manager: Oguz Bektas (6):
>   api: backup: allow SUs to use 'tmpdir', 'dumpdir' and 'script' options
>   api: vzdump: allow SUs to use 'bwlimit' and 'ionice' parameters
>   api: always drop to login prompt for non-root users on terminal proxy
>     calls
>   ui: include "SuperUser" in privilege selector
>   ui: lxc features: check for SU instead of 'root@pam'
>   ui: adapt sensible 'root@pam' checks to SU
> 
>  PVE/API2/Backup.pm                      | 11 +++++++----
>  PVE/API2/Nodes.pm                       | 11 ++++++++---
>  PVE/API2/VZDump.pm                      |  8 +++++---
>  www/manager6/form/PrivilegesSelector.js |  2 +-
>  www/manager6/lxc/Options.js             |  8 ++++++--
>  www/manager6/lxc/Resources.js           |  6 +++---
>  www/manager6/window/Migrate.js          |  4 ++--
>  7 files changed, 32 insertions(+), 18 deletions(-)
> 
>  container: Oguz Bektas (1):
>   fix #2582: api: add checks for 'SuperUser' privilege for root-only
>     options
> 
>  src/PVE/API2/LXC.pm        | 19 +++++++++----------
>  src/PVE/API2/LXC/Config.pm |  2 +-
>  src/PVE/API2/LXC/Status.pm | 12 ++++++++----
>  src/PVE/LXC.pm             | 21 ++++++++++++---------
>  src/PVE/LXC/Create.pm      |  2 +-
>  5 files changed, 31 insertions(+), 25 deletions(-)
> 
>  storage: Oguz Bektas (1):
>   check_volume_access: allow superusers to pass arbitrary fs paths
> 
>  PVE/Storage.pm | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
>  docs: Oguz Bektas (1):
>   pveum: add SU privilege and SA role
> 
>  pveum.adoc | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> -- 
> 2.30.2
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>