[pbs-devel] [PATCH backup] pbs-client: read credentials from $CREDENTIALS_DIRECTORY

[pbs-devel] [PATCH backup] pbs-client: read credentials from $CREDENTIALS_DIRECTORY

[Maximiliano Sandoval]

Allows to load credentials passed down by systemd. A possible use-case
is safely storing the server's password in a file encrypted by the
systems TPM, e.g. via

```
systemd-ask-password -n | systemd-creds encrypt --name=pbs-password - my-api-token.cred
```

which then can be used via

```
systemd-run --pipe --wait --property=LoadCredentialEncrypted=pbs-password:my-api-token.cred \
proxmox-backup-client ...
```

or from inside a service.

Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---
 pbs-client/src/tools/key_source.rs |  4 +-
 pbs-client/src/tools/mod.rs        | 81 ++++++++++++++++++++++++++++--
 2 files changed, 80 insertions(+), 5 deletions(-)

diff --git a/pbs-client/src/tools/key_source.rs b/pbs-client/src/tools/key_source.rs
index 7968e0c2a..94b86e8b6 100644
--- a/pbs-client/src/tools/key_source.rs
+++ b/pbs-client/src/tools/key_source.rs
@@ -345,8 +345,8 @@ pub(crate) unsafe fn set_test_default_master_pubkey(value: Result<Option<Vec<u8>
 pub fn get_encryption_key_password() -> Result<Vec<u8>, Error> {
     // fixme: implement other input methods
 
-    if let Some(password) = super::get_secret_from_env("PBS_ENCRYPTION_PASSWORD")? {
-        return Ok(password.as_bytes().to_vec());
+    if let Some(password) = super::get_encryption_password()? {
+        return Ok(password);
     }
 
     // If we're on a TTY, query the user for a password
diff --git a/pbs-client/src/tools/mod.rs b/pbs-client/src/tools/mod.rs
index 8068dc004..8b0392744 100644
--- a/pbs-client/src/tools/mod.rs
+++ b/pbs-client/src/tools/mod.rs
@@ -2,7 +2,7 @@
 use std::collections::HashMap;
 use std::env::VarError::{NotPresent, NotUnicode};
 use std::fs::File;
-use std::io::{BufRead, BufReader};
+use std::io::{BufRead, BufReader, Read};
 use std::os::unix::fs::OpenOptionsExt;
 use std::os::unix::io::FromRawFd;
 use std::process::Command;
@@ -28,6 +28,16 @@ pub mod key_source;
 
 const ENV_VAR_PBS_FINGERPRINT: &str = "PBS_FINGERPRINT";
 const ENV_VAR_PBS_PASSWORD: &str = "PBS_PASSWORD";
+const ENV_VAR_PBS_ENCRYPTION_PASSWORD: &str = "PBS_ENCRYPTION_PASSWORD";
+
+/// Directory with system [credential]s. See systemd-creds(1).
+///
+/// [credential]: https://systemd.io/CREDENTIALS/
+const CREDENTIALS_DIRECTORY: &str = "CREDENTIALS_DIRECTORY";
+/// Credential name of the encryption password.
+const CRED_PBS_ENCRYPTION_PASSWORD: &str = "pbs-encryption-password";
+/// Credential name of the the password.
+const CRED_PBS_PASSWORD: &str = "pbs-password";
 
 pub const REPO_URL_SCHEMA: Schema = StringSchema::new("Repository URL.")
     .format(&BACKUP_REPO_URL)
@@ -40,6 +50,43 @@ pub const CHUNK_SIZE_SCHEMA: Schema = IntegerSchema::new("Chunk size in KB. Must
     .default(4096)
     .schema();
 
+/// Retrieves a secret stored in a [credential] provided by systemd.
+///
+/// Returns `None` if the credential does not exist or on errors.
+///
+/// [credential]: https://systemd.io/CREDENTIALS/
+pub fn get_credential(cred_name: &str) -> Option<Vec<u8>> {
+    // This is fundamentally a fn that returns a Result, but in practice we
+    // would ignore errors so we hide the result in the public API.
+    fn get_credential_inner(cred_name: &str) -> Result<Option<Vec<u8>>, Error> {
+        let Some(creds_dir) = std::env::var_os(CREDENTIALS_DIRECTORY) else {
+            return Ok(None);
+        };
+        let path = std::path::Path::new(&creds_dir).join(cred_name);
+
+        let mut file = File::open(&path)?;
+        let mut buffer = vec![];
+
+        // We read the whole contents without a BufRead. As per
+        // systemd-creds(1): Credentials are limited-size binary or textual
+        // objects.
+        file.read_to_end(&mut buffer)?;
+
+        Ok(Some(buffer))
+    }
+    get_credential_inner(cred_name)
+        .inspect_err(|err| {
+            if err
+                .downcast_ref::<std::io::Error>()
+                .is_some_and(|e| e.kind() != std::io::ErrorKind::NotFound)
+            {
+                proxmox_log::error!("{err:#}")
+            }
+        })
+        .ok()
+        .flatten()
+}
+
 /// Helper to read a secret through a environment variable (ENV).
 ///
 /// Tries the following variable names in order and returns the value
@@ -118,6 +165,34 @@ pub fn get_secret_from_env(base_name: &str) -> Result<Option<String>, Error> {
     Ok(None)
 }
 
+/// Gets the backup server's password.
+///
+/// We first try reading from the `PBS_PASSWORD` environment variable, then we
+/// try reading from the `pbs-password` [credential]. We ignore errors on the
+/// latter.
+///
+/// [credential]: https://systemd.io/CREDENTIALS/
+pub fn get_password() -> Result<Option<String>, Error> {
+    let password = get_secret_from_env(ENV_VAR_PBS_PASSWORD)?.or_else(|| {
+        get_credential(CRED_PBS_PASSWORD).and_then(|bytes| String::from_utf8(bytes).ok())
+    });
+    Ok(password)
+}
+
+/// Gets an encryption password.
+///
+/// We first try reading from the `PBS_ENCRYPTION_PASSWORD` environment
+/// variable, then we try reading from the `pbs-encryption-password`
+/// [credential]. We ignore errors on the latter.
+///
+/// [credential]: https://systemd.io/CREDENTIALS/
+pub fn get_encryption_password() -> Result<Option<Vec<u8>>, Error> {
+    let password = get_secret_from_env(ENV_VAR_PBS_ENCRYPTION_PASSWORD)?
+        .map(String::into_bytes)
+        .or_else(|| get_credential(CRED_PBS_ENCRYPTION_PASSWORD));
+    Ok(password)
+}
+
 pub fn get_default_repository() -> Option<String> {
     std::env::var("PBS_REPOSITORY").ok()
 }
@@ -181,7 +256,7 @@ fn connect_do(
 ) -> Result<HttpClient, Error> {
     let fingerprint = std::env::var(ENV_VAR_PBS_FINGERPRINT).ok();
 
-    let password = get_secret_from_env(ENV_VAR_PBS_PASSWORD)?;
+    let password = get_password()?;
     let options = HttpClientOptions::new_interactive(password, fingerprint).rate_limit(rate_limit);
 
     HttpClient::new(server, port, auth_id, options)
@@ -190,7 +265,7 @@ fn connect_do(
 /// like get, but simply ignore errors and return Null instead
 pub async fn try_get(repo: &BackupRepository, url: &str) -> Value {
     let fingerprint = std::env::var(ENV_VAR_PBS_FINGERPRINT).ok();
-    let password = get_secret_from_env(ENV_VAR_PBS_PASSWORD).unwrap_or(None);
+    let password = get_password().unwrap_or(None);
 
     // ticket cache, but no questions asked
     let options = HttpClientOptions::new_interactive(password, fingerprint).interactive(false);
-- 
2.39.5



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

Re: [pbs-devel] [PATCH backup] pbs-client: read credentials from $CREDENTIALS_DIRECTORY

[Wolfgang Bumiller]

On Mon, Mar 24, 2025 at 01:35:42PM +0100, Maximiliano Sandoval wrote:
> Allows to load credentials passed down by systemd. A possible use-case
> is safely storing the server's password in a file encrypted by the
> systems TPM, e.g. via
> 
> ```
> systemd-ask-password -n | systemd-creds encrypt --name=pbs-password - my-api-token.cred
> ```
> 
> which then can be used via
> 
> ```
> systemd-run --pipe --wait --property=LoadCredentialEncrypted=pbs-password:my-api-token.cred \
> proxmox-backup-client ...
> ```
> 
> or from inside a service.
> 
> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> ---
>  pbs-client/src/tools/key_source.rs |  4 +-
>  pbs-client/src/tools/mod.rs        | 81 ++++++++++++++++++++++++++++--
>  2 files changed, 80 insertions(+), 5 deletions(-)
> 
> diff --git a/pbs-client/src/tools/key_source.rs b/pbs-client/src/tools/key_source.rs
> index 7968e0c2a..94b86e8b6 100644
> --- a/pbs-client/src/tools/key_source.rs
> +++ b/pbs-client/src/tools/key_source.rs
> @@ -345,8 +345,8 @@ pub(crate) unsafe fn set_test_default_master_pubkey(value: Result<Option<Vec<u8>
>  pub fn get_encryption_key_password() -> Result<Vec<u8>, Error> {
>      // fixme: implement other input methods
>  
> -    if let Some(password) = super::get_secret_from_env("PBS_ENCRYPTION_PASSWORD")? {
> -        return Ok(password.as_bytes().to_vec());
> +    if let Some(password) = super::get_encryption_password()? {
> +        return Ok(password);
>      }
>  
>      // If we're on a TTY, query the user for a password
> diff --git a/pbs-client/src/tools/mod.rs b/pbs-client/src/tools/mod.rs
> index 8068dc004..8b0392744 100644
> --- a/pbs-client/src/tools/mod.rs
> +++ b/pbs-client/src/tools/mod.rs
> @@ -2,7 +2,7 @@
>  use std::collections::HashMap;
>  use std::env::VarError::{NotPresent, NotUnicode};
>  use std::fs::File;
> -use std::io::{BufRead, BufReader};
> +use std::io::{BufRead, BufReader, Read};
>  use std::os::unix::fs::OpenOptionsExt;
>  use std::os::unix::io::FromRawFd;
>  use std::process::Command;
> @@ -28,6 +28,16 @@ pub mod key_source;
>  
>  const ENV_VAR_PBS_FINGERPRINT: &str = "PBS_FINGERPRINT";
>  const ENV_VAR_PBS_PASSWORD: &str = "PBS_PASSWORD";
> +const ENV_VAR_PBS_ENCRYPTION_PASSWORD: &str = "PBS_ENCRYPTION_PASSWORD";
> +
> +/// Directory with system [credential]s. See systemd-creds(1).
> +///
> +/// [credential]: https://systemd.io/CREDENTIALS/
> +const CREDENTIALS_DIRECTORY: &str = "CREDENTIALS_DIRECTORY";

^ The other env var consts have an `ENV_VAR_` prefix, so we should add
that here for consistency.

> +/// Credential name of the encryption password.
> +const CRED_PBS_ENCRYPTION_PASSWORD: &str = "pbs-encryption-password";
> +/// Credential name of the the password.
> +const CRED_PBS_PASSWORD: &str = "pbs-password";
>  
>  pub const REPO_URL_SCHEMA: Schema = StringSchema::new("Repository URL.")
>      .format(&BACKUP_REPO_URL)
> @@ -40,6 +50,43 @@ pub const CHUNK_SIZE_SCHEMA: Schema = IntegerSchema::new("Chunk size in KB. Must
>      .default(4096)
>      .schema();
>  
> +/// Retrieves a secret stored in a [credential] provided by systemd.
> +///
> +/// Returns `None` if the credential does not exist or on errors.
> +///
> +/// [credential]: https://systemd.io/CREDENTIALS/
> +pub fn get_credential(cred_name: &str) -> Option<Vec<u8>> {

I think *failure* to read an *existing* credential should actually be
propagated up. We don't want to just ignore this.

> +    // This is fundamentally a fn that returns a Result, but in practice we
> +    // would ignore errors so we hide the result in the public API.
> +    fn get_credential_inner(cred_name: &str) -> Result<Option<Vec<u8>>, Error> {
> +        let Some(creds_dir) = std::env::var_os(CREDENTIALS_DIRECTORY) else {
> +            return Ok(None);
> +        };
> +        let path = std::path::Path::new(&creds_dir).join(cred_name);
> +
> +        let mut file = File::open(&path)?;
> +        let mut buffer = vec![];
> +
> +        // We read the whole contents without a BufRead. As per
> +        // systemd-creds(1): Credentials are limited-size binary or textual
> +        // objects.
> +        file.read_to_end(&mut buffer)?;
> +
> +        Ok(Some(buffer))
> +    }
> +    get_credential_inner(cred_name)
> +        .inspect_err(|err| {
> +            if err
> +                .downcast_ref::<std::io::Error>()
> +                .is_some_and(|e| e.kind() != std::io::ErrorKind::NotFound)
> +            {
> +                proxmox_log::error!("{err:#}")
> +            }
> +        })
> +        .ok()
> +        .flatten()

I think this entire fn could be reduced to a std::fs::read() call with
the path included in the logged error, and maybe some more debug-logging
so that with debug-logging the log doesnt' stay empty on eg. ENOENT.

    let Some(creds_dir) = std::env::var_os(ENV_VAR_CREDENTIALS_DIRECTORY) else {
        return Ok(None);
    }
    let path = Path::new(creds_dir).join(cred_name);
    debug!("attempting to use credential {cred_name:?} from {creds_dir:?}");
    match std::fs::read(&path) {
        Ok(data) => Ok(Some(data)),
        Err(err) if err.kind() == io::ErrorKind::NotFound => {
            debug!("no {cred_name:?} credential found in {creds_dir:?}");
            Ok(None)
        }
        Err(err) => Err(err),
    }

> +}
> +
>  /// Helper to read a secret through a environment variable (ENV).
>  ///
>  /// Tries the following variable names in order and returns the value
> @@ -118,6 +165,34 @@ pub fn get_secret_from_env(base_name: &str) -> Result<Option<String>, Error> {
>      Ok(None)
>  }
>  
> +/// Gets the backup server's password.
> +///
> +/// We first try reading from the `PBS_PASSWORD` environment variable, then we
> +/// try reading from the `pbs-password` [credential]. We ignore errors on the
> +/// latter.
> +///
> +/// [credential]: https://systemd.io/CREDENTIALS/
> +pub fn get_password() -> Result<Option<String>, Error> {
> +    let password = get_secret_from_env(ENV_VAR_PBS_PASSWORD)?.or_else(|| {
> +        get_credential(CRED_PBS_PASSWORD).and_then(|bytes| String::from_utf8(bytes).ok())

A potential UTF-8 error here should be logged here!

I think the function would be a bit more readable with a simple

    if let Some(pw) = get_secret_from_env(...)? {
        return Ok(pw);
    }
    if let Some(pw) = get_credential(...)? {
        return String::from_utf8(pw)
            .map_err(|_| format_err!("non-utf8 password credential"));
    }

> +    });
> +    Ok(password)
> +}
> +
> +/// Gets an encryption password.
> +///
> +/// We first try reading from the `PBS_ENCRYPTION_PASSWORD` environment
> +/// variable, then we try reading from the `pbs-encryption-password`
> +/// [credential]. We ignore errors on the latter.
> +///
> +/// [credential]: https://systemd.io/CREDENTIALS/
> +pub fn get_encryption_password() -> Result<Option<Vec<u8>>, Error> {
> +    let password = get_secret_from_env(ENV_VAR_PBS_ENCRYPTION_PASSWORD)?
> +        .map(String::into_bytes)
> +        .or_else(|| get_credential(CRED_PBS_ENCRYPTION_PASSWORD));

This and `get_password` can just use the same helper function. They both
only support utf-8 strings anyway from what I can tell.
So this would just be

    get_secret_impl(ENV_VAR_PBS_ENCRYPTION_PASSWORD, CRED_PBS_ENCRYPTION_PASSWORD)
        .map(String::into_bytes)

> +    Ok(password)
> +}
> +
>  pub fn get_default_repository() -> Option<String> {
>      std::env::var("PBS_REPOSITORY").ok()
>  }
> @@ -181,7 +256,7 @@ fn connect_do(
>  ) -> Result<HttpClient, Error> {
>      let fingerprint = std::env::var(ENV_VAR_PBS_FINGERPRINT).ok();
>  
> -    let password = get_secret_from_env(ENV_VAR_PBS_PASSWORD)?;
> +    let password = get_password()?;
>      let options = HttpClientOptions::new_interactive(password, fingerprint).rate_limit(rate_limit);
>  
>      HttpClient::new(server, port, auth_id, options)
> @@ -190,7 +265,7 @@ fn connect_do(
>  /// like get, but simply ignore errors and return Null instead
>  pub async fn try_get(repo: &BackupRepository, url: &str) -> Value {
>      let fingerprint = std::env::var(ENV_VAR_PBS_FINGERPRINT).ok();
> -    let password = get_secret_from_env(ENV_VAR_PBS_PASSWORD).unwrap_or(None);
> +    let password = get_password().unwrap_or(None);
>  
>      // ticket cache, but no questions asked
>      let options = HttpClientOptions::new_interactive(password, fingerprint).interactive(false);
> -- 
> 2.39.5


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

Re: [pbs-devel] [PATCH backup] pbs-client: read credentials from $CREDENTIALS_DIRECTORY

[Maximiliano Sandoval]

Wolfgang Bumiller <w.bumiller@proxmox.com> writes:

>>  const ENV_VAR_PBS_FINGERPRINT: &str = "PBS_FINGERPRINT";
>>  const ENV_VAR_PBS_PASSWORD: &str = "PBS_PASSWORD";
>> +const ENV_VAR_PBS_ENCRYPTION_PASSWORD: &str = "PBS_ENCRYPTION_PASSWORD";
>> +
>> +/// Directory with system [credential]s. See systemd-creds(1).
>> +///
>> +/// [credential]: https://systemd.io/CREDENTIALS/
>> +const CREDENTIALS_DIRECTORY: &str = "CREDENTIALS_DIRECTORY";
>
> ^ The other env var consts have an `ENV_VAR_` prefix, so we should add
> that here for consistency.
>
>> +/// Credential name of the encryption password.
>> +const CRED_PBS_ENCRYPTION_PASSWORD: &str = "pbs-encryption-password";
>> +/// Credential name of the the password.
>> +const CRED_PBS_PASSWORD: &str = "pbs-password";
>>
>>  pub const REPO_URL_SCHEMA: Schema = StringSchema::new("Repository URL.")
>>      .format(&BACKUP_REPO_URL)
>> @@ -40,6 +50,43 @@ pub const CHUNK_SIZE_SCHEMA: Schema = IntegerSchema::new("Chunk size in KB. Must
>>      .default(4096)
>>      .schema();
>>
>> +/// Retrieves a secret stored in a [credential] provided by systemd.
>> +///
>> +/// Returns `None` if the credential does not exist or on errors.
>> +///
>> +/// [credential]: https://systemd.io/CREDENTIALS/
>> +pub fn get_credential(cred_name: &str) -> Option<Vec<u8>> {
>
> I think *failure* to read an *existing* credential should actually be
> propagated up. We don't want to just ignore this.
>
>> +    // This is fundamentally a fn that returns a Result, but in practice we
>> +    // would ignore errors so we hide the result in the public API.
>> +    fn get_credential_inner(cred_name: &str) -> Result<Option<Vec<u8>>, Error> {
>> +        let Some(creds_dir) = std::env::var_os(CREDENTIALS_DIRECTORY) else {
>> +            return Ok(None);
>> +        };
>> +        let path = std::path::Path::new(&creds_dir).join(cred_name);
>> +
>> +        let mut file = File::open(&path)?;
>> +        let mut buffer = vec![];
>> +
>> +        // We read the whole contents without a BufRead. As per
>> +        // systemd-creds(1): Credentials are limited-size binary or textual
>> +        // objects.
>> +        file.read_to_end(&mut buffer)?;
>> +
>> +        Ok(Some(buffer))
>> +    }
>> +    get_credential_inner(cred_name)
>> +        .inspect_err(|err| {
>> +            if err
>> +                .downcast_ref::<std::io::Error>()
>> +                .is_some_and(|e| e.kind() != std::io::ErrorKind::NotFound)
>> +            {
>> +                proxmox_log::error!("{err:#}")
>> +            }
>> +        })
>> +        .ok()
>> +        .flatten()
>
> I think this entire fn could be reduced to a std::fs::read() call with
> the path included in the logged error, and maybe some more debug-logging
> so that with debug-logging the log doesnt' stay empty on eg. ENOENT.
>
>     let Some(creds_dir) = std::env::var_os(ENV_VAR_CREDENTIALS_DIRECTORY) else {
>         return Ok(None);
>     }
>     let path = Path::new(creds_dir).join(cred_name);
>     debug!("attempting to use credential {cred_name:?} from {creds_dir:?}");
>     match std::fs::read(&path) {
>         Ok(data) => Ok(Some(data)),
>         Err(err) if err.kind() == io::ErrorKind::NotFound => {
>             debug!("no {cred_name:?} credential found in {creds_dir:?}");
>             Ok(None)
>         }
>         Err(err) => Err(err),
>     }
>
>> +}
>> +
>>  /// Helper to read a secret through a environment variable (ENV).
>>  ///
>>  /// Tries the following variable names in order and returns the value
>> @@ -118,6 +165,34 @@ pub fn get_secret_from_env(base_name: &str) -> Result<Option<String>, Error> {
>>      Ok(None)
>>  }
>>
>> +/// Gets the backup server's password.
>> +///
>> +/// We first try reading from the `PBS_PASSWORD` environment variable, then we
>> +/// try reading from the `pbs-password` [credential]. We ignore errors on the
>> +/// latter.
>> +///
>> +/// [credential]: https://systemd.io/CREDENTIALS/
>> +pub fn get_password() -> Result<Option<String>, Error> {
>> +    let password = get_secret_from_env(ENV_VAR_PBS_PASSWORD)?.or_else(|| {
>> +        get_credential(CRED_PBS_PASSWORD).and_then(|bytes| String::from_utf8(bytes).ok())
>
> A potential UTF-8 error here should be logged here!
>
> I think the function would be a bit more readable with a simple
>
>     if let Some(pw) = get_secret_from_env(...)? {
>         return Ok(pw);
>     }
>     if let Some(pw) = get_credential(...)? {
>         return String::from_utf8(pw)
>             .map_err(|_| format_err!("non-utf8 password credential"));
>     }
>
>> +    });
>> +    Ok(password)
>> +}
>> +
>> +/// Gets an encryption password.
>> +///
>> +/// We first try reading from the `PBS_ENCRYPTION_PASSWORD` environment
>> +/// variable, then we try reading from the `pbs-encryption-password`
>> +/// [credential]. We ignore errors on the latter.
>> +///
>> +/// [credential]: https://systemd.io/CREDENTIALS/
>> +pub fn get_encryption_password() -> Result<Option<Vec<u8>>, Error> {
>> +    let password = get_secret_from_env(ENV_VAR_PBS_ENCRYPTION_PASSWORD)?
>> +        .map(String::into_bytes)
>> +        .or_else(|| get_credential(CRED_PBS_ENCRYPTION_PASSWORD));
>
> This and `get_password` can just use the same helper function. They both
> only support utf-8 strings anyway from what I can tell.
> So this would just be
>
>     get_secret_impl(ENV_VAR_PBS_ENCRYPTION_PASSWORD, CRED_PBS_ENCRYPTION_PASSWORD)
>         .map(String::into_bytes)
>

I didn't implement this last part, imho the API feels better in this
way.

v2 sent at
https://lore.proxmox.com/pbs-devel/20250326094138.75437-1-m.sandoval@proxmox.com/T/#t.


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

Re: [pbs-devel] [PATCH backup] pbs-client: read credentials from $CREDENTIALS_DIRECTORY

[Jörg Behrmann]

Hi!

Sorry to chime in from the sidelines, I just saw this patch set, which makes me
very happy.

It would be great if everything that can be set via envvar could be set, via
credential as well, most importantly I am thinking about the repository, since
systemd can accept credentials passed in via smbios type 11 strings.

There is a bug report against the PVE web UI open to be able to set arbitrary
key pars for that [1]. This would allow to configure the pbs client for a VM
directly from the PVE web UI.

Another comment further down inline.

Thanks for this works!

best regards,
Jörg Behrmann

[1] https://bugzilla.proxmox.com/show_bug.cgi?id=5601

On Mon, Mar 24, 2025 at 01:35:42PM +0100, Maximiliano Sandoval wrote:
> Allows to load credentials passed down by systemd. A possible use-case
> is safely storing the server's password in a file encrypted by the
> systems TPM, e.g. via
> 
> ```
> systemd-ask-password -n | systemd-creds encrypt --name=pbs-password - my-api-token.cred
> ```
> 
> which then can be used via
> 
> ```
> systemd-run --pipe --wait --property=LoadCredentialEncrypted=pbs-password:my-api-token.cred \
> proxmox-backup-client ...
> ```
> 
> or from inside a service.
> 
> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> ---
>  pbs-client/src/tools/key_source.rs |  4 +-
>  pbs-client/src/tools/mod.rs        | 81 ++++++++++++++++++++++++++++--
>  2 files changed, 80 insertions(+), 5 deletions(-)
> 
> diff --git a/pbs-client/src/tools/key_source.rs b/pbs-client/src/tools/key_source.rs
> index 7968e0c2a..94b86e8b6 100644
> --- a/pbs-client/src/tools/key_source.rs
> +++ b/pbs-client/src/tools/key_source.rs
> @@ -345,8 +345,8 @@ pub(crate) unsafe fn set_test_default_master_pubkey(value: Result<Option<Vec<u8>
>  pub fn get_encryption_key_password() -> Result<Vec<u8>, Error> {
>      // fixme: implement other input methods
>  
> -    if let Some(password) = super::get_secret_from_env("PBS_ENCRYPTION_PASSWORD")? {
> -        return Ok(password.as_bytes().to_vec());
> +    if let Some(password) = super::get_encryption_password()? {
> +        return Ok(password);
>      }
>  
>      // If we're on a TTY, query the user for a password
> diff --git a/pbs-client/src/tools/mod.rs b/pbs-client/src/tools/mod.rs
> index 8068dc004..8b0392744 100644
> --- a/pbs-client/src/tools/mod.rs
> +++ b/pbs-client/src/tools/mod.rs
> @@ -2,7 +2,7 @@
>  use std::collections::HashMap;
>  use std::env::VarError::{NotPresent, NotUnicode};
>  use std::fs::File;
> -use std::io::{BufRead, BufReader};
> +use std::io::{BufRead, BufReader, Read};
>  use std::os::unix::fs::OpenOptionsExt;
>  use std::os::unix::io::FromRawFd;
>  use std::process::Command;
> @@ -28,6 +28,16 @@ pub mod key_source;
>  
>  const ENV_VAR_PBS_FINGERPRINT: &str = "PBS_FINGERPRINT";
>  const ENV_VAR_PBS_PASSWORD: &str = "PBS_PASSWORD";
> +const ENV_VAR_PBS_ENCRYPTION_PASSWORD: &str = "PBS_ENCRYPTION_PASSWORD";
> +
> +/// Directory with system [credential]s. See systemd-creds(1).
> +///
> +/// [credential]: https://systemd.io/CREDENTIALS/
> +const CREDENTIALS_DIRECTORY: &str = "CREDENTIALS_DIRECTORY";
> +/// Credential name of the encryption password.
> +const CRED_PBS_ENCRYPTION_PASSWORD: &str = "pbs-encryption-password";
> +/// Credential name of the the password.
> +const CRED_PBS_PASSWORD: &str = "pbs-password";
>  
>  pub const REPO_URL_SCHEMA: Schema = StringSchema::new("Repository URL.")
>      .format(&BACKUP_REPO_URL)
> @@ -40,6 +50,43 @@ pub const CHUNK_SIZE_SCHEMA: Schema = IntegerSchema::new("Chunk size in KB. Must
>      .default(4096)
>      .schema();
>  
> +/// Retrieves a secret stored in a [credential] provided by systemd.
> +///
> +/// Returns `None` if the credential does not exist or on errors.
> +///
> +/// [credential]: https://systemd.io/CREDENTIALS/
> +pub fn get_credential(cred_name: &str) -> Option<Vec<u8>> {
> +    // This is fundamentally a fn that returns a Result, but in practice we
> +    // would ignore errors so we hide the result in the public API.
> +    fn get_credential_inner(cred_name: &str) -> Result<Option<Vec<u8>>, Error> {
> +        let Some(creds_dir) = std::env::var_os(CREDENTIALS_DIRECTORY) else {
> +            return Ok(None);
> +        };
> +        let path = std::path::Path::new(&creds_dir).join(cred_name);
> +
> +        let mut file = File::open(&path)?;
> +        let mut buffer = vec![];
> +
> +        // We read the whole contents without a BufRead. As per
> +        // systemd-creds(1): Credentials are limited-size binary or textual
> +        // objects.
> +        file.read_to_end(&mut buffer)?;
> +
> +        Ok(Some(buffer))
> +    }
> +    get_credential_inner(cred_name)
> +        .inspect_err(|err| {
> +            if err
> +                .downcast_ref::<std::io::Error>()
> +                .is_some_and(|e| e.kind() != std::io::ErrorKind::NotFound)
> +            {
> +                proxmox_log::error!("{err:#}")
> +            }
> +        })
> +        .ok()
> +        .flatten()
> +}
> +
>  /// Helper to read a secret through a environment variable (ENV).
>  ///
>  /// Tries the following variable names in order and returns the value
> @@ -118,6 +165,34 @@ pub fn get_secret_from_env(base_name: &str) -> Result<Option<String>, Error> {
>      Ok(None)
>  }
>  
> +/// Gets the backup server's password.
> +///
> +/// We first try reading from the `PBS_PASSWORD` environment variable, then we
> +/// try reading from the `pbs-password` [credential]. We ignore errors on the
> +/// latter.
> +///
> +/// [credential]: https://systemd.io/CREDENTIALS/
> +pub fn get_password() -> Result<Option<String>, Error> {
> +    let password = get_secret_from_env(ENV_VAR_PBS_PASSWORD)?.or_else(|| {
> +        get_credential(CRED_PBS_PASSWORD).and_then(|bytes| String::from_utf8(bytes).ok())
> +    });
> +    Ok(password)
> +}
> +
> +/// Gets an encryption password.
> +///
> +/// We first try reading from the `PBS_ENCRYPTION_PASSWORD` environment
> +/// variable, then we try reading from the `pbs-encryption-password`

The name for credentials is pretty free form and dashes are the correct
namespacing for systemd units, but when grepping for SetCredential= and
LoadCredential= in the systemd codebase, you'll see that dots are the more
idiomatic way of namespacing credentials, this idiom has also spread to other
projects, e.g. util-linux (see the credential support in agetty).

The better name would therefore be pbs.encryption.password or
pbs.encryption-password or pbs.encryption_password, depending on what exactly
the namespacing you want to communicate is.

> +/// [credential]. We ignore errors on the latter.
> +///
> +/// [credential]: https://systemd.io/CREDENTIALS/
> +pub fn get_encryption_password() -> Result<Option<Vec<u8>>, Error> {
> +    let password = get_secret_from_env(ENV_VAR_PBS_ENCRYPTION_PASSWORD)?
> +        .map(String::into_bytes)
> +        .or_else(|| get_credential(CRED_PBS_ENCRYPTION_PASSWORD));
> +    Ok(password)
> +}
> +
>  pub fn get_default_repository() -> Option<String> {
>      std::env::var("PBS_REPOSITORY").ok()
>  }
> @@ -181,7 +256,7 @@ fn connect_do(
>  ) -> Result<HttpClient, Error> {
>      let fingerprint = std::env::var(ENV_VAR_PBS_FINGERPRINT).ok();
>  
> -    let password = get_secret_from_env(ENV_VAR_PBS_PASSWORD)?;
> +    let password = get_password()?;
>      let options = HttpClientOptions::new_interactive(password, fingerprint).rate_limit(rate_limit);
>  
>      HttpClient::new(server, port, auth_id, options)
> @@ -190,7 +265,7 @@ fn connect_do(
>  /// like get, but simply ignore errors and return Null instead
>  pub async fn try_get(repo: &BackupRepository, url: &str) -> Value {
>      let fingerprint = std::env::var(ENV_VAR_PBS_FINGERPRINT).ok();
> -    let password = get_secret_from_env(ENV_VAR_PBS_PASSWORD).unwrap_or(None);
> +    let password = get_password().unwrap_or(None);
>  
>      // ticket cache, but no questions asked
>      let options = HttpClientOptions::new_interactive(password, fingerprint).interactive(false);
> -- 
> 2.39.5
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

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

Re: [pbs-devel] [PATCH backup] pbs-client: read credentials from $CREDENTIALS_DIRECTORY

[Maximiliano Sandoval]

Jörg Behrmann <proxmox@behrmj87m.dialup.fu-berlin.de> writes:

> Hi!
>
> Sorry to chime in from the sidelines, I just saw this patch set, which makes me
> very happy.

:)

> It would be great if everything that can be set via envvar could be set, via
> credential as well, most importantly I am thinking about the repository, since
> systemd can accept credentials passed in via smbios type 11 strings.

Makes sense, I can prepare a follow-up with some of these if this patch
is applied.

> There is a bug report against the PVE web UI open to be able to set arbitrary
> key pars for that [1]. This would allow to configure the pbs client for a VM
> directly from the PVE web UI.
>
> Another comment further down inline.
>
> Thanks for this works!
>
> best regards,
> Jörg Behrmann
>
> [1] https://bugzilla.proxmox.com/show_bug.cgi?id=5601
>
> On Mon, Mar 24, 2025 at 01:35:42PM +0100, Maximiliano Sandoval wrote:
>> Allows to load credentials passed down by systemd. A possible use-case
>> is safely storing the server's password in a file encrypted by the
>> systems TPM, e.g. via
>>
>> ```
>> systemd-ask-password -n | systemd-creds encrypt --name=pbs-password - my-api-token.cred
>> ```
>> ...
>> +/// Gets an encryption password.
>> +///
>> +/// We first try reading from the `PBS_ENCRYPTION_PASSWORD` environment
>> +/// variable, then we try reading from the `pbs-encryption-password`
>
> The name for credentials is pretty free form and dashes are the correct
> namespacing for systemd units, but when grepping for SetCredential= and
> LoadCredential= in the systemd codebase, you'll see that dots are the more
> idiomatic way of namespacing credentials, this idiom has also spread to other
> projects, e.g. util-linux (see the credential support in agetty).
>
> The better name would therefore be pbs.encryption.password or
> pbs.encryption-password or pbs.encryption_password, depending on what exactly
> the namespacing you want to communicate is.

You are right, I will send a v3 using `proxmox-backup-client.password`
and `proxmox-backup-client.encryption-password`.


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