[pve-devel] [RFC access-control common container qemu-server 0/4] #2582: Sys.Root privilege

[pve-devel] [RFC access-control common container qemu-server 0/4] #2582: Sys.Root privilege

[Oguz Bektas]

sending this in as RFC, because i think it needs a bit of ironing
out and discussing the quirky bits ;)

i'm not done with pve-manager patch so that'll come with the v1, though API
should work for testing the changes.

it probably makes sense to also add a helper there, since at the moment
we only check if Proxmox.UserName === 'root@pam' or in some cases
specific permissions for storage and so forth, in order to decide
whether to show/enable some GUI elements.

container API already works pretty well. VM API should also work but i haven't
tested this extensively w.r.t. storage and migration.

some questions that popped up in my head:

+ should adding 'Sys.Root' privilege to a user give them all available
privileges on that path? this would make sense as having a
root-equivalent privilege should be already enough (otherwise it doesn't
have much of a point?). since at some places we have things like:

-------
        } elsif ($target_vmid) {
            $rpcenv->check_vm_perm($authuser, $target_vmid, undef, ['VM.Config.Disk'])
-               if $authuser ne 'root@pam';
+               if !$is_root;
-------

where one could theoretically have root-eq privs but not the 'VM.Config.Disk' for the target vm...


+ $authuser could also be an (optional?) parameter for the helper in
PVE::Tools, so that we could check arbitrary users and not only the current
one. also on most of these we already call $rpcenv->get_user() before doing the
check, so we could spare that call inside the helper if we do that
consistently.

+ would it make sense to be able to give 'Sys.Root' on a single node (like on
/nodes/foo instead of the whole cluster)? this seemed like a rabbithole to me
since we'd have to lock down quite a bit of stuff to limit movement from one
cluster member to the other, without any(?) worthwhile benefits? or might make
sense to just allow 'Sys.Root' to be given on '/' (since it should be
equivalent to root@pam anyway)

+ should root@pam have 'Sys.Root' by default? or does it make sense to still
differentiate the "real" root user and the "impersonated" one?





 pve-access-control:

 Oguz Bektas (1):
  add Sys.Root privilege

 src/PVE/AccessControl.pm | 1 +
 1 file changed, 1 insertion(+)

 pve-common:

 Oguz Bektas (1):
  tools: add 'check_for_root' helper

 src/PVE/Tools.pm | 9 +++++++++
 1 file changed, 9 insertions(+)

 pve-container:

 Oguz Bektas (1):
  fix #2582: api: use common helper for checking root privileges

 src/PVE/API2/LXC.pm | 5 ++---
 src/PVE/LXC.pm      | 8 +++++---
 2 files changed, 7 insertions(+), 6 deletions(-)

 qemu-server:

 Oguz Bektas (1):
  api: use common helper for checking root privileges

 PVE/API2/Qemu.pm | 74 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 49 insertions(+), 25 deletions(-)


-- 
2.30.2

[pve-devel] [RFC access-control 1/4] add Sys.Root privilege

[Oguz Bektas]

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
 src/PVE/AccessControl.pm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
index 1306576..b5e1094 100644
--- a/src/PVE/AccessControl.pm
+++ b/src/PVE/AccessControl.pm
@@ -1008,6 +1008,7 @@ my $privgroups = {
     },
     Sys => {
 	root => [
+	    'Sys.Root',
 	    'Sys.PowerMgmt',
 	    'Sys.Modify', # edit/change node settings
 	],
-- 
2.30.2

[pve-devel] [RFC common 2/4] tools: add 'check_for_root' helper

[Oguz Bektas]

checks if the rpcenv user is 'root@pam' or has the 'Sys.Root' privilege
on a given API path

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

diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
index 787942a..3e2e7f9 100644
--- a/src/PVE/Tools.pm
+++ b/src/PVE/Tools.pm
@@ -2017,4 +2017,13 @@ sub get_file_hash {
     return lc($digest);
 }
 
+sub check_for_root {
+    my ($path_to_check) = @_;
+
+    my $rpcenv = PVE::RPCEnvironment::get();
+    my $authuser = $rpcenv->get_user();
+    my $is_root = $authuser eq 'root@pam' || $rpcenv->check($authuser, $path_to_check, ['Sys.Root'], 1);
+    return $is_root;
+}
+
 1;
-- 
2.30.2

[pve-devel] [PATCH container 3/4] fix #2582: api: use common helper for checking root privileges

[Oguz Bektas]

we just check if the authenticated user has the 'Sys.Root' privilege on
a given path and set the $is_root variable accordingly if so.

tagged as fix for #2582 since with this series the usecase on the
bugreport should be fulfilled.

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

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 7573814..64ecc5f 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_root = PVE::Tools::check_for_root("/vms");
 
 	my $no_disk_param = {};
 	my $mp_param = {};
diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index b07d986..1844b04 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_root = PVE::Tools::check_for_root("/vms/$vmid");
+    return 1 if $is_root;
+
     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_root;
 	    $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_root;
 	} else {
 	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.Options']);
 	}
-- 
2.30.2

[pve-devel] [RFC qemu-server 4/4] api: use common helper for checking root privileges

[Oguz Bektas]

to allow API users with the 'Sys.Root' permission to call these
endpoints.

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
 PVE/API2/Qemu.pm | 74 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 49 insertions(+), 25 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 6992f6f..e846a1e 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 PVE::Tools::check_for_root("/vms/$vmid");
 
     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 PVE::Tools::check_for_root("/vms/$vmid");
 
     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 PVE::Tools::check_for_root("/vms/$vmid");
 
     foreach my $opt (@$key_list) {
 	# some checks (e.g., disk, serial port, usb) need to be done somewhere
@@ -1105,6 +1105,8 @@ my $update_vm_api  = sub {
 
     my $background_delay = extract_param($param, 'background_delay');
 
+    my $is_root = PVE::Tools::check_for_root("/vms/$vmid");
+
     if (defined(my $cipassword = $param->{cipassword})) {
 	# Same logic as in cloud-init (but with the regex fixed...)
 	$param->{cipassword} = PVE::Tools::encrypt_pw($cipassword)
@@ -1119,7 +1121,7 @@ my $update_vm_api  = sub {
 
     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_root;
 
     my $delete_str = extract_param($param, 'delete');
 
@@ -1340,7 +1342,7 @@ 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') {
+		    } elsif (!$is_root) {
 			die "only root can delete '$opt' config for real devices\n";
 		    }
 		    PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
@@ -1348,7 +1350,7 @@ my $update_vm_api  = sub {
 		} elsif ($opt =~ m/^usb\d+$/) {
 		    if ($val =~ m/spice/) {
 			$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
-		    } elsif ($authuser ne 'root@pam') {
+		    } elsif (!$is_root) {
 			die "only root can delete '$opt' config for real devices\n";
 		    }
 		    PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
@@ -1392,14 +1394,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') {
+		    } elsif (!$is_root) {
 			die "only root can modify '$opt' config for real devices\n";
 		    }
 		    $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') {
+		    } elsif (!$is_root) {
 			die "only root can modify '$opt' config for real devices\n";
 		    }
 		    $conf->{pending}->{$opt} = $param->{$opt};
@@ -1644,9 +1646,11 @@ __PACKAGE__->register_method({
 	my $authuser = $rpcenv->get_user();
 	my $vmid = $param->{vmid};
 
+	my $is_root = PVE::Tools::check_for_root("/vms/$vmid");
+
 	my $skiplock = $param->{skiplock};
 	raise_param_exc({ skiplock => "Only root may use this option." })
-	    if $skiplock && $authuser ne 'root@pam';
+	    if $skiplock && !$is_root;
 
 	my $early_checks = sub {
 	    # test if VM exists
@@ -2291,10 +2295,12 @@ __PACKAGE__->register_method({
 	my $machine = extract_param($param, 'machine');
 	my $force_cpu = extract_param($param, 'force-cpu');
 
+	my $is_root = PVE::Tools::check_for_root("/vms/$vmid");
+
 	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';
+		if $value && !$is_root;
 	    return $value;
 	};
 
@@ -2436,17 +2442,19 @@ __PACKAGE__->register_method({
 	my $node = extract_param($param, 'node');
 	my $vmid = extract_param($param, 'vmid');
 
+	my $is_root = PVE::Tools::check_for_root("/vms/$vmid");
+
 	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_root;
 
 	my $keepActive = extract_param($param, 'keepActive');
 	raise_param_exc({ keepActive => "Only root may use this option." })
-	    if $keepActive && $authuser ne 'root@pam';
+	    if $keepActive && !$is_root;
 
 	my $migratedfrom = extract_param($param, 'migratedfrom');
 	raise_param_exc({ migratedfrom => "Only root may use this option." })
-	    if $migratedfrom && $authuser ne 'root@pam';
+	    if $migratedfrom && !$is_root;
 
 
 	my $storecfg = PVE::Storage::config();
@@ -2513,9 +2521,11 @@ __PACKAGE__->register_method({
 
 	my $vmid = extract_param($param, 'vmid');
 
+	my $is_root = PVE::Tools::check_for_root("/vms/$vmid");
+
 	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_root;
 
 	die "VM $vmid not running\n" if !PVE::QemuServer::check_running($vmid);
 
@@ -2580,13 +2590,15 @@ __PACKAGE__->register_method({
 	my $node = extract_param($param, 'node');
 	my $vmid = extract_param($param, 'vmid');
 
+	my $is_root = PVE::Tools::check_for_root("/vms/$vmid");
+
 	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_root;
 
 	my $keepActive = extract_param($param, 'keepActive');
 	raise_param_exc({ keepActive => "Only root may use this option." })
-	    if $keepActive && $authuser ne 'root@pam';
+	    if $keepActive && !$is_root;
 
 	my $storecfg = PVE::Storage::config();
 
@@ -2735,13 +2747,15 @@ __PACKAGE__->register_method({
 	my $node = extract_param($param, 'node');
 	my $vmid = extract_param($param, 'vmid');
 
+	my $is_root = PVE::Tools::check_for_root("/vms/$vmid");
+
 	my $todisk = extract_param($param, 'todisk') // 0;
 
 	my $statestorage = extract_param($param, 'statestorage');
 
 	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_root;
 
 	die "VM $vmid not running\n" if !PVE::QemuServer::check_running($vmid);
 
@@ -2811,13 +2825,15 @@ __PACKAGE__->register_method({
 
 	my $vmid = extract_param($param, 'vmid');
 
+	my $is_root = PVE::Tools::check_for_root("/vms/$vmid");
+
 	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_root;
 
 	my $nocheck = extract_param($param, 'nocheck');
 	raise_param_exc({ nocheck => "Only root may use this option." })
-	    if $nocheck && $authuser ne 'root@pam';
+	    if $nocheck && !$is_root;
 
 	my $to_disk_suspended;
 	eval {
@@ -2883,9 +2899,11 @@ __PACKAGE__->register_method({
 
 	my $vmid = extract_param($param, 'vmid');
 
+	my $is_root = PVE::Tools::check_for_root("/vms/$vmid");
+
 	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_root;
 
 	PVE::QemuServer::vm_sendkey($vmid, $skiplock, $param->{key});
 
@@ -3392,6 +3410,8 @@ __PACKAGE__->register_method({
 
 	my $storecfg = PVE::Storage::config();
 
+	my $is_root = PVE::Tools::check_for_root("/vms/$vmid");
+
 	my $move_updatefn =  sub {
 	    my $conf = PVE::QemuConfig->load_config($vmid);
 	    PVE::QemuConfig->check_lock($conf);
@@ -3671,7 +3691,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 !$is_root;
 
 	    raise_param_exc({ 'target-vmid' => "must be different than source VMID to reassign disk" })
 		if $vmid eq $target_vmid;
@@ -3910,15 +3930,17 @@ __PACKAGE__->register_method({
 
 	my $vmid = extract_param($param, 'vmid');
 
+	my $is_root = PVE::Tools::check_for_root("/vms/$vmid");
+
 	raise_param_exc({ force => "Only root may use this option." })
-	    if $param->{force} && $authuser ne 'root@pam';
+	    if $param->{force} && !$is_root;
 
 	raise_param_exc({ migration_type => "Only root may use this option." })
-	    if $param->{migration_type} && $authuser ne 'root@pam';
+	    if $param->{migration_type} && !$is_root;
 
 	# 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';
+	    if $param->{migration_network} && !$is_root;
 
 	# test if VM exists
 	my $conf = PVE::QemuConfig->load_config($vmid);
@@ -4099,8 +4121,10 @@ __PACKAGE__->register_method({
 	my $sizestr = extract_param($param, 'size');
 
 	my $skiplock = extract_param($param, 'skiplock');
+	my $is_root = PVE::Tools::check_for_root("/vms/$vmid");
+
         raise_param_exc({ skiplock => "Only root may use this option." })
-            if $skiplock && $authuser ne 'root@pam';
+            if $skiplock && !$is_root;
 
         my $storecfg = PVE::Storage::config();
 
-- 
2.30.2

Re: [pve-devel] [RFC qemu-server 4/4] api: use common helper for checking root privileges

[Fabian Grünbichler]

this patch mixes a lot of "check for 'root@pam' as short cut" with 
"check for 'root@pam' for stuff limited to 'root@pam'". as per the 
discussion in the cover letter - if we make Sys.Root imply all other 
privileges like 'root@pam' currently does, then all the short cut 
instances can just remain as is..

On January 5, 2022 4:22 pm, Oguz Bektas wrote:
> to allow API users with the 'Sys.Root' permission to call these
> endpoints.
> 
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
>  PVE/API2/Qemu.pm | 74 ++++++++++++++++++++++++++++++++----------------
>  1 file changed, 49 insertions(+), 25 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 6992f6f..e846a1e 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 PVE::Tools::check_for_root("/vms/$vmid");
>  
>      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 PVE::Tools::check_for_root("/vms/$vmid");
>  
>      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 PVE::Tools::check_for_root("/vms/$vmid");
>  
>      foreach my $opt (@$key_list) {
>  	# some checks (e.g., disk, serial port, usb) need to be done somewhere
> @@ -1105,6 +1105,8 @@ my $update_vm_api  = sub {
>  
>      my $background_delay = extract_param($param, 'background_delay');
>  
> +    my $is_root = PVE::Tools::check_for_root("/vms/$vmid");
> +
>      if (defined(my $cipassword = $param->{cipassword})) {
>  	# Same logic as in cloud-init (but with the regex fixed...)
>  	$param->{cipassword} = PVE::Tools::encrypt_pw($cipassword)
> @@ -1119,7 +1121,7 @@ my $update_vm_api  = sub {
>  
>      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_root;
>  
>      my $delete_str = extract_param($param, 'delete');
>  
> @@ -1340,7 +1342,7 @@ 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') {
> +		    } elsif (!$is_root) {
>  			die "only root can delete '$opt' config for real devices\n";
>  		    }
>  		    PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
> @@ -1348,7 +1350,7 @@ my $update_vm_api  = sub {
>  		} elsif ($opt =~ m/^usb\d+$/) {
>  		    if ($val =~ m/spice/) {
>  			$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
> -		    } elsif ($authuser ne 'root@pam') {
> +		    } elsif (!$is_root) {
>  			die "only root can delete '$opt' config for real devices\n";
>  		    }
>  		    PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
> @@ -1392,14 +1394,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') {
> +		    } elsif (!$is_root) {
>  			die "only root can modify '$opt' config for real devices\n";
>  		    }
>  		    $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') {
> +		    } elsif (!$is_root) {
>  			die "only root can modify '$opt' config for real devices\n";
>  		    }
>  		    $conf->{pending}->{$opt} = $param->{$opt};
> @@ -1644,9 +1646,11 @@ __PACKAGE__->register_method({
>  	my $authuser = $rpcenv->get_user();
>  	my $vmid = $param->{vmid};
>  
> +	my $is_root = PVE::Tools::check_for_root("/vms/$vmid");
> +
>  	my $skiplock = $param->{skiplock};
>  	raise_param_exc({ skiplock => "Only root may use this option." })
> -	    if $skiplock && $authuser ne 'root@pam';
> +	    if $skiplock && !$is_root;
>  
>  	my $early_checks = sub {
>  	    # test if VM exists
> @@ -2291,10 +2295,12 @@ __PACKAGE__->register_method({
>  	my $machine = extract_param($param, 'machine');
>  	my $force_cpu = extract_param($param, 'force-cpu');
>  
> +	my $is_root = PVE::Tools::check_for_root("/vms/$vmid");
> +
>  	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';
> +		if $value && !$is_root;
>  	    return $value;
>  	};
>  
> @@ -2436,17 +2442,19 @@ __PACKAGE__->register_method({
>  	my $node = extract_param($param, 'node');
>  	my $vmid = extract_param($param, 'vmid');
>  
> +	my $is_root = PVE::Tools::check_for_root("/vms/$vmid");
> +
>  	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_root;
>  
>  	my $keepActive = extract_param($param, 'keepActive');
>  	raise_param_exc({ keepActive => "Only root may use this option." })
> -	    if $keepActive && $authuser ne 'root@pam';
> +	    if $keepActive && !$is_root;
>  
>  	my $migratedfrom = extract_param($param, 'migratedfrom');
>  	raise_param_exc({ migratedfrom => "Only root may use this option." })
> -	    if $migratedfrom && $authuser ne 'root@pam';
> +	    if $migratedfrom && !$is_root;
>  
>  
>  	my $storecfg = PVE::Storage::config();
> @@ -2513,9 +2521,11 @@ __PACKAGE__->register_method({
>  
>  	my $vmid = extract_param($param, 'vmid');
>  
> +	my $is_root = PVE::Tools::check_for_root("/vms/$vmid");
> +
>  	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_root;
>  
>  	die "VM $vmid not running\n" if !PVE::QemuServer::check_running($vmid);
>  
> @@ -2580,13 +2590,15 @@ __PACKAGE__->register_method({
>  	my $node = extract_param($param, 'node');
>  	my $vmid = extract_param($param, 'vmid');
>  
> +	my $is_root = PVE::Tools::check_for_root("/vms/$vmid");
> +
>  	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_root;
>  
>  	my $keepActive = extract_param($param, 'keepActive');
>  	raise_param_exc({ keepActive => "Only root may use this option." })
> -	    if $keepActive && $authuser ne 'root@pam';
> +	    if $keepActive && !$is_root;
>  
>  	my $storecfg = PVE::Storage::config();
>  
> @@ -2735,13 +2747,15 @@ __PACKAGE__->register_method({
>  	my $node = extract_param($param, 'node');
>  	my $vmid = extract_param($param, 'vmid');
>  
> +	my $is_root = PVE::Tools::check_for_root("/vms/$vmid");
> +
>  	my $todisk = extract_param($param, 'todisk') // 0;
>  
>  	my $statestorage = extract_param($param, 'statestorage');
>  
>  	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_root;
>  
>  	die "VM $vmid not running\n" if !PVE::QemuServer::check_running($vmid);
>  
> @@ -2811,13 +2825,15 @@ __PACKAGE__->register_method({
>  
>  	my $vmid = extract_param($param, 'vmid');
>  
> +	my $is_root = PVE::Tools::check_for_root("/vms/$vmid");
> +
>  	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_root;
>  
>  	my $nocheck = extract_param($param, 'nocheck');
>  	raise_param_exc({ nocheck => "Only root may use this option." })
> -	    if $nocheck && $authuser ne 'root@pam';
> +	    if $nocheck && !$is_root;
>  
>  	my $to_disk_suspended;
>  	eval {
> @@ -2883,9 +2899,11 @@ __PACKAGE__->register_method({
>  
>  	my $vmid = extract_param($param, 'vmid');
>  
> +	my $is_root = PVE::Tools::check_for_root("/vms/$vmid");
> +
>  	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_root;
>  
>  	PVE::QemuServer::vm_sendkey($vmid, $skiplock, $param->{key});
>  
> @@ -3392,6 +3410,8 @@ __PACKAGE__->register_method({
>  
>  	my $storecfg = PVE::Storage::config();
>  
> +	my $is_root = PVE::Tools::check_for_root("/vms/$vmid");
> +
>  	my $move_updatefn =  sub {
>  	    my $conf = PVE::QemuConfig->load_config($vmid);
>  	    PVE::QemuConfig->check_lock($conf);
> @@ -3671,7 +3691,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 !$is_root;
>  
>  	    raise_param_exc({ 'target-vmid' => "must be different than source VMID to reassign disk" })
>  		if $vmid eq $target_vmid;
> @@ -3910,15 +3930,17 @@ __PACKAGE__->register_method({
>  
>  	my $vmid = extract_param($param, 'vmid');
>  
> +	my $is_root = PVE::Tools::check_for_root("/vms/$vmid");
> +
>  	raise_param_exc({ force => "Only root may use this option." })
> -	    if $param->{force} && $authuser ne 'root@pam';
> +	    if $param->{force} && !$is_root;
>  
>  	raise_param_exc({ migration_type => "Only root may use this option." })
> -	    if $param->{migration_type} && $authuser ne 'root@pam';
> +	    if $param->{migration_type} && !$is_root;
>  
>  	# 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';
> +	    if $param->{migration_network} && !$is_root;
>  
>  	# test if VM exists
>  	my $conf = PVE::QemuConfig->load_config($vmid);
> @@ -4099,8 +4121,10 @@ __PACKAGE__->register_method({
>  	my $sizestr = extract_param($param, 'size');
>  
>  	my $skiplock = extract_param($param, 'skiplock');
> +	my $is_root = PVE::Tools::check_for_root("/vms/$vmid");
> +
>          raise_param_exc({ skiplock => "Only root may use this option." })
> -            if $skiplock && $authuser ne 'root@pam';
> +            if $skiplock && !$is_root;
>  
>          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] [RFC common 2/4] tools: add 'check_for_root' helper

[Fabian Grünbichler]

On January 5, 2022 4:22 pm, Oguz Bektas wrote:
> checks if the rpcenv user is 'root@pam' or has the 'Sys.Root' privilege
> on a given API path
> 
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
>  src/PVE/Tools.pm | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
> index 787942a..3e2e7f9 100644
> --- a/src/PVE/Tools.pm
> +++ b/src/PVE/Tools.pm
> @@ -2017,4 +2017,13 @@ sub get_file_hash {
>      return lc($digest);
>  }
>  
> +sub check_for_root {
> +    my ($path_to_check) = @_;
> +
> +    my $rpcenv = PVE::RPCEnvironment::get();
> +    my $authuser = $rpcenv->get_user();
> +    my $is_root = $authuser eq 'root@pam' || $rpcenv->check($authuser, $path_to_check, ['Sys.Root'], 1);
> +    return $is_root;
> +}

this is in the wrong place (pve-common is not concerned with 
access-control), move it to RPCEnvironment if kept.

this is only needed for the short-cut of not actually checking privs for 
the 'root@pam' user, since 'root@pam' by definition has 'Sys.Root' on 
all paths - so not sure whether the helper is necessary at all.

we don't have that many places limited to 'root@pam', so not sure 
whether simply doing:

# no short-cut
$rpcenv->check($authuser, $path_to_check, ['Sys.Root']);

or

# short-cut for user if in hot path somehow
$rpcenv->check($authuser, $path_to_check, ['Sys.Root'])
    if $authuser ne 'root@pam';

is not sufficient? it seems to be very short compared to the other 
helpers we have in PVE::RPCEnvironment ;)

> +
>  1;
> -- 
> 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] [RFC access-control common container qemu-server 0/4] #2582: Sys.Root privilege

[Fabian Grünbichler]

On January 5, 2022 4:22 pm, Oguz Bektas wrote:
> sending this in as RFC, because i think it needs a bit of ironing
> out and discussing the quirky bits ;)

thanks for picking up the work on this seemingly-open-forever bug ;) 

summarizing the previous discussions would be helpful for a 
cover-letter:
- why do we want/need/... this? (i.e., what problem(s) does this solve?)
- other possible approaches besides a new privilege (and why prefer a 
  priv over them?)
- clear semantics of the new privilege (e.g., below about whether it 
  implies all other privs or not)

> i'm not done with pve-manager patch so that'll come with the v1, though API
> should work for testing the changes.

besides pve-manager, there are also possible call-sites in 
pve-access-control and pve-cluster - did you check those?

> 
> it probably makes sense to also add a helper there, since at the moment
> we only check if Proxmox.UserName === 'root@pam' or in some cases
> specific permissions for storage and so forth, in order to decide
> whether to show/enable some GUI elements.
> 
> container API already works pretty well. VM API should also work but i haven't
> tested this extensively w.r.t. storage and migration.
> 
> some questions that popped up in my head:
> 
> + should adding 'Sys.Root' privilege to a user give them all available
> privileges on that path? this would make sense as having a
> root-equivalent privilege should be already enough (otherwise it doesn't
> have much of a point?). since at some places we have things like:

hmm, this is a tricky question and probably one that we should document 
explicitly. given that Sys.Root implies that you are allowed to do 
dangerous stuff that easily allows you to obtain root-level access, 
using the existing "'root@pam' has all privileges" as "'Sys.root' 
implies all other privileges on current path" probably makes sense and 
simplifies the logic considerably.

> 
> -------
>         } elsif ($target_vmid) {
>             $rpcenv->check_vm_perm($authuser, $target_vmid, undef, ['VM.Config.Disk'])
> -               if $authuser ne 'root@pam';
> +               if !$is_root;

this is just a short-cut to avoid the (possibly expensive) check, as the 
check cannot ever fail for root@pam by definition. I think these could 
just stay as checking for the user - if we include 'Sys.Root' the 
short-cut is no longer cheap (and obviously, if 'Sys.Root' does not 
imply having all the other privs, changing it would be wrong as well).

> -------
> 
> where one could theoretically have root-eq privs but not the 'VM.Config.Disk' for the target vm...
> 
> 
> + $authuser could also be an (optional?) parameter for the helper in
> PVE::Tools, so that we could check arbitrary users and not only the current
> one. also on most of these we already call $rpcenv->get_user() before doing the
> check, so we could spare that call inside the helper if we do that
> consistently.

see review for that patch

> 
> + would it make sense to be able to give 'Sys.Root' on a single node (like on
> /nodes/foo instead of the whole cluster)? this seemed like a rabbithole to me
> since we'd have to lock down quite a bit of stuff to limit movement from one
> cluster member to the other, without any(?) worthwhile benefits? or might make
> sense to just allow 'Sys.Root' to be given on '/' (since it should be
> equivalent to root@pam anyway)

like you said, this is a totally different can of worms and orthogonal 
to introducing this privilege. there are some areas where adding new ACL 
namespaces and paths might make sense (like nodes, but also things like 
the firewall, networks/.., ..), but those require really careful 
evaluation as changing stuff afterwards is quite involved.

this series might be a good way to go through all the places where we 
don't have real ACLs at the moment though and categorize them:

- local HW access
- 'forcing' stuff (skiplock et al)
- ...

to determine areas where we can improve without the root@pam/Sys.Root 
cludge (by introducing new abstractions / ACL namespaces / privileges).

> 
> + should root@pam have 'Sys.Root' by default? or does it make sense to still
> differentiate the "real" root user and the "impersonated" one?

see review for helper patch and comments above ;)

> 
>  pve-access-control:
> 
>  Oguz Bektas (1):
>   add Sys.Root privilege
> 
>  src/PVE/AccessControl.pm | 1 +
>  1 file changed, 1 insertion(+)
> 
>  pve-common:
> 
>  Oguz Bektas (1):
>   tools: add 'check_for_root' helper
> 
>  src/PVE/Tools.pm | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
>  pve-container:
> 
>  Oguz Bektas (1):
>   fix #2582: api: use common helper for checking root privileges
> 
>  src/PVE/API2/LXC.pm | 5 ++---
>  src/PVE/LXC.pm      | 8 +++++---
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
>  qemu-server:
> 
>  Oguz Bektas (1):
>   api: use common helper for checking root privileges
> 
>  PVE/API2/Qemu.pm | 74 ++++++++++++++++++++++++++++++++----------------
>  1 file changed, 49 insertions(+), 25 deletions(-)
> 
> 
> -- 
> 2.30.2
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>