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

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

[Dietmar Maurer]

> 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.

In general, passwords should be arbitrary binary data &[u8]. But this is clumsy because we want to
use the password with our REST interface, which requires UTF8 (json). We also read password from the 
console and use newline as input terminator. So it is impossible to read password with newlines from  
the tty.

Sigh...

> > +///
> > +/// 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)?;

Unfortunately, this returns the line including the '\n'. Instead, we need to use
the "std::io::Lines" iterator: reader.lines().next()

> > +            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 ?

Because that also includes the newline ... 

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

Will send a v2 with your suggestions.

[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

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.