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

[Stefan Reiter]

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 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 at 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.html

Though 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>>,
>>>
>>