Re: [pve-devel] [PATCH proxmox v5 1/4] add proxmox-pgp subcrate, move POM verifier code to it

["Shannon Sterz" <s.sterz@proxmox.com>]

On Thu Oct 23, 2025 at 12:39 PM CEST, Nicolas Frey wrote:
> Add new micro-crate `proxmox-pgp` for use in `proxmox-apt` to verify
> repository signatures. Code is derived from `proxmox-offline-mirror`.
> If this patch is applied, then `proxmox-offline-mirror` should use
> the `proxmox-pgp` crate.
>
> For the original development history, see commits related to [0].
>
> [0] https://git.proxmox.com/?p=proxmox-offline-mirror.git;a=blob;f=src/helpers/verifier.rs;h=15195a6ec3413ad3c016fffc38f9eee3cefc6827;hb=cd56fab
>
> Suggested-by: Thomas Lamprecht <t.lamprech@proxmox.com>
> Signed-off-by: Nicolas Frey <n.frey@proxmox.com>
> ---
>  Cargo.toml                       |   2 +
>  proxmox-pgp/Cargo.toml           |  17 +++
>  proxmox-pgp/debian/changelog     |   5 +
>  proxmox-pgp/debian/control       |  40 +++++++
>  proxmox-pgp/debian/copyright     |  18 +++
>  proxmox-pgp/debian/debcargo.toml |   7 ++
>  proxmox-pgp/src/lib.rs           |   5 +
>  proxmox-pgp/src/verifier.rs      | 200 +++++++++++++++++++++++++++++++
>  8 files changed, 294 insertions(+)
>  create mode 100644 proxmox-pgp/Cargo.toml
>  create mode 100644 proxmox-pgp/debian/changelog
>  create mode 100644 proxmox-pgp/debian/control
>  create mode 100644 proxmox-pgp/debian/copyright
>  create mode 100644 proxmox-pgp/debian/debcargo.toml
>  create mode 100644 proxmox-pgp/src/lib.rs
>  create mode 100644 proxmox-pgp/src/verifier.rs
>
> diff --git a/Cargo.toml b/Cargo.toml
> index 8091bf70..7d35c9fd 100644
> --- a/Cargo.toml
> +++ b/Cargo.toml
> @@ -28,6 +28,7 @@ members = [
>      "proxmox-network-types",
>      "proxmox-notify",
>      "proxmox-openid",
> +    "proxmox-pgp",
>      "proxmox-product-config",
>      "proxmox-resource-scheduling",
>      "proxmox-rest-server",
> @@ -154,6 +155,7 @@ proxmox-lang = { version = "1.5", path = "proxmox-lang" }
>  proxmox-log = { version = "1.0.0", path = "proxmox-log" }
>  proxmox-login = { version = "1.0.0", path = "proxmox-login" }
>  proxmox-network-types = { version = "0.1.0", path = "proxmox-network-types" }
> +proxmox-pgp = { version = "1.0.0", path = "proxmox-pgp" }
>  proxmox-product-config = { version = "1.0.0", path = "proxmox-product-config" }
>  proxmox-config-digest = { version = "1.0.0", path = "proxmox-config-digest" }
>  proxmox-rest-server = { version = "1.0.0", path = "proxmox-rest-server" }
> diff --git a/proxmox-pgp/Cargo.toml b/proxmox-pgp/Cargo.toml
> new file mode 100644
> index 00000000..66cac444
> --- /dev/null
> +++ b/proxmox-pgp/Cargo.toml
> @@ -0,0 +1,17 @@
> +[package]
> +name = "proxmox-pgp"
> +version = "1.0.0"
> +authors.workspace = true
> +edition.workspace = true
> +license.workspace = true
> +repository.workspace = true
> +exclude.workspace = true
> +description = "Proxmox wrapper for verifying pgp signatures using sequoia-openpgp"
> +
> +[dependencies]
> +anyhow.workspace = true
> +serde.workspace = true
> +sequoia-openpgp = "2"
> +
> +proxmox-api-macro.workspace = true
> +proxmox-schema.workspace = true
> diff --git a/proxmox-pgp/debian/changelog b/proxmox-pgp/debian/changelog
> new file mode 100644
> index 00000000..3a5a947c
> --- /dev/null
> +++ b/proxmox-pgp/debian/changelog
> @@ -0,0 +1,5 @@
> +rust-proxmox-pgp (1.0.0-1) stable; urgency=medium
> +
> +  * initial split out of proxmox offline mirror.
> +
> + -- Proxmox Support Team <support@proxmox.com>  Wed, 22 Oct 2025 14:06:52 +0200
> diff --git a/proxmox-pgp/debian/control b/proxmox-pgp/debian/control
> new file mode 100644
> index 00000000..23412bab
> --- /dev/null
> +++ b/proxmox-pgp/debian/control
> @@ -0,0 +1,40 @@
> +Source: rust-proxmox-pgp
> +Section: rust
> +Priority: optional
> +Build-Depends: debhelper-compat (= 13),
> + dh-sequence-cargo
> +Build-Depends-Arch: cargo:native <!nocheck>,
> + rustc:native <!nocheck>,
> + libstd-rust-dev <!nocheck>,
> + librust-anyhow-1+default-dev <!nocheck>,
> + librust-proxmox-api-macro-1+default-dev (>= 1.4.1-~~) <!nocheck>,
> + librust-proxmox-schema-5+default-dev <!nocheck>,
> + librust-sequoia-openpgp-2+default-dev <!nocheck>,
> + librust-serde-1+default-dev <!nocheck>
> +Maintainer: Proxmox Support Team <support@proxmox.com>
> +Standards-Version: 4.7.2
> +Vcs-Git: git://git.proxmox.com/git/proxmox.git
> +Vcs-Browser: https://git.proxmox.com/?p=proxmox.git
> +Homepage: https://git.proxmox.com/?p=proxmox.git
> +X-Cargo-Crate: proxmox-pgp
> +
> +Package: librust-proxmox-pgp-dev
> +Architecture: any
> +Multi-Arch: same
> +Depends:
> + ${misc:Depends},
> + librust-anyhow-1+default-dev,
> + librust-proxmox-api-macro-1+default-dev (>= 1.4.1-~~),
> + librust-proxmox-schema-5+default-dev,
> + librust-sequoia-openpgp-2+default-dev,
> + librust-serde-1+default-dev
> +Provides:
> + librust-proxmox-pgp+default-dev (= ${binary:Version}),
> + librust-proxmox-pgp-1-dev (= ${binary:Version}),
> + librust-proxmox-pgp-1+default-dev (= ${binary:Version}),
> + librust-proxmox-pgp-1.0-dev (= ${binary:Version}),
> + librust-proxmox-pgp-1.0+default-dev (= ${binary:Version}),
> + librust-proxmox-pgp-1.0.0-dev (= ${binary:Version}),
> + librust-proxmox-pgp-1.0.0+default-dev (= ${binary:Version})
> +Description: Proxmox wrapper for verifying pgp signatures using sequoia-openpgp - Rust source code
> + Source code for Debianized Rust crate "proxmox-pgp"
> diff --git a/proxmox-pgp/debian/copyright b/proxmox-pgp/debian/copyright
> new file mode 100644
> index 00000000..d6e3c304
> --- /dev/null
> +++ b/proxmox-pgp/debian/copyright
> @@ -0,0 +1,18 @@
> +Format: https://www.debian.org/doc/packaging-manuals/copyright-format/1.0/
> +
> +Files:
> + *
> +Copyright: 2025 Proxmox Server Solutions GmbH <support@proxmox.com>
> +License: AGPL-3.0-or-later
> + This program is free software: you can redistribute it and/or modify it under
> + the terms of the GNU Affero General Public License as published by the Free
> + Software Foundation, either version 3 of the License, or (at your option) any
> + later version.
> + .
> + This program is distributed in the hope that it will be useful, but WITHOUT
> + ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
> + FOR A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
> + details.
> + .
> + You should have received a copy of the GNU Affero General Public License along
> + with this program. If not, see <https://www.gnu.org/licenses/>.
> diff --git a/proxmox-pgp/debian/debcargo.toml b/proxmox-pgp/debian/debcargo.toml
> new file mode 100644
> index 00000000..b7864cdb
> --- /dev/null
> +++ b/proxmox-pgp/debian/debcargo.toml
> @@ -0,0 +1,7 @@
> +overlay = "."
> +crate_src_path = ".."
> +maintainer = "Proxmox Support Team <support@proxmox.com>"
> +
> +[source]
> +vcs_git = "git://git.proxmox.com/git/proxmox.git"
> +vcs_browser = "https://git.proxmox.com/?p=proxmox.git"
> diff --git a/proxmox-pgp/src/lib.rs b/proxmox-pgp/src/lib.rs
> new file mode 100644
> index 00000000..08a873fd
> --- /dev/null
> +++ b/proxmox-pgp/src/lib.rs
> @@ -0,0 +1,5 @@
> +#![cfg_attr(docsrs, feature(doc_cfg, doc_auto_cfg))]
> +
> +mod verifier;
> +
> +pub use verifier::{verify_signature, WeakCryptoConfig, WeakCryptoConfigUpdater};
> diff --git a/proxmox-pgp/src/verifier.rs b/proxmox-pgp/src/verifier.rs
> new file mode 100644
> index 00000000..84a15c4c
> --- /dev/null
> +++ b/proxmox-pgp/src/verifier.rs
> @@ -0,0 +1,200 @@
> +use anyhow::{bail, format_err, Error};
> +
> +use proxmox_api_macro::api;
> +use proxmox_schema::Updater;
> +use sequoia_openpgp::{
> +    cert::CertParser,
> +    parse::{
> +        stream::{
> +            DetachedVerifierBuilder, MessageLayer, MessageStructure, VerificationError,
> +            VerificationHelper, VerifierBuilder,
> +        },
> +        PacketParser, PacketParserResult, Parse,
> +    },
> +    policy::StandardPolicy,
> +    types::HashAlgorithm,
> +    Cert, KeyHandle,
> +};
> +use serde::{Deserialize, Serialize};
> +use std::io;
> +

nit: we usually prefer module level imports and the following ordering
of imports: std -> generic crates (e.g. anyhow, serde sequoia etc.) ->
proxmox* crates -> project specific crates. so this should be:

use std::io;

use anyhow::{..}
use sequoia_openpgp::cert::CertParser;
use sequoia_openpgp::parse::stream::{..};
[..]

use proxmox_api_macro::api;

and so on :)

> +#[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>,
> +}
> +
> +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,

this is probably pre-existing, but is there a good reason we can't short
circuit here and return `Ok(())` on the first good result?

if one signature is good, we set `good` here to true anyway, which means
the code below just returns `Ok(())` and ignores any errors.

however, if we can't short-circuit here, for example if this would open
a timing side channel, then it'd be good to document that here too imo.

> +                        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!();

since this is now a library function, i'm not sure that unconditionally
printing to `stderr` makes sense. might be nicer to use the log crate,
or using anyhow `Context` to return that with the error below. would be
nice to get another opinion here, though.

> +            Err(anyhow::anyhow!("No valid signature found."))
> +        }
> +    }
> +}
> +
> +/// Verifies GPG-signed `msg` was signed by `key`, returning the verified data without signature.
> +pub 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!");
> +}



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel