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

[pve-devel] [PATCH storage v3 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 v2:
 * Typos
 * reuse connecton on one more place previously fortotten
 * simplify syntax

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 | 122 ++++++++++++++++++++++++-----------
 1 file changed, 84 insertions(+), 38 deletions(-)

-- 
2.39.2

[pve-devel] [PATCH storage v3 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 v3 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
changed 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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/PVE/Storage/PBSPlugin.pm b/src/PVE/Storage/PBSPlugin.pm
index 96373a4..b4d7914 100644
--- a/src/PVE/Storage/PBSPlugin.pm
+++ b/src/PVE/Storage/PBSPlugin.pm
@@ -808,9 +808,9 @@ sub status {
 #   fingerprint   (optional for trusted certs)
 # }
 sub scan_datastores {
-    my ($scfg, $password) = @_;
+    my ($scfg, $password, $conn) = @_;
 
-    my $conn = pbs_api_connect($scfg, $password);
+    $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] [PATCH storage v3 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 unneeded
overhead.

Therefore, 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 b4d7914..3e0e155 100644
--- a/src/PVE/Storage/PBSPlugin.pm
+++ b/src/PVE/Storage/PBSPlugin.pm
@@ -817,17 +817,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;
@@ -837,6 +833,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] [PATCH storage v3 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 | 41 ++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/src/PVE/Storage/PBSPlugin.pm b/src/PVE/Storage/PBSPlugin.pm
index 3e0e155..0797cec 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;
 }
 
@@ -817,6 +827,18 @@ sub scan_datastores {
 
     return $response;
 }
+
+sub scan_namespaces {
+    my ($scfg, $datastore, $password, $conn) = @_;
+
+    $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) = @_;
 
@@ -833,6 +855,25 @@ sub check_datastore_exists {
     die "$storeid: Cannot find datastore '$datastore', check permissions and existence!\n";
 }
 
+sub check_namespace_exists {
+    my ($class, $storeid, $scfg, $password, $conn) = @_;
+
+    my $namespace = $scfg->{namespace};
+    return 1 if !defined($namespace);
+    my $datastore = $scfg->{datastore};
+
+    my $namespaces = eval { scan_namespaces($scfg, $datastore, $password, $conn) };
+    die "$storeid: $@" if $@;
+
+    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 v3 0/4] pbs: fix #5008: Prevent adding pbs storage with invalid namespace

[Christian Ebner]

> On 16.11.2023 11:35 CET Philipp Hufnagl <p.hufnagl@proxmox.com> wrote:
> 
>  
> 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 v2:
>  * Typos
>  * reuse connecton on one more place previously fortotten
>  * simplify syntax
> 
> 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 | 122 ++++++++++++++++++++++++-----------
>  1 file changed, 84 insertions(+), 38 deletions(-)
> 
> -- 
> 2.39.2

Something is still broken, while testing with your patches applied I am not able to edit an existing PBS storage backend.

When I try to e.g. disable the storage via the WebUI I get the following error:
`update storage failed: PBS-local: Cannot find datastore '', check permissions and existence! (500)`
Without your patches applied everything works as expected again.

Cheers,
Chris

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

[Philipp Hufnagl]

On 11/21/23 13:16, Christian Ebner wrote:
>> On 16.11.2023 11:35 CET Philipp Hufnagl <p.hufnagl@proxmox.com> wrote:
>>
>>  
>> 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 v2:
>>  * Typos
>>  * reuse connecton on one more place previously fortotten
>>  * simplify syntax
>>
>> 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 | 122 ++++++++++++++++++++++++-----------
>>  1 file changed, 84 insertions(+), 38 deletions(-)
>>
>> -- 
>> 2.39.2
> 
> Something is still broken, while testing with your patches applied I am not able to edit an existing PBS storage backend.
> 
> When I try to e.g. disable the storage via the WebUI I get the following error:
> `update storage failed: PBS-local: Cannot find datastore '', check permissions and existence! (500)`
> Without your patches applied everything works as expected again.
> 
> Cheers,
> Chris

Hmmm...  thats very curios. I tried that on my end and it worked fine.

What datastore did you try to delete? Did you have a valid
configuration for it?

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

[Christian Ebner]

> On 21.11.2023 14:12 CET Philipp Hufnagl <p.hufnagl@proxmox.com> wrote:
> 
> 
> Hmmm...  thats very curios. I tried that on my end and it worked fine.
> 
> What datastore did you try to delete? Did you have a valid
> configuration for it?

Not deleting a datastore (that works fine), I was testing if I am able
to deactivate it (or edit it in general).

Datastore config is valid, and as stated without your patches applied
it works as expected. I just double checked with a freshly created
datastore, still got the same behavior.

Cheers,
Chris

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

[Philipp Hufnagl]

On 11/21/23 14:40, Christian Ebner wrote:
> 
>> On 21.11.2023 14:12 CET Philipp Hufnagl <p.hufnagl@proxmox.com> wrote:
>>
>>
>> Hmmm...  thats very curios. I tried that on my end and it worked fine.
>>
>> What datastore did you try to delete? Did you have a valid
>> configuration for it?
> 
> Not deleting a datastore (that works fine), I was testing if I am able
> to deactivate it (or edit it in general).
> 
> Datastore config is valid, and as stated without your patches applied
> it works as expected. I just double checked with a freshly created
> datastore, still got the same behavior.
> 
> Cheers,
> Chris

I have managed to reproduce it. Thank you

I look into it. Thank you