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

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

[Lorenz Stechauner]

Another idea would be to introduce two new permissions:
Sys.RetrieveLocal - only local/private ip addresses allowed
Sys.RetrieveGlobal - all other ip addresses allowed (means only non-private)

> On 29.04.21 15:22 Thomas Lamprecht <t.lamprecht@proxmox.com> wrote:
> 
>  
> On 29.04.21 13:54, Dominik Csapak wrote:
> > On 4/28/21 16:13, Lorenz Stechauner wrote:
> >>   +__PACKAGE__->register_method({
> >> +    name => 'retrieve',
> >> +    path => '{storage}/retrieve',
> >> +    method => 'POST',
> >> +    description => "Download templates and ISO images by using an URL.",
> >> +    permissions => {
> >> +    check => ['perm', '/storage/{storage}', ['Datastore.AllocateTemplate']],
> >> +    },
> >> +    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',
> >> +        },
> > 
> > i am not quite sure if it is a good idea to have this feature
> > unrestricted for everybody who can download a template
> > 
> > it possibly gives access to an internal network to which
> > the users does not have access otherwise...
> > 
> > maybe we want to give the admin control over allow- and/or blocklists ?
> 
> I do not want such lists, PITA to manage for everybody.
> 
> Maybe we can just allow it only for users with Sys.Modify + Sys.Audit on / ?
> 
> We could also enforce that it needs to be a hostname (no IP) and/or resolve
> to something out of the priv. network ranges, at least if the aforementioned
> privs are not set.
> 
> Another idea would be enforcing the URL to match something like /\.(iso|img)$/ 
> and being not to informative on errors to avoid allowing to see which hsot are
> on/off line in a network. With that one could make this pretty safe I think.
> 
> 
> > 
> >> +        insecure => {
> >> +        description => "Allow TLS certificates to be invalid.",
> >> +        type => 'boolean',
> >> +        optional => 1,
> >> +        } > +    },
> >> +    },
> >> +    returns => {
> >> +    type => "object",
> >> +    properties => {
> >> +        filename => { type => 'string' },
> >> +        upid => { type => 'string' },
> >> +        size => {
> >> +        type => 'integer',
> >> +        renderer => 'bytes',
> >> +        },
> >> +    },
> >> +    },
> >> +    code => sub {
> >> +    my ($param) = @_;
> >> +
> >> +    my @hash_algs = ['md5', 'sha1', 'sha224', 'sha256', 'sha384', 'sha512'];
> > 
> > as written above, can be handled by api
> 
> and could be actually auto-detected too, at least optionally? All those are pretty
> much unique already in length, IIRC.

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

[Thomas Lamprecht]

On 29.04.21 15:46, Lorenz Stechauner wrote:
>> On 29.04.21 15:22 Thomas Lamprecht <t.lamprecht@proxmox.com> wrote:
>>> i am not quite sure if it is a good idea to have this feature
>>> unrestricted for everybody who can download a template
>>>
>>> it possibly gives access to an internal network to which
>>> the users does not have access otherwise...
>>>
>>> maybe we want to give the admin control over allow- and/or blocklists ?
>>
>> I do not want such lists, PITA to manage for everybody.
>>
>> Maybe we can just allow it only for users with Sys.Modify + Sys.Audit on / ?
>>
>> We could also enforce that it needs to be a hostname (no IP) and/or resolve
>> to something out of the priv. network ranges, at least if the aforementioned
>> privs are not set.
>>
>> Another idea would be enforcing the URL to match something like /\.(iso|img)$/ 
>> and being not to informative on errors to avoid allowing to see which hsot are
>> on/off line in a network. With that one could make this pretty safe I think.
>>
> Another idea would be to introduce two new permissions:
> Sys.RetrieveLocal - only local/private ip addresses allowed
> Sys.RetrieveGlobal - all other ip addresses allowed (means only non-private)

First, we try to avoid top-posting on the mailing list as it makes reading
replies unnecessarily hard without any real benefit. Interleaved style is
recommended.

IMO that is not a really good solution here due to:

* very specialized permission for a specific API call, makes the permission
  system crowded and confusing fast.

* Themself they have again a very broad reach, which would then lead to a
  situation where an admin needs to fully trust them again in the net as
  once enabled they have full access to everything.

I'd avoid adding new permissions for this, especially such overly specific
ones, rather:

* enforce the content/type or file name limits (must end with a valid type
  for the ISO or CT template type anyway, else it won't be returned by the
  content API)
* enforce existing Sys. poermissions if the hostname is a LAN address, public
  ones are not really problematic, anybody with allocate permissions can
  already upload them by indirection.
* suppress timeout vs. 404 distinction in error, possibly even adding a
  response delay so that they cannot be differed by timing either.

If that is deemed to complicated then the simple solution for now should be
to require Sys.Modify on / + Datastore.Allocate on the storage.

[pve-devel] [PATCH manager 1/1] 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.

Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
---
 www/manager6/Makefile                      |   1 +
 www/manager6/form/HashAlgorithmSelector.js |  21 +++
 www/manager6/storage/ContentView.js        | 161 +++++++++++++++++++++
 3 files changed, 183 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..4a72cc08
--- /dev/null
+++ b/www/manager6/form/HashAlgorithmSelector.js
@@ -0,0 +1,21 @@
+Ext.define('PVE.form.hashAlgorithmSelector', {
+    extend: 'Proxmox.form.KVComboBox',
+    alias: ['widget.pveHashAlgorithmSelector'],
+    comboItems: [],
+    hasNoneOption: false,
+    initComponent: function() {
+	var me = this;
+	me.comboItems = [
+	    ['md5', 'MD5'],
+	    ['sha1', 'SHA-1'],
+	    ['sha224', 'SHA-224'],
+	    ['sha256', 'SHA-256'],
+	    ['sha384', 'SHA-384'],
+	    ['sha512', 'SHA-512'],
+	];
+	if (me.hasNoneOption) {
+	    me.comboItems.unshift(['none', 'None']);
+	}
+	this.callParent();
+    },
+});
diff --git a/www/manager6/storage/ContentView.js b/www/manager6/storage/ContentView.js
index dd6df4b1..7187ebbe 100644
--- a/www/manager6/storage/ContentView.js
+++ b/www/manager6/storage/ContentView.js
@@ -191,6 +191,153 @@ Ext.define('PVE.storage.Upload', {
     },
 });
 
