Skip to content

Enable PKCS11 Support #331

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

Merged
merged 2 commits into from
May 21, 2019
Merged

Conversation

NoMoreFood
Copy link

@NoMoreFood NoMoreFood commented Jul 15, 2018

  • Enable use of PKCS11 library files by adjusting master configuration file.
  • Modified dlsym() to return a void pointer instead of an int which is consistent with POSIX. The previous return type caused an issue with 32-bit builds with PKCS11 enabled.

@NoMoreFood NoMoreFood changed the title - Enabled Compiler PKCS#11 Support Enable PKCS#11 Support Jul 18, 2018
@cloudhan
Copy link

Any chance will this PR be merged? I need PKCS#11 support to ssh into the server which only support smartcard authentication.

@manojampalam
Copy link

@cloudhan can you enlighten me on how your scenario works? Do you have a PKCS provider on Windows? What did you mean by "server only support smartcard authentication" ?

@cloudhan
Copy link

cloudhan commented Oct 2, 2018

@manojampalam My company provide me with an USBKey (aka, the smartcard, similar to Yubikey) to login to the dev server. To my understanding, it basically is ssh public key auth. I can retrieve a public key from the USBKey and register it to the server. To login, I need the private key, but there is no way to retrieve the private key literal (by design). So, I either use the MS-CAPI or PKCS#11 for login authentication . However, OpenSSH ship with Windows 10 does not support. Currently I'm using the PuTTY-CAC suggested by OpenSC. But its font rendering is pretty awful.

If this pr can be merged, it can slightly improve my workflow in the future. (as well as a much border audience, as you can see from the short list of OpenSC)

@NoMoreFood
Copy link
Author

@manojampalam The way that PKCS support works in OpenSSH is that you pass in a vendor-provided DLL (or .SO file on Linux) that enables the capability to interface with the hardware token. The exported functions in the DLL must match the PKCS11 specification in order to be used by OpenSSH. PuTTY-CAC (of which I am the maintainer) is an add-on to PuTTY to provide this functionality by either using the same PKCS DLL or by using the CAPI subsystem on Windows (which uses the Windows minidriver for the token). Admittedly, I do not control the font rendering part of the code --- that's part of the upstream PuTTY code.

@NoMoreFood
Copy link
Author

@manojampalam Thoughts on moving forward with this?

@jcotton42
Copy link

This is also something I'd like to see

@NoMoreFood
Copy link
Author

@bingbing8 I adjusted my commit for simplicity by defining the PKCS11 compiler flag in the central configuration file and it looks like the appveyor tests are giving me "The specified module 'pester' was not loaded because no valid module file was found in any module directory." I doubt it's related to my change. Any idea what might be going on here?

@bingbing8
Copy link

@NoMoreFood , if you check the set up log, pester is not installed successfully. https://ci.appveyor.com/api/buildjobs/u0fl8r630g54pf00/artifacts/TestSetupLog.txt

@NoMoreFood
Copy link
Author

@bingbing8 Understood. Shouldn't that be something that the test setup does automatically or is there something additional I should be doing in my commits to install pester?

@bingbing8
Copy link

bingbing8 commented Nov 29, 2018

@NoMoreFood , the tests download and install it. The failure could due to temporary chocolatey server issue, network issue. or the particular version does not exist any more. Previously we install pester 3.4.6 due to a pester bug at that time. Please refer https://github.com/PowerShell/openssh-portable/blob/latestw_all/contrib/win32/openssh/OpenSSHTestHelper.psm1#L366. looks like the version still exists https://chocolatey.org/packages/pester/3.4.6.
you can either push a changes, or close/reopen the PR to trigger another test run if it is temporary chocolatey server or network issue.

@bingbing8 bingbing8 closed this Nov 29, 2018
@bingbing8 bingbing8 reopened this Nov 29, 2018
@bingbing8
Copy link

@NoMoreFood , I trigger the build again by close/reopen the PR, all checks passed.

@NoMoreFood NoMoreFood changed the title Enable PKCS#11 Support Enable PKCS11 Support Nov 30, 2018
@yan4321 yan4321 mentioned this pull request Dec 18, 2018
@mattmccull
Copy link

@manojampalam Do we have a estimate as to when this will be merged? This is something I'm interested in.

@spencercw
Copy link

This PR exposes an existing issue with the dlerror() implementation which causes a crash if the PKCS#11 DLL cannot be loaded. See #379 for the fix.

void * __stdcall dlsym(HMODULE handle, const char *symbol);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be stdcall? It would probably make more sense to leave this with the default calling convention like the other functions in this file.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. We do not. Commit adjusted.

@NoMoreFood NoMoreFood force-pushed the enable_pkcs11 branch 2 times, most recently from e914bd4 to 330acad Compare March 6, 2019 04:41
- Enable use of PKCS11 library files by adjusting central configuration file.
- Modified dlsym() to return a void pointer instead of an int which is consistent with POSIX. The previous return type caused an issue with 32-bit builds with PKCS11 enabled.
@jmfrank63
Copy link

Is this pull request going to be accepted? Would be a great addition.

@keliansb
Copy link

Please accept this PR, I'd like this to be implemented too.

@manojampalam manojampalam merged commit 44ba548 into PowerShell:latestw_all May 21, 2019
@NoMoreFood NoMoreFood deleted the enable_pkcs11 branch May 22, 2019 11:14
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.

9 participants