[pve-devel] [PATCH-SERIES v2 manager/storage 0/2] fix #1710: add retrieve method for

[pve-devel] [PATCH-SERIES v2 manager/storage 0/2] fix #1710: add retrieve method for

[Lorenz Stechauner]

API call is now two API calls:
* GET /nodes/{node}/urlmeta
* POST /nodes/{node}/storage/{storage}/retrieve

User now needs to have Sys.Audit and Sys.Modify on / to retrieve
data and metadata.

"insecure" option now working in frot end.

Background URL meta check has now buffer of 500ms.

Code cleanup in JS and perl.


Lorenz Stechauner (1):
  fix #1710: add retrieve method for storage

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


Lorenz Stechauner (1):
  fix #1710: add retrieve from url button for storage

 PVE/API2/Nodes.pm                          |  97 +++++++
 www/manager6/Makefile                      |   1 +
 www/manager6/form/HashAlgorithmSelector.js |  16 ++
 www/manager6/storage/Browser.js            |   8 +
 www/manager6/storage/ContentView.js        | 281 +++++++++++++++++++--
 5 files changed, 378 insertions(+), 25 deletions(-)
 create mode 100644 www/manager6/form/HashAlgorithmSelector.js

-- 
2.20.1

[pve-devel] [PATCH v2 storage 1/2] fix #1710: add retrieve method for storage

[Lorenz Stechauner]

Users are now able to download/retrieve any .iso/... file onto their
storages and verify file integrity with checksums.

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

diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
index 897b4a7..a64101f 100644
--- a/PVE/API2/Storage/Status.pm
+++ b/PVE/API2/Storage/Status.pm
@@ -5,6 +5,8 @@ use warnings;
 
 use File::Path;
 use File::Basename;
+use HTTP::Request;
+use LWP::UserAgent;
 use PVE::Tools;
 use PVE::INotify;
 use PVE::Cluster;
@@ -412,11 +414,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 +495,152 @@ __PACKAGE__->register_method ({
 	return $upid;
    }});
 
+__PACKAGE__->register_method({
+    name => 'retrieve',
+    path => '{storage}/retrieve',
+    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 retrieve the file from.",
+		type => 'string',
+	    },
+	    content => {
+		description => "Content type.",
+		type => 'string', format => 'pve-storage-content',
+	    },
+	    filename => {
+		description => "The name of the file to create. Alternatively the file name given by the server will be used.",
+		type => 'string',
+	    },
+	    checksum => {
+		description => "The expected checksum of the file.",
+		type => 'string',
+		requires => 'checksumalg',
+		optional => 1,
+	    },
+	    checksumalg => {
+		description => "The algorithm to claculate the checksum of the file.",
+		type => 'string', enum => ['md5', 'sha1', 'sha224', 'sha256', 'sha384', 'sha512'],
+		requires => 'checksum',
+		optional => 1,
+	    },
+	    insecure => {
+		description => "Allow TLS certificates to be invalid.",
+		type => 'boolean',
+		optional => 1,
+	    }
+	},
+    },
+    returns => { type => "string" },
+    code => sub {
+	my ($param) = @_;
+
+	my $rpcenv = PVE::RPCEnvironment::get();
+
+	my $user = $rpcenv->get_user();
+
+	my $cfg = PVE::Storage::config();
+
+	my $node = $param->{node};
+	my $scfg = PVE::Storage::storage_check_enabled($cfg, $param->{storage}, $node);
+
+	die "can't upload to storage type '$scfg->{type}'"
+	    if !defined($scfg->{path});
+
+	my $content = $param->{content};
+
+	my $url = $param->{url};
+
+	die "invalid https or http url"
+	    if $url !~ qr!^https?://!;
+
+	my $checksum = $param->{checksum};
+	my $hash_alg = $param->{checksumalg};
+
+	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, $param->{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, $param->{storage});
+	} else {
+	    raise_param_exc({ content => "upload content type '$content' not allowed" });
+	}
+
+	die "storage '$param->{storage}' does not support '$content' content"
+	    if !$scfg->{content}->{$content};
+
+	my $dest = "$path/$filename";
+
+	PVE::Storage::activate_storage($cfg, $param->{storage});
+	File::Path::make_path($path);
+
+	# -L follows redirects
+	# -f silent fail on error
+	my @curlcmd = ('curl', '-L', '-o', $dest, '-f');
+	push @curlcmd, '--insecure'
+	    if $param->{insecure};
+
+	my $cmd = [@curlcmd, $url];
+
+	my $cmd_check = [ [ 'echo', $checksum, $dest ], [ "${hash_alg}sum", '-c' ] ];
+	my $cmd_check_flat = [ 'echo', $checksum, $dest, '|', "${hash_alg}sum", '-c'  ];  # only used for logging
+
+	my $worker = sub {
+	    my $upid = shift;
+
+	    print "starting file download from: $url\n";
+	    print "target node: $node\n";
+	    print "target file: $dest\n";
+	    print "command: " . join(' ', @$cmd) . "\n";
+
+	    eval { PVE::Tools::run_command($cmd, errmsg => 'download failed'); };
+	    if (my $err = $@) {
+		unlink $dest;
+		die $err;
+	    }
+	    print "finished file download successfully\n";
+
+	    if ($checksum) {
+		print "validating checksum...\n";
+		print "expected $hash_alg checksum is: $checksum\n";
+		print cmd_check print "checksum validation command: " . join(' ', @$cmd_check_flat) . "\n";
+
+		eval { PVE::Tools::run_command($cmd_check, errmsg => 'checksum mismatch'); };
+		if (my $err = $@) {
+		    unlink $dest;
+		    die $err;
+		}
+		print "validated checksum successfully\n";
+	    }
+	};
+
+	my $upid = $rpcenv->fork_worker('imgdownload', undef, $user, $worker);
+
+	return $upid;
+    }});
+
 1;
diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index 122c3e9..d57fd43 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -1931,4 +1931,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 v2 manager 2/2] fix #1710: add retrieve from url button for storage

[Lorenz Stechauner]

Add PVE.storage.Retrieve window and PVE.form.hashAlgorithmSelector.
Users are now able to download/retrieve any .iso/... file onto their
storages and verify file integrity with checksums.

Add new method: GET /nodes/{node}/urlmeta - returns url metadata

Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
---
 PVE/API2/Nodes.pm                          |  97 +++++++
 www/manager6/Makefile                      |   1 +
 www/manager6/form/HashAlgorithmSelector.js |  16 ++
 www/manager6/storage/Browser.js            |   8 +
 www/manager6/storage/ContentView.js        | 281 +++++++++++++++++++--
 5 files changed, 378 insertions(+), 25 deletions(-)
 create mode 100644 www/manager6/form/HashAlgorithmSelector.js

diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
index ba6621c6..c2407d99 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;
@@ -254,6 +255,7 @@ __PACKAGE__->register_method ({
 	    { name => 'tasks' },
 	    { name => 'termproxy' },
 	    { name => 'time' },
+	    { name => 'urlmeta' },
 	    { name => 'version' },
 	    { name => 'vncshell' },
 	    { name => 'vzdump' },
@@ -1596,6 +1598,101 @@ __PACKAGE__->register_method({
 	return $rpcenv->fork_worker('download', undef, $user, $worker);
     }});
 
+
+__PACKAGE__->register_method({
+    name => 'urlmeta',
+    path => 'urlmeta',
+    method => 'GET',
+    description => "Download templates and ISO images by using an URL.",
+    proxyto => 'node',
+    permissions => {
+	check => ['perm', '/', [ 'Sys.Audit', 'Sys.Modify' ]],
+    },
+    protected => 1,
+    parameters => {
+	additionalProperties => 0,
+	properties => {
+	    node => get_standard_option('pve-node'),
+	    url => {
+		description => "The URL to retrieve the file from.",
+		type => 'string',
+	    },
+	    insecure => {
+		description => "Allow TLS certificates to be invalid.",
+		type => 'boolean',
+		optional => 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};
+
+	die "invalid https or http url"
+	    if $url !~ qr!^https?://!;
+
+	my $ua = LWP::UserAgent->new();
+	$ua->ssl_opts(
+	    verify_hostname => 0,
+	    SSL_verify_mode => IO::Socket::SSL::SSL_VERIFY_NONE,
+	) if $param->{insecure};
+
+	my $req = HTTP::Request->new(HEAD => $url);
+	my $res = $ua->request($req);
+
+	die "invalid server response: '" . $res->status_line() . "'"
+	    if ($res->code() != 200);
+
+	my $size = $res->header("Content-Length");
+	my $dispo = $res->header("Content-Disposition");
+	my $type = $res->header("Content-Type");
+
+	my $filename;
+
+	if ($dispo && $dispo =~ 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',
diff --git a/www/manager6/Makefile b/www/manager6/Makefile
index afed3283..8e6557d8 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'],
+    ],
+});
diff --git a/www/manager6/storage/Browser.js b/www/manager6/storage/Browser.js
index 5fee94c7..da3e66c8 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 enableRetrieve = !!(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,
+		    enableRetrieveButton: enableUpload && enableRetrieve,
 		    useUploadButton: true,
 		});
 	    }
@@ -101,6 +106,9 @@ Ext.define('PVE.storage.Browser', {
 		    iconCls: 'fa fa-file-o lxc',
 		    itemId: 'contentVztmpl',
 		    pluginType: plugin,
+		    enableUploadButton: enableUpload,
+		    enableRetrieveButton: enableUpload && enableRetrieve,
+		    useUploadButton: true,
 		});
 	    }
 	    if (contents.includes('snippets')) {
diff --git a/www/manager6/storage/ContentView.js b/www/manager6/storage/ContentView.js
index dd6df4b1..d01526e1 100644
--- a/www/manager6/storage/ContentView.js
+++ b/www/manager6/storage/ContentView.js
@@ -191,6 +191,213 @@ Ext.define('PVE.storage.Upload', {
     },
 });
 
