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

[Thomas Lamprecht]

On 12/6/19 9:58 AM, Fabian Grünbichler wrote:
> On December 5, 2019 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
>>
>>  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) {
> 
> UX nit: we need to give the user the option to set a vmstatestorage on 
> the GUI now - otherwise this might reliably pick a storage that the user 
> cannot access, while there are other storages where they might be 
> allowed to allocate a vmstate volume but they don't get that choice.
> 
> -> make vmstatestorage option editable on GUI
> -> display a storage selector in hibernate dialogue, pre-select 
> vmstatestorage from config if set, allow any storages that the user can 
> access

I'd favour the latter but with the same logic fallback as the backend has,
if no storage is set in the config. Naturally with the disallowed storage
filtered out/marked invalid.

> 
>> +		# 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);
>> +		});
>> +	    }
>> +
>> +	    $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
>> +	    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);
>> -- 
>> 2.20.1