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

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

[Lorenz Stechauner]

changes to v9:
* dropped already applied paches
* split storage-patch into multiple patches
* added download-url api endpoint to index
* updated description of filename parameter (download-url)
* added two new patches to factor out regex'es (manager+storage)


pve-storage:
Lorenz Stechauner (3):
  factoring out regex'es for backup and vztmpl
  status: factoring out normalize_content_filename
  status: add download_url method

 PVE/API2/Storage/Status.pm     | 128 ++++++++++++++++++++++++++++++---
 PVE/Storage.pm                 |  23 +++++-
 PVE/Storage/Plugin.pm          |  15 ++--
 test/path_to_volume_id_test.pm |   7 +-
 4 files changed, 153 insertions(+), 20 deletions(-)


pve-manager
Lorenz Stechauner (4):
  aplinfo: factoring out regex for vztmpl
  api: nodes: add query_url_metadata method
  ui: Utils: change download task format
  fix #1710: ui: storage: add download from url button

 PVE/API2/Nodes.pm                   |  96 +++++++++++
 PVE/APLInfo.pm                      |   3 +-
 www/manager6/Utils.js               |   2 +-
 www/manager6/storage/Browser.js     |   8 +
 www/manager6/storage/ContentView.js | 247 +++++++++++++++++++++++++---
 5 files changed, 330 insertions(+), 26 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH v10 manager 1/4] aplinfo: factoring out regex for vztmpl

[Lorenz Stechauner]

Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
---
 PVE/APLInfo.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/PVE/APLInfo.pm b/PVE/APLInfo.pm
index 31522ae5..fa991a1b 100644
--- a/PVE/APLInfo.pm
+++ b/PVE/APLInfo.pm
@@ -8,6 +8,7 @@ use LWP::UserAgent;
 use POSIX qw(strftime);
 
 use PVE::SafeSyslog;
+use PVE::Storage;
 use PVE::Tools qw(run_command);
 use PVE::pvecfg;
 
