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

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

[Lorenz Stechauner]

changes to v7:
* adapted to thomas' fix (using download_file_from_url inside a worker)
* updated error message in thomas' fix


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: fix typo
  tools: download_file_from_url: adapt error messages to start at new
    line

 src/PVE/Tools.pm | 10 ++++++----
 1 file changed, 6 insertions(+), 4 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 v8 common 1/2] tools: download_file_from_url: fix typo

[Lorenz Stechauner]

---
 src/PVE/Tools.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
index 8d6734d..c90810c 100644
--- a/src/PVE/Tools.pm
+++ b/src/PVE/Tools.pm
@@ -1908,7 +1908,7 @@ sub download_file_from_url {
 	    if (lc($checksum_got) eq lc($checksum_expected)) {
 		print "OK, checksum verified\n";
 	    } else {
-		die "ERRROR, checksum mismatch: got '$checksum_got' != expect '$checksum_expected'\n";
+		die "ERROR, checksum mismatch: got '$checksum_got' != expect '$checksum_expected'\n";
 	    }
 	}
 
-- 
2.20.1

[pve-devel] [PATCH v8 common 2/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.
---
 src/PVE/Tools.pm | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
index c90810c..c02c259 100644
--- a/src/PVE/Tools.pm
+++ b/src/PVE/Tools.pm
@@ -1869,11 +1869,12 @@ sub download_file_from_url {
 	    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";
+		print " OK, got correct file already, no need to download\n";
 		return;
 	    } else {
+		print "\n";
 		# we could re-download, but may not be safe so just abort for now..
-		die "mismatch (got '$checksum_got' != expect '$checksum_expected'), aborting\n";
+		die "checksum mismatch: got '$checksum_got' != expect '$checksum_expected', aborting\n";
 	    }
 	}
 
@@ -1906,9 +1907,10 @@ sub download_file_from_url {
 	    my $checksum_got = get_file_hash($checksum_algorithm, $tmpdest);
 
 	    if (lc($checksum_got) eq lc($checksum_expected)) {
-		print "OK, checksum verified\n";
+		print " OK, checksum verified\n";
 	    } else {
-		die "ERROR, checksum mismatch: got '$checksum_got' != expect '$checksum_expected'\n";
+		print "\n";
+		die "checksum mismatch: got '$checksum_got' != expect '$checksum_expected'\n";
 	    }
 	}
 
-- 
2.20.1

[pve-devel] [PATCH v8 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 v8 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 v8 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 v8 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 v8 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 v8 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

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

[Thomas Lamprecht]

On 15.06.21 16:08, Lorenz Stechauner wrote:
> the front end expects the error message to be the first part of the
> last line.
> ---
>  src/PVE/Tools.pm | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
> index c90810c..c02c259 100644
> --- a/src/PVE/Tools.pm
> +++ b/src/PVE/Tools.pm
> @@ -1869,11 +1869,12 @@ sub download_file_from_url {
>  	    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";
> +		print " OK, got correct file already, no need to download\n";

this was done by design, as there's a ".." before that

>  		return;
>  	    } else {
> +		print "\n";
>  		# we could re-download, but may not be safe so just abort for now..
> -		die "mismatch (got '$checksum_got' != expect '$checksum_expected'), aborting\n";
> +		die "checksum mismatch: got '$checksum_got' != expect '$checksum_expected', aborting\n";

ok, slightly nicer

>  	    }
>  	}
>  
> @@ -1906,9 +1907,10 @@ sub download_file_from_url {
>  	    my $checksum_got = get_file_hash($checksum_algorithm, $tmpdest);
>  
>  	    if (lc($checksum_got) eq lc($checksum_expected)) {
> -		print "OK, checksum verified\n";
> +		print " OK, checksum verified\n";

as above, the previous print has a ".." at the end so I left any whitespace out.

>  	    } else {
> -		die "ERROR, checksum mismatch: got '$checksum_got' != expect '$checksum_expected'\n";
> +		print "\n";

If, we could just include the "\n" in the start of the die, but we do not really
add such newlines for separating errors in general in task logs, so I'd rather
leave this one out.

> +		die "checksum mismatch: got '$checksum_got' != expect '$checksum_expected'\n";



>  	    }
>  	}
>  
> 

