From dfec2813f79cd52727d3e2d8b824b9277b4aae0e Mon Sep 17 00:00:00 2001 From: Abdullah Alyan Date: Wed, 29 Nov 2023 15:30:26 +0300 Subject: [PATCH 1/5] feat!: Add `ssl_verify` field to `client::http::Options`. Currently this option only works in the curl backend. --- gix-transport/src/client/blocking_io/http/curl/remote.rs | 3 +++ gix-transport/src/client/blocking_io/http/mod.rs | 4 ++++ 2 files changed, 7 insertions(+) diff --git a/gix-transport/src/client/blocking_io/http/curl/remote.rs b/gix-transport/src/client/blocking_io/http/curl/remote.rs index e6b0bee03fe..1557f5a9cbd 100644 --- a/gix-transport/src/client/blocking_io/http/curl/remote.rs +++ b/gix-transport/src/client/blocking_io/http/curl/remote.rs @@ -157,6 +157,7 @@ pub fn new() -> ( verbose, ssl_ca_info, ssl_version, + ssl_verify, http_version, backend, }, @@ -194,6 +195,8 @@ pub fn new() -> ( } } + handle.ssl_verify_peer(ssl_verify)?; + if let Some(http_version) = http_version { let version = match http_version { HttpVersion::V1_1 => curl::easy::HttpVersion::V11, diff --git a/gix-transport/src/client/blocking_io/http/mod.rs b/gix-transport/src/client/blocking_io/http/mod.rs index 055b4ea591a..dbdb400551e 100644 --- a/gix-transport/src/client/blocking_io/http/mod.rs +++ b/gix-transport/src/client/blocking_io/http/mod.rs @@ -179,6 +179,10 @@ pub struct Options { pub ssl_ca_info: Option, /// The SSL version or version range to use, or `None` to let the TLS backend determine which versions are acceptable. pub ssl_version: Option, + /// Controls whether to perform SSL identity verification or not. Turning this off is not recommended and can lead to + /// various security risks. An example where this may be needed is when an internal git server uses a self-signed + /// certificate and the user accepts the associated security risks. + pub ssl_verify: bool, /// The HTTP version to enforce. If unset, it is implementation defined. pub http_version: Option, /// Backend specific options, if available. From ab6e89cb65b5ae6bdf7ff4193227c756cc0af644 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Fri, 1 Dec 2023 09:25:52 +0100 Subject: [PATCH 2/5] assure `ssl_verify` is enabled by default, and also affect host verification. The differentiation between peer and host is done (in `git`) here: https://github.com/git/git/blob/cfb8a6e9a93adbe81efca66e6110c9b4d2e57169/http.c#L980-L987 --- .../client/blocking_io/http/curl/remote.rs | 1 + .../src/client/blocking_io/http/mod.rs | 25 ++++++++++++++++++- 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/gix-transport/src/client/blocking_io/http/curl/remote.rs b/gix-transport/src/client/blocking_io/http/curl/remote.rs index 1557f5a9cbd..de2302a31e4 100644 --- a/gix-transport/src/client/blocking_io/http/curl/remote.rs +++ b/gix-transport/src/client/blocking_io/http/curl/remote.rs @@ -196,6 +196,7 @@ pub fn new() -> ( } handle.ssl_verify_peer(ssl_verify)?; + handle.ssl_verify_host(ssl_verify)?; if let Some(http_version) = http_version { let version = match http_version { diff --git a/gix-transport/src/client/blocking_io/http/mod.rs b/gix-transport/src/client/blocking_io/http/mod.rs index dbdb400551e..b07e7659623 100644 --- a/gix-transport/src/client/blocking_io/http/mod.rs +++ b/gix-transport/src/client/blocking_io/http/mod.rs @@ -120,7 +120,7 @@ pub mod options { /// Options to configure http requests. // TODO: testing most of these fields requires a lot of effort, unless special flags to introspect ongoing requests are added. -#[derive(Default, Clone)] +#[derive(Clone)] pub struct Options { /// Headers to be added to every request. /// They are applied unconditionally and are expected to be valid as they occur in an HTTP request, like `header: value`, without newlines. @@ -189,6 +189,29 @@ pub struct Options { pub backend: Option>>, } +impl Default for Options { + fn default() -> Self { + Options { + extra_headers: vec![], + follow_redirects: Default::default(), + low_speed_limit_bytes_per_second: 0, + low_speed_time_seconds: 0, + proxy: None, + no_proxy: None, + proxy_auth_method: Default::default(), + proxy_authenticate: None, + user_agent: None, + connect_timeout: None, + verbose: false, + ssl_ca_info: None, + ssl_version: None, + ssl_verify: true, + http_version: None, + backend: None, + } + } +} + /// The actual http client implementation, using curl #[cfg(feature = "http-client-curl")] pub type Impl = curl::Curl; From c6e83cf69f1a17e9ba3010bcce3a4ddd3305424c Mon Sep 17 00:00:00 2001 From: Abdullah Alyan Date: Wed, 29 Nov 2023 15:34:30 +0300 Subject: [PATCH 3/5] feat: In gix read http.sslVerify config value and pass it to gix-transport. --- gix/src/config/tree/sections/http.rs | 3 +++ gix/src/repository/config/transport.rs | 11 +++++++++++ gix/tests/fixtures/make_config_repos.sh | 5 +++++ gix/tests/repository/config/transport_options.rs | 13 +++++++++++++ src/plumbing/progress.rs | 4 ---- 5 files changed, 32 insertions(+), 4 deletions(-) diff --git a/gix/src/config/tree/sections/http.rs b/gix/src/config/tree/sections/http.rs index f45c3707616..2ceeeadc8a7 100644 --- a/gix/src/config/tree/sections/http.rs +++ b/gix/src/config/tree/sections/http.rs @@ -10,6 +10,9 @@ impl Http { .with_deviation( "accepts the new 'default' value which means to use the curl default just like the empty string does", ); + /// The `http.sslVerify` key. + pub const SSL_VERIFY: keys::Boolean = keys::Boolean::new_boolean("sslVerify", &config::Tree::HTTP) + .with_deviation("Only supported when using curl as https backend"); /// The `http.proxy` key. pub const PROXY: keys::String = keys::String::new_string("proxy", &config::Tree::HTTP).with_deviation("fails on strings with illformed UTF-8"); diff --git a/gix/src/repository/config/transport.rs b/gix/src/repository/config/transport.rs index 99b5a7f47ff..7d759c3bb03 100644 --- a/gix/src/repository/config/transport.rs +++ b/gix/src/repository/config/transport.rs @@ -405,6 +405,17 @@ impl crate::Repository { } } + { + let key = "http.sslVerify"; + opts.ssl_verify = config + .boolean_filter_by_key(key, &mut trusted_only) + .map(|value| config::tree::Http::SSL_VERIFY.enrich_error(value)) + .transpose() + .with_leniency(lenient) + .map_err(config::transport::http::Error::from)? + .unwrap_or(true); + } + #[cfg(feature = "blocking-http-transport-curl")] { let key = "http.schannelCheckRevoke"; diff --git a/gix/tests/fixtures/make_config_repos.sh b/gix/tests/fixtures/make_config_repos.sh index 447a7a72e07..dee8d1952af 100755 --- a/gix/tests/fixtures/make_config_repos.sh +++ b/gix/tests/fixtures/make_config_repos.sh @@ -164,3 +164,8 @@ mkdir not-a-repo-with-files; (cd not-a-repo-with-files touch this that ) + +git init no-ssl-verify +(cd no-ssl-verify + git config http.sslVerify false +) diff --git a/gix/tests/repository/config/transport_options.rs b/gix/tests/repository/config/transport_options.rs index 85f7b8e6299..16bdc3feb8d 100644 --- a/gix/tests/repository/config/transport_options.rs +++ b/gix/tests/repository/config/transport_options.rs @@ -55,6 +55,7 @@ mod http { verbose, ssl_ca_info, ssl_version, + ssl_verify, http_version, backend, } = http_options(&repo, None, "https://example.com/does/not/matter"); @@ -106,6 +107,9 @@ mod http { max: version }) ); + + assert!(ssl_verify, "SSL verification is enabled by default if not configured"); + assert_eq!(http_version, Some(HttpVersion::V1_1)); } @@ -314,4 +318,13 @@ mod http { assert_eq!(opts.proxy.as_deref(), Some("http://localhost:9090")); assert_eq!(opts.follow_redirects, FollowRedirects::Initial); } + + #[test] + fn no_ssl_verify() { + let repo = repo("no-ssl-verify"); + + let opts = http_options(&repo, None, "https://example.com/does/not/matter"); + + assert!(!opts.ssl_verify); + } } diff --git a/src/plumbing/progress.rs b/src/plumbing/progress.rs index 21b56fa71d8..5a9f033ec55 100644 --- a/src/plumbing/progress.rs +++ b/src/plumbing/progress.rs @@ -408,10 +408,6 @@ static GIT_CONFIG: &[Record] = &[ config: "http.sslCipherList", usage: NotPlanned { reason: "on demand" } }, - Record { - config: "http.sslVerify", - usage: NotPlanned { reason: "on demand" } - }, Record { config: "http.sslCert", usage: NotPlanned { reason: "on demand" } From dd575cd0e3749ff7f59c1582cec0524ff231667d Mon Sep 17 00:00:00 2001 From: Abdullah Alyan Date: Thu, 30 Nov 2023 14:24:27 +0300 Subject: [PATCH 4/5] feat: Add config value gitoxide.http.sslNoVerify This value can by overriden by GIT_SSL_NO_VERIFY env variable. We use the value to override http.sslVerify when specifying ssl_verify in transport Options. --- gix/src/config/cache/init.rs | 4 ++++ gix/src/config/tree/sections/gitoxide.rs | 10 ++++++++++ gix/src/repository/config/transport.rs | 20 +++++++++++++++++++- gix/tests/gix-init.rs | 2 ++ 4 files changed, 35 insertions(+), 1 deletion(-) diff --git a/gix/src/config/cache/init.rs b/gix/src/config/cache/init.rs index 7f6bc0ab3fc..4040a7e2f0b 100644 --- a/gix/src/config/cache/init.rs +++ b/gix/src/config/cache/init.rs @@ -384,6 +384,10 @@ fn apply_environment_overrides( let key = &gitoxide::Http::VERBOSE; (env(key), key.name) }, + { + let key = &gitoxide::Http::SSL_NO_VERIFY; + (env(key), key.name) + }, { let key = &gitoxide::Http::PROXY_AUTH_METHOD; (env(key), key.name) diff --git a/gix/src/config/tree/sections/gitoxide.rs b/gix/src/config/tree/sections/gitoxide.rs index f1eb7a23bd6..f386b43c0f7 100644 --- a/gix/src/config/tree/sections/gitoxide.rs +++ b/gix/src/config/tree/sections/gitoxide.rs @@ -179,6 +179,15 @@ mod subsections { http::SslVersion::new_ssl_version("sslVersionMax", &Gitoxide::HTTP).with_note( "entirely new to set the upper bound for the allowed ssl version range. Overwrites the max bound of `http.sslVersion` if set. Min and Max must be set to become effective.", ); + /// The `gitoxide.http.sslNoVerify` key. + /// + /// If set, disable SSL verification. Using this is discouraged as it can lead to + /// various security risks. An example where this may be needed is when an internal + /// git server uses a self-signed certificate and the user accepts the associated security risks. + pub const SSL_NO_VERIFY: keys::Boolean = keys::Boolean::new_boolean("sslNoVerify", &Gitoxide::HTTP) + .with_environment_override("GIT_SSL_NO_VERIFY") + .with_deviation("Only supported when using curl as https backend") + .with_note("Used to disable SSL verification. When this is enabled it takes prority over http.sslVerify."); /// The `gitoxide.http.proxyAuthMethod` key. pub const PROXY_AUTH_METHOD: http::ProxyAuthMethod = http::ProxyAuthMethod::new_proxy_auth_method("proxyAuthMethod", &Gitoxide::HTTP) @@ -199,6 +208,7 @@ mod subsections { &Self::CONNECT_TIMEOUT, &Self::SSL_VERSION_MIN, &Self::SSL_VERSION_MAX, + &Self::SSL_NO_VERIFY, &Self::PROXY_AUTH_METHOD, ] } diff --git a/gix/src/repository/config/transport.rs b/gix/src/repository/config/transport.rs index 7d759c3bb03..eb89e455b0a 100644 --- a/gix/src/repository/config/transport.rs +++ b/gix/src/repository/config/transport.rs @@ -407,13 +407,31 @@ impl crate::Repository { { let key = "http.sslVerify"; - opts.ssl_verify = config + let ssl_verify = config .boolean_filter_by_key(key, &mut trusted_only) .map(|value| config::tree::Http::SSL_VERIFY.enrich_error(value)) .transpose() .with_leniency(lenient) .map_err(config::transport::http::Error::from)? .unwrap_or(true); + + let ssl_no_verify = config + .boolean_filter( + "gitoxide", + Some("http".into()), + gitoxide::Http::SSL_NO_VERIFY.name, + &mut trusted_only, + ) + .and_then(Result::ok) + .unwrap_or_default(); + + // ssl_no_verify take prority here because it is based on environment variable + // and we try to match git behavior. + if ssl_no_verify { + opts.ssl_verify = false; + } else { + opts.ssl_verify = ssl_verify; + } } #[cfg(feature = "blocking-http-transport-curl")] diff --git a/gix/tests/gix-init.rs b/gix/tests/gix-init.rs index 531f89e9297..b6a51c5e1f7 100644 --- a/gix/tests/gix-init.rs +++ b/gix/tests/gix-init.rs @@ -19,6 +19,7 @@ mod with_overrides { .set("GIT_HTTP_LOW_SPEED_LIMIT", "1") .set("GIT_HTTP_LOW_SPEED_TIME", "1") .set("GIT_HTTP_PROXY_AUTHMETHOD", "proxy-auth-method-env") + .set("GIT_SSL_NO_VERIFY", "true") .set("GIT_CURL_VERBOSE", "true") .set("https_proxy", "https-lower-override") .set("HTTPS_PROXY", "https-upper") @@ -231,6 +232,7 @@ mod with_overrides { ] ); for (key, expected) in [ + ("gitoxide.http.sslNoVerify", "true"), ("gitoxide.http.verbose", "true"), ("gitoxide.allow.protocolFromUser", "file-allowed"), ("core.useReplaceRefs", "no-replace"), From ead00e9c8864eca804b8ba0dbf6792e28da85ecc Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Fri, 1 Dec 2023 09:33:30 +0100 Subject: [PATCH 5/5] refactor --- gix/src/config/cache/init.rs | 13 ++++++--- gix/src/config/tree/sections/gitoxide.rs | 3 +-- gix/src/config/tree/sections/http.rs | 3 ++- gix/src/repository/config/transport.rs | 27 ++++++++----------- .../make_config_repos.tar.xz | 4 +-- gix/tests/fixtures/make_config_repos.sh | 10 +++++-- .../repository/config/transport_options.rs | 17 +++++++++--- 7 files changed, 46 insertions(+), 31 deletions(-) diff --git a/gix/src/config/cache/init.rs b/gix/src/config/cache/init.rs index 4040a7e2f0b..73fb256092d 100644 --- a/gix/src/config/cache/init.rs +++ b/gix/src/config/cache/init.rs @@ -384,16 +384,21 @@ fn apply_environment_overrides( let key = &gitoxide::Http::VERBOSE; (env(key), key.name) }, - { - let key = &gitoxide::Http::SSL_NO_VERIFY; - (env(key), key.name) - }, { let key = &gitoxide::Http::PROXY_AUTH_METHOD; (env(key), key.name) }, ], ), + ( + "gitoxide", + Some(Cow::Borrowed("http".into())), + git_prefix, + &[{ + let key = &gitoxide::Http::SSL_NO_VERIFY; + (env(key), key.name) + }], + ), ( "gitoxide", Some(Cow::Borrowed("credentials".into())), diff --git a/gix/src/config/tree/sections/gitoxide.rs b/gix/src/config/tree/sections/gitoxide.rs index f386b43c0f7..4532895960e 100644 --- a/gix/src/config/tree/sections/gitoxide.rs +++ b/gix/src/config/tree/sections/gitoxide.rs @@ -186,8 +186,7 @@ mod subsections { /// git server uses a self-signed certificate and the user accepts the associated security risks. pub const SSL_NO_VERIFY: keys::Boolean = keys::Boolean::new_boolean("sslNoVerify", &Gitoxide::HTTP) .with_environment_override("GIT_SSL_NO_VERIFY") - .with_deviation("Only supported when using curl as https backend") - .with_note("Used to disable SSL verification. When this is enabled it takes prority over http.sslVerify."); + .with_note("used to disable SSL verification. When this is enabled it takes priority over http.sslVerify"); /// The `gitoxide.http.proxyAuthMethod` key. pub const PROXY_AUTH_METHOD: http::ProxyAuthMethod = http::ProxyAuthMethod::new_proxy_auth_method("proxyAuthMethod", &Gitoxide::HTTP) diff --git a/gix/src/config/tree/sections/http.rs b/gix/src/config/tree/sections/http.rs index 2ceeeadc8a7..4fc733564de 100644 --- a/gix/src/config/tree/sections/http.rs +++ b/gix/src/config/tree/sections/http.rs @@ -12,7 +12,7 @@ impl Http { ); /// The `http.sslVerify` key. pub const SSL_VERIFY: keys::Boolean = keys::Boolean::new_boolean("sslVerify", &config::Tree::HTTP) - .with_deviation("Only supported when using curl as https backend"); + .with_note("also see the `gitoxide.http.sslNoVerify` key"); /// The `http.proxy` key. pub const PROXY: keys::String = keys::String::new_string("proxy", &config::Tree::HTTP).with_deviation("fails on strings with illformed UTF-8"); @@ -61,6 +61,7 @@ impl Section for Http { fn keys(&self) -> &[&dyn Key] { &[ &Self::SSL_VERSION, + &Self::SSL_VERIFY, &Self::PROXY, &Self::PROXY_AUTH_METHOD, &Self::VERSION, diff --git a/gix/src/repository/config/transport.rs b/gix/src/repository/config/transport.rs index eb89e455b0a..907e2a4fb57 100644 --- a/gix/src/repository/config/transport.rs +++ b/gix/src/repository/config/transport.rs @@ -406,31 +406,26 @@ impl crate::Repository { } { - let key = "http.sslVerify"; - let ssl_verify = config + let key = "gitoxide.http.sslNoVerify"; + let ssl_no_verify = config .boolean_filter_by_key(key, &mut trusted_only) - .map(|value| config::tree::Http::SSL_VERIFY.enrich_error(value)) + .map(|value| config::tree::gitoxide::Http::SSL_NO_VERIFY.enrich_error(value)) .transpose() .with_leniency(lenient) .map_err(config::transport::http::Error::from)? - .unwrap_or(true); - - let ssl_no_verify = config - .boolean_filter( - "gitoxide", - Some("http".into()), - gitoxide::Http::SSL_NO_VERIFY.name, - &mut trusted_only, - ) - .and_then(Result::ok) .unwrap_or_default(); - // ssl_no_verify take prority here because it is based on environment variable - // and we try to match git behavior. if ssl_no_verify { opts.ssl_verify = false; } else { - opts.ssl_verify = ssl_verify; + let key = "http.sslVerify"; + opts.ssl_verify = config + .boolean_filter_by_key(key, &mut trusted_only) + .map(|value| config::tree::Http::SSL_VERIFY.enrich_error(value)) + .transpose() + .with_leniency(lenient) + .map_err(config::transport::http::Error::from)? + .unwrap_or(true); } } diff --git a/gix/tests/fixtures/generated-archives/make_config_repos.tar.xz b/gix/tests/fixtures/generated-archives/make_config_repos.tar.xz index 724f2a2914f..6f9cb959b66 100644 --- a/gix/tests/fixtures/generated-archives/make_config_repos.tar.xz +++ b/gix/tests/fixtures/generated-archives/make_config_repos.tar.xz @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:0d33c6282d94600cc6e6f32150e3b57fb523960564efacd1b3f9a13ad30643bd -size 15836 +oid sha256:6c8e4f85cbf1749c998afecef4e442e44b914d00f7626f767a06b3ad6e0cf2d2 +size 16376 diff --git a/gix/tests/fixtures/make_config_repos.sh b/gix/tests/fixtures/make_config_repos.sh index dee8d1952af..3219eb32a1c 100755 --- a/gix/tests/fixtures/make_config_repos.sh +++ b/gix/tests/fixtures/make_config_repos.sh @@ -165,7 +165,13 @@ mkdir not-a-repo-with-files; touch this that ) -git init no-ssl-verify -(cd no-ssl-verify +git init ssl-verify-disabled +(cd ssl-verify-disabled git config http.sslVerify false ) + +git init ssl-no-verify-enabled +(cd ssl-no-verify-enabled + git config http.sslVerify true + git config gitoxide.http.sslNoVerify true +) diff --git a/gix/tests/repository/config/transport_options.rs b/gix/tests/repository/config/transport_options.rs index 16bdc3feb8d..ae96d6a3ff6 100644 --- a/gix/tests/repository/config/transport_options.rs +++ b/gix/tests/repository/config/transport_options.rs @@ -109,7 +109,6 @@ mod http { ); assert!(ssl_verify, "SSL verification is enabled by default if not configured"); - assert_eq!(http_version, Some(HttpVersion::V1_1)); } @@ -320,11 +319,21 @@ mod http { } #[test] - fn no_ssl_verify() { - let repo = repo("no-ssl-verify"); + fn ssl_verify_disabled() { + let repo = repo("ssl-verify-disabled"); let opts = http_options(&repo, None, "https://example.com/does/not/matter"); - assert!(!opts.ssl_verify); } + + #[test] + fn ssl_no_verify_takes_precedence() { + let repo = repo("ssl-no-verify-enabled"); + + let opts = http_options(&repo, None, "https://example.com/does/not/matter"); + assert!( + !opts.ssl_verify, + "even with `http.sslVerify` enabled, `gitoxide.http.sslNoVerify` takes precedence`" + ); + } }