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 B90BE1FF191 for ; Tue, 21 Oct 2025 14:41:12 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 5783B1E23C; Tue, 21 Oct 2025 14:41:36 +0200 (CEST) Message-ID: Date: Tue, 21 Oct 2025 14:41:01 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta To: Proxmox VE development discussion , Nicolas Frey References: <20251020144553.360147-1-n.frey@proxmox.com> Content-Language: en-US From: Thomas Lamprecht In-Reply-To: <20251020144553.360147-1-n.frey@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1761050454877 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.027 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. [api.rs, repository.rs, verifier.rs, proxmox.com, mod.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" 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. > 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. 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. > 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 ... > 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