[pve-devel] [PATCH-SERIES storage/manager/container/qemu-server] improve check_volume_access

[pve-devel] [PATCH-SERIES storage/manager/container/qemu-server] improve check_volume_access

[Fabian Ebner]

The first patch is in preparation for the import-from API, allowing
users with VM.Config.Disk to list images of their VMs.

The rest of the series introduces a content type parameter to
check_volume_access() for future-proofing.


Dependency bumps for storage are needed for the parameter to actually
have an effect.


storage:

Fabian Ebner (4):
  check volume access: allow if user has VM.Config.Disk
  check volume accesss: add content type parameter
  pvesm: extract config: add content type check
  api: file restore: use check_volume_access to restrict content type

 PVE/API2/Storage/FileRestore.pm | 12 ++++--------
 PVE/CLI/pvesm.pm                |  9 ++++++++-
 PVE/Storage.pm                  |  9 ++++++++-
 3 files changed, 20 insertions(+), 10 deletions(-)


manager:

Fabian Ebner (2):
  pveam: remove: add content type check
  api: vzdump: extract config: add content type check

 PVE/API2/VZDump.pm | 9 ++++++++-
 PVE/CLI/pveam.pm   | 2 +-
 2 files changed, 9 insertions(+), 2 deletions(-)


container:

Fabian Ebner (1):
  api: create/modify: add content type checks

 src/PVE/API2/LXC.pm | 10 +++++++++-
 src/PVE/LXC.pm      |  9 ++++++++-
 2 files changed, 17 insertions(+), 2 deletions(-)


qemu-server

Fabian Ebner (1):
  api: create/modify: add content type checks

 PVE/API2/Qemu.pm | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH storage 1/4] check volume access: allow if user has VM.Config.Disk

[Fabian Ebner]

