[pve-devel] [PATCH storage v2 0/4] pbs: fix #5008: Prevent adding pbs storage with invalid namespace

[pve-devel] [PATCH storage v2 0/4] pbs: fix #5008: Prevent adding pbs storage with invalid namespace

[Philipp Hufnagl]

Currently, when adding a PBS storage with a namespace that does not
exist, the storage gets added normally, but browsing/using it only
returns a cryptic error message.

This change checks if the namespace entered when adding is valid and
prompts an error if it is not. If no namespace is provided, the storage
will be added without error.

This is done by adding code to check if the namespace exists and call it
as well as existing code to check if a datastore exists on the add and
update hooks of the PBS datastore.

Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
---

Changes since v1:
 * do not add any overhead to activate_storage calls
 * splits code from activate_storage so parts of it can be reused
 * adds new methods to check namespaces
 * calls checks on add/update hooks

Philipp Hufnagl (4):
  pbs: Move pbs_api_connect earlyer in the code
  pbs: Make it possible to reuse PBS connection for datastore API call
  pbs: Extraxt check_datastore_exists from activate_storage
  pbs: fix #5008: Check if datastore and namespace is valid on add- and
    update hooks

 src/PVE/Storage/PBSPlugin.pm | 126 ++++++++++++++++++++++++-----------
 1 file changed, 87 insertions(+), 39 deletions(-)

-- 
2.39.2

[pve-devel] [PATCH storage v2 1/4] pbs: Move pbs_api_connect earlyer in the code

[Philipp Hufnagl]

Because it is needed later in this patch series, the method
pbs_api_connect is moved earlyer in the code

Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
---
 src/PVE/Storage/PBSPlugin.pm | 63 ++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 31 deletions(-)

diff --git a/src/PVE/Storage/PBSPlugin.pm b/src/PVE/Storage/PBSPlugin.pm
index 4320974..96373a4 100644
--- a/src/PVE/Storage/PBSPlugin.pm
+++ b/src/PVE/Storage/PBSPlugin.pm
@@ -112,6 +112,38 @@ sub pbs_get_password {
     return PVE::Tools::file_read_firstline($pwfile);
 }
 
