[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 at proxmox.com
Thu Mar 17 11:05:07 CET 2022
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 at proxmox.com>
> ---
> v1->v2:
> * add comments at the shortcuts for root at 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 at pam
> return 1 if $authuser eq 'root at 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 at pam
> return 1 if $authuser eq 'root at 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 at pam
> return 1 if $authuser eq 'root at 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 at 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 at 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 at 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 at 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 at 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 at 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 at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
>
More information about the pve-devel
mailing list