Listing guest images should not require Datastore.Allocate in this
case. In preparation for adding disk import to the GUI.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/Storage.pm | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index 6112991..efa304a 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -486,6 +486,8 @@ sub check_volume_access {
 	} elsif ($vtype eq 'backup' && $ownervm) {
 	    $rpcenv->check($user, "/storage/$sid", ['Datastore.AllocateSpace']);
 	    $rpcenv->check($user, "/vms/$ownervm", ['VM.Backup']);
+	} elsif (($vtype eq 'images' || $vtype eq 'rootdir') && $ownervm) {
+	    $rpcenv->check($user, "/vms/$ownervm", ['VM.Config.Disk']);
 	} else {
 	    # allow if we are Datastore administrator
 	    $rpcenv->check($user, "/storage/$sid", ['Datastore.Allocate']);
-- 
2.30.2

[pve-devel] [PATCH storage 2/4] check volume accesss: add content type parameter

[Fabian Ebner]

Adding such a check here avoids the need to parse at the call sites in
many cases.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/Storage.pm | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index efa304a..83760c4 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -473,11 +473,16 @@ sub parse_volume_id {
 
 # test if we have read access to volid
 sub check_volume_access {
-    my ($rpcenv, $user, $cfg, $vmid, $volid) = @_;
+    my ($rpcenv, $user, $cfg, $vmid, $volid, $type) = @_;
 
     my ($sid, $volname) = parse_volume_id($volid, 1);
     if ($sid) {
 	my ($vtype, undef, $ownervm) = parse_volname($cfg, $volid);
+
+	# Need to allow 'images' when expecting 'rootdir' too - not cleanly separated in plugins.
+	die "unable to use volume $volid - content type needs to be '$type'\n"
+	    if defined($type) && $vtype ne $type && ($type ne 'rootdir' || $vtype ne 'images');
+
 	if ($vtype eq 'iso' || $vtype eq 'vztmpl') {
 	    # require at least read access to storage, (custom) templates/ISOs could be sensitive
 	    $rpcenv->check_any($user, "/storage/$sid", ['Datastore.AllocateSpace', 'Datastore.Audit']);
-- 
2.30.2

[pve-devel] [PATCH storage 3/4] pvesm: extract config: add content type check

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/CLI/pvesm.pm | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/PVE/CLI/pvesm.pm b/PVE/CLI/pvesm.pm
index 190de91..44d15fd 100755
--- a/PVE/CLI/pvesm.pm
+++ b/PVE/CLI/pvesm.pm
@@ -170,7 +170,14 @@ __PACKAGE__->register_method ({
 	my $authuser = $rpcenv->get_user();
 
 	my $storage_cfg = PVE::Storage::config();
-	PVE::Storage::check_volume_access($rpcenv, $authuser, $storage_cfg, undef, $volume);
+	PVE::Storage::check_volume_access(
+	    $rpcenv,
+	    $authuser,
+	    $storage_cfg,
+	    undef,
+	    $volume,
+	    'backup',
+	);
 
 	my $config_raw = PVE::Storage::extract_vzdump_config($storage_cfg, $volume);
 
-- 
2.30.2

[pve-devel] [PATCH storage 4/4] api: file restore: use check_volume_access to restrict content type

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/API2/Storage/FileRestore.pm | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/PVE/API2/Storage/FileRestore.pm b/PVE/API2/Storage/FileRestore.pm
index a4bad44..ccc56e5 100644
--- a/PVE/API2/Storage/FileRestore.pm
+++ b/PVE/API2/Storage/FileRestore.pm
@@ -111,14 +111,12 @@ __PACKAGE__->register_method ({
 	my $cfg = PVE::Storage::config();
 	my $scfg = PVE::Storage::storage_config($cfg, $storeid);
 
-	PVE::Storage::check_volume_access($rpcenv, $user, $cfg, undef, $volid);
+	PVE::Storage::check_volume_access($rpcenv, $user, $cfg, undef, $volid, 'backup');
 
 	raise_param_exc({'storage' => "Only PBS storages supported for file-restore."})
 	    if $scfg->{type} ne 'pbs';
 
-	my ($vtype, $snap) = PVE::Storage::parse_volname($cfg, $volid);
-	raise_param_exc({'volume' => 'Not a backup archive.'})
-	    if $vtype ne 'backup';
+	my (undef, $snap) = PVE::Storage::parse_volname($cfg, $volid);
 
 	my $client = PVE::PBSClient->new($scfg, $storeid);
 	my $ret = $client->file_restore_list($snap, $path, $base64);
@@ -177,14 +175,12 @@ __PACKAGE__->register_method ({
 	my $cfg = PVE::Storage::config();
 	my $scfg = PVE::Storage::storage_config($cfg, $storeid);
 
-	PVE::Storage::check_volume_access($rpcenv, $user, $cfg, undef, $volid);
+	PVE::Storage::check_volume_access($rpcenv, $user, $cfg, undef, $volid, 'backup');
 
 	raise_param_exc({'storage' => "Only PBS storages supported for file-restore."})
 	    if $scfg->{type} ne 'pbs';
 
-	my ($vtype, $snap) = PVE::Storage::parse_volname($cfg, $volid);
-	raise_param_exc({'volume' => 'Not a backup archive.'})
-	    if $vtype ne 'backup';
+	my (undef, $snap) = PVE::Storage::parse_volname($cfg, $volid);
 
 	my $client = PVE::PBSClient->new($scfg, $storeid);
 	my $fifo = $client->file_restore_extract_prepare();
-- 
2.30.2

[pve-devel] [PATCH manager 1/2] pveam: remove: add content type check

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/CLI/pveam.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/PVE/CLI/pveam.pm b/PVE/CLI/pveam.pm
index 6c26f209..67a912bd 100644
--- a/PVE/CLI/pveam.pm
+++ b/PVE/CLI/pveam.pm
@@ -170,7 +170,7 @@ __PACKAGE__->register_method ({
 
 	my $cfg = PVE::Storage::config();
 
-	PVE::Storage::check_volume_access($rpcenv, $authuser, $cfg, undef, $template);
+	PVE::Storage::check_volume_access($rpcenv, $authuser, $cfg, undef, $template, 'vztmpl');
 
 	my $abs_path = PVE::Storage::abs_filesystem_path($cfg, $template);
 
-- 
2.30.2

[pve-devel] [PATCH manager 2/2] api: vzdump: extract config: add content type check

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/API2/VZDump.pm | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/VZDump.pm b/PVE/API2/VZDump.pm
index 2c0df4c3..1adc169a 100644
--- a/PVE/API2/VZDump.pm
+++ b/PVE/API2/VZDump.pm
@@ -267,7 +267,14 @@ __PACKAGE__->register_method ({
 	my $authuser = $rpcenv->get_user();
 
 	my $storage_cfg = PVE::Storage::config();
-	PVE::Storage::check_volume_access($rpcenv, $authuser, $storage_cfg, undef, $volume);
+	PVE::Storage::check_volume_access(
+	    $rpcenv,
+	    $authuser,
+	    $storage_cfg,
+	    undef,
+	    $volume,
+	    'backup',
+	);
 
 	return PVE::Storage::extract_vzdump_config($storage_cfg, $volume);
     }});
-- 
2.30.2

[pve-devel] [PATCH container 1/1] api: create/modify: add content type checks

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

For root@pam, the check is skipped in check_ct_modify_config_perm()
(everything is), but I didn't want to refactor the whole function
just for this...

 src/PVE/API2/LXC.pm | 10 +++++++++-
 src/PVE/LXC.pm      |  9 ++++++++-
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 84712f7..ea4827f 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -283,7 +283,15 @@ __PACKAGE__->register_method({
 	    $archive = '-';
 	    die "restore from pipe requires rootfs parameter\n" if !defined($param->{rootfs});
 	} else {
-	    PVE::Storage::check_volume_access($rpcenv, $authuser, $storage_cfg, $vmid, $ostemplate);
+	    my $content_type = $restore ? 'backup' : 'vztmpl';
+	    PVE::Storage::check_volume_access(
+		$rpcenv,
+		$authuser,
+		$storage_cfg,
+		$vmid,
+		$ostemplate,
+		$content_type,
+	    );
 	    $archive = $ostemplate;
 	}
 
diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index b07d986..fe63087 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -1272,7 +1272,14 @@ sub check_ct_modify_config_perm {
 		my $sid = $1;
 		$rpcenv->check($authuser, "/storage/$sid", ['Datastore.AllocateSpace']);
 	    } else {
-		PVE::Storage::check_volume_access($rpcenv, $authuser, $storage_cfg, $vmid, $volid);
+		PVE::Storage::check_volume_access(
+		    $rpcenv,
+		    $authuser,
+		    $storage_cfg,
+		    $vmid,
+		    $volid,
+		    'rootdir',
+		);
 	    }
 	} elsif ($opt eq 'memory' || $opt eq 'swap') {
 	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.Memory']);
-- 
2.30.2

[pve-devel] [PATCH qemu-server 1/1] api: create/modify: add content type checks

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/API2/Qemu.pm | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index cb6973f1..1dd0cf28 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -104,7 +104,14 @@ my $check_storage_access = sub {
 	    raise_param_exc({ storage => "storage '$storeid' does not support vm images"})
 		if !$scfg->{content}->{images};
 	} else {
-	    PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $volid);
+	    PVE::Storage::check_volume_access(
+		$rpcenv,
+		$authuser,
+		$storecfg,
+		$vmid,
+		$volid,
+		'images',
+	    );
 	}
     });
 
@@ -230,7 +237,14 @@ my $create_disks = sub {
 	    delete $disk->{format}; # no longer needed
 	    $res->{$ds} = PVE::QemuServer::print_drive($disk);
 	} else {
-	    PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $volid);
+	    PVE::Storage::check_volume_access(
+		$rpcenv,
+		$authuser,
+		$storecfg,
+		$vmid,
+		$volid,
+		'images',
+	    );
 
 	    PVE::Storage::activate_volumes($storecfg, [ $volid ]) if $storeid;
 
@@ -645,7 +659,14 @@ __PACKAGE__->register_method({
 		die "pipe requires cli environment\n" if $rpcenv->{type} ne 'cli';
 		$archive = { type => 'pipe' };
 	    } else {
-		PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $archive);
+		PVE::Storage::check_volume_access(
+		    $rpcenv,
+		    $authuser,
+		    $storecfg,
+		    $vmid,
+		    $archive,
+		    'backup',
+		);
 
 		$archive = $parse_restore_archive->($storecfg, $archive);
 	    }
-- 
2.30.2

Re: [pve-devel] [PATCH storage 1/4] check volume access: allow if user has VM.Config.Disk

[Fabian Ebner]

Am 21.03.22 um 14:06 schrieb Fabian Ebner:
> Listing guest images should not require Datastore.Allocate in this
> case. In preparation for adding disk import to the GUI.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/Storage.pm | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index 6112991..efa304a 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -486,6 +486,8 @@ sub check_volume_access {
>  	} elsif ($vtype eq 'backup' && $ownervm) {
>  	    $rpcenv->check($user, "/storage/$sid", ['Datastore.AllocateSpace']);
>  	    $rpcenv->check($user, "/vms/$ownervm", ['VM.Backup']);
> +	} elsif (($vtype eq 'images' || $vtype eq 'rootdir') && $ownervm) {
> +	    $rpcenv->check($user, "/vms/$ownervm", ['VM.Config.Disk']);

Of course this needs to be or-ed with the Datastore.Allocate privilege.
Will fix it in v2.

>  	} else {
>  	    # allow if we are Datastore administrator
>  	    $rpcenv->check($user, "/storage/$sid", ['Datastore.Allocate']);

Re: [pve-devel] [PATCH storage 1/4] check volume access: allow if user has VM.Config.Disk

[Fabian Ebner]

Am 22.03.22 um 09:31 schrieb Fabian Ebner:
> Am 21.03.22 um 14:06 schrieb Fabian Ebner:
>> Listing guest images should not require Datastore.Allocate in this
>> case. In preparation for adding disk import to the GUI.
>>
>> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
>> ---
>>  PVE/Storage.pm | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
>> index 6112991..efa304a 100755
>> --- a/PVE/Storage.pm
>> +++ b/PVE/Storage.pm
>> @@ -486,6 +486,8 @@ sub check_volume_access {
>>  	} elsif ($vtype eq 'backup' && $ownervm) {
>>  	    $rpcenv->check($user, "/storage/$sid", ['Datastore.AllocateSpace']);
>>  	    $rpcenv->check($user, "/vms/$ownervm", ['VM.Backup']);

@Fabian G. should access to backups also be allowed if the user /just/
has Datastore.Allocate?

Otherwise, backups cannot be listed or removed (there is a separate
check, but works similarly) and attributes cannot be changed by a
supposedly high-privileged user.

On the other hand, we also use this check for extractconfig, where it
makes sense to be limited to users with VM.Backup, but could be changed
at the call site of course.

>> +	} elsif (($vtype eq 'images' || $vtype eq 'rootdir') && $ownervm) {
>> +	    $rpcenv->check($user, "/vms/$ownervm", ['VM.Config.Disk']);
> 
> Of course this needs to be or-ed with the Datastore.Allocate privilege.
> Will fix it in v2.
> 
>>  	} else {
>>  	    # allow if we are Datastore administrator
>>  	    $rpcenv->check($user, "/storage/$sid", ['Datastore.Allocate']);
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 

Re: [pve-devel] [PATCH storage 1/4] check volume access: allow if user has VM.Config.Disk

[Fabian Grünbichler]

On March 22, 2022 10:31 am, Fabian Ebner wrote:
> Am 22.03.22 um 09:31 schrieb Fabian Ebner:
>> Am 21.03.22 um 14:06 schrieb Fabian Ebner:
>>> Listing guest images should not require Datastore.Allocate in this
>>> case. In preparation for adding disk import to the GUI.
>>>
>>> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
>>> ---
>>>  PVE/Storage.pm | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
>>> index 6112991..efa304a 100755
>>> --- a/PVE/Storage.pm
>>> +++ b/PVE/Storage.pm
>>> @@ -486,6 +486,8 @@ sub check_volume_access {
>>>  	} elsif ($vtype eq 'backup' && $ownervm) {
>>>  	    $rpcenv->check($user, "/storage/$sid", ['Datastore.AllocateSpace']);
>>>  	    $rpcenv->check($user, "/vms/$ownervm", ['VM.Backup']);
> 
> @Fabian G. should access to backups also be allowed if the user /just/
> has Datastore.Allocate?
> 
> Otherwise, backups cannot be listed or removed (there is a separate
> check, but works similarly) and attributes cannot be changed by a
> supposedly high-privileged user.

yeah, I think Datastore.Allocate could be allowed here, it's documented 
as:

Datastore.Allocate: create/modify/remove a datastore and delete volumes

but in practice, any user that has Datastore.Allocate likely also has 
Datastore.AllocateSpace anyway which is probably why nobody complained 
yet ;)

> On the other hand, we also use this check for extractconfig, where it
> makes sense to be limited to users with VM.Backup, but could be changed
> at the call site of course.

IMHO same as above applies here, and I think the idea here was 'if you have 
VM.Backup and Datastore.AllocateSpace you are allowed to access "your 
own" backups, even if you can't access the whole range of files on the 
storage', the code was just not very thought through (and in practice, 
high-privileged users on storages are usually also high-privileged users 
on guests, so nobody noticed/cared).

I think adding an early return with the check from the else branch with 
$noerr set and replacing the else branch with a `die` would be fine (so 
Datastore admins are always allowed to access all volumes on their 
storages).

>>> +	} elsif (($vtype eq 'images' || $vtype eq 'rootdir') && $ownervm) {
>>> +	    $rpcenv->check($user, "/vms/$ownervm", ['VM.Config.Disk']);
>> 
>> Of course this needs to be or-ed with the Datastore.Allocate privilege.
>> Will fix it in v2.

and and-ed with Datastore.AllocateSpace?

>> 
>>>  	} else {
>>>  	    # allow if we are Datastore administrator
>>>  	    $rpcenv->check($user, "/storage/$sid", ['Datastore.Allocate']);
>> 
>> 
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>> 
>> 
> 

Re: [pve-devel] [PATCH storage 1/4] check volume access: allow if user has VM.Config.Disk

[Fabian Ebner]

Am 24.03.22 um 09:18 schrieb Fabian Grünbichler:
> On March 22, 2022 10:31 am, Fabian Ebner wrote:
>> Am 22.03.22 um 09:31 schrieb Fabian Ebner:
>>> Am 21.03.22 um 14:06 schrieb Fabian Ebner:
>>>> Listing guest images should not require Datastore.Allocate in this
>>>> case. In preparation for adding disk import to the GUI.
>>>>
>>>> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
>>>> ---
>>>>  PVE/Storage.pm | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
>>>> index 6112991..efa304a 100755
>>>> --- a/PVE/Storage.pm
>>>> +++ b/PVE/Storage.pm
>>>> @@ -486,6 +486,8 @@ sub check_volume_access {
>>>>  	} elsif ($vtype eq 'backup' && $ownervm) {
>>>>  	    $rpcenv->check($user, "/storage/$sid", ['Datastore.AllocateSpace']);
>>>>  	    $rpcenv->check($user, "/vms/$ownervm", ['VM.Backup']);
>>
>> @Fabian G. should access to backups also be allowed if the user /just/
>> has Datastore.Allocate?
>>
>> Otherwise, backups cannot be listed or removed (there is a separate
>> check, but works similarly) and attributes cannot be changed by a
>> supposedly high-privileged user.
> 
> yeah, I think Datastore.Allocate could be allowed here, it's documented 
> as:
> 
> Datastore.Allocate: create/modify/remove a datastore and delete volumes
> 
> but in practice, any user that has Datastore.Allocate likely also has 
> Datastore.AllocateSpace anyway which is probably why nobody complained 
> yet ;)
> 
>> On the other hand, we also use this check for extractconfig, where it
>> makes sense to be limited to users with VM.Backup, but could be changed
>> at the call site of course.
> 
> IMHO same as above applies here, and I think the idea here was 'if you have 
> VM.Backup and Datastore.AllocateSpace you are allowed to access "your 
> own" backups, even if you can't access the whole range of files on the 
> storage', the code was just not very thought through (and in practice, 
> high-privileged users on storages are usually also high-privileged users 
> on guests, so nobody noticed/cared).
> 
> I think adding an early return with the check from the else branch with 
> $noerr set and replacing the else branch with a `die` would be fine (so 
> Datastore admins are always allowed to access all volumes on their 
> storages).
> 

Sounds good, I'll go for that in v2.

>>>> +	} elsif (($vtype eq 'images' || $vtype eq 'rootdir') && $ownervm) {
>>>> +	    $rpcenv->check($user, "/vms/$ownervm", ['VM.Config.Disk']);
>>>
>>> Of course this needs to be or-ed with the Datastore.Allocate privilege.
>>> Will fix it in v2.
> 
> and and-ed with Datastore.AllocateSpace?
>

I'm not sure. For clone, that's currently not checked (it's enough to
have VM.Clone and Datastore.AllocateSpace on the target storage, and I
kept it consistent with that for the proposed import-from), so it would
be a bit weird if listing the images requires it, while the actual
operation doesn't. But I don't mind adding it, if you want me to?

>>>
>>>>  	} else {
>>>>  	    # allow if we are Datastore administrator
>>>>  	    $rpcenv->check($user, "/storage/$sid", ['Datastore.Allocate']);
>>>
>>>
>>> _______________________________________________
>>> pve-devel mailing list
>>> pve-devel@lists.proxmox.com
>>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>>
>>>
>>

Re: [pve-devel] [PATCH storage 1/4] check volume access: allow if user has VM.Config.Disk

[Fabian Grünbichler]

On March 28, 2022 11:07 am, Fabian Ebner wrote:
> Am 24.03.22 um 09:18 schrieb Fabian Grünbichler:
>> On March 22, 2022 10:31 am, Fabian Ebner wrote:
>>> Am 22.03.22 um 09:31 schrieb Fabian Ebner:
>>>> Am 21.03.22 um 14:06 schrieb Fabian Ebner:
>>>>> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
>>>>> index 6112991..efa304a 100755
>>>>> --- a/PVE/Storage.pm
>>>>> +++ b/PVE/Storage.pm

[..]

>>>>> +	} elsif (($vtype eq 'images' || $vtype eq 'rootdir') && $ownervm) {
>>>>> +	    $rpcenv->check($user, "/vms/$ownervm", ['VM.Config.Disk']);
>>>>
>>>> Of course this needs to be or-ed with the Datastore.Allocate privilege.
>>>> Will fix it in v2.
>> 
>> and and-ed with Datastore.AllocateSpace?
>>
> 
> I'm not sure. For clone, that's currently not checked (it's enough to
> have VM.Clone and Datastore.AllocateSpace on the target storage, and I
> kept it consistent with that for the proposed import-from), so it would
> be a bit weird if listing the images requires it, while the actual
> operation doesn't. But I don't mind adding it, if you want me to?

for listing a storage's contents we also already check Datastore.Audit | 
Datastore.AllocateSpace (as part of the API schema), but for info and 
attribute updating we only check `check_volume_access` which would 
mean that with your change these suddenly allow brute force listing 
(with /vm permission, but no permissions on the storage) which 
doesn't seem ideal. for those two API endpoints with the current version 
of check_volume_access one of Datastore.Allocatespace, Allocate or Audit 
(depending on volume type) is needed implicitly via check_volume_access..

basically I see two options:
- extend your new branch in check_volume_access to require Datastore.X 
  (Audit or Allocatespace?) in addition to VM.Config.Disk => import-from 
  would require it, info/update_attributes in the storage API would 
  require it if they take that branch
- change info/update_attributes to require any of Datastore.Allocate, 
  Datastore.AllocateSpace, Datastore.Audit => import-from would not 
  require them.

I think I prefer the first variant, since it's internally consistent in 
check_volume_access (all the branches check some storage priv, unless 
the special 'we checked already and if the volume is owned by this VMID 
it's okay' path is taken via a passed in owner $vmid) and is less 
'pitfall-y' (w.r.t. opening brute-force code paths like the info one).

we could of course think about extending it further in the direction of 
'Datastore.Audit | Datastore.AllocateSpace' vs 'Datastore.AllocateSpace' 
via a flag to differentiate between reading a volume and 
writing/allocating one (and then in import-from, the source would only 
need Audit, while the target would need AllocateSpace). but that would 
require some more thought I think..

side-note: the check in clone_vm is a bit strange, it overrides the 
source storage with the target storage, but not for the vmstatestorage, 
so it basically rechecks the permissions for that single config key but 
not for any others.. maybe we should even drop the check for 
vmstatestorage? if it's in the config, somebody with the appropriate 
permission put it there after all, and if a user can clone that VM all 
the config comes with it?

Re: [pve-devel] [PATCH storage 1/4] check volume access: allow if user has VM.Config.Disk

[Fabian Ebner]

Am 28.03.22 um 13:36 schrieb Fabian Grünbichler:
> On March 28, 2022 11:07 am, Fabian Ebner wrote:
>> Am 24.03.22 um 09:18 schrieb Fabian Grünbichler:
>>> On March 22, 2022 10:31 am, Fabian Ebner wrote:
>>>> Am 22.03.22 um 09:31 schrieb Fabian Ebner:
>>>>> Am 21.03.22 um 14:06 schrieb Fabian Ebner:
>>>>>> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
>>>>>> index 6112991..efa304a 100755
>>>>>> --- a/PVE/Storage.pm
>>>>>> +++ b/PVE/Storage.pm
> 
> [..]
> 
>>>>>> +	} elsif (($vtype eq 'images' || $vtype eq 'rootdir') && $ownervm) {
>>>>>> +	    $rpcenv->check($user, "/vms/$ownervm", ['VM.Config.Disk']);
>>>>>
>>>>> Of course this needs to be or-ed with the Datastore.Allocate privilege.
>>>>> Will fix it in v2.
>>>
>>> and and-ed with Datastore.AllocateSpace?
>>>
>>
>> I'm not sure. For clone, that's currently not checked (it's enough to
>> have VM.Clone and Datastore.AllocateSpace on the target storage, and I
>> kept it consistent with that for the proposed import-from), so it would
>> be a bit weird if listing the images requires it, while the actual
>> operation doesn't. But I don't mind adding it, if you want me to?
> 
> for listing a storage's contents we also already check Datastore.Audit | 
> Datastore.AllocateSpace (as part of the API schema), but for info and 
> attribute updating we only check `check_volume_access` which would 
> mean that with your change these suddenly allow brute force listing 
> (with /vm permission, but no permissions on the storage) which 
> doesn't seem ideal. for those two API endpoints with the current version 
> of check_volume_access one of Datastore.Allocatespace, Allocate or Audit 
> (depending on volume type) is needed implicitly via check_volume_access..
> 
> basically I see two options:
> - extend your new branch in check_volume_access to require Datastore.X 
>   (Audit or Allocatespace?) in addition to VM.Config.Disk => import-from 
>   would require it, info/update_attributes in the storage API would 
>   require it if they take that branch
> - change info/update_attributes to require any of Datastore.Allocate, 
>   Datastore.AllocateSpace, Datastore.Audit => import-from would not 
>   require them.

import-from in its current form doesn't use check_volume_access() for
the source volume if it belongs to a VM, but requires VM.Clone. Just
like the clone_vm API call. So it's not import-from itself that would
require different things depending on the variant we choose, but e.g.
listing images for import-from in the UI.

> 
> I think I prefer the first variant, since it's internally consistent in 
> check_volume_access (all the branches check some storage priv, unless 
> the special 'we checked already and if the volume is owned by this VMID 
> it's okay' path is taken via a passed in owner $vmid) and is less 
> 'pitfall-y' (w.r.t. opening brute-force code paths like the info one).

I also prefer the first variant, but it can lead to a case where I
cannot - via UI, to a target storage I have access to - import-from a
single disk of a VM (just because I cannot list the image), but can
clone the whole VM to the same target storage.

> 
> we could of course think about extending it further in the direction of 
> 'Datastore.Audit | Datastore.AllocateSpace' vs 'Datastore.AllocateSpace' 
> via a flag to differentiate between reading a volume and 
> writing/allocating one (and then in import-from, the source would only 
> need Audit, while the target would need AllocateSpace). but that would 
> require some more thought I think..
> 
> side-note: the check in clone_vm is a bit strange, it overrides the 
> source storage with the target storage, but not for the vmstatestorage, 
> so it basically rechecks the permissions for that single config key but 
> not for any others.. maybe we should even drop the check for 
> vmstatestorage? if it's in the config, somebody with the appropriate 
> permission put it there after all, and if a user can clone that VM all 
> the config comes with it?
I'm not opposed to dropping that either. It's not like it changes the
fact that the user can create state images on that storage (assuming the
VM.Snapshot permission is not only granted for the clone's ID or
something, but those are edge-cases worth ignoring IMHO).

Re: [pve-devel] [PATCH storage 1/4] check volume access: allow if user has VM.Config.Disk

[Fabian Grünbichler]

On March 29, 2022 9:48 am, Fabian Ebner wrote:
> Am 28.03.22 um 13:36 schrieb Fabian Grünbichler:
>> On March 28, 2022 11:07 am, Fabian Ebner wrote:
>>> Am 24.03.22 um 09:18 schrieb Fabian Grünbichler:
>>>> On March 22, 2022 10:31 am, Fabian Ebner wrote:
>>>>> Am 22.03.22 um 09:31 schrieb Fabian Ebner:
>>>>>> Am 21.03.22 um 14:06 schrieb Fabian Ebner:
>>>>>>> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
>>>>>>> index 6112991..efa304a 100755
>>>>>>> --- a/PVE/Storage.pm
>>>>>>> +++ b/PVE/Storage.pm
>> 
>> [..]
>> 
>>>>>>> +	} elsif (($vtype eq 'images' || $vtype eq 'rootdir') && $ownervm) {
>>>>>>> +	    $rpcenv->check($user, "/vms/$ownervm", ['VM.Config.Disk']);
>>>>>>
>>>>>> Of course this needs to be or-ed with the Datastore.Allocate privilege.
>>>>>> Will fix it in v2.
>>>>
>>>> and and-ed with Datastore.AllocateSpace?
>>>>
>>>
>>> I'm not sure. For clone, that's currently not checked (it's enough to
>>> have VM.Clone and Datastore.AllocateSpace on the target storage, and I
>>> kept it consistent with that for the proposed import-from), so it would
>>> be a bit weird if listing the images requires it, while the actual
>>> operation doesn't. But I don't mind adding it, if you want me to?
>> 
>> for listing a storage's contents we also already check Datastore.Audit | 
>> Datastore.AllocateSpace (as part of the API schema), but for info and 
>> attribute updating we only check `check_volume_access` which would 
>> mean that with your change these suddenly allow brute force listing 
>> (with /vm permission, but no permissions on the storage) which 
>> doesn't seem ideal. for those two API endpoints with the current version 
>> of check_volume_access one of Datastore.Allocatespace, Allocate or Audit 
>> (depending on volume type) is needed implicitly via check_volume_access..
>> 
>> basically I see two options:
>> - extend your new branch in check_volume_access to require Datastore.X 
>>   (Audit or Allocatespace?) in addition to VM.Config.Disk => import-from 
>>   would require it, info/update_attributes in the storage API would 
>>   require it if they take that branch
>> - change info/update_attributes to require any of Datastore.Allocate, 
>>   Datastore.AllocateSpace, Datastore.Audit => import-from would not 
>>   require them.
> 
> import-from in its current form doesn't use check_volume_access() for
> the source volume if it belongs to a VM, but requires VM.Clone. Just
> like the clone_vm API call. So it's not import-from itself that would
> require different things depending on the variant we choose, but e.g.
> listing images for import-from in the UI.

yeah, I meant the GUI when talking about import-from, sorry for not 
being explicit.

>> I think I prefer the first variant, since it's internally consistent in 
>> check_volume_access (all the branches check some storage priv, unless 
>> the special 'we checked already and if the volume is owned by this VMID 
>> it's okay' path is taken via a passed in owner $vmid) and is less 
>> 'pitfall-y' (w.r.t. opening brute-force code paths like the info one).
> 
> I also prefer the first variant, but it can lead to a case where I
> cannot - via UI, to a target storage I have access to - import-from a
> single disk of a VM (just because I cannot list the image), but can
> clone the whole VM to the same target storage.

I think that's fine, we have plenty of areas where the backend allows 
more then the GUI offers ;) granted, most of that is root@pam only, but 
there is stuff like the GUI doesn't allow adding a 'foreign' disk to a 
VM, while the backend has no issue with that as long as you satisfy the 
priv checks. just because cloning and importing are similar and share 
some checks doesn't mean they are the same after all, so that the one 
with more flexibility (import) doesn't have all the options on the GUI 
(at least from the get-go) is not much of a surprise.

we could get around the limitation by offering another level of selector 
'source type'
- storage -> select storage -> list all accessible volumes on storage 
  (storage priv required)
- VM -> select VMID -> list all volumes referenced in VM config (no 
  storage priv required, just VM access)
- manual -> freeform entry (priv required depends on value)

that being said, only showing those disks where the user has full list 
permissions on the storage (+ some level of access to the VM config) 
sounds like a good compromise that will work in almost all cases - users 
that have Clone/VM.Allocate usually also have storage access ;)

>> we could of course think about extending it further in the direction of 
>> 'Datastore.Audit | Datastore.AllocateSpace' vs 'Datastore.AllocateSpace' 
>> via a flag to differentiate between reading a volume and 
>> writing/allocating one (and then in import-from, the source would only 
>> need Audit, while the target would need AllocateSpace). but that would 
>> require some more thought I think..
>> 
>> side-note: the check in clone_vm is a bit strange, it overrides the 
>> source storage with the target storage, but not for the vmstatestorage, 
>> so it basically rechecks the permissions for that single config key but 
>> not for any others.. maybe we should even drop the check for 
>> vmstatestorage? if it's in the config, somebody with the appropriate 
>> permission put it there after all, and if a user can clone that VM all 
>> the config comes with it?
> I'm not opposed to dropping that either. It's not like it changes the
> fact that the user can create state images on that storage (assuming the
> VM.Snapshot permission is not only granted for the clone's ID or
> something, but those are edge-cases worth ignoring IMHO).