From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 19F931FF16F for ; Fri, 15 Nov 2024 14:06:13 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id DCC2713D8A; Fri, 15 Nov 2024 14:06:13 +0100 (CET) Message-ID: Date: Fri, 15 Nov 2024 14:06:10 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta To: Fiona Ebner , Proxmox VE development discussion References: <20241114093226.814530-1-d.csapak@proxmox.com> <20241114093226.814530-4-d.csapak@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: X-SPAM-LEVEL: Spam detection results: 0 AWL 0.014 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 RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. 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. [guestimport.pm, dirplugin.pm] 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-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" On 11/15/24 13:11, Fiona Ebner wrote: > 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? honestly not sure, i'll try if we can omit that here or document what makes problems > >> + >> + 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" > ok >> + } >> + >> + # 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? the target storage must always be file based, because we call 'get_image_dir' on target_storeid which dies when it's not file based > >> + }; >> + 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()? yeah we could, but we only extract the size here, and for that the existance of a backing file is irrelevant i think (also it's just a hint for the ui i think, on copying the whole thing must be copied regardless of what is here returned, the most problematic thing that can happen here is that the image returns a too large size so it fills up the storage, but that can always happen, even if we pass untrusted here) > >> - 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