From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 0AB4D1FF13A for ; Wed, 15 Apr 2026 08:48:23 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D509F268F; Wed, 15 Apr 2026 08:48:22 +0200 (CEST) Message-ID: <576f8999-1a20-417a-9c6d-ec79467661da@proxmox.com> Date: Wed, 15 Apr 2026 08:48:17 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH proxmox-backup v3 07/30] pbs-config: implement encryption key config handling To: =?UTF-8?Q?Michael_K=C3=B6ppl?= , pbs-devel@lists.proxmox.com References: <20260414125923.892345-1-c.ebner@proxmox.com> <20260414125923.892345-8-c.ebner@proxmox.com> Content-Language: en-US, de-DE From: Christian Ebner In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1776235620449 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.931 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 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 Message-ID-Hash: GGOKCP4H3JUGDHE43IG3EIEXCAOJQ4LZ X-Message-ID-Hash: GGOKCP4H3JUGDHE43IG3EIEXCAOJQ4LZ X-MailFrom: c.ebner@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: On 4/14/26 4:31 PM, Michael Köppl wrote: > 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 = lock_config()?; >> + let (mut config, _digest) = config()?; >> + >> + if config.sections.contains_key(id) { >> + bail!("key with id '{id}' already exists."); >> + } >> + >> + let backup_user = crate::backup_user()?; >> + let dir_options = 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 = format!("{ENCRYPTION_KEYS_DIR}{id}.enc"); >> + let key_lock_path = 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. Oh, good catch! Indeed without this the lock would be immediately dropped, will be fixed. Thanks! > > In other places we have something like let _lock = lock_config()?; > >> + >> + // assert the key file is empty or does not exist >> + match std::fs::metadata(&key_path) { >> + Ok(metadata) => { >> + if metadata.len() > 0 { >> + bail!("detected pre-existing key file, refusing to overwrite."); >> + } >> + } >> + Err(err) if err.kind() == std::io::ErrorKind::NotFound => (), >> + Err(err) => 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)) = config.sections.remove(id) { >> + let key = >> + CryptKey::deserialize(key).map_err(|_err| format_err!("failed to parse key config"))?; >> + >> + if key.archived_at.is_none() { >> + bail!("key still active, deleting is only possible for archived keys"); >> + } >> + >> + if let Some(key_path) = &key.info.path { >> + let key_lock_path = format!("{key_path}.lck"); >> + // Avoid races with key insertion >> + let _lock = open_backup_lockfile(key_lock_path, None, true)?; >> + >> + let key_config = KeyConfig::load(key_path)?; >> + let stored_key_info = KeyInfo::from(&key_config); >> + // Check the key is the expected one >> + if key.info.fingerprint != stored_key_info.fingerprint { >> + bail!("unexpected key detected in key file, refuse to delete"); >> + } >> + >> + let raw = CONFIG.write(ENCRYPTION_KEYS_CFG_FILENAME, &config)?; >> + // drops config lock >> + replace_backup_config(ENCRYPTION_KEYS_CFG_FILENAME, raw.as_bytes())?; >> + >> + 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. The ordering here was chosen as is on purpose, see https://lore.proxmox.com/pbs-devel/b7d0f730-a574-4454-bf18-933db202db8b@proxmox.com/ > >> + return Ok(true); >> + } >> + >> + bail!("missing key file path for key '{id}'"); >> + } >> + Ok(false) >> +} >> + > > [snip]