[pve-devel] applied: [PATCH v8 common 1/2] tools: download_file_from_url: fix typo

[Thomas Lamprecht]

On 15.06.21 16:08, Lorenz Stechauner wrote:
> ---
>  src/PVE/Tools.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
>

applied, thanks!

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

[Lorenz Stechauner]

On 15.06.21 16:23, Thomas Lamprecht wrote:
> On 15.06.21 16:08, Lorenz Stechauner wrote:
>> the front end expects the error message to be the first part of the
>> last line.
>> ---
>>   src/PVE/Tools.pm | 10 ++++++----
>>   1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
>> index c90810c..c02c259 100644
>> --- a/src/PVE/Tools.pm
>> +++ b/src/PVE/Tools.pm
>> @@ -1869,11 +1869,12 @@ sub download_file_from_url {
>>   	    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";
>> +		print " OK, got correct file already, no need to download\n";
> this was done by design, as there's a ".." before that
>
>>   		return;
>>   	    } else {
>> +		print "\n";
>>   		# we could re-download, but may not be safe so just abort for now..
>> -		die "mismatch (got '$checksum_got' != expect '$checksum_expected'), aborting\n";
>> +		die "checksum mismatch: got '$checksum_got' != expect '$checksum_expected', aborting\n";
> ok, slightly nicer
>
>>   	    }
>>   	}
>>   
>> @@ -1906,9 +1907,10 @@ sub download_file_from_url {
>>   	    my $checksum_got = get_file_hash($checksum_algorithm, $tmpdest);
>>   
>>   	    if (lc($checksum_got) eq lc($checksum_expected)) {
>> -		print "OK, checksum verified\n";
>> +		print " OK, checksum verified\n";
> as above, the previous print has a ".." at the end so I left any whitespace out.
>
>>   	    } else {
>> -		die "ERROR, checksum mismatch: got '$checksum_got' != expect '$checksum_expected'\n";
>> +		print "\n";
> If, we could just include the "\n" in the start of the die, but we do not really
> add such newlines for separating errors in general in task logs, so I'd rather
> leave this one out.

The new line at the beginning of die does not work either :(

calculating checksum of existing file...TASK ERROR:  checksum mismatch: 
got '73e74eef3d998d522f92295016d92fdc' != expect 'asdf', aborting

The new line was converted to an additional space and the message was 
prefixed with "TASK ERROR: "

The only way I can come up with to make this work for the frontend, is 
to use a print statement before the die...

>
>> +		die "checksum mismatch: got '$checksum_got' != expect '$checksum_expected'\n";
>
>
>>   	    }
>>   	}
>>   
>>

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

[Thomas Lamprecht]

On 16.06.21 10:46, Lorenz Stechauner wrote:
>>> -        die "ERROR, checksum mismatch: got '$checksum_got' != expect '$checksum_expected'\n";
>>> +        print "\n";
>> If, we could just include the "\n" in the start of the die, but we do not really
>> add such newlines for separating errors in general in task logs, so I'd rather
>> leave this one out.
> 
> The new line at the beginning of die does not work either :(
> 
> calculating checksum of existing file...TASK ERROR:  checksum mismatch: got '73e74eef3d998d522f92295016d92fdc' != expect 'asdf', aborting
> 
> The new line was converted to an additional space and the message was prefixed with "TASK ERROR: "
> 
> The only way I can come up with to make this work for the frontend, is to use a print statement before the die...

ah yeah, the whole error is taken prefixed with "TASK ERROR" and added to the log as final line, so whitespace
in the error itself has "no say" regarding this.

OK, let's do the `print "\n"` then but lets also add a comment about the why ^^