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

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

[Fabian Ebner]

The first few patches are to allow access for users with
Datastore.Allocate privilege, without automatically giving them
permission to extract a backup config.

Patch storage 3/6 is in preparation for the import-from API, allowing
users with VM.Config.Disk (and Datastore.Audit) to list images of
their VMs.

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

Patch storage 2/6 technically breaks older manager, allowing all users
with Datastore.Allocate to extract backup configs, but I'm not sure
that's worth bothering about.

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


Changes from v1:
    * Always allow with Datastore.Allocate privilege.
    * Also check for Datastore.Audit when listing guest images/rootdir
      rather than just VM.Config.Disk.


manager:

Fabian Ebner (3):
  api: vzdump: extract config: check for VM.Backup privilege
  pveam: remove: add content type check
  api: vzdump: extract config: add content type check

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


storage:

Fabian Ebner (6):
  pvesm: extract config: check for VM.Backup privilege
  check volume access: always allow with Datastore.Allocate privilege
  check volume access: allow for images/rootdir 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                | 14 +++++++++++++-
 PVE/Storage.pm                  | 15 ++++++++++++---
 3 files changed, 29 insertions(+), 12 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 v2 manager 1/3] api: vzdump: extract config: check for VM.Backup privilege

[Fabian Ebner]

In preparation to have check_volume_access() always allow access for
users with Datastore.Allocate privilege. As to not automatically give
all such users permission to extract the config too.

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

New in v2.

 PVE/API2/VZDump.pm | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/PVE/API2/VZDump.pm b/PVE/API2/VZDump.pm
index 2c0df4c3..a6c4d111 100644
--- a/PVE/API2/VZDump.pm
+++ b/PVE/API2/VZDump.pm
@@ -269,6 +269,11 @@ __PACKAGE__->register_method ({
 	my $storage_cfg = PVE::Storage::config();
 	PVE::Storage::check_volume_access($rpcenv, $authuser, $storage_cfg, undef, $volume);
 
+	if (PVE::Storage::parse_volume_id($volume, 1)) {
+	    my (undef, undef, $ownervm) = PVE::Storage::parse_volname($storage_cfg, $volume);
+	    $rpcenv->check($authuser, "/vms/$ownervm", ['VM.Backup']);
+	}
+
 	return PVE::Storage::extract_vzdump_config($storage_cfg, $volume);
     }});
 
-- 
2.30.2

[pve-devel] [PATCH v2 storage 1/6] pvesm: extract config: check for VM.Backup privilege

[Fabian Ebner]

In preparation to have check_volume_access() always allow access for
users with Datastore.Allocate privilege. As to not automatically give
all such users permission to extract the config too.

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

New in v2.

 PVE/CLI/pvesm.pm | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/PVE/CLI/pvesm.pm b/PVE/CLI/pvesm.pm
