Adding PKCS11 support to SSH agent #537

yan4321 · 2021-10-26T11:53:02Z

Currently, PKCS11 keys work when using the SSH client with the -I switch.
However, In the SSH agent, there is no support for adding/removing/signing PKCS11 keys.
This PR aims to add this functionality to the Windows SSH agent.

This PR is based on the original PR - #362. Since the original PR was made a while ago, It'll be challenging to review after a rebase - hence I'm opening this one.

Tested with SoftHSM2 and Yubikey 4. Instructions on how to test can be found here.

bagajjal · 2021-10-26T17:48:13Z

@yan4321 Thanks for adding this support.
CI tests are failing, please fix them.

bagajjal · 2021-10-28T17:44:37Z

@yan4321 - Please provide the instructions to test. It would be great if you can add some unit tests.

jdewitee · 2021-10-31T02:43:17Z

@yan4321 This will be extremely useful

yan4321 · 2021-10-31T08:03:42Z

@bagajjal , I've updated the PR description with instructions on how to test this.

matsmcp · 2021-11-05T08:32:05Z

Also looking forward to this for smartcard based authentications

fouwels · 2021-11-12T15:13:49Z

Can this be reviewed+merged before we diverge again and are back to square one? :)

fouwels · 2021-11-12T15:20:20Z

@yan4321 - would you be able to do an (unofficial) GH release on your fork, with the binaries, while this is in progress?

martelletto

Hi,

I was asked by @bagajjal to take a look at this PR and left some comments. A disclaimer: although I have been dipping my toes lately, I am not a Windows guy. Apologies in advance if anything I said comes across as nonsensical.

Thank you,

-p.

contrib/win32/win32compat/ssh-agent/keyagent-request.c

yan4321 · 2021-12-07T10:07:40Z

@martelletto, Thank you for the review and input - highly appriciated!
I've addressed your comments and did re-testing. Please let me know if anything needs further clarification and I'm open to having additional discussion.

@kaelanfouwels, Since an unofficial release would involve generating binaries, would it make more sense for the admins of the repo to generate them? It's as simple as recompiling the solution with the only difference being ssh-agent project would need to be compiled with ENABLE_PKCS11 macro.
Once users used this feature for a while and reported success we can include this macro in the official release.

contrib/win32/win32compat/ssh-agent/keyagent-request.c

martelletto · 2021-12-10T12:55:20Z

