[pve-devel] [PATCH-SERIES v9 manager/common/storage] fix #1710: add download from url button

[pve-devel] [PATCH-SERIES v9 manager/common/storage] fix #1710: add download from url button

[Lorenz Stechauner]

changes to v8:
* explanation why 'print "\n"' is needed
* move check for existing file outside eval block


pve-manager:
Lorenz Stechauner (5):
  api: nodes: add query_url_metadata method
  api: nodes: refactor aplinfo to use common download function
  ui: add HashAlgorithmSelector
  ui: Utils: change download task format
  fix #1710: ui: storage: add download from url button

 PVE/API2/Nodes.pm                          | 167 ++++++++------
 www/manager6/Makefile                      |   1 +
 www/manager6/Utils.js                      |   2 +-
 www/manager6/form/HashAlgorithmSelector.js |  16 ++
 www/manager6/storage/Browser.js            |   8 +
 www/manager6/storage/ContentView.js        | 247 +++++++++++++++++++--
 6 files changed, 351 insertions(+), 90 deletions(-)
 create mode 100644 www/manager6/form/HashAlgorithmSelector.js


pve-common:
Lorenz Stechauner (2):
  tools: download_file_from_url: adapt error messages to start at new
    line
  tools: download_file_from_url: move check for existing file outside
    eval

 src/PVE/Tools.pm | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)


pve-storage:
Lorenz Stechauner (1):
  status: add download_url method

 PVE/API2/Storage/Status.pm | 128 +++++++++++++++++++++++++++++++++++--
 PVE/Storage.pm             |  10 +++
 2 files changed, 133 insertions(+), 5 deletions(-)

-- 
2.20.1

[pve-devel] [PATCH v9 common 1/2] tools: download_file_from_url: adapt error messages to start at new line

[Lorenz Stechauner]

the front end expects the error message to be the first part of the
last line. putting the new line at the beginning of the die message
does not work, either.

https://lists.proxmox.com/pipermail/pve-devel/2021-June/048676.html
---
 src/PVE/Tools.pm | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
index c90810c..3cf7c4d 100644
--- a/src/PVE/Tools.pm
+++ b/src/PVE/Tools.pm
@@ -1873,7 +1873,8 @@ sub download_file_from_url {
 		return;
 	    } else {
 		# we could re-download, but may not be safe so just abort for now..
-		die "mismatch (got '$checksum_got' != expect '$checksum_expected'), aborting\n";
+		print "\n";  # the front end expects the error to reside at the last line without any noise
+		die "checksum mismatch: got '$checksum_got' != expect '$checksum_expected', aborting\n";
 	    }
 	}
 
@@ -1908,7 +1909,8 @@ sub download_file_from_url {
 	    if (lc($checksum_got) eq lc($checksum_expected)) {
 		print "OK, checksum verified\n";
 	    } else {
-		die "ERROR, checksum mismatch: got '$checksum_got' != expect '$checksum_expected'\n";
+		print "\n";  # the front end expects the error to reside at the last line without any noise
+		die "checksum mismatch: got '$checksum_got' != expect '$checksum_expected'\n";
 	    }
 	}
 
-- 
2.20.1

[pve-devel] [PATCH v9 common 2/2] tools: download_file_from_url: move check for existing file outside eval

[Lorenz Stechauner]

it is not necessary to include this block in the eval which when it
fails tries to unlink $tmpdest, because in the check for the existing
file $tmpdest is not used.
---
 src/PVE/Tools.pm | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
index 3cf7c4d..9046b4f 100644
--- a/src/PVE/Tools.pm
+++ b/src/PVE/Tools.pm
@@ -1862,22 +1862,22 @@ sub download_file_from_url {
 
     print "downloading $url to $dest\n";
 
-    my $tmpdest = "$dest.tmp.$$";
-    eval {
-	if (-f $dest && $checksum_algorithm) {
-	    print "calculating checksum of existing file...";
-	    my $checksum_got = get_file_hash($checksum_algorithm, $dest);
+    if (-f $dest && $checksum_algorithm) {
+	print "calculating checksum of existing file...";
+	my $checksum_got = get_file_hash($checksum_algorithm, $dest);
 
-	    if (lc($checksum_got) eq lc($checksum_expected)) {
-		print "OK, got correct file already, no need to download\n";
-		return;
-	    } else {
-		# we could re-download, but may not be safe so just abort for now..
-		print "\n";  # the front end expects the error to reside at the last line without any noise
-		die "checksum mismatch: got '$checksum_got' != expect '$checksum_expected', aborting\n";
-	    }
+	if (lc($checksum_got) eq lc($checksum_expected)) {
+	    print "OK, got correct file already, no need to download\n";
+	    return;
+	} else {
+	    # we could re-download, but may not be safe so just abort for now..
+	    print "\n";  # the front end expects the error to reside at the last line without any noise
+	    die "checksum mismatch: got '$checksum_got' != expect '$checksum_expected', aborting\n";
 	}
+    }
 
+    my $tmpdest = "$dest.tmp.$$";
+    eval {
 	local $SIG{INT} = sub {
 	    unlink $tmpdest or warn "could not cleanup temporary file: $!";
 	    die "got interrupted by signal\n";
-- 
2.20.1

[pve-devel] [PATCH v9 storage 1/1] status: add download_url method

[Lorenz Stechauner]

uses common function PVE::Tools::download_file_from_url to download
iso files.

Only users with permissions `Sys.Audit` and `Sys.Modify` on `/` are
permitted to perform this action. This restriction is due to the
fact, that the download function is able to download files from
internal networks (which are not visible/accessible from outside).
Users with these permissions anyway have the means to alter node
(network) config, so this does not create any further security risk.

Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
---
 PVE/API2/Storage/Status.pm | 128 +++++++++++++++++++++++++++++++++++--
 PVE/Storage.pm             |  10 +++
 2 files changed, 133 insertions(+), 5 deletions(-)

diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
index 897b4a7..584ba5d 100644
--- a/PVE/API2/Storage/Status.pm
+++ b/PVE/API2/Storage/Status.pm
@@ -412,11 +412,7 @@ __PACKAGE__->register_method ({
 	my $size = -s $tmpfilename;
 	die "temporary file '$tmpfilename' does not exist\n" if !defined($size);
 
-	my $filename = $param->{filename};
-
-	chomp $filename;
-	$filename =~ s/^.*[\/\\]//;
-	$filename =~ s/[^-a-zA-Z0-9_.]/_/g;
+	my $filename = PVE::Storage::normalize_content_filename($param->{filename});
 
 	my $path;
 
@@ -497,4 +493,126 @@ __PACKAGE__->register_method ({
 	return $upid;
    }});
 
+__PACKAGE__->register_method({
+    name => 'download_url',
+    path => '{storage}/download-url',
+    method => 'POST',
+    description => "Download templates and ISO images by using an URL.",
+    proxyto => 'node',
+    permissions => {
+	check => [ 'and',
+	    ['perm', '/storage/{storage}', [ 'Datastore.AllocateTemplate' ]],
+	    ['perm', '/', [ 'Sys.Audit', 'Sys.Modify' ]],
+	],
+    },
+    protected => 1,
+    parameters => {
+	additionalProperties => 0,
+	properties => {
+	    node => get_standard_option('pve-node'),
+	    storage => get_standard_option('pve-storage-id'),
+	    url => {
+		description => "The URL to download the file from.",
+		type => 'string',
+		pattern => 'https?://.*',
+	    },
+	    content => {
+		description => "Content type.",
+		type => 'string', format => 'pve-storage-content',
+	    },
+	    filename => {
+		description => "The name of the file to create.",
+		type => 'string',
+	    },
+	    checksum => {
+		description => "The expected checksum of the file.",
+		type => 'string',
+		requires => 'checksum-algorithm',
+		optional => 1,
+	    },
+	    'checksum-algorithm' => {
+		description => "The algorithm to calculate the checksum of the file.",
+		type => 'string',
+		enum => ['md5', 'sha1', 'sha224', 'sha256', 'sha384', 'sha512'],
+		requires => 'checksum',
+		optional => 1,
+	    },
+	    'verify-certificates' => {
+		description => "If false, no SSL/TLS certificates will be verified.",
+		type => 'boolean',
+		optional => 1,
+		default => 1,
+	    }
+	},
+    },
+    returns => {
+	type => "string"
+    },
+    code => sub {
+	my ($param) = @_;
+
+	my $cfg = PVE::Storage::config();
+
+	my ($node, $storage) = $param->@{'node', 'storage'};
+	my $scfg = PVE::Storage::storage_check_enabled($cfg, $storage, $node);
+
+	die "can't upload to storage type '$scfg->{type}, not a file based storage!'\n" if !defined($scfg->{path});
+
+	my ($content, $url) = $param->@{'content', 'url'};
+
+	my $filename = PVE::Storage::normalize_content_filename($param->{filename});
+	my $path;
+
+	# MIME type is checked in front end only
+	# this check is omitted here intentionally and replaced by file extension check
+	if ($content eq 'iso') {
+	    if ($filename !~ m![^/]+$PVE::Storage::iso_extension_re$!) {
+		raise_param_exc({ filename => "missing '.iso' or '.img' extension" });
+	    }
+	    $path = PVE::Storage::get_iso_dir($cfg, $storage);
+	} elsif ($content eq 'vztmpl') {
+	    if ($filename !~ m![^/]+\.tar\.[gx]z$!) {
+		raise_param_exc({ filename => "missing '.tar.gz' or '.tar.xz' extension" });
+	    }
+	    $path = PVE::Storage::get_vztmpl_dir($cfg, $storage);
+	} else {
+	    raise_param_exc({ content => "upload content type '$content' not allowed" });
+	}
+
+	die "storage '$storage' does not support '$content' content\n" if !$scfg->{content}->{$content};
+
+	PVE::Storage::activate_storage($cfg, $storage);
+	File::Path::make_path($path);
+
+	my $dest = "$path/$filename";
+
+	my $opts = {
+	    hash_required => 0,
+	    verify_certificates => $param->{'verify-certificates'} // 1,
+	};
+
+	my ($checksum, $checksum_algorithm) = $param->@{'checksum', 'checksum-algorithm'};
+	if ($checksum) {
+	    $opts->{"${checksum_algorithm}sum"} = $checksum;
+	    $opts->{hash_required} = 1;
+	}
+
+	my $dccfg = PVE::Cluster::cfs_read_file('datacenter.cfg');
+	if ($dccfg->{http_proxy}) {
+	    $opts->{http_proxy} = $dccfg->{http_proxy};
+	}
+
+	my $worker = sub {
+	    my $upid = shift;
+	    PVE::Tools::download_file_from_url($dest, $url, $opts);
+	};
+
+	my $rpcenv = PVE::RPCEnvironment::get();
+	my $user = $rpcenv->get_user();
+
+	my $upid = $rpcenv->fork_worker('download', $filename, $user, $worker);
+
+	return $upid;
+    }});
+
 1;
diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index aa36bad..f9a3d49 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -1929,4 +1929,14 @@ sub assert_sid_unused {
     return undef;
 }
 
+sub normalize_content_filename {
+    my ($filename) = @_;
+
+    chomp $filename;
+    $filename =~ s/^.*[\/\\]//;
+    $filename =~ s/[^-a-zA-Z0-9_.]/_/g;
+
+    return $filename;
+}
+
 1;
-- 
2.20.1

[pve-devel] [PATCH v9 manager 1/5] api: nodes: add query_url_metadata method

[Lorenz Stechauner]

metadata is gained using a HEAD request.

Due to the ability of this api endpoint to request files on internal
networks (which would not be visible/accessible from outside) it is
restricted to users with permissions `Sys.Audit` and `Sys.Modify` on
`/`. Users with these permissions are able to alter node (network)
config anyway, so this should not create any further security risk.

Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
---
 PVE/API2/Nodes.pm | 96 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 96 insertions(+)

diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
index e58d9c10..77fa710a 100644
--- a/PVE/API2/Nodes.pm
+++ b/PVE/API2/Nodes.pm
@@ -11,6 +11,7 @@ use JSON;
 use POSIX qw(LONG_MAX);
 use Time::Local qw(timegm_nocheck);
 use Socket;
+use IO::Socket::SSL;
 
 use PVE::API2Tools;
 use PVE::APLInfo;
@@ -238,6 +239,7 @@ __PACKAGE__->register_method ({
 	    { name => 'netstat' },
 	    { name => 'network' },
 	    { name => 'qemu' },
+	    { name => 'query-url-metadata' },
 	    { name => 'replication' },
 	    { name => 'report' },
 	    { name => 'rrd' }, # fixme: remove?
@@ -1595,6 +1597,100 @@ __PACKAGE__->register_method({
 	return $rpcenv->fork_worker('download', undef, $user, $worker);
     }});
 
+__PACKAGE__->register_method({
+    name => 'query_url_metadata',
+    path => 'query-url-metadata',
+    method => 'GET',
+    description => "Query metadata of an URL: file size, file name and mime type.",
+    proxyto => 'node',
+    permissions => {
+	check => ['perm', '/', [ 'Sys.Audit', 'Sys.Modify' ]],
+    },
+    parameters => {
+	additionalProperties => 0,
+	properties => {
+	    node => get_standard_option('pve-node'),
+	    url => {
+		description => "The URL to query the metadata from.",
+		type => 'string',
+		pattern => 'https?://.*',
+	    },
+	    'verify-certificates' => {
+		description => "If false, no SSL/TLS certificates will be verified.",
+		type => 'boolean',
+		optional => 1,
+		default => 1,
+	    }
+	},
+    },
+    returns => {
+	type => "object",
+	properties => {
+	    filename => {
+		type => 'string',
+		optional => 1,
+	    },
+	    size => {
+		type => 'integer',
+		renderer => 'bytes',
+		optional => 1,
+	    },
+	    mimetype => {
+		type => 'string',
+		optional => 1,
+	    },
+	},
+    },
+    code => sub {
+	my ($param) = @_;
+
+	my $url = $param->{url};
+
+	my $ua = LWP::UserAgent->new();
+
+	my $dccfg = PVE::Cluster::cfs_read_file('datacenter.cfg');
+	if ($dccfg->{http_proxy}) {
+	    $ua->proxy('http', $dccfg->{http_proxy});
+	}
+
+	my $verify = $param->{'verify-certificates'} // 1;
+	if (!$verify) {
+	    $ua->ssl_opts(
+		verify_hostname => 0,
+		SSL_verify_mode => IO::Socket::SSL::SSL_VERIFY_NONE,
+	    );
+	}
+
+	my $req = HTTP::Request->new(HEAD => $url);
+	my $res = $ua->request($req);
+
+	die "invalid server response: '" . $res->status_line() . "'\n" if ($res->code() != 200);
+
+	my $size = $res->header("Content-Length");
+	my $disposition = $res->header("Content-Disposition");
+	my $type = $res->header("Content-Type");
+
+	my $filename;
+
+	if ($disposition && ($disposition =~ m/filename="([^"]*)"/ || $disposition =~ m/filename=([^;]*)/)) {
+	    $filename = $1;
+	} elsif ($url =~ m!^[^?]+/([^?/]*)(?:\?.*)?$!) {
+	    $filename = $1;
+	}
+
+	# Content-Type: text/html; charset=utf-8
+	if ($type && $type =~ m/^([^;]+);/) {
+	    $type = $1;
+	}
+
+	my $ret = {};
+	$ret->{filename} = $filename if $filename;
+	$ret->{size} = $size + 0 if $size;
+	$ret->{mimetype} = $type if $type;
+
+	return $ret;
+    }});
+
 __PACKAGE__->register_method({
     name => 'report',
     path => 'report',
-- 
2.20.1

[pve-devel] [PATCH v9 manager 2/5] api: nodes: refactor aplinfo to use common download function

[Lorenz Stechauner]

a common function to download arbitrary files from urls has been
defined as PVE::Tools::download_file_from_url and is now used.

Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
---
 PVE/API2/Nodes.pm | 93 +++++++++--------------------------------------
 1 file changed, 17 insertions(+), 76 deletions(-)

diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
index 77fa710a..740547e7 100644
--- a/PVE/API2/Nodes.pm
+++ b/PVE/API2/Nodes.pm
@@ -1513,88 +1513,29 @@ __PACKAGE__->register_method({
 	my $src = $pd->{location};
 	my $tmpldir = PVE::Storage::get_vztmpl_dir($cfg, $param->{storage});
 	my $dest = "$tmpldir/$template";
-	my $tmpdest = "$tmpldir/${template}.tmp.$$";
 
-	my $worker = sub  {
-	    my $upid = shift;
-
-	    print "starting template download from: $src\n";
-	    print "target file: $dest\n";
-
-	    my $check_hash = sub {
-		my ($template_info, $filename, $noerr) = @_;
-
-		my $digest;
-		my $expected;
-
-		eval {
-		    open(my $fh, '<', $filename) or die "Can't open '$filename': $!";
-		    binmode($fh);
-		    if (defined($template_info->{sha512sum})) {
-			$expected = $template_info->{sha512sum};
-			$digest = Digest::SHA->new(512)->addfile($fh)->hexdigest;
-		    } elsif (defined($template_info->{md5sum})) {
-			#fallback to MD5
-			$expected = $template_info->{md5sum};
-			$digest = Digest::MD5->new->addfile($fh)->hexdigest;
-		    } else {
-			die "no expected checksum defined";
-		    }
-		    close($fh);
-		};
-
-		die "checking hash failed - $@\n" if $@ && !$noerr;
-
-		return ($digest, $digest ? lc($digest) eq lc($expected) : 0);
-	    };
-
-	    eval {
-		if (-f $dest) {
-		    my ($hash, $correct) = &$check_hash($pd, $dest, 1);
-
-		    if ($hash && $correct) {
-			print "file already exists $hash - no need to download\n";
-			return;
-		    }
-		}
-
-		local %ENV;
-		my $dccfg = PVE::Cluster::cfs_read_file('datacenter.cfg');
-		if ($dccfg->{http_proxy}) {
-		    $ENV{http_proxy} = $dccfg->{http_proxy};
-		}
-
-		my @cmd = ('/usr/bin/wget', '--progress=dot:mega', '-O', $tmpdest, $src);
-		if (system (@cmd) != 0) {
-		    die "download failed - $!\n";
-		}
-
-		my ($hash, $correct) = &$check_hash($pd, $tmpdest);
-
-		die "could not calculate checksum\n" if !$hash;
-
-		if (!$correct) {
-		    my $expected = $pd->{sha512sum} // $pd->{md5sum};
-		    die "wrong checksum: $hash != $expected\n";
-		}
+	my $opts = {
+	    hash_required => 1,
+	    sha512sum => $pd->{sha512sum},
+	    md5sum => $pd->{md5sum},
+	};
 
-		if (!rename($tmpdest, $dest)) {
-		    die "unable to save file - $!\n";
-		}
-	    };
-	    my $err = $@;
+	my $dccfg = PVE::Cluster::cfs_read_file('datacenter.cfg');
+	if ($dccfg->{http_proxy}) {
+	    $opts->{http_proxy} = $dccfg->{http_proxy};
+	}
 
-	    unlink $tmpdest;
+	my $worker = sub {
+	    my $upid = shift;
+	    PVE::Tools::download_file_from_url($dest, $src, $opts);
+	};
 
-	    if ($err) {
-		print "\n";
-		die $err if $err;
-	    }
+	my $rpcenv = PVE::RPCEnvironment::get();
+	my $user = $rpcenv->get_user();
 
-	    print "download finished\n";
-	};
+	my $upid = $rpcenv->fork_worker('download', $template, $user, $worker);
 
-	return $rpcenv->fork_worker('download', undef, $user, $worker);
+	return $upid;
     }});
 
 __PACKAGE__->register_method({
-- 
2.20.1

[pve-devel] [PATCH v9 manager 3/5] ui: add HashAlgorithmSelector

[Lorenz Stechauner]

Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
---
 www/manager6/Makefile                      |  1 +
 www/manager6/form/HashAlgorithmSelector.js | 16 ++++++++++++++++
 2 files changed, 17 insertions(+)
 create mode 100644 www/manager6/form/HashAlgorithmSelector.js

diff --git a/www/manager6/Makefile b/www/manager6/Makefile
index 506b5a4e..1aa62c9b 100644
--- a/www/manager6/Makefile
+++ b/www/manager6/Makefile
@@ -38,6 +38,7 @@ JSSRC= 							\
 	form/GlobalSearchField.js			\
 	form/GroupSelector.js				\
 	form/GuestIDSelector.js				\
+	form/HashAlgorithmSelector.js			\
 	form/HotplugFeatureSelector.js			\
 	form/IPProtocolSelector.js			\
 	form/IPRefSelector.js				\
diff --git a/www/manager6/form/HashAlgorithmSelector.js b/www/manager6/form/HashAlgorithmSelector.js
new file mode 100644
index 00000000..5ae7a08b
--- /dev/null
+++ b/www/manager6/form/HashAlgorithmSelector.js
@@ -0,0 +1,16 @@
+Ext.define('PVE.form.hashAlgorithmSelector', {
+    extend: 'Proxmox.form.KVComboBox',
+    alias: ['widget.pveHashAlgorithmSelector'],
+    config: {
+	deleteEmpty: false,
+    },
+    comboItems: [
+	['__default__', 'None'],
+	['md5', 'MD5'],
+	['sha1', 'SHA-1'],
+	['sha224', 'SHA-224'],
+	['sha256', 'SHA-256'],
+	['sha384', 'SHA-384'],
+	['sha512', 'SHA-512'],
+    ],
+});
-- 
2.20.1

[pve-devel] [PATCH v9 manager 4/5] ui: Utils: change download task format

[Lorenz Stechauner]

Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
---
 www/manager6/Utils.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index d9567979..9fef29bf 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -1776,7 +1776,7 @@ Ext.define('PVE.Utils', {
 	    clusterjoin: ['', gettext('Join Cluster')],
 	    dircreate: [gettext('Directory Storage'), gettext('Create')],
 	    dirremove: [gettext('Directory'), gettext('Remove')],
-	    download: ['', gettext('Download')],
+	    download: [gettext('File'), gettext('Download')],
 	    hamigrate: ['HA', gettext('Migrate')],
 	    hashutdown: ['HA', gettext('Shutdown')],
 	    hastart: ['HA', gettext('Start')],
-- 
2.20.1

[pve-devel] [PATCH v9 manager 5/5] fix #1710: ui: storage: add download from url button

[Lorenz Stechauner]

uses the common function PVE::Tools::download_file_from_url to
download a iso image or container template.

note: Only users with permissions `Sys.Audit` and `Sys.Modify` on
`/` are permitted to use the api endpoints due to security reasons.
(it is possible to download files from internal networks which would
be not visible/accessible from outside)

Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
---
 www/manager6/storage/Browser.js     |   8 +
 www/manager6/storage/ContentView.js | 247 +++++++++++++++++++++++++---
 2 files changed, 231 insertions(+), 24 deletions(-)

diff --git a/www/manager6/storage/Browser.js b/www/manager6/storage/Browser.js
index 5fee94c7..fe5df3e2 100644
--- a/www/manager6/storage/Browser.js
+++ b/www/manager6/storage/Browser.js
@@ -53,6 +53,9 @@ Ext.define('PVE.storage.Browser', {
 	    let plugin = res.plugintype;
 	    let contents = res.content.split(',');
 
+	    let enableUpload = !!caps.storage['Datastore.AllocateTemplate'];
+	    let enableDownloadUrl = enableUpload && !!(caps.nodes['Sys.Audit'] && caps.nodes['Sys.Modify']);
+
 	    if (contents.includes('backup')) {
 		me.items.push({
 		    xtype: 'pveStorageBackupView',
@@ -91,6 +94,8 @@ Ext.define('PVE.storage.Browser', {
 		    itemId: 'contentIso',
 		    content: 'iso',
 		    pluginType: plugin,
+		    enableUploadButton: enableUpload,
+		    enableDownloadUrlButton: enableDownloadUrl,
 		    useUploadButton: true,
 		});
 	    }
@@ -101,6 +106,9 @@ Ext.define('PVE.storage.Browser', {
 		    iconCls: 'fa fa-file-o lxc',
 		    itemId: 'contentVztmpl',
 		    pluginType: plugin,
+		    enableUploadButton: enableUpload,
+		    enableDownloadUrlButton: enableDownloadUrl,
+		    useUploadButton: true,
 		});
 	    }
 	    if (contents.includes('snippets')) {
diff --git a/www/manager6/storage/ContentView.js b/www/manager6/storage/ContentView.js
index dd6df4b1..6171d30c 100644
--- a/www/manager6/storage/ContentView.js
+++ b/www/manager6/storage/ContentView.js
@@ -191,6 +191,191 @@ Ext.define('PVE.storage.Upload', {
     },
 });
 
+Ext.define('PVE.storage.DownloadUrl', {
+    extend: 'Proxmox.window.Edit',
+    alias: 'widget.pveStorageDownloadUrl',
+    mixins: ['Proxmox.Mixin.CBind'],
+
+    isCreate: true,
+
+    method: 'POST',
+
+    showTaskViewer: true,
+
+    title: gettext('Download from URL'),
+    submitText: gettext('Download'),
+
+    cbindData: function(initialConfig) {
+	var me = this;
+	return {
+	    nodename: me.nodename,
+	    storage: me.storage,
+	    contents: me.contents,
+	    content: me.contents[0],
+	};
+    },
+
+    cbind: {
+	url: '/nodes/{nodename}/storage/{storage}/download-url',
+    },
+
+    controller: {
+	xclass: 'Ext.app.ViewController',
+
+	urlChange: function(field) {
+	    let me = this;
+	    let view = me.getView();
+	    field = view.down('[name=url]');
+	    field.setValidation("Waiting for response...");
+	    field.validate();
+	    view.setValues({ size: "" });
+	    Proxmox.Utils.API2Request({
+		url: `/nodes/${view.nodename}/query-url-metadata`,
+		method: 'GET',
+		params: {
+		    url: field.getValue(),
+		    'verify-certificates': view.getValues()['verify-certificates'],
+		},
+		failure: function(res, opt) {
+		    field.setValidation(res.result.message);
+		    field.validate();
+		    view.setValues({
+			size: "",
+			mimetype: "",
+		    });
+		},
+		success: function(res, opt) {
+		    field.setValidation();
+		    field.validate();
+
+		    let data = res.result.data;
+		    view.setValues({
+			filename: data.filename || "",
+			size: (data.size && Proxmox.Utils.format_size(data.size)) || "",
+			mimetype: data.mimetype || "",
+		    });
+		},
+	    });
+	},
+
+	hashChange: function(field) {
+	    let checksum = Ext.getCmp('downloadUrlChecksum');
+	    if (field.getValue() === '__default__') {
+		checksum.setDisabled(true);
+		checksum.setValue("");
+		checksum.allowBlank = true;
+	    } else {
+		checksum.setDisabled(false);
+		checksum.allowBlank = false;
+	    }
+	},
+    },
+
+    items: [
+	{
+	    xtype: 'inputpanel',
+	    waitMsgTarget: true,
+	    border: false,
+	    columnT: [
+		{
+		    xtype: 'textfield',
+		    name: 'url',
+		    allowBlank: false,
+		    fieldLabel: gettext('URL'),
+		    listeners: {
+			change: {
+			    buffer: 500,
+			    fn: 'urlChange',
+			},
+		    },
+		},
+		{
+		    xtype: 'textfield',
+		    name: 'filename',
+		    allowBlank: false,
+		    fieldLabel: gettext('File name'),
+		},
+	    ],
+	    column1: [
+		{
+		    xtype: 'pveContentTypeSelector',
+		    fieldLabel: gettext('Content'),
+		    name: 'content',
+		    allowBlank: false,
+		    cbind: {
+			cts: '{contents}',
+			value: '{content}',
+		    },
+		},
+	    ],
+	    column2: [
+		{
+		    xtype: 'textfield',
+		    name: 'size',
+		    disabled: true,
+		    fieldLabel: gettext('File size'),
+		    emptyText: gettext('unknown'),
+		},
+	    ],
+	    advancedColumn1: [
+		{
+		    xtype: 'pveHashAlgorithmSelector',
+		    name: 'checksum-algorithm',
+		    fieldLabel: gettext('Hash algorithm'),
+		    allowBlank: true,
+		    hasNoneOption: true,
+		    value: '__default__',
+		    listeners: {
+			change: 'hashChange',
+		    },
+		},
+		{
+		    xtype: 'textfield',
+		    name: 'checksum',
+		    fieldLabel: gettext('Checksum'),
+		    allowBlank: true,
+		    disabled: true,
+		    emptyText: gettext('none'),
+		    id: 'downloadUrlChecksum',
+		},
+	    ],
+	    advancedColumn2: [
+		{
+		    xtype: 'textfield',
+		    fieldLabel: gettext('MIME type'),
+		    name: 'mimetype',
+		    disabled: true,
+		    editable: false,
+		    emptyText: gettext('unknown'),
+		},
+		{
+		    xtype: 'proxmoxcheckbox',
+		    name: 'verify-certificates',
+		    fieldLabel: gettext('Verify certificates'),
+		    uncheckedValue: 0,
+		    checked: true,
+		    listeners: {
+			change: 'urlChange',
+		    },
+		},
+	    ],
+	},
+    ],
+
+    initComponent: function() {
+        var me = this;
+
+	if (!me.nodename) {
+	    throw "no node name specified";
+	}
+	if (!me.storage) {
+	    throw "no storage ID specified";
+	}
+
+        me.callParent();
+    },
+});
+
 Ext.define('PVE.storage.ContentView', {
     extend: 'Ext.grid.GridPanel',
 
@@ -249,36 +434,50 @@ Ext.define('PVE.storage.ContentView', {
 
 	Proxmox.Utils.monStoreErrors(me, store);
 
-	let uploadButton = Ext.create('Proxmox.button.Button', {
-	    text: gettext('Upload'),
-	    handler: function() {
-		let win = Ext.create('PVE.storage.Upload', {
-		    nodename: nodename,
-		    storage: storage,
-		    contents: [content],
-		});
-		win.show();
-		win.on('destroy', reload);
-	    },
-	});
-
-	let removeButton = Ext.create('Proxmox.button.StdRemoveButton', {
-	    selModel: sm,
-	    delay: 5,
-	    callback: function() {
-		reload();
-	    },
-	    baseurl: baseurl + '/',
-	});
-
 	if (!me.tbar) {
 	    me.tbar = [];
 	}
 	if (me.useUploadButton) {
-	    me.tbar.push(uploadButton);
+	    me.tbar.unshift(
+		{
+		    xtype: 'button',
+		    text: gettext('Upload'),
+		    disabled: !me.enableUploadButton,
+		    handler: function() {
+			Ext.create('PVE.storage.Upload', {
+			    nodename: nodename,
+			    storage: storage,
+			    contents: [content],
+			    autoShow: true,
+			    taskDone: () => reload(),
+			});
+		    },
+		},
+		{
+		    xtype: 'button',
+		    text: gettext('Download from URL'),
+		    disabled: !me.enableDownloadUrlButton,
+		    handler: function() {
+			Ext.create('PVE.storage.DownloadUrl', {
+			    nodename: nodename,
+			    storage: storage,
+			    contents: [content],
+			    autoShow: true,
+			    taskDone: () => reload(),
+			});
+		    },
+		},
+		'-',
+	    );
 	}
 	if (!me.useCustomRemoveButton) {
-	    me.tbar.push(removeButton);
+	    me.tbar.push({
+		xtype: 'proxmoxStdRemoveButton',
+		selModel: sm,
+		delay: 5,
+		callback: () => reload(),
+		baseurl: baseurl + '/',
+	    });
 	}
 	me.tbar.push(
 	    '->',
-- 
2.20.1

[pve-devel] applied: [PATCH v9 common 1/2] tools: download_file_from_url: adapt error messages to start at new line

[Thomas Lamprecht]

On 16.06.21 11:35, Lorenz Stechauner wrote:
> the front end expects the error message to be the first part of the
> last line. putting the new line at the beginning of the die message
> does not work, either.
> 
> https://lists.proxmox.com/pipermail/pve-devel/2021-June/048676.html
> ---
>  src/PVE/Tools.pm | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
>

applied, thanks!

[pve-devel] applied: [PATCH v9 common 2/2] tools: download_file_from_url: move check for existing file outside eval

[Thomas Lamprecht]

On 16.06.21 11:35, Lorenz Stechauner wrote:
> it is not necessary to include this block in the eval which when it
> fails tries to unlink $tmpdest, because in the check for the existing
> file $tmpdest is not used.
> ---
>  src/PVE/Tools.pm | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 


applied, thanks! FYI I made a not directly related followup (send to the list).

Re: [pve-devel] [PATCH v9 manager 2/5] api: nodes: refactor aplinfo to use common download function

[Thomas Lamprecht]

On 16.06.21 11:36, Lorenz Stechauner wrote:
> a common function to download arbitrary files from urls has been
> defined as PVE::Tools::download_file_from_url and is now used.
> 
> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
> ---
>  PVE/API2/Nodes.pm | 93 +++++++++--------------------------------------
>  1 file changed, 17 insertions(+), 76 deletions(-)
> 
> diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
> index 77fa710a..740547e7 100644
> --- a/PVE/API2/Nodes.pm
> +++ b/PVE/API2/Nodes.pm
> @@ -1513,88 +1513,29 @@ __PACKAGE__->register_method({
>  	my $src = $pd->{location};
>  	my $tmpldir = PVE::Storage::get_vztmpl_dir($cfg, $param->{storage});
>  	my $dest = "$tmpldir/$template";
> -	my $tmpdest = "$tmpldir/${template}.tmp.$$";
>  
> -	my $worker = sub  {
> -	    my $upid = shift;
> -
> -	    print "starting template download from: $src\n";
> -	    print "target file: $dest\n";
> -
> -	    my $check_hash = sub {
> -		my ($template_info, $filename, $noerr) = @_;
> -
> -		my $digest;
> -		my $expected;
> -
> -		eval {
> -		    open(my $fh, '<', $filename) or die "Can't open '$filename': $!";
> -		    binmode($fh);
> -		    if (defined($template_info->{sha512sum})) {
> -			$expected = $template_info->{sha512sum};
> -			$digest = Digest::SHA->new(512)->addfile($fh)->hexdigest;
> -		    } elsif (defined($template_info->{md5sum})) {
> -			#fallback to MD5
> -			$expected = $template_info->{md5sum};
> -			$digest = Digest::MD5->new->addfile($fh)->hexdigest;
> -		    } else {
> -			die "no expected checksum defined";
> -		    }
> -		    close($fh);
> -		};
> -
> -		die "checking hash failed - $@\n" if $@ && !$noerr;
> -
> -		return ($digest, $digest ? lc($digest) eq lc($expected) : 0);
> -	    };
> -
> -	    eval {
> -		if (-f $dest) {
> -		    my ($hash, $correct) = &$check_hash($pd, $dest, 1);
> -
> -		    if ($hash && $correct) {
> -			print "file already exists $hash - no need to download\n";
> -			return;
> -		    }
> -		}
> -
> -		local %ENV;
> -		my $dccfg = PVE::Cluster::cfs_read_file('datacenter.cfg');
> -		if ($dccfg->{http_proxy}) {
> -		    $ENV{http_proxy} = $dccfg->{http_proxy};
> -		}
> -
> -		my @cmd = ('/usr/bin/wget', '--progress=dot:mega', '-O', $tmpdest, $src);
> -		if (system (@cmd) != 0) {
> -		    die "download failed - $!\n";
> -		}
> -
> -		my ($hash, $correct) = &$check_hash($pd, $tmpdest);
> -
> -		die "could not calculate checksum\n" if !$hash;
> -
> -		if (!$correct) {
> -		    my $expected = $pd->{sha512sum} // $pd->{md5sum};
> -		    die "wrong checksum: $hash != $expected\n";
> -		}
> +	my $opts = {
> +	    hash_required => 1,
> +	    sha512sum => $pd->{sha512sum},
> +	    md5sum => $pd->{md5sum},
> +	};
>  
> -		if (!rename($tmpdest, $dest)) {
> -		    die "unable to save file - $!\n";
> -		}
> -	    };
> -	    my $err = $@;
> +	my $dccfg = PVE::Cluster::cfs_read_file('datacenter.cfg');
> +	if ($dccfg->{http_proxy}) {
> +	    $opts->{http_proxy} = $dccfg->{http_proxy};
> +	}
>  
> -	    unlink $tmpdest;
> +	my $worker = sub {
> +	    my $upid = shift;
> +	    PVE::Tools::download_file_from_url($dest, $src, $opts);
> +	};
>  
> -	    if ($err) {
> -		print "\n";
> -		die $err if $err;
> -	    }
> +	my $rpcenv = PVE::RPCEnvironment::get();
> +	my $user = $rpcenv->get_user();

those two where already defined and so you re-defined them here, which, besides doing extra and
needless work, gives one a warning every time this module is used, e.g., on autocomplete of pveam...

[pve-devel] applied: [PATCH v9 manager 2/5] api: nodes: refactor aplinfo to use common download function

[Thomas Lamprecht]

On 16.06.21 11:36, Lorenz Stechauner wrote:
> a common function to download arbitrary files from urls has been
> defined as PVE::Tools::download_file_from_url and is now used.
> 
> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
> ---
>  PVE/API2/Nodes.pm | 93 +++++++++--------------------------------------
>  1 file changed, 17 insertions(+), 76 deletions(-)
> 
>

applied, with the $rpcenv/$user re-definitions dropped in a followup, plus
some more cleanup (please check if it seems OK for you),  thanks!

Re: [pve-devel] [PATCH v9 storage 1/1] status: add download_url method

[Thomas Lamprecht]

On 16.06.21 11:35, Lorenz Stechauner wrote:
> uses common function PVE::Tools::download_file_from_url to download
> iso files.
> 
> Only users with permissions `Sys.Audit` and `Sys.Modify` on `/` are
> permitted to perform this action. This restriction is due to the
> fact, that the download function is able to download files from
> internal networks (which are not visible/accessible from outside).
> Users with these permissions anyway have the means to alter node
> (network) config, so this does not create any further security risk.
> 
> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
> ---
>  PVE/API2/Storage/Status.pm | 128 +++++++++++++++++++++++++++++++++++--
>  PVE/Storage.pm             |  10 +++
>  2 files changed, 133 insertions(+), 5 deletions(-)
> 
> diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
> index 897b4a7..584ba5d 100644
> --- a/PVE/API2/Storage/Status.pm
> +++ b/PVE/API2/Storage/Status.pm
> @@ -412,11 +412,7 @@ __PACKAGE__->register_method ({
>  	my $size = -s $tmpfilename;
>  	die "temporary file '$tmpfilename' does not exist\n" if !defined($size);
>  
> -	my $filename = $param->{filename};
> -
> -	chomp $filename;
> -	$filename =~ s/^.*[\/\\]//;
> -	$filename =~ s/[^-a-zA-Z0-9_.]/_/g;
> +	my $filename = PVE::Storage::normalize_content_filename($param->{filename});

nit/no hard feelings: factoring out normalize_content_filename could have been it's own
patch. IMO for re-using existing code that is nicer, when adding new code they can be
one patch too, if so small like here, as then it can be seen as part of the "single thing"
that patch does.

>  
>  	my $path;
>  
> @@ -497,4 +493,126 @@ __PACKAGE__->register_method ({
>  	return $upid;
>     }});
>  
> +__PACKAGE__->register_method({
> +    name => 'download_url',
> +    path => '{storage}/download-url',

you forgot to add this path to the index endpoint, making it basically a hidden
method..

We probably should assert this in the schema verifier to avoid, as the file-restore
from Stefan also forgot this.

> +    method => 'POST',
> +    description => "Download templates and ISO images by using an URL.",
> +    proxyto => 'node',
> +    permissions => {
> +	check => [ 'and',
> +	    ['perm', '/storage/{storage}', [ 'Datastore.AllocateTemplate' ]],
> +	    ['perm', '/', [ 'Sys.Audit', 'Sys.Modify' ]],
> +	],
> +    },
> +    protected => 1,
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    node => get_standard_option('pve-node'),
> +	    storage => get_standard_option('pve-storage-id'),
> +	    url => {
> +		description => "The URL to download the file from.",
> +		type => 'string',
> +		pattern => 'https?://.*',
> +	    },
> +	    content => {
> +		description => "Content type.",
> +		type => 'string', format => 'pve-storage-content',
> +	    },
> +	    filename => {
> +		description => "The name of the file to create.",

does not mention that it will be normalized, i.e., possibly altered. IMO this is important
as else the user may not find their file again if looking for the original name.

I'd also rather limit this to what we can actually return in the content API call, I mean you
already enforce a implicit limit in the code, so why not encode the union of the allowed
file endings below? ideally using factored out, shared regexes for the ending.


> +		type => 'string',
> +	    },
> +	    checksum => {
> +		description => "The expected checksum of the file.",
> +		type => 'string',
> +		requires => 'checksum-algorithm',
> +		optional => 1,
> +	    },
> +	    'checksum-algorithm' => {
> +		description => "The algorithm to calculate the checksum of the file.",
> +		type => 'string',
> +		enum => ['md5', 'sha1', 'sha224', 'sha256', 'sha384', 'sha512'],
> +		requires => 'checksum',
> +		optional => 1,
> +	    },
> +	    'verify-certificates' => {
> +		description => "If false, no SSL/TLS certificates will be verified.",
> +		type => 'boolean',
> +		optional => 1,
> +		default => 1,
> +	    }
> +	},
> +    },
> +    returns => {
> +	type => "string"
> +    },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $cfg = PVE::Storage::config();
> +
> +	my ($node, $storage) = $param->@{'node', 'storage'};
> +	my $scfg = PVE::Storage::storage_check_enabled($cfg, $storage, $node);
> +
> +	die "can't upload to storage type '$scfg->{type}, not a file based storage!'\n" if !defined($scfg->{path});
> +
> +	my ($content, $url) = $param->@{'content', 'url'};
> +
> +	my $filename = PVE::Storage::normalize_content_filename($param->{filename});
> +	my $path;
> +
> +	# MIME type is checked in front end only
> +	# this check is omitted here intentionally and replaced by file extension check
> +	if ($content eq 'iso') {
> +	    if ($filename !~ m![^/]+$PVE::Storage::iso_extension_re$!) {
> +		raise_param_exc({ filename => "missing '.iso' or '.img' extension" });
> +	    }
> +	    $path = PVE::Storage::get_iso_dir($cfg, $storage);
> +	} elsif ($content eq 'vztmpl') {
> +	    if ($filename !~ m![^/]+\.tar\.[gx]z$!) {

we npwadays also support .zst (zstd), and .vma is missing too, please just avoid reinventing
regexes, especially if not really closly checking what it should cover, rather use an existing
one, which sometimes needs to be factored out first, `path_to_volume_id`

> +		raise_param_exc({ filename => "missing '.tar.gz' or '.tar.xz' extension" });
> +	    }
> +	    $path = PVE::Storage::get_vztmpl_dir($cfg, $storage);
> +	} else {
> +	    raise_param_exc({ content => "upload content type '$content' not allowed" });
> +	}
> +
> +	die "storage '$storage' does not support '$content' content\n" if !$scfg->{content}->{$content};

"does not support" is not necesarrily true, the storage could support it but an admin
could have disabled it, or forgot to enable it, so rather something along the lines of:

"storage '$storage' is not configured for content-type '$content'\n" if !$scfg->{content}->{$content};

> +
> +	PVE::Storage::activate_storage($cfg, $storage);
> +	File::Path::make_path($path);
> +
> +	my $dest = "$path/$filename";
> +
> +	my $opts = {
> +	    hash_required => 0,
> +	    verify_certificates => $param->{'verify-certificates'} // 1,
> +	};
> +
> +	my ($checksum, $checksum_algorithm) = $param->@{'checksum', 'checksum-algorithm'};
> +	if ($checksum) {
> +	    $opts->{"${checksum_algorithm}sum"} = $checksum;
> +	    $opts->{hash_required} = 1;
> +	}
> +
> +	my $dccfg = PVE::Cluster::cfs_read_file('datacenter.cfg');
> +	if ($dccfg->{http_proxy}) {
> +	    $opts->{http_proxy} = $dccfg->{http_proxy};
> +	}
> +
> +	my $worker = sub {
> +	    my $upid = shift;

unused variable.

> +	    PVE::Tools::download_file_from_url($dest, $url, $opts);
> +	};
> +
> +	my $rpcenv = PVE::RPCEnvironment::get();
> +	my $user = $rpcenv->get_user();

nit and not too hard feelings, but basically all existing API methods have above two
lines rather at the start of the  API endpoint's code definition.

> +
> +	my $upid = $rpcenv->fork_worker('download', $filename, $user, $worker);

> +
> +	return $upid;
> +    }});
> +
>  1;
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index aa36bad..f9a3d49 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -1929,4 +1929,14 @@ sub assert_sid_unused {
>      return undef;
>  }
>  

the method below is small, but a small comment about what normalize means here
could still be nice as regex can change their meaning a lot by subtle changes.

> +sub normalize_content_filename {
> +    my ($filename) = @_;
> +
> +    chomp $filename;
> +    $filename =~ s/^.*[\/\\]//;
> +    $filename =~ s/[^-a-zA-Z0-9_.]/_/g;
> +
> +    return $filename;
> +}
> +
>  1;
> 

Re: [pve-devel] applied: [PATCH v9 manager 2/5] api: nodes: refactor aplinfo to use common download function

[Lorenz Stechauner]

On 18.06.21 18:58, Thomas Lamprecht wrote:
> On 16.06.21 11:36, Lorenz Stechauner wrote:
>> a common function to download arbitrary files from urls has been
>> defined as PVE::Tools::download_file_from_url and is now used.
>>
>> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
>> ---
>>   PVE/API2/Nodes.pm | 93 +++++++++--------------------------------------
>>   1 file changed, 17 insertions(+), 76 deletions(-)
>>
>>
> applied, with the $rpcenv/$user re-definitions dropped in a followup, plus
> some more cleanup (please check if it seems OK for you),  thanks!
Had a look at the followups. they look good to me

[pve-devel] applied: [PATCH v9 manager 3/5] ui: add HashAlgorithmSelector

[Thomas Lamprecht]

On 16.06.21 11:36, Lorenz Stechauner wrote:
> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
> ---
>  www/manager6/Makefile                      |  1 +
>  www/manager6/form/HashAlgorithmSelector.js | 16 ++++++++++++++++
>  2 files changed, 17 insertions(+)
>  create mode 100644 www/manager6/form/HashAlgorithmSelector.js
> 
>

applied, thanks!