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 03C7B1FF16F for ; Fri, 15 Nov 2024 13:13:00 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 5C20A12B9D; Fri, 15 Nov 2024 13:12:15 +0100 (CET) Message-ID: Date: Fri, 15 Nov 2024 13:11:37 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox VE development discussion , Dominik Csapak References: <20241114093226.814530-1-d.csapak@proxmox.com> <20241114093226.814530-4-d.csapak@proxmox.com> Content-Language: en-GB From: Fiona Ebner In-Reply-To: <20241114093226.814530-4-d.csapak@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL -0.055 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 v5 03/12] plugin: dir: handle ova files for import 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" On 14.11.24 10:32 AM, Dominik Csapak wrote: > diff --git a/src/PVE/GuestImport.pm b/src/PVE/GuestImport.pm > new file mode 100644 > index 0000000..c89fbc9 > --- /dev/null > +++ b/src/PVE/GuestImport.pm > @@ -0,0 +1,78 @@ > +package PVE::GuestImport; > + > +use strict; > +use warnings; > + > +use File::Path; > + > +use PVE::Storage; > +use PVE::Tools qw(run_command); > + > +sub extract_disk_from_import_file { > + my ($volid, $vmid, $target_storeid) = @_; > + > + my ($source_storeid, $volname) = PVE::Storage::parse_volume_id($volid); > + $target_storeid //= $source_storeid; > + my $cfg = PVE::Storage::config(); > + > + my ($vtype, $name, undef, undef, undef, undef, $fmt) = > + PVE::Storage::parse_volname($cfg, $volid); > + > + die "only files with content type 'import' can be extracted\n" > + if $vtype ne 'import' || $fmt !~ m/^ova\+/; Nit: could use a different error message for the unexpected/unsupported format case. > + > + # extract the inner file from the name > + my $archive_volid; > + my $inner_file; > + my $inner_fmt; > + if ($name =~ m!^(.*\.ova)/(${PVE::Storage::SAFE_CHAR_CLASS_RE}+)$!) { > + $archive_volid = "$source_storeid:import/$1"; > + $inner_file = $2; > + ($inner_fmt) = $fmt =~ /^ova\+(.*)$/; > + } else { > + die "cannot extract $volid - invalid volname $volname\n"; > + } > + > + my $ova_path = PVE::Storage::path($cfg, $archive_volid); > + > + my $tmpdir = PVE::Storage::get_image_dir($cfg, $target_storeid, $vmid); > + my $pid = $$; > + $tmpdir .= "/tmp_${pid}_${vmid}"; > + mkpath $tmpdir; > + > + ($ova_path) = $ova_path =~ m|^(.*)$|; # untaint Should plugins' path() implementations maybe untaint/validate what they return so we don't need this here? I'm wondering: with which plugins did you have issues here? > + > + my $source_path = "$tmpdir/$inner_file"; > + my $target_path; > + my $target_volid; > + eval { > + run_command(['tar', '-x', '--force-local', '-C', $tmpdir, '-f', $ova_path, $inner_file]); > + > + # check for symlinks and other non regular files > + if (-l $source_path || ! -f $source_path) { > + die "only regular files are allowed\n"; I'd add the path to the error message for context or maybe something like "extracted file $inner_file from import archive $archive_volid is not a regular file" > + } > + > + # check potentially untrusted image file! > + PVE::Storage::file_size_info($source_path, undef, 1); > + > + # create temporary 1M image that will get overwritten by the rename > + # to reserve the filename and take care of locking > + $target_volid = PVE::Storage::vdisk_alloc($cfg, $target_storeid, $vmid, $inner_fmt, undef, 1024); > + $target_path = PVE::Storage::path($cfg, $target_volid); > + > + print "renaming $source_path to $target_path\n"; > + > + rename($source_path, $target_path) or die "unable to move - $!\n"; This won't work for non-file based target storages, or what am I missing? > + }; > + if (my $err = $@) { > + File::Path::remove_tree($tmpdir); > + die "error during extraction: $err\n"; > + } > + > + File::Path::remove_tree($tmpdir); > + > + return $target_volid; > +} > + > +1; ---snip--- > @@ -224,16 +260,20 @@ ovf:Item[rasd:InstanceID='%s']/rasd:ResourceType", $controller_id); > ($filepath) = $filepath =~ m|^(${PVE::Storage::SAFE_CHAR_CLASS_RE}+)$|; # untaint & check no sub/parent dirs > die "invalid path\n" if !$filepath; > > - my $virtual_size = PVE::Storage::file_size_info($backing_file_path); Noticing only now, shouldn't we pass $untrusted=1 for file_size_info()? > - die "error parsing $backing_file_path, cannot determine file size\n" > - if !$virtual_size; > + if (!$isOva) { > + my $size = PVE::Storage::file_size_info($backing_file_path); > + die "error parsing $backing_file_path, cannot determine file size\n" > + if !$size; > > + $virtual_size = $size; > + } > $pve_disk = { > disk_address => $pve_disk_address, > backing_file => $backing_file_path, > virtual_size => $virtual_size, > relative_path => $filepath, > }; > + $pve_disk->{virtual_size} = $virtual_size if defined($virtual_size); > push @disks, $pve_disk; > > } ---snip--- > diff --git a/src/PVE/Storage/DirPlugin.pm b/src/PVE/Storage/DirPlugin.pm > index 3e3b1e7..ea89464 100644 > --- a/src/PVE/Storage/DirPlugin.pm > +++ b/src/PVE/Storage/DirPlugin.pm > @@ -258,15 +258,26 @@ sub get_import_metadata { > # NOTE: all types of warnings must be added to the return schema of the import-metadata API endpoint > my $warnings = []; > > + my $isOva = 0; > + if ($name =~ m/\.ova$/) { Nit: should rely on $fmt to check. > + $isOva = 1; > + push @$warnings, { type => 'ova-needs-extracting' }; > + } _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel