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

pkcs15-tool: added support for reading NIST ssh keys #1233

Merged
merged 1 commit into from Jun 21, 2018

Conversation

popovec
Copy link
Contributor

@popovec popovec commented Jan 5, 2018

'pkcs15-tool --read-ssh-key' is now able to read NIST ECC keys from card.
Only 256, 384 and 521 field lengths are supported (same as allowed in
ssh-keygen -t ecdsa). Issue #803 is partialy fixed by this patch.
Openssh PKCS11 interface patches for ECC are now available, please check
https://bugzilla.mindrot.org/show_bug.cgi?id=2474

Checklist
  • Tested with the following card: MyEID

Copy link
Member

@Jakuje Jakuje left a comment

Choose a reason for hiding this comment

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

On the fast read through, it looks fine, assuming OpenSSH will ever merge ECDSA support for PKCS#11 keys.

{NULL, {{-1}}, 0},
};
char alg[20];
unsigned char buf[4+30+4+9+4+256];
Copy link
Member

Choose a reason for hiding this comment

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

I would write there either "big enough" number as for the RSA section or add there some comment why specially these numbers. Otherwise the code becomes it is very hard to maintain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, what about:

#define ITEM_SIZE 4
#define ALG_NAME 30
#define CURVE_NAME 9
#define KEY_DATA 256
..
unsigned char buf[3*ITEM_SIZE+ALG_NAME+CURVE_NAME+KEY_DATA];

???

Copy link
Member

Choose a reason for hiding this comment

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

Well ... you already propose that 30 chars is enough to fit ecdsa-sha2-nistp521 (19 chars), so I would rather put there something like this:

/* Large enough to fit the following:
 * 3 x 4B item length headers
 * max 20B algorithm name,  9B curve name, max 256B key data */
unsigned char buf[300];

Using constants makes it clearer, but significantly longer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for hint, pull request is already updated as your suggested.

'pkcs15-tool --read-ssh-key' is now able to read NIST ECC keys from card.
Only 256, 384 and 521 field lengths are supported (same as allowed in
ssh-keygen -t ecdsa). Issue OpenSC#803 is partialy fixed by this patch.
Openssh PKCS11 interface patches for ECC are now available, please check
https://bugzilla.mindrot.org/show_bug.cgi?id=2474
@frankmorgner
Copy link
Member

@popovec could you keep us updated, when ECDSA support is merged into OpenSSH? We'll keep the PR hanging till then.

@popovec
Copy link
Contributor Author

popovec commented Jan 17, 2018

Ok, I will let you know when openssh support for ECSDA is integrated into official openssh release.

@frankmorgner
Copy link
Member

thanks

@Jakuje
Copy link
Member

Jakuje commented May 2, 2018

Fedora 28 shipped with ECDSA keys support in OpenSSH and it would be good to have also this in next OpenSC release.

@dengert
Copy link
Member

dengert commented May 2, 2018

In response to #1233 (comment)

The OpenSC pkcs11 can read ECC keys and do ECDSA and ECDH operations on some cards.
Is there something missing other then pkcs15-tool? Can you do what is needed using pkcs11-tool?

@Jakuje
Copy link
Member

Jakuje commented May 2, 2018

Is there something missing other then pkcs15-tool? Can you do what is needed using pkcs11-tool?

I don't think so. The pkcs11-tool does not allow listing the keys in SSH format and it is for some reason implemented in pkcs15-tool.

Regardless pkcs11-tool, the same can be achieved by ssh-keygen -D /path/to/pkcs11.so, with the OpenSSH ECDSA patch, but for consistency, it would be good to have this functionality also in OpenSC.

I just verified again with my yubikey, that this builds fine and the output matches the one from ssh-keygen for all the keys I have.

@mouse07410
Copy link
Contributor

Status update, please?

@martinpaljak
Copy link
Member

To be honest, the task of converting a pem public key (readable from pkcs11) to the SSH format should not necessarily be part of any of the OpenSC tools. Convenient - yes. Blocking or must have feature? Not really.

@popovec
Copy link
Contributor Author

popovec commented May 4, 2018

Even when openssh never gets support for ECC keys over pkcs#11 interface, pkcs15-tool should have support for listing any keys in ssh format. Or, to get consistent behavior, there is another option - support for listing only RSA keys in ssh format should be removed from pkcs15-tool.

@martinpaljak
Copy link
Member

Tru. A "pem2ssh" probably exists?

@frankmorgner
Copy link
Member

It is possible to do

ssh-keygen -D /usr/lib/opensc-pkcs11.so >> ~/.ssh/authorized_keys

@frankmorgner
Copy link
Member

Sorry, I just noticed it was already suggested above. Anyway, if it is still wanted, we can include this feature after release of 0.18.0.

@Jakuje
Copy link
Member

Jakuje commented May 4, 2018

Why after release? I am not saying this is a blocking issue, but it is something good to have for consistency as already mentioned by @popovec. It is always good to have more ways and tools of doing the same thing.

The OpenSSH upstream does not ship the ecdsa in pkcs11 yet, but there are several people using it already. If OpenSSH will release in a three months with it, the next OpenSC release will be until next year? It really has to take a year to get such simple and isolated change into OpenSC release?

@Jakuje
Copy link
Member

Jakuje commented Jun 21, 2018

@frankmorgner I see it is already after the release. Is anything else blocking merging this PR?

@frankmorgner frankmorgner merged commit 5dcea44 into OpenSC:master Jun 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants