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 E00CB1FF141 for ; Tue, 05 May 2026 12:05:04 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 22409222A5; Tue, 5 May 2026 12:05:04 +0200 (CEST) Date: Tue, 05 May 2026 12:04:22 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= Subject: Re: [PATCH proxmox-backup 4/6] sync: wire up strict encryption mode To: pbs-devel@lists.proxmox.com, Shannon Sterz References: <20260429140941.3537494-1-f.gruenbichler@proxmox.com> <20260429140941.3537494-5-f.gruenbichler@proxmox.com> In-Reply-To: MIME-Version: 1.0 User-Agent: astroid/0.17.0 (https://github.com/astroidmail/astroid) Message-Id: <1777975035.pgb4ylseln.astroid@yuna.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1777975359562 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.054 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 SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [sync.rs,pull.rs,push.rs] Message-ID-Hash: 2VW42SYCMQYMADLIEUJIBAPM7I7YJNKD X-Message-ID-Hash: 2VW42SYCMQYMADLIEUJIBAPM7I7YJNKD X-MailFrom: f.gruenbichler@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 May 5, 2026 10:12 am, Shannon Sterz wrote: > On Wed Apr 29, 2026 at 4:09 PM CEST, Fabian Gr=C3=BCnbichler wrote: >> enabling it requires configured encryption/decryption keys, disabling or >> unsetting it does not. >> >> Signed-off-by: Fabian Gr=C3=BCnbichler >> --- >> src/api2/config/sync.rs | 30 +++++++++++++++++++++++++++++- >> src/api2/pull.rs | 13 +++++++++---- >> src/api2/push.rs | 10 ++++++++-- >> src/server/sync.rs | 2 +- >> 4 files changed, 47 insertions(+), 8 deletions(-) >> >> diff --git a/src/api2/config/sync.rs b/src/api2/config/sync.rs >> index eb82745d9..ac1e6350e 100644 >> --- a/src/api2/config/sync.rs >> +++ b/src/api2/config/sync.rs >> @@ -276,9 +276,16 @@ pub fn create_sync_job( >> .unwrap_or_else(|| Authid::root_auth_id()); >> >> if sync_direction =3D=3D SyncDirection::Push { >> + if (config.strict_encryption_mode =3D=3D Some(true)) !=3D confi= g.active_encryption_key.is_some() { >> + bail!("'strict-encryption-mode' requires encryption key"); >> + } >=20 > is this also supposed to trigger if strict-encryption-mode is false or > not specified, but an active encryption key is set? if i'm not > misreading this, that's what would happen here. right, this is wrong.. strict | key | result --------------------- Y | Y | true !=3D true =3D> false =3D> OK N | Y | false !=3D true =3D> true =3D> NOT OK Y | N | true !=3D false =3D> true =3D> OK (we want to warn) N | N | false !=3D false =3D> false =3D> OK >=20 > if no maybe: >=20 > (data.strict_encryption_mode =3D=3D Some(true)) && !data.active_encry= ption_key.is_some() >=20 > would be slightly more legible? yes, or maybe unwrap_or(false) && !.. since it's just a bool we don't have to worry about being smart here.. >=20 >> sync_user_can_access_optional_key(config.active_encryption_key.= as_deref(), owner, true)?; >> } else { >> - for key in config.associated_key.as_deref().unwrap_or(&[]) { >> + let keys =3D config.associated_key.as_deref().unwrap_or(&[]); >> + if (config.strict_encryption_mode =3D=3D Some(true)) =3D=3D key= s.is_empty() { >> + bail!("'strict-encryption-mode' requires encryption key(s)"= ); >=20 > same here, is this supposed to fail when strict-mode is false and no > keys are specified? same as above :) >=20 >> + } >> + for key in keys { >> sync_user_can_access_optional_key(Some(key), owner, false)?= ; >> } >> } >> @@ -398,6 +405,8 @@ pub enum DeletableProperty { >> ActiveEncryptionKey, >> /// Delete associated key property, >> AssociatedKey, >> + /// Delete strict encryption_mode property, >> + StrictEncryptionMode, >> } >> >> #[api( >> @@ -538,6 +547,9 @@ pub fn update_sync_job( >> // Previous active encryption key might be added as= associated below. >> data.associated_key =3D None; >> } >> + DeletableProperty::StrictEncryptionMode =3D> { >> + data.strict_encryption_mode =3D None; >> + } >> } >> } >> keep_previous_key_as_associated( >> @@ -654,6 +666,21 @@ pub fn update_sync_job( >> data.associated_key =3D Some(associated_key); >> } >> >> + if let Some(strict_encryption_mode) =3D update.strict_encryption_mo= de { >> + data.strict_encryption_mode =3D Some(strict_encryption_mode); >> + } >> + >> + if data.sync_direction =3D=3D Some(SyncDirection::Push) { >> + if (data.strict_encryption_mode =3D=3D Some(true)) !=3D data.ac= tive_encryption_key.is_some() { >> + bail!("'strict-encryption-mode' requires encryption key"); >=20 > see above >=20 >> + } >> + } else { >> + let keys =3D data.associated_key.as_deref().unwrap_or(&[]); >> + if (data.strict_encryption_mode =3D=3D Some(true)) =3D=3D keys.= is_empty() { >> + bail!("'strict-encryption-mode' requires encryption key(s)"= ); >=20 > see above I'll adapt (and maybe pull it into a helper) for v2, thanks for catching it!