Re: [PATCH proxmox v3 1/6] http: factor out openssl verification callback

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

On Wed Jun 17, 2026 at 10:59 AM CEST, Dominik Csapak wrote:
> with the 'tls' feature offers a callback method that can be used within
> openssl's `set_verify_callback` with a given expected fingerprint.
>
> The logic is inspired by our perl and proxmox-websocket-tunnel
> verification logic:
>
> Use openssl's verification if no fingerprint is pinned. If a fingerprint
> is given, ignore openssl's verification and check if the leafs
> certificate is a match.
>
> This introduces a custom error type for this, since we need to handle
> errors differently for different users, e.g. pbs-client wants to be able
> to use a fingerprint cache and let the user accept it in interactive cli
> sessions. For this we want the 'thiserror' crate, so move it to the
> workspace Cargo.toml and depend from there. (also change this for
> proxmox-openid)
>
> One thing to note here is that  the APPLICATION_VERIFICATION error of
> openssl is used to mark the case where an untrusted root or intermediate
> certificate is trusted from the callback. When that happens, openssl
> might return true for the following certificates (if nothing else is
> wrong aside from a missing trust anchor), so the error is checked for
> this special value to determine if the openssl validation can be
> trusted.
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---

-->8 snip 8<--

> +/// Intended as an openssl verification callback.
> +///
> +/// The following things are checked:
> +///
> +/// * If no fingerprint is given, return the openssl verification result
> +/// * If a fingerprint is given, do:
> +///     * Ignore all non-leaf certificates/
> +pub fn openssl_verify_callback(
> +    openssl_valid: bool,
> +    ctx: &mut X509StoreContextRef,
> +    expected_fp: Option<&str>,
> +) -> Result<(), SslVerifyError> {
> +    let trust_openssl = ctx.error() != X509VerifyResult::APPLICATION_VERIFICATION;
> +    if expected_fp.is_none() && openssl_valid && trust_openssl {

i think this check could be simplified to

    if expected_fp.is_none() {
        return openssl_valid;
    }

checking `trust_openssl` only makes sense in a scenario where a chain is
being validated and we told OpenSSL to trust an otherwise untrusted
certificate so we can check the fingerprint of a certificate further
down the chain. so unless a fingerprint is added while verifying a
certificate chain, the extra checks just make this code harder to
understand. to my knowledge, this scenario can't happen.

or am i missing something here?

> +        return Ok(());
> +    }
> +
> +    let cert = match ctx.current_cert() {
> +        Some(cert) => cert,
> +        None => {
> +            return Err(SslVerifyError::NoCertificate);
> +        }
> +    };
> +
> +    if ctx.error_depth() > 0 {
> +        // openssl was not valid, but we want to continue, so save that we don't trust openssl
> +        ctx.set_error(X509VerifyResult::APPLICATION_VERIFICATION);
> +        return Ok(());
> +    }
> +
> +    let digest = cert
> +        .digest(openssl::hash::MessageDigest::sha256())
> +        .map_err(SslVerifyError::InvalidFingerprint)?;
> +    let fingerprint = get_fingerprint_from_u8(&digest);

imo, we should think about adding the `Fingerprint` type i already moved
from pdm/cli/client to the common pdm api-types in another series [1] to
either this crate or another crate that handles tls
(proxmox-acme-api/proxmox-client) and then re-use it here. then we don't
have to do the `to_lowercase()` dance manually and it would be more
efficient to use the actual fingerprint bytes instead of a string
representation.

the callback should then also take `Option<Fingerprint>` instead of an
`Option<&str>`.

[1]: https://lore.proxmox.com/pdm-devel/20260611120327.257523-9-s.sterz@proxmox.com/

-->8 snip 8<--