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

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

[Nicolas Frey]

As proxmox-pgp crate is now available, use it here and get rid of the
verifier module.

Also fixed two lifetime related clippy lints in the first patch.

proxmox-offline-mirror:

Nicolas Frey (3):
  clippy: elide redundant lifetimes
  use proxmox-pgp crate to replace verifier helper module
  verifier: remove module

 Cargo.toml                               |   1 +
 src/bin/proxmox-offline-mirror-helper.rs |   2 +-
 src/config.rs                            |  33 +----
 src/helpers/mod.rs                       |   3 -
 src/helpers/verifier.rs                  | 163 -----------------------
 src/medium.rs                            |   2 +-
 src/mirror.rs                            |   7 +-
 7 files changed, 7 insertions(+), 204 deletions(-)
 delete mode 100644 src/helpers/verifier.rs


Summary over all repositories:
  7 files changed, 7 insertions(+), 204 deletions(-)

-- 
Generated by git-murpp 0.8.1

[PATCH proxmox-offline-mirror 1/3] clippy: elide redundant lifetimes

[Nicolas Frey]

No functional changes.

for const, 'static lifetime is the default and
`mountpoint_to_path_exists` had needless lifetimes, so fix it

Signed-off-by: Nicolas Frey <n.frey@proxmox.com>
---
 src/bin/proxmox-offline-mirror-helper.rs | 2 +-
 src/medium.rs                            | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/bin/proxmox-offline-mirror-helper.rs b/src/bin/proxmox-offline-mirror-helper.rs
