[pve-devel] [PATCH proxmox-backup-qemu 1/2] update to proxmox-backup 0.9.1

[pve-devel] [PATCH proxmox-backup-qemu 1/2] update to proxmox-backup 0.9.1

[Fabian Grünbichler]

and pass along any port set in the repository string

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
Note: to switch to current proxmox 0.5.0, proxmox-backup needs to be
bumped first.. no changes are needed in proxmox-backup-qemu to build
with current master.

 Cargo.toml     | 4 ++--
 src/backup.rs  | 2 +-
 src/lib.rs     | 3 +++
 src/restore.rs | 2 +-
 4 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/Cargo.toml b/Cargo.toml
index 043ae96..d51ba93 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -23,8 +23,8 @@ lazy_static = "1.4"
 libc = "0.2"
 once_cell = "1.3.1"
 openssl = "0.10"
-proxmox = { version = "0.4.1", features = [ "sortable-macro", "api-macro" ] }
-proxmox-backup = { git = "git://git.proxmox.com/git/proxmox-backup.git", tag = "v0.8.21" }
+proxmox = { version = "0.4.3", features = [ "sortable-macro", "api-macro" ] }
+proxmox-backup = { git = "git://git.proxmox.com/git/proxmox-backup.git", tag = "v0.9.1" }
 #proxmox-backup = { path = "../proxmox-backup" }
 serde_json = "1.0"
 tokio = { version = "0.2.9", features = [ "blocking", "fs", "io-util", "macros", "rt-threaded", "signal", "stream", "tcp", "time", "uds" ] }
diff --git a/src/backup.rs b/src/backup.rs
index 9f8a240..7945575 100644
--- a/src/backup.rs
+++ b/src/backup.rs
@@ -112,7 +112,7 @@ impl BackupTask {
                 .fingerprint(self.setup.fingerprint.clone())
                 .password(self.setup.password.clone());
 
-            let http = HttpClient::new(&self.setup.host, &self.setup.user, options)?;
+            let http = HttpClient::new(&self.setup.host, self.setup.port, &self.setup.user, options)?;
             let writer = BackupWriter::start(
                 http, self.crypt_config.clone(), &self.setup.store, "vm", &self.setup.backup_id,
                 self.setup.backup_time, false, false).await?;
diff --git a/src/lib.rs b/src/lib.rs
index 08cac4c..b9ca905 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -114,6 +114,7 @@ pub extern "C" fn proxmox_backup_snapshot_string(
 #[derive(Clone)]
 pub(crate) struct BackupSetup {
     pub host: String,
+    pub port: u16,
     pub store: String,
     pub user: Userid,
     pub chunk_size: u64,
@@ -221,6 +222,7 @@ pub extern "C" fn proxmox_backup_new(
 
         let setup = BackupSetup {
             host: repo.host().to_owned(),
+            port: repo.port(),
             user: repo.user().to_owned(),
             store: repo.store().to_owned(),
             chunk_size: if chunk_size > 0 { chunk_size } else { PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE },
@@ -693,6 +695,7 @@ pub extern "C" fn proxmox_restore_new(
 
         let setup = BackupSetup {
             host: repo.host().to_owned(),
+            port: repo.port(),
             user: repo.user().to_owned(),
             store: repo.store().to_owned(),
             chunk_size: PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE, // not used by restore
diff --git a/src/restore.rs b/src/restore.rs
index fb779ca..4387025 100644
--- a/src/restore.rs
+++ b/src/restore.rs
@@ -80,7 +80,7 @@ impl RestoreTask {
             .fingerprint(self.setup.fingerprint.clone())
             .password(self.setup.password.clone());
 
-        let http = HttpClient::new(&self.setup.host, &self.setup.user, options)?;
+        let http = HttpClient::new(&self.setup.host, self.setup.port, &self.setup.user, options)?;
         let client = BackupReader::start(
             http,
             self.crypt_config.clone(),
-- 
2.20.1

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

[Fabian Grünbichler]

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

 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?;
-- 
2.20.1

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

[Stefan Reiter]

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?

In any case, this works fine:

Reviewed-by: Stefan Reiter <s.reiter@proxmox.com>

> 
>   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?;
> 

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

[Fabian Grünbichler]

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?;
>> 
> 

[pve-devel] applied-series: [PATCH proxmox-backup-qemu 1/2] update to proxmox-backup 0.9.1

[Thomas Lamprecht]

On 21.10.20 13:49, Fabian Grünbichler wrote:
> and pass along any port set in the repository string
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
> Note: to switch to current proxmox 0.5.0, proxmox-backup needs to be
> bumped first.. no changes are needed in proxmox-backup-qemu to build
> with current master.
> 
>  Cargo.toml     | 4 ++--
>  src/backup.rs  | 2 +-
>  src/lib.rs     | 3 +++
>  src/restore.rs | 2 +-
>  4 files changed, 7 insertions(+), 4 deletions(-)
> 
>

applied seires, thanks!