[PATCH 2/3] http: add support for disabling SSL revocation checks in cURL
From: Brendan Forster 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..* (which would mistake `schannel` for a URL). Helped by Agustín Martín Barbero. Signed-off-by: Brendan Forster Signed-off-by: Johannes Schindelin --- 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
Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL
On Mon, Oct 15, 2018 at 6:14 AM Brendan Forster via GitGitGadget 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 > --- > 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 > + }
Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL
"Brendan Forster via GitGitGadget" 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..* (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..$variable" to say "I want the http.$variable to be in effect but only when I am talking to ". 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);
Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL
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-65bdd376b...@mail.mipt.ru/ Since this one is clearly copied from there, it may be worth fixing the original. -Peff
Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL
Hi Eric, On Mon, 15 Oct 2018, Eric Sunshine wrote: > On Mon, Oct 15, 2018 at 6:14 AM Brendan Forster via GitGitGadget > 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 > > --- > > 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 > > + } >
Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL
Hi Junio, On Tue, 16 Oct 2018, Junio C Hamano wrote: > "Brendan Forster via GitGitGadget" 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..* (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..$variable" to say "I want the > http.$variable to be in effect but only when I am talking to ". > 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..` 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. 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. > > +#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 aff
Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL
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-65bdd376b...@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
Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL
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-65bdd376b...@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
Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL
Johannes Schindelin writes: >> In any case, you can use "http..$variable" to say "I want the >> http.$variable to be in effect but only when I am talking to ". >> 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. > > > Why does it fail? > ... > 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.
Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL
Eric Sunshine writes: > On Mon, Oct 15, 2018 at 6:14 AM Brendan Forster via GitGitGadget > 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 >> --- >> 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
Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL
Hi Junio, On Thu, 25 Oct 2018, Junio C Hamano wrote: > Eric Sunshine writes: > > > On Mon, Oct 15, 2018 at 6:14 AM Brendan Forster via GitGitGadget > > 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 > >> --- > >> 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@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 > >
Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL
Hi Junio, On Thu, 18 Oct 2018, Junio C Hamano wrote: > Johannes Schindelin writes: > > >> In any case, you can use "http..$variable" to say "I want the > >> http.$variable to be in effect but only when I am talking to ". > >> 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. > > > > > > Why does it fail? > > ... > > > > 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. >
Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL
Johannes Schindelin 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.