+Ext.define('PVE.storage.Retrieve', {
+    extend: 'Proxmox.window.Edit',
+    alias: 'widget.pveStorageRetrieve',
+
+    isCreate: true,
+
+    showTaskViewer: true,
+
+    title: gettext('Retrieve from URL'),
+    submitText: gettext('Download'),
+
+    id: 'retrieve',
+
+    controller: {
+	xclass: 'Ext.app.ViewController',
+
+	urlChange: function(field) {
+	    let me = Ext.getCmp('retrieve');
+	    field = me.down('[name=url]');
+	    field.setValidation("Waiting for response...");
+	    field.validate();
+	    me.setValues({size: ""});
+	    Proxmox.Utils.API2Request({
+		url: `/nodes/${me.nodename}/urlmeta`,
+		method: 'GET',
+		params: {
+		    url: field.getValue(),
+		    insecure: me.getValues()['insecure'],
+		},
+		failure: function(res, opt) {
+		    field.setValidation(res.result.message);
+		    field.validate();
+		    me.setValues({
+			size: "",
+			mimetype: "",
+		    });
+		},
+		success: function(res, opt) {
+		    field.setValidation();
+		    field.validate();
+
+		    let data = res.result.data;
+		    me.setValues({
+			filename: data.filename || "",
+			size: data.size && Proxmox.Utils.format_size(data.size) || "",
+			mimetype: data.mimetype || "",
+		    });
+		},
+	    });
+	},
+
+	hashChange: function(field) {
+	    let cecksum = Ext.getCmp('retrieveChecksum');
+	    if (field.getValue() === '__default__') {
+		cecksum.setDisabled(true);
+		cecksum.setValue("");
+		cecksum.allowBlank = true;
+	    } else {
+		cecksum.setDisabled(false);
+		cecksum.allowBlank = false;
+	    }
+	},
+
+	typeChange: function(field) {
+	    let me = Ext.getCmp('retrieve');
+	    let content = me.getValues()['content'];
+	    let type = field.getValue();
+
+	    const types = {
+		iso: [
+		    'application/octet-stream',
+		    'application/x-iso9660-image',
+		    'application/x-ima',
+		],
+		vztmpl: [
+		    'application/octet-stream',
+		    'application/gzip',
+		    'application/tar',
+		    'application/tar+gzip',
+		    'application/x-gzip',
+		    'application/x-gtar',
+		    'application/x-tgz',
+		    'application/x-tar',
+		],
+	    };
+
+	    if (type === "" || (types[content] && types[content].includes(type))) {
+		field.setValidation();
+		field.setDisabled(true);
+	    } else {
+		field.setDisabled(false);
+		field.setValidation("Invalid type");
+	    }
+	    field.validate();
+	},
+    },
+
+    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,
+		},
+	    ],
+	    column2: [
+		{
+		    xtype: 'textfield',
+		    name: 'size',
+		    disabled: true,
+		    fieldLabel: gettext('File size'),
+		    emptyText: gettext('unknown'),
+		},
+	    ],
+	    advancedColumn1: [
+		{
+		    xtype: 'textfield',
+		    name: 'checksum',
+		    fieldLabel: gettext('Checksum'),
+		    allowBlank: true,
+		    disabled: true,
+		    emptyText: gettext('none'),
+		    id: 'retrieveChecksum',
+		},
+		{
+		    xtype: 'pveHashAlgorithmSelector',
+		    name: 'checksumalg',
+		    fieldLabel: gettext('Hash algorithm'),
+		    allowBlank: true,
+		    hasNoneOption: true,
+		    value: '__default__',
+		    listeners: {
+			change: 'hashChange',
+		    },
+		},
+	    ],
+	    advancedColumn2: [
+		{
+		    xtype: 'textfield',
+		    fieldLabel: gettext('MIME type'),
+		    name: 'mimetype',
+		    disabled: true,
+		    editable: false,
+		    emptyText: gettext('unknown'),
+		    listeners: {
+			change: 'typeChange',
+		    },
+		},
+		{
+		    xtype: 'proxmoxcheckbox',
+		    name: 'insecure',
+		    fieldLabel: gettext('Trust invalid certificates'),
+		    uncheckedValue: 0,
+		    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.url = `/nodes/${me.nodename}/storage/${me.storage}/retrieve`;
+	me.method = 'POST';
+
+	let contentTypeSel = me.items[0].column1[0];
+	contentTypeSel.cts = me.contents;
+	contentTypeSel.value = me.contents[0] || '';
+
+        me.callParent();
+    },
+});
+
 Ext.define('PVE.storage.ContentView', {
     extend: 'Ext.grid.GridPanel',
 
@@ -249,36 +456,60 @@ 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.push(
+		{
+		    xtype: 'button',
+		    text: gettext('Upload'),
+		    disabled: !me.enableUploadButton,
+		    handler: function() {
+			Ext.create('PVE.storage.Upload', {
+			    nodename: nodename,
+			    storage: storage,
+			    contents: [content],
+			    autoShow: true,
+			    listeners:{
+				destroy: () => reload(),
+			    }
+			});
+		    },
+		},
+		{
+		    xtype: 'button',
+		    text: gettext('Retrieve from URL'),
+		    disabled: !me.enableRetrieveButton,
+		    handler: function() {
+			Ext.create('PVE.storage.Retrieve', {
+			    nodename: nodename,
+			    storage: storage,
+			    contents: [content],
+			    autoShow: true,
+			    listeners: {
+				destroy: () => reload(),
+			    },
+			});
+		    },
+		},
+		'-',
+	    );
 	}
-	if (!me.useCustomRemoveButton) {
-	    me.tbar.push(removeButton);
+	if (me.useCustomRemoveButton) {
+	    // custom remove button was inserted as first element
+	    // -> place it at the end of tbar
+	    me.tbar.push(me.tbar.shift());
+	} else {
+	    me.tbar.push({
+		xtype: 'proxmoxStdRemoveButton',
+		selModel: sm,
+		delay: 5,
+		callback: function() {
+		    reload();
+		},
+		baseurl: baseurl + '/',
+	    });
 	}
 	me.tbar.push(
 	    '->',
-- 
2.20.1

[pve-devel] [PATCH-SERIES v3 manager/storage 0/2] fix #1710: add retrieve from url button for storage

[Lorenz Stechauner]

changes to v2:

added storageid as task parameter.
gettext for imgdownload.

pve-storage:
Lorenz Stechauner (1):
  fix #1710: add retrieve method for storage

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

pve-manager:
Lorenz Stechauner (1):
  fix #1710: add retrieve from url button for storage

 PVE/API2/Nodes.pm                          |  97 +++++++
 www/manager6/Makefile                      |   1 +
 www/manager6/Utils.js                      |   1 +
 www/manager6/form/HashAlgorithmSelector.js |  16 ++
 www/manager6/storage/Browser.js            |   8 +
 www/manager6/storage/ContentView.js        | 281 +++++++++++++++++++--
 6 files changed, 379 insertions(+), 25 deletions(-)
 create mode 100644 www/manager6/form/HashAlgorithmSelector.js

-- 
2.20.1

[pve-devel] [PATCH v3 storage 1/2] fix #1710: add retrieve method for storage

[Lorenz Stechauner]

Users are now able to download/retrieve any .iso/... file onto their
storages and verify file integrity with checksums.

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

diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
index 897b4a7..8388714 100644
--- a/PVE/API2/Storage/Status.pm
+++ b/PVE/API2/Storage/Status.pm
@@ -5,6 +5,8 @@ use warnings;
 
 use File::Path;
 use File::Basename;
+use HTTP::Request;
+use LWP::UserAgent;
 use PVE::Tools;
 use PVE::INotify;
 use PVE::Cluster;
@@ -412,11 +414,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 +495,154 @@ __PACKAGE__->register_method ({
 	return $upid;
    }});
 
+__PACKAGE__->register_method({
+    name => 'retrieve',
+    path => '{storage}/retrieve',
+    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 retrieve the file from.",
+		type => 'string',
+	    },
+	    content => {
+		description => "Content type.",
+		type => 'string', format => 'pve-storage-content',
+	    },
+	    filename => {
+		description => "The name of the file to create. Alternatively the file name given by the server will be used.",
+		type => 'string',
+	    },
+	    checksum => {
+		description => "The expected checksum of the file.",
+		type => 'string',
+		requires => 'checksumalg',
+		optional => 1,
+	    },
+	    checksumalg => {
+		description => "The algorithm to claculate the checksum of the file.",
+		type => 'string', enum => ['md5', 'sha1', 'sha224', 'sha256', 'sha384', 'sha512'],
+		requires => 'checksum',
+		optional => 1,
+	    },
+	    insecure => {
+		description => "Allow TLS certificates to be invalid.",
+		type => 'boolean',
+		optional => 1,
+	    }
+	},
+    },
+    returns => { type => "string" },
+    code => sub {
+	my ($param) = @_;
+
+	my $rpcenv = PVE::RPCEnvironment::get();
+
+	my $user = $rpcenv->get_user();
+
+	my $cfg = PVE::Storage::config();
+
+	my $node = $param->{node};
+	my $storage = $param->{storage};
+	my $scfg = PVE::Storage::storage_check_enabled($cfg, $storage, $node);
+
+	die "can't upload to storage type '$scfg->{type}'"
+	    if !defined($scfg->{path});
+
+	my $content = $param->{content};
+
+	my $url = $param->{url};
+
+	die "invalid https or http url"
+	    if $url !~ qr!^https?://!;
+
+	my $checksum = $param->{checksum};
+	my $hash_alg = $param->{checksumalg};
+
+	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"
+	    if !$scfg->{content}->{$content};
+
+	my $dest = "$path/$filename";
+
+	PVE::Storage::activate_storage($cfg, $storage);
+	File::Path::make_path($path);
+
+	# -L follows redirects
+	# -f silent fail on error
+	my @curlcmd = ('curl', '-L', '-o', $dest, '-f');
+	push @curlcmd, '--insecure'
+	    if $param->{insecure};
+
+	my $cmd = [@curlcmd, $url];
+
+	my $cmd_check = [ [ 'echo', $checksum, $dest ], [ "${hash_alg}sum", '-c' ] ];
+	my $cmd_check_flat = [ 'echo', $checksum, $dest, '|', "${hash_alg}sum", '-c'  ];  # only used for logging
+
+	my $worker = sub {
+	    my $upid = shift;
+
+	    print "starting file download from: $url\n";
+	    print "target node: $node\n";
+	    print "target storage: $storage\n";
+	    print "target file: $dest\n";
+	    print "command: " . join(' ', @$cmd) . "\n";
+
+	    eval { PVE::Tools::run_command($cmd, errmsg => 'download failed'); };
+	    if (my $err = $@) {
+		unlink $dest;
+		die $err;
+	    }
+	    print "finished file download successfully\n";
+
+	    if ($checksum) {
+		print "validating checksum...\n";
+		print "expected $hash_alg checksum is: $checksum\n";
+		print cmd_check print "checksum validation command: " . join(' ', @$cmd_check_flat) . "\n";
+
+		eval { PVE::Tools::run_command($cmd_check, errmsg => 'checksum mismatch'); };
+		if (my $err = $@) {
+		    unlink $dest;
+		    die $err;
+		}
+		print "validated checksum successfully\n";
+	    }
+	};
+
+	my $upid = $rpcenv->fork_worker('imgdownload', $storage, $user, $worker);
+
+	return $upid;
+    }});
+
 1;
diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index 122c3e9..d57fd43 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -1931,4 +1931,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 v3 manager 2/2] fix #1710: add retrieve from url button for storage

[Lorenz Stechauner]

Add PVE.storage.Retrieve window and PVE.form.hashAlgorithmSelector.
Users are now able to download/retrieve any .iso/... file onto their
storages and verify file integrity with checksums.

Add new method: GET /nodes/{node}/urlmeta - returns url metadata

Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
---
 PVE/API2/Nodes.pm                          |  97 +++++++
 www/manager6/Makefile                      |   1 +
 www/manager6/Utils.js                      |   1 +
 www/manager6/form/HashAlgorithmSelector.js |  16 ++
 www/manager6/storage/Browser.js            |   8 +
 www/manager6/storage/ContentView.js        | 281 +++++++++++++++++++--
 6 files changed, 379 insertions(+), 25 deletions(-)
 create mode 100644 www/manager6/form/HashAlgorithmSelector.js

diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
index ba6621c6..c2407d99 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;
@@ -254,6 +255,7 @@ __PACKAGE__->register_method ({
 	    { name => 'tasks' },
 	    { name => 'termproxy' },
 	    { name => 'time' },
+	    { name => 'urlmeta' },
 	    { name => 'version' },
 	    { name => 'vncshell' },
 	    { name => 'vzdump' },
@@ -1596,6 +1598,101 @@ __PACKAGE__->register_method({
 	return $rpcenv->fork_worker('download', undef, $user, $worker);
     }});
 
+
+__PACKAGE__->register_method({
+    name => 'urlmeta',
+    path => 'urlmeta',
+    method => 'GET',
+    description => "Download templates and ISO images by using an URL.",
+    proxyto => 'node',
+    permissions => {
+	check => ['perm', '/', [ 'Sys.Audit', 'Sys.Modify' ]],
+    },
+    protected => 1,
+    parameters => {
+	additionalProperties => 0,
+	properties => {
+	    node => get_standard_option('pve-node'),
+	    url => {
+		description => "The URL to retrieve the file from.",
+		type => 'string',
+	    },
+	    insecure => {
+		description => "Allow TLS certificates to be invalid.",
+		type => 'boolean',
+		optional => 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};
+
+	die "invalid https or http url"
+	    if $url !~ qr!^https?://!;
+
+	my $ua = LWP::UserAgent->new();
+	$ua->ssl_opts(
+	    verify_hostname => 0,
+	    SSL_verify_mode => IO::Socket::SSL::SSL_VERIFY_NONE,
+	) if $param->{insecure};
+
+	my $req = HTTP::Request->new(HEAD => $url);
+	my $res = $ua->request($req);
+
+	die "invalid server response: '" . $res->status_line() . "'"
+	    if ($res->code() != 200);
+
+	my $size = $res->header("Content-Length");
+	my $dispo = $res->header("Content-Disposition");
+	my $type = $res->header("Content-Type");
+
+	my $filename;
+
+	if ($dispo && $dispo =~ 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',
diff --git a/www/manager6/Makefile b/www/manager6/Makefile
index afed3283..8e6557d8 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/Utils.js b/www/manager6/Utils.js
index 581d2040..b7479e7a 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -1826,6 +1826,7 @@ Ext.define('PVE.Utils', {
 	    hastop: ['HA', gettext('Stop')],
 	    imgcopy: ['', gettext('Copy data')],
 	    imgdel: ['', gettext('Erase data')],
+	    imgdownload: [gettext('Storage'), gettext('Download data')],
 	    lvmcreate: [gettext('LVM Storage'), gettext('Create')],
 	    lvmthincreate: [gettext('LVM-Thin Storage'), gettext('Create')],
 	    migrateall: ['', gettext('Migrate all VMs and Containers')],
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'],
+    ],
+});
diff --git a/www/manager6/storage/Browser.js b/www/manager6/storage/Browser.js
index 5fee94c7..da3e66c8 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 enableRetrieve = !!(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,
+		    enableRetrieveButton: enableUpload && enableRetrieve,
 		    useUploadButton: true,
 		});
 	    }
@@ -101,6 +106,9 @@ Ext.define('PVE.storage.Browser', {
 		    iconCls: 'fa fa-file-o lxc',
 		    itemId: 'contentVztmpl',
 		    pluginType: plugin,
+		    enableUploadButton: enableUpload,
+		    enableRetrieveButton: enableUpload && enableRetrieve,
+		    useUploadButton: true,
 		});
 	    }
 	    if (contents.includes('snippets')) {
diff --git a/www/manager6/storage/ContentView.js b/www/manager6/storage/ContentView.js
index dd6df4b1..d01526e1 100644
--- a/www/manager6/storage/ContentView.js
+++ b/www/manager6/storage/ContentView.js
@@ -191,6 +191,213 @@ Ext.define('PVE.storage.Upload', {
     },
 });
 
+Ext.define('PVE.storage.Retrieve', {
+    extend: 'Proxmox.window.Edit',
+    alias: 'widget.pveStorageRetrieve',
+
+    isCreate: true,
+
+    showTaskViewer: true,
+
+    title: gettext('Retrieve from URL'),
+    submitText: gettext('Download'),
+
+    id: 'retrieve',
+
+    controller: {
+	xclass: 'Ext.app.ViewController',
+
+	urlChange: function(field) {
+	    let me = Ext.getCmp('retrieve');
+	    field = me.down('[name=url]');
+	    field.setValidation("Waiting for response...");
+	    field.validate();
+	    me.setValues({size: ""});
+	    Proxmox.Utils.API2Request({
+		url: `/nodes/${me.nodename}/urlmeta`,
+		method: 'GET',
+		params: {
+		    url: field.getValue(),
+		    insecure: me.getValues()['insecure'],
+		},
+		failure: function(res, opt) {
+		    field.setValidation(res.result.message);
+		    field.validate();
+		    me.setValues({
+			size: "",
+			mimetype: "",
+		    });
+		},
+		success: function(res, opt) {
+		    field.setValidation();
+		    field.validate();
+
+		    let data = res.result.data;
+		    me.setValues({
+			filename: data.filename || "",
+			size: data.size && Proxmox.Utils.format_size(data.size) || "",
+			mimetype: data.mimetype || "",
+		    });
+		},
+	    });
+	},
+
+	hashChange: function(field) {
+	    let cecksum = Ext.getCmp('retrieveChecksum');
+	    if (field.getValue() === '__default__') {
+		cecksum.setDisabled(true);
+		cecksum.setValue("");
+		cecksum.allowBlank = true;
+	    } else {
+		cecksum.setDisabled(false);
+		cecksum.allowBlank = false;
+	    }
+	},
+
+	typeChange: function(field) {
+	    let me = Ext.getCmp('retrieve');
+	    let content = me.getValues()['content'];
+	    let type = field.getValue();
+
+	    const types = {
+		iso: [
+		    'application/octet-stream',
+		    'application/x-iso9660-image',
+		    'application/x-ima',
+		],
+		vztmpl: [
+		    'application/octet-stream',
+		    'application/gzip',
+		    'application/tar',
+		    'application/tar+gzip',
+		    'application/x-gzip',
+		    'application/x-gtar',
+		    'application/x-tgz',
+		    'application/x-tar',
+		],
+	    };
+
+	    if (type === "" || (types[content] && types[content].includes(type))) {
+		field.setValidation();
+		field.setDisabled(true);
+	    } else {
+		field.setDisabled(false);
+		field.setValidation("Invalid type");
+	    }
+	    field.validate();
+	},
+    },
+
+    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,
+		},
+	    ],
+	    column2: [
+		{
+		    xtype: 'textfield',
+		    name: 'size',
+		    disabled: true,
+		    fieldLabel: gettext('File size'),
+		    emptyText: gettext('unknown'),
+		},
+	    ],
+	    advancedColumn1: [
+		{
+		    xtype: 'textfield',
+		    name: 'checksum',
+		    fieldLabel: gettext('Checksum'),
+		    allowBlank: true,
+		    disabled: true,
+		    emptyText: gettext('none'),
+		    id: 'retrieveChecksum',
+		},
+		{
+		    xtype: 'pveHashAlgorithmSelector',
+		    name: 'checksumalg',
+		    fieldLabel: gettext('Hash algorithm'),
+		    allowBlank: true,
+		    hasNoneOption: true,
+		    value: '__default__',
+		    listeners: {
+			change: 'hashChange',
+		    },
+		},
+	    ],
+	    advancedColumn2: [
+		{
+		    xtype: 'textfield',
+		    fieldLabel: gettext('MIME type'),
+		    name: 'mimetype',
+		    disabled: true,
+		    editable: false,
+		    emptyText: gettext('unknown'),
+		    listeners: {
+			change: 'typeChange',
+		    },
+		},
+		{
+		    xtype: 'proxmoxcheckbox',
+		    name: 'insecure',
+		    fieldLabel: gettext('Trust invalid certificates'),
+		    uncheckedValue: 0,
+		    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.url = `/nodes/${me.nodename}/storage/${me.storage}/retrieve`;
+	me.method = 'POST';
+
+	let contentTypeSel = me.items[0].column1[0];
+	contentTypeSel.cts = me.contents;
+	contentTypeSel.value = me.contents[0] || '';
+
+        me.callParent();
+    },
+});
+
 Ext.define('PVE.storage.ContentView', {
     extend: 'Ext.grid.GridPanel',
 
@@ -249,36 +456,60 @@ 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.push(
+		{
+		    xtype: 'button',
+		    text: gettext('Upload'),
+		    disabled: !me.enableUploadButton,
+		    handler: function() {
+			Ext.create('PVE.storage.Upload', {
+			    nodename: nodename,
+			    storage: storage,
+			    contents: [content],
+			    autoShow: true,
+			    listeners:{
+				destroy: () => reload(),
+			    }
+			});
+		    },
+		},
+		{
+		    xtype: 'button',
+		    text: gettext('Retrieve from URL'),
+		    disabled: !me.enableRetrieveButton,
+		    handler: function() {
+			Ext.create('PVE.storage.Retrieve', {
+			    nodename: nodename,
+			    storage: storage,
+			    contents: [content],
+			    autoShow: true,
+			    listeners: {
+				destroy: () => reload(),
+			    },
+			});
+		    },
+		},
+		'-',
+	    );
 	}
-	if (!me.useCustomRemoveButton) {
-	    me.tbar.push(removeButton);
+	if (me.useCustomRemoveButton) {
+	    // custom remove button was inserted as first element
+	    // -> place it at the end of tbar
+	    me.tbar.push(me.tbar.shift());
+	} else {
+	    me.tbar.push({
+		xtype: 'proxmoxStdRemoveButton',
+		selModel: sm,
+		delay: 5,
+		callback: function() {
+		    reload();
+		},
+		baseurl: baseurl + '/',
+	    });
 	}
 	me.tbar.push(
 	    '->',
-- 
2.20.1

Re: [pve-devel] [PATCH v3 storage 1/2] fix #1710: add retrieve method for storage

[Thomas Lamprecht]

On 04.05.21 10:56, Lorenz Stechauner wrote:
> Users are now able to download/retrieve any .iso/... file onto their
> storages and verify file integrity with checksums.
> 
> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
> ---
>  PVE/API2/Storage/Status.pm | 158 +++++++++++++++++++++++++++++++++++--
>  PVE/Storage.pm             |  10 +++
>  2 files changed, 163 insertions(+), 5 deletions(-)
> 
> diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
> index 897b4a7..8388714 100644
> --- a/PVE/API2/Storage/Status.pm
> +++ b/PVE/API2/Storage/Status.pm
> @@ -5,6 +5,8 @@ use warnings;
>  
>  use File::Path;
>  use File::Basename;
> +use HTTP::Request;
> +use LWP::UserAgent;
>  use PVE::Tools;
>  use PVE::INotify;
>  use PVE::Cluster;
> @@ -412,11 +414,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 +495,154 @@ __PACKAGE__->register_method ({
>  	return $upid;
>     }});
>  
> +__PACKAGE__->register_method({
> +    name => 'retrieve',
> +    path => '{storage}/retrieve',

over general path/method name, I'd either use "retrieve-url" or use "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 retrieve the file from.",
> +		type => 'string',
> +	    },
> +	    content => {
> +		description => "Content type.",

auto-detect could be done in general? At least for a few whitelisted types
(e.g., .iso), but no hard feelings here, we can always make this more flexible
in the future

> +		type => 'string', format => 'pve-storage-content',
> +	    },
> +	    filename => {
> +		description => "The name of the file to create. Alternatively the file name given by the server will be used.",

You mention "alternatively", but this parameter is not optional?

> +		type => 'string',
> +	    },
> +	    checksum => {
> +		description => "The expected checksum of the file.",
> +		type => 'string',
> +		requires => 'checksumalg',
> +		optional => 1,
> +	    },
> +	    checksumalg => {

weird name? I really do not get the need to abbreviate everything in some custom way.
APIs parameter should speak for them self, at least somewhat, just use
"checksum-algorithm"

> +		description => "The algorithm to claculate the checksum of the file.",

typo s/claculate/calculate/

> +		type => 'string', enum => ['md5', 'sha1', 'sha224', 'sha256', 'sha384', 'sha512'],

The enum goes in its own line

> +		requires => 'checksum',
> +		optional => 1,
> +	    },
> +	    insecure => {
> +		description => "Allow TLS certificates to be invalid.",

Rather use "verify-certificates"  and default to 1 (true).

> +		type => 'boolean',
> +		optional => 1,
> +	    }
> +	},
> +    },
> +    returns => { type => "string" },

use
returns => {
   type => "string",
},


> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $rpcenv = PVE::RPCEnvironment::get();
> +

unnecessary extra newline

> +	my $user = $rpcenv->get_user();
> +
> +	my $cfg = PVE::Storage::config();
> +
> +	my $node = $param->{node};
> +	my $storage = $param->{storage};
> +	my $scfg = PVE::Storage::storage_check_enabled($cfg, $storage, $node);
> +
> +	die "can't upload to storage type '$scfg->{type}'"

1. Without a \n at then perl will add the line + module to that error message,
not nice for stuff which is not internal (i.e., assert-like).

2. normally its considered good to add some reason/explanation to an error, so
that user can know what needs to actually change to make it work.

Maybe:
"cannot upload to storage '$storage' (type '$scfg->{type}'), not a file based storage!\n"

> +	    if !defined($scfg->{path});
> +
> +	my $content = $param->{content};
> +
> +	my $url = $param->{url};

optimizer nit, above three lines could be:

my ($content, $url) = $param->@{'content', 'url'};

> +
> +	die "invalid https or http url"
> +	    if $url !~ qr!^https?://!;

we normally place short post-if's into the same line as the statement it guards

> +
> +	my $checksum = $param->{checksum};
> +	my $hash_alg = $param->{checksumalg};

consistency please, if the param is named checksum*something then the variable should be
named similar (hash vs. checksum here).

Also, in general I'd like to see some locality between variable declaration and use

> +
> +	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"
> +	    if !$scfg->{content}->{$content};
> +
> +	my $dest = "$path/$filename";
> +
> +	PVE::Storage::activate_storage($cfg, $storage);
> +	File::Path::make_path($path);
> +
> +	# -L follows redirects
> +	# -f silent fail on error

why silently failing? wouldn't the error be interesting, should not have to big implications
now that this call requires elevated privs, or?

> +	my @curlcmd = ('curl', '-L', '-o', $dest, '-f');

we use wget with --progress=dot:mega for the appliances, so why curl here?

In general I ask myself if there would be some common helper possible for both,
appliances and this here, to avoid code duplication, did you looked into that.

> +	push @curlcmd, '--insecure'
> +	    if $param->{insecure};
> +
> +	my $cmd = [@curlcmd, $url];

$cmd is overly general

> +
> +	my $cmd_check = [ [ 'echo', $checksum, $dest ], [ "${hash_alg}sum", '-c' ] ];
> +	my $cmd_check_flat = [ 'echo', $checksum, $dest, '|', "${hash_alg}sum", '-c'  ];  # only used for logging

yeah, no, we don't do that, the log command is at risk to get outdated without noticing.
Either auto generated it from the $cmd_check or omit in in general (i.e., by just logging the
checksum algorithm used.

Also why do we rely on the external tools if we have usage of the perl
Digest::{MD5,SHA256,..} quite some times already?

> +
> +	my $worker = sub {
> +	    my $upid = shift;
> +
> +	    print "starting file download from: $url\n";
> +	    print "target node: $node\n";

omit above node info, it's already in the task UPID

> +	    print "target storage: $storage\n";

isn't that to in the task UPID?

> +	    print "target file: $dest\n";

> +	    print "command: " . join(' ', @$cmd) . "\n";

if use PVE::Tools shellquote or the like, but actually I'd omit that...

The five prints could be

print "download '$url' to $storage as $filename\n";

further advantage, that does not leaks that much info, e.g., the full path
may not be known even for those with sys.audit.

> +
> +	    eval { PVE::Tools::run_command($cmd, errmsg => 'download failed'); };
> +	    if (my $err = $@) {
> +		unlink $dest;

check the unlink, for a single file you can use a simple or:

unlink $dest or warn "could not cleanup '$dest' - $!\n"

> +		die $err;
> +	    }
> +	    print "finished file download successfully\n";
> +
> +	    if ($checksum) {
> +		print "validating checksum...\n";
> +		print "expected $hash_alg checksum is: $checksum\n";

quite chatty, above could go into one line. Information is good to have in a task log,
but it should be concise to avoid drowning the relevant in noise.

> +		print cmd_check print "checksum validation command: " . join(' ', @$cmd_check_flat) . "\n";
> +
> +		eval { PVE::Tools::run_command($cmd_check, errmsg => 'checksum mismatch'); };

this all is racy, i.e., if something aborts/kills the task or above hangs we have a time
window where the downloaded file already shows up as valid (but not yet checked!) one
at the storage. Use a temporary file ending (we have some use of those in various places,
basically something like .tmp.$$) and do a rename only at the end.


> +		if (my $err = $@) {
> +		    unlink $dest;
> +		    die $err;
> +		}
> +		print "validated checksum successfully\n";
> +	    }
> +	};
> +
> +	my $upid = $rpcenv->fork_worker('imgdownload', $storage, $user, $worker);
> +
> +	return $upid;
> +    }});
> +
>  1;
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index 122c3e9..d57fd43 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -1931,4 +1931,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;
> 

