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 09D031FF179 for ; Wed, 29 Oct 2025 14:44:26 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 72F7ABC44; Wed, 29 Oct 2025 14:44:59 +0100 (CET) Message-ID: <27e6ea94-7c29-48df-919c-0b0208de9057@proxmox.com> Date: Wed, 29 Oct 2025 14:44:23 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Shannon Sterz References: <20251023103953.305810-1-n.frey@proxmox.com> <20251023103953.305810-2-n.frey@proxmox.com> From: Nicolas Frey Content-Language: en-US In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1761745450672 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.835 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 RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. 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 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 >> 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 :) > 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, >> + /// 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. > 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, 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