From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 796D776FF5 for ; Mon, 19 Jul 2021 15:41:22 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 662F927532 for ; Mon, 19 Jul 2021 15:40:52 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 2885427522 for ; Mon, 19 Jul 2021 15:40:51 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id E7854422C4 for ; Mon, 19 Jul 2021 15:40:50 +0200 (CEST) Message-ID: Date: Mon, 19 Jul 2021 15:40:25 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.0 Content-Language: en-US To: Proxmox Backup Server development discussion , Dietmar Maurer References: <20210719092114.4073945-1-dietmar@proxmox.com> From: Thomas Lamprecht In-Reply-To: <20210719092114.4073945-1-dietmar@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.466 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.07 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH proxmox-backup 1/2] support more ENV vars to get secret values X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 19 Jul 2021 13:41:22 -0000 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, 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.