Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http: rename http.schannel.checkRevoke to http.schannelCheckRevoke #1747

Merged
merged 2 commits into from
Jul 6, 2018

Conversation

dscho
Copy link
Member

@dscho dscho commented Jul 6, 2018

The http.* config settings can be limited to specific URLs via
http.<url>.*. Unfortunately, this prevented our beautiful
http.schannel.checkRevoke from ever working, as the schannel part
was interpreted as a URL.

Let's just change this completely, by avoiding having "a middle part"
that is then interpreted as a URL.

As a fringe benefit, this now also allows users to disable the
revocation checking on a URL-by-URL basis.

This fixes #1531 (for real)

Signed-off-by: Johannes Schindelin johannes.schindelin@gmx.de

@dscho dscho requested a review from shiftkey July 6, 2018 08:47
@dscho
Copy link
Member Author

dscho commented Jul 6, 2018

/cc @agusmba

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
certificate. This option is ignored if cURL lacks support for
setting the relevant SSL option at runtime.

http.schannel.useSSLCAInfo::

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

@agusmba
Copy link

agusmba commented Jul 6, 2018

Could you review this line also?

git/http.c

Line 834 in 34573d2

#if LIBCURL_VERSION_NUM >= 0x074400

Its probably meant to say

#if LIBCURL_VERSION_NUM >= 0x072c00

If we want to check against curl version 7.44.00

shiftkey and others added 2 commits July 6, 2018 13:26
http: add support for disabling SSL revocation checks in cURL

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>
http: when using Secure Channel, ignore sslCAInfo by default

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>
@dscho dscho force-pushed the fix-check-revoke branch from b191c06 to 6152657 Compare July 6, 2018 11:26
@dscho
Copy link
Member Author

dscho commented Jul 6, 2018

@agusmba @shiftkey thank you so much for your help. I am rather happy that this little foray into not pushing directly to master but instead having competent contributors "look over my shoulder" is so successful. I think I will do this always, from now on: open a PR instead of pushing directly to master. I like it.

@dscho
Copy link
Member Author

dscho commented Jul 6, 2018

BTW @shiftkey could you give this a thorough review (after CI passes, that is), and merge if you find it good? I want to make it the rule that reviewers merge the PRs they approve of, if you don't mind.

@shiftkey shiftkey self-assigned this Jul 6, 2018
@shiftkey
Copy link

shiftkey commented Jul 6, 2018

Nice work @dscho and @agusmba for getting to the bottom of this!

@shiftkey shiftkey merged commit 6d45d96 into git-for-windows:master Jul 6, 2018
@melscoop
Copy link

melscoop commented Jul 6, 2018

This is great, thanks for getting this shipped y'all ❤️

@Kelderic
Copy link

Kelderic commented Jul 6, 2018

This is wonderful to see!

@dscho dscho deleted the fix-check-revoke branch July 6, 2018 22:25
dscho added a commit to git-for-windows/build-extra that referenced this pull request Jul 6, 2018
The `http.schannel.checkRevoke`
setting (which never worked) [was renamed to
`http.schannelCheckRevoke`](git-for-windows/git#1747).
In the same run, `http.schannel.useSSLCAInfo` (which also did not work,
for the same reason) was renamed to `http.schannelUseSSLCAInfo`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@shiftkey shiftkey mentioned this pull request Jul 30, 2018
1 task
@Kelderic
Copy link

Any idea when a new version with this change will released?

@dscho
Copy link
Member Author

dscho commented Aug 13, 2018

@Kelderic the best way forward is to first test: https://wingit.blob.core.windows.net/files/index.html.

Once you confirm that the latest snapshot works for you, it is much more certain that the next official release will work for you, too.

The current plan is to wait for Git to release v2.19.0 (which is scheduled on or around September 9th: http://tinyurl.com/gitCal), and then work toward a Git for Windows release, which typically happens the day after (in rare cases, it can take a little longer).

So: please test!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

http.schannel.checkRevoke option is not recognized
5 participants