+Ext.define('PVE.storage.Retrieve', {
+    extend: 'Proxmox.window.Edit',
+    alias: 'widget.pveStorageRetrieve',
+
+    resizable: false,
+
+    modal: true,
+
+    isCreate: true,
+
+    showTaskViewer: true,
+    upidFieldName: 'upid',
+
+    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 defaultContent = me.contents[0] || '';
+
+	let urlField = Ext.create('Ext.form.field.Text', {
+	    name: 'url',
+	    allowBlank: false,
+	    fieldLabel: gettext('URL'),
+	});
+
+	let fileNameField = Ext.create('Ext.form.field.Text', {
+	    name: 'filename',
+	    allowBlank: false,
+	    fieldLabel: gettext('File name'),
+	});
+
+	let fileSizeField = Ext.create('Ext.form.field.Text', {
+	    name: 'size',
+	    disabled: true,
+	    fieldLabel: gettext('File size'),
+	    emptyText: gettext('unknown'),
+	});
+
+	let checksumField = Ext.create('Ext.form.field.Text', {
+	    name: 'checksum',
+	    fieldLabel: gettext('Checksum'),
+	    allowBlank: true,
+	    disabled: true,
+	    emptyText: gettext('none'),
+	});
+
+	let checksumAlgField = Ext.create('PVE.form.hashAlgorithmSelector', {
+	    name: 'checksumalg',
+	    fieldLabel: gettext('Hash algorithm'),
+	    allowBlank: true,
+	    hasNoneOption: true,
+	    value: 'none',
+	});
+
+	let inputPanel = Ext.create('Proxmox.panel.InputPanel', {
+	    method: 'POST',
+	    waitMsgTarget: true,
+	    border: false,
+	    columnT: [
+		urlField,
+		fileNameField,
+	    ],
+	    column1: [
+		{
+		    xtype: 'pveContentTypeSelector',
+		    cts: me.contents,
+		    fieldLabel: gettext('Content'),
+		    name: 'content',
+		    value: defaultContent,
+		    allowBlank: false,
+		},
+	    ],
+	    column2: [
+		fileSizeField,
+	    ],
+	    advancedColumn1: [
+		checksumField,
+		checksumAlgField,
+	    ],
+	    advancedColumn2: [
+		{
+		    xtype: 'checkbox',
+		    name: 'insecure',
+		    fieldLabel: gettext('Trust invalid certificates'),
+		    labelWidth: 150,
+		},
+	    ],
+	});
+
+	urlField.on('change', function() {
+	    urlField.setValidation("Waiting for response...");
+	    urlField.validate();
+	    fileSizeField.setValue("");
+	    Proxmox.Utils.API2Request({
+		url: me.url,
+		method: 'POST',
+		params: {
+		    metaonly: 1,
+		    url: me.getValues()['url'],
+		    content: me.getValues()['content'],
+		},
+		failure: function(res, opt) {
+		    urlField.setValidation(res.result.message);
+		    urlField.validate();
+		    fileSizeField.setValue("");
+		},
+		success: function(res, opt) {
+		    urlField.setValidation();
+		    urlField.validate();
+
+		    let data = res.result.data;
+		    fileNameField.setValue(data.filename);
+		    fileSizeField.setValue(Proxmox.Utils.format_size(data.size));
+		},
+	    });
+	});
+
+	checksumAlgField.on('change', function() {
+	    if (this.getValue() === 'none') {
+		checksumField.setDisabled(true);
+		checksumField.setValue("");
+		checksumField.allowBlank = true;
+	    } else {
+		checksumField.setDisabled(false);
+		checksumField.allowBlank = false;
+	    }
+	});
+
+	Ext.apply(me, {
+	    title: gettext('Retrieve from URL'),
+	    items: inputPanel,
+	    submitText: gettext('Download'),
+	});
+
+        me.callParent();
+    },
+});
+
 Ext.define('PVE.storage.ContentView', {
     extend: 'Ext.grid.GridPanel',
 
@@ -262,6 +409,19 @@ Ext.define('PVE.storage.ContentView', {
 	    },
 	});
 
+	let retrieveButton = Ext.create('Proxmox.button.Button', {
+	    text: gettext('Retrieve from URL'),
+	    handler: function() {
+		let win = Ext.create('PVE.storage.Retrieve', {
+		    nodename: nodename,
+		    storage: storage,
+		    contents: [content],
+		});
+		win.show();
+		win.on('destroy', reload);
+	    },
+	});
+
 	let removeButton = Ext.create('Proxmox.button.StdRemoveButton', {
 	    selModel: sm,
 	    delay: 5,
@@ -276,6 +436,7 @@ Ext.define('PVE.storage.ContentView', {
 	}
 	if (me.useUploadButton) {
 	    me.tbar.push(uploadButton);
+	    me.tbar.push(retrieveButton);
 	}
 	if (!me.useCustomRemoveButton) {
 	    me.tbar.push(removeButton);
-- 
2.20.1

[pve-devel] [PATCH storage 1/1] 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 | 244 +++++++++++++++++++++++++++++++++++++
 1 file changed, 244 insertions(+)

diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
index 897b4a7..3919be7 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;
@@ -497,4 +499,246 @@ __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.",
+    permissions => {
+	check => ['perm', '/storage/{storage}', ['Datastore.AllocateTemplate']],
+    },
+    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',
+		optional => 1,
+	    },
+	    checksum => {
+		description => "The expected checksum of the file.",
+		type => 'string',
+		optional => 1,
+	    },
+	    checksumalg => {
+		description => "The algorithm to claculate the checksum of the file.",
+		type => 'string',
+		optional => 1,
+	    },
+	    metaonly => {
+		description => "Only return the file name and size.",
+		type => 'boolean',
+		optional => 1,
+	    },
+	    insecure => {
+		description => "Allow TLS certificates to be invalid.",
+		type => 'boolean',
+		optional => 1,
+	    }
+	},
+    },
+    returns => {
+	type => "object",
+	properties => {
+	    filename => { type => 'string' },
+	    upid => { type => 'string' },
+	    size => {
+		type => 'integer',
+		renderer => 'bytes',
+	    },
+	},
+    },
+    code => sub {
+	my ($param) = @_;
+
+	my @hash_algs = ['md5', 'sha1', 'sha224', 'sha256', 'sha384', 'sha512'];
+
+	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};
+
+	$hash_alg = undef
+	    if $hash_alg eq 'none';
+
+	die "either 'checksum' and 'checksumalg' or none of these have to be specified"
+	    if ($checksum && !$hash_alg) || (!$checksum && $hash_alg);
+
+	die "unsupported checksumalg: '$hash_alg'"
+	    if $hash_alg && ($hash_alg !~ /^(md5|sha1|sha(224|256|384|512))$/);
+
+	my $req = HTTP::Request->new(HEAD => $url);
+	my $ua = LWP::UserAgent->new();
+	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 $filename_remote;
+
+	if ($dispo && $dispo =~ m/filename=(.+)/) {
+	    $filename_remote = $1;
+	} elsif ($url =~ m!^[^?]+/([^?/]*)(?:\?.*)?$!) {
+	    $filename_remote = $1;
+	}
+
+	chomp $filename_remote;
+	$filename_remote =~ s/^.*[\/\\]//;
+	$filename_remote =~ s/[^-a-zA-Z0-9_.]/_/g;
+
+	if ($param->{metaonly}) {
+	    return {
+		filename => $filename_remote,
+		upid => 0,
+		size => $size + 0,
+	    };
+	}
+
+	my $filename = $param->{filename} || $filename_remote;
+	chomp $filename;
+	$filename =~ s/^.*[\/\\]//;
+	$filename =~ s/[^-a-zA-Z0-9_.]/_/g;
+
+	my $path;
+
+	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";
+	my $dirname = dirname($dest);
+
+	my $cmd;
+	my $cmd_check;
+	my $cmd_check_flat;
+	my $cmd_del;
+
+	my @curlcmd = ('curl', '-L', '-o', $dest, '-f');
+	push @curlcmd, '--insecure'
+	    if $param->{insecure};
+
+	my @check1 = ('echo', $checksum, $dest);
+	my @check2 = ("${hash_alg}sum", '-c');
+
+	my @unlinkcmd = ('rm', '-f', $dest);
+
+	if ($node ne 'localhost' && $node ne PVE::INotify::nodename()) {
+	    my $remip = PVE::Cluster::remote_node_ip($node);
+
+	    my @ssh_options = ('-o', 'BatchMode=yes');
+
+	    my @remcmd = ('/usr/bin/ssh', @ssh_options, $remip, '--');
+
+	    eval {
+		# activate remote storage
+		PVE::Tools::run_command([@remcmd, '/usr/sbin/pvesm', 'status', '--storage', $param->{storage}]);
+	    };
+	    die "can't activate storage '$param->{storage}' on node '$node': $@"
+		if $@;
+
+	    PVE::Tools::run_command([@remcmd, '/bin/mkdir', '-p', '--', $dirname],
+				    errmsg => "mkdir failed");
+
+	    $cmd = [@remcmd, @curlcmd, PVE::Tools::shell_quote($url)];
+
+	    $cmd_check = [@remcmd, @check1, '|', @check2];
+	    $cmd_check_flat = [@remcmd, @check1, '|', @check2];
+
+	    $cmd_del = [@remcmd, @unlinkcmd];
+	} else {
+	    PVE::Storage::activate_storage($cfg, $param->{storage});
+	    File::Path::make_path($dirname);
+
+	    $cmd = [@curlcmd, $url];
+
+	    $cmd_check = [[@check1], [@check2]];
+	    $cmd_check_flat = [@check1, '|', @check2];
+
+	    $cmd_del = [@unlinkcmd];
+	}
+
+	my $worker = sub {
+	    my $upid = shift;
+
+	    print "starting file download from: $url\n";
+	    print "target node: $node\n";
+	    print "target file: $dest\n";
+	    print "file size is: $size\n";
+	    print "command: " . join(' ', @$cmd) . "\n";
+
+	    eval { PVE::Tools::run_command($cmd, errmsg => 'download failed'); };
+	    if (my $err = $@) {
+		PVE::Tools::run_command($cmd_del);
+		die $err;
+	    }
+	    print "finished file download successfully\n";
+
+	    if ($checksum) {
+		print "validating checksum...\n";
+		print "expected $hash_alg checksum is: $checksum\n";
+		print "checksum validation command: " . join(' ', @$cmd_check_flat) . "\n";
+
+		eval { PVE::Tools::run_command($cmd_check, errmsg => 'checksum mismatch'); };
+		if (my $err = $@) {
+		    PVE::Tools::run_command($cmd_del);
+		    die $err;
+		}
+		print "validated checksum successfully\n";
+	    }
+	};
+
+	my $upid = $rpcenv->fork_worker('imgdownload', undef, $user, $worker);
+
+	return {
+	    filename => $filename,
+	    upid => $upid,
+	    size => $size + 0,
+	};
+    }});
+
+
 1;
