Re: [PATCH backup 1/2] fix #7054: client: remove trailing newlines from credentials

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

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

> no in depth review, but just a quick comment
>
> On 2/20/26 12:19 PM, Maximiliano Sandoval wrote:
>> For repositories and fingerprints we simply strip trailing whitespaces.
>> For passwords, we refer to the password regex at proxmox-schema:
>> `^[[:^cntrl:]]*$`, we can only strip trailing control characters without
>> potentially breaking existing passwords.
>> The encryption password is just a blob of bytes handled locally by the
>> client, we cannot remove trailing whitespace here without potential
>> breakage. Creation of such passwords (via
>> proxmox_sys::tty::read_and_verify_password) only verifies valid utf-8
>> and len >= 5.
>> In order to prevent an allocation for credentials that do not need to be
>> stripped we perform a preemptive check with str::ends_with before
>> allocating.
>> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
>> ---
>>   pbs-client/src/tools/mod.rs | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>> diff --git a/pbs-client/src/tools/mod.rs b/pbs-client/src/tools/mod.rs
>> index 7a496d14c..8dc7c2cd5 100644
>> --- a/pbs-client/src/tools/mod.rs
>> +++ b/pbs-client/src/tools/mod.rs
>> @@ -169,6 +169,17 @@ fn get_secret_impl(env_variable: &str, credential_name: &str) -> Result<Option<S
>>           Ok(Some(password))
>>       } else if let Some(password) = get_credential(credential_name)? {
>>           String::from_utf8(password)
>> +            .map(|s| {
>> +                if matches!(credential_name, CRED_PBS_REPOSITORY | CRED_PBS_FINGERPRINT)
>> +                    && s.ends_with(char::is_whitespace)
>
> instead of doing the string allocation above and add additional checks to avoid
> allocation here and below (1), why not strip before string allocation of
> `password` directly?
>
> You can use str::from_utf8 [0] and only do the full string allocation once
> trailing characters are stripped.

When the password does not need any trimming, the current method does
not allocate since String::from_utf8 does not allocate. Using
str::from_utf8 would result in 1 allocation when doing such "final
allocation". For reference String::from_utf8 does:

pub fn from_utf8(vec: Vec<u8>) -> Result<String, FromUtf8Error> {
    match str::from_utf8(&vec) {
        Ok(..) => Ok(String { vec }),
        Err(e) => Err(FromUtf8Error { bytes: vec, error: e }),
    }
}

It should be possible to do the trimming before the allocation, but I
don't want to touch the get_credential bits at the moment.

>> +                {
>> +                    s.trim_end().to_string()
>> +                } else if credential_name == CRED_PBS_PASSWORD && s.ends_with(char::is_control) {
>
> (1)
>
>> +                    s.trim_end_matches(char::is_control).to_string()
>> +                } else {
>> +                    s
>> +                }
>> +            })
>>               .map(Option::Some)
>>               .map_err(|_err| format_err!("credential {credential_name} is not utf8 encoded"))
>>       } else {
>
> [0] https://doc.rust-lang.org/std/primitive.str.html#method.from_utf8

-- 
Maximiliano