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

schannel: add client certificate authentication #2376

Closed
wants to merge 1 commit into from

Conversation

ArchangelSDY
Copy link
Contributor

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

@bagder
Copy link
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.

User can now specify a client certificate in system certificates store
explicitly using expression like `--cert "CurrentUser\MY\<thumbprint>"`.
@ArchangelSDY
Copy link
Contributor Author

@bagder All code style issues should be addressed.

@bagder
Copy link
Member

bagder commented Apr 16, 2018

Thanks!

@bagder bagder closed this in e35b025 Apr 16, 2018
@jay
Copy link
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
Copy link
Contributor Author

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
Copy link
Member

jay commented Apr 17, 2018

I can submit another patch to improve the documentation.

That would be great, thanks.

@jay
Copy link
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
Copy link
Contributor Author

OK, I'll try to find an alternative.

@ArchangelSDY ArchangelSDY deleted the schannel-client-cert branch April 18, 2018 13:08
@bagder
Copy link
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 mentioned this pull request Apr 23, 2018
jay pushed a commit that referenced this pull request Apr 23, 2018
- Use CRYPT_STRING_HEX instead of CRYPT_STRING_HEXRAW since XP doesn't
  support the latter.

Ref: #2376 (comment)

Closes #2504
jay pushed a commit that referenced this pull request Apr 23, 2018
@jay
Copy link
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
Copy link
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
Copy link
Contributor Author

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

@mback2k
Copy link
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
Copy link
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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@mback2k
Copy link
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.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

5 participants