git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH 0/3] Allow choosing the SSL backend cURL uses (plus related patches)
@ 2018-10-15 10:14 Johannes Schindelin via GitGitGadget
  2018-10-15 10:14 ` [PATCH 1/3] http: add support for selecting SSL backends at runtime Johannes Schindelin via GitGitGadget
                   ` (3 more replies)
  0 siblings, 4 replies; 28+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2018-10-15 10:14 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano

This topic branch brings support for choosing cURL's SSL backend (a feature
developed in Git for Windows' context) at runtime via http.sslBackend, and
two more patches that are related (and only of interest for Windows users).

Brendan Forster (1):
  http: add support for disabling SSL revocation checks in cURL

Johannes Schindelin (2):
  http: add support for selecting SSL backends at runtime
  http: when using Secure Channel, ignore sslCAInfo by default

 Documentation/config.txt | 21 ++++++++++++
 http.c                   | 71 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 91 insertions(+), 1 deletion(-)


base-commit: 5a0cc8aca797dbd7d2be3b67458ff880ed45cddf
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-46%2Fdscho%2Fhttp-ssl-backend-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-46/dscho/http-ssl-backend-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/46
-- 
gitgitgadget

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

* [PATCH 1/3] http: add support for selecting SSL backends at runtime
  2018-10-15 10:14 [PATCH 0/3] Allow choosing the SSL backend cURL uses (plus related patches) Johannes Schindelin via GitGitGadget
@ 2018-10-15 10:14 ` Johannes Schindelin via GitGitGadget
  2018-10-15 14:06   ` Eric Sunshine
  2018-10-15 10:14 ` [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL Brendan Forster via GitGitGadget
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 28+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2018-10-15 10:14 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

As of version 7.56.0, curl supports being compiled with multiple SSL
backends.

This patch adds the Git side of that feature: by setting http.sslBackend
to "openssl" or "schannel", Git for Windows can now choose the SSL
backend at runtime.

This comes in handy on Windows because Secure Channel ("schannel") is
the native solution, accessing the Windows Credential Store, thereby
allowing for enterprise-wide management of certificates. For historical
reasons, Git for Windows needs to support OpenSSL still, as it has
previously been the only supported SSL backend in Git for Windows for
almost a decade.

The patch has been carried in Git for Windows for over a year, and is
considered mature.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/config.txt |  5 +++++
 http.c                   | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1546833213..7d38f0bf1a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1984,6 +1984,11 @@ http.sslCAPath::
 	with when fetching or pushing over HTTPS. Can be overridden
 	by the `GIT_SSL_CAPATH` environment variable.
 
+http.sslBackend::
+	Name of the SSL backend to use (e.g. "openssl" or "schannel").
+	This option is ignored if cURL lacks support for choosing the SSL
+	backend at runtime.
+
 http.pinnedpubkey::
 	Public key of the https service. It may either be the filename of
 	a PEM or DER encoded public key file or a string starting with
diff --git a/http.c b/http.c
index 98ff122585..7fb37a061b 100644
--- a/http.c
+++ b/http.c
@@ -155,6 +155,8 @@ static struct active_request_slot *active_queue_head;
 
 static char *cached_accept_language;
 
+static char *http_ssl_backend;
+
 size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
 {
 	size_t size = eltsize * nmemb;
@@ -302,6 +304,12 @@ static int http_options(const char *var, const char *value, void *cb)
 		curl_ssl_try = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp("http.sslbackend", var)) {
+		free(http_ssl_backend);
+		http_ssl_backend = xstrdup_or_null(value);
+		return 0;
+	}
+
 	if (!strcmp("http.minsessions", var)) {
 		min_curl_sessions = git_config_int(var, value);
 #ifndef USE_CURL_MULTI
@@ -995,6 +1003,33 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
 	git_config(urlmatch_config_entry, &config);
 	free(normalized_url);
 
+#if LIBCURL_VERSION_NUM >= 0x073800
+	if (http_ssl_backend) {
+		const curl_ssl_backend **backends;
+		struct strbuf buf = STRBUF_INIT;
+		int i;
+
+		switch (curl_global_sslset(-1, http_ssl_backend, &backends)) {
+		case CURLSSLSET_UNKNOWN_BACKEND:
+			strbuf_addf(&buf, _("Unsupported SSL backend '%s'. "
+					    "Supported SSL backends:"),
+					    http_ssl_backend);
+			for (i = 0; backends[i]; i++)
+				strbuf_addf(&buf, "\n\t%s", backends[i]->name);
+			die("%s", buf.buf);
+		case CURLSSLSET_NO_BACKENDS:
+			die(_("Could not set SSL backend to '%s': "
+			      "cURL was built without SSL backends"),
+			    http_ssl_backend);
+		case CURLSSLSET_TOO_LATE:
+			die(_("Could not set SSL backend to '%s': already set"),
+			    http_ssl_backend);
+		case CURLSSLSET_OK:
+			break; /* Okay! */
+		}
+	}
+#endif
+
 	if (curl_global_init(CURL_GLOBAL_ALL) != CURLE_OK)
 		die("curl_global_init failed");
 
-- 
gitgitgadget


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

* [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL
  2018-10-15 10:14 [PATCH 0/3] Allow choosing the SSL backend cURL uses (plus related patches) Johannes Schindelin via GitGitGadget
  2018-10-15 10:14 ` [PATCH 1/3] http: add support for selecting SSL backends at runtime Johannes Schindelin via GitGitGadget
@ 2018-10-15 10:14 ` Brendan Forster via GitGitGadget
  2018-10-15 14:10   ` Eric Sunshine
  2018-10-16  4:23   ` Junio C Hamano
  2018-10-15 10:14 ` [PATCH 3/3] http: when using Secure Channel, ignore sslCAInfo by default Johannes Schindelin via GitGitGadget
  2018-10-25 18:53 ` [PATCH v2 0/3] Allow choosing the SSL backend cURL uses (plus related patches) Johannes Schindelin via GitGitGadget
  3 siblings, 2 replies; 28+ messages in thread
From: Brendan Forster via GitGitGadget @ 2018-10-15 10:14 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Brendan Forster

From: Brendan Forster <github@brendanforster.com>

This adds support for a new http.schannelCheckRevoke config value.

This config value is only used if http.sslBackend is set to "schannel",
which forces cURL to use the Windows Certificate Store when validating
server certificates associated with a remote server.

This config value should only be set to "false" if you are in an
environment where revocation checks are blocked by the network, with
no alternative options.

This is only supported in cURL 7.44 or later.

Note: an earlier iteration tried to use the config setting
http.schannel.checkRevoke, but the http.* config settings can be limited
to specific URLs via http.<url>.* (which would mistake `schannel` for a
URL).

Helped by Agustín Martín Barbero.

Signed-off-by: Brendan Forster <github@brendanforster.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/config.txt |  8 ++++++++
 http.c                   | 17 +++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 7d38f0bf1a..d569ebd49e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1989,6 +1989,14 @@ http.sslBackend::
 	This option is ignored if cURL lacks support for choosing the SSL
 	backend at runtime.
 
+http.schannelCheckRevoke::
+	Used to enforce or disable certificate revocation checks in cURL
+	when http.sslBackend is set to "schannel". Defaults to `true` if
+	unset. Only necessary to disable this if Git consistently errors
+	and the message is about checking the revocation status of a
+	certificate. This option is ignored if cURL lacks support for
+	setting the relevant SSL option at runtime.
+
 http.pinnedpubkey::
 	Public key of the https service. It may either be the filename of
 	a PEM or DER encoded public key file or a string starting with
diff --git a/http.c b/http.c
index 7fb37a061b..8998056b60 100644
--- a/http.c
+++ b/http.c
@@ -157,6 +157,8 @@ static char *cached_accept_language;
 
 static char *http_ssl_backend;
 
+static int http_schannel_check_revoke = 1;
+
 size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
 {
 	size_t size = eltsize * nmemb;
@@ -310,6 +312,11 @@ static int http_options(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (!strcmp("http.schannelcheckrevoke", var)) {
+		http_schannel_check_revoke = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (!strcmp("http.minsessions", var)) {
 		min_curl_sessions = git_config_int(var, value);
 #ifndef USE_CURL_MULTI
@@ -811,6 +818,16 @@ static CURL *get_curl_handle(void)
 	}
 #endif
 
+	if (http_ssl_backend && !strcmp("schannel", http_ssl_backend) &&
+	    !http_schannel_check_revoke) {
+#if LIBCURL_VERSION_NUM >= 0x072c00
+		curl_easy_setopt(result, CURLOPT_SSL_OPTIONS, CURLSSLOPT_NO_REVOKE);
+#else
+		warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL options because\n"
+			"your curl version is too old (>= 7.44.0)");
+#endif
+	}
+
 	if (http_proactive_auth)
 		init_curl_http_auth(result);
 
-- 
gitgitgadget


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

* [PATCH 3/3] http: when using Secure Channel, ignore sslCAInfo by default
  2018-10-15 10:14 [PATCH 0/3] Allow choosing the SSL backend cURL uses (plus related patches) Johannes Schindelin via GitGitGadget
  2018-10-15 10:14 ` [PATCH 1/3] http: add support for selecting SSL backends at runtime Johannes Schindelin via GitGitGadget
  2018-10-15 10:14 ` [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL Brendan Forster via GitGitGadget
@ 2018-10-15 10:14 ` Johannes Schindelin via GitGitGadget
  2018-10-25 18:53 ` [PATCH v2 0/3] Allow choosing the SSL backend cURL uses (plus related patches) Johannes Schindelin via GitGitGadget
  3 siblings, 0 replies; 28+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2018-10-15 10:14 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

As of cURL v7.60.0, the Secure Channel backend can use the certificate
bundle provided via `http.sslCAInfo`, but that would override the
Windows Certificate Store. Since this is not desirable by default, let's
tell Git to not ask cURL to use that bundle by default when the `schannel`
backend was configured via `http.sslBackend`, unless
`http.schannelUseSSLCAInfo` overrides this behavior.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/config.txt |  8 ++++++++
 http.c                   | 19 ++++++++++++++++++-
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index d569ebd49e..1f6a6a4a6f 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1997,6 +1997,14 @@ http.schannelCheckRevoke::
 	certificate. This option is ignored if cURL lacks support for
 	setting the relevant SSL option at runtime.
 
+http.schannelUseSSLCAInfo::
+	As of cURL v7.60.0, the Secure Channel backend can use the
+	certificate bundle provided via `http.sslCAInfo`, but that would
+	override the Windows Certificate Store. Since this is not desirable
+	by default, Git will tell cURL not to use that bundle by default
+	when the `schannel` backend was configured via `http.sslBackend`,
+	unless `http.schannelUseSSLCAInfo` overrides this behavior.
+
 http.pinnedpubkey::
 	Public key of the https service. It may either be the filename of
 	a PEM or DER encoded public key file or a string starting with
diff --git a/http.c b/http.c
index 8998056b60..a0a8b93785 100644
--- a/http.c
+++ b/http.c
@@ -158,6 +158,12 @@ static char *cached_accept_language;
 static char *http_ssl_backend;
 
 static int http_schannel_check_revoke = 1;
+/*
+ * With the backend being set to `schannel`, setting sslCAinfo would override
+ * the Certificate Store in cURL v7.60.0 and later, which is not what we want
+ * by default.
+ */
+static int http_schannel_use_ssl_cainfo;
 
 size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
 {
@@ -317,6 +323,11 @@ static int http_options(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (!strcmp("http.schannelusesslcainfo", var)) {
+		http_schannel_use_ssl_cainfo = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (!strcmp("http.minsessions", var)) {
 		min_curl_sessions = git_config_int(var, value);
 #ifndef USE_CURL_MULTI
@@ -869,7 +880,13 @@ static CURL *get_curl_handle(void)
 	if (ssl_pinnedkey != NULL)
 		curl_easy_setopt(result, CURLOPT_PINNEDPUBLICKEY, ssl_pinnedkey);
 #endif
-	if (ssl_cainfo != NULL)
+	if (http_ssl_backend && !strcmp("schannel", http_ssl_backend) &&
+	    !http_schannel_use_ssl_cainfo) {
+		curl_easy_setopt(result, CURLOPT_CAINFO, NULL);
+#if LIBCURL_VERSION_NUM >= 0x073400
+		curl_easy_setopt(result, CURLOPT_PROXY_CAINFO, NULL);
+#endif
+	} else if (ssl_cainfo != NULL)
 		curl_easy_setopt(result, CURLOPT_CAINFO, ssl_cainfo);
 
 	if (curl_low_speed_limit > 0 && curl_low_speed_time > 0) {
-- 
gitgitgadget

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

* Re: [PATCH 1/3] http: add support for selecting SSL backends at runtime
  2018-10-15 10:14 ` [PATCH 1/3] http: add support for selecting SSL backends at runtime Johannes Schindelin via GitGitGadget