Re: [pve-devel] [PATCH v3 manager 2/2] fix #1710: add retrieve from url button for storage

[Thomas Lamprecht]

On 04.05.21 10:57, Lorenz Stechauner wrote:
> Add PVE.storage.Retrieve window and PVE.form.hashAlgorithmSelector.
> Users are now able to download/retrieve any .iso/... file onto their
> storages and verify file integrity with checksums.
> 
> Add new method: GET /nodes/{node}/urlmeta - returns url metadata

I'd separate backend and gui into at least two patches, e.g., the hash algorithm
widget has nothing directly to do with querying metadata from an URL.

Also, you do not handle the datacenter.cfg http_proxy setting, neither here
nor in the storage patch.

> 
> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
> ---
>  PVE/API2/Nodes.pm                          |  97 +++++++
>  www/manager6/Makefile                      |   1 +
>  www/manager6/Utils.js                      |   1 +
>  www/manager6/form/HashAlgorithmSelector.js |  16 ++
>  www/manager6/storage/Browser.js            |   8 +
>  www/manager6/storage/ContentView.js        | 281 +++++++++++++++++++--
>  6 files changed, 379 insertions(+), 25 deletions(-)
>  create mode 100644 www/manager6/form/HashAlgorithmSelector.js
> 
> diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
> index ba6621c6..c2407d99 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;
> @@ -254,6 +255,7 @@ __PACKAGE__->register_method ({
>  	    { name => 'tasks' },
>  	    { name => 'termproxy' },
>  	    { name => 'time' },
> +	    { name => 'urlmeta' },
>  	    { name => 'version' },
>  	    { name => 'vncshell' },
>  	    { name => 'vzdump' },
> @@ -1596,6 +1598,101 @@ __PACKAGE__->register_method({
>  	return $rpcenv->fork_worker('download', undef, $user, $worker);
>      }});
>  
> +
> +__PACKAGE__->register_method({
> +    name => 'urlmeta',
> +    path => 'urlmeta',

If one would just read this when doing a pvesh ls /nodes/foo they'd be probably
have no idea what this would possibly do...

Rather use something like "retrieve-url-metadata" or "query-url-metadata".

> +    method => 'GET',
> +    description => "Download templates and ISO images by using an URL.",

But, this does not actually downloads any template, just queries some
information about an url?

> +    proxyto => 'node',
> +    permissions => {
> +	check => ['perm', '/', [ 'Sys.Audit', 'Sys.Modify' ]],
> +    },
> +    protected => 1,

why is this protected? This can run as unprivileged www user just fine, or?

> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    node => get_standard_option('pve-node'),
> +	    url => {
> +		description => "The URL to retrieve the file from.",
> +		type => 'string',
> +	    },
> +	    insecure => {
> +		description => "Allow TLS certificates to be invalid.",
> +		type => 'boolean',
> +		optional => 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};
> +
> +	die "invalid https or http url"
> +	    if $url !~ qr!^https?://!;

this could get handled directly in the schema as regex

> +
> +	my $ua = LWP::UserAgent->new();
> +	$ua->ssl_opts(
> +	    verify_hostname => 0,
> +	    SSL_verify_mode => IO::Socket::SSL::SSL_VERIFY_NONE,
> +	) if $param->{insecure};

that post-if is easily overlooked and goes against our style guide
https://pve.proxmox.com/wiki/Perl_Style_Guide#Wrapping_Post-If

Use a "normal" if () { here

> +
> +	my $req = HTTP::Request->new(HEAD => $url);
> +	my $res = $ua->request($req);
> +
> +	die "invalid server response: '" . $res->status_line() . "'"
> +	    if ($res->code() != 200);
> +
> +	my $size = $res->header("Content-Length");
> +	my $dispo = $res->header("Content-Disposition");

dispo? please avoid truncating names if not really a common abbreviation where
the code would actually benefit from the shorter version.

> +	my $type = $res->header("Content-Type");
> +
> +	my $filename;
> +
> +	if ($dispo && $dispo =~ 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;

if would fit in same line as expression

> +
> +	$ret->{size} = $size + 0
> +	    if $size;

if would fit in same line as expression, also you may want to check for definendess
here.

> +
> +	$ret->{mimetype} = $type
> +	    if $type;

if would fit in same line as expression

> +
> +	return $ret;
> +    }});
> +
>  __PACKAGE__->register_method({
>      name => 'report',
>      path => 'report',
> diff --git a/www/manager6/Makefile b/www/manager6/Makefile
> index afed3283..8e6557d8 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/Utils.js b/www/manager6/Utils.js
> index 581d2040..b7479e7a 100644
> --- a/www/manager6/Utils.js
> +++ b/www/manager6/Utils.js
> @@ -1826,6 +1826,7 @@ Ext.define('PVE.Utils', {
>  	    hastop: ['HA', gettext('Stop')],
>  	    imgcopy: ['', gettext('Copy data')],
>  	    imgdel: ['', gettext('Erase data')],
> +	    imgdownload: [gettext('Storage'), gettext('Download data')],
>  	    lvmcreate: [gettext('LVM Storage'), gettext('Create')],
>  	    lvmthincreate: [gettext('LVM-Thin Storage'), gettext('Create')],
>  	    migrateall: ['', gettext('Migrate all VMs and Containers')],
> 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'],
> +    ],
> +});
> diff --git a/www/manager6/storage/Browser.js b/www/manager6/storage/Browser.js
> index 5fee94c7..da3e66c8 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 enableRetrieve = !!(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,
> +		    enableRetrieveButton: enableUpload && enableRetrieve,
>  		    useUploadButton: true,
>  		});
>  	    }
> @@ -101,6 +106,9 @@ Ext.define('PVE.storage.Browser', {
>  		    iconCls: 'fa fa-file-o lxc',
>  		    itemId: 'contentVztmpl',
>  		    pluginType: plugin,
> +		    enableUploadButton: enableUpload,
> +		    enableRetrieveButton: enableUpload && enableRetrieve,
> +		    useUploadButton: true,
>  		});
>  	    }
>  	    if (contents.includes('snippets')) {
> diff --git a/www/manager6/storage/ContentView.js b/www/manager6/storage/ContentView.js
> index dd6df4b1..d01526e1 100644
> --- a/www/manager6/storage/ContentView.js
> +++ b/www/manager6/storage/ContentView.js
> @@ -191,6 +191,213 @@ Ext.define('PVE.storage.Upload', {
>      },
>  });
>  
> +Ext.define('PVE.storage.Retrieve', {
> +    extend: 'Proxmox.window.Edit',
> +    alias: 'widget.pveStorageRetrieve',
> +
> +    isCreate: true,
> +
> +    showTaskViewer: true,
> +
> +    title: gettext('Retrieve from URL'),
> +    submitText: gettext('Download'),
> +
> +    id: 'retrieve',
> +
> +    controller: {
> +	xclass: 'Ext.app.ViewController',
> +
> +	urlChange: function(field) {
> +	    let me = Ext.getCmp('retrieve');
> +	    field = me.down('[name=url]');
> +	    field.setValidation("Waiting for response...");
> +	    field.validate();
> +	    me.setValues({size: ""});
> +	    Proxmox.Utils.API2Request({
> +		url: `/nodes/${me.nodename}/urlmeta`,
> +		method: 'GET',
> +		params: {
> +		    url: field.getValue(),
> +		    insecure: me.getValues()['insecure'],
> +		},
> +		failure: function(res, opt) {
> +		    field.setValidation(res.result.message);
> +		    field.validate();
> +		    me.setValues({
> +			size: "",
> +			mimetype: "",
> +		    });
> +		},
> +		success: function(res, opt) {
> +		    field.setValidation();
> +		    field.validate();
> +
> +		    let data = res.result.data;
> +		    me.setValues({
> +			filename: data.filename || "",
> +			size: data.size && Proxmox.Utils.format_size(data.size) || "",
> +			mimetype: data.mimetype || "",
> +		    });
> +		},
> +	    });
> +	},
> +
> +	hashChange: function(field) {
> +	    let cecksum = Ext.getCmp('retrieveChecksum');
> +	    if (field.getValue() === '__default__') {
> +		cecksum.setDisabled(true);
> +		cecksum.setValue("");
> +		cecksum.allowBlank = true;
> +	    } else {
> +		cecksum.setDisabled(false);
> +		cecksum.allowBlank = false;
> +	    }
> +	},
> +
> +	typeChange: function(field) {
> +	    let me = Ext.getCmp('retrieve');
> +	    let content = me.getValues()['content'];
> +	    let type = field.getValue();
> +
> +	    const types = {
> +		iso: [
> +		    'application/octet-stream',
> +		    'application/x-iso9660-image',
> +		    'application/x-ima',
> +		],
> +		vztmpl: [
> +		    'application/octet-stream',
> +		    'application/gzip',
> +		    'application/tar',
> +		    'application/tar+gzip',
> +		    'application/x-gzip',
> +		    'application/x-gtar',
> +		    'application/x-tgz',
> +		    'application/x-tar',
> +		],
> +	    };
> +
> +	    if (type === "" || (types[content] && types[content].includes(type))) {
> +		field.setValidation();
> +		field.setDisabled(true);
> +	    } else {
> +		field.setDisabled(false);
> +		field.setValidation("Invalid type");
> +	    }
> +	    field.validate();
> +	},
> +    },
> +
> +    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,
> +		},
> +	    ],
> +	    column2: [
> +		{
> +		    xtype: 'textfield',
> +		    name: 'size',
> +		    disabled: true,
> +		    fieldLabel: gettext('File size'),
> +		    emptyText: gettext('unknown'),
> +		},
> +	    ],
> +	    advancedColumn1: [
> +		{
> +		    xtype: 'textfield',
> +		    name: 'checksum',
> +		    fieldLabel: gettext('Checksum'),
> +		    allowBlank: true,
> +		    disabled: true,
> +		    emptyText: gettext('none'),
> +		    id: 'retrieveChecksum',
> +		},
> +		{
> +		    xtype: 'pveHashAlgorithmSelector',
> +		    name: 'checksumalg',
> +		    fieldLabel: gettext('Hash algorithm'),
> +		    allowBlank: true,
> +		    hasNoneOption: true,
> +		    value: '__default__',
> +		    listeners: {
> +			change: 'hashChange',
> +		    },
> +		},
> +	    ],
> +	    advancedColumn2: [
> +		{
> +		    xtype: 'textfield',
> +		    fieldLabel: gettext('MIME type'),
> +		    name: 'mimetype',
> +		    disabled: true,
> +		    editable: false,
> +		    emptyText: gettext('unknown'),
> +		    listeners: {
> +			change: 'typeChange',
> +		    },
> +		},
> +		{
> +		    xtype: 'proxmoxcheckbox',
> +		    name: 'insecure',
> +		    fieldLabel: gettext('Trust invalid certificates'),
> +		    uncheckedValue: 0,
> +		    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.url = `/nodes/${me.nodename}/storage/${me.storage}/retrieve`;
> +	me.method = 'POST';
> +
> +	let contentTypeSel = me.items[0].column1[0];
> +	contentTypeSel.cts = me.contents;
> +	contentTypeSel.value = me.contents[0] || '';
> +
> +        me.callParent();
> +    },
> +});
> +
>  Ext.define('PVE.storage.ContentView', {
>      extend: 'Ext.grid.GridPanel',
>  
> @@ -249,36 +456,60 @@ 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.push(
> +		{
> +		    xtype: 'button',
> +		    text: gettext('Upload'),
> +		    disabled: !me.enableUploadButton,
> +		    handler: function() {
> +			Ext.create('PVE.storage.Upload', {
> +			    nodename: nodename,
> +			    storage: storage,
> +			    contents: [content],
> +			    autoShow: true,
> +			    listeners:{
> +				destroy: () => reload(),
> +			    }
> +			});
> +		    },
> +		},
> +		{
> +		    xtype: 'button',
> +		    text: gettext('Retrieve from URL'),
> +		    disabled: !me.enableRetrieveButton,
> +		    handler: function() {
> +			Ext.create('PVE.storage.Retrieve', {
> +			    nodename: nodename,
> +			    storage: storage,
> +			    contents: [content],
> +			    autoShow: true,
> +			    listeners: {
> +				destroy: () => reload(),
> +			    },
> +			});
> +		    },
> +		},
> +		'-',
> +	    );
>  	}
> -	if (!me.useCustomRemoveButton) {
> -	    me.tbar.push(removeButton);
> +	if (me.useCustomRemoveButton) {
> +	    // custom remove button was inserted as first element
> +	    // -> place it at the end of tbar
> +	    me.tbar.push(me.tbar.shift());
> +	} else {
> +	    me.tbar.push({
> +		xtype: 'proxmoxStdRemoveButton',
> +		selModel: sm,
> +		delay: 5,
> +		callback: function() {
> +		    reload();
> +		},
> +		baseurl: baseurl + '/',
> +	    });
>  	}
>  	me.tbar.push(
>  	    '->',
> 

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

[Lorenz Stechauner]

changes to v3:

split up into more patches

CT template download (aplinfo) and download from url refactored
to use same (now new) function in PVE::Tools

patches:

1 - manager: add query_url_metadata method
2 - common: add download_file_from_url
3 - manager: refactor aplinfo to use common download function
4 - storage: add download_url method
5 - manager: add HashAlgorithmSelector
6 - manager: change download task format
7 - manager: fix #1710: add download from url button

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                          | 158 +++++++-----
 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        | 282 +++++++++++++++++++--
 6 files changed, 375 insertions(+), 92 deletions(-)
 create mode 100644 www/manager6/form/HashAlgorithmSelector.js


pve-common:
Lorenz Stechauner (1):
  tools: add download_file_from_url

 src/PVE/Tools.pm | 123 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 123 insertions(+)


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

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

-- 
2.20.1

[pve-devel] [PATCH v4 manager 1/7] api: nodes: add query_url_metadata method

[Lorenz Stechauner]

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

diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
index e58d9c10..a6681ab3 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,99 @@ __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});
+	}
+
+	if (!$param->{'verify-certificates'}) {
+	    $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=(.+)/) {
+	    $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 v4 common 2/7] tools: add download_file_from_url