+#
+# TODO: use a client with native rust/proxmox-backup bindings to profit from
+# API schema checks and types
+my sub pbs_api_connect {
+    my ($scfg, $password, $timeout) = @_;
+
+    my $params = {};
+
+    my $user = $scfg->{username} // 'root@pam';
+
+    if (my $tokenid = PVE::AccessControl::pve_verify_tokenid($user, 1)) {
+	$params->{apitoken} = "PBSAPIToken=${tokenid}:${password}";
+    } else {
+	$params->{password} = $password;
+	$params->{username} = $user;
+    }
+
+    if (my $fp = $scfg->{fingerprint}) {
+	$params->{cached_fingerprints}->{uc($fp)} = 1;
+    }
+
+    my $conn = PVE::APIClient::LWP->new(
+	%$params,
+	host => $scfg->{server},
+	port => $scfg->{port} // 8007,
+	timeout => ($timeout // 7), # cope with a 401 (3s api delay) and high latency
+	cookie_name => 'PBSAuthCookie',
+    );
+
+    return $conn;
+}
+
 sub pbs_encryption_key_file_name {
     my ($scfg, $storeid) = @_;
 
@@ -691,37 +723,6 @@ my sub snapshot_files_encrypted {
     return $any && $all;
 }
 
-# TODO: use a client with native rust/proxmox-backup bindings to profit from
-# API schema checks and types
-my sub pbs_api_connect {
-    my ($scfg, $password, $timeout) = @_;
-
-    my $params = {};
-
-    my $user = $scfg->{username} // 'root@pam';
-
-    if (my $tokenid = PVE::AccessControl::pve_verify_tokenid($user, 1)) {
-	$params->{apitoken} = "PBSAPIToken=${tokenid}:${password}";
-    } else {
-	$params->{password} = $password;
-	$params->{username} = $user;
-    }
-
-    if (my $fp = $scfg->{fingerprint}) {
-	$params->{cached_fingerprints}->{uc($fp)} = 1;
-    }
-
-    my $conn = PVE::APIClient::LWP->new(
-	%$params,
-	host => $scfg->{server},
-	port => $scfg->{port} // 8007,
-	timeout => ($timeout // 7), # cope with a 401 (3s api delay) and high latency
-	cookie_name => 'PBSAuthCookie',
-    );
-
-    return $conn;
-}
-
 sub list_volumes {
     my ($class, $storeid, $scfg, $vmid, $content_types) = @_;
 
-- 
2.39.2

[pve-devel] [PATCH storage v2 2/4] pbs: Make it possible to reuse PBS connection for datastore API call

[Philipp Hufnagl]

It would be nice to reuse an existing PBS connection for scan_datastore.
Because scan_datastore is used multiple in the code, it can not be
canged without breaking existing code.

This change add an optional connection parameter to scan_datastore. If
it is passed it will use this connection. If not, it will create a new
one.

Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
---
 src/PVE/Storage/PBSPlugin.pm | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/PVE/Storage/PBSPlugin.pm b/src/PVE/Storage/PBSPlugin.pm
index 96373a4..81df21e 100644
--- a/src/PVE/Storage/PBSPlugin.pm
+++ b/src/PVE/Storage/PBSPlugin.pm
@@ -808,9 +808,11 @@ sub status {
 #   fingerprint   (optional for trusted certs)
 # }
 sub scan_datastores {
-    my ($scfg, $password) = @_;
+    my ($scfg, $password, $conn) = @_;
 
-    my $conn = pbs_api_connect($scfg, $password);
+    if (!defined($conn)){
+	$conn = pbs_api_connect($scfg, $password);
+    }
 
     my $response = eval { $conn->get('/api2/json/admin/datastore', {}) };
     die "error fetching datastores - $@" if $@;
-- 
2.39.2

Re: [pve-devel] [PATCH storage v2 2/4] pbs: Make it possible to reuse PBS connection for datastore API call

[Christian Ebner]

> On 15.11.2023 17:00 CET Philipp Hufnagl <p.hufnagl@proxmox.com> wrote:
> 
>  
> It would be nice to reuse an existing PBS connection for scan_datastore.
> Because scan_datastore is used multiple in the code, it can not be
> canged without breaking existing code.

Typo s/canged/changed.
 
> 
> This change add an optional connection parameter to scan_datastore. If
> it is passed it will use this connection. If not, it will create a new
> one.
> 
> Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
> ---
>  src/PVE/Storage/PBSPlugin.pm | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/src/PVE/Storage/PBSPlugin.pm b/src/PVE/Storage/PBSPlugin.pm
> index 96373a4..81df21e 100644
> --- a/src/PVE/Storage/PBSPlugin.pm
> +++ b/src/PVE/Storage/PBSPlugin.pm
> @@ -808,9 +808,11 @@ sub status {
>  #   fingerprint   (optional for trusted certs)
>  # }
>  sub scan_datastores {
> -    my ($scfg, $password) = @_;
> +    my ($scfg, $password, $conn) = @_;
>  
> -    my $conn = pbs_api_connect($scfg, $password);
> +    if (!defined($conn)){
> +	$conn = pbs_api_connect($scfg, $password);
> +    }

This can be written more compactly as:
$conn = pbs_api_connect($scfg, $password) if !defined($conn);

>  
>      my $response = eval { $conn->get('/api2/json/admin/datastore', {}) };
>      die "error fetching datastores - $@" if $@;
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH storage v2 3/4] pbs: Extraxt check_datastore_exists from activate_storage

[Philipp Hufnagl]

Parts contained in activate_storage are needed to be run to fix #5008,
however, implementing a namespace check there would cause unneded
overhead.

Therfore, this patch extracts the method check_datastore_exists from
activate storage.

Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
---
 src/PVE/Storage/PBSPlugin.pm | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/src/PVE/Storage/PBSPlugin.pm b/src/PVE/Storage/PBSPlugin.pm
index 81df21e..104fe15 100644
--- a/src/PVE/Storage/PBSPlugin.pm
+++ b/src/PVE/Storage/PBSPlugin.pm
@@ -819,17 +819,13 @@ sub scan_datastores {
 
     return $response;
 }
-
-sub activate_storage {
-    my ($class, $storeid, $scfg, $cache) = @_;
-
-    my $password = pbs_get_password($scfg, $storeid);
-
-    my $datastores = eval { scan_datastores($scfg, $password) };
-    die "$storeid: $@" if $@;
+sub check_datastore_exists {
+    my ($class, $storeid, $scfg, $password, $conn) = @_;
 
     my $datastore = $scfg->{datastore};
 
+    my $datastores = eval { scan_datastores($scfg, $password, $conn) };
+    die "$storeid: $@" if $@;
     for my $ds (@$datastores) {
 	if ($ds->{store} eq $datastore) {
 	    return 1;
@@ -839,6 +835,14 @@ sub activate_storage {
     die "$storeid: Cannot find datastore '$datastore', check permissions and existence!\n";
 }
 
+sub activate_storage {
+    my ($class, $storeid, $scfg, $cache) = @_;
+
+    my $password = pbs_get_password($scfg, $storeid);
+    my $conn = pbs_api_connect($scfg, $password);
+    check_datastore_exists($class, $storeid, $scfg, $password, $conn);
+}
+
 sub deactivate_storage {
     my ($class, $storeid, $scfg, $cache) = @_;
     return 1;
-- 
2.39.2

Re: [pve-devel] [PATCH storage v2 3/4] pbs: Extraxt check_datastore_exists from activate_storage

[Christian Ebner]

Typi in heading s/Extraxt/Extract, maybe call it factor out instead.

> On 15.11.2023 17:00 CET Philipp Hufnagl <p.hufnagl@proxmox.com> wrote:
> 
>  
> Parts contained in activate_storage are needed to be run to fix #5008,
> however, implementing a namespace check there would cause unneded
> overhead.

Typo s/unneded/unneeded.

> 
> Therfore, this patch extracts the method check_datastore_exists from
> activate storage.

Typo s/Thefore/Therefore.

> 
> Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
> ---
>  src/PVE/Storage/PBSPlugin.pm | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/src/PVE/Storage/PBSPlugin.pm b/src/PVE/Storage/PBSPlugin.pm
> index 81df21e..104fe15 100644
> --- a/src/PVE/Storage/PBSPlugin.pm
> +++ b/src/PVE/Storage/PBSPlugin.pm
> @@ -819,17 +819,13 @@ sub scan_datastores {
>  
>      return $response;
>  }
> -
> -sub activate_storage {
> -    my ($class, $storeid, $scfg, $cache) = @_;
> -
> -    my $password = pbs_get_password($scfg, $storeid);
> -
> -    my $datastores = eval { scan_datastores($scfg, $password) };
> -    die "$storeid: $@" if $@;
> +sub check_datastore_exists {
> +    my ($class, $storeid, $scfg, $password, $conn) = @_;
>  
>      my $datastore = $scfg->{datastore};
>  
> +    my $datastores = eval { scan_datastores($scfg, $password, $conn) };
> +    die "$storeid: $@" if $@;
>      for my $ds (@$datastores) {
>  	if ($ds->{store} eq $datastore) {
>  	    return 1;
> @@ -839,6 +835,14 @@ sub activate_storage {
>      die "$storeid: Cannot find datastore '$datastore', check permissions and existence!\n";
>  }
>  
> +sub activate_storage {
> +    my ($class, $storeid, $scfg, $cache) = @_;
> +
> +    my $password = pbs_get_password($scfg, $storeid);
> +    my $conn = pbs_api_connect($scfg, $password);
> +    check_datastore_exists($class, $storeid, $scfg, $password, $conn);
> +}
> +
>  sub deactivate_storage {
>      my ($class, $storeid, $scfg, $cache) = @_;
>      return 1;
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH storage v2 4/4] pbs: fix #5008: Check if datastore and namespace is valid on add- and update hooks

[Philipp Hufnagl]

This adds a check if the datastore and the namespace is valid when a
user attempts to add a new PBS datastore.

Since the namespace only can be checked after the datastore is
validated, the datastore will be checked as well, regardless that it
will be done later in the superclass anyway.

The functionallity to check namespaces is added with this commit. For
checking the datastore, existing code that has previously been
refactored will be reused

Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
---
 src/PVE/Storage/PBSPlugin.pm | 43 +++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/src/PVE/Storage/PBSPlugin.pm b/src/PVE/Storage/PBSPlugin.pm
index 104fe15..fff8bb2 100644
--- a/src/PVE/Storage/PBSPlugin.pm
+++ b/src/PVE/Storage/PBSPlugin.pm
@@ -566,6 +566,11 @@ sub on_add_hook {
 	pbs_delete_master_pubkey($scfg, $storeid);
     }
 
+    my $password = pbs_get_password($scfg, $storeid);
+    my $conn = pbs_api_connect($scfg, $password);
+    check_datastore_exists($class, $storeid, $scfg, $password, $conn);
+    check_namespace_exists($class, $storeid, $scfg, $password, $conn);
+
     return $res;
 }
 
@@ -614,6 +619,11 @@ sub on_update_hook {
 	}
     }
 
+    my $password = pbs_get_password($scfg, $storeid);
+    my $conn = pbs_api_connect($scfg, $password);
+    check_datastore_exists($class, $storeid, $scfg, $password, $conn);
+    check_namespace_exists($class, $storeid, $scfg, $password, $conn);
+
     return $res;
 }
 
@@ -819,6 +829,20 @@ sub scan_datastores {
 
     return $response;
 }
+
+sub scan_namespaces {
+    my ($scfg, $datastore, $password, $conn) = @_;
+
+    if (!defined($conn)){
+	$conn = pbs_api_connect($scfg, $password);
+    }
+
+    my $namespaces = eval { $conn->get("/api2/json/admin/datastore/$datastore/namespace", {}); };
+    die "error fetching namespaces - $@" if $@;
+
+    return $namespaces;
+}
+
 sub check_datastore_exists {
     my ($class, $storeid, $scfg, $password, $conn) = @_;
 
@@ -831,10 +855,27 @@ sub check_datastore_exists {
 	    return 1;
 	}
     }
-
     die "$storeid: Cannot find datastore '$datastore', check permissions and existence!\n";
 }
 
+sub check_namespace_exists {
+    my ($class, $storeid, $scfg, $password, $conn) = @_;
+
+    my $datastore = $scfg->{datastore};
+    my $namespace = $scfg->{namespace};
+
+    my $namespaces = eval { scan_namespaces($scfg, $datastore, $password) };
+    die "$storeid: $@" if $@;
+    return 1 if !defined($namespace);
+
+    for my $ns (@$namespaces) {
+	if ($ns->{ns} eq $namespace) {
+	    return 1;
+	}
+    }
+    die "$storeid: Cannot find namespace '$namespace', check permissions and existence!\n";
+}
+
 sub activate_storage {
     my ($class, $storeid, $scfg, $cache) = @_;
 
-- 
2.39.2

Re: [pve-devel] [PATCH storage v2 4/4] pbs: fix #5008: Check if datastore and namespace is valid on add- and update hooks

[Christian Ebner]

> On 15.11.2023 17:00 CET Philipp Hufnagl <p.hufnagl@proxmox.com> wrote:
> 
>  
> This adds a check if the datastore and the namespace is valid when a
> user attempts to add a new PBS datastore.
> 
> Since the namespace only can be checked after the datastore is
> validated, the datastore will be checked as well, regardless that it
> will be done later in the superclass anyway.
> 
> The functionallity to check namespaces is added with this commit. For
> checking the datastore, existing code that has previously been
> refactored will be reused
> 
> Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
> ---
>  src/PVE/Storage/PBSPlugin.pm | 43 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PVE/Storage/PBSPlugin.pm b/src/PVE/Storage/PBSPlugin.pm
> index 104fe15..fff8bb2 100644
> --- a/src/PVE/Storage/PBSPlugin.pm
> +++ b/src/PVE/Storage/PBSPlugin.pm
> @@ -566,6 +566,11 @@ sub on_add_hook {
>  	pbs_delete_master_pubkey($scfg, $storeid);
>      }
>  
> +    my $password = pbs_get_password($scfg, $storeid);
> +    my $conn = pbs_api_connect($scfg, $password);
> +    check_datastore_exists($class, $storeid, $scfg, $password, $conn);
> +    check_namespace_exists($class, $storeid, $scfg, $password, $conn);
> +
>      return $res;
>  }
>  
> @@ -614,6 +619,11 @@ sub on_update_hook {
>  	}
>      }
>  
> +    my $password = pbs_get_password($scfg, $storeid);
> +    my $conn = pbs_api_connect($scfg, $password);
> +    check_datastore_exists($class, $storeid, $scfg, $password, $conn);
> +    check_namespace_exists($class, $storeid, $scfg, $password, $conn);
> +
>      return $res;
>  }
>  
> @@ -819,6 +829,20 @@ sub scan_datastores {
>  
>      return $response;
>  }
> +
> +sub scan_namespaces {
> +    my ($scfg, $datastore, $password, $conn) = @_;
> +
> +    if (!defined($conn)){
> +	$conn = pbs_api_connect($scfg, $password);
> +    }

This can be written more compactly as:
$conn = pbs_api_connect($scfg, $password) if !defined($conn);

> +
> +    my $namespaces = eval { $conn->get("/api2/json/admin/datastore/$datastore/namespace", {}); };
> +    die "error fetching namespaces - $@" if $@;
> +
> +    return $namespaces;
> +}
> +
>  sub check_datastore_exists {
>      my ($class, $storeid, $scfg, $password, $conn) = @_;
>  
> @@ -831,10 +855,27 @@ sub check_datastore_exists {
>  	    return 1;
>  	}
>      }
> -
>      die "$storeid: Cannot find datastore '$datastore', check permissions and existence!\n";
>  }
>  
> +sub check_namespace_exists {
> +    my ($class, $storeid, $scfg, $password, $conn) = @_;
> +
> +    my $datastore = $scfg->{datastore};
> +    my $namespace = $scfg->{namespace};
> +
> +    my $namespaces = eval { scan_namespaces($scfg, $datastore, $password) };
> +    die "$storeid: $@" if $@;
> +    return 1 if !defined($namespace);

You can move this before the call to scan_namespaces, no need to fetch them if there is nothing to compare to to begin with.

> +
> +    for my $ns (@$namespaces) {
> +	if ($ns->{ns} eq $namespace) {
> +	    return 1;
> +	}
> +    }
> +    die "$storeid: Cannot find namespace '$namespace', check permissions and existence!\n";
> +}
> +
>  sub activate_storage {
>      my ($class, $storeid, $scfg, $cache) = @_;
>  
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH storage v2 4/4] pbs: fix #5008: Check if datastore and namespace is valid on add- and update hooks

[Fiona Ebner]

Am 15.11.23 um 17:00 schrieb Philipp Hufnagl:
> @@ -831,10 +855,27 @@ sub check_datastore_exists {
>  	    return 1;
>  	}
>      }
> -
>      die "$storeid: Cannot find datastore '$datastore', check permissions and existence!\n";
>  }
>  
> +sub check_namespace_exists {
> +    my ($class, $storeid, $scfg, $password, $conn) = @_;
> +
> +    my $datastore = $scfg->{datastore};
> +    my $namespace = $scfg->{namespace};
> +
> +    my $namespaces = eval { scan_namespaces($scfg, $datastore, $password) };

You're not actually re-using the connection, because it's not passed
along here.

Ideally, the fix would've been ordered first and the connection re-use
refactoring later. Like this, the fix cannot be applied independently of
all that.

Re: [pve-devel] [PATCH storage v2 4/4] pbs: fix #5008: Check if datastore and namespace is valid on add- and update hooks

[Philipp Hufnagl]

On 11/16/23 10:57, Fiona Ebner wrote:
> Am 15.11.23 um 17:00 schrieb Philipp Hufnagl:
>> @@ -831,10 +855,27 @@ sub check_datastore_exists {
>>  	    return 1;
>>  	}
>>      }
>> -
>>      die "$storeid: Cannot find datastore '$datastore', check permissions and existence!\n";
>>  }
>>  
>> +sub check_namespace_exists {
>> +    my ($class, $storeid, $scfg, $password, $conn) = @_;
>> +
>> +    my $datastore = $scfg->{datastore};
>> +    my $namespace = $scfg->{namespace};
>> +
>> +    my $namespaces = eval { scan_namespaces($scfg, $datastore, $password) };
> 
> You're not actually re-using the connection, because it's not passed
> along here.
> 
> Ideally, the fix would've been ordered first and the connection re-use
> refactoring later. Like this, the fix cannot be applied independently of
> all that.

Thanks for noticing.

I was trying to order the commits in a way that, while they are
changing the code, the should not impact the behavior of the program.
It should be possible to apply all previous commits without modifying
the behavior.

If it is easier or maintainers, I can attempt to implement the fix
first, then the clean up after

Re: [pve-devel] [PATCH storage v2 4/4] pbs: fix #5008: Check if datastore and namespace is valid on add- and update hooks

[Fiona Ebner]

Am 16.11.23 um 11:45 schrieb Philipp Hufnagl:
> 
> I was trying to order the commits in a way that, while they are
> changing the code, the should not impact the behavior of the program.
> It should be possible to apply all previous commits without modifying
> the behavior.
> 
> If it is easier or maintainers, I can attempt to implement the fix
> first, then the clean up after

IMHO, preparatory patches should be required for the fix or make it
easier to get to the fix. If it's an optimization that requires a few
changes, it's better done as a follow-up. Of course there are exceptions
to this. From my side, you don't have to go out of your way and re-do it
for this series.