[2/3] http: add support for disabling SSL revocation checks in cURL

Commit Message

Linus Arver via GitGitGadget Oct. 15, 2018, 10:14 a.m. UTC

From: Brendan Forster <[e-mail]>

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 <[e-mail]>
Signed-off-by: Johannes Schindelin <[e-mail]>
---
 Documentation/config.txt |  8 ++++++++
 http.c                   | 17 +++++++++++++++++
 2 files changed, 25 insertions(+)

Comments

Eric Sunshine Oct. 15, 2018, 2:10 p.m. UTC | #1

On Mon, Oct 15, 2018 at 6:14 AM Brendan Forster via GitGitGadget
<[e-mail]> 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 <[e-mail]>
> ---
> 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
> +       }

Junio C Hamano Oct. 16, 2018, 4:23 a.m. UTC | #2

"Brendan Forster via GitGitGadget" <[e-mail]> 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);

Jeff King Oct. 16, 2018, 6:33 a.m. UTC | #3

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

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

-Peff

Johannes Schindelin Oct. 16, 2018, 12:21 p.m. UTC | #4

Hi Eric,


On Mon, 15 Oct 2018, Eric Sunshine wrote:

> On Mon, Oct 15, 2018 at 6:14 AM Brendan Forster via GitGitGadget
> <[e-mail]> 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 <[e-mail]>
> > ---
> > 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
> > +       }
>

Johannes Schindelin Oct. 16, 2018, 12:22 p.m. UTC | #5

Hi Junio,

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

> "Brendan Forster via GitGitGadget" <[e-mail]> 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);
>

Johannes Schindelin Oct. 16, 2018, 12:25 p.m. UTC | #6

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

Jeff King Oct. 16, 2018, 3:28 p.m. UTC | #7

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

Junio C Hamano Oct. 18, 2018, 1:53 a.m. UTC | #8

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

Junio C Hamano Oct. 25, 2018, 3:18 a.m. UTC | #9

Eric Sunshine <[e-mail]> writes:

> On Mon, Oct 15, 2018 at 6:14 AM Brendan Forster via GitGitGadget
> <[e-mail]> 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 <[e-mail]>
>> ---
>> 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
 	}

Johannes Schindelin Oct. 25, 2018, 12:12 p.m. UTC | #10

Hi Junio,

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

> Eric Sunshine <[e-mail]> writes:
> 
> > On Mon, Oct 15, 2018 at 6:14 AM Brendan Forster via GitGitGadget
> > <[e-mail]> 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 <[e-mail]>
> >> ---
> >> 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/[e-mail]/

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
> 
>

Johannes Schindelin Oct. 25, 2018, 6:52 p.m. UTC | #11

Hi Junio,

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

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

Junio C Hamano Oct. 26, 2018, 4:41 a.m. UTC | #12

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

Patch

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