From aa2ad97a77fcf126bdd7ceb73a0c5fbf936a2abb Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Tue, 28 May 2024 00:28:17 +0000 Subject: [PATCH] fix: adjust custom err from cert-check due to libgit2 1.8 change libgit2 disallows overriding errors from certificate check since v1.8.0, so we store the error additionally and unwrap it later. See https://github.com/libgit2/libgit2/commit/9a9f220119d9647a352867b24b0556195cb26548 --- Cargo.lock | 11 ++++------- Cargo.toml | 6 +++--- src/cargo/sources/git/known_hosts.rs | 23 +++++++++-------------- src/cargo/sources/git/utils.rs | 25 +++++++++++++++++++++---- tests/testsuite/https.rs | 2 +- 5 files changed, 38 insertions(+), 29 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 64dca07aa024..c510a4024966 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1059,8 +1059,7 @@ dependencies = [ [[package]] name = "git2" version = "0.18.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "232e6a7bfe35766bf715e55a88b39a700596c0ccfd88cd3680b4cdb40d66ef70" +source = "git+https://github.com/rust-lang/git2-rs.git?rev=b318ea3d2cbf79eed2eaa4af8ef74d8522d7ce9a#b318ea3d2cbf79eed2eaa4af8ef74d8522d7ce9a" dependencies = [ "bitflags 2.5.0", "libc", @@ -1074,8 +1073,7 @@ dependencies = [ [[package]] name = "git2-curl" version = "0.19.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "78e26b61608c573ffd26fc79061a823aa5147449a1afe1f61679a21e2031f7c3" +source = "git+https://github.com/rust-lang/git2-rs.git?rev=b318ea3d2cbf79eed2eaa4af8ef74d8522d7ce9a#b318ea3d2cbf79eed2eaa4af8ef74d8522d7ce9a" dependencies = [ "curl", "git2", @@ -2144,9 +2142,8 @@ dependencies = [ [[package]] name = "libgit2-sys" -version = "0.16.2+1.7.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ee4126d8b4ee5c9d9ea891dd875cfdc1e9d0950437179104b183d7d8a74d24e8" +version = "0.17.0+1.8.1" +source = "git+https://github.com/rust-lang/git2-rs.git?rev=b318ea3d2cbf79eed2eaa4af8ef74d8522d7ce9a#b318ea3d2cbf79eed2eaa4af8ef74d8522d7ce9a" dependencies = [ "cc", "libc", diff --git a/Cargo.toml b/Cargo.toml index 3ce3a6701f95..88b13379e096 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -44,8 +44,8 @@ curl = "0.4.46" curl-sys = "0.4.72" filetime = "0.2.23" flate2 = { version = "1.0.30", default-features = false, features = ["zlib"] } -git2 = "0.18.3" -git2-curl = "0.19.0" +git2 = { git = "https://github.com/rust-lang/git2-rs.git", rev = "b318ea3d2cbf79eed2eaa4af8ef74d8522d7ce9a" } +git2-curl = { git = "https://github.com/rust-lang/git2-rs.git", rev = "b318ea3d2cbf79eed2eaa4af8ef74d8522d7ce9a" } gix = { version = "0.63.0", default-features = false, features = ["blocking-http-transport-curl", "progress-tree", "revision", "parallel", "dirwalk"] } glob = "0.3.1" handlebars = { version = "5.1.2", features = ["dir_source"] } @@ -61,7 +61,7 @@ itertools = "0.12.1" jobserver = "0.1.31" lazycell = "1.3.0" libc = "0.2.154" -libgit2-sys = "0.16.2" +libgit2-sys = { git = "https://github.com/rust-lang/git2-rs.git", rev = "b318ea3d2cbf79eed2eaa4af8ef74d8522d7ce9a" } libloading = "0.8.3" memchr = "2.7.2" miow = "0.6.0" diff --git a/src/cargo/sources/git/known_hosts.rs b/src/cargo/sources/git/known_hosts.rs index 8ce081194b0e..25bcf478bd14 100644 --- a/src/cargo/sources/git/known_hosts.rs +++ b/src/cargo/sources/git/known_hosts.rs @@ -23,6 +23,7 @@ //! and revoked markers. See "FIXME" comments littered in this file. use crate::util::context::{Definition, GlobalContext, Value}; +use crate::CargoResult; use base64::engine::general_purpose::STANDARD; use base64::engine::general_purpose::STANDARD_NO_PAD; use base64::Engine as _; @@ -137,7 +138,7 @@ pub fn certificate_check( port: Option, config_known_hosts: Option<&Vec>>, diagnostic_home_config: &str, -) -> Result { +) -> CargoResult { let Some(host_key) = cert.as_hostkey() else { // Return passthrough for TLS X509 certificates to use whatever validation // was done in git2. @@ -150,13 +151,12 @@ pub fn certificate_check( _ => host.to_string(), }; // The error message must be constructed as a string to pass through the libgit2 C API. - let err_msg = match check_ssh_known_hosts(gctx, host_key, &host_maybe_port, config_known_hosts) - { + match check_ssh_known_hosts(gctx, host_key, &host_maybe_port, config_known_hosts) { Ok(()) => { return Ok(CertificateCheckStatus::CertificateOk); } Err(KnownHostError::CheckError(e)) => { - format!("error: failed to validate host key:\n{:#}", e) + anyhow::bail!("error: failed to validate host key:\n{:#}", e) } Err(KnownHostError::HostKeyNotFound { hostname, @@ -193,7 +193,7 @@ pub fn certificate_check( } msg }; - format!("error: unknown SSH host key\n\ + anyhow::bail!("error: unknown SSH host key\n\ The SSH host key for `{hostname}` is not known and cannot be validated.\n\ \n\ To resolve this issue, add the host key to {known_hosts_location}\n\ @@ -242,7 +242,7 @@ pub fn certificate_check( ) } }; - format!("error: SSH host key has changed for `{hostname}`\n\ + anyhow::bail!("error: SSH host key has changed for `{hostname}`\n\ *********************************\n\ * WARNING: HOST KEY HAS CHANGED *\n\ *********************************\n\ @@ -274,7 +274,7 @@ pub fn certificate_check( location, }) => { let key_type_short_name = key_type.short_name(); - format!( + anyhow::bail!( "error: Key has been revoked for `{hostname}`\n\ **************************************\n\ * WARNING: REVOKED HOST KEY DETECTED *\n\ @@ -288,7 +288,7 @@ pub fn certificate_check( ) } Err(KnownHostError::HostHasOnlyCertAuthority { hostname, location }) => { - format!("error: Found a `@cert-authority` marker for `{hostname}`\n\ + anyhow::bail!("error: Found a `@cert-authority` marker for `{hostname}`\n\ \n\ Cargo doesn't support certificate authorities for host key verification. It is\n\ recommended that the command line Git client is used instead. This can be achieved\n\ @@ -300,12 +300,7 @@ pub fn certificate_check( for more information.\n\ ") } - }; - Err(git2::Error::new( - git2::ErrorCode::GenericError, - git2::ErrorClass::Callback, - err_msg, - )) + } } /// Checks if the given host/host key pair is known. diff --git a/src/cargo/sources/git/utils.rs b/src/cargo/sources/git/utils.rs index 6117266559a4..5ade00902edd 100644 --- a/src/cargo/sources/git/utils.rs +++ b/src/cargo/sources/git/utils.rs @@ -831,7 +831,10 @@ pub fn with_fetch_options( let config_known_hosts = ssh_config.and_then(|ssh| ssh.known_hosts.as_ref()); let diagnostic_home_config = gctx.diagnostic_home_config(); network::retry::with_retry(gctx, || { - with_authentication(gctx, url, git_config, |f| { + // Hack: libgit2 disallows overriding the error from check_cb since v1.8.0, + // so we store the error additionally and unwrap it later + let mut check_cb_result = Ok(()); + let auth_result = with_authentication(gctx, url, git_config, |f| { let port = Url::parse(url).ok().and_then(|url| url.port()); let mut last_update = Instant::now(); let mut rcb = git2::RemoteCallbacks::new(); @@ -840,14 +843,24 @@ pub fn with_fetch_options( let mut counter = MetricsCounter::<10>::new(0, last_update); rcb.credentials(f); rcb.certificate_check(|cert, host| { - super::known_hosts::certificate_check( + match super::known_hosts::certificate_check( gctx, cert, host, port, config_known_hosts, &diagnostic_home_config, - ) + ) { + Ok(status) => Ok(status), + Err(e) => { + check_cb_result = Err(e); + // This is not really used because it'll be overridden by libgit2 + // See https://github.com/libgit2/libgit2/commit/9a9f220119d9647a352867b24b0556195cb26548 + Err(git2::Error::from_str( + "invalid or unknown remote ssh hostkey", + )) + } + } }); rcb.transfer_progress(|stats| { let indexed_deltas = stats.indexed_deltas(); @@ -889,7 +902,11 @@ pub fn with_fetch_options( let mut opts = git2::FetchOptions::new(); opts.remote_callbacks(rcb); cb(opts) - })?; + }); + if auth_result.is_err() { + check_cb_result?; + } + auth_result?; Ok(()) }) } diff --git a/tests/testsuite/https.rs b/tests/testsuite/https.rs index e87ea6bec9ac..8d83f9163ab5 100644 --- a/tests/testsuite/https.rs +++ b/tests/testsuite/https.rs @@ -33,7 +33,7 @@ fn self_signed_should_fail() { let err_msg = if cfg!(target_os = "macos") { "untrusted connection error; class=Ssl (16); code=Certificate (-17)" } else if cfg!(unix) { - "the SSL certificate is invalid; class=Ssl (16); code=Certificate (-17)" + "the SSL certificate is invalid; class=Ssl (16)" } else if cfg!(windows) { "user cancelled certificate check; class=Http (34); code=Certificate (-17)" } else {