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

[PATCH backup v4 0/4] 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.

Differences from v3:
 - Move trim_end calls to get_password, get_repository, get_fingerprint
 - Remove "in general" from the documentation
 - Rename get_secret_impl to get_secret_string

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 (4):
  fix #7054: client: remove trailing newlines from credentials
  docs: client: document further password constrains
  client: rename password to blob
  client: Rename get_secret_impl method

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

-- 
2.47.3

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

[Maximiliano Sandoval]

Some tools, like systemd-ask-password can add a trailing new line to the file.
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 and thus the password might contain newlines.

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

diff --git a/pbs-client/src/tools/mod.rs b/pbs-client/src/tools/mod.rs
index 700436f8c..ca64b239e 100644
--- a/pbs-client/src/tools/mod.rs
+++ b/pbs-client/src/tools/mod.rs
@@ -189,6 +189,7 @@ fn get_secret_impl(env_variable: &str, credential_name: &str) -> Result<Option<S
 /// [credential]: https://systemd.io/CREDENTIALS/
 pub fn get_password() -> Result<Option<String>, Error> {
     get_secret_impl(ENV_VAR_PBS_PASSWORD, CRED_PBS_PASSWORD)
+        .map(|opt| opt.map(|s| s.trim_end_matches('\n').to_string()))
 }
 
 /// Gets an encryption password.
@@ -214,6 +215,7 @@ pub fn get_default_repository() -> Option<String> {
         .inspect_err(|err| {
             proxmox_log::error!("could not read default repository: {err:#}");
         })
+        .map(|opt| opt.map(|s| s.trim_end().to_string()))
         .unwrap_or_default()
 }
 
@@ -232,6 +234,7 @@ pub fn get_fingerprint() -> Option<String> {
         .inspect_err(|err| {
             proxmox_log::error!("could not read fingerprint: {err:#}");
         })
+        .map(|opt| opt.map(|s| s.trim_end().to_string()))
         .unwrap_or_default()
 }
 
-- 
2.47.3

[PATCH backup v4 2/4] docs: client: document further password constrains

[Maximiliano Sandoval]

Even though newlines are control characters, we keep them explicitly for ease of
understanding.

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 e2a05d4fc..cd7488287 100644
--- a/docs/backup-client.rst
+++ b/docs/backup-client.rst
@@ -168,9 +168,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 character. 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 v4 3/4] client: rename password to blob

[Maximiliano Sandoval]

This is a Vec<u8> and not yet the password in its unicode 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 ca64b239e..d99215c44 100644
--- a/pbs-client/src/tools/mod.rs
+++ b/pbs-client/src/tools/mod.rs
@@ -169,8 +169,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)? {
-        String::from_utf8(password)
+    } else if let Some(blob) = get_credential(credential_name)? {
+        String::from_utf8(blob)
             .map(Option::Some)
             .map_err(|_err| format_err!("credential {credential_name} is not utf8 encoded"))
     } else {
-- 
2.47.3

[PATCH backup v4 4/4] client: Rename get_secret_impl method

[Maximiliano Sandoval]

To get_secret_string to reflect this is a unicode string.

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

diff --git a/pbs-client/src/tools/mod.rs b/pbs-client/src/tools/mod.rs
index d99215c44..778c301c6 100644
--- a/pbs-client/src/tools/mod.rs
+++ b/pbs-client/src/tools/mod.rs
@@ -166,7 +166,7 @@ fn get_secret_from_env(base_name: &str) -> Result<Option<String>, Error> {
 /// is UTF-8 encoded.
 ///
 /// [credential]: https://systemd.io/CREDENTIALS/
-fn get_secret_impl(env_variable: &str, credential_name: &str) -> Result<Option<String>, Error> {
+fn get_secret_string(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(blob) = get_credential(credential_name)? {
@@ -188,7 +188,7 @@ fn get_secret_impl(env_variable: &str, credential_name: &str) -> Result<Option<S
 ///
 /// [credential]: https://systemd.io/CREDENTIALS/
 pub fn get_password() -> Result<Option<String>, Error> {
-    get_secret_impl(ENV_VAR_PBS_PASSWORD, CRED_PBS_PASSWORD)
+    get_secret_string(ENV_VAR_PBS_PASSWORD, CRED_PBS_PASSWORD)
         .map(|opt| opt.map(|s| s.trim_end_matches('\n').to_string()))
 }
 
@@ -204,14 +204,14 @@ pub fn get_password() -> Result<Option<String>, Error> {
 ///
 /// [credential]: https://systemd.io/CREDENTIALS/
 pub fn get_encryption_password() -> Result<Option<String>, Error> {
-    get_secret_impl(
+    get_secret_string(
         ENV_VAR_PBS_ENCRYPTION_PASSWORD,
         CRED_PBS_ENCRYPTION_PASSWORD,
     )
 }
 
 pub fn get_default_repository() -> Option<String> {
-    get_secret_impl(ENV_VAR_PBS_REPOSITORY, CRED_PBS_REPOSITORY)
+    get_secret_string(ENV_VAR_PBS_REPOSITORY, CRED_PBS_REPOSITORY)
         .inspect_err(|err| {
             proxmox_log::error!("could not read default repository: {err:#}");
         })
@@ -230,7 +230,7 @@ pub fn get_default_repository() -> Option<String> {
 ///
 /// [credential]: https://systemd.io/CREDENTIALS/
 pub fn get_fingerprint() -> Option<String> {
-    get_secret_impl(ENV_VAR_PBS_FINGERPRINT, CRED_PBS_FINGERPRINT)
+    get_secret_string(ENV_VAR_PBS_FINGERPRINT, CRED_PBS_FINGERPRINT)
         .inspect_err(|err| {
             proxmox_log::error!("could not read fingerprint: {err:#}");
         })
-- 
2.47.3