[PATCH backup v2 0/3] fix #7054: client: remove trailing newlines from credentials

[PATCH backup v2 0/3] fix #7054: client: remove trailing newlines from credentials

[Maximiliano Sandoval]

See the first commit for more details.

This was tested with proxmox-backup-client making a login/backup using different
credentials with and without newlines. The commands were similar to the
systemd-run commands at the Backup Client Usage docs.

I did not find a way to create the keyfile with newlines in its password using
proxmox-backup-client since it reads from the tty stdin, but surely it could be
created manually. Perhaps it is safe-ish to also remove trailing control
characters from the encryption password but this seems a safer approach for now.

Diferences from v1:
 - Always do an extra allocation to keep the code clean
 - Rename password to blob
 - Only strip newlines on passwords

Maximiliano Sandoval (3):
  fix #7054: client: remove trailing newlines from credentials
  docs: client: document further password constrains
  client: rename password to blob

 docs/backup-client.rst      |  7 ++++---
 pbs-client/src/tools/mod.rs | 14 ++++++++++++--
 2 files changed, 16 insertions(+), 5 deletions(-)

-- 
2.47.3

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

[Maximiliano Sandoval]

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.

Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---
 pbs-client/src/tools/mod.rs | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/pbs-client/src/tools/mod.rs b/pbs-client/src/tools/mod.rs
index 7a496d14c..f28d9f32f 100644
--- a/pbs-client/src/tools/mod.rs
+++ b/pbs-client/src/tools/mod.rs
@@ -168,7 +168,17 @@ fn get_secret_impl(env_variable: &str, credential_name: &str) -> Result<Option<S
     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)
