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 AFC521FF143 for ; Sat, 11 Apr 2026 10:51:31 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D9B8F4DBE; Sat, 11 Apr 2026 10:52:11 +0200 (CEST) From: Thomas Lamprecht To: c.ebner@proxmox.com Subject: Re: [PATCH proxmox-backup v2 06/27] pbs-config: implement encryption key config handling Date: Sat, 11 Apr 2026 10:02:07 +0200 Message-ID: <20260411085154.1961287-3-t.lamprecht@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20260410165454.1578501-7-c.ebner@proxmox.com> References: <20260411085154.1961287-1-t.lamprecht@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1775897454793 X-SPAM-LEVEL: Spam detection results: 0 AWL -1.049 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 PROLO_LEO1 0.1 Meta Catches all Leo drug variations so far 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: XT4M6WPVRL4T3ED44XHIFE63BP7OVPB5 X-Message-ID-Hash: XT4M6WPVRL4T3ED44XHIFE63BP7OVPB5 X-MailFrom: t.lamprecht@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 CC: pbs-devel@lists.proxmox.com 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: Am 10.04.26 um 18:54 schrieb Christian Ebner: > diff --git a/pbs-config/src/encryption_keys.rs b/pbs-config/src/encryption_keys.rs > @@ -0,0 +1,210 @@ > > + let key_path = format!("{ENCRYPTION_KEYS_DIR}{id}.enc"); > + > + // lock to avoid race with key deletion, file ownership and permissions will > + // be adapted by replacing file on key store below. > + open_backup_lockfile(&key_path, None, true)?; > + > + // assert the key file is empty (new) > + let metadata = std::fs::metadata(&key_path)?; > + if metadata.len() > 0 { > + bail!("detected pre-existing key file, refusing to overwrite."); > + } The lockfile _is_ the key file path, which for one makes the check bogus AFAICT, as a previous open_backup_lockfile creates it if it does not exist. So the metadata check always succeeds on a fresh create (file exists, length 0), and on a stale leftover from a previous failed attempt it also passes (empty lockfile). And for another reusing the file to protect as lock file can have problems as the lock file never must be deleted, but for atomic (config, key, ...) writes one needs to use write-tmp+rename, which then breaks locking. I'd add and use something like: let key_lock_path = format!("{key_path}.lck"); Then the metadata len check afterwards should also work. > +pub fn delete_key(id: &str, mut config: SectionConfigData) -> Result { nit: this takes an owned SectionConfigData, which is unusual compared to the other config helpers that manage their own lock. Also, the caller (API handler) holds the lock, which is fine, but a doc comment noting that requirment might be good here. > + replace_backup_config(ENCRYPTION_KEYS_CFG_FILENAME, raw.as_bytes())?; > + > + // drop key file lock > + std::fs::remove_file(key_path)?; nit: if remove_file fails here, the config was already updated (key section removed). Not dangerous since the key is no longer referenced, but you'd leave a stale .enc file. Maybe just log a warning and return Ok instead of propagating the error?