@ 2018-10-15 14:06   ` Eric Sunshine
  0 siblings, 0 replies; 28+ messages in thread
From: Eric Sunshine @ 2018-10-15 14:06 UTC (permalink / raw)
  To: gitgitgadget; +Cc: Git List, Junio C Hamano, Johannes Schindelin

On Mon, Oct 15, 2018 at 6:14 AM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> This patch adds the Git side of that feature: by setting http.sslBackend
> to "openssl" or "schannel", Git for Windows can now choose the SSL
> backend at runtime.
> [...]
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/http.c b/http.c
> @@ -995,6 +1003,33 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
> +#if LIBCURL_VERSION_NUM >= 0x073800
> +       if (http_ssl_backend) {
> +               const curl_ssl_backend **backends;
> +               struct strbuf buf = STRBUF_INIT;
> +               int i;
> +
> +               switch (curl_global_sslset(-1, http_ssl_backend, &backends)) {
> +               case CURLSSLSET_UNKNOWN_BACKEND:
> +                       strbuf_addf(&buf, _("Unsupported SSL backend '%s'. "
> +                                           "Supported SSL backends:"),
> +                                           http_ssl_backend);
> +                       for (i = 0; backends[i]; i++)
> +                               strbuf_addf(&buf, "\n\t%s", backends[i]->name);
> +                       die("%s", buf.buf);

This is the only 'case' arm which utilizes 'strbuf buf', and it
die()s, so no leak despite a lack of strbuf_release(). Okay.

> +               case CURLSSLSET_NO_BACKENDS:
> +                       die(_("Could not set SSL backend to '%s': "
> +                             "cURL was built without SSL backends"),
> +                           http_ssl_backend);
> +               case CURLSSLSET_TOO_LATE:
> +                       die(_("Could not set SSL backend to '%s': already set"),
> +                           http_ssl_backend);
> +               case CURLSSLSET_OK:
> +                       break; /* Okay! */
> +               }
> +       }
> +#endif

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

* Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL
  2018-10-15 10:14 ` [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL Brendan Forster via GitGitGadget
@ 2018-10-15 14:10   ` Eric Sunshine
  2018-10-16 12:21     ` Johannes Schindelin
  2018-10-25  3:18     ` Junio C Hamano
  2018-10-16  4:23   ` Junio C Hamano
  1 sibling, 2 replies; 28+ messages in thread
From: Eric Sunshine @ 2018-10-15 14:10 UTC (permalink / raw)
  To: gitgitgadget; +Cc: Git List, Junio C Hamano, github

On Mon, Oct 15, 2018 at 6:14 AM Brendan Forster via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> This config value is only used if http.sslBackend is set to "schannel",
> which forces cURL to use the Windows Certificate Store when validating
> server certificates associated with a remote server.
>
> This is only supported in cURL 7.44 or later.
> [...]
> Signed-off-by: Brendan Forster <github@brendanforster.com>
> ---
> diff --git a/http.c b/http.c
> @@ -811,6 +818,16 @@ static CURL *get_curl_handle(void)
> +       if (http_ssl_backend && !strcmp("schannel", http_ssl_backend) &&
> +           !http_schannel_check_revoke) {
> +#if LIBCURL_VERSION_NUM >= 0x072c00
> +               curl_easy_setopt(result, CURLOPT_SSL_OPTIONS, CURLSSLOPT_NO_REVOKE);
> +#else
> +               warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL options because\n"
> +                       "your curl version is too old (>= 7.44.0)");

This message is confusing. If your curl is too old, shouldn't the ">=" be a "<"?

> +#endif
> +       }

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

* Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL
  2018-10-15 10:14 ` [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL Brendan Forster via GitGitGadget
  2018-10-15 14:10   ` Eric Sunshine
@ 2018-10-16  4:23   ` Junio C Hamano
  2018-10-16  6:33     ` Jeff King
  2018-10-16 12:22     ` Johannes Schindelin
  1 sibling, 2 replies; 28+ messages in thread
From: Junio C Hamano @ 2018-10-16  4:23 UTC (permalink / raw)
  To: Brendan Forster via GitGitGadget; +Cc: git, Brendan Forster

"Brendan Forster via GitGitGadget" <gitgitgadget@gmail.com> writes:

> Note: an earlier iteration tried to use the config setting
> http.schannel.checkRevoke, but the http.* config settings can be limited
> to specific URLs via http.<url>.* (which would mistake `schannel` for a
> URL).

Yeah, "http.schannel.anything" would not work, but is this note
relevant here?  As far as the git development community as a whole
is concerned, this is the first iteration of the patch we see and
review.

In any case, you can use "http.<url>.$variable" to say "I want the
http.$variable to be in effect but only when I am talking to <url>".
Does it make sense for this new variable, too?  That is, does it
benefit the users to be able to do something like this?

    [http] schannelCheckRevoke = no
    [http "https://microsoft.com/"] schannelCheckRevoke = yes

I am guessing that the answer is yes.

I guess the same comment applies to the previous step, but I suspect
that the code structure may not allow us to switch the SSL backend
so late in the game (e.g. "when talking to microsoft, use schannel,
but when talking to github, use openssl").

> +#if LIBCURL_VERSION_NUM >= 0x072c00
> +		curl_easy_setopt(result, CURLOPT_SSL_OPTIONS, CURLSSLOPT_NO_REVOKE);
> +#else
> +		warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL options because\n"
> +			"your curl version is too old (>= 7.44.0)");
> +#endif

That ">=" is hard to grok.  I think you meant it to be pronounced
"requries at least", but that is not a common reading.  People more
commonly pronounce it "is greater than or equal to".

> +	}
> +
>  	if (http_proactive_auth)
>  		init_curl_http_auth(result);

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

* Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL
  2018-10-16  4:23   ` Junio C Hamano
@ 2018-10-16  6:33     ` Jeff King
  2018-10-16 12:25       ` Johannes Schindelin
  2018-10-16 12:22     ` Johannes Schindelin
  1 sibling, 1 reply; 28+ messages in thread
From: Jeff King @ 2018-10-16  6:33 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Brendan Forster via GitGitGadget, git, Brendan Forster

On Tue, Oct 16, 2018 at 01:23:25PM +0900, Junio C Hamano wrote:

> > +#if LIBCURL_VERSION_NUM >= 0x072c00
> > +		curl_easy_setopt(result, CURLOPT_SSL_OPTIONS, CURLSSLOPT_NO_REVOKE);
> > +#else
> > +		warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL options because\n"
> > +			"your curl version is too old (>= 7.44.0)");
> > +#endif
> 
> That ">=" is hard to grok.  I think you meant it to be pronounced
> "requries at least", but that is not a common reading.  People more
> commonly pronounce it "is greater than or equal to".

This seemed oddly familiar:

  https://public-inbox.org/git/8da9d436-88b9-7959-dd9c-65bdd376bf54@mail.mipt.ru/

Since this one is clearly copied from there, it may be worth fixing the
original.

-Peff

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

* Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL
  2018-10-15 14:10   ` Eric Sunshine
@ 2018-10-16 12:21     ` Johannes Schindelin
  2018-10-25  3:18     ` Junio C Hamano
  1 sibling, 0 replies; 28+ messages in thread
From: Johannes Schindelin @ 2018-10-16 12:21 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: gitgitgadget, Git List, Junio C Hamano, github

Hi Eric,


On Mon, 15 Oct 2018, Eric Sunshine wrote:

> On Mon, Oct 15, 2018 at 6:14 AM Brendan Forster via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > This config value is only used if http.sslBackend is set to "schannel",
> > which forces cURL to use the Windows Certificate Store when validating
> > server certificates associated with a remote server.
> >
> > This is only supported in cURL 7.44 or later.
> > [...]
> > Signed-off-by: Brendan Forster <github@brendanforster.com>
> > ---
> > diff --git a/http.c b/http.c
> > @@ -811,6 +818,16 @@ static CURL *get_curl_handle(void)
> > +       if (http_ssl_backend && !strcmp("schannel", http_ssl_backend) &&
> > +           !http_schannel_check_revoke) {
> > +#if LIBCURL_VERSION_NUM >= 0x072c00
> > +               curl_easy_setopt(result, CURLOPT_SSL_OPTIONS, CURLSSLOPT_NO_REVOKE);
> > +#else
> > +               warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL options because\n"
> > +                       "your curl version is too old (>= 7.44.0)");
> 
> This message is confusing. If your curl is too old, shouldn't the ">=" be a "<"?

Absolutely correct. Will fix,
Dscho

> 
> > +#endif
> > +       }
> 

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

* Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL
  2018-10-16  4:23   ` Junio C Hamano
  2018-10-16  6:33     ` Jeff King
@ 2018-10-16 12:22     ` Johannes Schindelin
  2018-10-18  1:53       ` Junio C Hamano
  1 sibling, 1 reply; 28+ messages in thread
From: Johannes Schindelin @ 2018-10-16 12:22 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Brendan Forster via GitGitGadget, git, Brendan Forster

Hi Junio,

On Tue, 16 Oct 2018, Junio C Hamano wrote:

> "Brendan Forster via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> > Note: an earlier iteration tried to use the config setting
> > http.schannel.checkRevoke, but the http.* config settings can be limited
> > to specific URLs via http.<url>.* (which would mistake `schannel` for a
> > URL).
> 
> Yeah, "http.schannel.anything" would not work, but is this note
> relevant here?  As far as the git development community as a whole
> is concerned, this is the first iteration of the patch we see and
> review.

I did review that commit message before typing /submit, and I figured that
it would still make sense to leave a comment there why we did *not* use
http.schannel.checkRevoke. I know that *my* review comments would include
a question about this, had I not been part of the development of this
patch.

> In any case, you can use "http.<url>.$variable" to say "I want the
> http.$variable to be in effect but only when I am talking to <url>".
> Does it make sense for this new variable, too?  That is, does it
> benefit the users to be able to do something like this?
> 
>     [http] schannelCheckRevoke = no
>     [http "https://microsoft.com/"] schannelCheckRevoke = yes
> 
> I am guessing that the answer is yes.

Frankly, I do not know.  Does it hurt, though?

This patch neither introduces the `http.<url>.<key>` feature nor prevents
it. Its original design (which I found more logical than the current one),
however, clashes with that feature.

> I guess the same comment applies to the previous step, but I suspect
> that the code structure may not allow us to switch the SSL backend
> so late in the game (e.g. "when talking to microsoft, use schannel,
> but when talking to github, use openssl").

That's a really good question, and I suspect that it is actually not too
late. Let me try.

*clicketyclick*

-- snip --
$ git config --show-origin http.sslbackend
file:C:/Program Files/Git/mingw64/etc/gitconfig schannel

$ GIT_TRACE_CURL=1 git -c 'http.https://github.com/dscho/images.sslbackend=openssl' ls-remote https://github.com/dscho/images
14:03:52.319986 http.c:724              == Info: Couldn't find host github.com in the _netrc file; using defaults
14:03:52.366858 http.c:724              == Info:   Trying 192.30.253.113...
14:03:52.366858 http.c:724              == Info: TCP_NODELAY set
14:03:52.482825 http.c:724              == Info: Connected to github.com (192.30.253.113) port 443 (#0)
14:03:52.721173 http.c:724              == Info: ALPN, offering http/1.1
14:03:52.721173 http.c:724              == Info: Cipher selection: ALL:!EXPORT:!EXPORT40:!EXPORT56:!aNULL:!LOW:!RC4:@STRENGTH
14:03:52.721173 http.c:724              == Info: error setting certificate verify locations:
  CAfile: C:/Program Files/Git/mingw64/libexec/ssl/certs/ca-bundle.crt
  CApath: none
fatal: unable to access 'https://github.com/dscho/images/': error setting certificate verify locations:
  CAfile: C:/Program Files/Git/mingw64/libexec/ssl/certs/ca-bundle.crt
  CApath: none
-- snap --

Please allow me to help understand this log. First, I verified that the
currently-configured backend is Secure Channel. Then, I ask Git to list
the remote refs of a small repository, special-casing it to the OpenSSL
backend.

Crucially, this fails. The short version is: this is good! Because it
means that Git used the OpenSSL backend, as clearly intended.

<skip if="uninterested in the details">
Why does it fail?

Two reasons:

1) Git for Windows has to disable the certificate bundle. The gist of it
is: Git LFS uses Git for Windows' certificate bundle, if configured, and
that would override the Windows Certificate Store. When users configure
Secure Channel, however, they *want* to use the Windows Certificate Store,
so to accommodate Git LFS, we "unconfigure" http.sslCAInfo in that case.

2) The libcurl used by Git for Windows has some smarts built in to
understand relative paths to its "Unix pseudo root". However, this fails
when libcurl is loaded from libexec/git-core/ (which is the case, to avoid
any libcurl-4.dll in C:\Windows\system32 from being picked up by mistake).

If this explanation sounds obscure, the reason is that it *is* obscure. If
you are truly interested in the details, I can point you to the relevant
tickets on Git for Windows' bug tracker.
</skip>

> > +#if LIBCURL_VERSION_NUM >= 0x072c00
> > +		curl_easy_setopt(result, CURLOPT_SSL_OPTIONS, CURLSSLOPT_NO_REVOKE);
> > +#else
> > +		warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL options because\n"
> > +			"your curl version is too old (>= 7.44.0)");
> > +#endif
> 
> That ">=" is hard to grok.  I think you meant it to be pronounced
> "requries at least", but that is not a common reading.  People more
> commonly pronounce it "is greater than or equal to".

Eric made the same point. I will change it to '<'.

Side note: this is the kind of change that I am completely comfortable
making, even on patches that were battle-tested, because those kind of
changes are certain not to affect the correctness of the patch.

Thanks,
Dscho

> 
> > +	}
> > +
> >  	if (http_proactive_auth)
> >  		init_curl_http_auth(result);
> 

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

* Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL
  2018-10-16  6:33     ` Jeff King
@ 2018-10-16 12:25       ` Johannes Schindelin
  2018-10-16 15:28         ` Jeff King
  0 siblings, 1 reply; 28+ messages in thread
From: Johannes Schindelin @ 2018-10-16 12:25 UTC (permalink / raw)
  To: Jeff King
  Cc: Junio C Hamano, Brendan Forster via GitGitGadget, git,
	Brendan Forster

Hi Peff,

On Tue, 16 Oct 2018, Jeff King wrote:

> On Tue, Oct 16, 2018 at 01:23:25PM +0900, Junio C Hamano wrote:
> 
> > > +#if LIBCURL_VERSION_NUM >= 0x072c00
> > > +		curl_easy_setopt(result, CURLOPT_SSL_OPTIONS, CURLSSLOPT_NO_REVOKE);
> > > +#else
> > > +		warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL options because\n"
> > > +			"your curl version is too old (>= 7.44.0)");
> > > +#endif
> > 
> > That ">=" is hard to grok.  I think you meant it to be pronounced
> > "requries at least", but that is not a common reading.  People more
> > commonly pronounce it "is greater than or equal to".
> 
> This seemed oddly familiar:
> 
>   https://public-inbox.org/git/8da9d436-88b9-7959-dd9c-65bdd376bf54@mail.mipt.ru/
> 
> Since this one is clearly copied from there, it may be worth fixing the
> original.

Good memory. I just integrated the patch here. It was not signed off, but
it is too obvious to be protected under copyright, so I re-did it, adding
a nice commit message.

Ciao,
Dscho

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

* Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL
  2018-10-16 12:25       ` Johannes Schindelin
@ 2018-10-16 15:28         ` Jeff King
  0 siblings, 0 replies; 28+ messages in thread
From: Jeff King @ 2018-10-16 15:28 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Junio C Hamano, Brendan Forster via GitGitGadget, git,
	Brendan Forster

On Tue, Oct 16, 2018 at 02:25:57PM +0200, Johannes Schindelin wrote:

> > > That ">=" is hard to grok.  I think you meant it to be pronounced
> > > "requries at least", but that is not a common reading.  People more
> > > commonly pronounce it "is greater than or equal to".
> > 
> > This seemed oddly familiar:
> > 
> >   https://public-inbox.org/git/8da9d436-88b9-7959-dd9c-65bdd376bf54@mail.mipt.ru/
> > 
> > Since this one is clearly copied from there, it may be worth fixing the
> > original.
> 
> Good memory. I just integrated the patch here. It was not signed off, but
> it is too obvious to be protected under copyright, so I re-did it, adding
> a nice commit message.

Yay, thank you (I considered doing the same, but was scratching my head
over how to deal with authorship and signoff).

-Peff

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

* Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL
  2018-10-16 12:22     ` Johannes Schindelin
@ 2018-10-18  1:53       ` Junio C Hamano
  2018-10-25 18:52         ` Johannes Schindelin
  0 siblings, 1 reply; 28+ messages in thread
From: Junio C Hamano @ 2018-10-18  1:53 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Brendan Forster via GitGitGadget, git, Brendan Forster

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> In any case, you can use "http.<url>.$variable" to say "I want the
>> http.$variable to be in effect but only when I am talking to <url>".
>> Does it make sense for this new variable, too?  That is, does it
>> benefit the users to be able to do something like this?
>> 
>>     [http] schannelCheckRevoke = no
>>     [http "https://microsoft.com/"] schannelCheckRevoke = yes
>> 
>> I am guessing that the answer is yes.
>
> Frankly, I do not know.  Does it hurt, though?

I did not and I do not think it would.  I was wondering if the
ability to be able to specify these per destination is something
very useful and deserves to be called out in the doc, together with
...

