On 7/23/20 12:34 PM, Fabian Grünbichler wrote:
On July 23, 2020 12:07 pm, Stefan Reiter wrote:idea looks ok, comments inlineOn 7/23/20 11:21 AM, Fabian Grünbichler wrote:signed and plain backups share chunks, so bitmap reusal is okay for those combinations. switching from encrypted to not encrypted or vice-versa could have pretty fatal consequences - either referencing plain-text chunks in 'encrypted' backups, or referencing encrypted chunks in 'unencrypted' backups without still having the corresponding keys.. Signed-off-by: Fabian Grünbichler <f.gruenbich...@proxmox.com> --- Notes: requires recent proxmox-backup with public lookup_file_info src/backup.rs | 3 ++- src/commands.rs | 35 +++++++++++++++++++++++++++++++++-- 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/src/backup.rs b/src/backup.rs index 717e099..b8108ef 100644 --- a/src/backup.rs +++ b/src/backup.rs @@ -202,7 +202,8 @@ impl BackupTask { device_name: String, size: u64, ) -> bool { - check_last_incremental_csum(self.last_manifest(), device_name, size) + check_last_incremental_csum(self.last_manifest(), &device_name, size) + && check_last_encryption_mode(self.last_manifest(), &device_name, self.crypt_mode) }pub async fn register_image(diff --git a/src/commands.rs b/src/commands.rs index 6f26324..8d8f2a7 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -80,7 +80,7 @@ pub(crate) async fn add_config(pub(crate) fn check_last_incremental_csum(manifest: Option<Arc<BackupManifest>>, - device_name: String, + device_name: &str, device_size: u64, ) -> bool {@@ -91,12 +91,43 @@ pub(crate) fn check_last_incremental_csum( let archive_name = format!("{}.img.fidx", device_name); - match PREVIOUS_CSUMS.lock().unwrap().get(&device_name) {+ match PREVIOUS_CSUMS.lock().unwrap().get(device_name) { Some(csum) => manifest.verify_file(&archive_name, &csum, device_size).is_ok(), None => false, } }+pub(crate) fn check_last_encryption_mode(+ manifest: Option<Arc<BackupManifest>>, + device_name: &str, + crypt_mode: CryptMode, +) -> bool { + + let manifest = match manifest { + Some(ref manifest) => manifest, + None => return false, + };this...+ + let archive_name = format!("{}.img.fidx", device_name);...and this could probably be moved to check_incremental to avoid duplication.probably device to archive name could also be refactored into a helper? with this patch we have three identical format! calls..
would make sense, or at least encode the .img.fidx in a constant somewhere
+ match manifest.lookup_file_info(&archive_name) { + Ok(file) => { + eprintln!("device {} last mode: {:?} current mode {:?}", device_name, file.crypt_mode, crypt_mode);left over debug print or intentional? this would be hidden atm, as we don't track QEMU output anywhere.both :-P I figured with all the issues we had with encrypted backups, telling users to start in the foreground and watch the output might be helpful. but I'm fine with dropping it.
I suppose this would be a good point to ping this patch of mine: https://lists.proxmox.com/pipermail/pve-devel/2020-June/044143.htmlThough in case we want to actually use it this way, maybe even a bit more logging would be good?
+ match file.crypt_mode { + CryptMode::Encrypt => match crypt_mode { + CryptMode::Encrypt => true, + _ => false, + }, + CryptMode::SignOnly | CryptMode::None => match crypt_mode {you can use the _ match here too, same as in the inner match call.intentional, if we add a new CryptMode in proxmox-backup this forces us to match it here unless I misunderstood how match on enums works in Rust.
makes sense, though should probably be mentioned somewhere so no one "optimizes" it away in the future.
+ CryptMode::Encrypt => false, + _ => true, + }, + } + }, + _ => false, + } +} + + pub(crate) async fn register_image( client: Arc<BackupWriter>, crypt_config: Option<Arc<CryptConfig>>,
_______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
