Merge pull request #413 from blyxxyz/better-rustls-errors

Improve rustls errors for invalid certificates

Cargo.lock

@@ -814,9 +814,9 @@ checksum = "df3b46402a9d5adb4c86a0cf463f42e19994e3ee891101b1841f30a545cb49a9"
 
 [[package]]
 name = "humantime"
-version = "2.1.0"
+version = "2.2.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "9a3a5bfb195931eeb336b2a7b4d761daec841b97f947d34394601737a7bba5e4"
+checksum = "9b112acc8b3adf4b107a8ec20977da0273a8c386765a3ec0229bd500a1443f9f"
 
 [[package]]
 name = "hyper"
@@ -1760,9 +1760,9 @@ dependencies = [
 
 [[package]]
 name = "rustls"
-version = "0.23.23"
+version = "0.23.25"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "47796c98c480fce5406ef69d1c76378375492c3b0a0de587be0c1d9feb12f395"
+checksum = "822ee9188ac4ec04a2f0531e55d035fb2de73f18b41a63c70c2712503b6fb13c"
 dependencies = [
  "log",
  "once_cell",
@@ -1805,9 +1805,9 @@ dependencies = [
 
 [[package]]
 name = "rustls-webpki"
-version = "0.102.8"
+version = "0.103.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "64ca1bc8749bd4cf37b5ce386cc146580777b4e8572c7b97baf22c83f444bee9"
+checksum = "0aa4eeac2588ffff23e9d7a7e9b3f971c5fb5b7ebc9452745e0c232c64f83b2f"
 dependencies = [
  "ring",
  "rustls-pki-types",
@@ -2798,6 +2798,7 @@ dependencies = [
  "flate2",
  "form_urlencoded",
  "http-body-util",
+ "humantime",
  "hyper",
  "hyper-util",
  "indicatif",

Cargo.toml

@@ -48,6 +48,7 @@ serde_urlencoded = "0.7.0"
 supports-hyperlinks = "3.0.0"
 termcolor = "1.1.2"
 time = "0.3.16"
+humantime = "2.2.0"
 unicode-width = "0.1.9"
 url = "2.2.2"
 ruzstd = { version = "0.7", default-features = false, features = ["std"]}
@@ -56,7 +57,7 @@ log = "0.4.21"
 
 # Enable logging in transitive dependencies.
 # The rustls version number should be kept in sync with hyper/reqwest.
-rustls = { version = "0.23.14", optional = true, default-features = false, features = ["logging"] }
+rustls = { version = "0.23.25", optional = true, default-features = false, features = ["logging"] }
 tracing = { version = "0.1.41", default-features = false, features = ["log"] }
 reqwest_cookie_store = { version = "0.8.0", features = ["serde"] }
 

src/error_reporting.rs

@@ -0,0 +1,76 @@
+use std::process::ExitCode;
+
+pub(crate) fn additional_messages(err: &anyhow::Error, native_tls: bool) -> Vec<String> {
+    let mut msgs = Vec::new();
+
+    #[cfg(feature = "rustls")]
+    msgs.extend(format_rustls_error(err));
+
+    if native_tls && err.root_cause().to_string() == "invalid minimum TLS version for backend" {
+        msgs.push("Try running without the --native-tls flag.".into());
+    }
+
+    msgs
+}
+
+/// Format certificate expired/not valid yet messages. By default these print
+/// human-unfriendly Unix timestamps.
+///
+/// Other rustls error messages (e.g. wrong host) are readable enough.
+#[cfg(feature = "rustls")]
+fn format_rustls_error(err: &anyhow::Error) -> Option<String> {
+    use humantime::format_duration;
+    use rustls::pki_types::UnixTime;
+    use rustls::CertificateError;
+    use time::OffsetDateTime;
+
+    // Multiple layers of io::Error for some reason?
+    // This may be fragile
+    let err = err.root_cause().downcast_ref::<std::io::Error>()?;
+    let err = err.get_ref()?.downcast_ref::<std::io::Error>()?;
+    let err = err.get_ref()?.downcast_ref::<rustls::Error>()?;
+    let rustls::Error::InvalidCertificate(err) = err else {
+        return None;
+    };
+
+    fn conv_time(unix_time: &UnixTime) -> Option<OffsetDateTime> {
+        OffsetDateTime::from_unix_timestamp(unix_time.as_secs() as i64).ok()
+    }
+
+    match err {
+        CertificateError::ExpiredContext { time, not_after } => {
+            let time = conv_time(time)?;
+            let not_after = conv_time(not_after)?;
+            let diff = format_duration((time - not_after).try_into().ok()?);
+            Some(format!(
+                "Certificate not valid after {not_after} ({diff} ago).",
+            ))
+        }
+        CertificateError::NotValidYetContext { time, not_before } => {
+            let time = conv_time(time)?;
+            let not_before = conv_time(not_before)?;
+            let diff = format_duration((not_before - time).try_into().ok()?);
+            Some(format!(
+                "Certificate not valid before {not_before} ({diff} from now).",
+            ))
+        }
+        _ => None,
+    }
+}
+
+pub(crate) fn exit_code(err: &anyhow::Error) -> ExitCode {
+    if let Some(err) = err.downcast_ref::<reqwest::Error>() {
+        if err.is_timeout() {
+            return ExitCode::from(2);
+        }
+    }
+
+    if err
+        .downcast_ref::<crate::redirect::TooManyRedirects>()
+        .is_some()
+    {
+        return ExitCode::from(6);
+    }
+
+    ExitCode::FAILURE
+}

src/main.rs

@@ -4,6 +4,7 @@ mod buffer;
 mod cli;
 mod decoder;
 mod download;
+mod error_reporting;
 mod formatting;
 mod generation;
 mod middleware;
@@ -22,7 +23,7 @@ use std::fs::File;
 use std::io::{self, IsTerminal, Read, Write as _};
 use std::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr};
 use std::path::PathBuf;
-use std::process;
+use std::process::ExitCode;
 use std::str::FromStr;
 use std::sync::Arc;
 
@@ -61,7 +62,7 @@ fn get_user_agent() -> &'static str {
     }
 }
 
-fn main() {
+fn main() -> ExitCode {
     let args = Cli::parse();
 
     if args.debug {
@@ -77,39 +78,30 @@ fn main() {
     let bin_name = args.bin_name.clone();
 
     match run(args) {
-        Ok(exit_code) => {
-            process::exit(exit_code);
-        }
+        Ok(exit_code) => exit_code,
         Err(err) => {
             log::debug!("{err:#?}");
             eprintln!("{bin_name}: error: {err:?}");
-            let msg = err.root_cause().to_string();
-            if native_tls && msg == "invalid minimum TLS version for backend" {
+
+            for message in error_reporting::additional_messages(&err, native_tls) {
                 eprintln!();
-                eprintln!("Try running without the --native-tls flag.");
+                eprintln!("{message}");
             }
-            if let Some(err) = err.downcast_ref::<reqwest::Error>() {
-                if err.is_timeout() {
-                    process::exit(2);
-                }
-            }
-            if msg.starts_with("Too many redirects") {
-                process::exit(6);
-            }
-            process::exit(1);
+
+            error_reporting::exit_code(&err)
         }
     }
 }
 
-fn run(args: Cli) -> Result<i32> {
+fn run(args: Cli) -> Result<ExitCode> {
     if let Some(generate) = args.generate {
         generation::generate(&args.bin_name, generate);
-        return Ok(0);
+        return Ok(ExitCode::SUCCESS);
     }
 
     if args.curl {
         to_curl::print_curl_translation(args)?;
-        return Ok(0);
+        return Ok(ExitCode::SUCCESS);
     }
 
     let (mut headers, headers_to_unset) = args.request_items.headers()?;
@@ -189,7 +181,7 @@ fn run(args: Cli) -> Result<i32> {
         return Err(anyhow!("This binary was built without native-tls support"));
     }
 
-    let mut exit_code: i32 = 0;
+    let mut failure_code = None;
     let mut resume: Option<u64> = None;
     let mut auth = None;
     let mut save_auth_in_session = true;
@@ -622,16 +614,17 @@ fn run(args: Cli) -> Result<i32> {
 
         let status = response.status();
         if args.check_status.unwrap_or(!args.httpie_compat_mode) {
-            exit_code = match status.as_u16() {
-                300..=399 if !args.follow => 3,
-                400..=499 => 4,
-                500..=599 => 5,
-                _ => 0,
-            };
+            match status.as_u16() {
+                300..=399 if !args.follow => failure_code = Some(ExitCode::from(3)),
+                400..=499 => failure_code = Some(ExitCode::from(4)),
+                500..=599 => failure_code = Some(ExitCode::from(5)),
+                _ => (),
+            }
+
             // Print this if the status code isn't otherwise ending up in the terminal.
             // HTTPie looks at --quiet, since --quiet always suppresses the response
             // headers even if you pass --print=h. But --print takes precedence for us.
-            if exit_code != 0 && (is_output_redirected || !print.response_headers) {
+            if failure_code.is_some() && (is_output_redirected || !print.response_headers) {
                 log::warn!("HTTP {} {}", status.as_u16(), reason_phrase(&response));
             }
         }
@@ -640,7 +633,7 @@ fn run(args: Cli) -> Result<i32> {
             printer.print_response_headers(&response)?;
         }
         if args.download {
-            if exit_code == 0 {
+            if failure_code.is_none() {
                 download_file(
                     response,
                     args.output,
@@ -670,7 +663,7 @@ fn run(args: Cli) -> Result<i32> {
             .with_context(|| format!("couldn't persist session {}", s.path.display()))?;
     }
 
-    Ok(exit_code)
+    Ok(failure_code.unwrap_or(ExitCode::SUCCESS))
 }
 
 /// Configure backtraces for standard panics and anyhow using `$RUST_BACKTRACE`.

src/redirect.rs

@@ -1,4 +1,4 @@
-use anyhow::{anyhow, Result};
+use anyhow::Result;
 use reqwest::blocking::{Request, Response};
 use reqwest::header::{
     HeaderMap, AUTHORIZATION, CONTENT_ENCODING, CONTENT_LENGTH, CONTENT_TYPE, COOKIE, LOCATION,
@@ -31,10 +31,10 @@ impl Middleware for RedirectFollower {
             if remaining_redirects > 0 {
                 remaining_redirects -= 1;
             } else {
-                return Err(anyhow!(
-                    "Too many redirects (--max-redirects={})",
-                    self.max_redirects
-                ));
+                return Err(TooManyRedirects {
+                    max_redirects: self.max_redirects,
+                }
+                .into());
             }
             log::info!("Following redirect to {}", next_request.url());
             log::trace!("Remaining redirects: {}", remaining_redirects);
@@ -48,6 +48,23 @@ impl Middleware for RedirectFollower {
     }
 }
 
+#[derive(Debug)]
+pub(crate) struct TooManyRedirects {
+    max_redirects: usize,
+}
+
+impl std::fmt::Display for TooManyRedirects {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(
+            f,
+            "Too many redirects (--max-redirects={})",
+            self.max_redirects,
+        )
+    }
+}
+
+impl std::error::Error for TooManyRedirects {}
+
 // See https://github.com/seanmonstar/reqwest/blob/bbeb1ede4e8098481c3de6f2cafb8ecca1db4ede/src/async_impl/client.rs#L1500-L1607
 fn get_next_request(mut request: Request, response: &Response) -> Option<Request> {
     let get_next_url = |request: &Request| {

tests/cli.rs

@@ -1138,7 +1138,6 @@ fn proxy_multiple_valid_proxies() {
 
 // temporarily disabled for builds not using rustls
 #[cfg(all(feature = "online-tests", feature = "rustls"))]
-#[ignore = "endpoint is randomly timing out"]
 #[test]
 fn verify_default_yes() {
     use predicates::boolean::PredicateBooleanExt;
@@ -1154,7 +1153,6 @@ fn verify_default_yes() {
 
 // temporarily disabled for builds not using rustls
 #[cfg(all(feature = "online-tests", feature = "rustls"))]
-#[ignore = "endpoint is randomly timing out"]
 #[test]
 fn verify_explicit_yes() {
     use predicates::boolean::PredicateBooleanExt;
@@ -1169,7 +1167,6 @@ fn verify_explicit_yes() {
 }
 
 #[cfg(feature = "online-tests")]
-#[ignore = "endpoint is randomly timing out"]
 #[test]
 fn verify_no() {
     get_command()
@@ -1181,7 +1178,6 @@ fn verify_no() {
 }
 
 #[cfg(all(feature = "rustls", feature = "online-tests"))]
-#[ignore = "endpoint is randomly timing out"]
 #[test]
 fn verify_valid_file() {
     get_command()
@@ -1197,7 +1193,6 @@ fn verify_valid_file() {
 // This test may fail if https://github.com/seanmonstar/reqwest/issues/1260 is fixed
 // If that happens make sure to remove the warning, not just this test
 #[cfg(all(feature = "native-tls", feature = "online-tests"))]
-#[ignore = "endpoint is randomly timing out"]
 #[test]
 fn verify_valid_file_native_tls() {
     get_command()
@@ -1218,6 +1213,16 @@ fn cert_without_key() {
         .stderr(predicates::str::is_empty());
 }
 
+#[cfg(all(feature = "rustls", feature = "online-tests"))]
+#[test]
+fn formatted_certificate_expired_message() {
+    get_command()
+        .arg("https://expired.badssl.com")
+        .assert()
+        .failure()
+        .stderr(contains("Certificate not valid after 2015-04-12"));
+}
+
 #[test]
 fn override_dns_resolution() {
     let server = server::http(|req| async move {