Re: [pdm-devel] [PATCH datacenter-manager 04/21] server/ui: pve api: extend 'scan' so it can probe the tls connection

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

Hi,

some notes inline.

On Fri May 16, 2025 at 3:35 PM CEST, Dominik Csapak wrote:
> Makes the `authid` and `token` parameters optional. If they're omitted,
> opens a basic connection to probe the TLS state. If no fingerprint was
> given and the certificate is not trusted, the certificate information is
> returned (so it can be shown to the user)
>
> Adapt the UI, so it can cope with that change.
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  lib/pdm-api-types/Cargo.toml       |  1 +
>  lib/pdm-api-types/src/lib.rs       |  2 +
>  lib/pdm-api-types/src/remotes.rs   |  7 ++++
>  server/src/api/pve/mod.rs          | 60 ++++++++++++++++++++----------
>  ui/src/remotes/wizard_page_info.rs |  9 +++--
>  5 files changed, 57 insertions(+), 22 deletions(-)
>
> diff --git a/lib/pdm-api-types/Cargo.toml b/lib/pdm-api-types/Cargo.toml
> index 6575c03..fbaf5b3 100644
> --- a/lib/pdm-api-types/Cargo.toml
> +++ b/lib/pdm-api-types/Cargo.toml
> @@ -13,6 +13,7 @@ regex.workspace = true
>  serde.workspace = true
>  serde_plain.workspace = true
>  
> +proxmox-acme-api = { workspace = true, features = [] }
>  proxmox-auth-api = { workspace = true, features = ["api-types"] }
>  proxmox-lang.workspace = true
>  proxmox-config-digest.workspace = true
> diff --git a/lib/pdm-api-types/src/lib.rs b/lib/pdm-api-types/src/lib.rs
> index 3844907..31420a9 100644
> --- a/lib/pdm-api-types/src/lib.rs
> +++ b/lib/pdm-api-types/src/lib.rs
> @@ -79,6 +79,8 @@ pub use proxmox_dns_api::THIRD_DNS_SERVER_SCHEMA;
>  pub use proxmox_config_digest::ConfigDigest;
>  pub use proxmox_config_digest::PROXMOX_CONFIG_DIGEST_SCHEMA;
>  
> +pub use proxmox_acme_api::CertificateInfo;
> +
>  #[macro_use]
>  mod user;
>  pub use user::*;
> diff --git a/lib/pdm-api-types/src/remotes.rs b/lib/pdm-api-types/src/remotes.rs
> index dca2fa0..fd20327 100644
> --- a/lib/pdm-api-types/src/remotes.rs
> +++ b/lib/pdm-api-types/src/remotes.rs
> @@ -179,3 +179,10 @@ mod serde_option_uri {
>          }
>      }
>  }
> +
> +#[allow(clippy::large_enum_variant)]
> +#[derive(Clone, PartialEq, Deserialize, Serialize)]
> +pub enum ScanResult {
> +    TlsResult(Option<proxmox_acme_api::CertificateInfo>),
> +    Remote(Remote),
> +}

These enum variant names are a bit hard to understand ("what does
TlsResult mean?"). Maybe something like 

pub enum ScanResult {
    UntrustedCertificate(CertInfo),
    TrustedCertificate,
    ScanFinished(Remote)
}

 (the last one would be left out if you implement my other suggestion
 below)

 Also some doc-comments would be nice!

> diff --git a/server/src/api/pve/mod.rs b/server/src/api/pve/mod.rs
> index 1a3a725..810a38e 100644
> --- a/server/src/api/pve/mod.rs
> +++ b/server/src/api/pve/mod.rs
> @@ -13,7 +13,7 @@ use proxmox_schema::property_string::PropertyString;
>  use proxmox_section_config::typed::SectionConfigData;
>  use proxmox_sortable_macro::sortable;
>  
> -use pdm_api_types::remotes::{NodeUrl, Remote, RemoteType, REMOTE_ID_SCHEMA};
> +use pdm_api_types::remotes::{NodeUrl, Remote, RemoteType, ScanResult, REMOTE_ID_SCHEMA};
>  use pdm_api_types::resource::PveResource;
>  use pdm_api_types::{
>      Authid, RemoteUpid, HOST_OPTIONAL_PORT_FORMAT, PRIV_RESOURCE_AUDIT, PRIV_RESOURCE_DELETE,
> @@ -27,8 +27,8 @@ use pve_api_types::{ClusterResourceKind, ClusterResourceType};
>  
>  use super::resources::{map_pve_lxc, map_pve_node, map_pve_qemu, map_pve_storage};
>  
> -use crate::connection;
>  use crate::connection::PveClient;
> +use crate::connection::{self, probe_tls_connection};
>  use crate::remote_tasks;
>  
>  mod lxc;
> @@ -314,9 +314,11 @@ fn check_guest_delete_perms(
>              },
>              "authid": {
>                  type: Authid,
> +                optional: true,
>              },
>              "token": {
>                  type: String,
> +                optional: true,
>                  description: "The token secret or the user password.",
>              },
>          },
> @@ -326,13 +328,28 @@ fn check_guest_delete_perms(
>              &Permission::Privilege(&["/"], PRIV_SYS_MODIFY, false),
>      },
>  )]
> -/// Scans the given connection info for pve cluster information
> +/// Scans the given connection info for tls or pve cluster information
> +///
> +/// Returns the result for tls connection if only hostname (and optionally fingerprint) is given.
> +/// If authid and token are also provided, returns pve cluster information.
> +///
> +/// For each node that is returned, the TLS connection is probed, to check if using
> +/// a fingerprint is necessary.
>  pub async fn scan_remote_pve(
>      hostname: String,
>      fingerprint: Option<String>,
> -    authid: Authid,
> -    token: String,
> -) -> Result<Remote, Error> {
> +    authid: Option<Authid>,
> +    token: Option<String>,
> +) -> Result<ScanResult, Error> {
> +    let (authid, token) = match (authid, token) {
> +        (Some(authid), Some(token)) => (authid, token),
> +        _ => {
> +            let res = probe_tls_connection(RemoteType::Pve, hostname.clone(), fingerprint.clone())
> +                .await?;
> +            return Ok(ScanResult::TlsResult(res));
> +        }
> +    };
> +
>      let mut remote = Remote {
>          ty: RemoteType::Pve,
>          id: String::new(),
> @@ -349,18 +366,23 @@ pub async fn scan_remote_pve(
>          .await
>          .map_err(|err| format_err!("could not login: {err}"))?;
>  
> -    let nodes: Vec<_> = client
> -        .list_nodes()
> -        .await?
> -        .into_iter()
> -        .map(|node| {
> -            let url = NodeUrl {
> -                hostname: node.node,
> -                fingerprint: node.ssl_fingerprint,
> -            };
> -            PropertyString::new(url)
> -        })
> -        .collect();
> +    let mut nodes = Vec::new();
> +
> +    for node in client.list_nodes().await? {
> +        // probe without fingerprint to see if the certificate is trusted
> +        // TODO: how can we get the fqdn here?, otherwise it'll fail in most scenarios...
> +        let fingerprint = match probe_tls_connection(RemoteType::Pve, node.node.clone(), None).await
> +        {
> +            Ok(Some(cert)) => cert.fingerprint,
> +            Ok(None) => None,
> +            Err(_) => node.ssl_fingerprint,
> +        };
> +
> +        nodes.push(PropertyString::new(NodeUrl {
> +            hostname: node.node,
> +            fingerprint,
> +        }));
> +    }
>  
>      if nodes.is_empty() {
>          bail!("no node list returned");
> @@ -383,7 +405,7 @@ pub async fn scan_remote_pve(
>              .unwrap_or_default();
>      }
>  
> -    Ok(remote)
> +    Ok(ScanResult::Remote(remote))
>  }

This function feels a bit off to me. Would it maybe be cleaner to have TLS
probing and 'gathering cluster/node info' as two distinct steps that are
just done one after the other by the Web UI?
Sure, the probing is not needed if the certificate is in fact trusted, but
doing a second API call does not really hurt in this case, or does it?

>  
>  #[api(
> diff --git a/ui/src/remotes/wizard_page_info.rs b/ui/src/remotes/wizard_page_info.rs
> index b0a5ba2..9953f42 100644
> --- a/ui/src/remotes/wizard_page_info.rs
> +++ b/ui/src/remotes/wizard_page_info.rs
> @@ -1,6 +1,6 @@
>  use std::rc::Rc;
>  
> -use anyhow::Error;
> +use anyhow::{bail, Error};
>  use html::IntoEventCallback;
>  use proxmox_schema::property_string::PropertyString;
>  use serde::{Deserialize, Serialize};
> @@ -18,7 +18,7 @@ use pwt::{
>      AsyncPool,
>  };
>  
> -use pdm_api_types::remotes::{NodeUrl, Remote};
> +use pdm_api_types::remotes::{NodeUrl, Remote, ScanResult};
>  
>  use pwt_macros::builder;
>  
> @@ -97,7 +97,10 @@ async fn scan(connection_params: ConnectParams, form_ctx: FormContext) -> Result
>      let data: ScanParams = serde_json::from_value(data.clone())?;
>  
>      let params = serde_json::to_value(&data)?;
> -    let mut result: Remote = proxmox_yew_comp::http_post("/pve/scan", Some(params)).await?;
> +    let mut result = match proxmox_yew_comp::http_post("/pve/scan", Some(params)).await? {
> +        ScanResult::TlsResult(_) => bail!("Untrusted certificate or invalid fingerprint"),
> +        ScanResult::Remote(remote) => remote,
> +    };
>      result.nodes.insert(
>          0,
>          PropertyString::new(NodeUrl {



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