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

schannel: add client certificate authentication #2376

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@ArchangelSDY
Contributor

ArchangelSDY commented Mar 11, 2018

User can now specify a client certificate in system certificates store explicitly using expression like --cert "CurrentUser\MY\<thumbprint>".

@bagder

This comment has been minimized.

Member

bagder commented Mar 11, 2018

Thanks @ArchangelSDY. Please run checksrc on the code and adjust the code style nits it finds. You can see them here.

@bagder bagder added the SSL/TLS label Mar 11, 2018

schannel: add client certificate authentication
User can now specify a client certificate in system certificates store
explicitly using expression like `--cert "CurrentUser\MY\<thumbprint>"`.
@ArchangelSDY

This comment has been minimized.

Contributor

ArchangelSDY commented Mar 12, 2018

@bagder All code style issues should be addressed.

@bagder

This comment has been minimized.

Member

bagder commented Apr 16, 2018

Thanks!

@bagder bagder closed this in e35b025 Apr 16, 2018

@jay

This comment has been minimized.

Member

jay commented Apr 17, 2018

I think we will need to explain this better in the documentation. It could use a separate section explaining the system stores.

It seems that CURLOPT_SSLCERT for WinSSL will be supported in some limited way for client certificates but not by filename or nickname, only thumbprint, is that correct? Is there any reason we couldn't check names as well?

@ArchangelSDY

This comment has been minimized.

Contributor

ArchangelSDY commented Apr 17, 2018

Yes, only thumbprint is supported. There are a lot of ways to find a certificate but looks like Windows do not a have a concept like nickname or certificate name, so thumbprint seems to be the only way to locate a certificate from system store uniquely.

Loading certificate from a local pfx file can be supported. But usually a local file can be imported into system store while the opposite may not always be possible due to some secure policies. So I think it's not that useful.

I can submit another patch to improve the documentation.

@jay

This comment has been minimized.

Member

jay commented Apr 17, 2018

I can submit another patch to improve the documentation.

That would be great, thanks.

@jay

This comment has been minimized.

Member

jay commented Apr 17, 2018

Some CI builds are failing because CryptStringToBinary doesn't have flag CRYPT_STRING_HEXRAW in <=XP. Can you use a different flag that is compatible with xp

@ArchangelSDY

This comment has been minimized.

Contributor

ArchangelSDY commented Apr 18, 2018

OK, I'll try to find an alternative.

@ArchangelSDY ArchangelSDY deleted the ArchangelSDY:schannel-client-cert branch Apr 18, 2018

@bagder

This comment has been minimized.

Member

bagder commented Apr 23, 2018

Any news? I propose we revert this commit until we have a fix. One of the build failures: https://curlbuild.uxnr.de/builders/curl_winssl_cross_x64/builds/9672/steps/compile/logs/stdio

@bagder bagder referenced this pull request Apr 23, 2018

Closed

schannel build failure #2518

jay added a commit that referenced this pull request Apr 23, 2018

schannel: fix build error on targets <= XP
- Use CRYPT_STRING_HEX instead of CRYPT_STRING_HEXRAW since XP doesn't
  support the latter.

Ref: #2376 (comment)

Closes #2504

jay added a commit that referenced this pull request Apr 23, 2018

@jay

This comment has been minimized.

Member

jay commented Apr 23, 2018

Fix was proposed in #2504 but I held it as a blocker due to roffit bug, which in hindsight I shouldn't have done. Fixed now, will investigate roffit bug separately.

@bagder

This comment has been minimized.

Member

bagder commented Apr 23, 2018

There are however still build failures in Marc's builds even with that fixup: https://curlbuild.uxnr.de/builders/curl_winssl_mingw_x86/builds/4212/steps/compile/logs/stdio

@ArchangelSDY

This comment has been minimized.

Contributor

ArchangelSDY commented Apr 23, 2018

Looks like MinGW lacks some essential APIs. I will work on a fix to disable this feature in that condition.

@mback2k

This comment has been minimized.

Member

mback2k commented Apr 23, 2018

Thanks. I am testing various MinGW versions on purpose, since distributions and msys2 as well as older mingw-get installers ship different versions.

@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Apr 23, 2018

Original MinGW targets Windows 2000 by default. Maybe these definitions are only available in XP+? MSDN says so, but they also regularly delete mentions of old Windows versions.

@ArchangelSDY

This comment has been minimized.

Contributor

ArchangelSDY commented Apr 23, 2018

Yes. According to this doc the minimum supported version is XP. I'm not sure if this feature will actually work on Windows 2000.

@ArchangelSDY

This comment has been minimized.

Contributor

ArchangelSDY commented Apr 23, 2018

Sent another PR #2522 to disable this feature in MinGW.

@mback2k

This comment has been minimized.

Member

mback2k commented Apr 23, 2018

Thanks, I answered in #2522. BTW-- I am working on a Dockerized environment for all my builders, to provide CI for pull-requests as well.

@lock lock bot locked as resolved and limited conversation to collaborators Jul 22, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.