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

[Nicolas Frey <n.frey@proxmox.com>]

On 10/29/25 11:54 AM, Shannon Sterz wrote:
> 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 :)
> 

Ah, I remember seeing something on the list recently which talked
about this. I'll adjust my rustfmt to use module level imports then

Thanks for letting me know! Will adjust in a v6

>> +#[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.
> 

yes this is pre-existing. I *think* short-circuiting here is fine,
though if this code were to be used anywhere where a timing side
attack would be of concern, it could be argued to be left as-is.

If leaving this behaviour in, I can document it in a v6

>> +                        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.
> 

IMO leaving the error logging outside of this crate makes more sense
(i.e. using and returning anyhow `Context`) as this would otherwise
add another dependency to this micro-crate just for logging this one
error. might also be good to give control over the log level to the
caller of the function when doing it this way.

>> +            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