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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On July 23, 2020 12:07 pm, Stefan Reiter wrote:
> 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.

probably device to archive name could also be refactored into a helper? 
with this patch we have three identical format! calls..

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

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

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