Re: [PATCH datacenter-manager 07/15] server: connection: report mismatching fingerprint as untrusted on probe

["Shannon Sterz" <s.sterz@proxmox.com>]

On Fri Jul 31, 2026 at 11:16 AM CEST, Shannon Sterz wrote:
> instead of erroring out. previously this function returned a
> connection error if the provided fingerprint did not match the
> remote's fingerprint. instead, report the certificate as untrusted,
> giving client's more appropriate information in such cases.
>
> note that while the documentation for this function was technically
> correct, the probe_tls endpoints for pve and pbs remotes stated:
>
>> If the certificate is not trusted with the given parameters, returns
>> the certificate information.
>
> however, that was incorrect, since the endpoints returned an error if
> the fingerprint did not match. since those two endpoints are currently
> the only users that could actually provide a fingerprint (all other
> callers explicitly provide `None`), this is more of a bug fix than a
> public api break.
>
> Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
> ---
>  server/src/connection.rs | 62 ++++++++++++++++++++++++++++------------
>  1 file changed, 43 insertions(+), 19 deletions(-)
>
> diff --git a/server/src/connection.rs b/server/src/connection.rs
> index f635b74e..51bbb611 100644
> --- a/server/src/connection.rs
> +++ b/server/src/connection.rs
> @@ -15,6 +15,7 @@ use std::time::{Duration, SystemTime};
>  use anyhow::{Error, bail, format_err};
>  use http::Method;
>  use http::uri::Authority;
> +use openssl::hash::MessageDigest;
>  use openssl::x509::X509StoreContextRef;
>  use serde::Serialize;
>
> @@ -22,6 +23,7 @@ use proxmox_acme_api::CertificateInfo;
>  use proxmox_client::{Client, HttpApiClient, HttpApiResponse, HttpApiResponseStream, TlsOptions};
>  use proxmox_time::epoch_i64;
>
> +use pdm_api_types::Fingerprint;
>  use pdm_api_types::remotes::{NodeUrl, Remote, RemoteType, TlsProbeOutcome};
>  use pve_api_types::client::PveClientImpl;
>
> @@ -929,7 +931,7 @@ impl HttpApiClient for MultiClient {
>  /// Checks TLS connection to the given remote
>  ///
>  /// Returns `Ok(TlsProbeOutcome::TrustedCertificate)` if connecting with the given parameters works
> -/// Returns `Ok(TlsProbeOutcome::UntrustedCertificate)` if no fingerprint was given and some certificate could not be validated
> +/// Returns `Ok(TlsProbeOutcome::UntrustedCertificate)` if the provided fingerprint does not match or a certificate could not be validated
>  /// Returns `Err(err)` if some other error occurred
>  ///
>  /// # Example
> @@ -964,25 +966,47 @@ pub async fn probe_tls_connection(
>      // to save the invalid cert we find
>      let invalid_cert = Arc::new(StdMutex::new(None));
>
> -    let options = if let Some(fp) = &fingerprint {
> -        TlsOptions::parse_fingerprint(fp)?
> -    } else {
> -        TlsOptions::Callback(Box::new({
> -            let invalid_cert = invalid_cert.clone();
> -            move |valid: bool, chain: &mut X509StoreContextRef| {
> -                if let Some(cert) = chain.current_cert() {
> -                    if !valid {
> -                        let cert = cert
> -                            .to_pem()
> -                            .map_err(Error::from)
> -                            .and_then(|pem| CertificateInfo::from_pem("", &pem));
> -                        *invalid_cert.lock().unwrap() = Some(cert);
> -                    }
> -                }
> -                true
> +    let fingerprint = fingerprint
> +        .map(|fp| fp.parse::<Fingerprint>())
> +        .transpose()?;
> +
> +    let options = TlsOptions::Callback(Box::new({
> +        let invalid_cert = invalid_cert.clone();
> +        move |valid: bool, chain: &mut X509StoreContextRef| {
> +            // If no fingerprint was provided and the trust store trusts the certificate, the
> +            // connection is valid.
> +            if fingerprint.is_none() && valid {
> +                return true;
>              }
> -        }))
> -    };
> +
> +            let Some(cert) = chain.current_cert() else {

something i just noticed is that this here should also always get the
leaf certificate. sorry for missing that, i'll clean that up in a v2

for the record, all that needs to change is that the above line should
read:

    let Some(cert) = chain.chain().and_then(|c| c.get(0)) else {

to renaming the (pre-existing) chain parameter to "context" or similar
would probably also help with clarity here.

> +                return true;
> +            };
> +
> +            // If a fingerprint was provided and the certificate matches it, the connection is
> +            // valid.
> +            if let Some(provided_fp) = &fingerprint {
> +                if cert
> +                    .digest(MessageDigest::sha256())
> +                    .map(|fp| *fp == **provided_fp)
> +                    .unwrap_or(false)
> +                {
> +                    return true;
> +                }
> +            }
> +
> +            // Otherwise, the certificate is not trusted.
> +            let cert = cert
> +                .to_pem()
> +                .map_err(Error::from)
> +                .and_then(|pem| CertificateInfo::from_pem("", &pem));
> +
> +            *invalid_cert.lock().unwrap() = Some(cert);
> +
> +            true
> +        }
> +    }));
> +
>      let client = proxmox_client::Client::with_options(uri, options, Default::default())?;
>
>      // set fake auth info. we don't need any, but the proxmox client will return unauthenticated if