[pve-devel] [PATCH v3 access-control++ 00/17] SuperUser privilege

[pve-devel] [PATCH v3 access-control++ 00/17] SuperUser privilege

[Oguz Bektas]

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

* register_account
* deactivate_account

* destroyosd
* createosd

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


 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           | 17 +++++++++++++++++
 src/PVE/API2/AccessControl.pm | 23 +++++++++++++++--------
 src/PVE/API2/Role.pm          | 21 +++++++++++++++++++++
 src/PVE/API2/TFA.pm           | 11 ++++++++++-
 src/PVE/AccessControl.pm      |  9 ++++++---
 src/PVE/RPCEnvironment.pm     | 29 ++++++++++++++++++++++-------
 6 files changed, 91 insertions(+), 19 deletions(-)

 qemu-server: Oguz Bektas (3):
  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

 PVE/API2/Qemu.pm             | 133 +++++++++++++++++++++--------------
 test/MigrationTest/QmMock.pm |   5 ++
 2 files changed, 84 insertions(+), 54 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 show 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           |  2 +-
 www/manager6/window/Migrate.js          |  4 ++--
 7 files changed, 30 insertions(+), 16 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 | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

-- 
2.30.2

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

[Oguz Bektas]

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
v2->v3:
* no changes

 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 1306576..1137756 100644
--- a/src/PVE/AccessControl.pm
+++ b/src/PVE/AccessControl.pm
@@ -1008,6 +1008,7 @@ my $privgroups = {
     },
     Sys => {
 	root => [
+	    'SuperUser',
 	    'Sys.PowerMgmt',
 	    'Sys.Modify', # edit/change node settings
 	],
@@ -1074,7 +1075,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 {
@@ -1101,6 +1103,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();
@@ -1581,7 +1584,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, '');
@@ -1635,7 +1638,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 f11517f..cd4a0a5 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 v3 access-control 02/17] RPC env: add SuperUser API permission for GUI capabilities

[Oguz Bektas]

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
v2->v3:
* it was missing from v2 series

 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 cd4a0a5..34c8991 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 v3 access-control 03/17] api: acl: only allow granting SU privilege if user already has it

[Oguz Bektas]

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
v2->v3:
* also check for 'propagate' bit on $param->{path}

 src/PVE/API2/ACL.pm | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/src/PVE/API2/ACL.pm b/src/PVE/API2/ACL.pm
index 857c672..7ecf8f0 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,19 @@ __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;
+
+			# check if user has propagate bit
+			my $user_perms = $rpcenv->permissions($authuser, $param->{path});
+			my $has_propagate = $user_perms->{SuperUser};
+			die "cannot give away superuser on '$param->{path}' without 'propagate' bit!\n"
+			    if !$has_propagate;
+		    }
+
 		    foreach my $group (split_list($param->{groups})) {
 
 			die "group '$group' does not exist\n"
-- 
2.30.2

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

[Oguz Bektas]

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
v2->v3:
* also check if previous role contained SU privilege (when removing privilege)


 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 v3 access-control 05/17] api: allow superusers to edit tfa and password settings

[Oguz Bektas]

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
v2->v3:
* added `has_superuser_anywhere` helper
* only allow changing TFA or password settings of SUs if user has SU on /access endpoint


 src/PVE/API2/AccessControl.pm | 23 +++++++++++++++--------
 src/PVE/API2/TFA.pm           | 11 ++++++++++-
 src/PVE/RPCEnvironment.pm     | 15 +++++++++++++++
 3 files changed, 40 insertions(+), 9 deletions(-)

diff --git a/src/PVE/API2/AccessControl.pm b/src/PVE/API2/AccessControl.pm
index 5d78c6f..4ba9dc5 100644
--- a/src/PVE/API2/AccessControl.pm
+++ b/src/PVE/API2/AccessControl.pm
@@ -378,17 +378,24 @@ __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
+	} elsif ($authuser eq $userid) {
+	    $rpcenv->check_user_enabled($userid);
+	    # OK - each user can change its own password
 	} 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';
+	    # 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';
+
+	    if ($is_superuser) {
+		# OK - superusers can change any user's password except root@pam
+	    } elsif ($rpcenv->has_superuser_anywhere($userid)) {
+		die "only superusers can change another superuser's password!\n"
+		    if !$is_superuser;
 	    }
 	}
 
diff --git a/src/PVE/API2/TFA.pm b/src/PVE/API2/TFA.pm
index bee4dee..d7c0c4d 100644
--- a/src/PVE/API2/TFA.pm
+++ b/src/PVE/API2/TFA.pm
@@ -96,22 +96,31 @@ 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
+    raise_perm_exc() if $userid eq 'root@pam' && $authuser ne 'root@pam'; # can be only edited by itself
     ($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) {
+	    die "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 34c8991..04c5633 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

[pve-devel] [PATCH v3 qemu-server 06/17] 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 (such as `VM.Config.HWType`), as well as the SU
privilege.

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
v2->v3:
* changed condition styles
* fix inverted condition for real device configs to also check current value


 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 1dd0cf2..7fc9a77 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -365,16 +365,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;
@@ -383,16 +384,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;
@@ -401,8 +403,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
@@ -438,7 +443,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;
 	}
     }
 