>> I guess the same comment applies to the previous step, but I suspect
>> that the code structure may not allow us to switch the SSL backend
>> so late in the game (e.g. "when talking to microsoft, use schannel,
>> but when talking to github, use openssl").

... this bit.

> Crucially, this fails. The short version is: this is good! Because it
> means that Git used the OpenSSL backend, as clearly intended.
>
> <skip if="uninterested in the details">
> Why does it fail?
> ...
> </skip>

So there may still be some polishing needed, but as long as people
are not using the "per destination" thing, the code is already good?
That is something we may want to document.

Thanks.

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

* Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL
  2018-10-15 14:10   ` Eric Sunshine
  2018-10-16 12:21     ` Johannes Schindelin
@ 2018-10-25  3:18     ` Junio C Hamano
  2018-10-25  3:29       ` [PATCH] http: give curl version warnings consistently Junio C Hamano
  2018-10-25 12:12       ` [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL Johannes Schindelin
  1 sibling, 2 replies; 28+ messages in thread
From: Junio C Hamano @ 2018-10-25  3:18 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: gitgitgadget, Git List, github

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Mon, Oct 15, 2018 at 6:14 AM Brendan Forster via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> This config value is only used if http.sslBackend is set to "schannel",
>> which forces cURL to use the Windows Certificate Store when validating
>> server certificates associated with a remote server.
>>
>> This is only supported in cURL 7.44 or later.
>> [...]
>> Signed-off-by: Brendan Forster <github@brendanforster.com>
>> ---
>> diff --git a/http.c b/http.c
>> @@ -811,6 +818,16 @@ static CURL *get_curl_handle(void)
>> +       if (http_ssl_backend && !strcmp("schannel", http_ssl_backend) &&
>> +           !http_schannel_check_revoke) {
>> +#if LIBCURL_VERSION_NUM >= 0x072c00
>> +               curl_easy_setopt(result, CURLOPT_SSL_OPTIONS, CURLSSLOPT_NO_REVOKE);
>> +#else
>> +               warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL options because\n"
>> +                       "your curl version is too old (>= 7.44.0)");
>
> This message is confusing. If your curl is too old, shouldn't the ">=" be a "<"?

I do not think I saw any update to correct this, and worse yet I do
not offhand recall if there was any other issue raised on the
series.

So assuming that this is the only remaining one, I'll squash the
following to step 2/3 of this three-patch series and plan to merge
it down to 'next' in the coming few days.

I have a clean-up suggestion related to this but is orthogonal to
this three-patch series (after the fix-up is applied, anyway), which
I'll be sending out separately.

Thanks.

 http.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/http.c b/http.c
index 0ebf8f77a6..43e75ac583 100644
--- a/http.c
+++ b/http.c
@@ -835,7 +835,7 @@ static CURL *get_curl_handle(void)
 		curl_easy_setopt(result, CURLOPT_SSL_OPTIONS, CURLSSLOPT_NO_REVOKE);
 #else
 		warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL options because\n"
-			"your curl version is too old (>= 7.44.0)");
+			"your curl version is too old (< 7.44.0)");
 #endif
 	}
 
-- 
2.19.1-542-gc4df23f792


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

* [PATCH] http: give curl version warnings consistently
  2018-10-25  3:18     ` Junio C Hamano
@ 2018-10-25  3:29       ` Junio C Hamano
  2018-10-25  6:23         ` Jeff King
  2018-10-25 19:00         ` Johannes Schindelin
  2018-10-25 12:12       ` [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL Johannes Schindelin
  1 sibling, 2 replies; 28+ messages in thread
From: Junio C Hamano @ 2018-10-25  3:29 UTC (permalink / raw)
  To: Git List; +Cc: Eric Sunshine, github

When a requested feature cannot be activated because the version of
cURL library used to build Git with is too old, most of the codepaths
give a warning like "$Feature is not supported with cURL < $Version",
marked for l10n.  A few of them, however, did not follow that pattern
and said things like "$Feature is not activated, your curl version is
too old (>= $Version)", and without marking them for l10n.

Update these to match the style of the majority of warnings and mark
them for l10n.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 > I have a clean-up suggestion related to this but is orthogonal to
 > this three-patch series (after the fix-up is applied, anyway), which
 > I'll be sending out separately.

 So, here it is.

 http.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/http.c b/http.c
index 43e75ac583..2214100e3b 100644
--- a/http.c
+++ b/http.c
@@ -834,8 +834,7 @@ static CURL *get_curl_handle(void)
 #if LIBCURL_VERSION_NUM >= 0x072c00
 		curl_easy_setopt(result, CURLOPT_SSL_OPTIONS, CURLSSLOPT_NO_REVOKE);
 #else
-		warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL options because\n"
-			"your curl version is too old (< 7.44.0)");
+		warning(_("CURLSSLOPT_NO_REVOKE not suported with cURL < 7.44.0"));
 #endif
 	}
 
@@ -908,8 +907,7 @@ static CURL *get_curl_handle(void)
 	curl_easy_setopt(result, CURLOPT_PROTOCOLS,
 			 get_curl_allowed_protocols(-1));
 #else
-	warning("protocol restrictions not applied to curl redirects because\n"
-		"your curl version is too old (>= 7.19.4)");
+	warning(_("Protocol restrictions not supported with cURL < 7.19.4"));
 #endif
 	if (getenv("GIT_CURL_VERBOSE"))
 		curl_easy_setopt(result, CURLOPT_VERBOSE, 1L);
-- 
2.19.1-542-gc4df23f792


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

* Re: [PATCH] http: give curl version warnings consistently
  2018-10-25  3:29       ` [PATCH] http: give curl version warnings consistently Junio C Hamano
@ 2018-10-25  6:23         ` Jeff King
  2018-10-25 19:00         ` Johannes Schindelin
  1 sibling, 0 replies; 28+ messages in thread
From: Jeff King @ 2018-10-25  6:23 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git List, Eric Sunshine, github

On Thu, Oct 25, 2018 at 12:29:19PM +0900, Junio C Hamano wrote:

> When a requested feature cannot be activated because the version of
> cURL library used to build Git with is too old, most of the codepaths
> give a warning like "$Feature is not supported with cURL < $Version",
> marked for l10n.  A few of them, however, did not follow that pattern
> and said things like "$Feature is not activated, your curl version is
> too old (>= $Version)", and without marking them for l10n.
> 
> Update these to match the style of the majority of warnings and mark
> them for l10n.

This is definitely an improvement.

> @@ -908,8 +907,7 @@ static CURL *get_curl_handle(void)
>  	curl_easy_setopt(result, CURLOPT_PROTOCOLS,
>  			 get_curl_allowed_protocols(-1));
>  #else
> -	warning("protocol restrictions not applied to curl redirects because\n"
> -		"your curl version is too old (>= 7.19.4)");
> +	warning(_("Protocol restrictions not supported with cURL < 7.19.4"));

This loses the mention of redirects, but I think that is actually a
bonus. The #ifdef'd code covers both original and redirected requests.

-Peff

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

* Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL
  2018-10-25  3:18     ` Junio C Hamano
  2018-10-25  3:29       ` [PATCH] http: give curl version warnings consistently Junio C Hamano
@ 2018-10-25 12:12       ` Johannes Schindelin
  1 sibling, 0 replies; 28+ messages in thread
From: Johannes Schindelin @ 2018-10-25 12:12 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Eric Sunshine, gitgitgadget, Git List, github

Hi Junio,

On Thu, 25 Oct 2018, Junio C Hamano wrote:

> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
> > On Mon, Oct 15, 2018 at 6:14 AM Brendan Forster via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> >> This config value is only used if http.sslBackend is set to "schannel",
> >> which forces cURL to use the Windows Certificate Store when validating
> >> server certificates associated with a remote server.
> >>
> >> This is only supported in cURL 7.44 or later.
> >> [...]
> >> Signed-off-by: Brendan Forster <github@brendanforster.com>
> >> ---
> >> diff --git a/http.c b/http.c
> >> @@ -811,6 +818,16 @@ static CURL *get_curl_handle(void)
> >> +       if (http_ssl_backend && !strcmp("schannel", http_ssl_backend) &&
> >> +           !http_schannel_check_revoke) {
> >> +#if LIBCURL_VERSION_NUM >= 0x072c00
> >> +               curl_easy_setopt(result, CURLOPT_SSL_OPTIONS, CURLSSLOPT_NO_REVOKE);
> >> +#else
> >> +               warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL options because\n"
> >> +                       "your curl version is too old (>= 7.44.0)");
> >
> > This message is confusing. If your curl is too old, shouldn't the ">=" be a "<"?
> 
> I do not think I saw any update to correct this, and worse yet I do
> not offhand recall if there was any other issue raised on the
> series.

Sorry, my bad. I dropped the ball. As you can see here:

	https://github.com/gitgitgadget/git/pull/46

I have some updates that are already pushed, but I still wanted to really
think through your response here:

	https://public-inbox.org/git/xmqq1s8oxbpc.fsf@gitster-ct.c.googlers.com/

and what I should do about it, before sending off v2. You can see that I
already updated the description in preparation for sending another
iteration.

I hope to get back to this tonight, for now I must scramble off to
non-work-related activities.

Ciao,
Dscho

> So assuming that this is the only remaining one, I'll squash the
> following to step 2/3 of this three-patch series and plan to merge
> it down to 'next' in the coming few days.
> 
> I have a clean-up suggestion related to this but is orthogonal to
> this three-patch series (after the fix-up is applied, anyway), which
> I'll be sending out separately.
> 
> Thanks.
> 
>  http.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/http.c b/http.c
> index 0ebf8f77a6..43e75ac583 100644
> --- a/http.c
> +++ b/http.c
> @@ -835,7 +835,7 @@ static CURL *get_curl_handle(void)
>  		curl_easy_setopt(result, CURLOPT_SSL_OPTIONS, CURLSSLOPT_NO_REVOKE);
>  #else
>  		warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL options because\n"
> -			"your curl version is too old (>= 7.44.0)");
> +			"your curl version is too old (< 7.44.0)");
>  #endif
>  	}
>  
> -- 
> 2.19.1-542-gc4df23f792
> 
> 

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

* Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL
  2018-10-18  1:53       ` Junio C Hamano
@ 2018-10-25 18:52         ` Johannes Schindelin
  2018-10-26  4:41           ` Junio C Hamano
  0 siblings, 1 reply; 28+ messages in thread
From: Johannes Schindelin @ 2018-10-25 18:52 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Brendan Forster via GitGitGadget, git, Brendan Forster

Hi Junio,

On Thu, 18 Oct 2018, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> In any case, you can use "http.<url>.$variable" to say "I want the
> >> http.$variable to be in effect but only when I am talking to <url>".
> >> Does it make sense for this new variable, too?  That is, does it
> >> benefit the users to be able to do something like this?
> >> 
> >>     [http] schannelCheckRevoke = no
> >>     [http "https://microsoft.com/"] schannelCheckRevoke = yes
> >> 
> >> I am guessing that the answer is yes.
> >
> > Frankly, I do not know.  Does it hurt, though?
> 
> I did not and I do not think it would.  I was wondering if the
> ability to be able to specify these per destination is something
> very useful and deserves to be called out in the doc, together with
> ...

I do not think that it needs to be called out specifically in the docs. It
is just yet another http.* setting that can be overridden per-URL. It
would be different if it had not worked.

> >> I guess the same comment applies to the previous step, but I suspect
> >> that the code structure may not allow us to switch the SSL backend
> >> so late in the game (e.g. "when talking to microsoft, use schannel,
> >> but when talking to github, use openssl").
> 
> ... this bit.
> 
> > Crucially, this fails. The short version is: this is good! Because it
> > means that Git used the OpenSSL backend, as clearly intended.
> >
> > <skip if="uninterested in the details">
> > Why does it fail?
> > ...
> > </skip>
> 
> So there may still be some polishing needed, but as long as people
> are not using the "per destination" thing, the code is already good?
> That is something we may want to document.

Actually, just because there is a peculiar bug in this particular code
flow in Git for Windows should not necessarily be interesting to Git's
commit history.

On Linux, for example, it would work.

So I don't think we need to mention anything about that unrelated bug.

Thanks,
Dscho

> Thanks.
> 

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

* [PATCH v2 0/3] Allow choosing the SSL backend cURL uses (plus related patches)
  2018-10-15 10:14 [PATCH 0/3] Allow choosing the SSL backend cURL uses (plus related patches) Johannes Schindelin via GitGitGadget
                   ` (2 preceding siblings ...)
  2018-10-15 10:14 ` [PATCH 3/3] http: when using Secure Channel, ignore sslCAInfo by default Johannes Schindelin via GitGitGadget
@ 2018-10-25 18:53 ` Johannes Schindelin via GitGitGadget
  2018-10-25 18:53   ` [PATCH v2 1/3] http: add support for selecting SSL backends at runtime Johannes Schindelin via GitGitGadget
                     ` (2 more replies)
  3 siblings, 3 replies; 28+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2018-10-25 18:53 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano

This topic branch brings support for choosing cURL's SSL backend (a feature
developed in Git for Windows' context) at runtime via http.sslBackend, and
two more patches that are related (and only of interest for Windows users).

Changes since v1:

 * Reworded the commit message of v1's patch 2/3, to talk about the original
   design instead of "an earlier iteration" that was never contributed to
   the Git mailing list.
 * Changed the confusing >= 7.44.0 to < 7.44.0.

Note: I had prepared 
https://github.com/dscho/git/commit/81e8c9a4006c919747a0b6a287f28f25799fcaf4
, intended to be included in v2, but Junio came up with 
https://public-inbox.org/git/xmqqsh0uln5c.fsf_-_@gitster-ct.c.googlers.com/ 
in the meantime, which I like better.

Brendan Forster (1):
  http: add support for disabling SSL revocation checks in cURL

Johannes Schindelin (2):
  http: add support for selecting SSL backends at runtime
  http: when using Secure Channel, ignore sslCAInfo by default

 Documentation/config.txt | 21 ++++++++++++
 http.c                   | 71 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 91 insertions(+), 1 deletion(-)


base-commit: 5a0cc8aca797dbd7d2be3b67458ff880ed45cddf
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-46%2Fdscho%2Fhttp-ssl-backend-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-46/dscho/http-ssl-backend-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/46

Range-diff vs v1:

 1:  8c5ecdb6c = 1:  85bd0fb27 http: add support for selecting SSL backends at runtime
 2:  764791d13 ! 2:  951383695 http: add support for disabling SSL revocation checks in cURL
     @@ -14,10 +14,10 @@
      
          This is only supported in cURL 7.44 or later.
      
     -    Note: an earlier iteration tried to use the config setting
     -    http.schannel.checkRevoke, but the http.* config settings can be limited
     -    to specific URLs via http.<url>.* (which would mistake `schannel` for a
     -    URL).
     +    Note: originally, we wanted to call the config setting
     +    `http.schannel.checkRevoke`. This, however, does not work: the `http.*`
     +    config settings can be limited to specific URLs via `http.<url>.*`
     +    (and this feature would mistake `schannel` for a URL).
      
          Helped by Agustín Martín Barbero.
      
     @@ -77,7 +77,7 @@
      +		curl_easy_setopt(result, CURLOPT_SSL_OPTIONS, CURLSSLOPT_NO_REVOKE);
      +#else
      +		warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL options because\n"
     -+			"your curl version is too old (>= 7.44.0)");
     ++			"your curl version is too old (< 7.44.0)");
      +#endif
      +	}
      +
 3:  9927e4ce6 = 3:  a5f937a36 http: when using Secure Channel, ignore sslCAInfo by default

-- 
gitgitgadget

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

* [PATCH v2 1/3] http: add support for selecting SSL backends at runtime
  2018-10-25 18:53 ` [PATCH v2 0/3] Allow choosing the SSL backend cURL uses (plus related patches) Johannes Schindelin via GitGitGadget
@ 2018-10-25 18:53   ` Johannes Schindelin via GitGitGadget
  2018-12-13  9:33     ` Ævar Arnfjörð Bjarmason
  2018-10-25 18:53   ` [PATCH v2 2/3] http: add support for disabling SSL revocation checks in cURL Brendan Forster via GitGitGadget
  2018-10-25 18:53   ` [PATCH v2 3/3] http: when using Secure Channel, ignore sslCAInfo by default Johannes Schindelin via GitGitGadget
  2 siblings, 1 reply; 28+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2018-10-25 18:53 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

As of version 7.56.0, curl supports being compiled with multiple SSL
backends.

This patch adds the Git side of that feature: by setting http.sslBackend
to "openssl" or "schannel", Git for Windows can now choose the SSL
backend at runtime.

This comes in handy on Windows because Secure Channel ("schannel") is
the native solution, accessing the Windows Credential Store, thereby
allowing for enterprise-wide management of certificates. For historical
reasons, Git for Windows needs to support OpenSSL still, as it has
previously been the only supported SSL backend in Git for Windows for
almost a decade.

The patch has been carried in Git for Windows for over a year, and is
considered mature.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/config.txt |  5 +++++
 http.c                   | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 154683321..7d38f0bf1 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1984,6 +1984,11 @@ http.sslCAPath::
 	with when fetching or pushing over HTTPS. Can be overridden
 	by the `GIT_SSL_CAPATH` environment variable.
 
+http.sslBackend::
+	Name of the SSL backend to use (e.g. "openssl" or "schannel").
+	This option is ignored if cURL lacks support for choosing the SSL
+	backend at runtime.
+
 http.pinnedpubkey::
 	Public key of the https service. It may either be the filename of
 	a PEM or DER encoded public key file or a string starting with
diff --git a/http.c b/http.c
index 98ff12258..7fb37a061 100644
--- a/http.c
+++ b/http.c
@@ -155,6 +155,8 @@ static struct active_request_slot *active_queue_head;
 
 static char *cached_accept_language;
 
+static char *http_ssl_backend;
+
 size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
 {
 	size_t size = eltsize * nmemb;
@@ -302,6 +304,12 @@ static int http_options(const char *var, const char *value, void *cb)
 		curl_ssl_try = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp("http.sslbackend", var)) {
+		free(http_ssl_backend);
+		http_ssl_backend = xstrdup_or_null(value);
+		return 0;
+	}
+
 	if (!strcmp("http.minsessions", var)) {
 		min_curl_sessions = git_config_int(var, value);
 #ifndef USE_CURL_MULTI
@@ -995,6 +1003,33 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
 	git_config(urlmatch_config_entry, &config);
 	free(normalized_url);
 
+#if LIBCURL_VERSION_NUM >= 0x073800
+	if (http_ssl_backend) {
+		const curl_ssl_backend **backends;
+		struct strbuf buf = STRBUF_INIT;
+		int i;
+
+		switch (curl_global_sslset(-1, http_ssl_backend, &backends)) {
+		case CURLSSLSET_UNKNOWN_BACKEND:
+			strbuf_addf(&buf, _("Unsupported SSL backend '%s'. "
+					    "Supported SSL backends:"),
+					    http_ssl_backend);
+			for (i = 0; backends[i]; i++)
+				strbuf_addf(&buf, "\n\t%s", backends[i]->name);
+			die("%s", buf.buf);
+		case CURLSSLSET_NO_BACKENDS:
+			die(_("Could not set SSL backend to '%s': "
+			      "cURL was built without SSL backends"),
+			    http_ssl_backend);
+		case CURLSSLSET_TOO_LATE:
+			die(_("Could not set SSL backend to '%s': already set"),
+			    http_ssl_backend);
+		case CURLSSLSET_OK:
+			break; /* Okay! */
+		}
+	}
+#endif
+
 	if (curl_global_init(CURL_GLOBAL_ALL) != CURLE_OK)
 		die("curl_global_init failed");
 
-- 
gitgitgadget


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

* [PATCH v2 2/3] http: add support for disabling SSL revocation checks in cURL
  2018-10-25 18:53 ` [PATCH v2 0/3] Allow choosing the SSL backend cURL uses (plus related patches) Johannes Schindelin via GitGitGadget
  2018-10-25 18:53   ` [PATCH v2 1/3] http: add support for selecting SSL backends at runtime Johannes Schindelin via GitGitGadget
@ 2018-10-25 18:53   ` Brendan Forster via GitGitGadget
  2018-10-25 18:53   ` [PATCH v2 3/3] http: when using Secure Channel, ignore sslCAInfo by default Johannes Schindelin via GitGitGadget
  2 siblings, 0 replies; 28+ messages in thread
From: Brendan Forster via GitGitGadget @ 2018-10-25 18:53 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Brendan Forster

From: Brendan Forster <github@brendanforster.com>

This adds support for a new http.schannelCheckRevoke config value.

This config value is only used if http.sslBackend is set to "schannel",
which forces cURL to use the Windows Certificate Store when validating
server certificates associated with a remote server.

This config value should only be set to "false" if you are in an
environment where revocation checks are blocked by the network, with
no alternative options.

This is only supported in cURL 7.44 or later.

Note: originally, we wanted to call the config setting
`http.schannel.checkRevoke`. This, however, does not work: the `http.*`
config settings can be limited to specific URLs via `http.<url>.*`
(and this feature would mistake `schannel` for a URL).

Helped by Agustín Martín Barbero.

Signed-off-by: Brendan Forster <github@brendanforster.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/config.txt |  8 ++++++++
 http.c                   | 17 +++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 7d38f0bf1..d569ebd49 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1989,6 +1989,14 @@ http.sslBackend::
 	This option is ignored if cURL lacks support for choosing the SSL
 	backend at runtime.
 
+http.schannelCheckRevoke::
+	Used to enforce or disable certificate revocation checks in cURL
+	when http.sslBackend is set to "schannel". Defaults to `true` if
+	unset. Only necessary to disable this if Git consistently errors
+	and the message is about checking the revocation status of a
+	certificate. This option is ignored if cURL lacks support for
+	setting the relevant SSL option at runtime.
+
 http.pinnedpubkey::
 	Public key of the https service. It may either be the filename of
 	a PEM or DER encoded public key file or a string starting with
diff --git a/http.c b/http.c
index 7fb37a061..65daa9bfa 100644
--- a/http.c
+++ b/http.c
@@ -157,6 +157,8 @@ static char *cached_accept_language;
 
 static char *http_ssl_backend;
 
+static int http_schannel_check_revoke = 1;
+
 size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
 {
 	size_t size = eltsize * nmemb;
@@ -310,6 +312,11 @@ static int http_options(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (!strcmp("http.schannelcheckrevoke", var)) {
+		http_schannel_check_revoke = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (!strcmp("http.minsessions", var)) {
 		min_curl_sessions = git_config_int(var, value);
 #ifndef USE_CURL_MULTI
@@ -811,6 +818,16 @@ static CURL *get_curl_handle(void)
 	}
 #endif
 
+	if (http_ssl_backend && !strcmp("schannel", http_ssl_backend) &&
+	    !http_schannel_check_revoke) {
+#if LIBCURL_VERSION_NUM >= 0x072c00
+		curl_easy_setopt(result, CURLOPT_SSL_OPTIONS, CURLSSLOPT_NO_REVOKE);
+#else
+		warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL options because\n"
+			"your curl version is too old (< 7.44.0)");
+#endif
+	}
+
 	if (http_proactive_auth)
 		init_curl_http_auth(result);
 
-- 
gitgitgadget


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

* [PATCH v2 3/3] http: when using Secure Channel, ignore sslCAInfo by default
  2018-10-25 18:53 ` [PATCH v2 0/3] Allow choosing the SSL backend cURL uses (plus related patches) Johannes Schindelin via GitGitGadget
  2018-10-25 18:53   ` [PATCH v2 1/3] http: add support for selecting SSL backends at runtime Johannes Schindelin via GitGitGadget
  2018-10-25 18:53   ` [PATCH v2 2/3] http: add support for disabling SSL revocation checks in cURL Brendan Forster via GitGitGadget
@ 2018-10-25 18:53   ` Johannes Schindelin via GitGitGadget
  2 siblings, 0 replies; 28+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2018-10-25 18:53 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

As of cURL v7.60.0, the Secure Channel backend can use the certificate
bundle provided via `http.sslCAInfo`, but that would override the
Windows Certificate Store. Since this is not desirable by default, let's
tell Git to not ask cURL to use that bundle by default when the `schannel`
backend was configured via `http.sslBackend`, unless
`http.schannelUseSSLCAInfo` overrides this behavior.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/config.txt |  8 ++++++++
 http.c                   | 19 ++++++++++++++++++-
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index d569ebd49..1f6a6a4a6 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1997,6 +1997,14 @@ http.schannelCheckRevoke::
 	certificate. This option is ignored if cURL lacks support for
 	setting the relevant SSL option at runtime.
 
+http.schannelUseSSLCAInfo::
+	As of cURL v7.60.0, the Secure Channel backend can use the
+	certificate bundle provided via `http.sslCAInfo`, but that would
+	override the Windows Certificate Store. Since this is not desirable
+	by default, Git will tell cURL not to use that bundle by default
+	when the `schannel` backend was configured via `http.sslBackend`,
+	unless `http.schannelUseSSLCAInfo` overrides this behavior.
+
 http.pinnedpubkey::
 	Public key of the https service. It may either be the filename of
 	a PEM or DER encoded public key file or a string starting with
diff --git a/http.c b/http.c
index 65daa9bfa..28009ca73 100644
--- a/http.c
+++ b/http.c
@@ -158,6 +158,12 @@ static char *cached_accept_language;
 static char *http_ssl_backend;
 
 static int http_schannel_check_revoke = 1;
+/*
+ * With the backend being set to `schannel`, setting sslCAinfo would override
+ * the Certificate Store in cURL v7.60.0 and later, which is not what we want
+ * by default.
+ */
+static int http_schannel_use_ssl_cainfo;
 
 size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
 {
@@ -317,6 +323,11 @@ static int http_options(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (!strcmp("http.schannelusesslcainfo", var)) {
+		http_schannel_use_ssl_cainfo = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (!strcmp("http.minsessions", var)) {
 		min_curl_sessions = git_config_int(var, value);
 #ifndef USE_CURL_MULTI
@@ -869,7 +880,13 @@ static CURL *get_curl_handle(void)
 	if (ssl_pinnedkey != NULL)
 		curl_easy_setopt(result, CURLOPT_PINNEDPUBLICKEY, ssl_pinnedkey);
 #endif
-	if (ssl_cainfo != NULL)
+	if (http_ssl_backend && !strcmp("schannel", http_ssl_backend) &&
+	    !http_schannel_use_ssl_cainfo) {
+		curl_easy_setopt(result, CURLOPT_CAINFO, NULL);
+#if LIBCURL_VERSION_NUM >= 0x073400
+		curl_easy_setopt(result, CURLOPT_PROXY_CAINFO, NULL);
+#endif
+	} else if (ssl_cainfo != NULL)
 		curl_easy_setopt(result, CURLOPT_CAINFO, ssl_cainfo);
 
 	if (curl_low_speed_limit > 0 && curl_low_speed_time > 0) {
-- 
gitgitgadget

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

* Re: [PATCH] http: give curl version warnings consistently
  2018-10-25  3:29       ` [PATCH] http: give curl version warnings consistently Junio C Hamano
  2018-10-25  6:23         ` Jeff King
@ 2018-10-25 19:00         ` Johannes Schindelin
  2018-10-26  4:39           ` Junio C Hamano
  1 sibling, 1 reply; 28+ messages in thread
From: Johannes Schindelin @ 2018-10-25 19:00 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git List, Eric Sunshine, github

Hi Junio,

On Thu, 25 Oct 2018, Junio C Hamano wrote:

> When a requested feature cannot be activated because the version of
> cURL library used to build Git with is too old, most of the codepaths
> give a warning like "$Feature is not supported with cURL < $Version",
> marked for l10n.  A few of them, however, did not follow that pattern
> and said things like "$Feature is not activated, your curl version is
> too old (>= $Version)", and without marking them for l10n.
> 
> Update these to match the style of the majority of warnings and mark
> them for l10n.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---

I like this patch better than the one I had prepared for v2, so I dropped
it again, and "hit the Submit button".

Ciao,
Dscho

> 
>  > I have a clean-up suggestion related to this but is orthogonal to
>  > this three-patch series (after the fix-up is applied, anyway), which
>  > I'll be sending out separately.
> 
>  So, here it is.
> 
>  http.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/http.c b/http.c
> index 43e75ac583..2214100e3b 100644
> --- a/http.c
> +++ b/http.c
> @@ -834,8 +834,7 @@ static CURL *get_curl_handle(void)
>  #if LIBCURL_VERSION_NUM >= 0x072c00
>  		curl_easy_setopt(result, CURLOPT_SSL_OPTIONS, CURLSSLOPT_NO_REVOKE);
>  #else
> -		warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL options because\n"
> -			"your curl version is too old (< 7.44.0)");
> +		warning(_("CURLSSLOPT_NO_REVOKE not suported with cURL < 7.44.0"));
>  #endif
>  	}
>  
> @@ -908,8 +907,7 @@ static CURL *get_curl_handle(void)
>  	curl_easy_setopt(result, CURLOPT_PROTOCOLS,
>  			 get_curl_allowed_protocols(-1));
>  #else
> -	warning("protocol restrictions not applied to curl redirects because\n"
> -		"your curl version is too old (>= 7.19.4)");
> +	warning(_("Protocol restrictions not supported with cURL < 7.19.4"));
>  #endif
>  	if (getenv("GIT_CURL_VERBOSE"))
>  		curl_easy_setopt(result, CURLOPT_VERBOSE, 1L);
> -- 
> 2.19.1-542-gc4df23f792
> 
> 

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

* Re: [PATCH] http: give curl version warnings consistently
  2018-10-25 19:00         ` Johannes Schindelin
@ 2018-10-26  4:39           ` Junio C Hamano
  0 siblings, 0 replies; 28+ messages in thread
From: Junio C Hamano @ 2018-10-26  4:39 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Git List, Eric Sunshine, github

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Thu, 25 Oct 2018, Junio C Hamano wrote:
>
>> When a requested feature cannot be activated because the version of
>> cURL library used to build Git with is too old, most of the codepaths
>> give a warning like "$Feature is not supported with cURL < $Version",
>> marked for l10n.  A few of them, however, did not follow that pattern
>> and said things like "$Feature is not activated, your curl version is
>> too old (>= $Version)", and without marking them for l10n.
>> 
>> Update these to match the style of the majority of warnings and mark
>> them for l10n.
>> 
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>
> I like this patch better than the one I had prepared for v2, so I dropped
> it again, and "hit the Submit button".

I took your v3 and queue this on top, instead of the previous one
on which this was prepared.

By the way, I wondered if we want to unify them by introducing

	static void curl_version_warning(const char *feature, const char *verstring)
	{
		warning(_("%s is not supported with cURL < %s"),
			feature, verstring);
	}

so that translators need to deal with a single instance of the
message, but the "feature" part may have to get localized, in which
case we'd end up with sentence lego, which is not a good idea, so I
dropped it.

Thanks.

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

* Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL
  2018-10-25 18:52         ` Johannes Schindelin
