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.

Sign up for GitHub

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

Jump to bottom

adding http.schannel.checkRevoke support #1450

Merged
merged 1 commit into from
Jan 31, 2018
Merged

adding http.schannel.checkRevoke support #1450

merged 1 commit into from
Jan 31, 2018

Conversation

shiftkey
Copy link

@shiftkey shiftkey commented Jan 23, 2018 •
edited
Loading

Fixes #1446

Opening this up early to gather feedback (I'm not strong on curl's internals) as I think we're all in agreement on the approach:

  • implement the config lookup
  • set this flag on the curl operation when schannel is the backend
  • default to true if unset
  • add mention in docs

dscho
dscho reviewed Jan 23, 2018
View reviewed changes
http.c Outdated
@@ -753,6 +753,12 @@ static CURL *get_curl_handle(void)
}
#endif

#if LIBCURL_VERSION_NUM >= 0x074400
if (!strcmp("schannel", http_ssl_backend) && http_schannel_check_revoke) {

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@dscho
Copy link
Member

dscho commented Jan 23, 2018

Looks good so far, I offered a couple of suggestions.

I also would prefer to have these patches squashed into a single one, and some documentation added to Documentation/config.txt, for an example see e.g. dc8f921#diff-ba92ef40c548c691816362bbdc35a613R2002 (beware that the tabs are somehow lost in the web view, but you will easily see them in your local config.txt file).

As to tests, I do not really know that we can test for this. We have to rely on cURL's own testing, I would imagine. As it is, the implementation on the Git side is straight-forward enough that I cannot see any bug.

So essentially I would suggest to:

  • move the #if LIBCURL_VERSION_NUM into the conditional block,
  • add an #else arm that prints out a warning,
  • replace the http_schannel_check_revoke == 0 by a !http_schannel_check_revoke (because that conforms to the implicit coding convention of Git),
  • add documentation to Documentation/config.txt, and
  • squash the patches into a single one that pretty much copies its commit message from [RFC] new config value to ignore revocation checks when using schannel #1446's very nice opening description of the problem.

@shiftkey
Copy link
Author

@dscho thanks for the feedback. I'll bump this when it's ready to go around again.

@shiftkey
add support for disabling SSL revocation checks in cURL
3fdedea 
This adds support for a new http.schannel.checkRevoke 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.

Signed-off-by: Brendan Forster <github@brendanforster.com>
@shiftkey shiftkey changed the title [WIP] adding http.schannel.checkRevoke support adding http.schannel.checkRevoke support Jan 24, 2018
@shiftkey
Copy link
Author

🍎

@shiftkey shiftkey mentioned this pull request Jan 31, 2018
Closed
@dscho dscho merged commit 3016f0c into git-for-windows:master Jan 31, 2018
@dscho
Copy link
Member

dscho commented Jan 31, 2018

Sorry for the delay! I somehow missed that this was ready for merging.

For the record: I plan on releasing Git for Windows v2.16.1(2) tomorrow, thanks to a couple of component updates that address CVEs (nothing that I think is exploitable, but let's be safe).

@shiftkey shiftkey deleted the schannel-norevoke-support branch January 31, 2018 22:42
@shiftkey
Copy link
Author

Sorry for the delay! I somehow missed that this was ready for merging.

All good. I thought you were taking a well-deserved break 😛

@dscho
Copy link
Member

dscho commented Jan 31, 2018

I thought you were taking a well-deserved break

Heh... Nope, I was sick most of last week, but had to work nevertheless (too much important stuff required from me).

dscho added a commit to git-for-windows/build-extra that referenced this pull request Jan 31, 2018
@dscho
Mention New Feature in release notes
d688875 
When using Secure Channel as HTTPS transport behind a proxy,
it may be necessary to disable revocation checks, [which is now
possible](git-for-windows/git#1450).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho added a commit that referenced this pull request Feb 16, 2018
@dscho
Merge pull request #1450 from shiftkey/schannel-norevoke-support
efb9836 
adding http.schannel.checkRevoke support
@shiftkey shiftkey mentioned this pull request Feb 18, 2018
Closed
@dscho dscho mentioned this pull request Feb 19, 2018
Closed
@yopito yopito mentioned this pull request Mar 2, 2018
Closed
1 task
@shiftkey shiftkey mentioned this pull request Mar 6, 2018
Closed
dscho added a commit that referenced this pull request Mar 23, 2018
@dscho
Merge pull request #1450 from shiftkey/schannel-norevoke-support
eacb6a3 
adding http.schannel.checkRevoke support
dscho added a commit that referenced this pull request Apr 3, 2018
@dscho
Merge pull request #1450 from shiftkey/schannel-norevoke-support
fa966b5 
adding http.schannel.checkRevoke support
dscho added a commit that referenced this pull request May 29, 2018
@dscho
Merge pull request #1450 from shiftkey/schannel-norevoke-support
10066e7 
adding http.schannel.checkRevoke support
dscho added a commit that referenced this pull request May 29, 2018
@dscho
Merge pull request #1450 from shiftkey/schannel-norevoke-support
5885f3b 
adding http.schannel.checkRevoke support
PKRoma pushed a commit to PKRoma/git-for-windows that referenced this pull request Jun 22, 2018
@dscho
Merge pull request git-for-windows#1450 from shiftkey/schannel-norevo…
655cf8a 
…ke-support

adding http.schannel.checkRevoke support
PKRoma pushed a commit to PKRoma/git-for-windows that referenced this pull request Jun 22, 2018
@dscho
Merge pull request git-for-windows#1450 from shiftkey/schannel-norevo…
0422725 
…ke-support

adding http.schannel.checkRevoke support
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dscho dscho dscho left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@shiftkey @dscho