@@ -1140,6 +1146,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';
@@ -1356,19 +1364,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 {
@@ -1418,18 +1422,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

[pve-devel] [PATCH v3 qemu-server 07/17] 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>
---
v2->v3:

* added new, since we need to run $rpcenv->check inside the unit tests for migration


 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 v3 qemu-server 08/17] 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.

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
v2->v3:
* mark migration-internal parameters inside param description
* added comment above get_root_param
* drop root@pam shortcuts and check SU privilege as normal


 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 7fc9a77..3eca222 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -1149,8 +1149,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');
 
@@ -1672,9 +1672,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
@@ -2277,25 +2279,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)',
@@ -2317,6 +2321,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." })
@@ -2325,7 +2337,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');
@@ -2463,9 +2474,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." })
@@ -2540,9 +2553,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);
 
@@ -2607,9 +2622,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." })
@@ -2766,9 +2783,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);
 
@@ -2838,9 +2857,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." })
@@ -2910,9 +2931,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});
 
@@ -4163,9 +4186,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 v3 manager 09/17] 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>
---
v2->v3:
* change comment for SU check


 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 v3 manager 10/17] api: vzdump: allow SUs to use 'bwlimit' and 'ionice' parameters

[Oguz Bektas]

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
v2->v3:
* no changes

 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..99366212 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 = $user eq 'root@pam' || $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 v3 manager 11/17] api: always show login prompt for non-root users on terminal proxy calls

[Oguz Bektas]

we have a SU privilege now, but we still drop to a login prompt on our
spice/vnc/termproxy for such users.

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
v2->v3:
* limit non-root users to 'login' shellcmd on vncproxy, termproxy and spiceproxy


 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..52dae854 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} eq '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} eq '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} eq 'login' && $user ne 'root@pam') {
 	    raise_perm_exc('user != root@pam');
 	}
 
-- 
2.30.2

[pve-devel] [PATCH v3 manager 12/17] ui: include "SuperUser" in privilege selector

[Oguz Bektas]

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
v2->v3:
* added, we get all the privileges for SuperAdministrator so that the priv selector includes SuperUser

 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 v3 manager 13/17] ui: lxc features: check for SU instead of 'root@pam'

[Oguz Bektas]

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
v2->v3:
* added new, previously the editor was still double-clickable eventhough the 'edit'
button was grayed out
* check for SU instead of root@pam

 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 v3 manager 14/17] ui: adapt sensible 'root@pam' checks to SU

[Oguz Bektas]

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
 www/manager6/lxc/Resources.js  | 2 +-
 www/manager6/window/Migrate.js | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/www/manager6/lxc/Resources.js b/www/manager6/lxc/Resources.js
index 683be526..85d3889e 100644
--- a/www/manager6/lxc/Resources.js
+++ b/www/manager6/lxc/Resources.js
@@ -268,7 +268,7 @@ Ext.define('PVE.lxc.RessourceView', {
 	    var isUsedDisk = isDisk && !isUnusedDisk;
 
 	    var noedit = rec.data.delete || !rowdef.editor;
-	    if (!noedit && Proxmox.UserName !== 'root@pam' && key.match(/^mp\d+$/)) {
+	    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] [PATCH v3 container 15/17] 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>
---
v2->v3:
* update comments
* drop shortcuts for 'root@pam', just check for SU privilege

 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 ea4827f..4517b99 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 {
@@ -2224,7 +2223,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
 
 	    if ($vmid eq $target_vmid) {
 		my $msg = "must be different than source VMID to move disk to another container";
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 v3 storage 16/17] check_volume_access: allow superusers to pass arbitrary fs paths

[Oguz Bektas]

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
v2->v3:
* no changes

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

diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index 3b86956..32d90b7 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -475,6 +475,11 @@ sub parse_volume_id {
 sub check_volume_access {
     my ($rpcenv, $user, $cfg, $vmid, $volid, $type) = @_;
 
+    return if $user eq 'root@pam'; # always OK
+
+    # SU on "/" path is 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);
@@ -500,8 +505,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 v3 docs 17/17] pveum: add SU privilege and SA role

[Oguz Bektas]

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
v2->v3:
* separate SU and SA from the general privileges list (since they're not categorized easily)
* put actual notes inside and warn about potential danger and limitations regarding the privilege/role

 pveum.adoc | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/pveum.adoc b/pveum.adoc
index a5c8906..60c357d 100644
--- a/pveum.adoc
+++ b/pveum.adoc
@@ -684,7 +684,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`
+
+NOTE: `SuperAdministrator` role is equivalent to 'root@pam', be careful when giving it to a user!
+
+* `Administrator`: has all privileges except `SuperUser`
 * `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
@@ -725,6 +729,12 @@ assigned to users and paths without being part of a role.
 
 We currently support the following privileges:
 
+* `SuperUser`: modify root-only configuration options (dangerous! don't give this privilege to untrusted users)
+
+NOTE: the `SuperUser` privilege alone is not enough to provide root-equivalent access to a user.
+
+NOTE: certain actions on users with this privilege are restricted, such as modifying password or 2FA settings.
+
 Node / System related privileges::
 
 * `Permissions.Modify`: modify access permissions
-- 
2.30.2