[pbs-devel] [PATCH proxmox-backup 1/4] fix #3391: improve mismatched fingerprint handling

[pbs-devel] [PATCH proxmox-backup 1/4] fix #3391: improve mismatched fingerprint handling

[Fabian Grünbichler]

if the expected fingerprint and the one returned by the server don't
match, print a warning and allow confirmation and proceeding if running
interactive.

previous:

$ proxmox-backup-client ...
Error: error trying to connect: error:1416F086:SSL routines:tls_process_server_certificate:certificate verify failed:../ssl/statem/statem_clnt.c:1915:

new:

$ proxmox-backup-client ...
WARNING: certificate fingerprint does not match expected fingerprint!
expected:    ac:cb:6a:bc:d6:b7:b4:77:3e:17:05:d6:b6:29:dd:1f:05:9c:2b:3a:df:84:3b:4d:f9:06:2c:be:da:06:52:12
fingerprint: ab:cb:6a:bc:d6:b7:b4:77:3e:17:05:d6:b6:29:dd:1f:05:9c:2b:3a:df:84:3b:4d:f9:06:2c:be:da:06:52:12
Are you sure you want to continue connecting? (y/n): n
Error: error trying to connect: error:1416F086:SSL routines:tls_process_server_certificate:certificate verify failed:../ssl/statem/statem_clnt.c:1915:

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/client/http_client.rs | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/client/http_client.rs b/src/client/http_client.rs
index c8d1f743..1df22dc9 100644
--- a/src/client/http_client.rs
+++ b/src/client/http_client.rs
@@ -498,10 +498,12 @@ impl HttpClient {
             .collect::<Vec<&str>>().join(":");
 
         if let Some(expected_fingerprint) = expected_fingerprint {
-            if expected_fingerprint.to_lowercase() == fp_string {
+            let expected_fingerprint = expected_fingerprint.to_lowercase();
+            if expected_fingerprint == fp_string {
                 return (true, Some(fp_string));
             } else {
-                return (false, None);
+                eprintln!("WARNING: certificate fingerprint does not match expected fingerprint!");
+                eprintln!("expected:    {}", expected_fingerprint);
             }
         }
 
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 2/4] client: improve fingerprint variable names

[Fabian Grünbichler]

and pass as reference instead of cloning.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/client/http_client.rs | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/client/http_client.rs b/src/client/http_client.rs
index 1df22dc9..59162c76 100644
--- a/src/client/http_client.rs
+++ b/src/client/http_client.rs
@@ -298,13 +298,13 @@ impl HttpClient {
 
         let verified_fingerprint = Arc::new(Mutex::new(None));
 
-        let mut fingerprint = options.fingerprint.take();
+        let mut expected_fingerprint = options.fingerprint.take();
 
-        if fingerprint.is_some() {
+        if expected_fingerprint.is_some() {
             // do not store fingerprints passed via options in cache
             options.fingerprint_cache = false;
         } else if options.fingerprint_cache && options.prefix.is_some() {
-            fingerprint = load_fingerprint(options.prefix.as_ref().unwrap(), server);
+            expected_fingerprint = load_fingerprint(options.prefix.as_ref().unwrap(), server);
         }
 
         let mut ssl_connector_builder = SslConnector::builder(SslMethod::tls()).unwrap();
@@ -316,7 +316,7 @@ impl HttpClient {
             let fingerprint_cache = options.fingerprint_cache;
             let prefix = options.prefix.clone();
             ssl_connector_builder.set_verify_callback(openssl::ssl::SslVerifyMode::PEER, move |valid, ctx| {
-                let (valid, fingerprint) = Self::verify_callback(valid, ctx, fingerprint.clone(), interactive);
+                let (valid, fingerprint) = Self::verify_callback(valid, ctx, expected_fingerprint.as_ref(), interactive);
                 if valid {
                     if let Some(fingerprint) = fingerprint {
                         if fingerprint_cache && prefix.is_some() {
@@ -474,9 +474,9 @@ impl HttpClient {
     }
 
     fn verify_callback(
-        valid: bool, ctx:
-        &mut X509StoreContextRef,
-        expected_fingerprint: Option<String>,
+        valid: bool,
+        ctx: &mut X509StoreContextRef,
+        expected_fingerprint: Option<&String>,
         interactive: bool,
     ) -> (bool, Option<String>) {
         if valid { return (true, None); }
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 3/4] client: refactor verification callback

[Fabian Grünbichler]

return a result with optional fingerprint instead of tuple, allowing
easy extraction of a meaningful error message.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/client/http_client.rs | 40 +++++++++++++++++++++++----------------
 1 file changed, 24 insertions(+), 16 deletions(-)

diff --git a/src/client/http_client.rs b/src/client/http_client.rs
index 59162c76..5ba82468 100644
--- a/src/client/http_client.rs
+++ b/src/client/http_client.rs
@@ -316,9 +316,9 @@ impl HttpClient {
             let fingerprint_cache = options.fingerprint_cache;
             let prefix = options.prefix.clone();
             ssl_connector_builder.set_verify_callback(openssl::ssl::SslVerifyMode::PEER, move |valid, ctx| {
-                let (valid, fingerprint) = Self::verify_callback(valid, ctx, expected_fingerprint.as_ref(), interactive);
-                if valid {
-                    if let Some(fingerprint) = fingerprint {
+                match Self::verify_callback(valid, ctx, expected_fingerprint.as_ref(), interactive) {
+                    Ok(None) => true,
+                    Ok(Some(fingerprint)) => {
                         if fingerprint_cache && prefix.is_some() {
                             if let Err(err) = store_fingerprint(
                                 prefix.as_ref().unwrap(), &server, &fingerprint) {
@@ -326,9 +326,13 @@ impl HttpClient {
                             }
                         }
                         *verified_fingerprint.lock().unwrap() = Some(fingerprint);
-                    }
+                        true
+                    },
+                    Err(err) => {
+                        eprintln!("certificate validation failed - {}", err);
+                        false
+                    },
                 }
-                valid
             });
         } else {
             ssl_connector_builder.set_verify(openssl::ssl::SslVerifyMode::NONE);
@@ -474,24 +478,27 @@ impl HttpClient {
     }
 
     fn verify_callback(
-        valid: bool,
+        openssl_valid: bool,
         ctx: &mut X509StoreContextRef,
         expected_fingerprint: Option<&String>,
         interactive: bool,
-    ) -> (bool, Option<String>) {
-        if valid { return (true, None); }
+    ) -> Result<Option<String>, Error> {
+
+        if openssl_valid {
+            return Ok(None);
+        }
 
         let cert = match ctx.current_cert() {
             Some(cert) => cert,
-            None => return (false, None),
+            None => bail!("context lacks current certificate."),
         };
 
         let depth = ctx.error_depth();
-        if depth != 0 { return (false, None); }
+        if depth != 0 { bail!("context depth != 0") }
 
         let fp = match cert.digest(openssl::hash::MessageDigest::sha256()) {
             Ok(fp) => fp,
-            Err(_) => return (false, None), // should not happen
+            Err(err) => bail!("failed to calculate certificate FP - {}", err), // should not happen
         };
         let fp_string = proxmox::tools::digest_to_hex(&fp);
         let fp_string = fp_string.as_bytes().chunks(2).map(|v| std::str::from_utf8(v).unwrap())
@@ -500,7 +507,7 @@ impl HttpClient {
         if let Some(expected_fingerprint) = expected_fingerprint {
             let expected_fingerprint = expected_fingerprint.to_lowercase();
             if expected_fingerprint == fp_string {
-                return (true, Some(fp_string));
+                return Ok(Some(fp_string));
             } else {
                 eprintln!("WARNING: certificate fingerprint does not match expected fingerprint!");
                 eprintln!("expected:    {}", expected_fingerprint);
@@ -519,18 +526,19 @@ impl HttpClient {
                     Ok(_) => {
                         let trimmed = line.trim();
                         if trimmed == "y" || trimmed == "Y" {
-                            return (true, Some(fp_string));
+                            return Ok(Some(fp_string));
                         } else if trimmed == "n" || trimmed == "N" {
-                            return (false, None);
+                            bail!("Certificate fingerprint was not confirmed.");
                         } else {
                             continue;
                         }
                     }
-                    Err(_) => return (false, None),
+                    Err(err) => bail!("Certificate fingerprint was not confirmed - {}.", err),
                 }
             }
         }
-        (false, None)
+
+        bail!("Certificate fingerprint was not confirmed.");
     }
 
     pub async fn request(&self, mut req: Request<Body>) -> Result<Value, Error> {
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 4/4] client: use stderr for all fingerprint confirm msgs

[Fabian Grünbichler]

an interactive client might still want machine-readable output on
stdout.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/client/http_client.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/client/http_client.rs b/src/client/http_client.rs
index 5ba82468..7fe33bcc 100644
--- a/src/client/http_client.rs
+++ b/src/client/http_client.rs
@@ -516,9 +516,9 @@ impl HttpClient {
 
         // If we're on a TTY, query the user
         if interactive && tty::stdin_isatty() {
-            println!("fingerprint: {}", fp_string);
+            eprintln!("fingerprint: {}", fp_string);
             loop {
-                print!("Are you sure you want to continue connecting? (y/n): ");
+                eprint!("Are you sure you want to continue connecting? (y/n): ");
                 let _ = std::io::stdout().flush();
                 use std::io::{BufRead, BufReader};
                 let mut line = String::new();
-- 
2.20.1

[pbs-devel] applied-series: [PATCH proxmox-backup 1/4] fix #3391: improve mismatched fingerprint handling

[Thomas Lamprecht]

On 10.05.21 10:52, Fabian Grünbichler wrote:
> if the expected fingerprint and the one returned by the server don't
> match, print a warning and allow confirmation and proceeding if running
> interactive.
> 
> previous:
> 
> $ proxmox-backup-client ...
> Error: error trying to connect: error:1416F086:SSL routines:tls_process_server_certificate:certificate verify failed:../ssl/statem/statem_clnt.c:1915:
> 
> new:
> 
> $ proxmox-backup-client ...
> WARNING: certificate fingerprint does not match expected fingerprint!
> expected:    ac:cb:6a:bc:d6:b7:b4:77:3e:17:05:d6:b6:29:dd:1f:05:9c:2b:3a:df:84:3b:4d:f9:06:2c:be:da:06:52:12
> fingerprint: ab:cb:6a:bc:d6:b7:b4:77:3e:17:05:d6:b6:29:dd:1f:05:9c:2b:3a:df:84:3b:4d:f9:06:2c:be:da:06:52:12
> Are you sure you want to continue connecting? (y/n): n
> Error: error trying to connect: error:1416F086:SSL routines:tls_process_server_certificate:certificate verify failed:../ssl/statem/statem_clnt.c:1915:
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
>  src/client/http_client.rs | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
>

applied all four patches, thanks!