[pve-devel] [PATCH v1 access-control++ 0/5] SuperUser privilege

[pve-devel] [PATCH v1 access-control++ 0/5] SuperUser privilege

[Oguz Bektas]

"SuperUser" (henceforth referred as SU) privilege allows to give
root-only permissions to API users, enabling them to perform privileged
actions on behalf of root@pam.

this privilege is enabled by default for "root@pam", and also mapped
inside "SuperAdministrator" (referred as SA)

changes from RFC (thanks for the review fabian g.!):
* manager: allow SAs to see/edit certain things on GUI
* qemu-server: also check the required non-root
VM privileges along with the SU priv
* pve-container: adapted error messages, changed variable name to
"is_superuser" for better clarity (in comparison to prev. "is_root"
which is a bit confusing)
* access-control: TFA permissions adaptation for SAs


 access-control:

 Oguz Bektas (2):
  add "SuperAdministrator" role with the new "SuperUser" privilege
  tfa: allow superusers to edit root@pam tfa

 src/PVE/API2/TFA.pm       | 7 +++++--
 src/PVE/AccessControl.pm  | 9 ++++++---
 src/PVE/RPCEnvironment.pm | 2 +-
 3 files changed, 12 insertions(+), 6 deletions(-)

 container:

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

 src/PVE/API2/LXC.pm        | 13 ++++++-------
 src/PVE/API2/LXC/Status.pm |  8 ++++++--
 src/PVE/LXC.pm             |  9 ++++++---
 3 files changed, 18 insertions(+), 12 deletions(-)

 manager:

 Oguz Bektas (1):
  change 'root@pam' checks with 'SuperUser' capability check

 www/manager6/Utils.js          | 3 ++-
 www/manager6/dc/Config.js      | 2 +-
 www/manager6/dc/UserView.js    | 2 +-
 www/manager6/lxc/Options.js    | 2 +-
 www/manager6/lxc/Resources.js  | 2 +-
 www/manager6/node/Config.js    | 2 +-
 www/manager6/window/Migrate.js | 4 ++--
 7 files changed, 9 insertions(+), 8 deletions(-)

 qemu-server:

 Oguz Bektas (1):
  add SuperUser privilege checks for root-only options

 PVE/API2/Qemu.pm | 119 +++++++++++++++++++++++++++++------------------
 1 file changed, 73 insertions(+), 46 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH v1 access-control 1/5] add default "SuperAdministrator" role with the new "SuperUser" privilege

[Oguz Bektas]

we map all valid privileges to the "Administrator" role except "SuperUser".

"SuperAdministrator"/SA gets all valid privileges (including the new
"SuperUser"/SU priv), and 'root@pam' is assigned as an SA by default.

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 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 b1348fa..3afb311 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 v1 access-control 2/5] tfa: allow superusers to edit root@pam tfa

[Oguz Bektas]

users with the SU privilege are able to override the existing check for
'root@pam' when calling tfa-related endpoints of the API.

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

diff --git a/src/PVE/API2/TFA.pm b/src/PVE/API2/TFA.pm
index bee4dee..70555cc 100644
--- a/src/PVE/API2/TFA.pm
+++ b/src/PVE/API2/TFA.pm
@@ -102,13 +102,16 @@ my $TFA_UPDATE_INFO_SCHEMA = {
 my sub root_permission_check : prototype($$$$) {
     my ($rpcenv, $authuser, $userid, $password) = @_;
 
+    # authuser = the user making the change
+    # userid = the user to be changed
     ($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 = $authuser eq 'root@pam' || $rpcenv->check($authuser, "/access/groups", ['SuperUser'], 1);
+    raise_perm_exc() if $userid eq 'root@pam' && !$is_superuser; # root@pam can be only edited by himself or a SuperUser
 
     # Regular users need to confirm their password to change TFA settings.
-    if ($authuser ne 'root@pam') {
+    if ($authuser ne 'root@pam') { # we still do password confirmation for superusers like the other users
 	raise_param_exc({ 'password' => 'password is required to modify TFA data' })
 	    if !defined($password);
 
-- 
2.30.2

[pve-devel] [PATCH v1 container 3/5] fix #2582: api: add checks for 'SuperUser' privilege for root-only options

[Oguz Bektas]

this way we can allow non-root users to act as a SU on specific
root-only API paths by giving them the built-in SA role or a custom role
with the SU privilege included.

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
 src/PVE/API2/LXC.pm        | 13 ++++++-------
 src/PVE/API2/LXC/Status.pm |  8 ++++++--
 src/PVE/LXC.pm             |  9 ++++++---
 3 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 7573814..b24e405 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -295,7 +295,7 @@ __PACKAGE__->register_method({
 
 	my $conf = {};
 
-	my $is_root = $authuser eq 'root@pam';
+	my $is_superuser = $authuser eq 'root@pam' || $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
 
 	my $no_disk_param = {};
 	my $mp_param = {};
@@ -330,8 +330,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);
@@ -371,7 +371,7 @@ __PACKAGE__->register_method({
 			# 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});
@@ -405,8 +405,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 root\n" if !$is_superuser;
 
 				if ($mountpoint->{backup}) {
 				    warn "WARNING - unsupported configuration!\n";
@@ -447,7 +446,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 {
diff --git a/src/PVE/API2/LXC/Status.pm b/src/PVE/API2/LXC/Status.pm
index f7e3128..791cdc6 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 = $authuser eq 'root@pam' || $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';
+	    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 = $authuser eq 'root@pam' || $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';
+	    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 b07d986..6b5125c 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -1254,7 +1254,9 @@ 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';
+    my $is_superuser = $authuser eq 'root@pam' || $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser']);
+    return 1 if $is_superuser;
+
     my $storage_cfg = PVE::Storage::config();
 
     my $check = sub {
@@ -1320,12 +1322,13 @@ sub check_ct_modify_config_perm {
 		}
 	    }
 	    raise_perm_exc("changing feature flags (except nesting) is only allowed for root\@pam")
-		if $other_changed;
+		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 root\@pam")
+		if !$is_superuser;
 	} else {
 	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.Options']);
 	}
-- 
2.30.2

[pve-devel] [PATCH v1 manager 4/5] change 'root@pam' checks with 'SuperUser' capability check

[Oguz Bektas]

'root@pam' has the privilege by default (since it's an SA), so we can
drop the string comparisons all around and check that privilege instead
when deciding to enable/disable buttons or views

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
 www/manager6/Utils.js          | 3 ++-
 www/manager6/dc/Config.js      | 2 +-
 www/manager6/dc/UserView.js    | 2 +-
 www/manager6/lxc/Options.js    | 2 +-
 www/manager6/lxc/Resources.js  | 2 +-
 www/manager6/node/Config.js    | 2 +-
 www/manager6/window/Migrate.js | 4 ++--
 7 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index aafe359a..31ab94e8 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -1656,7 +1656,8 @@ Ext.define('PVE.Utils', {
 
     showCephInstallOrMask: function(container, msg, nodename, callback) {
 	if (msg.match(/not (installed|initialized)/i)) {
-	    if (Proxmox.UserName === 'root@pam') {
+	    let caps = Ext.state.Manager.get('GuiCap');
+	    if (caps.node.SuperUser) {
 		container.el.mask();
 		if (!container.down('pveCephInstallWindow')) {
 		    var isInstalled = !!msg.match(/not initialized/i);
diff --git a/www/manager6/dc/Config.js b/www/manager6/dc/Config.js
index 9c54b19d..917c426f 100644
--- a/www/manager6/dc/Config.js
+++ b/www/manager6/dc/Config.js
@@ -197,7 +197,7 @@ Ext.define('PVE.dc.Config', {
 		});
 	    }
 
-	    if (Proxmox.UserName === 'root@pam') {
+	    if (caps.dc.SuperUser) {
 		me.items.push({
 		    xtype: 'pveACMEClusterView',
 		    title: 'ACME',
diff --git a/www/manager6/dc/UserView.js b/www/manager6/dc/UserView.js
index bbfc4f7c..fe0c0149 100644
--- a/www/manager6/dc/UserView.js
+++ b/www/manager6/dc/UserView.js
@@ -29,7 +29,7 @@ Ext.define('PVE.dc.UserView', {
 	    selModel: sm,
 	    baseurl: '/access/users/',
 	    dangerous: true,
-	    enableFn: rec => caps.access['User.Modify'] && rec.data.userid !== 'root@pam',
+	    enableFn: rec => caps.access['User.Modify'] && !caps.access.SuperUser,
 	    callback: () => reload(),
 	});
 	let run_editor = function() {
diff --git a/www/manager6/lxc/Options.js b/www/manager6/lxc/Options.js
index f2661dfc..f8eb8a5c 100644
--- a/www/manager6/lxc/Options.js
+++ b/www/manager6/lxc/Options.js
@@ -136,7 +136,7 @@ Ext.define('PVE.lxc.Options', {
 	    features: {
 		header: gettext('Features'),
 		defaultValue: Proxmox.Utils.noneText,
-		editor: Proxmox.UserName === 'root@pam' || caps.vms['VM.Allocate']
+		editor: caps.vms.SuperUser || caps.vms['VM.Allocate']
 		    ? 'PVE.lxc.FeaturesEdit' : undefined,
 	    },
 	    hookscript: {
diff --git a/www/manager6/lxc/Resources.js b/www/manager6/lxc/Resources.js
index 15ee3c67..2081b4a2 100644
--- a/www/manager6/lxc/Resources.js
+++ b/www/manager6/lxc/Resources.js
@@ -257,7 +257,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+$/)) {
 		var mp = PVE.Parser.parseLxcMountPoint(value);
 		if (mp.type !== 'volume') {
 		    noedit = true;
diff --git a/www/manager6/node/Config.js b/www/manager6/node/Config.js
index 68f80391..9f49f0dd 100644
--- a/www/manager6/node/Config.js
+++ b/www/manager6/node/Config.js
@@ -236,7 +236,7 @@ Ext.define('PVE.node.Config', {
 		    itemId: 'apt',
 		    upgradeBtn: {
 			xtype: 'pveConsoleButton',
-			disabled: Proxmox.UserName !== 'root@pam',
+			disabled: !caps.nodes.SuperUser,
 			text: gettext('Upgrade'),
 			consoleType: 'upgrade',
 			nodename: nodename,
diff --git a/www/manager6/window/Migrate.js b/www/manager6/window/Migrate.js
index 1c23abb3..20fcf81d 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) {
 		    return true;
 		} else {
 		    return false;
-- 
2.30.2

[pve-devel] [PATCH v1 qemu-server 5/5] add SuperUser privilege checks for root-only options

[Oguz Bektas]

analogous to the changes in container.

we now allow users with SU privilege to edit real device configurations,
provided that they also have the necessary VM privileges.

note that root@pam is still able to do everything as usual

---
 PVE/API2/Qemu.pm | 119 +++++++++++++++++++++++++++++------------------
 1 file changed, 73 insertions(+), 46 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 6992f6f..9d403b4 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -352,7 +352,7 @@ my $cloudinitoptions = {
 my $check_vm_create_serial_perm = sub {
     my ($rpcenv, $authuser, $vmid, $pool, $param) = @_;
 
-    return 1 if $authuser eq 'root@pam';
+    return 1 if $authuser eq 'root@pam' || $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
 
     foreach my $opt (keys %{$param}) {
 	next if $opt !~ m/^serial\d+$/;
@@ -370,7 +370,7 @@ my $check_vm_create_serial_perm = sub {
 my $check_vm_create_usb_perm = sub {
     my ($rpcenv, $authuser, $vmid, $pool, $param) = @_;
 
-    return 1 if $authuser eq 'root@pam';
+    return 1 if $authuser eq 'root@pam' || $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
 
     foreach my $opt (keys %{$param}) {
 	next if $opt !~ m/^usb\d+$/;
@@ -388,7 +388,7 @@ my $check_vm_create_usb_perm = sub {
 my $check_vm_modify_config_perm = sub {
     my ($rpcenv, $authuser, $vmid, $pool, $key_list) = @_;
 
-    return 1 if $authuser eq 'root@pam';
+    return 1 if $authuser eq 'root@pam' || $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
@@ -1117,9 +1117,10 @@ my $update_vm_api  = sub {
 	push @paramarr, "-$key", $value;
     }
 
+    my $is_superuser = $authuser eq 'root@pam' || $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');
 
@@ -1340,16 +1341,18 @@ my $update_vm_api  = sub {
 		} 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";
+		    } elsif (!$is_superuser) {
+			die "only superusers can delete '$opt' config for real devices\n"
+			    if !$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
 		    }
 		    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";
+		    } elsif (!$is_superuser) {
+			die "only superusers can delete '$opt' config for real devices\n"
+			    if !$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
 		    }
 		    PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
 		    PVE::QemuConfig->write_config($vmid, $conf);
@@ -1392,15 +1395,17 @@ 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";
+		    } elsif (!$is_superuser) {
+			die "only superuser can modify '$opt' config for real devices\n"
+			    if !$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
 		    }
 		    $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";
+		    } elsif (!$is_superuser) {
+			die "only superuser can modify '$opt' config for real devices\n"
+			    if !$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
 		    }
 		    $conf->{pending}->{$opt} = $param->{$opt};
 		} else {
@@ -1644,9 +1649,11 @@ __PACKAGE__->register_method({
 	my $authuser = $rpcenv->get_user();
 	my $vmid = $param->{vmid};
 
+	my $is_superuser = $authuser eq 'root@pam' || $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
@@ -2291,10 +2298,12 @@ __PACKAGE__->register_method({
 	my $machine = extract_param($param, 'machine');
 	my $force_cpu = extract_param($param, 'force-cpu');
 
+	my $is_superuser = $authuser eq 'root@pam' || $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
+
 	my $get_root_param = sub {
 	    my $value = extract_param($param, $_[0]);
-	    raise_param_exc({ "$_[0]" => "Only root may use this option." })
-		if $value && $authuser ne 'root@pam';
+	    raise_param_exc({ "$_[0]" => "Only superusers may use this option." })
+		if $value && !$is_superuser;
 	    return $value;
 	};
 
@@ -2436,17 +2445,19 @@ __PACKAGE__->register_method({
 	my $node = extract_param($param, 'node');
 	my $vmid = extract_param($param, 'vmid');
 
+	my $is_superuser = $authuser eq 'root@pam' || $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." })
-	    if $keepActive && $authuser ne 'root@pam';
+	raise_param_exc({ keepActive => "Only superusers may use this option." })
+	    if $keepActive && !$is_superuser;
 
 	my $migratedfrom = extract_param($param, 'migratedfrom');
-	raise_param_exc({ migratedfrom => "Only root may use this option." })
-	    if $migratedfrom && $authuser ne 'root@pam';
+	raise_param_exc({ migratedfrom => "Only superusers may use this option." })
+	    if $migratedfrom && !$is_superuser;
 
 
 	my $storecfg = PVE::Storage::config();
@@ -2513,9 +2524,11 @@ __PACKAGE__->register_method({
 
 	my $vmid = extract_param($param, 'vmid');
 
+	my $is_superuser = $authuser eq 'root@pam' || $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);
 
@@ -2580,13 +2593,15 @@ __PACKAGE__->register_method({
 	my $node = extract_param($param, 'node');
 	my $vmid = extract_param($param, 'vmid');
 
+	my $is_superuser = $authuser eq 'root@pam' || $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." })
-	    if $keepActive && $authuser ne 'root@pam';
+	raise_param_exc({ keepActive => "Only superusers may use this option." })
+	    if $keepActive && !$is_superuser;
 
 	my $storecfg = PVE::Storage::config();
 
@@ -2739,9 +2754,11 @@ __PACKAGE__->register_method({
 
 	my $statestorage = extract_param($param, 'statestorage');
 
+	my $is_superuser = $authuser eq 'root@pam' || $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);
 
@@ -2811,13 +2828,15 @@ __PACKAGE__->register_method({
 
 	my $vmid = extract_param($param, 'vmid');
 
+	my $is_superuser = $authuser eq 'root@pam' || $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." })
-	    if $nocheck && $authuser ne 'root@pam';
+	raise_param_exc({ nocheck => "Only superusers may use this option." })
+	    if $nocheck && !$is_superuser;
 
 	my $to_disk_suspended;
 	eval {
@@ -2883,9 +2902,11 @@ __PACKAGE__->register_method({
 
 	my $vmid = extract_param($param, 'vmid');
 
+	my $is_superuser = $authuser eq 'root@pam' || $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});
 
@@ -3392,6 +3413,8 @@ __PACKAGE__->register_method({
 
 	my $storecfg = PVE::Storage::config();
 
+	my $is_superuser = $authuser eq 'root@pam' || $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
+
 	my $move_updatefn =  sub {
 	    my $conf = PVE::QemuConfig->load_config($vmid);
 	    PVE::QemuConfig->check_lock($conf);
@@ -3856,7 +3879,7 @@ __PACKAGE__->register_method({
 	    },
 	    force => {
 		type => 'boolean',
-		description => "Allow to migrate VMs which use local devices. Only root may use this option.",
+		description => "Allow to migrate VMs which use local devices. Only superusers may use this option.",
 		optional => 1,
 	    },
 	    migration_type => {
@@ -3910,15 +3933,17 @@ __PACKAGE__->register_method({
 
 	my $vmid = extract_param($param, 'vmid');
 
-	raise_param_exc({ force => "Only root may use this option." })
-	    if $param->{force} && $authuser ne 'root@pam';
+	my $is_superuser = $authuser eq 'root@pam' || $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
 
-	raise_param_exc({ migration_type => "Only root may use this option." })
-	    if $param->{migration_type} && $authuser ne 'root@pam';
+	raise_param_exc({ force => "Only superusers may use this option." })
+	    if $param->{force} && !$is_superuser;
+
+	raise_param_exc({ migration_type => "Only superusers may use this option." })
+	    if $param->{migration_type} && !$is_superuser;
 
 	# allow root only until better network permissions are available
-	raise_param_exc({ migration_network => "Only root may use this option." })
-	    if $param->{migration_network} && $authuser ne 'root@pam';
+	raise_param_exc({ migration_network => "Only superusers may use this option." })
+	    if $param->{migration_network} && !$is_superuser;
 
 	# test if VM exists
 	my $conf = PVE::QemuConfig->load_config($vmid);
@@ -4098,9 +4123,11 @@ __PACKAGE__->register_method({
 
 	my $sizestr = extract_param($param, 'size');
 
+	my $is_superuser = $authuser eq 'root@pam' || $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 v1 access-control++ 0/5] SuperUser privilege

[Fabian Grünbichler]

see individual patches for in-depth review of changes, some broader 
remarks:
- I am missing info about parts you looked at but left as-is (and 
  rationale for why those parts stay root only)
- especially relevant would be a list of currently 
  unqualified/root-only-by-default API endpoints and actions taken or 
  not taken + reasons

e.g. some parts that are missing that I found with random grepping (in 
addition to stuff I mentioned in my other replies), where I have no idea 
whether you even look at them and decided they are okay as-is, or missed 
them:

- PVE::API2::Backup (tmpdir, dumpdir, script parameters)
- PVE::API2::VZDump (maxfiles prune-backups tmpdir dumpdir script bwlimit ionice)
- PVE::API2::ClusterConfig (we talked about that off-list, but some 
  mention in the actual patch series would still be good)
- PVE::Storage::check_volume_access , which is used in quite a few 
  places..

On February 8, 2022 2:10 pm, Oguz Bektas wrote:
> "SuperUser" (henceforth referred as SU) privilege allows to give
> root-only permissions to API users, enabling them to perform privileged
> actions on behalf of root@pam.
> 
> this privilege is enabled by default for "root@pam", and also mapped
> inside "SuperAdministrator" (referred as SA)
> 
> changes from RFC (thanks for the review fabian g.!):
> * manager: allow SAs to see/edit certain things on GUI
> * qemu-server: also check the required non-root
> VM privileges along with the SU priv
> * pve-container: adapted error messages, changed variable name to
> "is_superuser" for better clarity (in comparison to prev. "is_root"
> which is a bit confusing)
> * access-control: TFA permissions adaptation for SAs
> 
> 
>  access-control:
> 
>  Oguz Bektas (2):
>   add "SuperAdministrator" role with the new "SuperUser" privilege
>   tfa: allow superusers to edit root@pam tfa
> 
>  src/PVE/API2/TFA.pm       | 7 +++++--
>  src/PVE/AccessControl.pm  | 9 ++++++---
>  src/PVE/RPCEnvironment.pm | 2 +-
>  3 files changed, 12 insertions(+), 6 deletions(-)
> 
>  container:
> 
>  Oguz Bektas (1):
>   fix #2582: api: add checks for 'SuperUser' privilege for root-only
>     options
> 
>  src/PVE/API2/LXC.pm        | 13 ++++++-------
>  src/PVE/API2/LXC/Status.pm |  8 ++++++--
>  src/PVE/LXC.pm             |  9 ++++++---
>  3 files changed, 18 insertions(+), 12 deletions(-)
> 
>  manager:
> 
>  Oguz Bektas (1):
>   change 'root@pam' checks with 'SuperUser' capability check
> 
>  www/manager6/Utils.js          | 3 ++-
>  www/manager6/dc/Config.js      | 2 +-
>  www/manager6/dc/UserView.js    | 2 +-
>  www/manager6/lxc/Options.js    | 2 +-
>  www/manager6/lxc/Resources.js  | 2 +-
>  www/manager6/node/Config.js    | 2 +-
>  www/manager6/window/Migrate.js | 4 ++--
>  7 files changed, 9 insertions(+), 8 deletions(-)
> 
>  qemu-server:
> 
>  Oguz Bektas (1):
>   add SuperUser privilege checks for root-only options
> 
>  PVE/API2/Qemu.pm | 119 +++++++++++++++++++++++++++++------------------
>  1 file changed, 73 insertions(+), 46 deletions(-)
> 
> -- 
> 2.30.2
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [pve-devel] [PATCH v1 qemu-server 5/5] add SuperUser privilege checks for root-only options

[Fabian Grünbichler]

this needs a rebase anyhow, I reviewed it on HEAD~3 where it still 
applies ;)

there's one check for 'root@pam' that looks okay
same for the one in the move disk API endpoint

it might be worth it to mention that you checked those and not switched 
them to superuser since they are only a shortcut for skipping the 
permission check for root, and not limiting access to some root-only 
action (or maybe even add a comment so that future people looking at it 
don't have to remember that).

On February 8, 2022 2:10 pm, Oguz Bektas wrote:
> analogous to the changes in container.
> 
> we now allow users with SU privilege to edit real device configurations,
> provided that they also have the necessary VM privileges.
> 
> note that root@pam is still able to do everything as usual
> 
> ---
>  PVE/API2/Qemu.pm | 119 +++++++++++++++++++++++++++++------------------
>  1 file changed, 73 insertions(+), 46 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 6992f6f..9d403b4 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -352,7 +352,7 @@ my $cloudinitoptions = {
>  my $check_vm_create_serial_perm = sub {
>      my ($rpcenv, $authuser, $vmid, $pool, $param) = @_;
>  
> -    return 1 if $authuser eq 'root@pam';
> +    return 1 if $authuser eq 'root@pam' || $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);

nope - this still needs to check VM.Config.HWType in the SuperUser 
case..

e.g., should look like this:

my $check_vm_create_serial_perm = sub {
    my ($rpcenv, $authuser, $vmid, $pool, $param) = @_;

    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+$/;

	$rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.HWType']);
	die "only superusers can set '$opt' config for real devices\n"
	    if !($param->{$opt} eq 'socket' || $is_superuser);
    }

    return 1;
};

>  
>      foreach my $opt (keys %{$param}) {
>  	next if $opt !~ m/^serial\d+$/;
> @@ -370,7 +370,7 @@ my $check_vm_create_serial_perm = sub {
>  my $check_vm_create_usb_perm = sub {
>      my ($rpcenv, $authuser, $vmid, $pool, $param) = @_;
>  
> -    return 1 if $authuser eq 'root@pam';
> +    return 1 if $authuser eq 'root@pam' || $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);

same here, but with spice instead of socket

>  
>      foreach my $opt (keys %{$param}) {
>  	next if $opt !~ m/^usb\d+$/;
> @@ -388,7 +388,7 @@ my $check_vm_create_usb_perm = sub {
>  my $check_vm_modify_config_perm = sub {
>      my ($rpcenv, $authuser, $vmid, $pool, $key_list) = @_;
>  
> -    return 1 if $authuser eq 'root@pam';
> +    return 1 if $authuser eq 'root@pam' || $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);

completely wrong, like in pve-container! this effectively skips most 
checks for SuperUser, instead of only skipping the root-only parts..

only the else part of $check_vm_modify_config_perm should check for 
superuser and die otherwise..

>  
>      foreach my $opt (@$key_list) {
>  	# some checks (e.g., disk, serial port, usb) need to be done somewhere
> @@ -1117,9 +1117,10 @@ my $update_vm_api  = sub {
>  	push @paramarr, "-$key", $value;
>      }
>  
> +    my $is_superuser = $authuser eq 'root@pam' || $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');
>  
> @@ -1340,16 +1341,18 @@ my $update_vm_api  = sub {
>  		} 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";
> +		    } elsif (!$is_superuser) {
> +			die "only superusers can delete '$opt' config for real devices\n"
> +			    if !$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);

same as with check_vm_create_serial_perm above..

>  		    }
>  		    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";
> +		    } elsif (!$is_superuser) {
> +			die "only superusers can delete '$opt' config for real devices\n"
> +			    if !$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);

also same, but with check_vm_create_usb_perm..

>  		    }
>  		    PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
>  		    PVE::QemuConfig->write_config($vmid, $conf);
> @@ -1392,15 +1395,17 @@ 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";
> +		    } elsif (!$is_superuser) {
> +			die "only superuser can modify '$opt' config for real devices\n"
> +			    if !$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);

again

>  		    }
>  		    $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";
> +		    } elsif (!$is_superuser) {
> +			die "only superuser can modify '$opt' config for real devices\n"
> +			    if !$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);

again

>  		    }
>  		    $conf->{pending}->{$opt} = $param->{$opt};
>  		} else {
> @@ -1644,9 +1649,11 @@ __PACKAGE__->register_method({
>  	my $authuser = $rpcenv->get_user();
>  	my $vmid = $param->{vmid};
>  
> +	my $is_superuser = $authuser eq 'root@pam' || $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
> @@ -2291,10 +2298,12 @@ __PACKAGE__->register_method({
>  	my $machine = extract_param($param, 'machine');
>  	my $force_cpu = extract_param($param, 'force-cpu');
>  
> +	my $is_superuser = $authuser eq 'root@pam' || $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
> +
>  	my $get_root_param = sub {
>  	    my $value = extract_param($param, $_[0]);
> -	    raise_param_exc({ "$_[0]" => "Only root may use this option." })
> -		if $value && $authuser ne 'root@pam';
> +	    raise_param_exc({ "$_[0]" => "Only superusers may use this option." })
> +		if $value && !$is_superuser;
>  	    return $value;

this isn't 100% correct either - most of these are migration-internal 
parameters (which are only ever supposed to be set when called via SSH 
via `qm`, which runs as root@pam anyway)

skiplock can switch to being superuser-limited, the rest should stay 
root@pam..

>  	};
>  
> @@ -2436,17 +2445,19 @@ __PACKAGE__->register_method({
>  	my $node = extract_param($param, 'node');
>  	my $vmid = extract_param($param, 'vmid');
>  
> +	my $is_superuser = $authuser eq 'root@pam' || $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." })
> -	    if $keepActive && $authuser ne 'root@pam';
> +	raise_param_exc({ keepActive => "Only superusers may use this option." })
> +	    if $keepActive && !$is_superuser;

same here - internal param only set in vzdump context

>  
>  	my $migratedfrom = extract_param($param, 'migratedfrom');
> -	raise_param_exc({ migratedfrom => "Only root may use this option." })
> -	    if $migratedfrom && $authuser ne 'root@pam';
> +	raise_param_exc({ migratedfrom => "Only superusers may use this option." })
> +	    if $migratedfrom && !$is_superuser;

and this one is again only set when stopping the target VM during 
migration (via ssh, via qm, so always root).

>  
>  
>  	my $storecfg = PVE::Storage::config();
> @@ -2513,9 +2524,11 @@ __PACKAGE__->register_method({
>  
>  	my $vmid = extract_param($param, 'vmid');
>  
> +	my $is_superuser = $authuser eq 'root@pam' || $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);
>  
> @@ -2580,13 +2593,15 @@ __PACKAGE__->register_method({
>  	my $node = extract_param($param, 'node');
>  	my $vmid = extract_param($param, 'vmid');
>  
> +	my $is_superuser = $authuser eq 'root@pam' || $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." })
> -	    if $keepActive && $authuser ne 'root@pam';
> +	raise_param_exc({ keepActive => "Only superusers may use this option." })
> +	    if $keepActive && !$is_superuser;

same as above - AFAICT this is vzdump only..

>  
>  	my $storecfg = PVE::Storage::config();
>  
> @@ -2739,9 +2754,11 @@ __PACKAGE__->register_method({
>  
>  	my $statestorage = extract_param($param, 'statestorage');
>  
> +	my $is_superuser = $authuser eq 'root@pam' || $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);
>  
> @@ -2811,13 +2828,15 @@ __PACKAGE__->register_method({
>  
>  	my $vmid = extract_param($param, 'vmid');
>  
> +	my $is_superuser = $authuser eq 'root@pam' || $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." })
> -	    if $nocheck && $authuser ne 'root@pam';
> +	raise_param_exc({ nocheck => "Only superusers may use this option." })
> +	    if $nocheck && !$is_superuser;

this is also migration only (for resuming while the config is still on 
the source node)

>  
>  	my $to_disk_suspended;
>  	eval {
> @@ -2883,9 +2902,11 @@ __PACKAGE__->register_method({
>  
>  	my $vmid = extract_param($param, 'vmid');
>  
> +	my $is_superuser = $authuser eq 'root@pam' || $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});
>  
> @@ -3392,6 +3413,8 @@ __PACKAGE__->register_method({
>  
>  	my $storecfg = PVE::Storage::config();
>  
> +	my $is_superuser = $authuser eq 'root@pam' || $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);

not used?

> +
>  	my $move_updatefn =  sub {
>  	    my $conf = PVE::QemuConfig->load_config($vmid);
>  	    PVE::QemuConfig->check_lock($conf);
> @@ -3856,7 +3879,7 @@ __PACKAGE__->register_method({
>  	    },
>  	    force => {
>  		type => 'boolean',
> -		description => "Allow to migrate VMs which use local devices. Only root may use this option.",
> +		description => "Allow to migrate VMs which use local devices. Only superusers may use this option.",
>  		optional => 1,
>  	    },
>  	    migration_type => {
> @@ -3910,15 +3933,17 @@ __PACKAGE__->register_method({
>  
>  	my $vmid = extract_param($param, 'vmid');
>  
> -	raise_param_exc({ force => "Only root may use this option." })
> -	    if $param->{force} && $authuser ne 'root@pam';
> +	my $is_superuser = $authuser eq 'root@pam' || $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
>  
> -	raise_param_exc({ migration_type => "Only root may use this option." })
> -	    if $param->{migration_type} && $authuser ne 'root@pam';
> +	raise_param_exc({ force => "Only superusers may use this option." })
> +	    if $param->{force} && !$is_superuser;
> +
> +	raise_param_exc({ migration_type => "Only superusers may use this option." })
> +	    if $param->{migration_type} && !$is_superuser;
>  
>  	# allow root only until better network permissions are available
> -	raise_param_exc({ migration_network => "Only root may use this option." })
> -	    if $param->{migration_network} && $authuser ne 'root@pam';
> +	raise_param_exc({ migration_network => "Only superusers may use this option." })
> +	    if $param->{migration_network} && !$is_superuser;
>  
>  	# test if VM exists
>  	my $conf = PVE::QemuConfig->load_config($vmid);
> @@ -4098,9 +4123,11 @@ __PACKAGE__->register_method({
>  
>  	my $sizestr = extract_param($param, 'size');
>  
> +	my $is_superuser = $authuser eq 'root@pam' || $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 mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [pve-devel] [PATCH v1 manager 4/5] change 'root@pam' checks with 'SuperUser' capability check

[Fabian Grünbichler]

On February 8, 2022 2:10 pm, Oguz Bektas wrote:
> 'root@pam' has the privilege by default (since it's an SA), so we can
> drop the string comparisons all around and check that privilege instead
> when deciding to enable/disable buttons or views
> 
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
>  www/manager6/Utils.js          | 3 ++-
>  www/manager6/dc/Config.js      | 2 +-
>  www/manager6/dc/UserView.js    | 2 +-
>  www/manager6/lxc/Options.js    | 2 +-
>  www/manager6/lxc/Resources.js  | 2 +-
>  www/manager6/node/Config.js    | 2 +-
>  www/manager6/window/Migrate.js | 4 ++--
>  7 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
> index aafe359a..31ab94e8 100644
> --- a/www/manager6/Utils.js
> +++ b/www/manager6/Utils.js
> @@ -1656,7 +1656,8 @@ Ext.define('PVE.Utils', {
>  
>      showCephInstallOrMask: function(container, msg, nodename, callback) {
>  	if (msg.match(/not (installed|initialized)/i)) {
> -	    if (Proxmox.UserName === 'root@pam') {
> +	    let caps = Ext.state.Manager.get('GuiCap');
> +	    if (caps.node.SuperUser) {

but if you change this here, you also need to change the backend - as 
this is currently root-only (the API path called by the Ceph install 
wizard requires Sys.Console which is not a given just because you have 
SuperUser, and the ceph_install handling itself requires root@pam - the 
user is then presented with a login shell). so either this remains 
root-only for now (like the upgrade thing - both have the same problem 
after all!), but then please add a comment why or mention that in the 
commit message - or you find a good safe solution, then please argue why 
it is safe ;)

>  		container.el.mask();
>  		if (!container.down('pveCephInstallWindow')) {
>  		    var isInstalled = !!msg.match(/not initialized/i);
> diff --git a/www/manager6/dc/Config.js b/www/manager6/dc/Config.js
> index 9c54b19d..917c426f 100644
> --- a/www/manager6/dc/Config.js
> +++ b/www/manager6/dc/Config.js
> @@ -197,7 +197,7 @@ Ext.define('PVE.dc.Config', {
>  		});
>  	    }
>  
> -	    if (Proxmox.UserName === 'root@pam') {
> +	    if (caps.dc.SuperUser) {

the plugins actually require 'Sys.Modify', and the account endpoints are 
unqualified (root-only) or open to everyone (those returning 
schema/static stuff for front-end re-use) at the moment but that can 
probably be re-evaluated. in any case, switching this just in the 
front-end cannot work..

>  		me.items.push({
>  		    xtype: 'pveACMEClusterView',
>  		    title: 'ACME',
> diff --git a/www/manager6/dc/UserView.js b/www/manager6/dc/UserView.js
> index bbfc4f7c..fe0c0149 100644
> --- a/www/manager6/dc/UserView.js
> +++ b/www/manager6/dc/UserView.js
> @@ -29,7 +29,7 @@ Ext.define('PVE.dc.UserView', {
>  	    selModel: sm,
>  	    baseurl: '/access/users/',
>  	    dangerous: true,
> -	    enableFn: rec => caps.access['User.Modify'] && rec.data.userid !== 'root@pam',
> +	    enableFn: rec => caps.access['User.Modify'] && !caps.access.SuperUser,

no rationale given for the different way of accessing - I'll leave it to 
more JS affine reviewers to decide whether this is sensible or not, but 
please provide the reason WHY this doesn't use `caps.access['SuperUser']`

also, it's wrong - a SuperUser still requires User.Modify to modify 
users, so this either needs to stay as it is or simply drop the root@pam 
shortcut.

>  	    callback: () => reload(),
>  	});
>  	let run_editor = function() {
> diff --git a/www/manager6/lxc/Options.js b/www/manager6/lxc/Options.js
> index f2661dfc..f8eb8a5c 100644
> --- a/www/manager6/lxc/Options.js
> +++ b/www/manager6/lxc/Options.js
> @@ -136,7 +136,7 @@ Ext.define('PVE.lxc.Options', {
>  	    features: {
>  		header: gettext('Features'),
>  		defaultValue: Proxmox.Utils.noneText,
> -		editor: Proxmox.UserName === 'root@pam' || caps.vms['VM.Allocate']
> +		editor: caps.vms.SuperUser || caps.vms['VM.Allocate']
>  		    ? 'PVE.lxc.FeaturesEdit' : undefined,
>  	    },
>  	    hookscript: {
> diff --git a/www/manager6/lxc/Resources.js b/www/manager6/lxc/Resources.js
> index 15ee3c67..2081b4a2 100644
> --- a/www/manager6/lxc/Resources.js
> +++ b/www/manager6/lxc/Resources.js
> @@ -257,7 +257,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+$/)) {
>  		var mp = PVE.Parser.parseLxcMountPoint(value);
>  		if (mp.type !== 'volume') {
>  		    noedit = true;
> diff --git a/www/manager6/node/Config.js b/www/manager6/node/Config.js
> index 68f80391..9f49f0dd 100644
> --- a/www/manager6/node/Config.js
> +++ b/www/manager6/node/Config.js
> @@ -236,7 +236,7 @@ Ext.define('PVE.node.Config', {
>  		    itemId: 'apt',
>  		    upgradeBtn: {
>  			xtype: 'pveConsoleButton',
> -			disabled: Proxmox.UserName !== 'root@pam',
> +			disabled: !caps.nodes.SuperUser,

we discussed this in depth and said we'll keep the upgrade console root 
only for now.. also, the backend isn't change to allow this for 
SuperUser so it's moot anyway?

>  			text: gettext('Upgrade'),
>  			consoleType: 'upgrade',
>  			nodename: nodename,
> diff --git a/www/manager6/window/Migrate.js b/www/manager6/window/Migrate.js
> index 1c23abb3..20fcf81d 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) {
>  		    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
> 
> 
> 

Re: [pve-devel] [PATCH v1 container 3/5] fix #2582: api: add checks for 'SuperUser' privilege for root-only options

[Fabian Grünbichler]

On February 8, 2022 2:10 pm, Oguz Bektas wrote:
> this way we can allow non-root users to act as a SU on specific
> root-only API paths by giving them the built-in SA role or a custom role
> with the SU privilege included.
> 
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
>  src/PVE/API2/LXC.pm        | 13 ++++++-------
>  src/PVE/API2/LXC/Status.pm |  8 ++++++--
>  src/PVE/LXC.pm             |  9 ++++++---
>  3 files changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> index 7573814..b24e405 100644
> --- a/src/PVE/API2/LXC.pm
> +++ b/src/PVE/API2/LXC.pm
> @@ -295,7 +295,7 @@ __PACKAGE__->register_method({
>  
>  	my $conf = {};
>  
> -	my $is_root = $authuser eq 'root@pam';
> +	my $is_superuser = $authuser eq 'root@pam' || $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
>  
>  	my $no_disk_param = {};
>  	my $mp_param = {};
> @@ -330,8 +330,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;

schema still has root-only in the description, both here and in the 
update path..

>  	    } else {
>  		my ($sid, $volname) = PVE::Storage::parse_volume_id($volid);
>  		&$check_and_activate_storage($sid);
> @@ -371,7 +371,7 @@ __PACKAGE__->register_method({
>  			# 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});
> @@ -405,8 +405,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 root\n" if !$is_superuser;

still references root

>  
>  				if ($mountpoint->{backup}) {
>  				    warn "WARNING - unsupported configuration!\n";
> @@ -447,7 +446,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 {
> diff --git a/src/PVE/API2/LXC/Status.pm b/src/PVE/API2/LXC/Status.pm
> index f7e3128..791cdc6 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 = $authuser eq 'root@pam' || $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
> +
>  	my $skiplock = extract_param($param, 'skiplock');
>  	raise_param_exc({ skiplock => "Only root may use this option." })

nit: message needs updating (repeated a few times below)

> -	    if $skiplock && $authuser ne 'root@pam';
> +	    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 = $authuser eq 'root@pam' || $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';
> +	    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 b07d986..6b5125c 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -1254,7 +1254,9 @@ 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';
> +    my $is_superuser = $authuser eq 'root@pam' || $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser']);
> +    return 1 if $is_superuser;

so.. 'SuperUser' on its own should only skip those parts where root@pam 
was previously a requirement on top of other checks, but instead 
'SuperUser' can now modify the config however even with no other privs?

> +
>      my $storage_cfg = PVE::Storage::config();
>  
>      my $check = sub {
> @@ -1320,12 +1322,13 @@ sub check_ct_modify_config_perm {

here there is the non-volume part that is still root-only, with no 
comment about why that one is not switched to 'SuperUser'?

also, features for privileged containers is still root-only (should 
probably require VM.Allocate + SuperUser, when looking at the 
create/restore path).

>  		}
>  	    }
>  	    raise_perm_exc("changing feature flags (except nesting) is only allowed for root\@pam")

wrong message now

> -		if $other_changed;
> +		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 root\@pam")
> +		if !$is_superuser;

comment, message and code don't agree

>  	} else {
>  	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.Options']);
>  	}
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [pve-devel] [PATCH v1 access-control 2/5] tfa: allow superusers to edit root@pam tfa

[Fabian Grünbichler]

On February 8, 2022 2:10 pm, Oguz Bektas wrote:
> users with the SU privilege are able to override the existing check for
> 'root@pam' when calling tfa-related endpoints of the API.
> 
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
>  src/PVE/API2/TFA.pm | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/src/PVE/API2/TFA.pm b/src/PVE/API2/TFA.pm
> index bee4dee..70555cc 100644
> --- a/src/PVE/API2/TFA.pm
> +++ b/src/PVE/API2/TFA.pm
> @@ -102,13 +102,16 @@ my $TFA_UPDATE_INFO_SCHEMA = {
>  my sub root_permission_check : prototype($$$$) {
>      my ($rpcenv, $authuser, $userid, $password) = @_;
>  
> +    # authuser = the user making the change
> +    # userid = the user to be changed
>      ($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 = $authuser eq 'root@pam' || $rpcenv->check($authuser, "/access/groups", ['SuperUser'], 1);
> +    raise_perm_exc() if $userid eq 'root@pam' && !$is_superuser; # root@pam can be only edited by himself or a SuperUser

can be shorted (root@pam is by definition a SuperUser), but IMHO, the 
whole comment can be dropped - the variables have meaningful names 
anyway?

some rationale why this was changed, but the very similar code in 
PVE:API2::AccessControl->change_password is not adapted is also missing. 

also, a similar mechanism exists in the other direction as well: 
root@pam can create tickets for other users - should SuperUser also be 
allowed to do that? if not, the reason for that exception should be 
added to the relevant part as comment (or if it should, then the 
relevant changes should be part of the series ;))

one more point - check_api2_permissions has the general 'no permission 
specified in schema => only root@pam allowed' rule. obviously with the 
semantics of 'superuser does not entail all privileges' that can't be 
switched to superuser - but it means all the existing API calls without 
specific permissions need to be checked and either adapted (e.g., with 
an appropriate ACL with regular privilege + SuperUser), or marked as 
root only with a reason. one way to do this would be to analyze the API 
dump, or even forbid endpoints without a permission in the schema (e.g. 
in the api verification code we run at build time), and switch the 
remaining root-only endpoints to just have an explicit permission in the 
schema that encodes that. obviously in any case the check in 
check_api2_permissions needs to remain to catch anything that slipped 
through.

>  
>      # Regular users need to confirm their password to change TFA settings.
> -    if ($authuser ne 'root@pam') {
> +    if ($authuser ne 'root@pam') { # we still do password confirmation for superusers like the other users
>  	raise_param_exc({ 'password' => 'password is required to modify TFA data' })
>  	    if !defined($password);
>  
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [pve-devel] [PATCH v1 manager 4/5] change 'root@pam' checks with 'SuperUser' capability check

[Dominik Csapak]

On 2/10/22 16:29, Fabian Grünbichler wrote:
>>   		me.items.push({
>>   		    xtype: 'pveACMEClusterView',
>>   		    title: 'ACME',
>> diff --git a/www/manager6/dc/UserView.js b/www/manager6/dc/UserView.js
>> index bbfc4f7c..fe0c0149 100644
>> --- a/www/manager6/dc/UserView.js
>> +++ b/www/manager6/dc/UserView.js
>> @@ -29,7 +29,7 @@ Ext.define('PVE.dc.UserView', {
>>   	    selModel: sm,
>>   	    baseurl: '/access/users/',
>>   	    dangerous: true,
>> -	    enableFn: rec => caps.access['User.Modify'] && rec.data.userid !== 'root@pam',
>> +	    enableFn: rec => caps.access['User.Modify'] && !caps.access.SuperUser,
> no rationale given for the different way of accessing - I'll leave it to
> more JS affine reviewers to decide whether this is sensible or not, but
> please provide the reason WHY this doesn't use `caps.access['SuperUser']`

just to give the reason:

eslint complains if we use foo['bar'] when we could use foo.bar

we prefer 'dot-notation' (so foo.bar.baz) for objects, but in
all other cases of the caps object that does not work because
the keys themselves contain a '.'


> 
> also, it's wrong - a SuperUser still requires User.Modify to modify
> users, so this either needs to stay as it is or simply drop the root@pam
> shortcut.
> 

Re: [pve-devel] [PATCH v1 manager 4/5] change 'root@pam' checks with 'SuperUser' capability check

[Thomas Lamprecht]

On 25/02/2022 11:13, Dominik Csapak wrote:
>>> -        enableFn: rec => caps.access['User.Modify'] && rec.data.userid !== 'root@pam',
>>> +        enableFn: rec => caps.access['User.Modify'] && !caps.access.SuperUser,
>> no rationale given for the different way of accessing - I'll leave it to
>> more JS affine reviewers to decide whether this is sensible or not, but
>> please provide the reason WHY this doesn't use `caps.access['SuperUser']`
> 
> just to give the reason:
> 
> eslint complains if we use foo['bar'] when we could use foo.bar
> 
> we prefer 'dot-notation' (so foo.bar.baz) for objects, but in
> all other cases of the caps object that does not work because
> the keys themselves contain a '.'

one just add a comment to tell eslint to ignore that one in that case
though, else this may always look a bit weird to people reading over the
code by accident.