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

[Fabian Grünbichler]

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

I thought this is such a basic helpful rust feature that everybody uses 
it - is there a reason to avoid it? IMHO matching like this instead of 
using a wildcard is great, since the compiler will shout at me and tell 
me all the places I potentially need to adapt when I extend an enum.. so 
it should be clear that this is not an optimization, but disabling a 
compiler check that should not be done without a reason?

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