[pbs-devel] [PATCH proxmox-backup(-qemu) 0/6] various crpyo/digest enhancements

[pbs-devel] [PATCH proxmox-backup(-qemu) 0/6] various crpyo/digest enhancements

[Fabian Grünbichler]

mostly blocking some requests that don't make sense, and adding
additional checks.

[pbs-devel] [PATCH proxmox-backup-qemu] adapt to chunk reader changes

[Fabian Grünbichler]

for verification of chunk crypt mode <=> index crypt mode match

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
follows patch #4 in proxmox-backup

 src/restore.rs | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/src/restore.rs b/src/restore.rs
index e43d040..9d8c4a2 100644
--- a/src/restore.rs
+++ b/src/restore.rs
@@ -1,5 +1,4 @@
 use std::sync::{Arc, Mutex};
-use std::collections::HashMap;
 use std::io::SeekFrom;
 use std::convert::TryInto;
 
@@ -27,7 +26,6 @@ pub(crate) struct RestoreTask {
     runtime: Arc<Runtime>,
     crypt_config: Option<Arc<CryptConfig>>,
     client: OnceCell<Arc<BackupReader>>,
-    chunk_reader: OnceCell<RemoteChunkReader>,
     manifest: OnceCell<Arc<BackupManifest>>,
     image_registry: Arc<Mutex<Registry<ImageAccessInfo>>>,
 }
@@ -59,7 +57,6 @@ impl RestoreTask {
             crypt_config,
             client: OnceCell::new(),
             manifest: OnceCell::new(),
-            chunk_reader: OnceCell::new(),
             image_registry: Arc::new(Mutex::new(Registry::<ImageAccessInfo>::new())),
         })
     }
@@ -94,15 +91,6 @@ impl RestoreTask {
             true
         ).await?;
 
-        let chunk_reader = RemoteChunkReader::new(
-            client.clone(),
-            self.crypt_config.clone(),
-            HashMap::with_capacity(0),
-        );
-
-        self.chunk_reader.set(chunk_reader)
-            .map_err(|_| format_err!("already connected!"))?;
-
         let (manifest, _) = client.download_manifest().await?;
 
         self.manifest.set(Arc::new(manifest))
