[pve-devel] [RFC storage] Find iso and vztmpl in subdirs
Fabian Grünbichler
f.gruenbichler at proxmox.com
Thu Mar 5 10:48:46 CET 2020
this does not handle '..' in paths, so it allows circumvention of
permissions on storages:
# pvesm path storage_a:iso/../../../storage_b/iso/something.iso
/mnt/pve/storage_a/template/iso/../../../storage_b/template/iso/something.iso
you need to ensure that you stay within storage_a/template/iso/ (at
least on a logical level - if the admin has set up explicit links we do
want to follow them ;))
I think the same problem applies to the upload path, but I haven't
tested it.
I'd also make the recursive scanning opt-in until 7.0, to avoid
surprises (e.g., people might have symlinked the ISO dir to some general
directory where ISOs are stored among other things, and now their users
see all ISOs from all sub directories).
another nit inline
On December 2, 2019 12:45 pm, Dominic Jäger wrote:
> Each content type has a predefined location in a storage where they can be found.
> iso and vztmpl files can now be uploaded and found in subdirectories of those
> locations, too.
>
> There are some additional tests that show what some functions of Plugin.pm do.
> They test for example #2467 (duplicate volumes).
>
> Signed-off-by: Dominic Jäger <d.jaeger at proxmox.com>
> ---
> Thanks to Stefan for the feedback about the tests.
>
> get_file_properties is a mix of parse_volname and what was in get_subdir_files.
> We could probably change parse_volname to return subdirectories and replace
> get_file_properties with it or (as it was in get_subdir_files) put it back into
> get_subdir_files_rec.
>
> In the API content list call we see a volid, that is
> storage:vtype/volname
> For example
> my-storage:iso/file.iso
> As volname could not have a slash / it was clear that it separates vtype and
> volname. However, the volname can now contain those / which leads to
> my-storage:iso/some/sub/directories/file.iso
> IMO it's a bit confusing that "iso" is a vtype here but also part of the
> path
> storage_root/template/iso/some/sub/directories/file.iso
> but unfortunately I haven't had a better idea yet.
>
> PVE/API2/Storage/Status.pm | 10 +-
> PVE/Storage/Plugin.pm | 86 +++++++++++++----
> test/Makefile | 5 +-
> test/run_path_tests.pl | 190 +++++++++++++++++++++++++++++++++++++
> 4 files changed, 266 insertions(+), 25 deletions(-)
> create mode 100755 test/run_path_tests.pl
>
> diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
> index 1a856fb..dccdcd7 100644
> --- a/PVE/API2/Storage/Status.pm
> +++ b/PVE/API2/Storage/Status.pm
> @@ -403,25 +403,23 @@ __PACKAGE__->register_method ({
> my $filename = $param->{filename};
>
> chomp $filename;
> - $filename =~ s/^.*[\/\\]//;
> - $filename =~ s/[^-a-zA-Z0-9_.]/_/g;
> + $filename =~ s/^[\/|\\]*//; # files are uploaded relative to storage path
> + $filename =~ s/[^-a-zA-Z0-9_.\/\\]/_/g;
>
> my $path;
> -
> if ($content eq 'iso') {
> - if ($filename !~ m![^/]+$PVE::Storage::iso_extension_re$!) {
> + if ($filename !~ m!$PVE::Storage::iso_extension_re$!) {
> raise_param_exc({ filename => "missing '.iso' or '.img' extension" });
> }
> $path = PVE::Storage::get_iso_dir($cfg, $param->{storage});
> } elsif ($content eq 'vztmpl') {
> - if ($filename !~ m![^/]+\.tar\.[gx]z$!) {
> + if ($filename !~ m!\.tar\.[gx]z$!) {
> raise_param_exc({ filename => "missing '.tar.gz' or '.tar.xz' extension" });
> }
> $path = PVE::Storage::get_vztmpl_dir($cfg, $param->{storage});
> } else {
> raise_param_exc({ content => "upload content type '$content' not allowed" });
> }
> -
> die "storage '$param->{storage}' does not support '$content' content\n"
> if !$scfg->{content}->{$content};
>
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index d0f1df6..885b08f 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -404,6 +404,9 @@ sub parse_name_dir {
> die "unable to parse volume filename '$name'\n";
> }
>
> +# for iso and vztmpl returns
> +# [0] format
> +# [1] notdir (=basename+suffix; no directories)
> sub parse_volname {
> my ($class, $volname) = @_;
>
> @@ -417,9 +420,9 @@ sub parse_volname {
> my ($vmid, $name) = ($1, $2);
> my (undef, $format, $isBase) = parse_name_dir($name);
> return ('images', $name, $vmid, undef, undef, $isBase, $format);
> - } elsif ($volname =~ m!^iso/([^/]+$PVE::Storage::iso_extension_re)$!) {
> + } elsif ($volname =~ m!^iso/(?:.+/)*([^/]+$PVE::Storage::iso_extension_re)$!) {
> return ('iso', $1);
> - } elsif ($volname =~ m!^vztmpl/([^/]+\.tar\.[gx]z)$!) {
> + } elsif ($volname =~ m!^vztmpl/(?:.+/)*([^/]+\.tar\.[gx]z)$!) {
> return ('vztmpl', $1);
> } elsif ($volname =~ m!^rootdir/(\d+)$!) {
> return ('rootdir', $1, $1);
> @@ -474,7 +477,14 @@ sub filesystem_path {
>
> $dir .= "/$vmid" if $vtype eq 'images';
>
> - my $path = "$dir/$name";
> + my $path;
> + if ($vtype eq 'iso' || $vtype eq 'vztmpl') {
> + # remove vtype => file name relative to storage path
> + $volname =~ m!^$vtype/(.+)!;
> + $path = "$dir/$1";
> + } else {
> + $path = "$dir/$name";
> + }
>
> return wantarray ? ($path, $vmid, $vtype) : $path;
> }
> @@ -889,9 +899,60 @@ sub list_images {
> return $res;
> }
>
> -# list templates ($tt = <iso|vztmpl|backup|snippets>)
> +
> +# retvals if file name fits $tt
> +# [0] volname = file name relative to $storage_path = right side of volid
> +# [1] format
> +# undef otherwise
> +sub get_file_properties {
> + my ($fn, $storage_path, $tt) = @_;
> + if ($tt eq 'iso') {
> + if ($fn =~ m!$storage_path/(.+$PVE::Storage::iso_extension_re)$!i) {
> + return ["$tt/$1", $tt];
> + } else {
> + return undef;
> + }
> + } elsif ($tt eq 'vztmpl') {
> + if ($fn =~ m!$storage_path/(.+\.tar\.([gx]z))$!) {
> + return ["$tt/$1", "t$2"];
> + } else {
> + return undef;
> + }
> + } else {
> + die "Invalid parameter tt=$tt";
> + }
> +}
> +
> +# $tt = <iso|vztmpl>
> +sub get_subdir_files_rec {
> + my ($sid, $path, $tt, $vmid, $storage_path) = @_;
> + die "Invalid parameter" if not ($tt eq 'iso' || $tt eq 'vztmpl');
> + # Each call needs to know the original storage path
> + if (!defined $storage_path) {
> + $storage_path = $path;
> + }
> +
> + my @result = ();
> +
> + if (-d $path) {
this does not handly symlink loops (or symlinks at all). I am not sure
what we want to do about them though. they have to have been set up by
someone with shell access, so we could argue that any problems caused by
them are out of scope here (if we make recursive scanning opt-in).
> + foreach my $fn (<$path/*>) {
> + push @result, get_subdir_files_rec($sid, $fn, $tt, $vmid, $storage_path);
> + }
> + } else {
> + my $properties = get_file_properties($path, $storage_path, $tt);
> + if ($properties) {
> + my $file = { volid => "$sid:$$properties[0]", format => $$properties[1] };
> + $file->{size} = -s $path // 0;
> + push @result, $file;
> + }
> + }
> + return @result;
> +}
> +
> +# list templates ($tt = <backup|snippets>)
> my $get_subdir_files = sub {
> my ($sid, $path, $tt, $vmid) = @_;
> + die "Invalid parameter" if ($tt eq 'iso' || $tt eq 'vztmpl');
>
> my $res = [];
>
> @@ -900,18 +961,7 @@ my $get_subdir_files = sub {
> next if -d $fn;
>
> my $info;
> -
> - if ($tt eq 'iso') {
> - next if $fn !~ m!/([^/]+$PVE::Storage::iso_extension_re)$!i;
> -
> - $info = { volid => "$sid:iso/$1", format => 'iso' };
> -
> - } elsif ($tt eq 'vztmpl') {
> - next if $fn !~ m!/([^/]+\.tar\.([gx]z))$!;
> -
> - $info = { volid => "$sid:vztmpl/$1", format => "t$2" };
> -
> - } elsif ($tt eq 'backup') {
> + if ($tt eq 'backup') {
> next if defined($vmid) && $fn !~ m/\S+-$vmid-\S+/;
> next if $fn !~ m!/([^/]+\.(tar|tar\.gz|tar\.lzo|tgz|vma|vma\.gz|vma\.lzo))$!;
>
> @@ -947,9 +997,9 @@ sub list_volumes {
> my $path = $class->get_subdir($scfg, $type);
>
> if ($type eq 'iso' && !defined($vmid)) {
> - $data = $get_subdir_files->($storeid, $path, 'iso');
> + $data = [get_subdir_files_rec($storeid, $path, 'iso')];
> } elsif ($type eq 'vztmpl'&& !defined($vmid)) {
> - $data = $get_subdir_files->($storeid, $path, 'vztmpl');
> + $data = [get_subdir_files_rec($storeid, $path, 'vztmpl')];
> } elsif ($type eq 'backup') {
> $data = $get_subdir_files->($storeid, $path, 'backup', $vmid);
> } elsif ($type eq 'snippets') {
> diff --git a/test/Makefile b/test/Makefile
> index 833a597..525acf7 100644
> --- a/test/Makefile
> +++ b/test/Makefile
> @@ -1,6 +1,6 @@
> all: test
>
> -test: test_zfspoolplugin test_disklist test_bwlimit
> +test: test_zfspoolplugin test_disklist test_bwlimit test_paths
>
> test_zfspoolplugin: run_test_zfspoolplugin.pl
> ./run_test_zfspoolplugin.pl
> @@ -10,3 +10,6 @@ test_disklist: run_disk_tests.pl
>
> test_bwlimit: run_bwlimit_tests.pl
> ./run_bwlimit_tests.pl
> +
> +test_paths: run_path_tests.pl
> + ./run_path_tests.pl
> diff --git a/test/run_path_tests.pl b/test/run_path_tests.pl
> new file mode 100755
> index 0000000..638c939
> --- /dev/null
> +++ b/test/run_path_tests.pl
> @@ -0,0 +1,190 @@
> +#!/usr/bin/perl
> +use strict;
> +use warnings;
> +
> +use lib ('.', '..');
> +use Data::Dumper qw(Dumper);
> +
> +use Test::More tests => 37;
> +use Test::MockModule;
> +use File::Temp;
> +use File::Path qw(make_path);
> +use PVE::Storage::Plugin;
> +
> +my $dir_plugin = 'PVE::Storage::DirPlugin';
> +my $nfs_plugin = 'PVE::Storage::NFSPlugin';
> +my $basename = "test";
> +my $subdirectories = "some/more/subdirectories/";
> +
> +my $iso_vtype = "iso";
> +my $iso_suffix = ".iso";
> +my $iso_notdir = "$basename$iso_suffix";
> +my $iso_volname = "$iso_vtype/$iso_notdir";
> +my $iso_volname_with_subdirs = "$iso_vtype/$subdirectories$iso_notdir";
> +my $repeating_iso_subdir = "a/template/iso/b/template/iso/c/template/iso/";
> +
> +my $vztmpl_vtype = "vztmpl";
> +my $vztmpl_suffix = ".tar.gz";
> +my $vztmpl_notdir = "$basename$vztmpl_suffix";
> +my $vztmpl_volname = "$vztmpl_vtype/$vztmpl_notdir";
> +my $vztmpl_volname_with_subdirs = "$vztmpl_vtype/$subdirectories$vztmpl_notdir";
> +my $repeating_vztmpl_subdir = "a/template/cache/b/template/cache/c/template/cache/";
> +
> +my $image_vtype = "images";
> +my $vmid = "100";
> +my $image_basename = "vm-100-disk-0";
> +my $raw_image_format = "raw";
> +my $raw_image_notdir = "$image_basename.$raw_image_format";
> +my $raw_image_volname = "$vmid/$raw_image_notdir";
> +my $qcow_image_format = "qcow2";
> +my $qcow_image_notdir = "$image_basename.$qcow_image_format";
> +my $qcow_image_volname = "$vmid/$qcow_image_notdir";
> +my $vmdk_image_format = "vmdk";
> +my $vmdk_image_notdir = "$image_basename.$vmdk_image_format";
> +my $vmdk_image_volname = "$vmid/$vmdk_image_notdir";
> +{
> + use PVE::Storage;
> + my $vtype_index = 0;
> + my $notdir_index = 1;
> + my $format_index = 6;
> +
> + is (($dir_plugin->parse_volname($iso_volname))[$vtype_index],
> + $iso_vtype, "parse_volname: vtype for iso");
> + is (($dir_plugin->parse_volname($iso_volname))[$notdir_index],
> + $iso_notdir, "parse_volname: notdir for iso");
> + is (($dir_plugin->parse_volname($iso_volname_with_subdirs))[$vtype_index],
> + $iso_vtype, "parse_volname: vtype for iso in subdir");
> + is (($dir_plugin->parse_volname($iso_volname_with_subdirs))[$notdir_index],
> + $iso_notdir, "parse_volname: notdir for iso in subdir");
> +
> + is (($dir_plugin->parse_volname($vztmpl_volname))[$vtype_index],
> + $vztmpl_vtype, "parse_volname: vtype for vztmpl");
> + is (($dir_plugin->parse_volname($vztmpl_volname))[$notdir_index],
> + $vztmpl_notdir, "parse_volname: notdir for vztmpl");
> + is (($dir_plugin->parse_volname($vztmpl_volname_with_subdirs))[$vtype_index],
> + $vztmpl_vtype, "parse_volname: vtype for vztmpl in subdir");
> + is (($dir_plugin->parse_volname($vztmpl_volname_with_subdirs))[$notdir_index],
> + $vztmpl_notdir, "parse_volname: notdir for vztmpl in subdir");
> +
> + is (($dir_plugin->parse_volname($raw_image_volname))[$vtype_index],
> + $image_vtype, "parse_volname: vtype for raw image");
> + is (($dir_plugin->parse_volname($raw_image_volname))[$notdir_index],
> + $raw_image_notdir, "parse_volname: notdir for raw image");
> + is (($dir_plugin->parse_volname($raw_image_volname))[$format_index],
> + $raw_image_format, "parse_volname: format for raw image");
> +
> + is (($dir_plugin->parse_volname($qcow_image_volname))[$vtype_index],
> + $image_vtype, "parse_volname: vtype for qcow image");
> + is (($dir_plugin->parse_volname($qcow_image_volname))[$notdir_index],
> + $qcow_image_notdir, "parse_volname: notdir for qcow image");
> + is (($dir_plugin->parse_volname($qcow_image_volname))[$format_index],
> + $qcow_image_format, "parse_volname: format for qcow image");
> +
> + is (($dir_plugin->parse_volname($vmdk_image_volname))[$vtype_index],
> + $image_vtype, "parse_volname: vtype for vmdk image");
> + is (($dir_plugin->parse_volname($vmdk_image_volname))[$notdir_index],
> + $vmdk_image_notdir, "parse_volname: notdir for vmdk image");
> + is (($dir_plugin->parse_volname($vmdk_image_volname))[$format_index],
> + $vmdk_image_format, "parse_volname: format for vmdk image");
> +}
> +{
> + my $scfg = { path => '/some/path' };
> + is ($dir_plugin->get_subdir($scfg, "iso"), "$scfg->{path}/template/iso", "get_subdir for iso" );
> + is ($dir_plugin->get_subdir($scfg, "vztmpl"), "$scfg->{path}/template/cache", "get_subdir for vztmpl");
> + is ($dir_plugin->get_subdir($scfg, "backup"), "$scfg->{path}/dump", "get_subdir for backup");
> + is ($dir_plugin->get_subdir($scfg, "images"), "$scfg->{path}/images", "get_subdir for images");
> + is ($dir_plugin->get_subdir($scfg, "rootdir"), "$scfg->{path}/private", "get_subdir for rootdir");
> +}
> +{
> + my $scfg = { path => '/some/path' };
> + is ($dir_plugin->filesystem_path($scfg, $iso_volname, undef),
> + "$scfg->{path}/template/$iso_volname",
> + "filesystem_path for iso");
> + is ($dir_plugin->filesystem_path($scfg, $iso_volname_with_subdirs, undef),
> + "$scfg->{path}/template/$iso_volname_with_subdirs",
> + "filesystem_path for iso in subdirs");
> +
> + is ($dir_plugin->filesystem_path($scfg, $vztmpl_volname, undef),
> + "$scfg->{path}/template/cache/$vztmpl_notdir",
> + "filesystem_path for vztmpl");
> + is ($dir_plugin->filesystem_path($scfg, $vztmpl_volname_with_subdirs, undef),
> + "$scfg->{path}/template/cache/$subdirectories$vztmpl_notdir",
> + "filesystem_path for vztmpl in subdirs");
> +
> + is ($dir_plugin->filesystem_path($scfg, $raw_image_volname, undef),
> + "$scfg->{path}/images/$raw_image_volname",
> + "filesystem_path for raw images");
> + is ($dir_plugin->filesystem_path($scfg, $qcow_image_volname, undef),
> + "$scfg->{path}/images/$qcow_image_volname",
> + "filesystem_path for qcow image");
> + is ($dir_plugin->filesystem_path($scfg, $vmdk_image_volname, undef),
> + "$scfg->{path}/images/$vmdk_image_volname",
> + "filesystem_path for vmdk image");
> +}
> +
> +# @expected_volnames may be unsorted
> +# $required_vtype is a reference to an array
> +sub test_list_volumes {
> + my ($scfg, $required_vtype, @expected_volnames) = @_;
> + my $storage_id = "sid";
> + my $volumes_ref = $dir_plugin->list_volumes($storage_id, $scfg, undef, $required_vtype);
> + my @received = map {my $i = $_->{volid}; $i =~ s/$storage_id://g; $i } @$volumes_ref;
> + my @x = sort @received;
> + my @y = sort @expected_volnames;
> + ok ( @x ~~ @y, "list_volumes with required vtype [@$required_vtype]") || diag "Expected\n", Dumper (@y), "but received\n",
> + Dumper(@x);
> +}
> +{
> + my $storage_dir = File::Temp->newdir();
> + my @paths = (
> + "$storage_dir/template/cache",
> + "$storage_dir/template/cache/$subdirectories",
> + "$storage_dir/template/cache/$repeating_iso_subdir",
> + "$storage_dir/template/cache/$repeating_vztmpl_subdir",
> + "$storage_dir/template/iso",
> + "$storage_dir/template/iso/$subdirectories",
> + "$storage_dir/template/iso/$repeating_iso_subdir",
> + "$storage_dir/template/iso/$repeating_vztmpl_subdir",
> + "$storage_dir/images/$vmid");
> +
> + foreach my $path (@paths) {
> + make_path($path) || BAIL_OUT "Could not create directory $path!";
> + IO::File->new("$path/$iso_notdir", "w");
> + IO::File->new("$path/$iso_notdir", "w");
> + IO::File->new("$path/$raw_image_notdir", "w");
> + IO::File->new("$path/$qcow_image_notdir", "w");
> + IO::File->new("$path/$vmdk_image_notdir", "w");
> + IO::File->new("$path/$vztmpl_notdir", "w");
> + IO::File->new("$path/garbage", "w");
> + }
> +# print "Content of temporary directory is:\n", `tree $storage_dir`;
> + {
> + my $scfg = { path => "$storage_dir" };
> + test_list_volumes($scfg, ["$iso_vtype"], (
> + "$iso_vtype/$iso_notdir",
> + "$iso_vtype/$subdirectories$iso_notdir",
> + "$iso_vtype/$repeating_vztmpl_subdir$iso_notdir",
> + "$iso_vtype/$repeating_iso_subdir$iso_notdir"));
> + test_list_volumes($scfg, [$vztmpl_vtype], (
> + "$vztmpl_vtype/$vztmpl_notdir",
> + "$vztmpl_vtype/$subdirectories$vztmpl_notdir",
> + "$vztmpl_vtype/$repeating_vztmpl_subdir$vztmpl_notdir",
> + "$vztmpl_vtype/$repeating_iso_subdir$vztmpl_notdir"));
> + }
> + {
> + my $scfg = { path => "$storage_dir", type => "dir" };
> + my $pluginMock = Test::MockModule->new('PVE::Cluster');
> + $pluginMock->redefine('get_vmlist' => sub { return undef });
> + my @expected_volnames = ($qcow_image_volname, $raw_image_volname, $vmdk_image_volname);
> + note "Tests for undefined VM type => KVM/Qemu";
> + test_list_volumes($scfg, ["images"], @expected_volnames);
> + test_list_volumes($scfg, ["rootdir"], ());
> + test_list_volumes($scfg, ["images", "rootdir"], @expected_volnames);
> +
> + note "Tests for VM type lxc";
> + $pluginMock->redefine('get_vmlist' => sub { return {ids => {$vmid => { type => "lxc" }}}});
> + test_list_volumes($scfg, ["images"], ());
> + test_list_volumes($scfg, ["rootdir"], @expected_volnames);
> + test_list_volumes($scfg, ["images", "rootdir"], @expected_volnames);
> + }
> +}
> --
> 2.20.1
>
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
More information about the pve-devel
mailing list