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

[Dietmar Maurer <dietmar@proxmox.com>]

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