From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 893951FF15F for ; Mon, 18 Nov 2024 13:42:41 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 5CD7CFCE9; Mon, 18 Nov 2024 13:42:45 +0100 (CET) Message-ID: <57748177-1da3-4b59-8691-09f18904c413@proxmox.com> Date: Mon, 18 Nov 2024 13:42:12 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox VE development discussion , Dominik Csapak References: <20241115151749.633407-1-d.csapak@proxmox.com> <20241115151749.633407-10-d.csapak@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: <20241115151749.633407-10-d.csapak@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL -0.054 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH storage v6 09/12] api: allow ova upload/download 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: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" Am 15.11.24 um 16:17 schrieb Dominik Csapak: > introducing a separate regex that only contains ova, since > upload/downloading ovfs does not make sense (since the disks are then > missing). > > Add a sanity check after up/downloading the ova file (and delete if it > does not match). > > Signed-off-by: Dominik Csapak > --- > changes from v2: > * add sanity check for ova content after up/downloading > > src/PVE/API2/Storage/Status.pm | 69 +++++++++++++++++++++++++++++++--- > src/PVE/Storage.pm | 11 ++++++ > 2 files changed, 75 insertions(+), 5 deletions(-) > > diff --git a/src/PVE/API2/Storage/Status.pm b/src/PVE/API2/Storage/Status.pm > index bdf1c18..8bbb5a7 100644 > --- a/src/PVE/API2/Storage/Status.pm > +++ b/src/PVE/API2/Storage/Status.pm > @@ -41,6 +41,24 @@ __PACKAGE__->register_method ({ > path => '{storage}/file-restore', > }); > > +my sub assert_ova_contents { > + my ($file) = @_; > + > + # test if it's really a tar file with an ovf file inside > + my $hasOvf = 0; > + run_command(['tar', '-t', '-f', $file], outfunc => sub { > + my ($line) = @_; > + > + if ($line =~ m/\.ovf$/) { > + $hasOvf = 1; > + } > + }); Style nit: wrong indentation > + > + die "ova archive has no .ovf file inside\n" if !$hasOvf; > + > + return undef; Nit: I'd prefer a truthy-return, but no big deal > +}; Style nit: no need for semicolon > + > __PACKAGE__->register_method ({ > name => 'index', > path => '', > @@ -369,7 +387,7 @@ __PACKAGE__->register_method ({ > name => 'upload', > path => '{storage}/upload', > method => 'POST', > - description => "Upload templates and ISO images.", > + description => "Upload templates, ISO images and OVAs.", > permissions => { > check => ['perm', '/storage/{storage}', ['Datastore.AllocateTemplate']], > }, > @@ -382,7 +400,7 @@ __PACKAGE__->register_method ({ > content => { > description => "Content type.", > type => 'string', format => 'pve-storage-content', > - enum => ['iso', 'vztmpl'], > + enum => ['iso', 'vztmpl', 'import'], > }, > filename => { > description => "The name of the file to create. Caution: This will be normalized!", > @@ -437,6 +455,7 @@ __PACKAGE__->register_method ({ > my $filename = PVE::Storage::normalize_content_filename($param->{filename}); > > my $path; > + my $isOva = 0; > > if ($content eq 'iso') { > if ($filename !~ m![^/]+$PVE::Storage::ISO_EXT_RE_0$!) { > @@ -448,6 +467,16 @@ __PACKAGE__->register_method ({ > raise_param_exc({ filename => "wrong file extension" }); > } > $path = PVE::Storage::get_vztmpl_dir($cfg, $param->{storage}); > + } elsif ($content eq 'import') { > + if ($filename !~ m!${PVE::Storage::SAFE_CHAR_CLASS_RE}+$PVE::Storage::UPLOAD_IMPORT_EXT_RE_1$!) { > + raise_param_exc({ filename => "invalid filename or wrong extension" }); > + } > + > + if ($filename =~ m/\.ova$/) { Nit: we already get the extension from the above match, and could use that. Also, it's always an ova, so no need for special casing. > + $isOva = 1; > + } > + > + $path = PVE::Storage::get_import_dir($cfg, $param->{storage}); > } else { > raise_param_exc({ content => "upload content type '$content' not allowed" }); > } ---snip--- > @@ -669,7 +714,21 @@ __PACKAGE__->register_method({ > die "no decompression method found\n" if !$info->{decompressor}; > $opts->{decompression_command} = $info->{decompressor}; > } > - PVE::Tools::download_file_from_url("$path/$filename", $url, $opts); > + > + my $target_path = "$path/$filename"; > + PVE::Tools::download_file_from_url($target_path, $url, $opts); As already discussed off-list, would be nice to do the check before the file is put in its final location ;) > + > + if ($isOva) { > + eval { > + assert_ova_contents($target_path); > + }; > + if (my $err = $@) { > + # unlinks only the temporary file from the http server > + unlink $target_path or $! == ENOENT > + or warn "unable to clean up temporory file '$target_path' - $!\n"; > + die $err; > + } > + } > }; > > my $worker_id = PVE::Tools::encode_text($filename); # must not pass : or the like as w-ID _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel