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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Oct 16, 2018, 12:22 UTC
Message-ID: <nycvar.QRO.7.76.6.1810161225310.4546@tvgsbejvaqbjf.bet>
In-Reply-To: <xmqq1s8q34g2.fsf@gitster-ct.c.googlers.com>

Hi Junio,

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

Show 11 quoted lines

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

Show 9 quoted lines

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

Show 10 quoted lines

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

Show 6 quoted lines

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