Re: [pve-devel] [PATCH v4] fix #5207: apt: check signage of repos with gpgv for POM

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

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 <n.frey@proxmox.com>
>> ---
> 
> ... 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<String> {
>> @@ -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