[v2,1/3] http: add support for selecting SSL backends at runtime

Commit Message

Johannes Schindelin via GitGitGadget Oct. 25, 2018, 6:53 p.m. UTC

From: Johannes Schindelin <[e-mail]>

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 <[e-mail]>
---
 Documentation/config.txt |  5 +++++
 http.c                   | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

Comments

Ævar Arnfjörð Bjarmason Dec. 13, 2018, 9:33 a.m. UTC | #1

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

> From: Johannes Schindelin <[e-mail]>
>
> 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 <[e-mail]>
> ---
>  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?

Johannes Schindelin Dec. 13, 2018, 1:08 p.m. UTC | #2

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 <[e-mail]>
> >
> > 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 <[e-mail]>
> > ---
> >  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

Johannes Schindelin Dec. 13, 2018, 1:15 p.m. UTC | #3

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 <[e-mail]>
> > >
> > > 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 <[e-mail]>
> > > ---
> > >  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

Patch

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");