* [pve-devel] [PATCH storage v3 0/4] pbs: fix #5008: Prevent adding pbs storage with invalid namespace
@ 2023-11-16 10:35 Philipp Hufnagl
2023-11-16 10:35 ` [pve-devel] [PATCH storage v3 1/4] pbs: Move pbs_api_connect earlyer in the code Philipp Hufnagl
` (4 more replies)
0 siblings, 5 replies; 9+ messages in thread
From: Philipp Hufnagl @ 2023-11-16 10:35 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [pve-devel] [PATCH storage v3 1/4] pbs: Move pbs_api_connect earlyer in the code
2023-11-16 10:35 [pve-devel] [PATCH storage v3 0/4] pbs: fix #5008: Prevent adding pbs storage with invalid namespace Philipp Hufnagl
@ 2023-11-16 10:35 ` Philipp Hufnagl
2023-11-16 10:35 ` [pve-devel] [PATCH storage v3 2/4] pbs: Make it possible to reuse PBS connection for datastore API call Philipp Hufnagl
` (3 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Philipp Hufnagl @ 2023-11-16 10:35 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [pve-devel] [PATCH storage v3 2/4] pbs: Make it possible to reuse PBS connection for datastore API call
2023-11-16 10:35 [pve-devel] [PATCH storage v3 0/4] pbs: fix #5008: Prevent adding pbs storage with invalid namespace Philipp Hufnagl
2023-11-16 10:35 ` [pve-devel] [PATCH storage v3 1/4] pbs: Move pbs_api_connect earlyer in the code Philipp Hufnagl
@ 2023-11-16 10:35 ` Philipp Hufnagl
2023-11-16 10:35 ` [pve-devel] [PATCH storage v3 3/4] pbs: Extraxt check_datastore_exists from activate_storage Philipp Hufnagl
` (2 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Philipp Hufnagl @ 2023-11-16 10:35 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [pve-devel] [PATCH storage v3 3/4] pbs: Extraxt check_datastore_exists from activate_storage
2023-11-16 10:35 [pve-devel] [PATCH storage v3 0/4] pbs: fix #5008: Prevent adding pbs storage with invalid namespace Philipp Hufnagl
2023-11-16 10:35 ` [pve-devel] [PATCH storage v3 1/4] pbs: Move pbs_api_connect earlyer in the code Philipp Hufnagl
2023-11-16 10:35 ` [pve-devel] [PATCH storage v3 2/4] pbs: Make it possible to reuse PBS connection for datastore API call Philipp Hufnagl
@ 2023-11-16 10:35 ` Philipp Hufnagl
2023-11-16 10:35 ` [pve-devel] [PATCH storage v3 4/4] pbs: fix #5008: Check if datastore and namespace is valid on add- and update hooks Philipp Hufnagl
2023-11-21 12:16 ` [pve-devel] [PATCH storage v3 0/4] pbs: fix #5008: Prevent adding pbs storage with invalid namespace Christian Ebner
4 siblings, 0 replies; 9+ messages in thread
From: Philipp Hufnagl @ 2023-11-16 10:35 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [pve-devel] [PATCH storage v3 4/4] pbs: fix #5008: Check if datastore and namespace is valid on add- and update hooks
2023-11-16 10:35 [pve-devel] [PATCH storage v3 0/4] pbs: fix #5008: Prevent adding pbs storage with invalid namespace Philipp Hufnagl
` (2 preceding siblings ...)
2023-11-16 10:35 ` [pve-devel] [PATCH storage v3 3/4] pbs: Extraxt check_datastore_exists from activate_storage Philipp Hufnagl
@ 2023-11-16 10:35 ` Philipp Hufnagl
2023-11-21 12:16 ` [pve-devel] [PATCH storage v3 0/4] pbs: fix #5008: Prevent adding pbs storage with invalid namespace Christian Ebner
4 siblings, 0 replies; 9+ messages in thread
From: Philipp Hufnagl @ 2023-11-16 10:35 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [pve-devel] [PATCH storage v3 0/4] pbs: fix #5008: Prevent adding pbs storage with invalid namespace
2023-11-16 10:35 [pve-devel] [PATCH storage v3 0/4] pbs: fix #5008: Prevent adding pbs storage with invalid namespace Philipp Hufnagl
` (3 preceding siblings ...)
2023-11-16 10:35 ` [pve-devel] [PATCH storage v3 4/4] pbs: fix #5008: Check if datastore and namespace is valid on add- and update hooks Philipp Hufnagl
@ 2023-11-21 12:16 ` Christian Ebner
2023-11-21 13:12 ` Philipp Hufnagl
4 siblings, 1 reply; 9+ messages in thread
From: Christian Ebner @ 2023-11-21 12:16 UTC (permalink / raw)
To: Proxmox VE development discussion, Philipp Hufnagl
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [pve-devel] [PATCH storage v3 0/4] pbs: fix #5008: Prevent adding pbs storage with invalid namespace
2023-11-21 12:16 ` [pve-devel] [PATCH storage v3 0/4] pbs: fix #5008: Prevent adding pbs storage with invalid namespace Christian Ebner
@ 2023-11-21 13:12 ` Philipp Hufnagl
2023-11-21 13:40 ` Christian Ebner
0 siblings, 1 reply; 9+ messages in thread
From: Philipp Hufnagl @ 2023-11-21 13:12 UTC (permalink / raw)
To: Christian Ebner, Proxmox VE development discussion
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?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [pve-devel] [PATCH storage v3 0/4] pbs: fix #5008: Prevent adding pbs storage with invalid namespace
2023-11-21 13:12 ` Philipp Hufnagl
@ 2023-11-21 13:40 ` Christian Ebner
2023-11-21 14:13 ` Philipp Hufnagl
0 siblings, 1 reply; 9+ messages in thread
From: Christian Ebner @ 2023-11-21 13:40 UTC (permalink / raw)
To: Philipp Hufnagl, Proxmox VE development discussion
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [pve-devel] [PATCH storage v3 0/4] pbs: fix #5008: Prevent adding pbs storage with invalid namespace
2023-11-21 13:40 ` Christian Ebner
@ 2023-11-21 14:13 ` Philipp Hufnagl
0 siblings, 0 replies; 9+ messages in thread
From: Philipp Hufnagl @ 2023-11-21 14:13 UTC (permalink / raw)
To: Christian Ebner, Proxmox VE development discussion
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-11-21 14:13 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-11-16 10:35 [pve-devel] [PATCH storage v3 0/4] pbs: fix #5008: Prevent adding pbs storage with invalid namespace Philipp Hufnagl
2023-11-16 10:35 ` [pve-devel] [PATCH storage v3 1/4] pbs: Move pbs_api_connect earlyer in the code Philipp Hufnagl
2023-11-16 10:35 ` [pve-devel] [PATCH storage v3 2/4] pbs: Make it possible to reuse PBS connection for datastore API call Philipp Hufnagl
2023-11-16 10:35 ` [pve-devel] [PATCH storage v3 3/4] pbs: Extraxt check_datastore_exists from activate_storage Philipp Hufnagl
2023-11-16 10:35 ` [pve-devel] [PATCH storage v3 4/4] pbs: fix #5008: Check if datastore and namespace is valid on add- and update hooks Philipp Hufnagl
2023-11-21 12:16 ` [pve-devel] [PATCH storage v3 0/4] pbs: fix #5008: Prevent adding pbs storage with invalid namespace Christian Ebner
2023-11-21 13:12 ` Philipp Hufnagl
2023-11-21 13:40 ` Christian Ebner
2023-11-21 14:13 ` Philipp Hufnagl
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox