Re: [pve-devel] [PATCH proxmox-backup-qemu] fix #2866: invalidate bitmap on crypt_mode change

[Stefan Reiter <s.reiter@proxmox.com>]

idea looks ok, comments inline

On 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.gruenbichler@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.

> +    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.

> +            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.

> +                    CryptMode::Encrypt => false,
> +                    _ => true,
> +                },
> +            }
> +        },
> +        _ => false,
> +    }
> +}
> +
> +
>   pub(crate) async fn register_image(
>       client: Arc<BackupWriter>,
>       crypt_config: Option<Arc<CryptConfig>>,
>