Re: [pdm-devel] [PATCH datacenter-manager 03/21] server: connection: add probe_tls_connection helper

["Lukas Wagner" <l.wagner@proxmox.com>]

Hey,

looks mostly good, two suggestions inline.

On Fri May 16, 2025 at 3:35 PM CEST, Dominik Csapak wrote:
> this is intended to help us probe a remote/host before using it to check
> whether the tls connection is working fine, or it returns the
> certificate information so we can show it to the user.
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  server/src/connection.rs | 79 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
>
> diff --git a/server/src/connection.rs b/server/src/connection.rs
> index 0be9033..c7b2558 100644
> --- a/server/src/connection.rs
> +++ b/server/src/connection.rs
> @@ -15,8 +15,10 @@ use std::time::{Duration, SystemTime};
>  use anyhow::{bail, format_err, Error};
>  use http::uri::Authority;
>  use http::Method;
> +use openssl::x509::X509StoreContextRef;
>  use serde::Serialize;
>  
> +use proxmox_acme_api::CertificateInfo;
>  use proxmox_client::{Client, HttpApiClient, HttpApiResponse, HttpApiResponseStream, TlsOptions};
>  
>  use pdm_api_types::remotes::{NodeUrl, Remote, RemoteType};
> @@ -799,3 +801,80 @@ impl HttpApiClient for MultiClient {
>          try_request! { self, method, path_and_query, params, streaming_request }
>      }
>  }
> +
> +/// Checks TLS connection to the given remote
> +///
> +/// Returns `Ok(None)` if connecting with the given parameters works
> +/// Returns `Ok(Some(cert))` if no fingerprint was given and some certificate could not be validated
> +/// Returns `Err(err)` if some other error occurred

The intent could be maybe a bit more clear if you used some kind of enum
instead of the Option:

e.g.

enum ProbeOutcome {
   Success,
   UntrustedCertificate(CertificateInfo),
}

(Maybe there's a better name than 'Success' for the first variant, this
is just what came to mind right now).

> +///
> +/// # Example
> +///
> +/// ```
> +/// use server::connection::probe_tls_connection;
> +/// use pdm_api_types::remotes::RemoteType;
> +///
> +/// # async fn function() {
> +/// let result = probe_tls_connection(RemoteType::Pve, "192.168.2.100".to_string(), None).await;
> +/// match result {
> +///     Ok(None) => { /* everything ok */ },
> +///     Ok(Some(cert)) => { /* do something with cert */ },
> +///     Err(err) => { /* do something with error */ },
> +/// }
> +/// # }
> +/// ```
> +pub async fn probe_tls_connection(
> +    remote_type: RemoteType,
> +    hostname: String,
> +    fingerprint: Option<String>,
> +) -> Result<Option<CertificateInfo>, Error> {
> +    let host_port: Authority = hostname.parse()?;
> +
> +    let uri: http::uri::Uri = format!(
> +        "https://{}:{}",
> +        host_port.host(),
> +        host_port.port_u16().unwrap_or(remote_type.default_port())
> +    )
> +    .parse()?;
> +
> +    // 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(|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
> +    // none is set.
> +    client.set_authentication(proxmox_client::Token {
> +        userid: "".to_string(),
> +        value: "".to_string(),
> +        prefix: "".to_string(),
> +        perl_compat: false,
> +    });
> +
> +    client.request(Method::GET, "/", None::<()>).await?;
> +
> +    let cert = invalid_cert.lock().unwrap().take();
> +    if let Some(cert) = cert {
> +        let cert = cert?;
> +        let cert = cert?;

I think doing a 

let cert = cert??;

or 

Ok(Some(cert??))

is better here. Otherwise it reads a bit like a copy-paste mistake.

> +        Ok(Some(cert))
> +    } else {
> +        Ok(None)
> +    }
> +}



_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel