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

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

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.