-- 
2.20.1

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

[Dominik Csapak]

thanks for the patch, a few comments inline

On 4/28/21 16:13, 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 | 244 +++++++++++++++++++++++++++++++++++++
>   1 file changed, 244 insertions(+)
> 
> diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
> index 897b4a7..3919be7 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;
> @@ -497,4 +499,246 @@ __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.",
> +    permissions => {
> +	check => ['perm', '/storage/{storage}', ['Datastore.AllocateTemplate']],
> +    },
> +    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',
> +	    },

i am not quite sure if it is a good idea to have this feature
unrestricted for everybody who can download a template

it possibly gives access to an internal network to which
the users does not have access otherwise...

maybe we want to give the admin control over allow- and/or blocklists ?


> +	    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',
> +		optional => 1,
> +	    },
> +	    checksum => {
> +		description => "The expected checksum of the file.",
> +		type => 'string',
> +		optional => 1,
> +	    },
> +	    checksumalg => {
> +		description => "The algorithm to claculate the checksum of the file.",
> +		type => 'string',
> +		optional => 1,
> +	    },

we can define enums in the api like so:

type => 'string',
enum => ['val1', 'val2'],

also we can define dependencies here with
requires => 'checksumalg'
for checksum

> +	    metaonly => {
> +		description => "Only return the file name and size.",
> +		type => 'boolean',
> +		optional => 1,
> +	    },