+        str::from_utf8(&password)
+            .map(|s| {
+                if matches!(credential_name, CRED_PBS_REPOSITORY | CRED_PBS_FINGERPRINT) {
+                    s.trim_end()
+                } else if credential_name == CRED_PBS_PASSWORD {
+                    s.trim_end_matches('\n')
+                } else {
+                    s
+                }
+            })
+            .map(ToOwned::to_owned)
             .map(Option::Some)
             .map_err(|_err| format_err!("credential {credential_name} is not utf8 encoded"))
     } else {
-- 
2.47.3

[PATCH backup v2 2/3] docs: client: document further password constrains

[Maximiliano Sandoval]

We leave the explicit newlines as "control character" might not mean
much anything to some readers.

Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---
 docs/backup-client.rst | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/docs/backup-client.rst b/docs/backup-client.rst
index 40962f0e2..03a383d9c 100644
--- a/docs/backup-client.rst
+++ b/docs/backup-client.rst
@@ -104,9 +104,10 @@ Environment Variables
    wireguard, instead of using an HTTP proxy.
 
 
-.. Note:: Passwords must be valid UTF-8 and may not contain newlines. For your
-   convenience, Proxmox Backup Server only uses the first line as password, so
-   you can add arbitrary comments after the first newline.
+.. Note:: Passwords must be valid UTF-8 and may not contain newlines or any
+   control characters in general. For your convenience, Proxmox Backup Server
+   only uses the first line as password, so you can add arbitrary comments after
+   the first newline.
 
 
 System and Service Credentials
-- 
2.47.3

[PATCH backup v2 3/3] client: rename password to blob

[Maximiliano Sandoval]

This is a Vec<u8> and not yet the password in its final form.

Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---
 pbs-client/src/tools/mod.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/pbs-client/src/tools/mod.rs b/pbs-client/src/tools/mod.rs
index f28d9f32f..aa3ae94f2 100644
--- a/pbs-client/src/tools/mod.rs
+++ b/pbs-client/src/tools/mod.rs
@@ -167,8 +167,8 @@ fn get_secret_from_env(base_name: &str) -> Result<Option<String>, Error> {
 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)? {
-        str::from_utf8(&password)
+    } else if let Some(blob) = get_credential(credential_name)? {
+        str::from_utf8(&blob)
             .map(|s| {
                 if matches!(credential_name, CRED_PBS_REPOSITORY | CRED_PBS_FINGERPRINT) {
                     s.trim_end()
-- 
2.47.3

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

[Christian Ebner]

On 2/20/26 1:38 PM, Maximiliano Sandoval wrote:
> For repositories and fingerprints we simply strip trailing whitespaces.

This should explicitly state that this is done for improved usability.

> For passwords, we refer to the password regex at proxmox-schema:
> `^[[:^cntrl:]]*$`, we can only strip trailing control characters without
> potentially breaking existing passwords.

This is however not what the patch does, it only strips trailing 
newlines from the password blob, leaving other control characters in 
place to be matched by the schema's regex.

> 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.
> 
> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> ---
>   pbs-client/src/tools/mod.rs | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/pbs-client/src/tools/mod.rs b/pbs-client/src/tools/mod.rs
> index 7a496d14c..f28d9f32f 100644
> --- a/pbs-client/src/tools/mod.rs
> +++ b/pbs-client/src/tools/mod.rs
> @@ -168,7 +168,17 @@ fn get_secret_impl(env_variable: &str, credential_name: &str) -> Result<Option<S
>       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)
> +        str::from_utf8(&password)
> +            .map(|s| {
> +                if matches!(credential_name, CRED_PBS_REPOSITORY | CRED_PBS_FINGERPRINT) {
> +                    s.trim_end()
> +                } else if credential_name == CRED_PBS_PASSWORD {
> +                    s.trim_end_matches('\n')
> +                } else {
> +                    s
> +                }
> +            })
> +            .map(ToOwned::to_owned)
>               .map(Option::Some)
>               .map_err(|_err| format_err!("credential {credential_name} is not utf8 encoded"))
>       } else {

Re: [PATCH backup v2 2/3] docs: client: document further password constrains

[Christian Ebner]

On 2/20/26 1:38 PM, Maximiliano Sandoval wrote:
> We leave the explicit newlines as "control character" might not mean
> much anything to some readers.

This is grammatically not correct, you might drop either the "much" or 
the "anything"

> 
> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> ---
>   docs/backup-client.rst | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/backup-client.rst b/docs/backup-client.rst
> index 40962f0e2..03a383d9c 100644
> --- a/docs/backup-client.rst
> +++ b/docs/backup-client.rst
> @@ -104,9 +104,10 @@ Environment Variables
>      wireguard, instead of using an HTTP proxy.
>   
>   
> -.. Note:: Passwords must be valid UTF-8 and may not contain newlines. For your
> -   convenience, Proxmox Backup Server only uses the first line as password, so
> -   you can add arbitrary comments after the first newline.
> +.. Note:: Passwords must be valid UTF-8 and may not contain newlines or any
> +   control characters in general. For your convenience, Proxmox Backup Server

nit: "... any other control characters." is a bit more concise.

> +   only uses the first line as password, so you can add arbitrary comments after
> +   the first newline.
>   
>   
>   System and Service Credentials

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

[Maximiliano Sandoval]

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

> On 2/20/26 1:38 PM, Maximiliano Sandoval wrote:
>> For repositories and fingerprints we simply strip trailing whitespaces.
>
> This should explicitly state that this is done for improved usability.
>
>> For passwords, we refer to the password regex at proxmox-schema:
>> `^[[:^cntrl:]]*$`, we can only strip trailing control characters without
>> potentially breaking existing passwords.
>
> This is however not what the patch does, it only strips trailing newlines from
> the password blob, leaving other control characters in place to be matched by
> the schema's regex.

This was recommended off-list. I will update the commit message and send
v3.

>> 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.
>> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
>> ---
>>   pbs-client/src/tools/mod.rs | 12 +++++++++++-
>>   1 file changed, 11 insertions(+), 1 deletion(-)
>> diff --git a/pbs-client/src/tools/mod.rs b/pbs-client/src/tools/mod.rs
>> index 7a496d14c..f28d9f32f 100644
>> --- a/pbs-client/src/tools/mod.rs
>> +++ b/pbs-client/src/tools/mod.rs
>> @@ -168,7 +168,17 @@ fn get_secret_impl(env_variable: &str, credential_name: &str) -> Result<Option<S
>>       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)
>> +        str::from_utf8(&password)
>> +            .map(|s| {
>> +                if matches!(credential_name, CRED_PBS_REPOSITORY | CRED_PBS_FINGERPRINT) {
>> +                    s.trim_end()
>> +                } else if credential_name == CRED_PBS_PASSWORD {
>> +                    s.trim_end_matches('\n')
>> +                } else {
>> +                    s
>> +                }
>> +            })
>> +            .map(ToOwned::to_owned)
>>               .map(Option::Some)
>>               .map_err(|_err| format_err!("credential {credential_name} is not utf8 encoded"))
>>       } else {

-- 
Maximiliano

superseded: [PATCH backup v2 0/3] fix #7054: client: remove trailing newlines from credentials

[Maximiliano Sandoval]

Maximiliano Sandoval <m.sandoval@proxmox.com> writes:

> See the first commit for more details.
>
> This was tested with proxmox-backup-client making a login/backup using different
> credentials with and without newlines. The commands were similar to the
> systemd-run commands at the Backup Client Usage docs.
>
> I did not find a way to create the keyfile with newlines in its password using
> proxmox-backup-client since it reads from the tty stdin, but surely it could be
> created manually. Perhaps it is safe-ish to also remove trailing control
> characters from the encryption password but this seems a safer approach for now.
>
> Diferences from v1:
>  - Always do an extra allocation to keep the code clean
>  - Rename password to blob
>  - Only strip newlines on passwords

Superseded-by: https://lore.proxmox.com/all/20260223093710.36009-1-m.sandoval@proxmox.com/

-- 
Maximiliano