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.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..
>
>> + 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>>,
>>
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
