[pve-devel] [PATCH qemu-server 2/3] suspend to disk: check more permissions

[Thomas Lamprecht]

On 12/5/19 3:38 PM, Dominik Csapak wrote:
> only VM.PowerMgmt is not enough, since we allocate space on a storage,
> so we need VM.Config.Disk on the vm and Datastore.AllocateSpace on the storage
> 
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
> i am not sure if we need the last hunk (the check inside vm_suspend) at all,
> since we now always give a statestorage to vm_suspend and this api call
> is the only user of that parameter...
> 
> but i would like to hear others people opinions, since i believe
> i am overlooking something

I mean, no, if you check early and this all is in the same request, you won't
require that last hunk. I mean it's either the last or the one before that,
but as the one before that brings us the immediate feedback, it would be
preferred, IMO.

> 
>  PVE/API2/Qemu.pm  | 20 ++++++++++++++++++++
>  PVE/QemuServer.pm | 11 +++++++++++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 51fe142..9b25106 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -2387,6 +2387,9 @@ __PACKAGE__->register_method({
>      proxyto => 'node',
>      description => "Suspend virtual machine.",
>      permissions => {
> +	description => "You need 'VM.PowerMgmt' on /vms/{vmid}, and if you have set 'todisk',".
> +	    " you need also 'VM.Config.Disk' on /vms/{vmid} and 'Datastore.AllocateSpace'".
> +	    " on the storage for the vmstate.",
>  	check => ['perm', '/vms/{vmid}', [ 'VM.PowerMgmt' ]],
>      },
>      parameters => {
> @@ -2435,6 +2438,23 @@ __PACKAGE__->register_method({
>  	die "Cannot suspend HA managed VM to disk\n"
>  	    if $todisk && PVE::HA::Config::vm_is_ha_managed($vmid);
>  
> +	# early check for storage permission, will again be checked
> +	# in vm_suspend to avoid races with config writes
> +	if ($todisk) {
> +	    $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.Disk']);
> +
> +	    if (!$statestorage) {
> +		# get statestorage from config if none is given
> +		PVE::QemuConfig->lock_config($vmid, sub {
> +		    my $conf = PVE::QemuConfig->load_config($vmid);
> +		    my $storecfg = PVE::Storage::config();
> +		    $statestorage = PVE::QemuServer::find_vmstate_storage($conf, $storecfg);
> +		});

With the approach you used in this patch, we'd not need to be locked here, this
is just the "abort early and give immediate feedback" check, and loadconfig will
never give you a partial config file, so it's fine do to this lockless at this point.

> +	    }
> +
> +	    $rpcenv->check($authuser, "/storage/$statestorage", ['Datastore.AllocateSpace']);
> +	}
> +
>  	my $realcmd = sub {
>  	    my $upid = shift;
>  
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 33b1d50..23d71f4 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -5761,6 +5761,17 @@ sub vm_suspend {
>  	    $conf->{lock} = 'suspending';
>  	    my $date = strftime("%Y-%m-%d", localtime(time()));
>  	    $storecfg = PVE::Storage::config();
> +	    if (!$statestorage) {
> +		$statestorage = find_vmstate_storage($conf, $storecfg);
> +	    }
> +
> +	    # check permissions for the storage

alternatively you could also move this check in the if above, then the call semantic
would be:
* if one passes a storage the checks had to be done by the caller
* if we (this method) has to find the storage we also check it

I mean, this is just some hash walk and comparison check, it's not really expensive
so we could just always do it, as you do in the patch, and be on the "safe site" if
this gets reused in the future.

> +	    my $rpcenv = PVE::RPCEnvironment::get();
> +	    if ($rpcenv->{type} ne 'cli') {
> +		my $authuser = $rpcenv->get_user();
> +		$rpcenv->check($authuser, "/storage/$statestorage", ['Datastore.AllocateSpace']);
> +	    }
> +
>  	    $vmstate = PVE::QemuConfig->__snapshot_save_vmstate($vmid, $conf, "suspend-$date", $storecfg, $statestorage, 1);
>  	    $path = PVE::Storage::path($storecfg, $vmstate);
>  	    PVE::QemuConfig->write_config($vmid, $conf);
>