From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 6EC2D61010 for ; Fri, 4 Sep 2020 11:11:07 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 59BEE1E94E for ; Fri, 4 Sep 2020 11:10:37 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 92C531E943 for ; Fri, 4 Sep 2020 11:10:36 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 4DD6044A17 for ; Fri, 4 Sep 2020 11:10:36 +0200 (CEST) To: Proxmox VE development discussion , Dominik Csapak References: <20200825092449.49410-1-d.jaeger@proxmox.com> <20200825092449.49410-3-d.jaeger@proxmox.com> <4f1eb138-fa5d-a8a6-7b48-fcad171c5c1e@proxmox.com> From: Thomas Lamprecht Message-ID: <215b4215-4200-2057-a3e6-de072f3e7560@proxmox.com> Date: Fri, 4 Sep 2020 11:10:34 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:81.0) Gecko/20100101 Thunderbird/81.0 MIME-Version: 1.0 In-Reply-To: <4f1eb138-fa5d-a8a6-7b48-fcad171c5c1e@proxmox.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 1.077 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -2.403 Looks like a legit reply (A) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [importdisk.pm, qemu.pm] Subject: Re: [pve-devel] [PATCH qemu-server v3 2/3] Move importdisk from qm to API X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 04 Sep 2020 09:11:07 -0000 On 03.09.20 15:52, Dominik Csapak wrote: > comments inline >=20 > On 8/25/20 11:24 AM, Dominic J=C3=A4ger wrote: >> Additionally, add parameters that enable directly (avoiding the interm= ediate >> 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=C3=A4ger >> --- >> v2->v3: unchanged >> >> =C2=A0 PVE/API2/Qemu.pm=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 | 113 ++++++++++++++++++++++++++++++++++- >> =C2=A0 PVE/CLI/qm.pm=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0 57 +----------------- >> =C2=A0 PVE/QemuServer/Drive.pm=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0 1= 4 +++++ >> =C2=A0 PVE/QemuServer/ImportDisk.pm |=C2=A0=C2=A0 2 +- >> =C2=A0 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; >> =C2=A0 use PVE::QemuServer::Drive; >> =C2=A0 use PVE::QemuServer::CPUConfig; >> =C2=A0 use PVE::QemuServer::Monitor qw(mon_cmd); >> +use PVE::QemuServer::ImportDisk qw(do_import); >> =C2=A0 use PVE::QemuMigrate; >> =C2=A0 use PVE::RPCEnvironment; >> =C2=A0 use PVE::AccessControl; >> @@ -45,8 +46,6 @@ BEGIN { >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } >> =C2=A0 } >> =C2=A0 -use Data::Dumper; # fixme: remove >> - >> =C2=A0 use base qw(PVE::RESTHandler); >> =C2=A0 =C2=A0 my $opt_force_description =3D "Force physical removal. W= ithout this, we simple remove the disk from the config file and create an= additional configuration entry called 'unused[n]', which contains the vo= lume ID. Unlink of unused[n] always cause physical removal."; >> @@ -4265,4 +4264,114 @@ __PACKAGE__->register_method({ >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return PVE::QemuServer::Cloudinit::dump= _cloudinit_config($conf, $param->{vmid}, $param->{type}); >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 }}); >> =C2=A0 +__PACKAGE__->register_method ({ >> +=C2=A0=C2=A0=C2=A0 name =3D> 'importdisk', >> +=C2=A0=C2=A0=C2=A0 path =3D> '{vmid}/importdisk', >> +=C2=A0=C2=A0=C2=A0 method =3D> 'POST', >> +=C2=A0=C2=A0=C2=A0 protected =3D> 1, # for worker upid file >> +=C2=A0=C2=A0=C2=A0 proxyto =3D> 'node', >> +=C2=A0=C2=A0=C2=A0 description =3D> "Import an external disk image in= to a VM. The image format ". >> +=C2=A0=C2=A0=C2=A0 "has to be supported by qemu-img.", >> +=C2=A0=C2=A0=C2=A0 permissions =3D> { >> +=C2=A0=C2=A0=C2=A0 check =3D> [ 'and', >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 [ 'perm', '/storage/{stora= ge}', ['Datastore.AllocateTemplate']], >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 [ 'perm', '/storage/{stora= ge}', ['Datastore.AllocateSpace']], >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 [ 'perm', '/vms/{vmid}', [= 'VM.Allocate']], >> +=C2=A0=C2=A0=C2=A0 ], >=20 > this is a big no-no >=20 > now everyone that has permissions to create a vm can import any > file they want from any path? >=20 > (e.g. another vm image, /dev/sda (!!), etc.) >=20 > this is basically circumventing our whole permission system... >=20 > 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 >=20 > in general, we have to do either: > * restrict the source path to a very small subset of possible > =C2=A0 paths, that are guaranteed to not be dangerous > =C2=A0 (/tmp/pve-import-$userid/ ?) No, for sure not tmp, this can clash easily and is outside of every permi= ssion management of us (PVE) We already have paths which are owned and have permissions attached, allo= w all paths the user can access anyway. This consists of all disk volumes/i= mages they can access already. We encode the owner (VMID) in the disk name afte= r 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 get= s=20 already associated with a VMID and all of our ownership rules and enforce= ment work out. But yes, we do not want to allow using an arbitrary path.