index 0c40338..19bf39e 100644
--- a/src/bin/proxmox-offline-mirror-helper.rs
+++ b/src/bin/proxmox-offline-mirror-helper.rs
@@ -27,7 +27,7 @@ use proxmox_offline_mirror::helpers::tty::{
 use proxmox_offline_mirror::medium::{self, MIRROR_STATE_FILE, MediumState, generate_repo_snippet};
 
 /// Converts a string slice to a Path, if it exists, otherwise returns an error result.
-fn mountpoint_to_path_exists<'a>(mountpoint: &'a str) -> Result<&'a Path, Error> {
+fn mountpoint_to_path_exists(mountpoint: &str) -> Result<&Path, Error> {
     let mountpoint = Path::new(mountpoint);
     if !mountpoint.exists() {
         bail!("Medium mountpoint doesn't exist.");
diff --git a/src/medium.rs b/src/medium.rs
index 6d9a456..3eb935d 100644
--- a/src/medium.rs
+++ b/src/medium.rs
@@ -18,7 +18,7 @@ use crate::mirror::pool;
 use crate::pool::Pool;
 use crate::types::{Diff, SNAPSHOT_REGEX, Snapshot};
 
-pub const MIRROR_STATE_FILE: &'static str = ".mirror-state";
+pub const MIRROR_STATE_FILE: &str = ".mirror-state";
 
 #[derive(Clone, Debug, Serialize, Deserialize)]
 #[serde(rename_all = "kebab-case")]
-- 
2.47.3

Re: [PATCH proxmox-offline-mirror 1/3] clippy: elide redundant lifetimes

[Erik Fastermann]

> No functional changes.
> 
> for const, 'static lifetime is the default and
> `mountpoint_to_path_exists` had needless lifetimes, so fix it
> 
> Signed-off-by: Nicolas Frey <n.frey@proxmox.com>

LGTM

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

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

[Nicolas Frey]

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;
 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)?;
     println!("Success");
 
     let sha512 = Some(openssl::sha::sha512(content));
-- 
2.47.3

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

[Erik Fastermann]

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

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

[Nicolas Frey]

Thanks for the review! (almost forgot about this series)
I'll send a v2 with your proposals

On 7/22/26 2:44 PM, Erik Fastermann wrote:
> 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.
> 

ack

[snip]

>> @@ -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`.
> 

I suppose it's not clear here, as the code lives in a different crate
now. I'll add it as context to the commit message. Thanks!

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

[PATCH proxmox-offline-mirror 3/3] verifier: remove module

[Nicolas Frey]

Signed-off-by: Nicolas Frey <n.frey@proxmox.com>
---
 src/helpers/mod.rs      |   3 -
 src/helpers/verifier.rs | 163 ----------------------------------------
 2 files changed, 166 deletions(-)
 delete mode 100644 src/helpers/verifier.rs

diff --git a/src/helpers/mod.rs b/src/helpers/mod.rs
index fb084a0..906a087 100644
--- a/src/helpers/mod.rs
+++ b/src/helpers/mod.rs
@@ -1,6 +1,3 @@
 pub mod http;
 
 pub mod tty;
-
-mod verifier;
-pub(crate) use verifier::verify_signature;
diff --git a/src/helpers/verifier.rs b/src/helpers/verifier.rs
deleted file mode 100644
index e1a663e..0000000
--- a/src/helpers/verifier.rs
+++ /dev/null
@@ -1,163 +0,0 @@
-use std::io;
-
-use anyhow::{Error, bail, format_err};
-use sequoia_openpgp::cert::CertParser;
-use sequoia_openpgp::parse::stream::{
-    DetachedVerifierBuilder, MessageLayer, MessageStructure, VerificationError, VerificationHelper,
-    VerifierBuilder,
-};
-use sequoia_openpgp::parse::{PacketParser, PacketParserResult, Parse};
-use sequoia_openpgp::policy::StandardPolicy;
-use sequoia_openpgp::types::HashAlgorithm;
-use sequoia_openpgp::{Cert, KeyHandle};
-
-use crate::config::WeakCryptoConfig;
-
-struct Helper<'a> {
-    cert: &'a Cert,
-}
-
-impl VerificationHelper for Helper<'_> {
-    fn get_certs(&mut self, _ids: &[KeyHandle]) -> sequoia_openpgp::Result<Vec<Cert>> {
-        // Return public keys for signature verification here.
-        Ok(vec![self.cert.clone()])
-    }
-
-    fn check(&mut self, structure: MessageStructure) -> sequoia_openpgp::Result<()> {
-        // In this function, we implement our signature verification policy.
-
-        let mut good = false;
-
-        // we don't want compression and/or encryption
-        let layers: Vec<_> = structure.iter().collect();
-        if layers.len() > 1 || layers.is_empty() {
-            bail!(
-                "unexpected GPG message structure - expected plain signed data, got {} layers!",
-                layers.len()
-            );
-        }
-        let layer = &layers[0];
-        let mut errors = Vec::new();
-        match layer {
-            MessageLayer::SignatureGroup { results } => {
-                // We possibly have multiple signatures, but not all keys, so `or` all the individual results.
-                for result in results {
-                    match result {
-                        Ok(_) => good = true,
-                        Err(e) => errors.push(e),
-                    }
-                }
-            }
-            _ => return Err(anyhow::anyhow!("Unexpected message structure")),
-        }
-
-        if good {
-            Ok(()) // Good signature.
-        } else {
-            if errors.len() > 1 {
-                eprintln!("\nEncountered {} errors:", errors.len());
-            }
-
-            for (n, err) in errors.iter().enumerate() {
-                if errors.len() > 1 {
-                    eprintln!("\nSignature #{n}: {err}");
-                } else {
-                    eprintln!("\n{err}");
-                }
-                match err {
-                    VerificationError::MalformedSignature { error, .. }
-                    | VerificationError::UnboundKey { error, .. }
-                    | VerificationError::BadKey { error, .. }
-                    | VerificationError::BadSignature { error, .. } => {
-                        let mut cause = error.chain();
-                        if cause.len() > 1 {
-                            cause.next(); // already included in `err` above
-                            eprintln!("Caused by:");
-                            for (n, e) in cause.enumerate() {
-                                eprintln!("\t{n}: {e}");
-                            }
-                        }
-                    }
-                    VerificationError::MissingKey { .. }
-                    | VerificationError::UnknownSignature { .. } => {} // doesn't contain a cause
-                    _ => {} // we already print the error above in full
-                };
-            }
-            eprintln!();
-            Err(anyhow::anyhow!("No valid signature found."))
-        }
-    }
-}
-
-/// Verifies GPG-signed `msg` was signed by `key`, returning the verified data without signature.
-pub(crate) fn verify_signature(
-    msg: &[u8],
-    key: &[u8],
-    detached_sig: Option<&[u8]>,
-    weak_crypto: &WeakCryptoConfig,
-) -> Result<Vec<u8>, Error> {
-    let mut policy = StandardPolicy::new();
-    if weak_crypto.allow_sha1 {
-        policy.accept_hash(HashAlgorithm::SHA1);
-    }
-    if let Some(min_dsa) = weak_crypto.min_dsa_key_size {
-        if min_dsa <= 1024 {
-            policy.accept_asymmetric_algo(sequoia_openpgp::policy::AsymmetricAlgorithm::DSA1024);
-        }
-    }
-    if let Some(min_rsa) = weak_crypto.min_rsa_key_size {
-        if min_rsa <= 1024 {
-            policy.accept_asymmetric_algo(sequoia_openpgp::policy::AsymmetricAlgorithm::RSA1024);
-        }
-    }
-
-    let verifier = |cert| {
-        let helper = Helper { cert: &cert };
-
-        if let Some(sig) = detached_sig {
-            let mut verifier =
-                DetachedVerifierBuilder::from_bytes(sig)?.with_policy(&policy, None, helper)?;
-            verifier.verify_bytes(msg)?;
-            Ok(msg.to_vec())
-        } else {
-            let mut verified = Vec::new();
-            let mut verifier =
-                VerifierBuilder::from_bytes(msg)?.with_policy(&policy, None, helper)?;
-            let bytes = io::copy(&mut verifier, &mut verified)?;
-            println!("{bytes} bytes verified");
-            if !verifier.message_processed() {
-                bail!("Failed to verify message!");
-            }
-            Ok(verified)
-        }
-    };
-
-    let mut packed_parser = PacketParser::from_bytes(key)?;
-
-    // parse all packets to see whether this is a simple certificate or a keyring
-    while let PacketParserResult::Some(pp) = packed_parser {
-        packed_parser = pp.recurse()?.1;
-    }
-
-    if let PacketParserResult::EOF(eof) = packed_parser {
-        // verify against a single certificate
-        if eof.is_cert().is_ok() {
-            let cert = Cert::from_bytes(key)?;
-            return verifier(cert);
-        // verify against a keyring
-        } else if eof.is_keyring().is_ok() {
-            let packed_parser = PacketParser::from_bytes(key)?;
-
-            return CertParser::from(packed_parser)
-                // flatten here as we ignore packets that aren't a certificate
-                .flatten()
-                // keep trying to verify the message until the first certificate that succeeds
-                .find_map(|c| verifier(c).ok())
-                // if no certificate verified the message, abort
-                .ok_or_else(|| format_err!("No key in keyring could verify the message!"));
-        }
-    }
-
-    // neither a keyring nor a certificate was detect, so we abort here
-    bail!("'key-path' contains neither a keyring nor a certificate, aborting!");
-}
-- 
2.47.3

Re: [PATCH proxmox-offline-mirror 3/3] verifier: remove module

[Erik Fastermann]

LGTM

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

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

[Nicolas Frey]

ping, still applies

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

[Nicolas Frey]

Superseded-by: https://lore.proxmox.com/pve-devel/20260722142234.309937-1-n.frey@proxmox.com/T/#t