index 190de91..1daed71 100755
--- a/PVE/CLI/pvesm.pm
+++ b/PVE/CLI/pvesm.pm
@@ -172,6 +172,11 @@ __PACKAGE__->register_method ({
 	my $storage_cfg = PVE::Storage::config();
 	PVE::Storage::check_volume_access($rpcenv, $authuser, $storage_cfg, undef, $volume);
 
+	if (PVE::Storage::parse_volume_id($volume, 1)) {
+	    my (undef, undef, $ownervm) = PVE::Storage::parse_volname($storage_cfg, $volume);
+	    $rpcenv->check($authuser, "/vms/$ownervm", ['VM.Backup']);
+	}
+
 	my $config_raw = PVE::Storage::extract_vzdump_config($storage_cfg, $volume);
 
 	print "$config_raw\n";
-- 
2.30.2

[pve-devel] [PATCH v2 storage 2/6] check volume access: always allow with Datastore.Allocate privilege

[Fabian Ebner]

Such users are supposed to be administrators of the storage, but
previously, access to backups was not allowed when not also having
VM.Backup.

Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

New in v2.

Technically, a breaking change for pve-manager, because a user without
VM.Backup privilege can now extract the guest config.

 PVE/Storage.pm | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index 6112991..0349564 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -477,6 +477,8 @@ sub check_volume_access {
 
     my ($sid, $volname) = parse_volume_id($volid, 1);
     if ($sid) {
+	return if $rpcenv->check($user, "/storage/$sid", ['Datastore.Allocate'], 1);
+
 	my ($vtype, undef, $ownervm) = parse_volname($cfg, $volid);
 	if ($vtype eq 'iso' || $vtype eq 'vztmpl') {
 	    # require at least read access to storage, (custom) templates/ISOs could be sensitive
@@ -487,8 +489,7 @@ sub check_volume_access {
 	    $rpcenv->check($user, "/storage/$sid", ['Datastore.AllocateSpace']);
 	    $rpcenv->check($user, "/vms/$ownervm", ['VM.Backup']);
 	} else {
-	    # allow if we are Datastore administrator
-	    $rpcenv->check($user, "/storage/$sid", ['Datastore.Allocate']);
+	    die "missing privileges to access $volid\n";
 	}
     } else {
 	die "Only root can pass arbitrary filesystem paths."
-- 
2.30.2

[pve-devel] [PATCH v2 storage 3/6] check volume access: allow for images/rootdir 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>
---

Changes from v1:
    * Also check for Datastore.Audit privilege.

 PVE/Storage.pm | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index 0349564..a864c33 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -488,6 +488,9 @@ 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, "/storage/$sid", ['Datastore.Audit']);
+	    $rpcenv->check($user, "/vms/$ownervm", ['VM.Config.Disk']);
 	} else {
 	    die "missing privileges to access $volid\n";
 	}
-- 
2.30.2

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

[Fabian Ebner]

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

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

No changes from v1.

 PVE/Storage.pm | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index a864c33..3b86956 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -473,13 +473,18 @@ 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');
+
 	return if $rpcenv->check($user, "/storage/$sid", ['Datastore.Allocate'], 1);
 
-	my ($vtype, undef, $ownervm) = parse_volname($cfg, $volid);
 	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 v2 storage 5/6] pvesm: extract config: add content type check

[Fabian Ebner]

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

No changes from v1.

 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 1daed71..003b019 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',
+	);
 
 	if (PVE::Storage::parse_volume_id($volume, 1)) {
 	    my (undef, undef, $ownervm) = PVE::Storage::parse_volname($storage_cfg, $volume);
-- 
2.30.2

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

[Fabian Ebner]

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

No changes from v1.

 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 v2 manager 2/3] pveam: remove: add content type check

[Fabian Ebner]

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

No changes from v1.

 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 v2 manager 3/3] api: vzdump: extract config: add content type check

[Fabian Ebner]

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

No changes from v1.

 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 a6c4d111..13b6cd46 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',
+	);
 
 	if (PVE::Storage::parse_volume_id($volume, 1)) {
 	    my (undef, undef, $ownervm) = PVE::Storage::parse_volname($storage_cfg, $volume);
-- 
2.30.2

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

[Fabian Ebner]

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

No changes from v1.

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 v2 qemu-server 1/1] api: create/modify: add content type checks

[Fabian Ebner]

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

No changes from v1.

 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

[pve-devel] applied-series: [PATCH-SERIES v2 manager/storage/container/qemu-server] improve check_volume_access

[Fabian Grünbichler]

with potential for the following follow-up:
- uploading isos/templates or downloading them currently requires 
  Datastore.AllocateTemplate
- deleting isos/templates currently requires Datastore.Allocate

the latter could get a less restrictive check like we already have for 
backup volumes, to allow users with just 'manage templates/isos' 
permissions to remove the stuff they can create ;)

On March 30, 2022 12:24 pm, Fabian Ebner wrote:
> The first few patches are to allow access for users with
> Datastore.Allocate privilege, without automatically giving them
> permission to extract a backup config.
> 
> Patch storage 3/6 is in preparation for the import-from API, allowing
> users with VM.Config.Disk (and Datastore.Audit) to list images of
> their VMs.
> 
> The rest of the series introduces a content type parameter to
> check_volume_access() for future-proofing.
> 
> Patch storage 2/6 technically breaks older manager, allowing all users
> with Datastore.Allocate to extract backup configs, but I'm not sure
> that's worth bothering about.
> 
> Dependency bumps for storage are needed for the content parameter to
> actually have an effect.
> 
> 
> Changes from v1:
>     * Always allow with Datastore.Allocate privilege.
>     * Also check for Datastore.Audit when listing guest images/rootdir
>       rather than just VM.Config.Disk.
> 
> 
> manager:
> 
> Fabian Ebner (3):
>   api: vzdump: extract config: check for VM.Backup privilege
>   pveam: remove: add content type check
>   api: vzdump: extract config: add content type check
> 
>  PVE/API2/VZDump.pm | 14 +++++++++++++-
>  PVE/CLI/pveam.pm   |  2 +-
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> 
> storage:
> 
> Fabian Ebner (6):
>   pvesm: extract config: check for VM.Backup privilege
>   check volume access: always allow with Datastore.Allocate privilege
>   check volume access: allow for images/rootdir 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                | 14 +++++++++++++-
>  PVE/Storage.pm                  | 15 ++++++++++++---
>  3 files changed, 29 insertions(+), 12 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 mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>