[PATCH-SERIES storage/manager 0/4] fix #8169: backup: extract config: fix regression with backup type check

[PATCH-SERIES storage/manager 0/4] fix #8169: backup: extract config: fix regression with backup type check

[Fiona Ebner]

Allow a user with Datastore.Allocate to extract the configuration from
a guest backup. Avoid early checks for the backup's associated guest
ID, because not all guest backups have one. Extracting the
configuration from an ID-less guest backup regressed and this fixes
the regression. The storage layer already checks for the backup's
type.

storage:

Fiona Ebner (2):
  pvesm: extractconfig: allow access for Datastore.Allocate
  fix #8169: pvesm: extractconfig: fix regression with backup type check

 src/PVE/CLI/pvesm.pm | 8 --------
 1 file changed, 8 deletions(-)


manager:

Fiona Ebner (2):
  api: backup: extract config: allow access for Datastore.Allocate
  fix #8169: api: backup: extract config: fix regression with backup
    type check

 PVE/API2/VZDump.pm | 8 --------
 1 file changed, 8 deletions(-)


Summary over all repositories:
  2 files changed, 0 insertions(+), 16 deletions(-)

-- 
Generated by git-murpp 0.5.0

[PATCH storage 1/4] pvesm: extractconfig: allow access for Datastore.Allocate

[Fiona Ebner]

The check_volume_access() call checks that the user has 'VM.Backup'
for the owning guest's ID or 'Datastore.Allocate' for the storage. The
latter is the highest possible storage permission. Consider that
enough to extract the configuration even without an explicit
'VM.Backup' permission.

Note that for restoring a backup, only the check_volume_access() call
is used. So this change also better aligns the checks.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/CLI/pvesm.pm | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/PVE/CLI/pvesm.pm b/src/PVE/CLI/pvesm.pm
index 1aaa9f3..4b42734 100755
--- a/src/PVE/CLI/pvesm.pm
+++ b/src/PVE/CLI/pvesm.pm
@@ -187,7 +187,6 @@ __PACKAGE__->register_method({
             if (!defined($ownervm)) {
                 die "unable to extract configuration for '$volume' - not a guest backup\n";
             }
-            $rpcenv->check($authuser, "/vms/$ownervm", ['VM.Backup']);
         }
 
         my $config_raw = PVE::Storage::extract_vzdump_config($storage_cfg, $volume);
-- 
2.47.3

[PATCH storage 2/4] fix #8169: pvesm: extractconfig: fix regression with backup type check

[Fiona Ebner]

Commit 9d0049e ("pvesm: extractconfig: clean early error for non-guest
backup") introduced a regression for guest backups without a guest ID:
it is no longer possible to extract the configuration from such
backups. Before, there was a Perl warning about the uninitialized ID
and the permission check would need to pass for 'VM.Backup' on '/vms',
but extraction was possible and it was relied on in practice, see the
regression report.

Avoid the early check for a guest ID, which does not cover (e.g.
manually renamed) guest backups without a guest ID. The storage layer
already checks the backup type later. In particular, the
extract_vzdump_config() function has three branches:

1. PBS: the PBS plugin's extract_vzdump_config() checks that the
   backup's format is either 'pbs-vm' or 'pbs-ct'.
2. backup provider: only guest backups are part of the backup provider
   API. It is the implemenation's responsibility to avoid exposing
   other content, in particular via archive_get_guest_config() here.
3. path-based vzdump backup: the backup type is checked via
   archive_info() to be one of 'lxc', 'openvz' or 'qemu'.

Fixes: 9d0049e ("pvesm: extractconfig: clean early error for non-guest backup")
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/CLI/pvesm.pm | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/src/PVE/CLI/pvesm.pm b/src/PVE/CLI/pvesm.pm
index 4b42734..fa32295 100755
--- a/src/PVE/CLI/pvesm.pm
+++ b/src/PVE/CLI/pvesm.pm
@@ -182,13 +182,6 @@ __PACKAGE__->register_method({
             $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);
-            if (!defined($ownervm)) {
-                die "unable to extract configuration for '$volume' - not a guest backup\n";
-            }
-        }
-
         my $config_raw = PVE::Storage::extract_vzdump_config($storage_cfg, $volume);
 
         print "$config_raw\n";
-- 
2.47.3

[PATCH manager 3/4] api: backup: extract config: allow access for Datastore.Allocate

[Fiona Ebner]

The check_volume_access() call checks that the user has 'VM.Backup'
for the owning guest's ID or 'Datastore.Allocate' for the storage. The
latter is the highest possible storage permission. Consider that
enough to extract the configuration even without an explicit
'VM.Backup' permission.

Note that for restoring a backup, only the check_volume_access() call
is used. So this change also better aligns the checks.

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

diff --git a/PVE/API2/VZDump.pm b/PVE/API2/VZDump.pm
index 8875070e..98caf1d9 100644
--- a/PVE/API2/VZDump.pm
+++ b/PVE/API2/VZDump.pm
@@ -308,7 +308,6 @@ __PACKAGE__->register_method({
             if (!defined($ownervm)) {
                 die "unable to extract configuration for '$volume' - not a guest backup\n";
             }
-            $rpcenv->check($authuser, "/vms/$ownervm", ['VM.Backup']);
         }
 
         return PVE::Storage::extract_vzdump_config($storage_cfg, $volume);
-- 
2.47.3

[PATCH manager 4/4] fix #8169: api: backup: extract config: fix regression with backup type check

[Fiona Ebner]

Commit c7d53321 ("backup: extractconfig: clean early error for
non-guest backup") introduced a regression for guest backups without a
guest ID: it is no longer possible to extract the configuration from
such backups. Before, there was a Perl warning about the uninitialized
ID and the permission check would need to pass for 'VM.Backup' on
'/vms', but extraction was possible and it was relied on in practice,
see the regression report.

Avoid the early check for a guest ID, which does not cover (e.g.
manually renamed) guest backups without a guest ID. The storage layer
already checks the backup type later. In particular, the
extract_vzdump_config() function has three branches:

1. PBS: the PBS plugin's extract_vzdump_config() checks that the
   backup's format is either 'pbs-vm' or 'pbs-ct'.
2. backup provider: only guest backups are part of the backup provider
   API. It is the implemenation's responsibility to avoid exposing
   other content, in particular via archive_get_guest_config() here.
3. path-based vzdump backup: the backup type is checked via
   archive_info() to be one of 'lxc', 'openvz' or 'qemu'.

Fixes: c7d53321 ("backup: extractconfig: clean early error for non-guest backup")
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/API2/VZDump.pm | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/PVE/API2/VZDump.pm b/PVE/API2/VZDump.pm
index 98caf1d9..62ca153c 100644
--- a/PVE/API2/VZDump.pm
+++ b/PVE/API2/VZDump.pm
@@ -303,13 +303,6 @@ __PACKAGE__->register_method({
             $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);
-            if (!defined($ownervm)) {
-                die "unable to extract configuration for '$volume' - not a guest backup\n";
-            }
-        }
-
         return PVE::Storage::extract_vzdump_config($storage_cfg, $volume);
     },
 });
-- 
2.47.3