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

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

On 3/27/25 13:16, Maximiliano Sandoval wrote:
> 
> 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.

Fine by me, although I do not really see that as an issue here. 
get_credential is a private helper without external call sides. Also, 
refactoring this will still be possible in the future if other use cases 
arise requiring the raw bytes instead of the encoded string.


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