@yan4321 Looks good; thank you! I left two further small comments. Please feel free to mark resolved conversations as such (GitHub won't let me, since I am not the PR author nor do I have write permission on the repository).

yan4321 · 2021-12-13T12:13:17Z

@martelletto, Thanks again for reviewing and for the additional comments. I've addressed them and would appreciate it if you could take another look.

contrib/win32/win32compat/ssh-agent/keyagent-request.c

martelletto · 2021-12-13T14:46:08Z

@yan4321 Thank you! I've left a small remark above.

bagajjal · 2021-12-13T22:20:06Z

ssh-pkcs11.h

-void	pkcs11_terminate(void);
-int	pkcs11_add_provider(char *, char *, struct sshkey ***, char ***);
-int	pkcs11_del_provider(char *);
+struct sshkey * lookup_key(const struct sshkey *);


Please move the windows specific code to #ifdef WINDOWS block. Please add comments why there is a deviation.

bagajjal · 2021-12-13T22:26:29Z

contrib/win32/win32compat/ssh-agent/connection.c

@@ -31,6 +31,11 @@
 #include "agent.h"
 #include "agent-request.h"

+#ifdef ENABLE_PKCS11
+#include "ssh-pkcs11.h"
+#define ENABLE_PKCS11


Can you define ENABLE_PKCS11 in https://github.com/PowerShell/openssh-portable/blob/latestw_all/contrib/win32/openssh/config.h.vs

Line 36 is hit only when ENABLE_PKCS11 is defined? This is redundant and can be deleted?

I see this is already defined in

openssh-portable/contrib/win32/openssh/config.h.vs

Line 138 in 991465e

#define ENABLE_PKCS11 1

bagajjal · 2021-12-14T03:14:54Z

@yan4321 - ssh-keygen is crashing while I was trying, ssh-keygen -f <path_to_pubkey_from_step_2i> -i -m PKCS8.
I compiled the binaries from your remote branch on my windows 10 machine. I don't have any private changes, all the code is from remote branch. What am I missing?

yan4321 · 2021-12-14T07:32:29Z

@bagajjal , The issue you described above has nothing to do with the changes in this PR.
If you run the exact same command on the master branch, you will get the exact same result.
This makes sense because this PR doesn't attempt to make any changes to the functionality of ssh-keygen.
This conversion is a preparation step to configure the setup for testing the changes in this PR.
We need to tell sshd to trust the key, by adding it to the authorized_keys file which expects the key to be in openssh format, however yubico-piv-tool outputs the public key in PEM format, hence conversion is needed.
This specific conversion functionality seems to be broken in PowerShell/openssh-portable.
It's worth opening a separate issue to address this, but again, it's unrelated to the changes in this PR.
To make this conversion, you can run the same command on a Linux-based system. If you're constrained to a Windows environment, you can use git bash to run the same command.

I'm also happy to assist with the testing on a side-channel such as email and if we find any issues we can bubble them here. This might work better as it'll keep this PR thread cleaner and more focused on the review process. Either way, happy to help with any issues.

bagajjal · 2021-12-15T07:18:44Z

I'm also happy to assist with the testing on a side-channel such as email and if we find any issues we can bubble them here. This might work better as it'll keep this PR thread cleaner and more focused on the review process. Either way, happy to help with any issues.

Thanks for your quick reply. I'm running into issues as "ssh-add -s <path_to_opensc-pkcs11.dll>” asks for passphrase and returns an error “communication with agent failed”. I sent an email to your Gmail. Thanks for your support in testing.

martelletto · 2021-12-16T13:12:09Z

@bagajjal, was ssh-agent built with ENABLE_PKCS11? That's step zero of https://gist.github.com/yan4321/9c86119d0c83d3da644f73b5b2fef987. I missed it when testing the changes today and ran into the same problem. Modulo that hiccup (on my part), the changes worked for me.

@yan4321, two quick observations regarding the gist:

If we swap steps 3 and 4, we can use ssh-add -L to obtain the public key in ssh format;
Once we have the public key in ssh format, we can use ssh-add -T to test sign/verify operations.

bagajjal · 2021-12-17T01:33:28Z

@bagajjal, was ssh-agent built with ENABLE_PKCS11? That's step zero of https://gist.github.com/yan4321/9c86119d0c83d3da644f73b5b2fef987. I missed it when testing the changes today and ran into the same problem. Modulo that hiccup (on my part), the changes worked for me.

@yan4321, two quick observations regarding the gist:

If we swap steps 3 and 4, we can use ssh-add -L to obtain the public key in ssh format;

Once we have the public key in ssh format, we can use ssh-add -T to test sign/verify operations.

@martelletto - Isn't it (ENABLE_PKCS11) already defined in config.h.vs? What additoinal changes are required in ssh-agent?

openssh-portable/contrib/win32/openssh/config.h.vs

Line 138 in 991465e

#define ENABLE_PKCS11 1

martelletto · 2021-12-17T07:26:30Z

@bagajjal, ENABLE_PKCS11 is defined in config.h.vs, but config.h is not included by any file under contrib/win32/win32compat/ssh-agent/. To enable PKCS11 support in ssh-agent as developed in this PR, the definition needs to find its way to ssh-agent/connection.c and ssh-agent/keyagent-request.c. That's what @yan4321 refers to in the first step of the gist.

yan4321 · 2021-12-17T08:44:53Z

@bagajjal , @martelletto , I think it boils down to the following question:
Do we want this PR to also:

Enable PKCS11 support in ssh-agent by default (which seems to be @bagajjal 's implicit assumption); or
For an interim period, keep it disabled by default and either provide users who are interested in this functionality an unofficial release or let them compile the project by themselves by flipping ENABLE_PKCS11 on manually (the first step in the gist). Once a few users reported success with their specific use-cases - enable it by default in a separate PR. This was my original intent but I'm open to option (1) as well.

The end result will be the same. With option number (2), we separate between implementing the support and enabling it by default, which could be considered safer.
Would appreciate your thoughts/input on this.

bagajjal · 2021-12-18T01:59:59Z

@yan4321 , @martelletto,
I defined ENABLE_PKCS11 in ssh-agent/connection.c and ssh-agent/keyagent-request.c. I still get the same error.
Do I need to modify any other files?

@yan4321 , please make changes to enable it by default.

ssh-add.exe asks for passphrase while trying to add opensc-pkcs11.dll.
I tried empty password, "123456" but none of them work.

bagajjal · 2022-02-15T02:46:59Z

contrib/win32/win32compat/w32fd.c

@@ -1071,7 +1071,9 @@ spawn_child_internal(const char* cmd, char *const argv[], HANDLE in, HANDLE out,
 	if (strstr(cmd, "sshd.exe")) {
 		flags |= DETACHED_PROCESS;
 	}
-
+	if (strstr(cmd, "helper.exe")) {


Please be explicit with the binary names..

change this to
ssh-pkcs11-helper.exe, ssh-sk-helper.exe

bagajjal · 2022-02-15T02:48:12Z

ssh-pkcs11-client.c

+
+	if ((user_token = get_current_user_token()) == NULL ||
+		posix_spawnp_as_user((pid_t *)&pid, av[0], &actions, NULL, av, NULL, user_token) != 0) {
+		error("posix_spawnp_as_user failed");


Changed. Thanks!

ssh-pkcs11-client.c

bagajjal · 2022-02-15T02:49:31Z

ssh-pkcs11.c

@@ -77,6 +77,15 @@ struct pkcs11_key {
 	int			keyid_len;
 };

+#ifdef WINDOWS


I don't think the changes in this file are required as we have a copy of these functions/structs in ssh-pkcs11-client.c

Yup, you're right. Removed the unnecessary parts here.

bagajjal · 2022-02-15T02:51:44Z

ssh-pkcs11-client.c

+	av[2] = NULL;
+	HANDLE user_token = NULL;
+
+	if ((user_token = get_current_user_token()) == NULL ||


Please have a error log if get_current_user_token() fails to return the user token

With the latest commit, we no longer use get_current_user_token()

bagajjal · 2022-02-15T02:55:22Z

@yan4321 - I had similar code changes (private) except the way the user token is retrieved. I provided my private binaries on friday 2/11. I'm going to take your changes as they are pretty close.
I left few comments, please address them. I'm ready to merge this PR.

Appreciate all your contributions.

bagajjal · 2022-02-15T03:09:43Z

contrib/win32/win32compat/win32_usertoken_utils.c

+	WTSFreeMemory(session_info);
+
+	// Get token of the logged in user by the active session ID
+	ret = WTSQueryUserToken(session_id, &current_token);


Major comment.

windows 10, windows 11 allows multiple RDP sessions. https://www.helpwire.app/blog/allow-multiple-remote-desktop-connections-windows-10/

If a customer has multiple RDP sessions then line 432 doesn't guarantee to return the session ID associated with the correct login user? We end up creating the process in different user namespace?

This again leads to security issue.

Let's say two users have active RDP sessions. One of them is non-admin and the other is admin.
Let's say non-admin runs ssh-add with a malicious PKCS11 provider, this function returns the token associated with admin user then he gains the access to the whole SYSTEM?

You are correct that get_current_user_token() assumes there is only single logged-in user. I'll address that shortly. Thanks!

With the latest commit, we use the pid of the client connecting to ssh-agent to retrieve the user token, and use that token to start the helper as that user. That way, even if there are multiple logged-on users, the helper will be spawned as the exact user who originated the request.
I've verified with procmon that the helper is spawned as the expected user.
Thanks for bringing up the multi logged-on users scenario!

yan4321 · 2022-02-16T10:12:57Z

@bagajjal , Appreciate your review!
I addressed your comments above and re-tested.
Please let me know if it looks good or if there's anything else I can address.

bagajjal · 2022-02-18T00:47:21Z

ssh-pkcs11-client.c

+	av[2] = NULL;
+
+	if ((client_process_handle = OpenProcess(PROCESS_QUERY_INFORMATION, FALSE, sshagent_client_pid)) == NULL ||
+		OpenProcessToken(client_process_handle, TOKEN_QUERY | TOKEN_ASSIGN_PRIMARY, &client_token) == FALSE) {


We shouldn't use the same token, we should duplicate the token and pass it to line 473.
I will take care of this. I will merge your PR.
Thanks a lot for your contributions.

One more issue, over SSH to localhost this wouldn't work. Because the SSH session is in session 0, smart card authentication wouldn't work in session 0. I think it's fine because customer is expected to physically present with the smart card and there wouldn't be a need to do over SSH to localhost. .

@bagajjal , Can you be a bit more clear about the scenario you are describing, please?
The helper shouldn't run in session 0 since this session has elevated privileges and the whole point here is for the helper to be executed with the same (reduced) privileges as the client (which doesn't run in session 0).
This implementation achieves that and ssh to localhost works as shown in the screenshot below

That's the normal behavior. I'm talking about SSH connection made over the SSH.
First you make an SSH connection to the machine (which has smart card). Now over the SSH connection, if you try to run "ssh-add -s " then it will fail because the ssh-add.exe process id is running in session 0.

As I said, this is a corner case as we expect the customer to be physically present with the smart card to use it. When customer is physically present, there is no point why customer should try ssh-add.exe over an SSH connection.

@bagajjal , What you are describing requires ssh-agent forwarding to be enabled (except, running ssh-add -s on the jump host doesn't make much sense. Running ssh <target_host> with the forwarded agent does).
On a Linux-based system, with agent-forwarding enabled the following scenario is supported and would work:

Client machine initiates ssh connection to the jump host with agent forwarding using a pkcs11 key.

Jump host (which the smartcard isn't connected to) initiates ssh connection to the target host - since the agent was forwarded from the client machine via a socket- the authentication (sign request) will happen on the client machine, where the smartcard is connected.

This is one of the reasons Yubikeys have the capability to set touch policy on pkcs11 keys - If an authentication attempt was made through a forwarded agent on a remote host (where the Yubikey isn't connected), the user will be made aware of this (the Yubikey will flash) and will have to physically touch the key for the authentication to proceed.
If agent forwarding is implemented for Windows, this should also work. I'll try to test this out soon.

Agree. Windows is missing ssh-agent forwarding. It's there in our backlog. It would be great if you can contribute for the ssh-agent forwarding.

fouwels · 2022-02-18T11:27:34Z

💯Thanks for the effort Yan and bagajjal

matsmcp · 2022-02-21T14:47:39Z

@yan4321
@bagajjal

Sorry for late reply. I do have good news though.
We have done initial testing with smartcards containing X509 certificates and Secmaker NetID as CSP.

Adding the keys from our cards with ssh-add -s do work
ssh-add -L shows the keys as expected

Extracting the public keys with ssh-keygen -D also works as expected.
By adding the public key extracted above to authorized_keys on another server we can log on to that server using the key pair.

With the card removed it fails as expected.

SSH-add -D removes the keys

Therefore I would say that all test cases completed successfully

jdewitee · 2022-03-18T02:27:13Z

@bagajjal I have installed V8.9.0.0p1-Beta from https://github.com/PowerShell/Win32-OpenSSH/releases/tag/v8.9.0.0p1-Beta but I am getting an error after entering PIN when trying to add yubikey to ssh-agent using either OpenSC or Yubico PIV:

ssh-add -s "C:\Program Files\Yubico\Yubico PIV Tool\bin\libykcs11.dll"
Enter passphrase for PKCS#11:
Could not add card "C:\Program Files\Yubico\Yubico PIV Tool\bin\libykcs11.dll": agent refused operation

ssh-add -s "C:\Program Files\OpenSC Project\OpenSC\pkcs11\opensc-pkcs11.dll"
Enter passphrase for PKCS#11:
Could not add card "C:\Program Files\OpenSC Project\OpenSC\pkcs11\opensc-pkcs11.dll": agent refused operation

bagajjal · 2022-03-18T23:24:56Z

@jdewitee - I'm guessing you installed the V8.9 using the MSI. If yes, there is a bug in the MSI. please refer to PowerShell/Win32-OpenSSH#1914.

If not, please have a look at event viewer log for the error message.

jdewitee · 2022-03-24T00:27:52Z

@bagajjal Thanks. Yes I was using MSI. Working now with zip version.

bagajjal · 2022-03-24T00:29:33Z

@jdewitee - You can try the MSI of V8.9.1.0 as well. V8.9.1.0 released yesterday.

https://github.com/PowerShell/Win32-OpenSSH/releases/tag/v8.9.1.0p1-Beta

meistars · 2022-04-12T08:09:06Z

Seeing same issue using MSI or zip for OpenSSH_for_Windows_8.9p1, LibreSSL 3.4.3
ssh-add -s "C:\Program Files\Yubico\Yubico PIV Tool\bin\libykcs11.dll" Enter passphrase for PKCS#11: Could not add card "C:\Program Files\Yubico\Yubico PIV Tool\bin\libykcs11.dll": agent refused operation
Setup: Downloaded the zip -> ran install-sshd.ps1 -> started ssh-agent and sshd services -> ran the ssh-add -s
Do not see anything in event viewer, tried cmd with admin/nonadmin, ensured that yubikey is activated, etc,

daemonhorn · 2022-08-27T17:38:58Z

Seeing same issue using MSI or zip for OpenSSH_for_Windows_8.9p1, LibreSSL 3.4.3 ssh-add -s "C:\Program Files\Yubico\Yubico PIV Tool\bin\libykcs11.dll" Enter passphrase for PKCS#11: Could not add card "C:\Program Files\Yubico\Yubico PIV Tool\bin\libykcs11.dll": agent refused operation Setup: Downloaded the zip -> ran install-sshd.ps1 -> started ssh-agent and sshd services -> ran the ssh-add -s Do not see anything in event viewer, tried cmd with admin/nonadmin, ensured that yubikey is activated, etc,

@meistars Make sure you have the C:\Program Files\Yubico PIV Tool\bin\ path listed in the SYSTEM environment variable for %Path% (not the user environment variable, as ssh-agent runs in a different environment context to your normal user. Check your Event Viewer logs (under Application and Services Logs->OpenSSH->Admin) for detailed error messages. Just wanted to share, as I was initially running into this same issue. Works now.

baimard · 2023-07-12T08:56:39Z

I try different solution but I have the same issue as @meistars

Path :

Copy paste in openssh folder:

Errors :

minfrin · 2024-06-13T10:28:56Z

Also just hit PowerShell/Win32-OpenSSH#1548. Was PKCS11 support in the ssh-agent ever confirmed to have worked?

fouwels · 2024-06-13T10:48:22Z

No, this never worked.(I am not a maintainer on this package)On 13 Jun 2024, at 11:29, minfrin ***@***.***> wrote: Also just hit PowerShell/Win32-OpenSSH#1548. Was PKCS11 support in the ssh-agent ever confirmed to have worked? —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

bwachter · 2024-06-24T14:04:43Z

Also just hit PowerShell/Win32-OpenSSH#1548. Was PKCS11 support in the ssh-agent ever confirmed to have worked?

I've just tested with the latest release (9.5p1) installed via MSI - pkcs#11 works with a Nitrokey HSM and OpenSC. Make sure the agent service as well as ssh-add and ssh are the new binaries - the agent should be updated automatically, but you'll have the old ssh/ssh-add in the path.

yan4321 · 2024-07-08T08:41:24Z

@meistars, @baimard, @minfrin, @fouwels, @bwachter, I just re-tested this using both 32 and 64-bit binaries of the latest release (v9.5.0.0p1-Beta) and all smartcard functionality seems to be working well.
This also has been reported to be working by others in the past (1, 2, 3, 4).

I posted some suggestions for troubleshooting here.

SSH Agent - PKCS11 Support

cc16f02

Fix includes

4d05d2d

martelletto reviewed Nov 18, 2021

View reviewed changes

Address comments

177d72c

martelletto reviewed Dec 10, 2021

View reviewed changes

contrib/win32/win32compat/ssh-agent/keyagent-request.c Outdated Show resolved Hide resolved

martelletto reviewed Dec 10, 2021

View reviewed changes

contrib/win32/win32compat/ssh-agent/keyagent-request.c Outdated Show resolved Hide resolved

Address additional comments

866cd78

martelletto reviewed Dec 13, 2021

View reviewed changes

contrib/win32/win32compat/ssh-agent/keyagent-request.c Outdated Show resolved Hide resolved

bagajjal reviewed Dec 13, 2021

View reviewed changes

Address comment

ada6e38

bagajjal reviewed Feb 15, 2022

View reviewed changes

ssh-pkcs11-client.c Show resolved Hide resolved

bagajjal reviewed Feb 15, 2022

View reviewed changes

bagajjal mentioned this pull request Feb 15, 2022

Publish to WinGet PowerShell/Win32-OpenSSH#1896

Closed

yan4321 added 2 commits February 16, 2022 00:18

Address review comments

08b207e

Adjust indentation

0c1ee33

bagajjal reviewed Feb 18, 2022

View reviewed changes

bagajjal merged commit c89890c into PowerShell:latestw_all Feb 18, 2022

yan4321 mentioned this pull request Feb 21, 2022

Bagajjal/fix minor issues #568

Merged

stevefan1999 mentioned this pull request Feb 24, 2022

Remote-SSH using PKI card on Windows: vscode-server download fails microsoft/vscode-remote-release#3432

Open

bagajjal mentioned this pull request Mar 17, 2022

Adding PKCS11 support to SSH agent PowerShell/Win32-OpenSSH#1908

Closed

kayavila mentioned this pull request May 3, 2024

ssh-add -s fails PowerShell/Win32-OpenSSH#1548

Open

Adding PKCS11 support to SSH agent #537

Adding PKCS11 support to SSH agent #537

Uh oh!

Conversation

yan4321 commented Oct 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bagajjal commented Oct 26, 2021

Uh oh!

bagajjal commented Oct 28, 2021

Uh oh!

jdewitee commented Oct 31, 2021

Uh oh!

yan4321 commented Oct 31, 2021

Uh oh!

matsmcp commented Nov 5, 2021

Uh oh!

fouwels commented Nov 12, 2021

Uh oh!

fouwels commented Nov 12, 2021

Uh oh!

martelletto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yan4321 commented Dec 7, 2021

Uh oh!

Uh oh!

Uh oh!

martelletto commented Dec 10, 2021

Uh oh!

yan4321 commented Dec 13, 2021

Uh oh!

Uh oh!

martelletto commented Dec 13, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bagajjal Dec 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bagajjal commented Dec 14, 2021

Uh oh!

yan4321 commented Dec 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bagajjal commented Dec 15, 2021

Uh oh!

martelletto commented Dec 16, 2021

Uh oh!

bagajjal commented Dec 17, 2021

Uh oh!

martelletto commented Dec 17, 2021

Uh oh!

yan4321 commented Dec 17, 2021

Uh oh!

bagajjal commented Dec 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yan4321 commented Oct 26, 2021 •

edited

Loading

bagajjal Dec 14, 2021 •

edited

Loading

yan4321 commented Dec 14, 2021 •

edited

Loading

yan4321 Feb 15, 2022 •

edited

Loading

yan4321 Feb 18, 2022 •

edited

Loading

yan4321 Feb 18, 2022 •

edited

Loading