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

Show key labels for PKCS#11 keys instead of library name #138

Closed
wants to merge 2 commits into from

Conversation

dmchurch
Copy link

@dmchurch dmchurch commented Jul 10, 2019

This is useful e.g. for Yubikeys, which can have many RSA keys defined. It extends the sshkey struct to add a *label member, which is filled by pkcs11_add_provider, and ssh-agent and ssh-keygen use that field.

Danielle Church added 2 commits Jul 8, 2019
Currently, ssh-agent uses the name of the PKCS#11 library as the key
comment returned by ssh-add -l. This is a problem when you have a
single smartcard that stores multiple private keys. This patch makes
ssh-agent ask the PKCS#11 driver for the key label and uses that
instead.
Uses the same PKCS#11 label interface exposed by the previous commit.
@WSLUser
Copy link

@WSLUser WSLUser commented Sep 9, 2019

Suggest you go upstream with this patch.

@djmdjm
Copy link
Contributor

@djmdjm djmdjm commented Sep 13, 2019

I don't think we want to add another member to sshkey for this.

ssh-agent has a per-key comment field that just gets filled with the PKCS11 provider path at the moment. IMO that would be better to use.

@dmchurch
Copy link
Author

@dmchurch dmchurch commented Sep 13, 2019

I considered that. (And, fwiw, the comment field is in fact where this eventually goes - see ssh-agent.c:616 in the change list.)

The issue is how the label gets from the PKCS#11 library to ssh-agent, since the comment field is in a struct defined locally to ssh-agent.c. The call path is as follows:

ssh-agent.c:process_add_smartcard_key(SocketEntry *)
ssh-pkcs11-client.c:pkcs11_add_provider(char *, char *, sshkey ***keysp)
(socket connection to child pkcs11-helper)
ssh-pkcs11-helper.c:process_add()
ssh-pkcs11.c:pkcs11_add_provider(char *, char *, sshkey ***keysp)
ssh-pkcs11.c:pkcs11_register_provider(char *, char *, sshkey ***keysp, pkcs11_provider **, CK_ULONG)
ssh-pkcs11.c:pkcs11_fetch_keys(pkcs11_provider *, CK_ULONG, sshkey ***keysp, int *nkeys)
[PKCS#11 API] C_GetAttributeValue(CK_SESSION_HANDLE, CK_OBJECT_HANDLE, CK_ATTRIBUTE *, CK_ULONG)

If we don't want to modify the sshkey struct, then the labels will have to exist in an array parallel to ***keysp. This means modifying the function signature of the four functions above to add a char ***labelsp parameter along with fixing every reference to those functions to add the extra NULL. And since pkcs11_fetch_keys() is only one of the functions that pkcs11_register_provider calls to assist with populating the ***keysp array, every other function - which I think is only pkcs11_fetch_certs() at present, though that might change - will also become responsible for maintaining and extending the ***labelsp array to stay parallel with ***keysp.

It's certainly doable, and I actually started down that path before I discovered how many complications it would entail, and I judged that it would be less invasive, with fewer chances for regression, just to modify the sshkey struct. And, since sshkey has been modified to add new members in the past, most recently to add the private key shield functionality in post-8.0, I figured that maintaining strict ABI compatibility wasn't high on the priority list.

That said, I'm not involved with the project, so I can't speak to what the priorities are. If you want, I'll rewrite this PR to do this without touching struct sshkey, but I wanted to let you know what would be involved there.

@Jakuje
Copy link
Contributor

@Jakuje Jakuje commented Sep 16, 2019

I have similar patch including support for the PKCS#11 URIs which also improves the naming in the comments. More updated version is available in Fedora and in my github branch. Feel free to try and test it.

@dmchurch
Copy link
Author

@dmchurch dmchurch commented Nov 16, 2019

@Jakuje I finally had a chance to test your branch - looks like we're working on slightly different problems here. The PKCS#11 URI support looks fantastic, but pkcs11: uris aren't a whole lot better for readability than simple file pathnames. Case in point, see what happens if you let ssh-askpass ask you for confirmation after doing an ssh-add -c /path/to/pkcs11.so. 😉 Testing on my local system seems to indicate that both of our patches play nicely together, though! Best of both worlds - pkcs11: URI support plus readable output from ssh-keygen -D et al. When do you think your patch might go into upstream? I was surprised not to see it in 8.1.

@djmdjm Can I get some feedback on this? I'm certainly willing to make changes here but I'm curious how you'd address the issues I mentioned above.

@Jakuje
Copy link
Contributor

@Jakuje Jakuje commented Nov 18, 2019

Thank you for the comment. This really depends on how your cards are enrolled and whether it has sensible labels (you can not always bet on that). The same way as library paths can work for someone with single key on the card and labels work for you, there might be others with different setup, where this would not be enough. Even though the PKCS#11 URIs are not that "nice", they are always unique so whenever you need to distinguish two keys on the card, it is trivial (unlike with just the paths to pkcs11 library, which is always the same).

I agree here with @djmdjm that this should not go to the sshkey structure, but to the pkcs11_key with all the other pkcs11 related information.

Whether the PKCS#11 URIS will ever get to upstream, I don't know. I already pushed for other PKCS#11 related fixes and features that are finally in and make the pkcs11 support usable, but this one is bigger and changes significantly more code paths, but I use it for few years in Fedora without issues.

djmdjm added a commit to djmdjm/openssh-wip that referenced this issue Jan 24, 2020
Extract the key label or X.509 subject string when PKCS#11 keys
are retrieved from the token and plumb this through to places where
it may be used as a comment.

based on openssh/openssh-portable#138
by Danielle Church
djmdjm added a commit to djmdjm/openbsd-openssh-src that referenced this issue Jan 25, 2020
Extract the key label or X.509 subject string when PKCS#11 keys
are retrieved from the token and plumb this through to places where
it may be used as a comment.

based on openssh/openssh-portable#138
by Danielle Church

feedback and ok markus@
djmdjm added a commit that referenced this issue Jan 25, 2020
Extract the key label or X.509 subject string when PKCS#11 keys
are retrieved from the token and plumb this through to places where
it may be used as a comment.

based on #138
by Danielle Church

feedback and ok markus@

OpenBSD-Commit-ID: cae1fda10d9e10971dea29520916e27cfec7ca35
@djmdjm
Copy link
Contributor

@djmdjm djmdjm commented Jan 25, 2020

I just committed in 89a8d45 a version of this patch reworked as per my comment above, but also to use the X.509 subject as the key label for keys extracted from x.509 certificates on PKCS#11 tokens.

@djmdjm djmdjm closed this Jan 25, 2020
djmdjm added a commit to djmdjm/openssh-wip that referenced this issue Jan 25, 2020
Extract the key label or X.509 subject string when PKCS#11 keys
are retrieved from the token and plumb this through to places where
it may be used as a comment.

based on openssh/openssh-portable#138
by Danielle Church
bob-beck pushed a commit to openbsd/src that referenced this issue Jan 25, 2020
Extract the key label or X.509 subject string when PKCS#11 keys
are retrieved from the token and plumb this through to places where
it may be used as a comment.

based on openssh/openssh-portable#138
by Danielle Church

feedback and ok markus@
mirabilos pushed a commit to MirBSD/ssh that referenced this issue Feb 16, 2020
Extract the key label or X.509 subject string when PKCS#11 keys
are retrieved from the token and plumb this through to places where
it may be used as a comment.

based on openssh/openssh-portable#138
by Danielle Church

feedback and ok markus@
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants