From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pbs-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
by lore.proxmox.com (Postfix) with ESMTPS id 7D2211FF15C
for <inbox@lore.proxmox.com>; Wed, 26 Mar 2025 10:45:33 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 53D3232A0F;
Wed, 26 Mar 2025 10:45:28 +0100 (CET)
References: <20250324123542.3248210-1-m.sandoval@proxmox.com>
<ogi3pcmhpreghkh23bt2dpcmlv6mf5fv5pyxh7626atpbrddpl@6lsfh3qn34c2>
User-agent: mu4e 1.10.8; emacs 30.1
From: Maximiliano Sandoval <m.sandoval@proxmox.com>
To: Wolfgang Bumiller <w.bumiller@proxmox.com>
Date: Wed, 26 Mar 2025 10:41:45 +0100
In-reply-to: <ogi3pcmhpreghkh23bt2dpcmlv6mf5fv5pyxh7626atpbrddpl@6lsfh3qn34c2>
Message-ID: <s8opli43z3d.fsf@proxmox.com>
MIME-Version: 1.0
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.097 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
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 backup] pbs-client: read credentials from
$CREDENTIALS_DIRECTORY
X-BeenThere: pbs-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox Backup Server development discussion
<pbs-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pbs-devel/>
List-Post: <mailto:pbs-devel@lists.proxmox.com>
List-Help: <mailto:pbs-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox Backup Server development discussion
<pbs-devel@lists.proxmox.com>
Cc: pbs-devel@lists.proxmox.com
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: pbs-devel-bounces@lists.proxmox.com
Sender: "pbs-devel" <pbs-devel-bounces@lists.proxmox.com>
Wolfgang Bumiller <w.bumiller@proxmox.com> writes:
>> const ENV_VAR_PBS_FINGERPRINT: &str = "PBS_FINGERPRINT";
>> const ENV_VAR_PBS_PASSWORD: &str = "PBS_PASSWORD";
>> +const ENV_VAR_PBS_ENCRYPTION_PASSWORD: &str = "PBS_ENCRYPTION_PASSWORD";
>> +
>> +/// Directory with system [credential]s. See systemd-creds(1).
>> +///
>> +/// [credential]: https://systemd.io/CREDENTIALS/
>> +const CREDENTIALS_DIRECTORY: &str = "CREDENTIALS_DIRECTORY";
>
> ^ The other env var consts have an `ENV_VAR_` prefix, so we should add
> that here for consistency.
>
>> +/// Credential name of the encryption password.
>> +const CRED_PBS_ENCRYPTION_PASSWORD: &str = "pbs-encryption-password";
>> +/// Credential name of the the password.
>> +const CRED_PBS_PASSWORD: &str = "pbs-password";
>>
>> pub const REPO_URL_SCHEMA: Schema = StringSchema::new("Repository URL.")
>> .format(&BACKUP_REPO_URL)
>> @@ -40,6 +50,43 @@ pub const CHUNK_SIZE_SCHEMA: Schema = IntegerSchema::new("Chunk size in KB. Must
>> .default(4096)
>> .schema();
>>
>> +/// Retrieves a secret stored in a [credential] provided by systemd.
>> +///
>> +/// Returns `None` if the credential does not exist or on errors.
>> +///
>> +/// [credential]: https://systemd.io/CREDENTIALS/
>> +pub fn get_credential(cred_name: &str) -> Option<Vec<u8>> {
>
> I think *failure* to read an *existing* credential should actually be
> propagated up. We don't want to just ignore this.
>
>> + // This is fundamentally a fn that returns a Result, but in practice we
>> + // would ignore errors so we hide the result in the public API.
>> + fn get_credential_inner(cred_name: &str) -> Result<Option<Vec<u8>>, Error> {
>> + let Some(creds_dir) = std::env::var_os(CREDENTIALS_DIRECTORY) else {
>> + return Ok(None);
>> + };
>> + let path = std::path::Path::new(&creds_dir).join(cred_name);
>> +
>> + let mut file = File::open(&path)?;
>> + let mut buffer = vec![];
>> +
>> + // We read the whole contents without a BufRead. As per
>> + // systemd-creds(1): Credentials are limited-size binary or textual
>> + // objects.
>> + file.read_to_end(&mut buffer)?;
>> +
>> + Ok(Some(buffer))
>> + }
>> + get_credential_inner(cred_name)
>> + .inspect_err(|err| {
>> + if err
>> + .downcast_ref::<std::io::Error>()
>> + .is_some_and(|e| e.kind() != std::io::ErrorKind::NotFound)
>> + {
>> + proxmox_log::error!("{err:#}")
>> + }
>> + })
>> + .ok()
>> + .flatten()
>
> I think this entire fn could be reduced to a std::fs::read() call with
> the path included in the logged error, and maybe some more debug-logging
> so that with debug-logging the log doesnt' stay empty on eg. ENOENT.
>
> let Some(creds_dir) = std::env::var_os(ENV_VAR_CREDENTIALS_DIRECTORY) else {
> return Ok(None);
> }
> let path = Path::new(creds_dir).join(cred_name);
> debug!("attempting to use credential {cred_name:?} from {creds_dir:?}");
> match std::fs::read(&path) {
> Ok(data) => Ok(Some(data)),
> Err(err) if err.kind() == io::ErrorKind::NotFound => {
> debug!("no {cred_name:?} credential found in {creds_dir:?}");
> Ok(None)
> }
> Err(err) => Err(err),
> }
>
>> +}
>> +
>> /// Helper to read a secret through a environment variable (ENV).
>> ///
>> /// Tries the following variable names in order and returns the value
>> @@ -118,6 +165,34 @@ pub fn get_secret_from_env(base_name: &str) -> Result<Option<String>, Error> {
>> Ok(None)
>> }
>>
>> +/// Gets the backup server's password.
>> +///
>> +/// We first try reading from the `PBS_PASSWORD` environment variable, then we
>> +/// try reading from the `pbs-password` [credential]. We ignore errors on the
>> +/// latter.
>> +///
>> +/// [credential]: https://systemd.io/CREDENTIALS/
>> +pub fn get_password() -> Result<Option<String>, Error> {
>> + let password = get_secret_from_env(ENV_VAR_PBS_PASSWORD)?.or_else(|| {
>> + get_credential(CRED_PBS_PASSWORD).and_then(|bytes| String::from_utf8(bytes).ok())
>
> A potential UTF-8 error here should be logged here!
>
> I think the function would be a bit more readable with a simple
>
> if let Some(pw) = get_secret_from_env(...)? {
> return Ok(pw);
> }
> if let Some(pw) = get_credential(...)? {
> return String::from_utf8(pw)
> .map_err(|_| format_err!("non-utf8 password credential"));
> }
>
>> + });
>> + Ok(password)
>> +}
>> +
>> +/// Gets an encryption password.
>> +///
>> +/// We first try reading from the `PBS_ENCRYPTION_PASSWORD` environment
>> +/// variable, then we try reading from the `pbs-encryption-password`
>> +/// [credential]. We ignore errors on the latter.
>> +///
>> +/// [credential]: https://systemd.io/CREDENTIALS/
>> +pub fn get_encryption_password() -> Result<Option<Vec<u8>>, Error> {
>> + let password = get_secret_from_env(ENV_VAR_PBS_ENCRYPTION_PASSWORD)?
>> + .map(String::into_bytes)
>> + .or_else(|| get_credential(CRED_PBS_ENCRYPTION_PASSWORD));
>
> This and `get_password` can just use the same helper function. They both
> only support utf-8 strings anyway from what I can tell.
> So this would just be
>
> get_secret_impl(ENV_VAR_PBS_ENCRYPTION_PASSWORD, CRED_PBS_ENCRYPTION_PASSWORD)
> .map(String::into_bytes)
>
I didn't implement this last part, imho the API feels better in this
way.
v2 sent at
https://lore.proxmox.com/pbs-devel/20250326094138.75437-1-m.sandoval@proxmox.com/T/#t.
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel