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

[pve-devel] [PATCH storage v4 0/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

Since the server address has was not included on for the on_update_hook,
I had to slightly modify src/PVE/API2/Storage/Config.pm. I checked and
are pretty sure that this modification does not break anything, however
a second look would be appreciated!

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

Changes since v3:
 * Modify src/PVE/API2/Storage/Config.pm to add server address

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/API2/Storage/Config.pm |   4 +-
 src/PVE/Storage/PBSPlugin.pm   | 122 +++++++++++++++++++++++----------
 2 files changed, 86 insertions(+), 40 deletions(-)

-- 
2.39.2

[pve-devel] [PATCH storage v4 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

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

[Fiona Ebner]

Am 27.11.23 um 12:39 schrieb Philipp Hufnagl:
> Because it is needed later in this patch series, the method
> pbs_api_connect is moved earlyer in the code

Typo: /earlyer/earlier/ (also in the title)

> 
> 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);
>  }
>  

I don't think this is the best place to move it to, because it's now
beteween the helpers for password and the helpers for the encryption
key. I'd like to have it either before or after those password and key
related helpers.

> +#

Nit: adds an extra #

> +# TODO: use a client with native rust/proxmox-backup bindings to profit from
> +# API schema checks and types

[pve-devel] [PATCH storage v4 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

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

[Fiona Ebner]

Am 27.11.23 um 12:39 schrieb 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.
> 

I'm no fan of the second sentence. You do change it with the patch
without breaking existing code ;)
I'd rather just mention that this is in preparation for a call site
where the connection will be re-used.

> 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);
>  

Style nit: using '//=' would be slightly shorter/more straightforward.

>      my $response = eval { $conn->get('/api2/json/admin/datastore', {}) };
>      die "error fetching datastores - $@" if $@;

[pve-devel] [PATCH storage v4 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

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

[Fiona Ebner]

Am 27.11.23 um 12:40 schrieb 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;
>  }
> -

Style nit: there is no blank line between the subs anymore

> -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";
>  }
>  
Since the function dies, it should rather be named assert_ and not check_

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

[Fiona Ebner]

Am 27.11.23 um 12:40 schrieb 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.
> 

Actually, maybe we can do without the overhead, which would also avoid
the need for making the connection in scan_datastores() optional. In
activate_storage(), we could do

if there is a namespace in scfg
  assert_namespace_exists
else
  assert_datastore_exists

because for a nonexistent datastore, the API call querying the
namespaces will still fail with a nice "no such datastore 'nonexistent'"
error, so we'd still catch that without making two API calls.

Not sure if we should even add additional checks in the on_{add,update}
then.

What do you (or other devs) think?

[pve-devel] [PATCH storage v4 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.

Because the server address is needed to check the namespaces in the
update hook, it has to be included in the Storage/Config.pm.

Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
---
 src/PVE/API2/Storage/Config.pm |  4 ++--
 src/PVE/Storage/PBSPlugin.pm   | 41 ++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/src/PVE/API2/Storage/Config.pm b/src/PVE/API2/Storage/Config.pm
index e04b6ab..651d4bc 100755
--- a/src/PVE/API2/Storage/Config.pm
+++ b/src/PVE/API2/Storage/Config.pm
@@ -362,12 +362,12 @@ __PACKAGE__->register_method ({
 		}
 	    }
 
-	    $returned_config = $plugin->on_update_hook($storeid, $opts, %$sensitive);
-
 	    for my $k (keys %$opts) {
 		$scfg->{$k} = $opts->{$k};
 	    }
 
+	    $returned_config = $plugin->on_update_hook($storeid, $scfg, %$sensitive);
+
 	    if (defined($scfg->{mkdir})) { # TODO: remove complete option in Proxmox VE 9
 		warn "NOTE: The 'mkdir' option set for '${storeid}' is deprecated and will be removed"
 		    ." in Proxmox VE 9. Use 'create-base-path' or 'create-subdirs' instead.\n"
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 v4 4/4] pbs: fix #5008: Check if datastore and namespace is valid on add- and update hooks

[Fiona Ebner]

Am 27.11.23 um 12:40 schrieb 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.
> 
> Because the server address is needed to check the namespaces in the
> update hook, it has to be included in the Storage/Config.pm.

Rather than "in the Storage/Config.pm" I suppose you mean "in the

> 
> Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
> ---
>  src/PVE/API2/Storage/Config.pm |  4 ++--
>  src/PVE/Storage/PBSPlugin.pm   | 41 ++++++++++++++++++++++++++++++++++
>  2 files changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/src/PVE/API2/Storage/Config.pm b/src/PVE/API2/Storage/Config.pm
> index e04b6ab..651d4bc 100755
> --- a/src/PVE/API2/Storage/Config.pm
> +++ b/src/PVE/API2/Storage/Config.pm
> @@ -362,12 +362,12 @@ __PACKAGE__->register_method ({
>  		}
>  	    }
>  
> -	    $returned_config = $plugin->on_update_hook($storeid, $opts, %$sensitive);
> -
>  	    for my $k (keys %$opts) {
>  		$scfg->{$k} = $opts->{$k};
>  	    }
>  
> +	    $returned_config = $plugin->on_update_hook($storeid, $scfg, %$sensitive);
> +

This hunk should be its own patch.

Hmm, could be fine in practice, but there is potential for breakage.
Existing (third-party) plugins might be relying on the fact that the
hook is only called with updated values somehow?

On the other hand, currently our CIFS plugin prints a warning that the
updated password will be ignored, because no user is set if the user is
not also updated at same time but does exist in the current config.
Luckily the actual setting of the credentials is not affected. This
change would correct this.

>  	    if (defined($scfg->{mkdir})) { # TODO: remove complete option in Proxmox VE 9
>  		warn "NOTE: The 'mkdir' option set for '${storeid}' is deprecated and will be removed"
>  		    ." in Proxmox VE 9. Use 'create-base-path' or 'create-subdirs' instead.\n"

---snip---

> @@ -833,6 +855,25 @@ sub check_datastore_exists {
>      die "$storeid: Cannot find datastore '$datastore', check permissions and existence!\n";
>  }
>  
> +sub check_namespace_exists {

Because it dies when the namespace doesn't exist, I'd prefer to call it
assert_ rather than check_

> +    my ($class, $storeid, $scfg, $password, $conn) = @_;
> +
> +    my $namespace = $scfg->{namespace};
> +    return 1 if !defined($namespace);
> +    my $datastore = $scfg->{datastore};

Style nit: rather than having this one-time-use variable, please use
$scfg->{datastore} directly below.

> +
> +    my $namespaces = eval { scan_namespaces($scfg, $datastore, $password, $conn) };
> +    die "$storeid: $@" if $@;
> +
> +    for my $ns (@$namespaces) {
> +	if ($ns->{ns} eq $namespace) {
> +	    return 1;
> +	}

Style nit: would save two lines using post-if

> +    }
> +
> +    die "$storeid: Cannot find namespace '$namespace', check permissions and existence!\n";
> +}
> +
>  sub activate_storage {
>      my ($class, $storeid, $scfg, $cache) = @_;
>