[pbs-devel] [PATCH proxmox-backup 1/2] support more ENV vars to get secret values

[pbs-devel] [PATCH proxmox-backup 1/2] support more ENV vars to get secret values

[Dietmar Maurer]

---
 src/bin/proxmox_client_tools/key_source.rs |  9 +--
 src/bin/proxmox_client_tools/mod.rs        | 76 +++++++++++++++++++---
 2 files changed, 70 insertions(+), 15 deletions(-)

diff --git a/src/bin/proxmox_client_tools/key_source.rs b/src/bin/proxmox_client_tools/key_source.rs
index fee00723..a611cd76 100644
--- a/src/bin/proxmox_client_tools/key_source.rs
+++ b/src/bin/proxmox_client_tools/key_source.rs
@@ -343,13 +343,8 @@ pub(crate) unsafe fn set_test_default_master_pubkey(value: Result<Option<Vec<u8>
 pub fn get_encryption_key_password() -> Result<Vec<u8>, Error> {
     // fixme: implement other input methods
 
-    use std::env::VarError::*;
-    match std::env::var("PBS_ENCRYPTION_PASSWORD") {
-        Ok(p) => return Ok(p.as_bytes().to_vec()),
-        Err(NotUnicode(_)) => bail!("PBS_ENCRYPTION_PASSWORD contains bad characters"),
-        Err(NotPresent) => {
-            // Try another method
-        }
+    if let Some(password) = super::get_secret_from_env("PBS_ENCRYPTION_PASSWORD")? {
+        return Ok(password.as_bytes().to_vec());
     }
 
     // If we're on a TTY, query the user for a password
diff --git a/src/bin/proxmox_client_tools/mod.rs b/src/bin/proxmox_client_tools/mod.rs
index 77c33ff0..e4dfb859 100644
--- a/src/bin/proxmox_client_tools/mod.rs
+++ b/src/bin/proxmox_client_tools/mod.rs
@@ -1,5 +1,10 @@
 //! Shared tools useful for common CLI clients.
 use std::collections::HashMap;
+use std::fs::File;
+use std::os::unix::io::FromRawFd;
+use std::env::VarError::{NotUnicode, NotPresent};
+use std::io::Read;
+use std::process::Command;
 
 use anyhow::{bail, format_err, Context, Error};
 use serde_json::{json, Value};
@@ -7,6 +12,7 @@ use xdg::BaseDirectories;
 
 use proxmox::{
     api::schema::*,
+    api::cli::shellword_split,
     tools::fs::file_get_json,
 };
 
@@ -34,6 +40,66 @@ pub const CHUNK_SIZE_SCHEMA: Schema = IntegerSchema::new("Chunk size in KB. Must
     .default(4096)
     .schema();
 
+/// Helper to read secrets from ENV vars. Reads the following VARs:
+///
+/// BASE_NAME => use value from ENV(BASE_NAME)
+/// BASE_NAME_FD => read from specified file descriptor
+/// BASE_NAME_FILE => read from specified file name
+/// BASE_NAME_CMD => read from specified file name
+///
+/// Only return the first line when reading from a file or command.
+pub fn get_secret_from_env(base_name: &str) -> Result<Option<String>, Error> {
+
+    match std::env::var(base_name) {
+        Ok(p) => return Ok(Some(p)),
+        Err(NotUnicode(_)) => bail!(format!("{} contains bad characters", base_name)),
+        Err(NotPresent) => {},
+    };
+
+    let firstline = |data: String| -> String {
+        match data.lines().next() {
+            Some(line) => line.to_string(),
+            None => String::new(),
+        }
+    };
+
+    let env_name = format!("{}_FD", base_name);
+    match std::env::var(&env_name) {
+        Ok(fd_str) => {
+            let fd: i32 = fd_str.parse()
+                .map_err(|err| format_err!("unable to parse file descriptor in ENV({}): {}", env_name, err))?;
+            let mut file = unsafe { File::from_raw_fd(fd) };
+            let mut buffer = String::new();
+            let _ = file.read_to_string(&mut buffer)?;
+            return Ok(Some(firstline(buffer)));
+        }
+        _ => {}
+    }
+
+    let env_name = format!("{}_FILE", base_name);
+    match std::env::var(&env_name) {
+        Ok(filename) => {
+            let data = proxmox::tools::fs::file_read_string(filename)?;
+            return Ok(Some(firstline(data)));
+        }
+        _ => {}
+    }
+
+    let env_name = format!("{}_CMD", base_name);
+    match std::env::var(&env_name) {
+        Ok(ref command) => {
+            let args = shellword_split(command)?;
+            let mut command = Command::new(&args[0]);
+            command.args(&args[1..]);
+            let output = tools::run_command(command, None)?;
+            return Ok(Some(firstline(output)));
+        }
+        _ => {}
+    }
+
+    Ok(None)
+}
+
 pub fn get_default_repository() -> Option<String> {
     std::env::var("PBS_REPOSITORY").ok()
 }
@@ -66,13 +132,7 @@ pub fn connect(repo: &BackupRepository) -> Result<HttpClient, Error> {
 fn connect_do(server: &str, port: u16, auth_id: &Authid) -> Result<HttpClient, Error> {
     let fingerprint = std::env::var(ENV_VAR_PBS_FINGERPRINT).ok();
 
-    use std::env::VarError::*;
-    let password = match std::env::var(ENV_VAR_PBS_PASSWORD) {
-        Ok(p) => Some(p),
-        Err(NotUnicode(_)) => bail!(format!("{} contains bad characters", ENV_VAR_PBS_PASSWORD)),
-        Err(NotPresent) => None,
-    };
-
+    let password = get_secret_from_env(ENV_VAR_PBS_PASSWORD)?;
     let options = HttpClientOptions::new_interactive(password, fingerprint);
 
     HttpClient::new(server, port, auth_id, options)
@@ -82,7 +142,7 @@ fn connect_do(server: &str, port: u16, auth_id: &Authid) -> Result<HttpClient, E
 pub async fn try_get(repo: &BackupRepository, url: &str) -> Value {
 
     let fingerprint = std::env::var(ENV_VAR_PBS_FINGERPRINT).ok();
-    let password = std::env::var(ENV_VAR_PBS_PASSWORD).ok();
+    let password = get_secret_from_env(ENV_VAR_PBS_PASSWORD).unwrap_or(None);
 
     // ticket cache, but no questions asked
     let options = HttpClientOptions::new_interactive(password, fingerprint)
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup 2/2] doc: Document new environment vars to specify secret values

[Dietmar Maurer]

---
 docs/backup-client.rst | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/docs/backup-client.rst b/docs/backup-client.rst
index bd0fdfed..cae7ab83 100644
--- a/docs/backup-client.rst
+++ b/docs/backup-client.rst
@@ -49,10 +49,28 @@ Environment Variables
   When set, this value is used for the password required for the backup server.
   You can also set this to a API token secret.
 
+``PBS_PASSWORD_FD``
+  Like ``PBS_PASSWORD``, but read data from Unix file descriptor.
+
+``PBS_PASSWORD_FILE``
+  Like ``PBS_PASSWORD``, but read data from specified file name.
+
+``PBS_PASSWORD_CMD``
+  Like ``PBS_PASSWORD``, but read data from specified command (stdout).
+
 ``PBS_ENCRYPTION_PASSWORD``
   When set, this value is used to access the secret encryption key (if
   protected by password).
 
+``PBS_ENCRYPTION_PASSWORD_FD``
+  Like ``PBS_ENCRYPTION_PASSWORD``, but read data from Unix file descriptor.
+
+``PBS_ENCRYPTION_PASSWORD_FILE``
+  Like ``PBS_ENCRYPTION_PASSWORD``, but read data from specified file name.
+
+``PBS_ENCRYPTION_PASSWORD_CMD``
+  Like ``PBS_ENCRYPTION_PASSWORD``, but read data from specified command (stdout).
+
 ``PBS_FINGERPRINT`` When set, this value is used to verify the server
   certificate (only used if the system CA certificates cannot validate the
   certificate).
-- 
2.30.2

Re: [pbs-devel] [PATCH proxmox-backup 1/2] support more ENV vars to get secret values

[Thomas Lamprecht]

On 19.07.21 11:21, Dietmar Maurer wrote:
> diff --git a/src/bin/proxmox_client_tools/mod.rs b/src/bin/proxmox_client_tools/mod.rs
> index 77c33ff0..e4dfb859 100644
> --- a/src/bin/proxmox_client_tools/mod.rs
> +++ b/src/bin/proxmox_client_tools/mod.rs

> @@ -34,6 +40,66 @@ pub const CHUNK_SIZE_SCHEMA: Schema = IntegerSchema::new("Chunk size in KB. Must
>      .default(4096)
>      .schema();
>  
> +/// Helper to read secrets from ENV vars. Reads the following VARs:
> +///

"VARs" is bad capitalization.

IMO it would be good to document the precedence order too, as if one environment
defines more that one of the variants below it may not be clear what one will "win"
to the user.

So for above using something like the following could help to avoid confusion when reading
about that helper in rustdoc:

/// Helper to read a secret through a environment variable (ENV).
///
/// Tries the following variable names in order and returns the value it will resolve
/// for the first defined one:



> +/// BASE_NAME => use value from ENV(BASE_NAME)

/// use value from ENV(BASE_NAME) directly as secret

> +/// BASE_NAME_FD => read from specified file descriptor

s/read from/read the secret from the/

> +/// BASE_NAME_FILE => read from specified file name

s/read from/read the secret from the/

> +/// BASE_NAME_CMD => read from specified file name

above comment should be:

/// read the secret from specified command first line of output on stdout 

what about newlines in secrets? I can have them in the "direct" use, where we may not
change that for backward compatibility, but not in the fd/file/command use?

I can get where this comes from, and while it feels a bit inconsistent it can be OK but must
be at least documented a bit more explicitly.

> +///
> +/// Only return the first line when reading from a file or command.
> +pub fn get_secret_from_env(base_name: &str) -> Result<Option<String>, Error> {
> +
> +    match std::env::var(base_name) {
> +        Ok(p) => return Ok(Some(p)),
> +        Err(NotUnicode(_)) => bail!(format!("{} contains bad characters", base_name)),
> +        Err(NotPresent) => {},
> +    };
> +
> +    let firstline = |data: String| -> String {
> +        match data.lines().next() {
> +            Some(line) => line.to_string(),
> +            None => String::new(),
> +        }
> +    };
> +
> +    let env_name = format!("{}_FD", base_name);
> +    match std::env::var(&env_name) {
> +        Ok(fd_str) => {
> +            let fd: i32 = fd_str.parse()
> +                .map_err(|err| format_err!("unable to parse file descriptor in ENV({}): {}", env_name, err))?;
> +            let mut file = unsafe { File::from_raw_fd(fd) };
> +            let mut buffer = String::new();
> +            let _ = file.read_to_string(&mut buffer)?;


Avoiding to read all of the file (which could be rather big in theory)
wouldn't be that much more code, i.e., above line would become

let mut reader = BufReader::new(file);
let _ = reader.read_line(&mut line)?;


> +            return Ok(Some(firstline(buffer)));
> +        }
> +        _ => {}
> +    }
> +
> +    let env_name = format!("{}_FILE", base_name);
> +    match std::env::var(&env_name) {
> +        Ok(filename) => {
> +            let data = proxmox::tools::fs::file_read_string(filename)?;

why not proxmox::tools::fs::file_read_firstline ?

looks OK besides that, and besides a more detailed rust-docs I have no hard feelings for the other things.