Re: [PATCH proxmox-backup v2 5/5] sync: push: gracefully handle previous manifest signature mismatches

[Robert Obkircher <r.obkircher@proxmox.com>]

> During push sync jobs with a given active encryption key, the key is
> loaded for the backup writer, used to encrypt the source snapshot on
> the fly. As optimization, the backup writer deduplicates chunks
> already present in the previous snapshot by loading them from the
> index files as stored in the previous manifest.
> 
> Reuse of the previous manifest and index must however only happen if:
> - The previous manifest is not encrypted, neither will push encrypt.
> - The previous manifest is encrypted and passes signature
>   verification using the same key to be used during push encryption.
> 
> Since these checks are now performed when loading the previous
> manifest, drop redundant and now useless checks performed before
> loading the chunks from index files.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>
> diff --git a/src/server/push.rs b/src/server/push.rs
> index 17fa605a4..6df83cbd8 100644
> --- a/src/server/push.rs
> +++ b/src/server/push.rs
> @@ -979,25 +979,12 @@ pub(crate) async fn push_group(
>  }
>  
>  async fn load_previous_snapshot_known_chunks(
> -    params: &PushParameters,
>      upload_options: &UploadOptions,
>      backup_writer: &BackupWriter,
>      archive_name: &BackupArchiveName,
>      known_chunks: Arc<Mutex<HashSet<[u8; 32]>>>,
>  ) {
>      if let Some(manifest) = upload_options.previous_manifest.as_ref() {
> -        if let Some((_id, crypt_config)) = &params.crypt_config {
> -            // no fingerprint in previous manifest -> no reuse possible
> -            let Ok(Some(fingerprint)) = manifest.fingerprint() else {
> -                return;
> -            };
> -
> -            if *fingerprint.bytes() != crypt_config.fingerprint() {
> -                // key mismatch -> no reuse possible
> -                return;
> -            }
> -        }
> -
>          // Add known chunks, ignore errors since archive might not be present and it is better
>          // to proceed on unrelated errors than to fail here.
>          match archive_name.archive_type() {
> @@ -1128,18 +1115,73 @@ pub(crate) async fn push_snapshot(
>      .await
>      .with_context(|| prefix.to_string())?;
>  
> -    let mut previous_manifest = None;
>      // Use manifest of previous snapshots in group on target for chunk upload deduplication
> -    if fetch_previous_manifest {
> -        match backup_writer.download_previous_manifest(false).await {
> -            Ok(manifest) => previous_manifest = Some(Arc::new(manifest)),
> +    let previous_manifest = if !fetch_previous_manifest {
> +        None
> +    } else {
> +        let result = backup_writer
> +            .download_previous_manifest(false)
> +            .await
> +            .map_err(|err| err.context("failed to download"))
nit: this could instead call Context::context without the map_err
> +            .and_then(|manifest| {
> +                let fingerprint = manifest
> +                    .fingerprint()
> +                    .context("failed getting fingerprint")?;
> +
> +                let Some(manifest_key_fp) = fingerprint else {
> +                    if encrypt_using_key.is_none() {
> +                        return Ok(Arc::new(manifest));
What if encrypt_using_key is None because the local chunks are already
encrypted? Shouldn't we expect a fingerprint in that case?

(I'm really not sure if I understand this)
> +                    }
> +                    bail!("manifest not encrypted");
This error message confused me initially because, according to the 3rd
commit, manifest blobs are never encrypted. Should it maybe be "backup
not encrypted" or "missing fingerprint"?
> +                };
> +
> +                let signed_only = manifest
> +                    .files()
> +                    .iter()
> +                    .all(|f| f.chunk_crypt_mode() == CryptMode::None);
> +
> +                let Some((_key_id, crypt_config)) = &encrypt_using_key else {
> +                    if signed_only {
> +                        return Ok(Arc::new(manifest));
> +                    }
> +                    bail!("manifest encrypted but no encryption key configured");
My previous two comments might also apply here.

-- 
Robert Obkircher <r.obkircher@proxmox.com>