From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id D3E5C1FF137 for ; Tue, 14 Apr 2026 16:31:52 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 2D8EE1C01A; Tue, 14 Apr 2026 16:32:42 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Tue, 14 Apr 2026 16:32:37 +0200 Message-Id: To: "Christian Ebner" , Subject: Re: [PATCH proxmox-backup v3 07/30] pbs-config: implement encryption key config handling From: =?utf-8?q?Michael_K=C3=B6ppl?= X-Mailer: aerc 0.21.0 References: <20260414125923.892345-1-c.ebner@proxmox.com> <20260414125923.892345-8-c.ebner@proxmox.com> In-Reply-To: <20260414125923.892345-8-c.ebner@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1776177081897 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.899 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 KAM_MAILER 2 Automated Mailer Tag Left in Email SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: KONC7ECJ7B3QRVWCNCRHBDZXXBNHVJII X-Message-ID-Hash: KONC7ECJ7B3QRVWCNCRHBDZXXBNHVJII X-MailFrom: m.koeppl@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 2 comments inline On Tue Apr 14, 2026 at 2:59 PM CEST, Christian Ebner wrote: [snip] > +/// Store the encryption key to file. > +/// > +/// Inserts the key in the config and stores it to the given file. > +pub fn store_key(id: &str, key: &KeyConfig) -> Result<(), Error> { > + let _lock =3D lock_config()?; > + let (mut config, _digest) =3D config()?; > + > + if config.sections.contains_key(id) { > + bail!("key with id '{id}' already exists."); > + } > + > + let backup_user =3D crate::backup_user()?; > + let dir_options =3D CreateOptions::new() > + .perm(Mode::from_bits_truncate(0o0750)) > + .owner(Uid::from_raw(0)) > + .group(backup_user.gid); > + > + proxmox_sys::fs::ensure_dir_exists(ENCRYPTION_KEYS_DIR, &dir_options= , true)?; > + > + let key_path =3D format!("{ENCRYPTION_KEYS_DIR}{id}.enc"); > + let key_lock_path =3D format!("{key_path}.lck"); > + > + // lock to avoid race with key deletion > + open_backup_lockfile(&key_lock_path, None, true)?; This needs to be assigned to a variable, no? Otherwise, the lock would be immediately dropped. In other places we have something like let _lock =3D lock_config()?; > + > + // assert the key file is empty or does not exist > + match std::fs::metadata(&key_path) { > + Ok(metadata) =3D> { > + if metadata.len() > 0 { > + bail!("detected pre-existing key file, refusing to overw= rite."); > + } > + } > + Err(err) if err.kind() =3D=3D std::io::ErrorKind::NotFound =3D> = (), > + Err(err) =3D> return Err(err.into()), > + } > + [snip] > +/// Delete the encryption key from config. > +/// > +/// Returns true if the key was removed successfully, false if there was= no matching key. > +/// Safety: caller must acquire and hold config lock. > +pub fn delete_key(id: &str, mut config: SectionConfigData) -> Result { > + if let Some((_, key)) =3D config.sections.remove(id) { > + let key =3D > + CryptKey::deserialize(key).map_err(|_err| format_err!("faile= d to parse key config"))?; > + > + if key.archived_at.is_none() { > + bail!("key still active, deleting is only possible for archi= ved keys"); > + } > + > + if let Some(key_path) =3D &key.info.path { > + let key_lock_path =3D format!("{key_path}.lck"); > + // Avoid races with key insertion > + let _lock =3D open_backup_lockfile(key_lock_path, None, true= )?; > + > + let key_config =3D KeyConfig::load(key_path)?; > + let stored_key_info =3D KeyInfo::from(&key_config); > + // Check the key is the expected one > + if key.info.fingerprint !=3D stored_key_info.fingerprint { > + bail!("unexpected key detected in key file, refuse to de= lete"); > + } > + > + let raw =3D CONFIG.write(ENCRYPTION_KEYS_CFG_FILENAME, &conf= ig)?; > + // drops config lock > + replace_backup_config(ENCRYPTION_KEYS_CFG_FILENAME, raw.as_b= ytes())?; > + > + std::fs::remove_file(key_path)?; Wouldn't it make more sense to delete the key before writing the config? If removing the key fails, an error would be returned, the key file would (possibly) still be there, but the config would already have been updated, leaving an orphaned key file. > + return Ok(true); > + } > + > + bail!("missing key file path for key '{id}'"); > + } > + Ok(false) > +} > + [snip]