Re: [pbs-devel] [PATCH backup 4/5] pbs-client: make common helper for getting UTF-8 secrets

[Christian Ebner <c.ebner@proxmox.com>]

This patch could be ordered before the change to the reading of the 
default repository, so you don not have to remove the hunks introduced 
by that patch again here.

Further, I see that the get_secrets_impl and get_encryption_password do 
look almost identical now, so latter could maybe be covered by the same 
implementation logic as well, doing the UTF-8 string conversion/checking 
only after?

On 3/26/25 15:26, Maximiliano Sandoval wrote:
> Now that there are three credentials it makes sense to have a common
> helper.
> 
> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> ---
>   pbs-client/src/tools/mod.rs | 61 ++++++++++++++++++-------------------
>   1 file changed, 30 insertions(+), 31 deletions(-)
> 
> diff --git a/pbs-client/src/tools/mod.rs b/pbs-client/src/tools/mod.rs
> index 5dd3b6b10..bd553d88b 100644
> --- a/pbs-client/src/tools/mod.rs
> +++ b/pbs-client/src/tools/mod.rs
> @@ -157,6 +157,25 @@ fn get_secret_from_env(base_name: &str) -> Result<Option<String>, Error> {
>       Ok(None)
>   }
>   
> +/// Gets a secret or value from the environment.
> +///
> +/// Checks for an environment variable named `env_variable`, and if missing, it
> +/// checks for a system [credential] named `credential_name`. Assumes the secret
> +/// is UTF-8 encoded.
> +///
> +/// [credential]: https://systemd.io/CREDENTIALS/
> +fn get_secret_impl(env_variable: &str, credential_name: &str) -> Result<Option<String>, Error> {
> +    if let Some(password) = get_secret_from_env(env_variable)? {
> +        Ok(Some(password))
> +    } else if let Some(password) = get_credential(credential_name)? {
> +        String::from_utf8(password)
> +            .map(Option::Some)
> +            .map_err(|_err| format_err!("credential {credential_name} is not utf8 encoded"))
> +    } else {
> +        Ok(None)
> +    }
> +}
> +
>   /// Gets the backup server's password.
>   ///
>   /// Looks for a password in the `PBS_PASSWORD` environment variable, if there
> @@ -167,15 +186,7 @@ fn get_secret_from_env(base_name: &str) -> Result<Option<String>, Error> {
>   ///
>   /// [credential]: https://systemd.io/CREDENTIALS/
>   pub fn get_password() -> Result<Option<String>, Error> {
> -    if let Some(password) = get_secret_from_env(ENV_VAR_PBS_PASSWORD)? {
> -        Ok(Some(password))
> -    } else if let Some(password) = get_credential(CRED_PBS_PASSWORD)? {
> -        String::from_utf8(password)
> -            .map(Option::Some)
> -            .map_err(|_err| format_err!("non-utf8 password credential"))
> -    } else {
> -        Ok(None)
> -    }
> +    get_secret_impl(ENV_VAR_PBS_PASSWORD, CRED_PBS_PASSWORD)
>   }
>   
>   /// Gets an encryption password.
> @@ -200,17 +211,11 @@ pub fn get_encryption_password() -> Result<Option<Vec<u8>>, Error> {
>   }
>   
>   pub fn get_default_repository() -> Option<String> {
> -    if let Ok(repository) = std::env::var(ENV_VAR_PBS_REPOSITORY) {
> -        Some(repository)
> -    } else if let Ok(Some(repository)) = get_credential(CRED_PBS_REPOSITORY).inspect_err(|err| {
> -        proxmox_log::error!("Could not read credential {CRED_PBS_REPOSITORY}: {err}")
> -    }) {
> -        String::from_utf8(repository)
> -            .inspect_err(|_err| proxmox_log::error!("non-utf8 repository credential"))
> -            .ok()
> -    } else {
> -        None
> -    }
> +    get_secret_impl(ENV_VAR_PBS_REPOSITORY, CRED_PBS_REPOSITORY)
> +        .inspect_err(|err| {
> +            proxmox_log::error!("could not read default repository: {err:#}");
> +        })
> +        .unwrap_or_default()
>   }
>   
>   /// Gets the repository fingerprint.
> @@ -224,17 +229,11 @@ pub fn get_default_repository() -> Option<String> {
>   ///
>   /// [credential]: https://systemd.io/CREDENTIALS/
>   pub fn get_fingerprint() -> Option<String> {
> -    if let Ok(fingerprint) = std::env::var(ENV_VAR_PBS_FINGERPRINT) {
> -        Some(fingerprint)
> -    } else if let Ok(Some(fingerprint)) = get_credential(CRED_PBS_FINGERPRINT).inspect_err(|err| {
> -        proxmox_log::error!("Could not read credential {CRED_PBS_FINGERPRINT}: {err}")
> -    }) {
> -        String::from_utf8(fingerprint)
> -            .inspect_err(|_err| proxmox_log::error!("non-utf8 fingerprint credential"))
> -            .ok()
> -    } else {
> -        None
> -    }
> +    get_secret_impl(ENV_VAR_PBS_FINGERPRINT, CRED_PBS_FINGERPRINT)
> +        .inspect_err(|err| {
> +            proxmox_log::error!("could not read fingerprint: {err:#}");
> +        })
> +        .unwrap_or_default()
>   }
>   
>   pub fn remove_repository_from_value(param: &mut Value) -> Result<BackupRepository, Error> {



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