Re: [PATCH proxmox-offline-mirror 2/3] use proxmox-pgp crate to replace verifier helper module

[Erik Fastermann <e.fastermann@proxmox.com>]

Comments inline.

With those fixed consider:

Reviewed-by: Erik Fastermann <e.fastermann@proxmox.com>


> Signed-off-by: Nicolas Frey <n.frey@proxmox.com>
> ---
>   Cargo.toml    |  1 +
>   src/config.rs | 33 +--------------------------------
>   src/mirror.rs |  7 +++----
>   3 files changed, 5 insertions(+), 36 deletions(-)
> 
> diff --git a/Cargo.toml b/Cargo.toml
> index ff54637..7aa285a 100644
> --- a/Cargo.toml
> +++ b/Cargo.toml
> @@ -29,6 +29,7 @@ proxmox-apt-api-types = "2.0"
>   proxmox-async = "0.5"
>   proxmox-base64 = "1"
>   proxmox-http = { version = "1", features = [ "client-sync", "client-trait" ]}
> +proxmox-pgp = "1"
>   proxmox-router = { version = "3", features = [ "cli" ], default-features = false }
>   proxmox-schema = { version = "5", features = [ "api-macro" ] }
>   proxmox-section-config = "3"
> diff --git a/src/config.rs b/src/config.rs
> index 0ca296e..c8916a0 100644
> --- a/src/config.rs
> +++ b/src/config.rs
> @@ -2,6 +2,7 @@ use std::path::Path;
>   use std::sync::LazyLock;
>   
>   use anyhow::{Error, bail};
> +use proxmox_pgp::WeakCryptoConfig;

Nit: The use statement should be in the same block with all the other 
proxmox imports.

>   use serde::{Deserialize, Serialize};
>   
>   use proxmox_schema::{ApiStringFormat, ApiType, Updater, api};
> @@ -47,38 +48,6 @@ pub struct SkipConfig {
>       pub skip_packages: Option<Vec<String>>,
>   }
>   
> -#[api(
> -    properties: {
> -        "allow-sha1": {
> -            type: bool,
> -            default: false,
> -            optional: true,
> -        },
> -        "min-dsa-key-size": {
> -            type: u64,
> -            optional: true,
> -        },
> -        "min-rsa-key-size": {
> -            type: u64,
> -            optional: true,
> -        },
> -    },
> -)]
> -#[derive(Default, Serialize, Deserialize, Updater, Clone, Debug)]
> -#[serde(rename_all = "kebab-case")]
> -/// Weak Cryptography Configuration
> -pub struct WeakCryptoConfig {
> -    /// Whether to allow SHA-1 based signatures
> -    #[serde(default)]
> -    pub allow_sha1: bool,
> -    /// Whether to lower the key size cutoff for DSA-based signatures
> -    #[serde(default)]
> -    pub min_dsa_key_size: Option<u64>,
> -    /// Whether to lower the key size cutoff for RSA-based signatures
> -    #[serde(default)]
> -    pub min_rsa_key_size: Option<u64>,
> -}
> -
>   #[api(
>       properties: {
>           id: {
> diff --git a/src/mirror.rs b/src/mirror.rs
> index b94fbdc..4e153df 100644
> --- a/src/mirror.rs
> +++ b/src/mirror.rs
> @@ -9,10 +9,11 @@ use globset::{Glob, GlobSet, GlobSetBuilder};
>   use nix::libc;
>   
>   use proxmox_http::{HttpClient, client::sync::Client};
> +use proxmox_pgp::WeakCryptoConfig;
>   use proxmox_schema::{ApiType, Schema};
>   use proxmox_sys::fs::file_get_contents;
>   
> -use crate::config::{MirrorConfig, SkipConfig, SubscriptionKey, WeakCryptoConfig};
> +use crate::config::{MirrorConfig, SkipConfig, SubscriptionKey};
>   use crate::helpers::http;
>   use crate::pool::Pool;
>   use crate::types::{Diff, SNAPSHOT_REGEX, Snapshot};
> @@ -24,8 +25,6 @@ use proxmox_apt::deb822::{
>   };
>   use proxmox_apt_api_types::{APTRepository, APTRepositoryPackageType};
>   
> -use crate::helpers;
> -
>   fn mirror_dir(config: &MirrorConfig) -> PathBuf {
>       PathBuf::from(&config.base_dir).join(&config.id)
>   }
> @@ -207,7 +206,7 @@ fn fetch_release(
>       println!("Verifying '{name}' signature using provided repository key..");
>       let content = fetched.data_ref();
>       let verified =
> -        helpers::verify_signature(content, &config.key, sig.as_deref(), &config.weak_crypto)?;
> +        proxmox_pgp::verify_signature(content, &config.key, sig.as_deref(), &config.weak_crypto)?;

Consider documenting in the commit message that this change should have 
no user visible effect (AFAICT), except for some minor changes in error 
and status reporting introduced by the refactored 
`proxmox_pgp::verify_signature`.

>       println!("Success");
>   
>       let sha512 = Some(openssl::sha::sha512(content));