@@ -150,9 +138,12 @@ impl RestoreTask {
 
         let most_used = index.find_most_used_chunks(8);
 
+        let file_info = manifest.lookup_file_info(&archive_name)?;
+
         let mut chunk_reader = RemoteChunkReader::new(
             client.clone(),
             self.crypt_config.clone(),
+            file_info.chunk_crypt_mode(),
             most_used,
         );
 
@@ -219,11 +210,6 @@ impl RestoreTask {
             None => bail!("not connected"),
         };
 
-        let chunk_reader = match self.chunk_reader.get() {
-            Some(chunk_reader) => chunk_reader.clone(),
-            None => bail!("not connected"),
-        };
-
         let manifest = match self.manifest.get() {
             Some(manifest) => manifest.clone(),
             None => bail!("no manifest"),
@@ -231,6 +217,16 @@ impl RestoreTask {
 
         let index = client.download_fixed_index(&manifest, &archive_name).await?;
         let archive_size = index.index_bytes();
+        let most_used = index.find_most_used_chunks(8);
+
+        let file_info = manifest.lookup_file_info(&archive_name)?;
+
+        let chunk_reader = RemoteChunkReader::new(
+            client.clone(),
+            self.crypt_config.clone(),
+            file_info.chunk_crypt_mode(),
+            most_used,
+        );
 
         let reader = AsyncIndexReader::new(index, chunk_reader);
 
-- 
2.20.1

Re: [pbs-devel] [PATCH proxmox-backup-qemu] adapt to chunk reader changes

[Stefan Reiter]

makes sense in general, one comment inline

On 8/10/20 1:25 PM, Fabian Grünbichler wrote:
> for verification of chunk crypt mode <=> index crypt mode match
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
> follows patch #4 in proxmox-backup
> 
>   src/restore.rs | 30 +++++++++++++-----------------
>   1 file changed, 13 insertions(+), 17 deletions(-)
> 
> diff --git a/src/restore.rs b/src/restore.rs
> index e43d040..9d8c4a2 100644
> --- a/src/restore.rs
> +++ b/src/restore.rs
> @@ -1,5 +1,4 @@
>   use std::sync::{Arc, Mutex};
> -use std::collections::HashMap;
>   use std::io::SeekFrom;
>   use std::convert::TryInto;
>   
> @@ -27,7 +26,6 @@ pub(crate) struct RestoreTask {
>       runtime: Arc<Runtime>,
>       crypt_config: Option<Arc<CryptConfig>>,
>       client: OnceCell<Arc<BackupReader>>,
> -    chunk_reader: OnceCell<RemoteChunkReader>,
>       manifest: OnceCell<Arc<BackupManifest>>,
>       image_registry: Arc<Mutex<Registry<ImageAccessInfo>>>,
>   }
> @@ -59,7 +57,6 @@ impl RestoreTask {
>               crypt_config,
>               client: OnceCell::new(),
>               manifest: OnceCell::new(),
> -            chunk_reader: OnceCell::new(),
>               image_registry: Arc::new(Mutex::new(Registry::<ImageAccessInfo>::new())),
>           })
>       }
> @@ -94,15 +91,6 @@ impl RestoreTask {
>               true
>           ).await?;
>   
> -        let chunk_reader = RemoteChunkReader::new(
> -            client.clone(),
> -            self.crypt_config.clone(),
> -            HashMap::with_capacity(0),
> -        );
> -
> -        self.chunk_reader.set(chunk_reader)
> -            .map_err(|_| format_err!("already connected!"))?;
> -
>           let (manifest, _) = client.download_manifest().await?;
>   
>           self.manifest.set(Arc::new(manifest))
> @@ -150,9 +138,12 @@ impl RestoreTask {
>   
>           let most_used = index.find_most_used_chunks(8);
>   
> +        let file_info = manifest.lookup_file_info(&archive_name)?;
> +
>           let mut chunk_reader = RemoteChunkReader::new(
>               client.clone(),
>               self.crypt_config.clone(),
> +            file_info.chunk_crypt_mode(),
>               most_used,
>           );
>   
> @@ -219,11 +210,6 @@ impl RestoreTask {
>               None => bail!("not connected"),
>           };
>   
> -        let chunk_reader = match self.chunk_reader.get() {
> -            Some(chunk_reader) => chunk_reader.clone(),
> -            None => bail!("not connected"),
> -        };
> -
>           let manifest = match self.manifest.get() {
>               Some(manifest) => manifest.clone(),
>               None => bail!("no manifest"),
> @@ -231,6 +217,16 @@ impl RestoreTask {
>   
>           let index = client.download_fixed_index(&manifest, &archive_name).await?;
>           let archive_size = index.index_bytes();
> +        let most_used = index.find_most_used_chunks(8);

You change the cache hint from previously an empty HashMap to the 8 
most-used chunks, is that intentional?

I don't believe that really does anything for a fixed index, and the 
AsyncIndexReader already caches the currently accessed chunk.

If anything, maybe add the zero digest here to keep that in cache, or 
even better maybe add that to the RemoteChunkReader?

> +
> +        let file_info = manifest.lookup_file_info(&archive_name)?;
> +
> +        let chunk_reader = RemoteChunkReader::new(
> +            client.clone(),
> +            self.crypt_config.clone(),
> +            file_info.chunk_crypt_mode(),
> +            most_used,
> +        );
>   
>           let reader = AsyncIndexReader::new(index, chunk_reader);
>   
> 

Re: [pbs-devel] [PATCH proxmox-backup-qemu] adapt to chunk reader changes

[Fabian Grünbichler]

On August 10, 2020 5:13 pm, Stefan Reiter wrote:
> makes sense in general, one comment inline
> 
> On 8/10/20 1:25 PM, Fabian Grünbichler wrote:
>> for verification of chunk crypt mode <=> index crypt mode match
>> 
>> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>> ---
>> follows patch #4 in proxmox-backup
>> 
>>   src/restore.rs | 30 +++++++++++++-----------------
>>   1 file changed, 13 insertions(+), 17 deletions(-)
>> 
>> diff --git a/src/restore.rs b/src/restore.rs
>> index e43d040..9d8c4a2 100644
>> --- a/src/restore.rs
>> +++ b/src/restore.rs
>> @@ -1,5 +1,4 @@
>>   use std::sync::{Arc, Mutex};
>> -use std::collections::HashMap;
>>   use std::io::SeekFrom;
>>   use std::convert::TryInto;
>>   
>> @@ -27,7 +26,6 @@ pub(crate) struct RestoreTask {
>>       runtime: Arc<Runtime>,
>>       crypt_config: Option<Arc<CryptConfig>>,
>>       client: OnceCell<Arc<BackupReader>>,
>> -    chunk_reader: OnceCell<RemoteChunkReader>,
>>       manifest: OnceCell<Arc<BackupManifest>>,
>>       image_registry: Arc<Mutex<Registry<ImageAccessInfo>>>,
>>   }
>> @@ -59,7 +57,6 @@ impl RestoreTask {
>>               crypt_config,
>>               client: OnceCell::new(),
>>               manifest: OnceCell::new(),
>> -            chunk_reader: OnceCell::new(),
>>               image_registry: Arc::new(Mutex::new(Registry::<ImageAccessInfo>::new())),
>>           })
>>       }
>> @@ -94,15 +91,6 @@ impl RestoreTask {
>>               true
>>           ).await?;
>>   
>> -        let chunk_reader = RemoteChunkReader::new(
>> -            client.clone(),
>> -            self.crypt_config.clone(),
>> -            HashMap::with_capacity(0),
>> -        );
>> -
>> -        self.chunk_reader.set(chunk_reader)
>> -            .map_err(|_| format_err!("already connected!"))?;
>> -
>>           let (manifest, _) = client.download_manifest().await?;
>>   
>>           self.manifest.set(Arc::new(manifest))
>> @@ -150,9 +138,12 @@ impl RestoreTask {
>>   
>>           let most_used = index.find_most_used_chunks(8);
>>   
>> +        let file_info = manifest.lookup_file_info(&archive_name)?;
>> +
>>           let mut chunk_reader = RemoteChunkReader::new(
>>               client.clone(),
>>               self.crypt_config.clone(),
>> +            file_info.chunk_crypt_mode(),
>>               most_used,
>>           );
>>   
>> @@ -219,11 +210,6 @@ impl RestoreTask {
>>               None => bail!("not connected"),
>>           };
>>   
>> -        let chunk_reader = match self.chunk_reader.get() {
>> -            Some(chunk_reader) => chunk_reader.clone(),
>> -            None => bail!("not connected"),
>> -        };
>> -
>>           let manifest = match self.manifest.get() {
>>               Some(manifest) => manifest.clone(),
>>               None => bail!("no manifest"),
>> @@ -231,6 +217,16 @@ impl RestoreTask {
>>   
>>           let index = client.download_fixed_index(&manifest, &archive_name).await?;
>>           let archive_size = index.index_bytes();
>> +        let most_used = index.find_most_used_chunks(8);
> 
> You change the cache hint from previously an empty HashMap to the 8 
> most-used chunks, is that intentional?
> 
> I don't believe that really does anything for a fixed index, and the 
> AsyncIndexReader already caches the currently accessed chunk.

I made it do that same thing that the other RemoteChunkReader in 
restore.rs does ;)

> If anything, maybe add the zero digest here to keep that in cache, or 
> even better maybe add that to the RemoteChunkReader?

yeah, could do that. the restore itself already skips it to write zeroes 
directly without even asking the chunk reader, so not sure how much we 
gain by teaching the chunk reader to skip them as well..

> 
>> +
>> +        let file_info = manifest.lookup_file_info(&archive_name)?;
>> +
>> +        let chunk_reader = RemoteChunkReader::new(
>> +            client.clone(),
>> +            self.crypt_config.clone(),
>> +            file_info.chunk_crypt_mode(),
>> +            most_used,
>> +        );
>>   
>>           let reader = AsyncIndexReader::new(index, chunk_reader);
>>   
>> 
> 

Re: [pbs-devel] [PATCH proxmox-backup-qemu] adapt to chunk reader changes

[Stefan Reiter]

On 8/11/20 9:53 AM, Fabian Grünbichler wrote:
> On August 10, 2020 5:13 pm, Stefan Reiter wrote:
>> makes sense in general, one comment inline
>>
>> On 8/10/20 1:25 PM, Fabian Grünbichler wrote:
>>> for verification of chunk crypt mode <=> index crypt mode match
>>>
>>> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>>> ---
>>> follows patch #4 in proxmox-backup
>>>
>>>    src/restore.rs | 30 +++++++++++++-----------------
>>>    1 file changed, 13 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/src/restore.rs b/src/restore.rs
>>> index e43d040..9d8c4a2 100644
>>> --- a/src/restore.rs
>>> +++ b/src/restore.rs
>>> @@ -1,5 +1,4 @@
>>>    use std::sync::{Arc, Mutex};
>>> -use std::collections::HashMap;
>>>    use std::io::SeekFrom;
>>>    use std::convert::TryInto;
>>>    
>>> @@ -27,7 +26,6 @@ pub(crate) struct RestoreTask {
>>>        runtime: Arc<Runtime>,
>>>        crypt_config: Option<Arc<CryptConfig>>,
>>>        client: OnceCell<Arc<BackupReader>>,
>>> -    chunk_reader: OnceCell<RemoteChunkReader>,
>>>        manifest: OnceCell<Arc<BackupManifest>>,
>>>        image_registry: Arc<Mutex<Registry<ImageAccessInfo>>>,
>>>    }
>>> @@ -59,7 +57,6 @@ impl RestoreTask {
>>>                crypt_config,
>>>                client: OnceCell::new(),
>>>                manifest: OnceCell::new(),
>>> -            chunk_reader: OnceCell::new(),
>>>                image_registry: Arc::new(Mutex::new(Registry::<ImageAccessInfo>::new())),
>>>            })
>>>        }
>>> @@ -94,15 +91,6 @@ impl RestoreTask {
>>>                true
>>>            ).await?;
>>>    
>>> -        let chunk_reader = RemoteChunkReader::new(
>>> -            client.clone(),
>>> -            self.crypt_config.clone(),
>>> -            HashMap::with_capacity(0),
>>> -        );
>>> -
>>> -        self.chunk_reader.set(chunk_reader)
>>> -            .map_err(|_| format_err!("already connected!"))?;
>>> -
>>>            let (manifest, _) = client.download_manifest().await?;
>>>    
>>>            self.manifest.set(Arc::new(manifest))
>>> @@ -150,9 +138,12 @@ impl RestoreTask {
>>>    
>>>            let most_used = index.find_most_used_chunks(8);
>>>    
>>> +        let file_info = manifest.lookup_file_info(&archive_name)?;
>>> +
>>>            let mut chunk_reader = RemoteChunkReader::new(
>>>                client.clone(),
>>>                self.crypt_config.clone(),
>>> +            file_info.chunk_crypt_mode(),
>>>                most_used,
>>>            );
>>>    
>>> @@ -219,11 +210,6 @@ impl RestoreTask {
>>>                None => bail!("not connected"),
>>>            };
>>>    
>>> -        let chunk_reader = match self.chunk_reader.get() {
>>> -            Some(chunk_reader) => chunk_reader.clone(),
>>> -            None => bail!("not connected"),
>>> -        };
>>> -
>>>            let manifest = match self.manifest.get() {
>>>                Some(manifest) => manifest.clone(),
>>>                None => bail!("no manifest"),
>>> @@ -231,6 +217,16 @@ impl RestoreTask {
>>>    
>>>            let index = client.download_fixed_index(&manifest, &archive_name).await?;
>>>            let archive_size = index.index_bytes();
>>> +        let most_used = index.find_most_used_chunks(8);
>>
>> You change the cache hint from previously an empty HashMap to the 8
>> most-used chunks, is that intentional?
>>
>> I don't believe that really does anything for a fixed index, and the
>> AsyncIndexReader already caches the currently accessed chunk.
> 
> I made it do that same thing that the other RemoteChunkReader in
> restore.rs does ;)
> 
>> If anything, maybe add the zero digest here to keep that in cache, or
>> even better maybe add that to the RemoteChunkReader?
> 
> yeah, could do that. the restore itself already skips it to write zeroes
> directly without even asking the chunk reader, so not sure how much we
> gain by teaching the chunk reader to skip them as well..
> 

Well then other usages of the readers would gain that functionality too, 
like mapping backup images (both in VMs and soon on the host). But we 
can change this later anyway, not too important atm.

>>
>>> +
>>> +        let file_info = manifest.lookup_file_info(&archive_name)?;
>>> +
>>> +        let chunk_reader = RemoteChunkReader::new(
>>> +            client.clone(),
>>> +            self.crypt_config.clone(),
>>> +            file_info.chunk_crypt_mode(),
>>> +            most_used,
>>> +        );
>>>    
>>>            let reader = AsyncIndexReader::new(index, chunk_reader);
>>>    
>>>
>>

[pbs-devel] applied: [PATCH proxmox-backup-qemu] adapt to chunk reader changes

[Dietmar Maurer]

applied

[pbs-devel] [PATCH proxmox-backup 1/5] datastore api: only decode unencrypted indices

[Fabian Grünbichler]

these checks were already in place for regular downloading of backed up
files, also do them when attempting to decode a catalog, or when
downloading decoded files referenced by a pxar index.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/api2/admin/datastore.rs | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index a0a14a2f..d535b4d2 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -1133,9 +1133,18 @@ fn catalog(
     let allowed = (user_privs & PRIV_DATASTORE_READ) != 0;
     if !allowed { check_backup_owner(&datastore, backup_dir.group(), &userid)?; }
 
+    let file_name = CATALOG_NAME;
+
+    let (_manifest, files) = read_backup_index(&datastore, &backup_dir)?;
+    for file in files {
+        if file.filename == file_name && file.crypt_mode == Some(CryptMode::Encrypt) {
+            bail!("cannot decode '{}' - is encrypted", file_name);
+        }
+    }
+
     let mut path = datastore.base_path();
     path.push(backup_dir.relative_path());
-    path.push(CATALOG_NAME);
+    path.push(file_name);
 
     let index = DynamicIndexReader::open(&path)
         .map_err(|err| format_err!("unable to read dynamic index '{:?}' - {}", &path, err))?;
@@ -1238,19 +1247,24 @@ fn pxar_file_download(
         let allowed = (user_privs & PRIV_DATASTORE_READ) != 0;
         if !allowed { check_backup_owner(&datastore, backup_dir.group(), &userid)?; }
 
-        let mut path = datastore.base_path();
-        path.push(backup_dir.relative_path());
-
         let mut components = base64::decode(&filepath)?;
         if components.len() > 0 && components[0] == '/' as u8 {
             components.remove(0);
         }
 
         let mut split = components.splitn(2, |c| *c == '/' as u8);
-        let pxar_name = split.next().unwrap();
+        let pxar_name = std::str::from_utf8(split.next().unwrap())?;
         let file_path = split.next().ok_or(format_err!("filepath looks strange '{}'", filepath))?;
+        let (_manifest, files) = read_backup_index(&datastore, &backup_dir)?;
+        for file in files {
+            if file.filename == pxar_name && file.crypt_mode == Some(CryptMode::Encrypt) {
+                bail!("cannot decode '{}' - is encrypted", pxar_name);
+            }
+        }
 
-        path.push(OsStr::from_bytes(&pxar_name));
+        let mut path = datastore.base_path();
+        path.push(backup_dir.relative_path());
+        path.push(pxar_name);
 
         let index = DynamicIndexReader::open(&path)
             .map_err(|err| format_err!("unable to read dynamic index '{:?}' - {}", &path, err))?;
-- 
2.20.1

[pbs-devel] applied: [PATCH proxmox-backup 1/5] datastore api: only decode unencrypted indices

[Dietmar Maurer]

applied patch 1 - 5

[pbs-devel] [PATCH proxmox-backup 2/5] datastore api: verify blob/index csum from manifest

[Fabian Grünbichler]

when dowloading decoded files.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/api2/admin/datastore.rs | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index d535b4d2..462b8d9c 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -951,7 +951,7 @@ fn download_file_decoded(
         let allowed = (user_privs & PRIV_DATASTORE_READ) != 0;
         if !allowed { check_backup_owner(&datastore, backup_dir.group(), &userid)?; }
 
-        let (_manifest, files) = read_backup_index(&datastore, &backup_dir)?;
+        let (manifest, files) = read_backup_index(&datastore, &backup_dir)?;
         for file in files {
             if file.filename == file_name && file.crypt_mode == Some(CryptMode::Encrypt) {
                 bail!("cannot decode '{}' - is encrypted", file_name);
@@ -970,6 +970,8 @@ fn download_file_decoded(
             "didx" => {
                 let index = DynamicIndexReader::open(&path)
                     .map_err(|err| format_err!("unable to read dynamic index '{:?}' - {}", &path, err))?;
+                let (csum, size) = index.compute_csum();
+                manifest.verify_file(&file_name, &csum, size)?;
 
                 let chunk_reader = LocalChunkReader::new(datastore, None);
                 let reader = AsyncIndexReader::new(index, chunk_reader);
@@ -983,6 +985,9 @@ fn download_file_decoded(
                 let index = FixedIndexReader::open(&path)
                     .map_err(|err| format_err!("unable to read fixed index '{:?}' - {}", &path, err))?;
 
+                let (csum, size) = index.compute_csum();
+                manifest.verify_file(&file_name, &csum, size)?;
+
                 let chunk_reader = LocalChunkReader::new(datastore, None);
                 let reader = AsyncIndexReader::new(index, chunk_reader);
                 Body::wrap_stream(AsyncReaderStream::with_buffer_size(reader, 4*1024*1024)
@@ -995,6 +1000,8 @@ fn download_file_decoded(
                 let file = std::fs::File::open(&path)
                     .map_err(|err| http_err!(BAD_REQUEST, "File open failed: {}", err))?;
 
+                // FIXME: load full blob to verify index checksum?
+
                 Body::wrap_stream(
                     WrappedReaderStream::new(DataBlobReader::new(file, None)?)
                         .map_err(move |err| {
@@ -1135,7 +1142,7 @@ fn catalog(
 
     let file_name = CATALOG_NAME;
 
-    let (_manifest, files) = read_backup_index(&datastore, &backup_dir)?;
+    let (manifest, files) = read_backup_index(&datastore, &backup_dir)?;
     for file in files {
         if file.filename == file_name && file.crypt_mode == Some(CryptMode::Encrypt) {
             bail!("cannot decode '{}' - is encrypted", file_name);
@@ -1149,6 +1156,9 @@ fn catalog(
     let index = DynamicIndexReader::open(&path)
         .map_err(|err| format_err!("unable to read dynamic index '{:?}' - {}", &path, err))?;
 
+    let (csum, size) = index.compute_csum();
+    manifest.verify_file(&file_name, &csum, size)?;
+
     let chunk_reader = LocalChunkReader::new(datastore, None);
     let reader = BufferedDynamicReader::new(index, chunk_reader);
 
@@ -1255,7 +1265,7 @@ fn pxar_file_download(
         let mut split = components.splitn(2, |c| *c == '/' as u8);
         let pxar_name = std::str::from_utf8(split.next().unwrap())?;
         let file_path = split.next().ok_or(format_err!("filepath looks strange '{}'", filepath))?;
-        let (_manifest, files) = read_backup_index(&datastore, &backup_dir)?;
+        let (manifest, files) = read_backup_index(&datastore, &backup_dir)?;
         for file in files {
             if file.filename == pxar_name && file.crypt_mode == Some(CryptMode::Encrypt) {
                 bail!("cannot decode '{}' - is encrypted", pxar_name);
@@ -1269,6 +1279,9 @@ fn pxar_file_download(
         let index = DynamicIndexReader::open(&path)
             .map_err(|err| format_err!("unable to read dynamic index '{:?}' - {}", &path, err))?;
 
+        let (csum, size) = index.compute_csum();
+        manifest.verify_file(&pxar_name, &csum, size)?;
+
         let chunk_reader = LocalChunkReader::new(datastore, None);
         let reader = BufferedDynamicReader::new(index, chunk_reader);
         let archive_size = reader.archive_size();
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 3/5] chunk readers: ensure chunk/index CryptMode matches

[Fabian Grünbichler]

an encrypted Index should never reference a plain-text chunk, and an
unencrypted Index should never reference an encrypted chunk.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/api2/admin/datastore.rs              |  8 +++---
 src/backup/manifest.rs                   | 14 +++++++++++
 src/backup/read_chunk.rs                 | 32 ++++++++++++++++++++----
 src/bin/proxmox-backup-client.rs         | 14 ++++++++---
 src/bin/proxmox_backup_client/catalog.rs | 12 ++++++---
 src/bin/proxmox_backup_client/mount.rs   |  4 ++-
 src/client/pull.rs                       |  4 +--
 src/client/remote_chunk_reader.rs        | 22 +++++++++++++---
 8 files changed, 88 insertions(+), 22 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 462b8d9c..81ca02eb 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -973,7 +973,7 @@ fn download_file_decoded(
                 let (csum, size) = index.compute_csum();
                 manifest.verify_file(&file_name, &csum, size)?;
 
-                let chunk_reader = LocalChunkReader::new(datastore, None);
+                let chunk_reader = LocalChunkReader::new(datastore, None, CryptMode::None);
                 let reader = AsyncIndexReader::new(index, chunk_reader);
                 Body::wrap_stream(AsyncReaderStream::new(reader)
                     .map_err(move |err| {
@@ -988,7 +988,7 @@ fn download_file_decoded(
                 let (csum, size) = index.compute_csum();
                 manifest.verify_file(&file_name, &csum, size)?;
 
-                let chunk_reader = LocalChunkReader::new(datastore, None);
+                let chunk_reader = LocalChunkReader::new(datastore, None, CryptMode::None);
                 let reader = AsyncIndexReader::new(index, chunk_reader);
                 Body::wrap_stream(AsyncReaderStream::with_buffer_size(reader, 4*1024*1024)
                     .map_err(move |err| {
@@ -1159,7 +1159,7 @@ fn catalog(
     let (csum, size) = index.compute_csum();
     manifest.verify_file(&file_name, &csum, size)?;
 
-    let chunk_reader = LocalChunkReader::new(datastore, None);
+    let chunk_reader = LocalChunkReader::new(datastore, None, CryptMode::None);
     let reader = BufferedDynamicReader::new(index, chunk_reader);
 
     let mut catalog_reader = CatalogReader::new(reader);
@@ -1282,7 +1282,7 @@ fn pxar_file_download(
         let (csum, size) = index.compute_csum();
         manifest.verify_file(&pxar_name, &csum, size)?;
 
-        let chunk_reader = LocalChunkReader::new(datastore, None);
+        let chunk_reader = LocalChunkReader::new(datastore, None, CryptMode::None);
         let reader = BufferedDynamicReader::new(index, chunk_reader);
         let archive_size = reader.archive_size();
         let reader = LocalDynamicReadAt::new(reader);
diff --git a/src/backup/manifest.rs b/src/backup/manifest.rs
index a42cdeb7..6af110d1 100644
--- a/src/backup/manifest.rs
+++ b/src/backup/manifest.rs
@@ -49,6 +49,20 @@ pub struct FileInfo {
     pub csum: [u8; 32],
 }
 
+impl FileInfo {
+
+    /// Return expected CryptMode of referenced chunks
+    ///
+    /// Encrypted Indices should only reference encrypted chunks, while signed or plain indices
+    /// should only reference plain chunks.
+    pub fn chunk_crypt_mode (&self) -> CryptMode {
+        match self.crypt_mode {
+            CryptMode::Encrypt => CryptMode::Encrypt,
+            CryptMode::SignOnly | CryptMode::None => CryptMode::None,
+        }
+    }
+}
+
 #[derive(Serialize, Deserialize)]
 #[serde(rename_all="kebab-case")]
 pub struct BackupManifest {
diff --git a/src/backup/read_chunk.rs b/src/backup/read_chunk.rs
index 200f53ea..078779de 100644
--- a/src/backup/read_chunk.rs
+++ b/src/backup/read_chunk.rs
@@ -2,9 +2,9 @@ use std::future::Future;
 use std::pin::Pin;
 use std::sync::Arc;
 
-use anyhow::Error;
+use anyhow::{bail, Error};
 
-use super::crypt_config::CryptConfig;
+use super::crypt_config::{CryptConfig, CryptMode};
 use super::data_blob::DataBlob;
 use super::datastore::DataStore;
 
@@ -21,20 +21,41 @@ pub trait ReadChunk {
 pub struct LocalChunkReader {
     store: Arc<DataStore>,
     crypt_config: Option<Arc<CryptConfig>>,
+    crypt_mode: CryptMode,
 }
 
 impl LocalChunkReader {
-    pub fn new(store: Arc<DataStore>, crypt_config: Option<Arc<CryptConfig>>) -> Self {
+    pub fn new(store: Arc<DataStore>, crypt_config: Option<Arc<CryptConfig>>, crypt_mode: CryptMode) -> Self {
         Self {
             store,
             crypt_config,
+            crypt_mode,
+        }
+    }
+
+    fn ensure_crypt_mode(&self, chunk_mode: CryptMode) -> Result<(), Error> {
+        match self.crypt_mode {
+            CryptMode::Encrypt => {
+                match chunk_mode {
+                    CryptMode::Encrypt => Ok(()),
+                    CryptMode::SignOnly | CryptMode::None => bail!("Index and chunk CryptMode don't match."),
+                }
+            },
+            CryptMode::SignOnly | CryptMode::None => {
+                match chunk_mode {
+                    CryptMode::Encrypt => bail!("Index and chunk CryptMode don't match."),
+                    CryptMode::SignOnly | CryptMode::None => Ok(()),
+                }
+            },
         }
     }
 }
 
 impl ReadChunk for LocalChunkReader {
     fn read_raw_chunk(&self, digest: &[u8; 32]) -> Result<DataBlob, Error> {
-        self.store.load_chunk(digest)
+        let chunk = self.store.load_chunk(digest)?;
+        self.ensure_crypt_mode(chunk.crypt_mode()?)?;
+        Ok(chunk)
     }
 
     fn read_chunk(&self, digest: &[u8; 32]) -> Result<Vec<u8>, Error> {
@@ -71,7 +92,8 @@ impl AsyncReadChunk for LocalChunkReader {
             let raw_data = tokio::fs::read(&path).await?;
 
             let chunk = DataBlob::load_from_reader(&mut &raw_data[..])?;
-           
+            self.ensure_crypt_mode(chunk.crypt_mode()?)?;
+
             Ok(chunk)
         })
     }
diff --git a/src/bin/proxmox-backup-client.rs b/src/bin/proxmox-backup-client.rs
index c6c11c75..9a6f309d 100644
--- a/src/bin/proxmox-backup-client.rs
+++ b/src/bin/proxmox-backup-client.rs
@@ -1182,6 +1182,7 @@ fn complete_backup_source(arg: &str, param: &HashMap<String, String>) -> Vec<Str
 async fn dump_image<W: Write>(
     client: Arc<BackupReader>,
     crypt_config: Option<Arc<CryptConfig>>,
+    crypt_mode: CryptMode,
     index: FixedIndexReader,
     mut writer: W,
     verbose: bool,
@@ -1189,7 +1190,7 @@ async fn dump_image<W: Write>(
 
     let most_used = index.find_most_used_chunks(8);
 
-    let chunk_reader = RemoteChunkReader::new(client.clone(), crypt_config, most_used);
+    let chunk_reader = RemoteChunkReader::new(client.clone(), crypt_config, crypt_mode, most_used);
 
     // Note: we avoid using BufferedFixedReader, because that add an additional buffer/copy
     // and thus slows down reading. Instead, directly use RemoteChunkReader
@@ -1340,7 +1341,12 @@ async fn restore(param: Value) -> Result<Value, Error> {
                 .map_err(|err| format_err!("unable to pipe data - {}", err))?;
         }
 
-    } else if archive_type == ArchiveType::Blob {
+        return Ok(Value::Null);
+    }
+
+    let file_info = manifest.lookup_file_info(&archive_name)?;
+
+    if archive_type == ArchiveType::Blob {
 
         let mut reader = client.download_blob(&manifest, &archive_name).await?;
 
@@ -1365,7 +1371,7 @@ async fn restore(param: Value) -> Result<Value, Error> {
 
         let most_used = index.find_most_used_chunks(8);
 
-        let chunk_reader = RemoteChunkReader::new(client.clone(), crypt_config, most_used);
+        let chunk_reader = RemoteChunkReader::new(client.clone(), crypt_config, file_info.chunk_crypt_mode(), most_used);
 
         let mut reader = BufferedDynamicReader::new(index, chunk_reader);
 
@@ -1412,7 +1418,7 @@ async fn restore(param: Value) -> Result<Value, Error> {
                 .map_err(|err| format_err!("unable to open /dev/stdout - {}", err))?
         };
 
-        dump_image(client.clone(), crypt_config.clone(), index, &mut writer, verbose).await?;
+        dump_image(client.clone(), crypt_config.clone(), file_info.chunk_crypt_mode(), index, &mut writer, verbose).await?;
     }
 
     Ok(Value::Null)
diff --git a/src/bin/proxmox_backup_client/catalog.rs b/src/bin/proxmox_backup_client/catalog.rs
index 1c0865e6..b419728e 100644
--- a/src/bin/proxmox_backup_client/catalog.rs
+++ b/src/bin/proxmox_backup_client/catalog.rs
@@ -97,7 +97,9 @@ async fn dump_catalog(param: Value) -> Result<Value, Error> {
 
     let most_used = index.find_most_used_chunks(8);
 
-    let chunk_reader = RemoteChunkReader::new(client.clone(), crypt_config, most_used);
+    let file_info = manifest.lookup_file_info(&CATALOG_NAME)?;
+
+    let chunk_reader = RemoteChunkReader::new(client.clone(), crypt_config, file_info.chunk_crypt_mode(), most_used);
 
     let mut reader = BufferedDynamicReader::new(index, chunk_reader);
 
@@ -200,7 +202,9 @@ async fn catalog_shell(param: Value) -> Result<(), Error> {
 
     let index = client.download_dynamic_index(&manifest, &server_archive_name).await?;
     let most_used = index.find_most_used_chunks(8);
-    let chunk_reader = RemoteChunkReader::new(client.clone(), crypt_config.clone(), most_used);
+
+    let file_info = manifest.lookup_file_info(&server_archive_name)?;
+    let chunk_reader = RemoteChunkReader::new(client.clone(), crypt_config.clone(), file_info.chunk_crypt_mode(), most_used);
     let reader = BufferedDynamicReader::new(index, chunk_reader);
     let archive_size = reader.archive_size();
     let reader: proxmox_backup::pxar::fuse::Reader =
@@ -216,7 +220,9 @@ async fn catalog_shell(param: Value) -> Result<(), Error> {
     manifest.verify_file(CATALOG_NAME, &csum, size)?;
 
     let most_used = index.find_most_used_chunks(8);
-    let chunk_reader = RemoteChunkReader::new(client.clone(), crypt_config, most_used);
+
+    let file_info = manifest.lookup_file_info(&CATALOG_NAME)?;
+    let chunk_reader = RemoteChunkReader::new(client.clone(), crypt_config, file_info.chunk_crypt_mode(), most_used);
     let mut reader = BufferedDynamicReader::new(index, chunk_reader);
     let mut catalogfile = std::fs::OpenOptions::new()
         .write(true)
diff --git a/src/bin/proxmox_backup_client/mount.rs b/src/bin/proxmox_backup_client/mount.rs
index 73bb8d4c..7646e98c 100644
--- a/src/bin/proxmox_backup_client/mount.rs
+++ b/src/bin/proxmox_backup_client/mount.rs
@@ -141,10 +141,12 @@ async fn mount_do(param: Value, pipe: Option<RawFd>) -> Result<Value, Error> {
 
     let (manifest, _) = client.download_manifest().await?;
 
+    let file_info = manifest.lookup_file_info(&archive_name)?;
+
     if server_archive_name.ends_with(".didx") {
         let index = client.download_dynamic_index(&manifest, &server_archive_name).await?;
         let most_used = index.find_most_used_chunks(8);
-        let chunk_reader = RemoteChunkReader::new(client.clone(), crypt_config, most_used);
+        let chunk_reader = RemoteChunkReader::new(client.clone(), crypt_config, file_info.chunk_crypt_mode(), most_used);
         let reader = BufferedDynamicReader::new(index, chunk_reader);
         let archive_size = reader.archive_size();
         let reader: proxmox_backup::pxar::fuse::Reader =
diff --git a/src/client/pull.rs b/src/client/pull.rs
index f1dde9b4..05b7c66c 100644
--- a/src/client/pull.rs
+++ b/src/client/pull.rs
@@ -224,8 +224,6 @@ async fn pull_snapshot(
 
     let manifest = BackupManifest::try_from(tmp_manifest_blob)?;
 
-    let mut chunk_reader = RemoteChunkReader::new(reader.clone(), None, HashMap::new());
-
     for item in manifest.files() {
         let mut path = tgt_store.base_path();
         path.push(snapshot.relative_path());
@@ -266,6 +264,8 @@ async fn pull_snapshot(
             }
         }
 
+        let mut chunk_reader = RemoteChunkReader::new(reader.clone(), None, item.chunk_crypt_mode(), HashMap::new());
+
         pull_single_archive(
             worker,
             &reader,
diff --git a/src/client/remote_chunk_reader.rs b/src/client/remote_chunk_reader.rs
index b30d0567..2670bf22 100644
--- a/src/client/remote_chunk_reader.rs
+++ b/src/client/remote_chunk_reader.rs
@@ -3,10 +3,10 @@ use std::collections::HashMap;
 use std::pin::Pin;
 use std::sync::{Arc, Mutex};
 
-use anyhow::Error;
+use anyhow::{bail, Error};
 
 use super::BackupReader;
-use crate::backup::{AsyncReadChunk, CryptConfig, DataBlob, ReadChunk};
+use crate::backup::{AsyncReadChunk, CryptConfig, CryptMode, DataBlob, ReadChunk};
 use crate::tools::runtime::block_on;
 
 /// Read chunks from remote host using ``BackupReader``
@@ -14,6 +14,7 @@ use crate::tools::runtime::block_on;
 pub struct RemoteChunkReader {
     client: Arc<BackupReader>,
     crypt_config: Option<Arc<CryptConfig>>,
+    crypt_mode: CryptMode,
     cache_hint: HashMap<[u8; 32], usize>,
     cache: Arc<Mutex<HashMap<[u8; 32], Vec<u8>>>>,
 }
@@ -25,11 +26,13 @@ impl RemoteChunkReader {
     pub fn new(
         client: Arc<BackupReader>,
         crypt_config: Option<Arc<CryptConfig>>,
+        crypt_mode: CryptMode,
         cache_hint: HashMap<[u8; 32], usize>,
     ) -> Self {
         Self {
             client,
             crypt_config,
+            crypt_mode,
             cache_hint,
             cache: Arc::new(Mutex::new(HashMap::new())),
         }
@@ -46,7 +49,20 @@ impl RemoteChunkReader {
 
         let chunk = DataBlob::load_from_reader(&mut &chunk_data[..])?;
 
-        Ok(chunk)
+        match self.crypt_mode {
+            CryptMode::Encrypt => {
+                match chunk.crypt_mode()? {
+                    CryptMode::Encrypt => Ok(chunk),
+                    CryptMode::SignOnly | CryptMode::None => bail!("Index and chunk CryptMode don't match."),
+                }
+            },
+            CryptMode::SignOnly | CryptMode::None => {
+                match chunk.crypt_mode()? {
+                    CryptMode::Encrypt => bail!("Index and chunk CryptMode don't match."),
+                    CryptMode::SignOnly | CryptMode::None => Ok(chunk),
+                }
+            },
+        }
     }
 }
 
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 4/5] verify: also check chunk CryptMode

[Fabian Grünbichler]

and in-line verify_stored_chunk to avoid double-loading each chunk.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    not 100% happy with this

 src/backup/datastore.rs |  6 ------
 src/backup/verify.rs    | 36 +++++++++++++++++++++++++++++++++---
 2 files changed, 33 insertions(+), 9 deletions(-)

diff --git a/src/backup/datastore.rs b/src/backup/datastore.rs
index 5b6075ec..afdff224 100644
--- a/src/backup/datastore.rs
+++ b/src/backup/datastore.rs
@@ -551,12 +551,6 @@ impl DataStore {
         self.chunk_store.insert_chunk(chunk, digest)
     }
 
-    pub fn verify_stored_chunk(&self, digest: &[u8; 32], expected_chunk_size: u64) -> Result<(), Error> {
-        let blob = self.load_chunk(digest)?;
-        blob.verify_unencrypted(expected_chunk_size as usize, digest)?;
-        Ok(())
-    }
-
     pub fn load_blob(&self, backup_dir: &BackupDir, filename: &str) -> Result<DataBlob, Error> {
         let mut path = self.base_path();
         path.push(backup_dir.relative_path());
diff --git a/src/backup/verify.rs b/src/backup/verify.rs
index ec47534c..fa2f0aa5 100644
--- a/src/backup/verify.rs
+++ b/src/backup/verify.rs
@@ -40,6 +40,7 @@ fn verify_index_chunks(
     index: Box<dyn IndexFile>,
     verified_chunks: &mut HashSet<[u8;32]>,
     corrupt_chunks: &mut HashSet<[u8; 32]>,
+    crypt_mode: CryptMode,
     worker: &WorkerTask,
 ) -> Result<(), Error> {
 
@@ -51,9 +52,38 @@ fn verify_index_chunks(
         let info = index.chunk_info(pos).unwrap();
         let size = info.range.end - info.range.start;
 
+        let chunk = match datastore.load_chunk(&info.digest) {
+            Err(err) => {
+                corrupt_chunks.insert(info.digest);
+                worker.log(format!("can't verify chunk, load failed - {}", err));
+                errors += 1;
+                continue;
+            },
+            Ok(chunk) => chunk,
+        };
+
+        let chunk_crypt_mode = match chunk.crypt_mode() {
+            Err(err) => {
+                corrupt_chunks.insert(info.digest);
+                worker.log(format!("can't verify chunk, unknown CryptMode - {}", err));
+                errors += 1;
+                continue;
+            },
+            Ok(mode) => mode,
+        };
+
+        if chunk_crypt_mode != crypt_mode {
+            worker.log(format!(
+                "chunk CryptMode {:?} does not match index CryptMode {:?}",
+                chunk_crypt_mode,
+                crypt_mode
+            ));
+            errors += 1;
+        }
+
         if !verified_chunks.contains(&info.digest) {
             if !corrupt_chunks.contains(&info.digest) {
-                if let Err(err) = datastore.verify_stored_chunk(&info.digest, size) {
+                if let Err(err) = chunk.verify_unencrypted(size as usize, &info.digest) {
                     corrupt_chunks.insert(info.digest);
                     worker.log(format!("{}", err));
                     errors += 1;
@@ -98,7 +128,7 @@ fn verify_fixed_index(
         bail!("wrong index checksum");
     }
 
-    verify_index_chunks(datastore, Box::new(index), verified_chunks, corrupt_chunks, worker)
+    verify_index_chunks(datastore, Box::new(index), verified_chunks, corrupt_chunks, info.chunk_crypt_mode(), worker)
 }
 
 fn verify_dynamic_index(
@@ -124,7 +154,7 @@ fn verify_dynamic_index(
         bail!("wrong index checksum");
     }
 
-    verify_index_chunks(datastore, Box::new(index), verified_chunks, corrupt_chunks, worker)
+    verify_index_chunks(datastore, Box::new(index), verified_chunks, corrupt_chunks, info.chunk_crypt_mode(), worker)
 }
 
 /// Verify a single backup snapshot
-- 
2.20.1

[pbs-devel] [RFC proxmox-backup 5/5] mark signed manifests as such

[Fabian Grünbichler]

for less-confusing display in the web interface

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    I am not so sure about this one - I think I'd rather move the index.json and
    client.log out of the backup content and into some new backup metadata struct,
    and split the display/add an info button to the GUI view..

 src/api2/admin/datastore.rs | 5 ++++-
 src/backup/manifest.rs      | 2 ++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 81ca02eb..fe72ea32 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -67,7 +67,10 @@ fn read_backup_index(
 
     result.push(BackupContent {
         filename: MANIFEST_BLOB_NAME.to_string(),
-        crypt_mode: Some(CryptMode::None),
+        crypt_mode: match manifest.signature {
+            Some(_) => Some(CryptMode::SignOnly),
+            None => Some(CryptMode::None),
+        },
         size: Some(index_size),
     });
 
diff --git a/src/backup/manifest.rs b/src/backup/manifest.rs
index 6af110d1..56f6cf42 100644
--- a/src/backup/manifest.rs
+++ b/src/backup/manifest.rs
@@ -72,6 +72,7 @@ pub struct BackupManifest {
     files: Vec<FileInfo>,
     #[serde(default="empty_value")] // to be compatible with < 0.8.0 backups
     pub unprotected: Value,
+    pub signature: Option<String>,
 }
 
 #[derive(PartialEq)]
@@ -105,6 +106,7 @@ impl BackupManifest {
             backup_time: snapshot.backup_time().timestamp(),
             files: Vec::new(),
             unprotected: json!({}),
+            signature: None,
         }
     }
 
-- 
2.20.1