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

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

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