[pbs-devel] [PATCH proxmox-backup 0/4] fix #7219: align encryption key loading behavior for mount

[pbs-devel] [PATCH proxmox-backup 0/4] fix #7219: align encryption key loading behavior for mount

[Christian Ebner]

Align the proxmox-backup-client mount subcommand behavior to the
behavior of other subcommands with respect to encryption key loading.

Previously this did not load the encryption key from the default
location, if present.

While at it, also fix issues reported by cargo clippy in the first 3
patches of the series

Christian Ebner (4):
  datastore: fix clippy too many arguments warning
  tree-wide: fix clippy warnings needless borrow
  api: access: silence too may arguments warning on api handler
  fix #7219: client: mount: align encryption key loading behavior

 pbs-datastore/src/chunk_store.rs   | 48 +++++++++++++++++-------------
 pbs-datastore/src/datastore.rs     | 18 ++++++-----
 proxmox-backup-client/src/mount.rs | 24 ++++++++-------
 src/api2/access/user.rs            |  1 +
 src/api2/admin/datastore.rs        |  4 +--
 src/auth.rs                        |  2 +-
 6 files changed, 56 insertions(+), 41 deletions(-)

-- 
2.47.3



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup 1/4] datastore: fix clippy too many arguments warning

[Christian Ebner]

Introduce a transient CondSweepChunkParams type to limit the
function call arguments for the ChunkStore::cond_sweep_chunk()
method.

No functional changes.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/chunk_store.rs | 46 +++++++++++++++++++-------------
 pbs-datastore/src/datastore.rs   | 14 +++++-----
 2 files changed, 35 insertions(+), 25 deletions(-)

diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index 7fe09b914..cccfcfcdf 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -39,6 +39,16 @@ pub struct ChunkStore {
 
 // TODO: what about sysctl setting vm.vfs_cache_pressure (0 - 100) ?
 
+/// Transient type used to limit the number of function call parameters
+/// for ChunkStore::cond_sweep_chunk()
+pub(super) struct CondSweepChunkParams {
+    pub(super) atime: i64,
+    pub(super) min_atime: i64,
+    pub(super) oldest_writer: i64,
+    pub(super) size: u64,
+    pub(super) bad: bool,
+}
+
 pub fn verify_chunk_size(size: usize) -> Result<(), Error> {
     static SIZES: [usize; 7] = [
         64 * 1024,
@@ -443,11 +453,13 @@ impl ChunkStore {
 
                 unsafe {
                     self.cond_sweep_chunk(
-                        stat.st_atime,
-                        min_atime,
-                        oldest_writer,
-                        stat.st_size as u64,
-                        bad,
+                        CondSweepChunkParams {
+                            atime: stat.st_atime,
+                            min_atime,
+                            oldest_writer,
+                            size: stat.st_size as u64,
+                            bad,
+                        },
                         status,
                         || {
                             // non-bad S3 chunks need to be removed via cache
@@ -495,39 +507,35 @@ impl ChunkStore {
     /// FIXME: make this internal with further refactoring
     pub(super) unsafe fn cond_sweep_chunk<T: FnOnce() -> Result<(), Error>>(
         &self,
-        atime: i64,
-        min_atime: i64,
-        oldest_writer: i64,
-        size: u64,
-        bad: bool,
+        params: CondSweepChunkParams,
         gc_status: &mut GarbageCollectionStatus,
         remove_callback: T,
     ) -> Result<(), Error> {
-        if atime < min_atime {
+        if params.atime < params.min_atime {
             if let Err(err) = remove_callback() {
-                if bad {
+                if params.bad {
                     gc_status.still_bad += 1;
                 }
                 return Err(err);
             }
-            if bad {
+            if params.bad {
                 gc_status.removed_bad += 1;
             } else {
                 gc_status.removed_chunks += 1;
             }
-            gc_status.removed_bytes += size;
-        } else if atime < oldest_writer {
-            if bad {
+            gc_status.removed_bytes += params.size;
+        } else if params.atime < params.oldest_writer {
+            if params.bad {
                 gc_status.still_bad += 1;
             } else {
                 gc_status.pending_chunks += 1;
             }
-            gc_status.pending_bytes += size;
+            gc_status.pending_bytes += params.size;
         } else {
-            if !bad {
+            if !params.bad {
                 gc_status.disk_chunks += 1;
             }
-            gc_status.disk_bytes += size;
+            gc_status.disk_bytes += params.size;
         }
         Ok(())
     }
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 9c57aaac1..2f401f6fd 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -39,7 +39,7 @@ use pbs_config::BackupLockGuard;
 use crate::backup_info::{
     BackupDir, BackupGroup, BackupInfo, OLD_LOCKING, PROTECTED_MARKER_FILENAME,
 };
-use crate::chunk_store::ChunkStore;
+use crate::chunk_store::{ChunkStore, CondSweepChunkParams};
 use crate::dynamic_index::{DynamicIndexReader, DynamicIndexWriter};
 use crate::fixed_index::{FixedIndexReader, FixedIndexWriter};
 use crate::hierarchy::{ListGroups, ListGroupsType, ListNamespaces, ListNamespacesRecursive};
@@ -1765,11 +1765,13 @@ impl DataStore {
 
                         unsafe {
                             self.inner.chunk_store.cond_sweep_chunk(
-                                atime,
-                                min_atime,
-                                oldest_writer,
-                                content.size,
-                                bad,
+                                CondSweepChunkParams {
+                                    atime,
+                                    min_atime,
+                                    oldest_writer,
+                                    size: content.size,
+                                    bad,
+                                },
                                 &mut gc_status,
                                 || {
                                     if let Some(cache) = self.cache() {
-- 
2.47.3



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup 2/4] tree-wide: fix clippy warnings needless borrow

[Christian Ebner]

Omit all needless borrows currently reported by clippy run.

No functional changes.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/chunk_store.rs | 2 +-
 pbs-datastore/src/datastore.rs   | 4 ++--
 src/api2/admin/datastore.rs      | 4 ++--
 src/auth.rs                      | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index cccfcfcdf..315e18218 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -758,7 +758,7 @@ impl ChunkStore {
             let gid = pbs_config::backup_group()?.gid;
             create_options = create_options.owner(uid).group(gid);
         }
-        proxmox_sys::fs::replace_file(&path, &[], create_options, false)
+        proxmox_sys::fs::replace_file(path, &[], create_options, false)
     }
 
     /// Mark chunk as expected to be present by writing a file the chunk store.
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 2f401f6fd..83d209414 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -2007,7 +2007,7 @@ impl DataStore {
     ) -> Result<(bool, u64), Error> {
         match backend {
             DatastoreBackend::Filesystem => self.inner.chunk_store.insert_chunk(chunk, digest),
-            DatastoreBackend::S3(s3_client) => self.insert_chunk_cached(chunk, digest, &s3_client),
+            DatastoreBackend::S3(s3_client) => self.insert_chunk_cached(chunk, digest, s3_client),
         }
     }
 
@@ -2062,7 +2062,7 @@ impl DataStore {
         // or the chunk marker file exists on filesystem. The latter means the chunk has
         // been uploaded in the past, but was evicted from the LRU cache since but was not
         // cleaned up by garbage collection, so contained in the S3 object store.
-        if self.cache_contains(&digest) {
+        if self.cache_contains(digest) {
             tracing::info!("Skip upload of cached chunk {}", hex::encode(digest));
             return Ok((true, chunk_size));
         }
diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index ddd894c12..88ad5d53b 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -2734,8 +2734,8 @@ pub fn s3_refresh(store: String, rpcenv: &mut dyn RpcEnvironment) -> Result<Valu
 /// Performs an s3 refresh for given datastore. Expects the store to already be in maintenance mode
 /// s3-refresh.
 pub(crate) fn do_s3_refresh(store: &str, worker: &dyn WorkerTaskContext) -> Result<(), Error> {
-    let datastore = DataStore::lookup_datastore(&store, Some(Operation::Lookup))?;
-    run_maintenance_locked(&store, MaintenanceType::S3Refresh, worker, || {
+    let datastore = DataStore::lookup_datastore(store, Some(Operation::Lookup))?;
+    run_maintenance_locked(store, MaintenanceType::S3Refresh, worker, || {
         proxmox_async::runtime::block_on(datastore.s3_refresh())
     })
 }
diff --git a/src/auth.rs b/src/auth.rs
index a930d8cd9..24bb3f753 100644
--- a/src/auth.rs
+++ b/src/auth.rs
@@ -475,7 +475,7 @@ impl proxmox_auth_api::api::AuthContext for PbsAuthContext {
                             path_vec.push(part);
                         }
                     }
-                    user_info.check_privs(&auth_id, &path_vec, *privilege, false)?;
+                    user_info.check_privs(auth_id, &path_vec, *privilege, false)?;
                     return Ok(Some(true));
                 }
             }
-- 
2.47.3



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup 3/4] api: access: silence too may arguments warning on api handler

[Christian Ebner]

To keep the easy to read function signature for the api method.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/api2/access/user.rs | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/api2/access/user.rs b/src/api2/access/user.rs
index a8dd4c0d0..3b4cf1214 100644
--- a/src/api2/access/user.rs
+++ b/src/api2/access/user.rs
@@ -598,6 +598,7 @@ pub enum DeletableTokenProperty {
     },
 )]
 /// Update user's API token metadata
+#[allow(clippy::too_many_arguments)]
 pub fn update_token(
     userid: Userid,
     token_name: Tokenname,
-- 
2.47.3



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup 4/4] fix #7219: client: mount: align encryption key loading behavior

[Christian Ebner]

The mount subcommand currently does not load the encrypton key from
the default key location, requiring to explicitley set the `keyfile`
parameter on command invocation.

Align the behaviour to the rest of the client commands by using the
pbs_client::tools::key_source::crypto_parameters helper to load the
key instead.

The same current behaviour for the benchmark command is not touched,
as there loading the encryption key should always be conrolled by
explicitley setting it, to avoid possible pitfalls.

Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=7219
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 proxmox-backup-client/src/mount.rs | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/proxmox-backup-client/src/mount.rs b/proxmox-backup-client/src/mount.rs
index fa3385597..e815c8a9c 100644
--- a/proxmox-backup-client/src/mount.rs
+++ b/proxmox-backup-client/src/mount.rs
@@ -2,7 +2,7 @@ use std::collections::HashMap;
 use std::ffi::OsStr;
 use std::hash::BuildHasher;
 use std::os::unix::io::{AsRawFd, OwnedFd};
-use std::path::{Path, PathBuf};
+use std::path::Path;
 use std::sync::Arc;
 
 use anyhow::{bail, format_err, Error};
@@ -18,11 +18,13 @@ use proxmox_schema::*;
 use proxmox_sortable_macro::sortable;
 
 use pbs_api_types::{ArchiveType, BackupArchiveName, BackupNamespace};
-use pbs_client::tools::key_source::get_encryption_key_password;
+use pbs_client::tools::key_source::{
+    crypto_parameters, format_key_source, get_encryption_key_password,
+};
 use pbs_client::{BackupReader, RemoteChunkReader};
 use pbs_datastore::cached_chunk_reader::CachedChunkReader;
 use pbs_datastore::index::IndexFile;
-use pbs_key_config::load_and_decrypt_key;
+use pbs_key_config::decrypt_key;
 use pbs_tools::crypt_config::CryptConfig;
 use pbs_tools::json::required_string_param;
 
@@ -208,14 +210,16 @@ async fn mount_do(param: Value, pipe: Option<OwnedFd>) -> Result<Value, Error> {
     let path = required_string_param(&param, "snapshot")?;
     let backup_dir = dir_or_last_from_group(&client, &repo, &backup_ns, path).await?;
 
-    let keyfile = param["keyfile"].as_str().map(PathBuf::from);
-    let crypt_config = match keyfile {
+    let crypto = crypto_parameters(&param)?;
+
+    let crypt_config = match crypto.enc_key {
         None => None,
-        Some(path) => {
-            log::info!("Encryption key file: '{:?}'", path);
-            let (key, _, fingerprint) = load_and_decrypt_key(&path, &get_encryption_key_password)?;
-            log::info!("Encryption key fingerprint: '{}'", fingerprint);
-            Some(Arc::new(CryptConfig::new(key)?))
+        Some(key) => {
+            log::info!("{}", format_key_source(&key.source, "encryption"));
+            let (key, _created, fingerprint) = decrypt_key(&key.key, &get_encryption_key_password)?;
+            log::info!("Encryption key fingerprint: '{fingerprint}'");
+            let crypt_config = CryptConfig::new(key)?;
+            Some(Arc::new(crypt_config))
         }
     };
 
-- 
2.47.3



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup 4/4] fix #7219: client: mount: align encryption key loading behavior

[Daniel Herzig]

Thanks -- I just gave this a quick spin and works as expected here (no 
need to pass the default key location anymore).

Tested-by: Daniel Herzig <d.herzig@proxmox.com>

On 1/8/26 4:25 PM, Christian Ebner wrote:
> The mount subcommand currently does not load the encrypton key from
> the default key location, requiring to explicitley set the `keyfile`
> parameter on command invocation.
>
> Align the behaviour to the rest of the client commands by using the
> pbs_client::tools::key_source::crypto_parameters helper to load the
> key instead.
>
> The same current behaviour for the benchmark command is not touched,
> as there loading the encryption key should always be conrolled by
> explicitley setting it, to avoid possible pitfalls.
>
> Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=7219
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
>   proxmox-backup-client/src/mount.rs | 24 ++++++++++++++----------
>   1 file changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/proxmox-backup-client/src/mount.rs b/proxmox-backup-client/src/mount.rs
> index fa3385597..e815c8a9c 100644
> --- a/proxmox-backup-client/src/mount.rs
> +++ b/proxmox-backup-client/src/mount.rs
> @@ -2,7 +2,7 @@ use std::collections::HashMap;
>   use std::ffi::OsStr;
>   use std::hash::BuildHasher;
>   use std::os::unix::io::{AsRawFd, OwnedFd};
> -use std::path::{Path, PathBuf};
> +use std::path::Path;
>   use std::sync::Arc;
>   
>   use anyhow::{bail, format_err, Error};
> @@ -18,11 +18,13 @@ use proxmox_schema::*;
>   use proxmox_sortable_macro::sortable;
>   
>   use pbs_api_types::{ArchiveType, BackupArchiveName, BackupNamespace};
> -use pbs_client::tools::key_source::get_encryption_key_password;
> +use pbs_client::tools::key_source::{
> +    crypto_parameters, format_key_source, get_encryption_key_password,
> +};
>   use pbs_client::{BackupReader, RemoteChunkReader};
>   use pbs_datastore::cached_chunk_reader::CachedChunkReader;
>   use pbs_datastore::index::IndexFile;
> -use pbs_key_config::load_and_decrypt_key;
> +use pbs_key_config::decrypt_key;
>   use pbs_tools::crypt_config::CryptConfig;
>   use pbs_tools::json::required_string_param;
>   
> @@ -208,14 +210,16 @@ async fn mount_do(param: Value, pipe: Option<OwnedFd>) -> Result<Value, Error> {
>       let path = required_string_param(&param, "snapshot")?;
>       let backup_dir = dir_or_last_from_group(&client, &repo, &backup_ns, path).await?;
>   
> -    let keyfile = param["keyfile"].as_str().map(PathBuf::from);
> -    let crypt_config = match keyfile {
> +    let crypto = crypto_parameters(&param)?;
> +
> +    let crypt_config = match crypto.enc_key {
>           None => None,
> -        Some(path) => {
> -            log::info!("Encryption key file: '{:?}'", path);
> -            let (key, _, fingerprint) = load_and_decrypt_key(&path, &get_encryption_key_password)?;
> -            log::info!("Encryption key fingerprint: '{}'", fingerprint);
> -            Some(Arc::new(CryptConfig::new(key)?))
> +        Some(key) => {
> +            log::info!("{}", format_key_source(&key.source, "encryption"));
> +            let (key, _created, fingerprint) = decrypt_key(&key.key, &get_encryption_key_password)?;
> +            log::info!("Encryption key fingerprint: '{fingerprint}'");
> +            let crypt_config = CryptConfig::new(key)?;
> +            Some(Arc::new(crypt_config))
>           }
>       };
>   


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel