public inbox for pdm-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pdm-devel] [PATCH proxmox] client: handle tls verification errors more gracefully
@ 2025-12-02 11:22 Shannon Sterz
  2025-12-03 10:53 ` [pdm-devel] applied: " Lukas Wagner
  2025-12-03 12:22 ` [pdm-devel] " Thomas Lamprecht
  0 siblings, 2 replies; 3+ messages in thread
From: Shannon Sterz @ 2025-12-02 11:22 UTC (permalink / raw)
  To: pdm-devel

this makes it so that errors that previously would simply yield a
"client error (Connect)" now show a more useful error message, such
as:

Could not establish a TLS connection. Check whether the fingerprint
matches or the certificate is valid. OpenSSL Error: error:0A000086:SSL
routines:tls_post_process_server_certificate:certificate verify
failed:../ssl/statem/statem_clnt.c:2123:

this should help users figure out what's really happened more easily.

Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
there might also be one or two bugs hiding in how the client handles
certificate validation. imo setting a fingerprint should override
certificate validity and not the other way round. as it is somewhat akin
to pinning a certificate.

also as fabian pointed out in an off list discussion, the use of
`current_cert` in `verify_fingerprint` may be wrong as it could yield a
parent certificate if it is already unverifiable by openssl. this one
would need some more investigation, though.

left both of these as-is, as that seems like some bigger changes for
right now.

 proxmox-client/src/client.rs | 42 ++++++++++++++++++++++++++++--------
 1 file changed, 33 insertions(+), 9 deletions(-)

diff --git a/proxmox-client/src/client.rs b/proxmox-client/src/client.rs
index da2c5c59..81af5681 100644
--- a/proxmox-client/src/client.rs
+++ b/proxmox-client/src/client.rs
@@ -242,10 +242,22 @@ impl Client {
         }
         .map_err(|err| Error::internal("failed to build request", err))?;

-        let response = client
-            .request(request)
-            .await
-            .map_err(|err| Error::Client(err.into()))?;
+        let response = client.request(request).await.map_err(|err| {
+            for err in err.chain() {
+                if let Some(err) = err.downcast_ref::<openssl::error::ErrorStack>() {
+                    return Error::Client(
+                        format!(
+                            "Could not establish a TLS connection. Check \
+                            whether the fingerprint matches or the certificate is valid. \
+                            OpenSSL Error: {err}"
+                        )
+                        .into(),
+                    );
+                }
+            }
+
+            Error::Client(err.into())
+        })?;

         if response.status() == StatusCode::UNAUTHORIZED {
             return Err(Error::Unauthorized);
@@ -352,11 +364,23 @@ impl Client {
             .body(request.body.into())
             .map_err(|err| Error::internal("error building login http request", err))?;

-        let api_response = self
-            .client
-            .request(request)
-            .await
-            .map_err(|err| Error::Client(err.into()))?;
+        let api_response = self.client.request(request).await.map_err(|err| {
+            for err in err.chain() {
+                if let Some(err) = err.downcast_ref::<openssl::error::ErrorStack>() {
+                    return Error::Client(
+                        format!(
+                            "Could not establish a TLS connection. Check \
+                            whether the fingerprint matches or the certificate is valid. \
+                            OpenSSL Error: {err}"
+                        )
+                        .into(),
+                    );
+                }
+            }
+
+            Error::Client(err.into())
+        })?;
+
         if !api_response.status().is_success() {
             return Err(Error::api(api_response.status(), "authentication failed"));
         }
--
2.47.3



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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* [pdm-devel] applied: [PATCH proxmox] client: handle tls verification errors more gracefully
  2025-12-02 11:22 [pdm-devel] [PATCH proxmox] client: handle tls verification errors more gracefully Shannon Sterz
@ 2025-12-03 10:53 ` Lukas Wagner
  2025-12-03 12:22 ` [pdm-devel] " Thomas Lamprecht
  1 sibling, 0 replies; 3+ messages in thread
From: Lukas Wagner @ 2025-12-03 10:53 UTC (permalink / raw)
  To: Proxmox Datacenter Manager development discussion, Shannon Sterz

On Tue Dec 2, 2025 at 12:22 PM CET, Shannon Sterz wrote:
> this makes it so that errors that previously would simply yield a
> "client error (Connect)" now show a more useful error message, such
> as:
>
> Could not establish a TLS connection. Check whether the fingerprint
> matches or the certificate is valid. OpenSSL Error: error:0A000086:SSL
> routines:tls_post_process_server_certificate:certificate verify
> failed:../ssl/statem/statem_clnt.c:2123:
>
> this should help users figure out what's really happened more easily.
>
> Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
> ---
> there might also be one or two bugs hiding in how the client handles
> certificate validation. imo setting a fingerprint should override
> certificate validity and not the other way round. as it is somewhat akin
> to pinning a certificate.
>
> also as fabian pointed out in an off list discussion, the use of
> `current_cert` in `verify_fingerprint` may be wrong as it could yield a
> parent certificate if it is already unverifiable by openssl. this one
> would need some more investigation, though.
>
> left both of these as-is, as that seems like some bigger changes for
> right now.
>

Just for the record, this was already applied [1].

[1] https://git.proxmox.com/?p=proxmox.git;a=commitdiff;h=0d96b40f3d13653f44d2e56d887bef2b123c4b92


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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [pdm-devel] [PATCH proxmox] client: handle tls verification errors more gracefully
  2025-12-02 11:22 [pdm-devel] [PATCH proxmox] client: handle tls verification errors more gracefully Shannon Sterz
  2025-12-03 10:53 ` [pdm-devel] applied: " Lukas Wagner
@ 2025-12-03 12:22 ` Thomas Lamprecht
  1 sibling, 0 replies; 3+ messages in thread
From: Thomas Lamprecht @ 2025-12-03 12:22 UTC (permalink / raw)
  To: pdm-devel, Shannon Sterz

On Tue, 02 Dec 2025 12:22:36 +0100, Shannon Sterz wrote:
> this makes it so that errors that previously would simply yield a
> "client error (Connect)" now show a more useful error message, such
> as:
> 
> Could not establish a TLS connection. Check whether the fingerprint
> matches or the certificate is valid. OpenSSL Error: error:0A000086:SSL
> routines:tls_post_process_server_certificate:certificate verify
> failed:../ssl/statem/statem_clnt.c:2123:
> 
> [...]

Applied, thanks!

[1/1] client: handle tls verification errors more gracefully
      commit: 0d96b40f3d13653f44d2e56d887bef2b123c4b92


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


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2025-12-03 12:22 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-12-02 11:22 [pdm-devel] [PATCH proxmox] client: handle tls verification errors more gracefully Shannon Sterz
2025-12-03 10:53 ` [pdm-devel] applied: " Lukas Wagner
2025-12-03 12:22 ` [pdm-devel] " Thomas Lamprecht

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal