From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id A283861A73 for ; Thu, 22 Oct 2020 09:51:59 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 968E81E0C0 for ; Thu, 22 Oct 2020 09:51:59 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id EFF501E0B6 for ; Thu, 22 Oct 2020 09:51:58 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id B9DDD409AE for ; Thu, 22 Oct 2020 09:51:58 +0200 (CEST) Date: Thu, 22 Oct 2020 09:51:51 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: pve-devel@lists.proxmox.com, Stefan Reiter References: <20201021114951.3006517-1-f.gruenbichler@proxmox.com> <20201021114951.3006517-2-f.gruenbichler@proxmox.com> <538c080f-25c7-b10c-bc93-8e23ec0a07e8@proxmox.com> In-Reply-To: <538c080f-25c7-b10c-bc93-8e23ec0a07e8@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.15.0 (https://github.com/astroidmail/astroid) Message-Id: <1603353001.5rl0cqxqid.astroid@nora.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.031 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust 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. [commands.rs, backup.rs] Subject: Re: [pve-devel] [PATCH proxmox-backup-qemu 2/2] invalidate bitmap when crypto key changes X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 22 Oct 2020 07:51:59 -0000 On October 21, 2020 5:17 pm, Stefan Reiter wrote: > On 10/21/20 1:49 PM, Fabian Gr=C3=BCnbichler wrote: >> by computing and remembering the ID digest of a static string, we can >> detect when the passed in key has changed without keeping a copy of it >> around inbetween backup jobs. >>=20 >> this is a follow-up/fixup for >>=20 >> 104fae9111cd9a4e4dd7779172d39580a393165d fix #2866: invalidate bitmap on= crypt_mode change >>=20 >> which implemented detection for crypt MODE changes. >>=20 >> Signed-off-by: Fabian Gr=C3=BCnbichler >> --- >> note: the string is just an implementation detail right now, but once we >> start sending that along with migration or persisting it together with >> bitmaps, we probably want a more robust scheme and store the input >> string + digest >=20 > I'm not sure I follow... as long as this string stays the same it=20 > shouldn't ever be necessary to store it alongside the digest? yes. but once this is no longer something that's only valid for a single=20 process and not leaked (in which case, we can just change it between=20 versions as there is no compatibility concern), I'd make sure we can=20 change it in case we need to, and the easiest way is to include both=20 input and output so that the new version can verify the old one :) > In any case, this works fine: >=20 > Reviewed-by: Stefan Reiter thanks! >=20 >>=20 >> src/backup.rs | 1 + >> src/commands.rs | 36 ++++++++++++++++++++++++++++++++++++ >> 2 files changed, 37 insertions(+) >>=20 >> diff --git a/src/backup.rs b/src/backup.rs >> index 7945575..9f6d176 100644 >> --- a/src/backup.rs >> +++ b/src/backup.rs >> @@ -203,6 +203,7 @@ impl BackupTask { >> Some(ref manifest) =3D> { >> check_last_incremental_csum(Arc::clone(manifest), &dev= ice_name, size) >> && check_last_encryption_mode(Arc::clone(manifest)= , &device_name, self.crypt_mode) >> + && check_last_encryption_key(self.crypt_config.clon= e()) >> }, >> None =3D> false, >> } >> diff --git a/src/commands.rs b/src/commands.rs >> index b9b0161..f7e0f62 100644 >> --- a/src/commands.rs >> +++ b/src/commands.rs >> @@ -19,6 +19,10 @@ lazy_static!{ >> static ref PREVIOUS_CSUMS: Mutex> =3D { >> Mutex::new(HashMap::new()) >> }; >> + >> + static ref PREVIOUS_CRYPT_CONFIG_DIGEST: Mutex> =3D= { >> + Mutex::new(None) >> + }; >> } >> =20 >> pub struct ImageUploadInfo { >> @@ -82,6 +86,15 @@ fn archive_name(device_name: &str) -> String { >> format!("{}.img.fidx", device_name) >> } >> =20 >> +const CRYPT_CONFIG_HASH_INPUT:&[u8] =3D b"this is just a static string = to protect against key changes"; >> + >> +/// Create an identifying digest for the crypt config >> +pub(crate) fn crypt_config_digest( >> + config: Arc, >> +) -> [u8;32] { >> + config.compute_digest(CRYPT_CONFIG_HASH_INPUT) >> +} >> + >> pub(crate) fn check_last_incremental_csum( >> manifest: Arc, >> device_name: &str, >> @@ -114,6 +127,19 @@ pub(crate) fn check_last_encryption_mode( >> } >> } >> =20 >> +pub(crate) fn check_last_encryption_key( >> + config: Option>, >> +) -> bool { >> + let digest_guard =3D PREVIOUS_CRYPT_CONFIG_DIGEST.lock().unwrap(); >> + match (*digest_guard, config) { >> + (Some(last_digest), Some(current_config)) =3D> { >> + crypt_config_digest(current_config) =3D=3D last_digest >> + }, >> + (None, None) =3D> true, >> + _ =3D> false, >> + } >> +} >> + >> #[allow(clippy::too_many_arguments)] >> pub(crate) async fn register_image( >> client: Arc, >> @@ -393,6 +419,16 @@ pub(crate) async fn finish_backup( >> .map_err(|err| format_err!("unable to format manifest - {}= ", err))? >> }; >> =20 >> + { >> + let crypt_config_digest =3D match crypt_config { >> + Some(current_config) =3D> Some(crypt_config_digest(current_= config)), >> + None =3D> None, >> + }; >> + >> + let mut crypt_config_digest_guard =3D PREVIOUS_CRYPT_CONFIG_DIG= EST.lock().unwrap(); >> + *crypt_config_digest_guard =3D crypt_config_digest; >> + } >> + >> client >> .upload_blob_from_data(manifest.into_bytes(), MANIFEST_BLOB_NA= ME, true, false) >> .await?; >>=20 >=20 =