Re: [pbs-devel] [PATCH backup v2 2/7] pbs-client: add helper for getting UTF-8 secrets

[Maximiliano Sandoval <m.sandoval@proxmox.com>]

Christian Ebner <c.ebner@proxmox.com> writes:

> On 3/27/25 11:47, Maximiliano Sandoval wrote:
>> We are going to add more credentials so it makes sense to have a common
>> helper to get the secrets.
>> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
>> ---
>>   pbs-client/src/tools/mod.rs | 19 +++++++++++++++++++
>>   1 file changed, 19 insertions(+)
>> diff --git a/pbs-client/src/tools/mod.rs b/pbs-client/src/tools/mod.rs
>> index a42fa114..efd2e139 100644
>> --- a/pbs-client/src/tools/mod.rs
>> +++ b/pbs-client/src/tools/mod.rs
>> @@ -153,6 +153,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"))
>
> This check for valid UTF-8 could rather happen directly in get_credential since
> you will enforce get_encryption_password to return a String anyways in patch 4?
> And the others already return a String anyways.

Yes, you could. However, our secrets being valid UTF-8 is an
implementation detail that I would prefer to not leak onto that layer of
the API, credentials are intrinsically arbitrary bytes (and sometimes
UTF-8 text) and imho the get_credential function should reflect that.

> Only allowing UTF-8 content from get_credential would bring us in line with
> secrets obtained from the other sources, and since this has been introduced only
> recently (and never packaged until now), this could still be changed without
> break changes.
>
> As a side effect this helper would than simplify further.
>
>> +    } else {
>> +        Ok(None)
>> +    }
>> +}
>> +
>>   /// Gets the backup server's password.
>>   ///
>>   /// Looks for a password in the `PBS_PASSWORD` environment variable, if there



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