generally i do not like this design of the api call that does two things
i'd prefer to have 2 api calls:

query_url (or smthg like this), checks the url for filename/size
retrieve, actually downloads the file

> +	    insecure => {
> +		description => "Allow TLS certificates to be invalid.",
> +		type => 'boolean',
> +		optional => 1,
> +	    } > +	},
> +    },
> +    returns => {
> +	type => "object",
> +	properties => {
> +	    filename => { type => 'string' },
> +	    upid => { type => 'string' },
> +	    size => {
> +		type => 'integer',
> +		renderer => 'bytes',
> +	    },
> +	},
> +    },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my @hash_algs = ['md5', 'sha1', 'sha224', 'sha256', 'sha384', 'sha512'];

as written above, can be handled by api

> +
> +	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};
> +
> +	$hash_alg = undef
> +	    if $hash_alg eq 'none';
> +
> +	die "either 'checksum' and 'checksumalg' or none of these have to be specified"
> +	    if ($checksum && !$hash_alg) || (!$checksum && $hash_alg);\

as written above, can also be handled by api

> +
> +	die "unsupported checksumalg: '$hash_alg'"
> +	    if $hash_alg && ($hash_alg !~ /^(md5|sha1|sha(224|256|384|512))$/);

same

> +
> +	my $req = HTTP::Request->new(HEAD => $url);
> +	my $ua = LWP::UserAgent->new();
> +	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 $filename_remote;
> +
> +	if ($dispo && $dispo =~ m/filename=(.+)/) {
> +	    $filename_remote = $1;
> +	} elsif ($url =~ m!^[^?]+/([^?/]*)(?:\?.*)?$!) {
> +	    $filename_remote = $1;
> +	}
> +
> +	chomp $filename_remote;
> +	$filename_remote =~ s/^.*[\/\\]//;
> +	$filename_remote =~ s/[^-a-zA-Z0-9_.]/_/g;