[Lorenz Stechauner]

code is based on
manager:PVE/API2/Nodes.pm:aplinfo

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

diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
index 16ae3d2..c751426 100644
--- a/src/PVE/Tools.pm
+++ b/src/PVE/Tools.pm
@@ -1829,4 +1829,127 @@ sub safe_compare {
     return $cmp->($left, $right);
 }
 
+
+# opts
+#  -> hash_required
+#  -> http_proxy
+#  -> verify_certificates
+#  -> sha(1|224|256|384|512)sum
+#  -> md5sum
+sub download_file_from_url {
+    my ($dest, $url, $opts) = @_;
+
+    my $tmpdest = "$dest.tmp.$$";
+
+    my $worker = sub  {
+	my $upid = shift;
+
+	print "donwloading $url to $dest\n";
+
+	eval {
+	    if (-f $dest) {
+		print "calculating checksum of existing file...\n";
+		my ($correct, $hash, $expected) = check_file_hash($opts, $dest, 1);
+
+		if ($hash && $correct) {
+		    print "file already exists - no need to download\n";
+		    return;
+		} else {
+		    print "mismatch, downloading\n";
+		}
+	    }
+
+	    my @cmd = ('/usr/bin/wget', '--progress=dot:mega', '-O', $tmpdest, $url);
+
+	    local %ENV;
+	    if ($opts->{http_proxy}) {
+		$ENV{http_proxy} = $opts->{http_proxy};
+	    }
+
+	    if (defined($opts->{verify_certificates}) && $opts->{verify_certificates} == 0) {
+		push @cmd, '--no-check-certificate';
+	    }
+
+	    if (system(@cmd) != 0) {
+		die "download failed - $!\n";
+	    }
+
+	    print "trying to calculate checksum...\n";
+
+	    my ($correct, $hash, $expected) = check_file_hash($opts, $tmpdest, !$opts->{hash_required});
+
+	    die "could not calculate checksum\n" if ($opts->{hash_required} && !$hash);
+
+	    if ($hash) {
+		if ($correct) {
+		    print "checksum verified\n";
+		} else {
+		    die "wrong checksum: $hash != $expected\n";
+		}
+	    } else {
+		print "no checksum for verification specified\n";
+	    }
+
+	    if (!rename($tmpdest, $dest)) {
+		die "unable to save file - $!\n";
+	    }
+	};
+	my $err = $@;
+
+	unlink $tmpdest;
+
+	if ($err) {
+	    print "\n";
+	    die $err;
+	}
+
+	print "download finished\n";
+    };
+
+    my $rpcenv = PVE::RPCEnvironment::get();
+    my $user = $rpcenv->get_user();
+
+    (my $filename = $dest) =~ s!.*/([^/]*)$!\1!;
+
+    return $rpcenv->fork_worker('download', $filename, $user, $worker);
+}
+
+sub check_file_hash {
+    my ($checksums, $filename, $noerr) = @_;
+
+    my $digest;
+    my $expected;
+
+    eval {
+	open(my $fh, '<', $filename) or die "Can't open '$filename': $!";
+	binmode($fh);
+	if (defined($checksums->{sha512sum})) {
+	    $expected = $checksums->{sha512sum};
+	    $digest = Digest::SHA->new(512)->addfile($fh)->hexdigest;
+	} elsif (defined($checksums->{sha384sum})) {
+	    $expected = $checksums->{sha384sum};
+	    $digest = Digest::SHA->new(384)->addfile($fh)->hexdigest;
+	} elsif (defined($checksums->{sha256sum})) {
+	    $expected = $checksums->{sha256sum};
+	    $digest = Digest::SHA->new(256)->addfile($fh)->hexdigest;
+	} elsif (defined($checksums->{sha224sum})) {
+	    $expected = $checksums->{sha224sum};
+	    $digest = Digest::SHA->new(224)->addfile($fh)->hexdigest;
+	} elsif (defined($checksums->{sha1sum})) {
+	    $expected = $checksums->{sha1sum};
+	    $digest = Digest::SHA->new(1)->addfile($fh)->hexdigest;
+	} elsif (defined($checksums->{md5sum})) {
+	    $expected = $checksums->{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 ? lc($digest) eq lc($expected) : 0), $digest, $expected);
+}
+
 1;
-- 
2.20.1

[pve-devel] [PATCH v4 manager 3/7] api: nodes: refactor aplinfo to use common download function

[Lorenz Stechauner]

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

diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
index a6681ab3..34cd2f7c 100644
--- a/PVE/API2/Nodes.pm
+++ b/PVE/API2/Nodes.pm
@@ -1513,88 +1513,19 @@ __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";
-		}
-
-		if (!rename($tmpdest, $dest)) {
-		    die "unable to save file - $!\n";
-		}
-	    };
-	    my $err = $@;
-
-	    unlink $tmpdest;
-
-	    if ($err) {
-		print "\n";
-		die $err if $err;
-	    }
-
-	    print "download finished\n";
+	my $opts = {
+	    hash_required => 1,
+	    sha512sum => $pd->{sha512sum},
+	    md5sum => $pd->{md5sum},
 	};
 
-	return $rpcenv->fork_worker('download', undef, $user, $worker);
+	my $dccfg = PVE::Cluster::cfs_read_file('datacenter.cfg');
+	if ($dccfg->{http_proxy}) {
+	    $opts->{http_proxy} = $dccfg->{http_proxy};
+	}
+
+	return PVE::Tools::download_file_from_url($dest, $src, $opts);
     }});
 
 __PACKAGE__->register_method({
-- 
2.20.1

[pve-devel] [PATCH v4 storage 4/7] status: add download_url method

[Lorenz Stechauner]

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

diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
index 897b4a7..e1d656e 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,115 @@ __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,
+	};
+
+	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};
+	}
+
+	return PVE::Tools::download_file_from_url($dest, $url, $opts);
+    }});
+
 1;
diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index 122c3e9..d57fd43 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -1931,4 +1931,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 v4 manager 5/7] 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 afed3283..8e6557d8 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 v4 manager 6/7] 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 581d2040..38306c11 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -1819,7 +1819,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 v4 manager 7/7] fix #1710: ui: storage: add download from url button

[Lorenz Stechauner]

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

diff --git a/www/manager6/storage/Browser.js b/www/manager6/storage/Browser.js
index 5fee94c7..c0d647d3 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 = !!(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: enableUpload && 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: enableUpload && enableDownloadUrl,
+		    useUploadButton: true,
 		});
 	    }
 	    if (contents.includes('snippets')) {
diff --git a/www/manager6/storage/ContentView.js b/www/manager6/storage/ContentView.js
index dd6df4b1..9125c0bb 100644
--- a/www/manager6/storage/ContentView.js
+++ b/www/manager6/storage/ContentView.js
@@ -191,6 +191,214 @@ Ext.define('PVE.storage.Upload', {
     },
 });
 
+Ext.define('PVE.storage.DownloadUrl', {
+    extend: 'Proxmox.window.Edit',
+    alias: 'widget.pveStorageDownloadUrl',
+
+    isCreate: true,
+
+    showTaskViewer: true,
+
+    title: gettext('Download from URL'),
+    submitText: gettext('Download'),
+
+    id: 'download-url',
+
+    controller: {
+	xclass: 'Ext.app.ViewController',
+
+	urlChange: function(field) {
+	    let me = Ext.getCmp('download-url');
+	    field = me.down('[name=url]');
+	    field.setValidation("Waiting for response...");
+	    field.validate();
+	    me.setValues({size: ""});
+	    Proxmox.Utils.API2Request({
+		url: `/nodes/${me.nodename}/query-url-metadata`,
+		method: 'GET',
+		params: {
+		    url: field.getValue(),
+		    'verify-certificates': me.getValues()['verify-certificates'],
+		},
+		failure: function(res, opt) {
+		    field.setValidation(res.result.message);
+		    field.validate();
+		    me.setValues({
+			size: "",
+			mimetype: "",
+		    });
+		},
+		success: function(res, opt) {
+		    field.setValidation();
+		    field.validate();
+
+		    let data = res.result.data;
+		    me.setValues({
+			filename: data.filename || "",
+			size: data.size && Proxmox.Utils.format_size(data.size) || "",
+			mimetype: data.mimetype || "",
+		    });
+		},
+	    });
+	},
+
+	hashChange: function(field) {
+	    let cecksum = Ext.getCmp('downloadUrlChecksum');
+	    if (field.getValue() === '__default__') {
+		cecksum.setDisabled(true);
+		cecksum.setValue("");
+		cecksum.allowBlank = true;
+	    } else {
+		cecksum.setDisabled(false);
+		cecksum.allowBlank = false;
+	    }
+	},
+
+	typeChange: function(field) {
+	    let me = Ext.getCmp('download-url');
+	    let content = me.getValues()['content'];
+	    let type = field.getValue();
+
+	    const types = {
+		iso: [
+		    'application/octet-stream',
+		    'application/x-iso9660-image',
+		    'application/x-ima',
+		],
+		vztmpl: [
+		    'application/octet-stream',
+		    'application/gzip',
+		    'application/tar',
+		    'application/tar+gzip',
+		    'application/x-gzip',
+		    'application/x-gtar',
+		    'application/x-tgz',
+		    'application/x-tar',
+		],
+	    };
+
+	    if (type === "" || (types[content] && types[content].includes(type))) {
+		field.setValidation();
+		field.setDisabled(true);
+	    } else {
+		field.setDisabled(false);
+		field.setValidation("Invalid type");
+	    }
+	    field.validate();
+	},
+    },
+
+    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,
+		},
+	    ],
+	    column2: [
+		{
+		    xtype: 'textfield',
+		    name: 'size',
+		    disabled: true,
+		    fieldLabel: gettext('File size'),
+		    emptyText: gettext('unknown'),
+		},
+	    ],
+	    advancedColumn1: [
+		{
+		    xtype: 'textfield',
+		    name: 'checksum',
+		    fieldLabel: gettext('Checksum'),
+		    allowBlank: true,
+		    disabled: true,
+		    emptyText: gettext('none'),
+		    id: 'downloadUrlChecksum',
+		},
+		{
+		    xtype: 'pveHashAlgorithmSelector',
+		    name: 'checksum-algorithm',
+		    fieldLabel: gettext('Hash algorithm'),
+		    allowBlank: true,
+		    hasNoneOption: true,
+		    value: '__default__',
+		    listeners: {
+			change: 'hashChange',
+		    },
+		},
+	    ],
+	    advancedColumn2: [
+		{
+		    xtype: 'textfield',
+		    fieldLabel: gettext('MIME type'),
+		    name: 'mimetype',
+		    disabled: true,
+		    editable: false,
+		    emptyText: gettext('unknown'),
+		    listeners: {
+			change: 'typeChange',
+		    },
+		},
+		{
+		    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.url = `/nodes/${me.nodename}/storage/${me.storage}/download-url`;
+	me.method = 'POST';
+
+	let contentTypeSel = me.items[0].column1[0];
+	contentTypeSel.cts = me.contents;
+	contentTypeSel.value = me.contents[0] || '';
+
+        me.callParent();
+    },
+});
+
 Ext.define('PVE.storage.ContentView', {
     extend: 'Ext.grid.GridPanel',
 
@@ -249,36 +457,60 @@ 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.push(
+		{
+		    xtype: 'button',
+		    text: gettext('Upload'),
+		    disabled: !me.enableUploadButton,
+		    handler: function() {
+			Ext.create('PVE.storage.Upload', {
+			    nodename: nodename,
+			    storage: storage,
+			    contents: [content],
+			    autoShow: true,
+			    listeners:{
+				destroy: () => 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,
+			    listeners: {
+				destroy: () => reload(),
+			    },
+			});
+		    },
+		},
+		'-',
+	    );
 	}
-	if (!me.useCustomRemoveButton) {
-	    me.tbar.push(removeButton);
+	if (me.useCustomRemoveButton) {
+	    // custom remove button was inserted as first element
+	    // -> place it at the end of tbar
+	    me.tbar.push(me.tbar.shift());
+	} else {
+	    me.tbar.push({
+		xtype: 'proxmoxStdRemoveButton',
+		selModel: sm,
+		delay: 5,
+		callback: function() {
+		    reload();
+		},
+		baseurl: baseurl + '/',
+	    });
 	}
 	me.tbar.push(
 	    '->',
-- 
2.20.1

[pve-devel] [PATCH v5 storage] status: add download_url method

[Lorenz Stechauner]

Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
---
 Somehow I forgot to git add Status.pm after a fix...

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

diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
index 897b4a7..f8ca329 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,115 @@ __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,
+	};
+
+	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};
+	}
+
+	return PVE::Tools::download_file_from_url($dest, $url, $opts);
+    }});
+
 1;
diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index 122c3e9..d57fd43 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -1931,4 +1931,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

Re: [pve-devel] [PATCH v4 common 2/7] tools: add download_file_from_url

[Oguz Bektas]

hi,

see inline for some small suggestions :)

On Thu, May 06, 2021 at 11:11:00AM +0200, Lorenz Stechauner wrote:
> code is based on
> manager:PVE/API2/Nodes.pm:aplinfo
> 
> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
> ---
>  src/PVE/Tools.pm | 123 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 123 insertions(+)
> 
> diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
> index 16ae3d2..c751426 100644
> --- a/src/PVE/Tools.pm
> +++ b/src/PVE/Tools.pm
> @@ -1829,4 +1829,127 @@ sub safe_compare {
>      return $cmp->($left, $right);
>  }
>  
> +
> +# opts
> +#  -> hash_required
> +#  -> http_proxy
> +#  -> verify_certificates
> +#  -> sha(1|224|256|384|512)sum
> +#  -> md5sum
> +sub download_file_from_url {
> +    my ($dest, $url, $opts) = @_;
> +
> +    my $tmpdest = "$dest.tmp.$$";
> +
> +    my $worker = sub  {
> +	my $upid = shift;
> +
> +	print "donwloading $url to $dest\n";

small typo here

> +
> +	eval {
> +	    if (-f $dest) {
> +		print "calculating checksum of existing file...\n";
> +		my ($correct, $hash, $expected) = check_file_hash($opts, $dest, 1);
> +
> +		if ($hash && $correct) {
> +		    print "file already exists - no need to download\n";
> +		    return;
> +		} else {
> +		    print "mismatch, downloading\n";
> +		}
> +	    }
> +
> +	    my @cmd = ('/usr/bin/wget', '--progress=dot:mega', '-O', $tmpdest, $url);
> +
> +	    local %ENV;
> +	    if ($opts->{http_proxy}) {
> +		$ENV{http_proxy} = $opts->{http_proxy};

might be worth it to also add https_proxy here

> +	    }
> +
> +	    if (defined($opts->{verify_certificates}) && $opts->{verify_certificates} == 0) {
> +		push @cmd, '--no-check-certificate';
> +	    }
> +
> +	    if (system(@cmd) != 0) {
> +		die "download failed - $!\n";
> +	    }

we don't use 'system' for executing commands (especially when a command
parameter is supplied by a user!). see the 'run_command' helper in
pve-common (which also does shellquoting)

> +
> +	    print "trying to calculate checksum...\n";
> +
> +	    my ($correct, $hash, $expected) = check_file_hash($opts, $tmpdest, !$opts->{hash_required});

is it necessary to call check_file_hash unless the option hash_required
is passed?

> +
> +	    die "could not calculate checksum\n" if ($opts->{hash_required} && !$hash);
> +
> +	    if ($hash) {
> +		if ($correct) {
> +		    print "checksum verified\n";
> +		} else {
> +		    die "wrong checksum: $hash != $expected\n";
> +		}
> +	    } else {
> +		print "no checksum for verification specified\n";
> +	    }
> +
> +	    if (!rename($tmpdest, $dest)) {
> +		die "unable to save file - $!\n";
> +	    }
> +	};
> +	my $err = $@;
> +
> +	unlink $tmpdest;
> +
> +	if ($err) {
> +	    print "\n";
> +	    die $err;
> +	}
> +
> +	print "download finished\n";
> +    };
> +
> +    my $rpcenv = PVE::RPCEnvironment::get();
> +    my $user = $rpcenv->get_user();
> +
> +    (my $filename = $dest) =~ s!.*/([^/]*)$!\1!;
> +
> +    return $rpcenv->fork_worker('download', $filename, $user, $worker);
> +}
> +
> +sub check_file_hash {
> +    my ($checksums, $filename, $noerr) = @_;
> +
> +    my $digest;
> +    my $expected;
> +
> +    eval {
> +	open(my $fh, '<', $filename) or die "Can't open '$filename': $!";
> +	binmode($fh);
> +	if (defined($checksums->{sha512sum})) {
> +	    $expected = $checksums->{sha512sum};
> +	    $digest = Digest::SHA->new(512)->addfile($fh)->hexdigest;
> +	} elsif (defined($checksums->{sha384sum})) {
> +	    $expected = $checksums->{sha384sum};
> +	    $digest = Digest::SHA->new(384)->addfile($fh)->hexdigest;
> +	} elsif (defined($checksums->{sha256sum})) {
> +	    $expected = $checksums->{sha256sum};
> +	    $digest = Digest::SHA->new(256)->addfile($fh)->hexdigest;
> +	} elsif (defined($checksums->{sha224sum})) {
> +	    $expected = $checksums->{sha224sum};
> +	    $digest = Digest::SHA->new(224)->addfile($fh)->hexdigest;
> +	} elsif (defined($checksums->{sha1sum})) {
> +	    $expected = $checksums->{sha1sum};
> +	    $digest = Digest::SHA->new(1)->addfile($fh)->hexdigest;
> +	} elsif (defined($checksums->{md5sum})) {
> +	    $expected = $checksums->{md5sum};
> +	    $digest = Digest::MD5->new->addfile($fh)->hexdigest;

hmm not necessary but maybe you could also do something like this (not
tested):

...
my $sha_algorithms = ('1', '224', '256', '384', '512');
foreach my $algorithm (@$sha_algorithms) {
    if (defined($checksums->{"sha$algorithm"})) {
	$expected = $checksums->{"sha$algorithm"};
	$digest = Digest::SHA->new($algorithm)->addfile($fh)->hexdigest;
    }
}

to avoid having a lot of if/elsif clauses (md5 would probably have another
clause but 2 is better than 5-6).


> +	} else {
> +	    die "no expected checksum defined";
> +	}
> +	close($fh);
> +    };
> +
> +    die "checking hash failed - $@\n" if $@ && !$noerr;
> +
> +    return (($digest ? lc($digest) eq lc($expected) : 0), $digest, $expected);
> +}
> +
>  1;
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 

Re: [pve-devel] [PATCH v4 common 2/7] tools: add download_file_from_url

[Thomas Lamprecht]

thanks for the review, something on top inline.

On 06.05.21 12:04, Oguz Bektas wrote:
> On Thu, May 06, 2021 at 11:11:00AM +0200, Lorenz Stechauner wrote:

>> +
>> +	    my @cmd = ('/usr/bin/wget', '--progress=dot:mega', '-O', $tmpdest, $url);
>> +
>> +	    local %ENV;
>> +	    if ($opts->{http_proxy}) {
>> +		$ENV{http_proxy} = $opts->{http_proxy};
> 
> might be worth it to also add https_proxy here

True, but would be a separate series out of scope here, needs to gain
support in datacenter.cfg
https://pve.proxmox.com/pve-docs/datacenter.cfg.5.html#_options

May be relevant to talk with Dietmar about the upcomming possibilities in PBS,
he checked out HTTP proxies quite closely recently.

> [snip]
>> +
>> +sub check_file_hash {
>> +    my ($checksums, $filename, $noerr) = @_;
>> +
>> +    my $digest;
>> +    my $expected;
>> +
>> +    eval {
>> +	open(my $fh, '<', $filename) or die "Can't open '$filename': $!";


as already mentioned in a previous review, add the trailing new line "\n" to
die statements, else they will get ugly by adding internal information!

>> +	binmode($fh);
>> +	if (defined($checksums->{sha512sum})) {
>> +	    $expected = $checksums->{sha512sum};
>> +	    $digest = Digest::SHA->new(512)->addfile($fh)->hexdigest;
>> +	} elsif (defined($checksums->{sha384sum})) {
>> +	    $expected = $checksums->{sha384sum};
>> +	    $digest = Digest::SHA->new(384)->addfile($fh)->hexdigest;
>> +	} elsif (defined($checksums->{sha256sum})) {
>> +	    $expected = $checksums->{sha256sum};
>> +	    $digest = Digest::SHA->new(256)->addfile($fh)->hexdigest;
>> +	} elsif (defined($checksums->{sha224sum})) {
>> +	    $expected = $checksums->{sha224sum};
>> +	    $digest = Digest::SHA->new(224)->addfile($fh)->hexdigest;
>> +	} elsif (defined($checksums->{sha1sum})) {
>> +	    $expected = $checksums->{sha1sum};
>> +	    $digest = Digest::SHA->new(1)->addfile($fh)->hexdigest;
>> +	} elsif (defined($checksums->{md5sum})) {
>> +	    $expected = $checksums->{md5sum};
>> +	    $digest = Digest::MD5->new->addfile($fh)->hexdigest;
> 
> hmm not necessary but maybe you could also do something like this (not
> tested):
> 
> ...
> my $sha_algorithms = ('1', '224', '256', '384', '512');
> foreach my $algorithm (@$sha_algorithms) {

use for over foreach, and a list can be used directly, no need for a useless
intermediate variable:

for my $foo ('a', 'b', 'c') {
    ...


>     if (defined($checksums->{"sha$algorithm"})) {
> 	$expected = $checksums->{"sha$algorithm"};
> 	$digest = Digest::SHA->new($algorithm)->addfile($fh)->hexdigest;

You can also use strings as module in perl:

$digest = "Digest::$algorithm"->new->addfile...


>     }
> }
> 
> to avoid having a lot of if/elsif clauses (md5 would probably have another
> clause but 2 is better than 5-6).

with < 10 elements that can be fine, but here the whole method is weird in UX
IMO and could be improved in general by:

1. pass alogrirhm and expected hash string directly
2. use a map for the different modules
3. let the caller handle the error (albeit no hard feelings here)

sub check_file_hash
   my ($algorithm, $expected, $file) = @_;

   my $algorithm_map = {
      'sha256' => sub { Digest::SHA->new(512) },
      'sha512' => sub { Digest::SHA->new(512) },
      # etc...
   };

   my $digester = $algorithm_map->{$algorithm}->() or die "unknown algorithm '$algorithm'\n";

   open(my $fh, '<', $filename) or die "cannot open file '$file': $!\n";

   my $got = $digester->addfile($fh)->hexdigest;
   close($fh);

   return lc($digest) eq lc($expected);
}


IMO much simpler/shorter and still easy to grasp.

> 
> 
>> +	} else {
>> +	    die "no expected checksum defined";
>> +	}
>> +	close($fh);
>> +    };
>> +
>> +    die "checking hash failed - $@\n" if $@ && !$noerr;
>> +
>> +    return (($digest ? lc($digest) eq lc($expected) : 0), $digest, $expected);
>> +}
>> +
>>  1;
>> -- 
>> 2.20.1

Re: [pve-devel] [PATCH v4 common 2/7] tools: add download_file_from_url

[Thomas Lamprecht]

On 06.05.21 14:15, Thomas Lamprecht wrote:
> sub check_file_hash
>    my ($algorithm, $expected, $file) = @_;
> 
>    my $algorithm_map = {
>       'sha256' => sub { Digest::SHA->new(512) },

argh, naturally above should use 256, but you get the idea ;-)

>       'sha512' => sub { Digest::SHA->new(512) },
>       # etc...
>    };
> 
>    my $digester = $algorithm_map->{$algorithm}->() or die "unknown algorithm '$algorithm'\n";
> 
>    open(my $fh, '<', $filename) or die "cannot open file '$file': $!\n";
> 
>    my $got = $digester->addfile($fh)->hexdigest;
>    close($fh);
> 
>    return lc($digest) eq lc($expected);
> }

Re: [pve-devel] [PATCH v4 manager 1/7] api: nodes: add query_url_metadata method

[Dominik Csapak]

one comment inline

On 5/6/21 11:10, Lorenz Stechauner wrote:
> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
> ---
>   PVE/API2/Nodes.pm | 95 +++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 95 insertions(+)
> 
> diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
> index e58d9c10..a6681ab3 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,99 @@ __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});
> +	}
> +
> +	if (!$param->{'verify-certificates'}) {

this does not work sadly

we do not fill in the defaults from the api schema into $param
so when nothing is given
$param->{'verify-certificates'} will be 'undef'

if you want a 'truthy' default you have to do it yourself:

my $verify = $param->{'verify-certificates'} // 1;

(although there are some pieces of code where we *do* inject the 
default, e.g. pmg config parsing...)


> +	    $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=(.+)/) {
> +	    $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',
>