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

[PATCH backup v3 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 v2:
 - Reworded commit messages following feedback form v1.

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 v3 1/3] fix #7054: client: remove trailing newlines from credentials

[Maximiliano Sandoval]

Many tools, including systemd-ask-password will add a trailing new line, in
order to improve usability we strip trailing newlines.

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. For the sake of avoiding to
strip more than necessary we only strip trailing newlines.

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 v3 2/3] docs: client: document further password constrains

[Maximiliano Sandoval]

We leave the explicit newlines as "control character" might not mean
much 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 v3 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 v3 1/3] fix #7054: client: remove trailing newlines from credentials

[Christian Ebner]

two comments inline.

On 2/23/26 10:37 AM, Maximiliano Sandoval wrote:
> Many tools, including systemd-ask-password will add a trailing new line, in
> order to improve usability we strip trailing newlines.
> 
> 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. For the sake of avoiding to
> strip more than necessary we only strip trailing newlines.
> 
> 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
> +                }

since this depends on the provided credential name anyways, why not do 
it in the respective helpers instead, e.g. get_password() or 
get_default_repository()? By that it is possible to avoid the 
conditional trimming here.

> +            })
> +            .map(ToOwned::to_owned)
>               .map(Option::Some)
>               .map_err(|_err| format_err!("credential {credential_name} is not utf8 encoded"))
>       } else {

What about CRED_PBS_ENCRYPTION_PASSWORD? That should be handled as well.

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

[Christian Ebner]

one nit inline

On 2/23/26 10:37 AM, Maximiliano Sandoval wrote:
> We leave the explicit newlines as "control character" might not mean
> much 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

nit: I suggest to drop the ` in general` here or even rephrase to `may 
not contain any control characters or newlines.`. The `in general` makes 
it less concise IMO, without providing additional relevant information.

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

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

[Christian Ebner]

On 2/23/26 10:37 AM, Maximiliano Sandoval wrote:
> 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()

since the conditional trimming is suggested to be done on the individual 
helpers (see comments on patch 1), this helper should also be renamed. 
E.g. it could be called get_secrets_blob() or raw_secret_as_string(), 
reflecting this.