since we also use this in 'upload' and below again,
it may be nice to put that in a function and reuse
it (so they cannot diverge)

> +
> +	if ($param->{metaonly}) {
> +	    return {
> +		filename => $filename_remote,
> +		upid => 0,
> +		size => $size + 0,
> +	    };
> +	}
> +
> +	my $filename = $param->{filename} || $filename_remote;

please use '//' here, since this also triggers if my filename is "0"

> +	chomp $filename;
> +	$filename =~ s/^.*[\/\\]//;
> +	$filename =~ s/[^-a-zA-Z0-9_.]/_/g;
> +
> +	my $path;
> +
> +	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" });
> +	}

i'd put this check (the 'not allowed' one) above, before doing any request

> +
> +	die "storage '$param->{storage}' does not support '$content' content"
> +	    if !$scfg->{content}->{$content};

as well

> +
> +	my $dest = "$path/$filename";
> +	my $dirname = dirname($dest);

should that not be simply '$path' ?
or do we allow subdirectories?
(afair, those would not appear in the content of the storage)

> +
> +	my $cmd;
> +	my $cmd_check;
> +	my $cmd_check_flat;
> +	my $cmd_del;
> +
> +	my @curlcmd = ('curl', '-L', '-o', $dest, '-f');
> +	push @curlcmd, '--insecure'
> +	    if $param->{insecure};
> +
> +	my @check1 = ('echo', $checksum, $dest);
> +	my @check2 = ("${hash_alg}sum", '-c');
> +
> +	my @unlinkcmd = ('rm', '-f', $dest);
> +
> +	if ($node ne 'localhost' && $node ne PVE::INotify::nodename()) {

this should not be necessary here, we can define

proxyto => '{node}',
in the api definition, then
the pveproxy automatically proxies the request to the correct node and
it will be executed thhere

> +	    my $remip = PVE::Cluster::remote_node_ip($node);
> +
> +	    my @ssh_options = ('-o', 'BatchMode=yes');
> +
> +	    my @remcmd = ('/usr/bin/ssh', @ssh_options, $remip, '--');
> +
> +	    eval {
> +		# activate remote storage
> +		PVE::Tools::run_command([@remcmd, '/usr/sbin/pvesm', 'status', '--storage', $param->{storage}]);
> +	    };
> +	    die "can't activate storage '$param->{storage}' on node '$node': $@"
> +		if $@;
> +
> +	    PVE::Tools::run_command([@remcmd, '/bin/mkdir', '-p', '--', $dirname],
> +				    errmsg => "mkdir failed");
> +
> +	    $cmd = [@remcmd, @curlcmd, PVE::Tools::shell_quote($url)];
> +
> +	    $cmd_check = [@remcmd, @check1, '|', @check2];
> +	    $cmd_check_flat = [@remcmd, @check1, '|', @check2];
> +
> +	    $cmd_del = [@remcmd, @unlinkcmd];
> +	} else {
> +	    PVE::Storage::activate_storage($cfg, $param->{storage});
> +	    File::Path::make_path($dirname);
> +
> +	    $cmd = [@curlcmd, $url];
> +
> +	    $cmd_check = [[@check1], [@check2]];
> +	    $cmd_check_flat = [@check1, '|', @check2];
> +
> +	    $cmd_del = [@unlinkcmd];
> +	}
> +
> +	my $worker = sub {
> +	    my $upid = shift;
> +
> +	    print "starting file download from: $url\n";
> +	    print "target node: $node\n";
> +	    print "target file: $dest\n";
> +	    print "file size is: $size\n";
> +	    print "command: " . join(' ', @$cmd) . "\n";
> +
> +	    eval { PVE::Tools::run_command($cmd, errmsg => 'download failed'); };
> +	    if (my $err = $@) {
> +		PVE::Tools::run_command($cmd_del);
> +		die $err;
> +	    }
> +	    print "finished file download successfully\n";
> +
> +	    if ($checksum) {
> +		print "validating checksum...\n";
> +		print "expected $hash_alg checksum is: $checksum\n";
> +		print "checksum validation command: " . join(' ', @$cmd_check_flat) . "\n";
> +
> +		eval { PVE::Tools::run_command($cmd_check, errmsg => 'checksum mismatch'); };
> +		if (my $err = $@) {
> +		    PVE::Tools::run_command($cmd_del);
> +		    die $err;
> +		}
> +		print "validated checksum successfully\n";
> +	    }
> +	};
> +
> +	my $upid = $rpcenv->fork_worker('imgdownload', undef, $user, $worker);
> +
> +	return {
> +	    filename => $filename,
> +	    upid => $upid,
> +	    size => $size + 0,
> +	};
> +    }});
> +
> +
>   1;
> 

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

[Thomas Lamprecht]

On 29.04.21 13:54, Dominik Csapak wrote:
> On 4/28/21 16:13, Lorenz Stechauner wrote:
>>   +__PACKAGE__->register_method({
>> +    name => 'retrieve',
>> +    path => '{storage}/retrieve',
>> +    method => 'POST',
>> +    description => "Download templates and ISO images by using an URL.",
>> +    permissions => {
>> +    check => ['perm', '/storage/{storage}', ['Datastore.AllocateTemplate']],
>> +    },
>> +    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',
>> +        },
> 
> i am not quite sure if it is a good idea to have this feature
> unrestricted for everybody who can download a template
> 
> it possibly gives access to an internal network to which
> the users does not have access otherwise...
> 
> maybe we want to give the admin control over allow- and/or blocklists ?

I do not want such lists, PITA to manage for everybody.

Maybe we can just allow it only for users with Sys.Modify + Sys.Audit on / ?

We could also enforce that it needs to be a hostname (no IP) and/or resolve
to something out of the priv. network ranges, at least if the aforementioned
privs are not set.

Another idea would be enforcing the URL to match something like /\.(iso|img)$/ 
and being not to informative on errors to avoid allowing to see which hsot are
on/off line in a network. With that one could make this pretty safe I think.


> 
>> +        insecure => {
>> +        description => "Allow TLS certificates to be invalid.",
>> +        type => 'boolean',
>> +        optional => 1,
>> +        } > +    },
>> +    },
>> +    returns => {
>> +    type => "object",
>> +    properties => {
>> +        filename => { type => 'string' },
>> +        upid => { type => 'string' },
>> +        size => {
>> +        type => 'integer',
>> +        renderer => 'bytes',
>> +        },
>> +    },
>> +    },
>> +    code => sub {
>> +    my ($param) = @_;
>> +
>> +    my @hash_algs = ['md5', 'sha1', 'sha224', 'sha256', 'sha384', 'sha512'];
> 
> as written above, can be handled by api

and could be actually auto-detected too, at least optionally? All those are pretty
much unique already in length, IIRC.

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

[Dominik Csapak]

On 4/29/21 15:22, Thomas Lamprecht wrote:
> On 29.04.21 13:54, Dominik Csapak wrote:
>> On 4/28/21 16:13, Lorenz Stechauner wrote:
>>>    +__PACKAGE__->register_method({
>>> +    name => 'retrieve',
>>> +    path => '{storage}/retrieve',
>>> +    method => 'POST',
>>> +    description => "Download templates and ISO images by using an URL.",
>>> +    permissions => {
>>> +    check => ['perm', '/storage/{storage}', ['Datastore.AllocateTemplate']],
>>> +    },
>>> +    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',
>>> +        },
>>
>> i am not quite sure if it is a good idea to have this feature
>> unrestricted for everybody who can download a template
>>
>> it possibly gives access to an internal network to which
>> the users does not have access otherwise...
>>
>> maybe we want to give the admin control over allow- and/or blocklists ?
> 
> I do not want such lists, PITA to manage for everybody.

understandable, was just the first thing that came to my mind

> 
> Maybe we can just allow it only for users with Sys.Modify + Sys.Audit on / ?
> 
> We could also enforce that it needs to be a hostname (no IP) and/or resolve
> to something out of the priv. network ranges, at least if the aforementioned
> privs are not set.

yes, sounds good, but then we have to disallow redirects

> 
> Another idea would be enforcing the URL to match something like /\.(iso|img)$/
> and being not to informative on errors to avoid allowing to see which hsot are
> on/off line in a network. With that one could make this pretty safe I think.

mhmm.. could work, but then we'd have to use a fixed timeout
(like on authentication) to avoid timing based probes

> 
> 
>>
>>> +        insecure => {
>>> +        description => "Allow TLS certificates to be invalid.",
>>> +        type => 'boolean',
>>> +        optional => 1,
>>> +        } > +    },
>>> +    },
>>> +    returns => {
>>> +    type => "object",
>>> +    properties => {
>>> +        filename => { type => 'string' },
>>> +        upid => { type => 'string' },
>>> +        size => {
>>> +        type => 'integer',
>>> +        renderer => 'bytes',
>>> +        },
>>> +    },
>>> +    },
>>> +    code => sub {
>>> +    my ($param) = @_;
>>> +
>>> +    my @hash_algs = ['md5', 'sha1', 'sha224', 'sha256', 'sha384', 'sha512'];
>>
>> as written above, can be handled by api
> 
> and could be actually auto-detected too, at least optionally? All those are pretty
> much unique already in length, IIRC.
> 

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

[Thomas Lamprecht]

On 29.04.21 16:01, Dominik Csapak wrote:
> On 4/29/21 15:22, Thomas Lamprecht wrote:
>> Maybe we can just allow it only for users with Sys.Modify + Sys.Audit on / ?
>>
>> We could also enforce that it needs to be a hostname (no IP) and/or resolve
>> to something out of the priv. network ranges, at least if the aforementioned
>> privs are not set.
> 
> yes, sounds good, but then we have to disallow redirects
> 

true, or at least resolve them manually ourself.

>>
>> Another idea would be enforcing the URL to match something like /\.(iso|img)$/
>> and being not to informative on errors to avoid allowing to see which hsot are
>> on/off line in a network. With that one could make this pretty safe I think.
> 
> mhmm.. could work, but then we'd have to use a fixed timeout
> (like on authentication) to avoid timing based probes

had that in mind too, actually replied that to lorenz just now ^^