[pve-devel] [PATCH v1 qemu-server 5/5] add SuperUser privilege checks for root-only options
Fabian Grünbichler
f.gruenbichler at proxmox.com
Thu Feb 10 16:29:36 CET 2022
this needs a rebase anyhow, I reviewed it on HEAD~3 where it still
applies ;)
there's one check for 'root at 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 at 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 at pam';
> + return 1 if $authuser eq 'root at 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 at 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 at pam';
> + return 1 if $authuser eq 'root at 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 at pam';
> + return 1 if $authuser eq 'root at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at pam anyway)
skiplock can switch to being superuser-limited, the rest should stay
root at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at pam';
> + my $is_superuser = $authuser eq 'root at 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 at 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 at 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 at 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 at 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 at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
>
More information about the pve-devel
mailing list