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

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

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 <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