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.bek...@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
>
>
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
