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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On March 11, 2022 12:24 pm, Oguz Bektas wrote:
> we now allow users with SU privilege to edit real device configurations
> for VMs.
> 
> they still need the required privilege to edit the corresponding
> configuration options (such as `VM.Config.HWType`), as well as the SU
> privilege.
> 
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
> v1->v2:
> * add comments at the shortcuts for root@pam
> * remove wrong shortcut for SU, instead check required privileges + SU
> * revert migration-only parameters and vzdump internal ones
> 
> 
>  PVE/API2/Qemu.pm | 63 ++++++++++++++++++++++++------------------------
>  1 file changed, 32 insertions(+), 31 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 68077cc..21fc82b 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -352,16 +352,17 @@ my $cloudinitoptions = {
>  my $check_vm_create_serial_perm = sub {
>      my ($rpcenv, $authuser, $vmid, $pool, $param) = @_;
>  
> +    # no need to check permissions for root@pam
>      return 1 if $authuser eq 'root@pam';
>  
> +    my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
> +
>      foreach my $opt (keys %{$param}) {
>  	next if $opt !~ m/^serial\d+$/;
>  
> -	if ($param->{$opt} eq 'socket') {
> -	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.HWType']);
> -	} else {
> -	    die "only root can set '$opt' config for real devices\n";
> -	}
> +	$rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.HWType']);
> +	die "only superusers can set '$opt' config for real devices\n"
> +	    if !($param->{$opt} eq 'socket' || $is_superuser);

check and message match, but not really. IMHO

    if $param->{$opt} ne 'socket' && !$is_superuser;

matches the text of the message in a more readable fashion

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

same here

>      }
>  
>      return 1;
> @@ -388,8 +390,11 @@ my $check_vm_create_usb_perm = sub {
>  my $check_vm_modify_config_perm = sub {
>      my ($rpcenv, $authuser, $vmid, $pool, $key_list) = @_;
>  
> +    # no need to check permissions for root@pam
>      return 1 if $authuser eq 'root@pam';
>  
> +    my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
> +
>      foreach my $opt (@$key_list) {
>  	# some checks (e.g., disk, serial port, usb) need to be done somewhere
>  	# else, as there the permission can be value dependend
> @@ -425,7 +430,8 @@ my $check_vm_modify_config_perm = sub {
>  	} else {
>  	    # catches hostpci\d+, args, lock, etc.
>  	    # new options will be checked here
> -	    die "only root can set '$opt' config\n";
> +	    die "only superusers can set '$opt' config\n"
> +		if !$is_superuser;
>  	}
>      }
>  
> @@ -1117,6 +1123,8 @@ my $update_vm_api  = sub {
>  	push @paramarr, "-$key", $value;
>      }
>  
> +    my $is_superuser = $authuser eq 'root@pam' || $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);

nit: line too long (I know, there are others as well, but that doesn't 
mean we want to introduce even more mess ;))

not sure whether the shortcut here is worth it anyway, this is a single 
call and we have a few others that are not skipped either.

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

condition style here

>  		    PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
>  		    PVE::QemuConfig->write_config($vmid, $conf);
>  		} elsif ($opt =~ m/^usb\d+$/) {
> -		    if ($val =~ m/spice/) {
> -			$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
> -		    } elsif ($authuser ne 'root@pam') {
> -			die "only root can delete '$opt' config for real devices\n";
> -		    }
> +		    $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
> +		    die "only superusers can delete '$opt' config for real devices\n"
> +			if !($val =~ m/spice/ || $is_superuser);

and here

>  		    PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
>  		    PVE::QemuConfig->write_config($vmid, $conf);
>  		} else {
> @@ -1362,6 +1366,7 @@ my $update_vm_api  = sub {
>  	    foreach my $opt (keys %$param) { # add/change
>  		$modified->{$opt} = 1;
>  		$conf = PVE::QemuConfig->load_config($vmid); # update/reload
> +		my $val = $conf->{$opt} // $conf->{pending}->{$opt};

no explanation for this change here, but $val now has the current value 
if one is set, or the pending value if that is set.

it seems to be copied from the code handling deletions (where this makes 
sense - there is no new value in that case), but we want to ensure the 
thing we remove is something we are allowed to add back/in the first 
place.

>  		next if defined($conf->{pending}->{$opt}) && ($param->{$opt} eq $conf->{pending}->{$opt}); # skip if nothing changed
>  
>  		my $arch = PVE::QemuServer::get_vm_arch($conf);
> @@ -1390,18 +1395,14 @@ my $update_vm_api  = sub {
>  			}
>  		    }
>  		} elsif ($opt =~ m/^serial\d+/) {
> -		    if ((!defined($conf->{$opt}) || $conf->{$opt} eq 'socket') && $param->{$opt} eq 'socket') {

but here we have totally different rules applying, and can't just 
apply the ones for deleting? this here checks both the old value if one 
is set (which we remove) and the new value (which we set)

> -			$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
> -		    } elsif ($authuser ne 'root@pam') {
> -			die "only root can modify '$opt' config for real devices\n";
> -		    }
> +		    $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
> +		    die "only superusers can modify '$opt' config for real devices\n"
> +			if !($val eq 'socket' || $is_superuser);

so this completely changes the semantics, no longer checking the new 
value at all.. so again I wonder how did you test this?  this allows 
skipping the SU check as long as I first set the allowed value, and then 
replace it with the high-privilege one..

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

same applies here..

>  		    $conf->{pending}->{$opt} = $param->{$opt};
>  		} else {
>  		    $conf->{pending}->{$opt} = $param->{$opt};
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>