@ 2018-10-26  4:41           ` Junio C Hamano
  0 siblings, 0 replies; 28+ messages in thread
From: Junio C Hamano @ 2018-10-26  4:41 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Brendan Forster via GitGitGadget, git, Brendan Forster

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> I did not and I do not think it would.  I was wondering if the
>> ability to be able to specify these per destination is something
>> very useful and deserves to be called out in the doc, together with
>> ...
>
> I do not think that it needs to be called out specifically in the docs. It
> is just yet another http.* setting that can be overridden per-URL. It
> would be different if it had not worked.

OK, thanks for sanity checking.

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

* Re: [PATCH v2 1/3] http: add support for selecting SSL backends at runtime
  2018-10-25 18:53   ` [PATCH v2 1/3] http: add support for selecting SSL backends at runtime Johannes Schindelin via GitGitGadget
@ 2018-12-13  9:33     ` Ævar Arnfjörð Bjarmason
  2018-12-13 13:08       ` Johannes Schindelin
  0 siblings, 1 reply; 28+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-12-13  9:33 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget
  Cc: git, Junio C Hamano, Johannes Schindelin


On Thu, Oct 25 2018, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> As of version 7.56.0, curl supports being compiled with multiple SSL
> backends.
>
> This patch adds the Git side of that feature: by setting http.sslBackend
> to "openssl" or "schannel", Git for Windows can now choose the SSL
> backend at runtime.
>
> This comes in handy on Windows because Secure Channel ("schannel") is
> the native solution, accessing the Windows Credential Store, thereby
> allowing for enterprise-wide management of certificates. For historical
> reasons, Git for Windows needs to support OpenSSL still, as it has
> previously been the only supported SSL backend in Git for Windows for
> almost a decade.
>
> The patch has been carried in Git for Windows for over a year, and is
> considered mature.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  Documentation/config.txt |  5 +++++
>  http.c                   | 35 +++++++++++++++++++++++++++++++++++
>  2 files changed, 40 insertions(+)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 154683321..7d38f0bf1 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1984,6 +1984,11 @@ http.sslCAPath::
>  	with when fetching or pushing over HTTPS. Can be overridden
>  	by the `GIT_SSL_CAPATH` environment variable.
>
> +http.sslBackend::
> +	Name of the SSL backend to use (e.g. "openssl" or "schannel").
> +	This option is ignored if cURL lacks support for choosing the SSL
> +	backend at runtime.
> +
>  http.pinnedpubkey::
>  	Public key of the https service. It may either be the filename of
>  	a PEM or DER encoded public key file or a string starting with
> diff --git a/http.c b/http.c
> index 98ff12258..7fb37a061 100644
> --- a/http.c
> +++ b/http.c
> @@ -155,6 +155,8 @@ static struct active_request_slot *active_queue_head;
>
>  static char *cached_accept_language;
>
> +static char *http_ssl_backend;
> +
>  size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
>  {
>  	size_t size = eltsize * nmemb;
> @@ -302,6 +304,12 @@ static int http_options(const char *var, const char *value, void *cb)
>  		curl_ssl_try = git_config_bool(var, value);
>  		return 0;
>  	}
> +	if (!strcmp("http.sslbackend", var)) {
> +		free(http_ssl_backend);
> +		http_ssl_backend = xstrdup_or_null(value);
> +		return 0;
> +	}
> +
>  	if (!strcmp("http.minsessions", var)) {
>  		min_curl_sessions = git_config_int(var, value);
>  #ifndef USE_CURL_MULTI
> @@ -995,6 +1003,33 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
>  	git_config(urlmatch_config_entry, &config);
>  	free(normalized_url);
>
> +#if LIBCURL_VERSION_NUM >= 0x073800
> +	if (http_ssl_backend) {
> +		const curl_ssl_backend **backends;
> +		struct strbuf buf = STRBUF_INIT;
> +		int i;
> +
> +		switch (curl_global_sslset(-1, http_ssl_backend, &backends)) {
> +		case CURLSSLSET_UNKNOWN_BACKEND:
> +			strbuf_addf(&buf, _("Unsupported SSL backend '%s'. "
> +					    "Supported SSL backends:"),
> +					    http_ssl_backend);
> +			for (i = 0; backends[i]; i++)
> +				strbuf_addf(&buf, "\n\t%s", backends[i]->name);
> +			die("%s", buf.buf);
> +		case CURLSSLSET_NO_BACKENDS:
> +			die(_("Could not set SSL backend to '%s': "
> +			      "cURL was built without SSL backends"),
> +			    http_ssl_backend);
> +		case CURLSSLSET_TOO_LATE:
> +			die(_("Could not set SSL backend to '%s': already set"),
> +			    http_ssl_backend);
> +		case CURLSSLSET_OK:
> +			break; /* Okay! */
> +		}
> +	}
> +#endif
> +
>  	if (curl_global_init(CURL_GLOBAL_ALL) != CURLE_OK)
>  		die("curl_global_init failed");

Here's someone who upgraded to 2.20 on Arch linux & started getting
"Could not set..." errors because of this change:
https://www.reddit.com/r/git/comments/a5ne5v/git_fatal_could_not_set_ssl_backend_to_openssl/

I don't know the context well enough, but is there perhaps enough info
here so we could give a better error message, e.g. "don't set xyz twice
in your config", or just emit a warning?

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

* Re: [PATCH v2 1/3] http: add support for selecting SSL backends at runtime
  2018-12-13  9:33     ` Ævar Arnfjörð Bjarmason
@ 2018-12-13 13:08       ` Johannes Schindelin
  2018-12-13 13:15         ` Johannes Schindelin
  0 siblings, 1 reply; 28+ messages in thread
From: Johannes Schindelin @ 2018-12-13 13:08 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Johannes Schindelin via GitGitGadget, git, Junio C Hamano

[-- Attachment #1: Type: text/plain, Size: 5116 bytes --]

Hi Ævar,

On Thu, 13 Dec 2018, Ævar Arnfjörð Bjarmason wrote:

> On Thu, Oct 25 2018, Johannes Schindelin via GitGitGadget wrote:
> 
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > As of version 7.56.0, curl supports being compiled with multiple SSL
> > backends.
> >
> > This patch adds the Git side of that feature: by setting http.sslBackend
> > to "openssl" or "schannel", Git for Windows can now choose the SSL
> > backend at runtime.
> >
> > This comes in handy on Windows because Secure Channel ("schannel") is
> > the native solution, accessing the Windows Credential Store, thereby
> > allowing for enterprise-wide management of certificates. For historical
> > reasons, Git for Windows needs to support OpenSSL still, as it has
> > previously been the only supported SSL backend in Git for Windows for
> > almost a decade.
> >
> > The patch has been carried in Git for Windows for over a year, and is
> > considered mature.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  Documentation/config.txt |  5 +++++
> >  http.c                   | 35 +++++++++++++++++++++++++++++++++++
> >  2 files changed, 40 insertions(+)
> >
> > diff --git a/Documentation/config.txt b/Documentation/config.txt
> > index 154683321..7d38f0bf1 100644
> > --- a/Documentation/config.txt
> > +++ b/Documentation/config.txt
> > @@ -1984,6 +1984,11 @@ http.sslCAPath::
> >  	with when fetching or pushing over HTTPS. Can be overridden
> >  	by the `GIT_SSL_CAPATH` environment variable.
> >
> > +http.sslBackend::
> > +	Name of the SSL backend to use (e.g. "openssl" or "schannel").
> > +	This option is ignored if cURL lacks support for choosing the SSL
> > +	backend at runtime.
> > +
> >  http.pinnedpubkey::
> >  	Public key of the https service. It may either be the filename of
> >  	a PEM or DER encoded public key file or a string starting with
> > diff --git a/http.c b/http.c
> > index 98ff12258..7fb37a061 100644
> > --- a/http.c
> > +++ b/http.c
> > @@ -155,6 +155,8 @@ static struct active_request_slot *active_queue_head;
> >
> >  static char *cached_accept_language;
> >
> > +static char *http_ssl_backend;
> > +
> >  size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
> >  {
> >  	size_t size = eltsize * nmemb;
> > @@ -302,6 +304,12 @@ static int http_options(const char *var, const char *value, void *cb)
> >  		curl_ssl_try = git_config_bool(var, value);
> >  		return 0;
> >  	}
> > +	if (!strcmp("http.sslbackend", var)) {
> > +		free(http_ssl_backend);
> > +		http_ssl_backend = xstrdup_or_null(value);
> > +		return 0;
> > +	}
> > +
> >  	if (!strcmp("http.minsessions", var)) {
> >  		min_curl_sessions = git_config_int(var, value);
> >  #ifndef USE_CURL_MULTI
> > @@ -995,6 +1003,33 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
> >  	git_config(urlmatch_config_entry, &config);
> >  	free(normalized_url);
> >
> > +#if LIBCURL_VERSION_NUM >= 0x073800
> > +	if (http_ssl_backend) {
> > +		const curl_ssl_backend **backends;
> > +		struct strbuf buf = STRBUF_INIT;
> > +		int i;
> > +
> > +		switch (curl_global_sslset(-1, http_ssl_backend, &backends)) {
> > +		case CURLSSLSET_UNKNOWN_BACKEND:
> > +			strbuf_addf(&buf, _("Unsupported SSL backend '%s'. "
> > +					    "Supported SSL backends:"),
> > +					    http_ssl_backend);
> > +			for (i = 0; backends[i]; i++)
> > +				strbuf_addf(&buf, "\n\t%s", backends[i]->name);
> > +			die("%s", buf.buf);
> > +		case CURLSSLSET_NO_BACKENDS:
> > +			die(_("Could not set SSL backend to '%s': "
> > +			      "cURL was built without SSL backends"),
> > +			    http_ssl_backend);
> > +		case CURLSSLSET_TOO_LATE:
> > +			die(_("Could not set SSL backend to '%s': already set"),
> > +			    http_ssl_backend);
> > +		case CURLSSLSET_OK:
> > +			break; /* Okay! */
> > +		}
> > +	}
> > +#endif
> > +
> >  	if (curl_global_init(CURL_GLOBAL_ALL) != CURLE_OK)
> >  		die("curl_global_init failed");
> 
> Here's someone who upgraded to 2.20 on Arch linux & started getting
> "Could not set..." errors because of this change:
> https://www.reddit.com/r/git/comments/a5ne5v/git_fatal_could_not_set_ssl_backend_to_openssl/

Yeah, I don't see bug reports that were opened via Reddit.

> I don't know the context well enough, but is there perhaps enough info
> here so we could give a better error message, e.g. "don't set xyz twice
> in your config", or just emit a warning?

This is actually not the symptom of a Git bug, but of a cURL bug that I
fixed in https://github.com/curl/curl/pull/3346. I suspect the fix for
this particular symptom to be
https://github.com/curl/curl/commit/231a328c1c563acb53d8222894975e96bf7e6ea7

(Granted, I introduced that bug, and did not catch it earlier because I
almost never build cURL with a single TLS backend these days, and that is
necessary to trigger the bug.)

According to https://curl.haxx.se/changes.html, this bug fix
(https://curl.haxx.se/bug/?i=3346) made it into v7.63.0, which is one day
old.

Feel free to update that Reddit post (I don't have an account, nor any
inclination to get one).

Ciao,
Dscho

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

* Re: [PATCH v2 1/3] http: add support for selecting SSL backends at runtime
  2018-12-13 13:08       ` Johannes Schindelin
@ 2018-12-13 13:15         ` Johannes Schindelin
  0 siblings, 0 replies; 28+ messages in thread
From: Johannes Schindelin @ 2018-12-13 13:15 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Johannes Schindelin via GitGitGadget, git, Junio C Hamano

[-- Attachment #1: Type: text/plain, Size: 6083 bytes --]

Hi,

On Thu, 13 Dec 2018, Johannes Schindelin wrote:

> On Thu, 13 Dec 2018, Ævar Arnfjörð Bjarmason wrote:
> 
> > On Thu, Oct 25 2018, Johannes Schindelin via GitGitGadget wrote:
> > 
> > > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > >
> > > As of version 7.56.0, curl supports being compiled with multiple SSL
> > > backends.
> > >
> > > This patch adds the Git side of that feature: by setting http.sslBackend
> > > to "openssl" or "schannel", Git for Windows can now choose the SSL
> > > backend at runtime.
> > >
> > > This comes in handy on Windows because Secure Channel ("schannel") is
> > > the native solution, accessing the Windows Credential Store, thereby
> > > allowing for enterprise-wide management of certificates. For historical
> > > reasons, Git for Windows needs to support OpenSSL still, as it has
> > > previously been the only supported SSL backend in Git for Windows for
> > > almost a decade.
> > >
> > > The patch has been carried in Git for Windows for over a year, and is
> > > considered mature.
> > >
> > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > > ---
> > >  Documentation/config.txt |  5 +++++
> > >  http.c                   | 35 +++++++++++++++++++++++++++++++++++
> > >  2 files changed, 40 insertions(+)
> > >
> > > diff --git a/Documentation/config.txt b/Documentation/config.txt
> > > index 154683321..7d38f0bf1 100644
> > > --- a/Documentation/config.txt
> > > +++ b/Documentation/config.txt
> > > @@ -1984,6 +1984,11 @@ http.sslCAPath::
> > >  	with when fetching or pushing over HTTPS. Can be overridden
> > >  	by the `GIT_SSL_CAPATH` environment variable.
> > >
> > > +http.sslBackend::
> > > +	Name of the SSL backend to use (e.g. "openssl" or "schannel").
> > > +	This option is ignored if cURL lacks support for choosing the SSL
> > > +	backend at runtime.
> > > +
> > >  http.pinnedpubkey::
> > >  	Public key of the https service. It may either be the filename of
> > >  	a PEM or DER encoded public key file or a string starting with
> > > diff --git a/http.c b/http.c
> > > index 98ff12258..7fb37a061 100644
> > > --- a/http.c
> > > +++ b/http.c
> > > @@ -155,6 +155,8 @@ static struct active_request_slot *active_queue_head;
> > >
> > >  static char *cached_accept_language;
> > >
> > > +static char *http_ssl_backend;
> > > +
> > >  size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
> > >  {
> > >  	size_t size = eltsize * nmemb;
> > > @@ -302,6 +304,12 @@ static int http_options(const char *var, const char *value, void *cb)
> > >  		curl_ssl_try = git_config_bool(var, value);
> > >  		return 0;
> > >  	}
> > > +	if (!strcmp("http.sslbackend", var)) {
> > > +		free(http_ssl_backend);
> > > +		http_ssl_backend = xstrdup_or_null(value);
> > > +		return 0;
> > > +	}
> > > +
> > >  	if (!strcmp("http.minsessions", var)) {
> > >  		min_curl_sessions = git_config_int(var, value);
> > >  #ifndef USE_CURL_MULTI
> > > @@ -995,6 +1003,33 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
> > >  	git_config(urlmatch_config_entry, &config);
> > >  	free(normalized_url);
> > >
> > > +#if LIBCURL_VERSION_NUM >= 0x073800
> > > +	if (http_ssl_backend) {
> > > +		const curl_ssl_backend **backends;
> > > +		struct strbuf buf = STRBUF_INIT;
> > > +		int i;
> > > +
> > > +		switch (curl_global_sslset(-1, http_ssl_backend, &backends)) {
> > > +		case CURLSSLSET_UNKNOWN_BACKEND:
> > > +			strbuf_addf(&buf, _("Unsupported SSL backend '%s'. "
> > > +					    "Supported SSL backends:"),
> > > +					    http_ssl_backend);
> > > +			for (i = 0; backends[i]; i++)
> > > +				strbuf_addf(&buf, "\n\t%s", backends[i]->name);
> > > +			die("%s", buf.buf);
> > > +		case CURLSSLSET_NO_BACKENDS:
> > > +			die(_("Could not set SSL backend to '%s': "
> > > +			      "cURL was built without SSL backends"),
> > > +			    http_ssl_backend);
> > > +		case CURLSSLSET_TOO_LATE:
> > > +			die(_("Could not set SSL backend to '%s': already set"),
> > > +			    http_ssl_backend);
> > > +		case CURLSSLSET_OK:
> > > +			break; /* Okay! */
> > > +		}
> > > +	}
> > > +#endif
> > > +
> > >  	if (curl_global_init(CURL_GLOBAL_ALL) != CURLE_OK)
> > >  		die("curl_global_init failed");
> > 
> > Here's someone who upgraded to 2.20 on Arch linux & started getting
> > "Could not set..." errors because of this change:
> > https://www.reddit.com/r/git/comments/a5ne5v/git_fatal_could_not_set_ssl_backend_to_openssl/
> 
> Yeah, I don't see bug reports that were opened via Reddit.
> 
> > I don't know the context well enough, but is there perhaps enough info
> > here so we could give a better error message, e.g. "don't set xyz twice
> > in your config", or just emit a warning?
> 
> This is actually not the symptom of a Git bug, but of a cURL bug that I
> fixed in https://github.com/curl/curl/pull/3346. I suspect the fix for
> this particular symptom to be
> https://github.com/curl/curl/commit/231a328c1c563acb53d8222894975e96bf7e6ea7

I should actually talk about that symptom a bit more so you understand
where it comes from: the idea of cURL when compiled with multiple TLS
backends is that it has a meta backend that, upon first call, will see
which backend to use and then plugs that.

When compiled with a single backend, that meta backend is not plugged at
all, the single backend is plugged by default.

And here lies the rub, when *now* trying to select a TLS backend *by
name*, the code incorrectly reported an error (instead of success in case
that the correct backend was already "selected", or *another* failure if
trying to select a backend that was not compiled in).

Ciao,
Dscho

> (Granted, I introduced that bug, and did not catch it earlier because I
> almost never build cURL with a single TLS backend these days, and that is
> necessary to trigger the bug.)
> 
> According to https://curl.haxx.se/changes.html, this bug fix
> (https://curl.haxx.se/bug/?i=3346) made it into v7.63.0, which is one day
> old.
> 
> Feel free to update that Reddit post (I don't have an account, nor any
> inclination to get one).
> 
> Ciao,
> Dscho

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

end of thread, other threads:[~2018-12-13 13:16 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-15 10:14 [PATCH 0/3] Allow choosing the SSL backend cURL uses (plus related patches) Johannes Schindelin via GitGitGadget
2018-10-15 10:14 ` [PATCH 1/3] http: add support for selecting SSL backends at runtime Johannes Schindelin via GitGitGadget
2018-10-15 14:06   ` Eric Sunshine
2018-10-15 10:14 ` [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL Brendan Forster via GitGitGadget
2018-10-15 14:10   ` Eric Sunshine
2018-10-16 12:21     ` Johannes Schindelin
2018-10-25  3:18     ` Junio C Hamano
2018-10-25  3:29       ` [PATCH] http: give curl version warnings consistently Junio C Hamano
2018-10-25  6:23         ` Jeff King
2018-10-25 19:00         ` Johannes Schindelin
2018-10-26  4:39           ` Junio C Hamano
2018-10-25 12:12       ` [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL Johannes Schindelin
2018-10-16  4:23   ` Junio C Hamano
2018-10-16  6:33     ` Jeff King
2018-10-16 12:25       ` Johannes Schindelin
2018-10-16 15:28         ` Jeff King
2018-10-16 12:22     ` Johannes Schindelin
2018-10-18  1:53       ` Junio C Hamano
2018-10-25 18:52         ` Johannes Schindelin
2018-10-26  4:41           ` Junio C Hamano
2018-10-15 10:14 ` [PATCH 3/3] http: when using Secure Channel, ignore sslCAInfo by default Johannes Schindelin via GitGitGadget
2018-10-25 18:53 ` [PATCH v2 0/3] Allow choosing the SSL backend cURL uses (plus related patches) Johannes Schindelin via GitGitGadget
2018-10-25 18:53   ` [PATCH v2 1/3] http: add support for selecting SSL backends at runtime Johannes Schindelin via GitGitGadget
2018-12-13  9:33     ` Ævar Arnfjörð Bjarmason
2018-12-13 13:08       ` Johannes Schindelin
2018-12-13 13:15         ` Johannes Schindelin
2018-10-25 18:53   ` [PATCH v2 2/3] http: add support for disabling SSL revocation checks in cURL Brendan Forster via GitGitGadget
2018-10-25 18:53   ` [PATCH v2 3/3] http: when using Secure Channel, ignore sslCAInfo by default Johannes Schindelin via GitGitGadget

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).