@@ -83,7 +84,7 @@ sub read_aplinfo_from_fh {
 	    my $template;
 	    if ($res->{location}) {
 		$template = $res->{location};
-		$template =~ s|.*/([^/]+.tar.[gx]z)$|$1|;
+		$template =~ s|.*/([^/]+$PVE::Storage::vztmpl_extension_re)$|$1|;
 		if ($res->{location} !~ m|^([a-zA-Z]+)\://|) {
 		    # relative localtion (no http:// prefix)
 		    $res->{location} = "$source/$res->{location}";
-- 
2.30.2

[pve-devel] [PATCH v10 manager 2/4] api: nodes: add query_url_metadata method

[Lorenz Stechauner]

metadata is gained using a HEAD request.

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

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

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

[pve-devel] [PATCH v10 manager 3/4] 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 3415c9eb..6f208954 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -1776,7 +1776,7 @@ Ext.define('PVE.Utils', {
 	    clusterjoin: ['', gettext('Join Cluster')],
 	    dircreate: [gettext('Directory Storage'), gettext('Create')],
 	    dirremove: [gettext('Directory'), gettext('Remove')],
-	    download: ['', gettext('Download')],
+	    download: [gettext('File'), gettext('Download')],
 	    hamigrate: ['HA', gettext('Migrate')],
 	    hashutdown: ['HA', gettext('Shutdown')],
 	    hastart: ['HA', gettext('Start')],
-- 
2.30.2

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

[Lorenz Stechauner]

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

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

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

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

[pve-devel] [PATCH v10 storage 1/3] factoring out regex'es for backup and vztmpl

[Lorenz Stechauner]

uniformly stores these regex definitions in PVE::Storage.

One test had to be adapted because it tested obsolete code. Namely:
it expects vztmpl to only end with .tar.gz, but the new regex also
includes .tar.xz, there is nothing against allowing .tar.xz files as
vztmpl files.

Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
---
 PVE/API2/Storage/Status.pm     |  6 +++---
 PVE/Storage.pm                 | 11 ++++++++---
 PVE/Storage/Plugin.pm          | 15 +++++++++------
 test/path_to_volume_id_test.pm |  7 ++++---
 4 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
index 8a36aef..7069244 100644
--- a/PVE/API2/Storage/Status.pm
+++ b/PVE/API2/Storage/Status.pm
@@ -423,12 +423,12 @@ __PACKAGE__->register_method ({
 
 	if ($content eq 'iso') {
 	    if ($filename !~ m![^/]+$PVE::Storage::iso_extension_re$!) {
-		raise_param_exc({ filename => "missing '.iso' or '.img' extension" });
+		raise_param_exc({ filename => "wrong file 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" });
+	    if ($filename !~ m![^/]+$PVE::Storage::vztmpl_extension_re$!) {
+		raise_param_exc({ filename => "wrong file extension" });
 	    }
 	    $path = PVE::Storage::get_vztmpl_dir($cfg, $param->{storage});
 	} else {
diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index ea887a4..519431c 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -101,6 +101,11 @@ PVE::Storage::Plugin->init();
 
 our $iso_extension_re = qr/\.(?:iso|img)/i;
 
+our $vztmpl_extension_re = qr/\.tar\.([gx]z)/i;
+
+# Caution: because of the '$' inside, this regex may only used to match at the end of strings
+our $backup_extension_re = qr/\.(tgz$|tar|vma)(?:\.(${\PVE::Storage::Plugin::COMPRESSOR_RE}))?/i;
+
 #  PVE::Storage utility functions
 
 sub config {
@@ -573,13 +578,13 @@ sub path_to_volume_id {
 	} elsif ($path =~ m!^$isodir/([^/]+$iso_extension_re)$!) {
 	    my $name = $1;
 	    return ('iso', "$sid:iso/$name");
-	} elsif ($path =~ m!^$tmpldir/([^/]+\.tar\.gz)$!) {
+	} elsif ($path =~ m!^$tmpldir/([^/]+$vztmpl_extension_re)$!) {
 	    my $name = $1;
 	    return ('vztmpl', "$sid:vztmpl/$name");
 	} elsif ($path =~ m!^$privatedir/(\d+)$!) {
 	    my $vmid = $1;
 	    return ('rootdir', "$sid:rootdir/$vmid");
-	} elsif ($path =~ m!^$backupdir/([^/]+\.(?:tgz|(?:(?:tar|vma)(?:\.(?:${\PVE::Storage::Plugin::COMPRESSOR_RE}))?)))$!) {
+	} elsif ($path =~ m!^$backupdir/([^/]+$backup_extension_re)$!) {
 	    my $name = $1;
 	    return ('backup', "$sid:backup/$name");
 	} elsif ($path =~ m!^$snippetsdir/([^/]+)$!) {
@@ -1463,7 +1468,7 @@ sub archive_info {
     my $info;
 
     my $volid = basename($archive);
-    if ($volid =~ /^(vzdump-(lxc|openvz|qemu)-.+\.(tgz$|tar|vma)(?:\.(${\PVE::Storage::Plugin::COMPRESSOR_RE}))?)$/) {
+    if ($volid =~ /^(vzdump-(lxc|openvz|qemu)-.+$backup_extension_re)$/) {
 	my $filename = "$1"; # untaint
 	my ($type, $format, $comp) = ($2, $3, $4);
 	my $format_re = defined($comp) ? "$format.$comp" : "$format";
diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index d330845..afb05ed 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -518,11 +518,11 @@ sub parse_volname {
 	return ('images', $name, $vmid, undef, undef, $isBase, $format);
     } elsif ($volname =~ m!^iso/([^/]+$PVE::Storage::iso_extension_re)$!) {
 	return ('iso', $1);
-    } elsif ($volname =~ m!^vztmpl/([^/]+\.tar\.[gx]z)$!) {
+    } elsif ($volname =~ m!^vztmpl/([^/]+$PVE::Storage::vztmpl_extension_re)$!) {
 	return ('vztmpl', $1);
     } elsif ($volname =~ m!^rootdir/(\d+)$!) {
 	return ('rootdir', $1, $1);
-    } elsif ($volname =~ m!^backup/([^/]+(?:\.(?:tgz|(?:(?:tar|vma)(?:\.(?:${\COMPRESSOR_RE}))?))))$!) {
+    } elsif ($volname =~ m!^backup/([^/]+$PVE::Storage::backup_extension_re)$!) {
 	my $fn = $1;
 	if ($fn =~ m/^vzdump-(openvz|lxc|qemu)-(\d+)-.+/) {
 	    return ('backup', $fn, $2);
@@ -1041,15 +1041,18 @@ my $get_subdir_files = sub {
 	    $info = { volid => "$sid:iso/$1", format => 'iso' };
 
 	} elsif ($tt eq 'vztmpl') {
-	    next if $fn !~ m!/([^/]+\.tar\.([gx]z))$!;
+	    next if $fn !~ m!/([^/]+$PVE::Storage::vztmpl_extension_re)$!;
 
 	    $info = { volid => "$sid:vztmpl/$1", format => "t$2" };
 
 	} elsif ($tt eq 'backup') {
-	    next if $fn !~ m!/([^/]+\.(tgz|(?:(?:tar|vma)(?:\.(${\COMPRESSOR_RE}))?)))$!;
+	    next if $fn !~ m!/([^/]+$PVE::Storage::backup_extension_re)$!;
+
 	    my $original = $fn;
-	    my $format = $2;
-	    $fn = $1;
+	    my ($format, $comp);
+
+	    ($fn, $format, $comp) = ($1, $2, $3);
+	    $format = defined($comp) ? "$format.$comp" : $format;
 
 	    # only match for VMID now, to avoid false positives (VMID in parent directory name)
 	    next if defined($vmid) && $fn !~ m/\S+-$vmid-\S+/;
diff --git a/test/path_to_volume_id_test.pm b/test/path_to_volume_id_test.pm
index 5eee2f6..e0c46d9 100644
--- a/test/path_to_volume_id_test.pm
+++ b/test/path_to_volume_id_test.pm
@@ -166,12 +166,13 @@ my @tests = (
 	    'local:snippets/hookscript.pl',
 	],
     },
-
-    # no matches
     {
 	description => 'CT template, tar.xz',
 	volname     => "$storage_dir/template/cache/debian-10.0-standard_10.0-1_amd64.tar.xz",
-	expected    => [''],
+	expected    => [
+	    'vztmpl',
+	    'local:vztmpl/debian-10.0-standard_10.0-1_amd64.tar.xz'
+	],
     },
 
     # no matches, path or files with failures
-- 
2.30.2

[pve-devel] [PATCH v10 storage 2/3] status: factoring out normalize_content_filename

[Lorenz Stechauner]

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

diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
index 7069244..11ad60f 100644
--- a/PVE/API2/Storage/Status.pm
+++ b/PVE/API2/Storage/Status.pm
@@ -413,11 +413,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;
 
diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index 519431c..20f1c3d 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -1930,4 +1930,16 @@ sub assert_sid_unused {
     return undef;
 }
 
+# removes leading/trailing spaces and (back)slashes completely
+# substitutes every non-ASCII-alphanumerical char with '_', except '_.-'
+sub normalize_content_filename {
+    my ($filename) = @_;
+
+    chomp $filename;
+    $filename =~ s/^.*[\/\\]//;
+    $filename =~ s/[^a-zA-Z0-9_.-]/_/g;
+
+    return $filename;
+}
+
 1;
-- 
2.30.2

[pve-devel] [PATCH v10 storage 3/3] status: add download_url method

[Lorenz Stechauner]

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

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

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

diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
index 11ad60f..5395ddd 100644
--- a/PVE/API2/Storage/Status.pm
+++ b/PVE/API2/Storage/Status.pm
@@ -222,6 +222,7 @@ __PACKAGE__->register_method ({
 
 	my $res = [
 	    { subdir => 'content' },
+	    { subdir => 'download-url' },
 	    { subdir => 'file-restore' },
 	    { subdir => 'prunebackups' },
 	    { subdir => 'rrd' },
@@ -494,4 +495,119 @@ __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',
+		enum => ['iso', 'vztmpl'],
+	    },
+	    filename => {
+		description => "The name of the file to create. Caution: This will be normalized!",
+		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 $rpcenv = PVE::RPCEnvironment::get();
+	my $user = $rpcenv->get_user();
+
+	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'};
+
+	die "storage '$storage' is not configured for content-type '$content'\n"
+	    if !$scfg->{content}->{$content};
+
+	my $filename = PVE::Storage::normalize_content_filename($param->{filename});
+
+	my $path;
+	if ($content eq 'iso') {
+	    if ($filename !~ m![^/]+$PVE::Storage::iso_extension_re$!) {
+		raise_param_exc({ filename => "wrong file extension" });
+	    }
+	    $path = PVE::Storage::get_iso_dir($cfg, $storage);
+	} elsif ($content eq 'vztmpl') {
+	    if ($filename !~ m![^/]+$PVE::Storage::vztmpl_extension_re$!) {
+		raise_param_exc({ filename => "wrong file extension" });
+	    }
+	    $path = PVE::Storage::get_vztmpl_dir($cfg, $storage);
+	} else {
+	    raise_param_exc({ content => "upload content-type '$content' is not allowed" });
+	}
+
+	PVE::Storage::activate_storage($cfg, $storage);
+	File::Path::make_path($path);
+
+	my $dccfg = PVE::Cluster::cfs_read_file('datacenter.cfg');
+	my $opts = {
+	    hash_required => 0,
+	    verify_certificates => $param->{'verify-certificates'} // 1,
+	    http_proxy => $dccfg->{http_proxy},
+	};
+
+	my ($checksum, $checksum_algorithm) = $param->@{'checksum', 'checksum-algorithm'};
+	if ($checksum) {
+	    $opts->{"${checksum_algorithm}sum"} = $checksum;
+	    $opts->{hash_required} = 1;
+	}
+
+	my $worker = sub {
+	    PVE::Tools::download_file_from_url("$path/$filename", $url, $opts);
+	};
+
+	return $rpcenv->fork_worker('download', $filename, $user, $worker);;
+    }});
+
 1;
-- 
2.30.2

Re: [pve-devel] [PATCH v10 storage 1/3] factoring out regex'es for backup and vztmpl

[Thomas Lamprecht]

On 22.06.21 11:19, Lorenz Stechauner wrote:
> uniformly stores these regex definitions in PVE::Storage.
> 
> One test had to be adapted because it tested obsolete code. Namely:
> it expects vztmpl to only end with .tar.gz, but the new regex also
> includes .tar.xz, there is nothing against allowing .tar.xz files as
> vztmpl files.
> 
> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
> ---
>  PVE/API2/Storage/Status.pm     |  6 +++---
>  PVE/Storage.pm                 | 11 ++++++++---
>  PVE/Storage/Plugin.pm          | 15 +++++++++------
>  test/path_to_volume_id_test.pm |  7 ++++---
>  4 files changed, 24 insertions(+), 15 deletions(-)
> 
> diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
> index 8a36aef..7069244 100644
> --- a/PVE/API2/Storage/Status.pm
> +++ b/PVE/API2/Storage/Status.pm
> @@ -423,12 +423,12 @@ __PACKAGE__->register_method ({
>  
>  	if ($content eq 'iso') {
>  	    if ($filename !~ m![^/]+$PVE::Storage::iso_extension_re$!) {
> -		raise_param_exc({ filename => "missing '.iso' or '.img' extension" });
> +		raise_param_exc({ filename => "wrong file 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" });
> +	    if ($filename !~ m![^/]+$PVE::Storage::vztmpl_extension_re$!) {
> +		raise_param_exc({ filename => "wrong file extension" });
>  	    }
>  	    $path = PVE::Storage::get_vztmpl_dir($cfg, $param->{storage});
>  	} else {
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index ea887a4..519431c 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -101,6 +101,11 @@ PVE::Storage::Plugin->init();
>  
>  our $iso_extension_re = qr/\.(?:iso|img)/i;
>  
> +our $vztmpl_extension_re = qr/\.tar\.([gx]z)/i;
> +
> +# Caution: because of the '$' inside, this regex may only used to match at the end of strings
> +our $backup_extension_re = qr/\.(tgz$|tar|vma)(?:\.(${\PVE::Storage::Plugin::COMPRESSOR_RE}))?/i;

should the $ be at the actual end of the regex, not just for .tgz? I mean, then below usage
would need to adapt and it would not be ideal, as it makes fixed choices for any user, but
at least not confusing. Otherwise, maybe even preferring that, I'd drop *any* $ anchor.

our $backup_extension_re = qr/\.(?:(?:(tar|vma)(?:\.(${\PVE::Storage::Plugin::COMPRESSOR_RE}))?|tgz))/i;

But it really needs to be either all $-anchored or none, a mix is just weird and confusing,
commented or not.

> +
>  #  PVE::Storage utility functions
>  
>  sub config {
> @@ -573,13 +578,13 @@ sub path_to_volume_id {
>  	} elsif ($path =~ m!^$isodir/([^/]+$iso_extension_re)$!) {
>  	    my $name = $1;
>  	    return ('iso', "$sid:iso/$name");
> -	} elsif ($path =~ m!^$tmpldir/([^/]+\.tar\.gz)$!) {
> +	} elsif ($path =~ m!^$tmpldir/([^/]+$vztmpl_extension_re)$!) {
>  	    my $name = $1;
>  	    return ('vztmpl', "$sid:vztmpl/$name");
>  	} elsif ($path =~ m!^$privatedir/(\d+)$!) {
>  	    my $vmid = $1;
>  	    return ('rootdir', "$sid:rootdir/$vmid");
> -	} elsif ($path =~ m!^$backupdir/([^/]+\.(?:tgz|(?:(?:tar|vma)(?:\.(?:${\PVE::Storage::Plugin::COMPRESSOR_RE}))?)))$!) {
> +	} elsif ($path =~ m!^$backupdir/([^/]+$backup_extension_re)$!) {
>  	    my $name = $1;
>  	    return ('backup', "$sid:backup/$name");
>  	} elsif ($path =~ m!^$snippetsdir/([^/]+)$!) {
> @@ -1463,7 +1468,7 @@ sub archive_info {
>      my $info;
>  
>      my $volid = basename($archive);
> -    if ($volid =~ /^(vzdump-(lxc|openvz|qemu)-.+\.(tgz$|tar|vma)(?:\.(${\PVE::Storage::Plugin::COMPRESSOR_RE}))?)$/) {
> +    if ($volid =~ /^(vzdump-(lxc|openvz|qemu)-.+$backup_extension_re)$/) {
>  	my $filename = "$1"; # untaint
>  	my ($type, $format, $comp) = ($2, $3, $4);
>  	my $format_re = defined($comp) ? "$format.$comp" : "$format";
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index d330845..afb05ed 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -518,11 +518,11 @@ sub parse_volname {
>  	return ('images', $name, $vmid, undef, undef, $isBase, $format);
>      } elsif ($volname =~ m!^iso/([^/]+$PVE::Storage::iso_extension_re)$!) {
>  	return ('iso', $1);
> -    } elsif ($volname =~ m!^vztmpl/([^/]+\.tar\.[gx]z)$!) {
> +    } elsif ($volname =~ m!^vztmpl/([^/]+$PVE::Storage::vztmpl_extension_re)$!) {
>  	return ('vztmpl', $1);
>      } elsif ($volname =~ m!^rootdir/(\d+)$!) {
>  	return ('rootdir', $1, $1);
> -    } elsif ($volname =~ m!^backup/([^/]+(?:\.(?:tgz|(?:(?:tar|vma)(?:\.(?:${\COMPRESSOR_RE}))?))))$!) {
> +    } elsif ($volname =~ m!^backup/([^/]+$PVE::Storage::backup_extension_re)$!) {
>  	my $fn = $1;
>  	if ($fn =~ m/^vzdump-(openvz|lxc|qemu)-(\d+)-.+/) {
>  	    return ('backup', $fn, $2);
> @@ -1041,15 +1041,18 @@ my $get_subdir_files = sub {
>  	    $info = { volid => "$sid:iso/$1", format => 'iso' };
>  
>  	} elsif ($tt eq 'vztmpl') {
> -	    next if $fn !~ m!/([^/]+\.tar\.([gx]z))$!;
> +	    next if $fn !~ m!/([^/]+$PVE::Storage::vztmpl_extension_re)$!;
>  
>  	    $info = { volid => "$sid:vztmpl/$1", format => "t$2" };
>  
>  	} elsif ($tt eq 'backup') {
> -	    next if $fn !~ m!/([^/]+\.(tgz|(?:(?:tar|vma)(?:\.(${\COMPRESSOR_RE}))?)))$!;
> +	    next if $fn !~ m!/([^/]+$PVE::Storage::backup_extension_re)$!;
> +
>  	    my $original = $fn;
> -	    my $format = $2;
> -	    $fn = $1;
> +	    my ($format, $comp);
> +
> +	    ($fn, $format, $comp) = ($1, $2, $3);
> +	    $format = defined($comp) ? "$format.$comp" : $format;
>  
>  	    # only match for VMID now, to avoid false positives (VMID in parent directory name)
>  	    next if defined($vmid) && $fn !~ m/\S+-$vmid-\S+/;
> diff --git a/test/path_to_volume_id_test.pm b/test/path_to_volume_id_test.pm
> index 5eee2f6..e0c46d9 100644
> --- a/test/path_to_volume_id_test.pm
> +++ b/test/path_to_volume_id_test.pm
> @@ -166,12 +166,13 @@ my @tests = (
>  	    'local:snippets/hookscript.pl',
>  	],
>      },
> -
> -    # no matches
>      {
>  	description => 'CT template, tar.xz',
>  	volname     => "$storage_dir/template/cache/debian-10.0-standard_10.0-1_amd64.tar.xz",
> -	expected    => [''],
> +	expected    => [
> +	    'vztmpl',
> +	    'local:vztmpl/debian-10.0-standard_10.0-1_amd64.tar.xz'
> +	],
>      },
>  
>      # no matches, path or files with failures
> 

[pve-devel] applied: [PATCH v10 storage 2/3] status: factoring out normalize_content_filename

[Thomas Lamprecht]

On 22.06.21 11:19, Lorenz Stechauner wrote:
> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
> ---
>  PVE/API2/Storage/Status.pm |  6 +-----
>  PVE/Storage.pm             | 12 ++++++++++++
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 
>

applied, thanks!

[pve-devel] applied: [PATCH v10 storage 3/3] status: add download_url method

[Thomas Lamprecht]

On 22.06.21 11:19, Lorenz Stechauner wrote:
> uses common function PVE::Tools::download_file_from_url to download
> iso files.
> 
> Only users with permissions `Sys.Audit` and `Sys.Modify` on `/` are
> permitted to perform this action. This restriction is due to the
> fact, that the download function is able to download files from
> internal networks (which are not visible/accessible from outside).
> Users with these permissions anyway have the means to alter node
> (network) config, so this does not create any further security risk.
> 
> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
> ---
>  PVE/API2/Storage/Status.pm | 116 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 116 insertions(+)
> 
> diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
> index 11ad60f..5395ddd 100644
> --- a/PVE/API2/Storage/Status.pm
> +++ b/PVE/API2/Storage/Status.pm
> @@ -222,6 +222,7 @@ __PACKAGE__->register_method ({
>  
>  	my $res = [
>  	    { subdir => 'content' },
> +	    { subdir => 'download-url' },
>  	    { subdir => 'file-restore' },
>  	    { subdir => 'prunebackups' },
>  	    { subdir => 'rrd' },
> @@ -494,4 +495,119 @@ __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',
> +		enum => ['iso', 'vztmpl'],
> +	    },
> +	    filename => {
> +		description => "The name of the file to create. Caution: This will be normalized!",

FYI: I added a maxLength to this with 255, quite some FS have that as limit
per file anyway (or had, it is generally quite long).

> +		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 $rpcenv = PVE::RPCEnvironment::get();
> +	my $user = $rpcenv->get_user();
> +
> +	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'};
> +
> +	die "storage '$storage' is not configured for content-type '$content'\n"
> +	    if !$scfg->{content}->{$content};
> +
> +	my $filename = PVE::Storage::normalize_content_filename($param->{filename});
> +
> +	my $path;
> +	if ($content eq 'iso') {
> +	    if ($filename !~ m![^/]+$PVE::Storage::iso_extension_re$!) {
> +		raise_param_exc({ filename => "wrong file extension" });
> +	    }
> +	    $path = PVE::Storage::get_iso_dir($cfg, $storage);
> +	} elsif ($content eq 'vztmpl') {
> +	    if ($filename !~ m![^/]+$PVE::Storage::vztmpl_extension_re$!) {
> +		raise_param_exc({ filename => "wrong file extension" });
> +	    }
> +	    $path = PVE::Storage::get_vztmpl_dir($cfg, $storage);
> +	} else {
> +	    raise_param_exc({ content => "upload content-type '$content' is not allowed" });
> +	}
> +
> +	PVE::Storage::activate_storage($cfg, $storage);
> +	File::Path::make_path($path);
> +
> +	my $dccfg = PVE::Cluster::cfs_read_file('datacenter.cfg');
> +	my $opts = {
> +	    hash_required => 0,
> +	    verify_certificates => $param->{'verify-certificates'} // 1,
> +	    http_proxy => $dccfg->{http_proxy},
> +	};
> +
> +	my ($checksum, $checksum_algorithm) = $param->@{'checksum', 'checksum-algorithm'};
> +	if ($checksum) {
> +	    $opts->{"${checksum_algorithm}sum"} = $checksum;
> +	    $opts->{hash_required} = 1;
> +	}
> +
> +	my $worker = sub {
> +	    PVE::Tools::download_file_from_url("$path/$filename", $url, $opts);
> +	};
> +
> +	return $rpcenv->fork_worker('download', $filename, $user, $worker);;

double semi-colons and the worker ID is still not made safe, i.e., colons are still
able to go through, FWICT, so I threw in a call to PVE::Tools::encode_text(), albeit
I could swear we had another helper for exactly this...

> +    }});
> +
>  1;
> 

[pve-devel] applied: [PATCH v10 manager 1/4] aplinfo: factoring out regex for vztmpl

[Thomas Lamprecht]

On 22.06.21 11:19, Lorenz Stechauner wrote:
> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
> ---
>  PVE/APLInfo.pm | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
>

applied, thanks!