From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 312161FF179 for ; Wed, 29 Oct 2025 11:55:00 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 7ED4D6F2C; Wed, 29 Oct 2025 11:55:33 +0100 (CET) Mime-Version: 1.0 Date: Wed, 29 Oct 2025 11:54:57 +0100 Message-Id: To: "Nicolas Frey" X-Mailer: aerc 0.20.0 References: <20251023103953.305810-1-n.frey@proxmox.com> <20251023103953.305810-2-n.frey@proxmox.com> In-Reply-To: <20251023103953.305810-2-n.frey@proxmox.com> From: "Shannon Sterz" X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1761735284651 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.060 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment KAM_SHORT 0.001 Use of a URL Shortener for very short URL SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH proxmox v5 1/4] add proxmox-pgp subcrate, move POM verifier code to it X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Cc: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" 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 > Signed-off-by: Nicolas Frey > --- > 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 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 , > + rustc:native , > + libstd-rust-dev , > + 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 > +Maintainer: Proxmox Support Team > +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 > +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 . > 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 " > + > +[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, > + /// Whether to lower the key size cutoff for RSA-based signatures > + #[serde(default)] > + pub min_rsa_key_size: Option, > +} > + > +struct Helper<'a> { > + cert: &'a Cert, > +} > + > +impl VerificationHelper for Helper<'_> { > + fn get_certs(&mut self, _ids: &[KeyHandle]) -> sequoia_openpgp::Result> { > + // 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, 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