From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 802591FF191 for ; Tue, 21 Oct 2025 15:49:04 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 384D81FB17; Tue, 21 Oct 2025 15:49:28 +0200 (CEST) Message-ID: <3c043071-c50a-49a9-93ae-095feeede22e@proxmox.com> Date: Tue, 21 Oct 2025 15:49:23 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Thomas Lamprecht , Proxmox VE development discussion References: <20251020144553.360147-1-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: 1761054556601 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.873 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 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 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [verifier.rs, mod.rs, proxmox.com, repository.rs, api.rs] Subject: Re: [pve-devel] [PATCH v4] fix #5207: apt: check signage of repos with gpgv for POM 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 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" On 10/21/25 2:40 PM, Thomas Lamprecht wrote: > Am 20.10.25 um 16:46 schrieb Nicolas Frey: >> If POM is set up to mirror the PVE repository and only this repository >> is added on a PVE host, the `Repositories` panel will show an `Error` >> status with the message: >> >> `No Proxmox VE repository is enabled, you do not get any updates!` >> >> This is because the current implementation only checks if the uri of >> the repo matches that of one of the standard repos. >> >> This patch aims to fix this issue by verifying it through signage >> info via gpgv. The InRelease file is used to check whether the > > The apt version in Debian Trixie switched from using gpgv to sqv from > the sequioa project (basically doing PGP in rust), so using sqv here > would be probably good to keep in sync and future proof. > > And while it can be OK to use the executable, especially as stop-gap > to avoid scope creep now, it might be nicer in the long term to use the > fact that sequoia provides rust library crates which we actually already > use in POM. One actually could copy the relevant code from POM's > src/helpers/verifier.rs module over here (either inline it as it's not > much or, for bonus points, add a new promxox-pgp (or similar named) > micro-crate and reuse it here and in POM. > > But again, we can split this into two separate steps to avoid having to > much upfront work/scope. > I did actually look into using the sequoia rust crate, and wanted to ask for advice/if that would be a good Idea, but ultimately forgot to do so, sorry. >> package is of Proxmox Origin. The cached file at `/var/lib/apt/lists/` >> is used if present, otherwise it is fetched from the repos URI, which >> solves the issue of needing to update the cached files, which would >> need an apt update. This way the feedback is immediate even for POM >> configured repos. > > Well, immediate if fetching works out (POM might not be online all > the time) and all is fast enough. > > But IMO fetching InRelease files via the HTTP client should not be the > job of the apt crate, requiring the user to have refreshed the repository > states at least once is really fine, especially as this is done > automatically once a day anyway. > That makes sense, though IMO checking for the local InRelease if the repo points to a mounted POM (i.e. file:// uri prefix) could in that case be kept as a bit of an improvement to the UX. What do you think? > I.e., this falls a bit to close to over engineering for a fix for my > taste. If the need to get the initial repo state at least once is really a > problem for users, I'd wait on actual reports to see how to address that, > as that could be also a documentation update and/or showing a hint on the > web UI that the repo was never fetched. I think displaying a hint would be the best UX here, I can do that in a follow-up. > >> Added tests to ensure common file paths for POM would be encoded >> correctly. >> > > The intra-patch changes list below is great to have, so thanks for that, > but as it's meta information relevant for patch review, it's not really > relevant to be encoded in the git history through the commit message, > so it should rather go into the diffstat area below, namely down ... Okay, will do. Thanks for pointing it out! > >> Changes since v3: >> * Moved found_uri_or_signed to function and to the end of bool chain >> to prevent redundant signage checks to improve performance >> * Added fallback to the cached InRelease file to get it from repos URI >> >> Changes since v2: >> * correct the mapping in `gpg_signed` >> >> Changes since v1: >> * rewrite test so it compiles >> >> Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=5207 >> Signed-off-by: Nicolas Frey >> --- > > ... here > >> proxmox-apt/Cargo.toml | 1 + >> proxmox-apt/src/api.rs | 5 +- >> proxmox-apt/src/repositories/mod.rs | 6 +- >> proxmox-apt/src/repositories/repository.rs | 105 +++++++++++++++++++-- >> 4 files changed, 105 insertions(+), 12 deletions(-) >> >> diff --git a/proxmox-apt/Cargo.toml b/proxmox-apt/Cargo.toml >> index e5beb4e6..ae78e31c 100644 >> --- a/proxmox-apt/Cargo.toml >> +++ b/proxmox-apt/Cargo.toml >> @@ -17,6 +17,7 @@ hex.workspace = true >> openssl.workspace = true >> serde = { workspace = true, features = ["derive"] } >> serde_json.workspace = true >> +proxmox-http = { workspace = true, features = [ "client-sync" ] } >> >> rfc822-like = "0.2.1" >> >> diff --git a/proxmox-apt/src/api.rs b/proxmox-apt/src/api.rs >> index 14dfb53b..b5fa10f4 100644 >> --- a/proxmox-apt/src/api.rs >> +++ b/proxmox-apt/src/api.rs >> @@ -8,6 +8,7 @@ use proxmox_apt_api_types::{ >> APTRepositoryHandle, >> }; >> use proxmox_config_digest::ConfigDigest; >> +use proxmox_http::client::sync::Client; >> >> use crate::repositories::{APTRepositoryFileImpl, APTRepositoryImpl}; >> >> @@ -61,10 +62,12 @@ pub fn add_repository_handle( >> >> let suite = crate::repositories::get_current_release_codename()?; >> >> + let client = Client::default(); >> + >> // check if it's already configured first >> for file in files.iter_mut() { >> for repo in file.repositories.iter_mut() { >> - if repo.is_referenced_repository(handle, "pbs", &suite.to_string()) { >> + if repo.is_referenced_repository(handle, "pbs", &suite.to_string(), &client) { >> if repo.enabled { >> return Ok(()); >> } >> diff --git a/proxmox-apt/src/repositories/mod.rs b/proxmox-apt/src/repositories/mod.rs >> index 2b52c774..4fa043fc 100644 >> --- a/proxmox-apt/src/repositories/mod.rs >> +++ b/proxmox-apt/src/repositories/mod.rs >> @@ -10,6 +10,7 @@ use proxmox_apt_api_types::{ >> APTStandardRepository, >> }; >> use proxmox_config_digest::ConfigDigest; >> +use proxmox_http::client::sync::Client; >> pub use repository::APTRepositoryImpl; >> >> mod file; >> @@ -100,6 +101,8 @@ pub fn standard_repositories( >> ]); >> } >> >> + let client = Client::default(); >> + >> for file in files.iter() { >> for repo in file.repositories.iter() { >> for entry in result.iter_mut() { >> @@ -107,7 +110,8 @@ pub fn standard_repositories( >> continue; >> } >> >> - if repo.is_referenced_repository(entry.handle, product, &suite.to_string()) { >> + if repo.is_referenced_repository(entry.handle, product, &suite.to_string(), &client) >> + { >> entry.status = Some(repo.enabled); >> } >> } >> diff --git a/proxmox-apt/src/repositories/repository.rs b/proxmox-apt/src/repositories/repository.rs >> index 24e7943b..47cf50e3 100644 >> --- a/proxmox-apt/src/repositories/repository.rs >> +++ b/proxmox-apt/src/repositories/repository.rs >> @@ -1,7 +1,10 @@ >> use std::io::{BufRead, BufReader, Write}; >> use std::path::{Path, PathBuf}; >> +use std::process::{Command, Stdio}; >> >> use anyhow::{bail, format_err, Error}; >> +use proxmox_http::client::sync::Client; >> +use proxmox_http::HttpClient; >> >> use crate::repositories::standard::APTRepositoryHandleImpl; >> use proxmox_apt_api_types::{ >> @@ -25,6 +28,7 @@ pub trait APTRepositoryImpl { >> handle: APTRepositoryHandle, >> product: &str, >> suite: &str, >> + client: &Client, >> ) -> bool; >> >> /// Guess the origin from the repository's URIs. >> @@ -121,22 +125,26 @@ impl APTRepositoryImpl for APTRepository { >> handle: APTRepositoryHandle, >> product: &str, >> suite: &str, >> + client: &Client, >> ) -> bool { >> - let (package_type, handle_uris, component, _key) = handle.info(product); >> - >> - let mut found_uri = false; >> - >> - for uri in self.uris.iter() { >> - let uri = uri.trim_end_matches('/'); >> - >> - found_uri = found_uri || handle_uris.iter().any(|handle_uri| handle_uri == uri); >> - } >> + let (package_type, handle_uris, component, key) = handle.info(product); >> + >> + let found_uri_or_signed = || { >> + let mut found = false; >> + for uri in self.uris.iter() { >> + let uri = uri.trim_end_matches('/'); >> + found = found >> + || handle_uris.iter().any(|handle_uri| handle_uri == uri) >> + || gpg_signed(&client, uri, suite, key); >> + } >> + found >> + }; >> >> self.types.contains(&package_type) >> - && found_uri >> // using contains would require a &String >> && self.suites.iter().any(|self_suite| self_suite == suite) >> && self.components.contains(&component) >> + && found_uri_or_signed() >> } >> >> fn origin_from_uris(&self) -> Option { >> @@ -389,8 +397,85 @@ fn write_stanza(repo: &APTRepository, w: &mut dyn Write) -> Result<(), Error> { >> Ok(()) >> } >> >> +fn gpg_signed(client: &Client, uri: &str, suite: &str, key: &str) -> bool { > > Even if we'd go for the auto-download functionality, which I'm rather against: > This is not a nice function as it does way to much, i.e. a fn named gpg_signed > should check purely if something is gpg signed, not also download stuff from > HTTP, let's please keep the scope of fns clean and avoid to many side-effects. > >> + let path = release_filename(Path::new("/var/lib/apt/lists"), uri, suite, false); >> + >> + // check cached file path first >> + let data = if path.exists() { >> + let Ok(d) = std::fs::read(path) else { >> + return false; >> + }; >> + d >> + } else if uri.starts_with("file://") { >> + let Ok(d) = std::fs::read(&format!( >> + "{}/dists/{}/InRelease", >> + &uri["file://".len()..], >> + suite >> + )) else { >> + return false; >> + }; >> + d >> + } else { >> + match client.get( >> + &format!("{}/dists/{}/InRelease", uri.trim_end_matches('/'), suite), >> + None, >> + ) { >> + Ok(resp) if resp.status().is_success() => resp.into_body(), >> + _ => return false, >> + } >> + }; >> + >> + let Ok(mut gpgv) = Command::new("gpgv") >> + .args(["--keyring", key, "-"]) >> + .stdin(Stdio::piped()) >> + .stdout(Stdio::null()) >> + .stderr(Stdio::null()) >> + .spawn() >> + else { >> + return false; >> + }; >> + >> + if let Some(mut stdin) = gpgv.stdin.take() { >> + if stdin.write_all(&data).is_err() { > > error reporting (log) would be great, as otherwise hunting an issue down > here becomes needlessly harder than it could be. > >> + let _ = gpgv.kill(); >> + return false; >> + } >> + } >> + >> + gpgv.wait().is_ok_and(|status| status.success()) >> +} >> + >> #[test] >> fn test_uri_to_filename() { >> let filename = uri_to_filename("https://some_host/some/path"); >> assert_eq!(filename, "some%5fhost_some_path".to_string()); >> } >> + >> +#[test] >> +fn test_release_filename() { >> + let data = [ >> + // testcase for proxmox offline mirror (mounted) >> + ( >> + Path::new("/var/lib/apt/lists"), >> + "file:///mnt/mirror/pve-no-subscription/2025-10-16T08:07:41Z", >> + "trixie", >> + false, >> + // expected >> + "/var/lib/apt/lists/_mnt_mirror_pve-no-subscription_2025-10-16T08:07:41Z_dists_trixie_InRelease" >> + ), >> + // testcase for proxmox offline mirror (local http server) >> + ( >> + Path::new("/var/lib/apt/lists"), >> + "http://proxmox-offline-mirror.domain.example/pve-subscription/2025-10-16T08:07:41Z", >> + "trixie", >> + false, >> + // expected >> + "/var/lib/apt/lists/proxmox-offline-mirror.domain.example_pve-subscription_2025-10-16T08:07:41Z_dists_trixie_InRelease" >> + ), >> + ]; >> + >> + for d in data { >> + let filename = release_filename(d.0, d.1, d.2, d.3).display().to_string(); >> + assert_eq!(filename, d.4); >> + } >> +} > _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel