Re: [pve-devel] [PATCH proxmox-backup-qemu 2/2] invalidate bitmap when crypto key changes

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

On October 21, 2020 5:17 pm, Stefan Reiter wrote:
> On 10/21/20 1:49 PM, Fabian Grünbichler wrote:
>> by computing and remembering the ID digest of a static string, we can
>> detect when the passed in key has changed without keeping a copy of it
>> around inbetween backup jobs.
>> 
>> this is a follow-up/fixup for
>> 
>> 104fae9111cd9a4e4dd7779172d39580a393165d fix #2866: invalidate bitmap on crypt_mode change
>> 
>> which implemented detection for crypt MODE changes.
>> 
>> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>> ---
>> note: the string is just an implementation detail right now, but once we
>> start sending that along with migration or persisting it together with
>> bitmaps, we probably want a more robust scheme and store the input
>> string + digest
> 
> I'm not sure I follow... as long as this string stays the same it 
> shouldn't ever be necessary to store it alongside the digest?

yes. but once this is no longer something that's only valid for a single 
process and not leaked (in which case, we can just change it between 
versions as there is no compatibility concern), I'd make sure we can 
change it in case we need to, and the easiest way is to include both 
input and output so that the new version can verify the old one :)

> In any case, this works fine:
> 
> Reviewed-by: Stefan Reiter <s.reiter@proxmox.com>

thanks!

> 
>> 
>>   src/backup.rs   |  1 +
>>   src/commands.rs | 36 ++++++++++++++++++++++++++++++++++++
>>   2 files changed, 37 insertions(+)
>> 
>> diff --git a/src/backup.rs b/src/backup.rs
>> index 7945575..9f6d176 100644
>> --- a/src/backup.rs
>> +++ b/src/backup.rs
>> @@ -203,6 +203,7 @@ impl BackupTask {
>>               Some(ref manifest) => {
>>                   check_last_incremental_csum(Arc::clone(manifest), &device_name, size)
>>                       && check_last_encryption_mode(Arc::clone(manifest), &device_name, self.crypt_mode)
>> +                    && check_last_encryption_key(self.crypt_config.clone())
>>               },
>>               None => false,
>>           }
>> diff --git a/src/commands.rs b/src/commands.rs
>> index b9b0161..f7e0f62 100644
>> --- a/src/commands.rs
>> +++ b/src/commands.rs
>> @@ -19,6 +19,10 @@ lazy_static!{
>>       static ref PREVIOUS_CSUMS: Mutex<HashMap<String, [u8;32]>> = {
>>           Mutex::new(HashMap::new())
>>       };
>> +
>> +    static ref PREVIOUS_CRYPT_CONFIG_DIGEST: Mutex<Option<[u8;32]>> = {
>> +        Mutex::new(None)
>> +    };
>>   }
>>   
>>   pub struct ImageUploadInfo {
>> @@ -82,6 +86,15 @@ fn archive_name(device_name: &str) -> String {
>>       format!("{}.img.fidx", device_name)
>>   }
>>   
>> +const CRYPT_CONFIG_HASH_INPUT:&[u8] = b"this is just a static string to protect against key changes";
>> +
>> +/// Create an identifying digest for the crypt config
>> +pub(crate) fn crypt_config_digest(
>> +    config: Arc<CryptConfig>,
>> +) -> [u8;32] {
>> +    config.compute_digest(CRYPT_CONFIG_HASH_INPUT)
>> +}
>> +
>>   pub(crate) fn check_last_incremental_csum(
>>       manifest: Arc<BackupManifest>,
>>       device_name: &str,
>> @@ -114,6 +127,19 @@ pub(crate) fn check_last_encryption_mode(
>>       }
>>   }
>>   
>> +pub(crate) fn check_last_encryption_key(
>> +    config: Option<Arc<CryptConfig>>,
>> +) -> bool {
>> +    let digest_guard = PREVIOUS_CRYPT_CONFIG_DIGEST.lock().unwrap();
>> +    match (*digest_guard, config)  {
>> +        (Some(last_digest), Some(current_config)) => {
>> +            crypt_config_digest(current_config) == last_digest
>> +        },
>> +        (None, None) => true,
>> +        _ => false,
>> +    }
>> +}
>> +
>>   #[allow(clippy::too_many_arguments)]
>>   pub(crate) async fn register_image(
>>       client: Arc<BackupWriter>,
>> @@ -393,6 +419,16 @@ pub(crate) async fn finish_backup(
>>               .map_err(|err| format_err!("unable to format manifest - {}", err))?
>>       };
>>   
>> +    {
>> +        let crypt_config_digest = match crypt_config {
>> +            Some(current_config) => Some(crypt_config_digest(current_config)),
>> +            None => None,
>> +        };
>> +
>> +        let mut crypt_config_digest_guard = PREVIOUS_CRYPT_CONFIG_DIGEST.lock().unwrap();
>> +        *crypt_config_digest_guard = crypt_config_digest;
>> +    }
>> +
>>       client
>>           .upload_blob_from_data(manifest.into_bytes(), MANIFEST_BLOB_NAME, true, false)
>>           .await?;
>> 
>