Re: [pve-devel] [PATCH qemu-server v3 2/3] Move importdisk from qm to API

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 03.09.20 15:52, Dominik Csapak wrote:
> comments inline
> 
> On 8/25/20 11:24 AM, Dominic Jäger wrote:
>> Additionally, add parameters that enable directly (avoiding the intermediate
>> step as unused disk) attaching the disk to a bus/device with all known disk
>> options.
>>
>> Required to create a GUI for importdisk.
>>
>> Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
>> ---
>> v2->v3: unchanged
>>
>>   PVE/API2/Qemu.pm             | 113 ++++++++++++++++++++++++++++++++++-
>>   PVE/CLI/qm.pm                |  57 +-----------------
>>   PVE/QemuServer/Drive.pm      |  14 +++++
>>   PVE/QemuServer/ImportDisk.pm |   2 +-
>>   4 files changed, 127 insertions(+), 59 deletions(-)
>>
>> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
>> index 8da616a..66e630d 100644
>> --- a/PVE/API2/Qemu.pm
>> +++ b/PVE/API2/Qemu.pm
>> @@ -24,6 +24,7 @@ use PVE::QemuServer;
>>   use PVE::QemuServer::Drive;
>>   use PVE::QemuServer::CPUConfig;
>>   use PVE::QemuServer::Monitor qw(mon_cmd);
>> +use PVE::QemuServer::ImportDisk qw(do_import);
>>   use PVE::QemuMigrate;
>>   use PVE::RPCEnvironment;
>>   use PVE::AccessControl;
>> @@ -45,8 +46,6 @@ BEGIN {
>>       }
>>   }
>>   -use Data::Dumper; # fixme: remove
>> -
>>   use base qw(PVE::RESTHandler);
>>     my $opt_force_description = "Force physical removal. Without this, we simple remove the disk from the config file and create an additional configuration entry called 'unused[n]', which contains the volume ID. Unlink of unused[n] always cause physical removal.";
>> @@ -4265,4 +4264,114 @@ __PACKAGE__->register_method({
>>       return PVE::QemuServer::Cloudinit::dump_cloudinit_config($conf, $param->{vmid}, $param->{type});
>>       }});
>>   +__PACKAGE__->register_method ({
>> +    name => 'importdisk',
>> +    path => '{vmid}/importdisk',
>> +    method => 'POST',
>> +    protected => 1, # for worker upid file
>> +    proxyto => 'node',
>> +    description => "Import an external disk image into a VM. The image format ".
>> +    "has to be supported by qemu-img.",
>> +    permissions => {
>> +    check => [ 'and',
>> +        [ 'perm', '/storage/{storage}', ['Datastore.AllocateTemplate']],
>> +        [ 'perm', '/storage/{storage}', ['Datastore.AllocateSpace']],
>> +        [ 'perm', '/vms/{vmid}', ['VM.Allocate']],
>> +    ],
> 
> this is a big no-no
> 
> now everyone that has permissions to create a vm can import any
> file they want from any path?
> 
> (e.g. another vm image, /dev/sda (!!), etc.)
> 
> this is basically circumventing our whole permission system...
> 
> for 'qm' this was okay since that was root@pam only, but if
> we are in the api, we have to be careful what to import
> 
> in general, we have to do either:
> * restrict the source path to a very small subset of possible
>   paths, that are guaranteed to not be dangerous
>   (/tmp/pve-import-$userid/ ?)

No, for sure not tmp, this can clash easily and is outside of every permission
management of us (PVE)

We already have paths which are owned and have permissions attached, allow
all paths the user can access anyway. This consists of all disk volumes/images
they can access already. We encode the owner (VMID) in the disk name after all
for a reason.

A disk image uploader, or "puller" (if it can pull it over https directly to
the server) should import such images with a defined name, ideally it gets 
already associated with a VMID and all of our ownership rules and enforcement
work out.

But yes, we do not want to allow using an arbitrary path.