Pkcs11 Edwards support #230

beldmit · 2021-02-25T13:34:04Z

This patch provides basic support of Ed25519 keys via PKCS#11 tokens

Jakuje · 2021-02-25T13:56:52Z

ssh-pkcs11.c

+	if (pObj != NULL) {
+		int nid = OBJ_obj2nid(pObj);
+		ASN1_OBJECT_free(pObj);
+		return (nid == NID_ED25519) ? (0) : (-1);


Initially, after getting the PKCS#11 3.0 out, there was a bit unclear whether the parameters should be DER encoded OID or printableString "edwards25519". For that reason I was changing this in softhsm in the following PR:

https://github.com/opendnssec/SoftHSMv2/pull/526/files#diff-c2f98368371548f9c5c9ca522d545eeb756885689b8db595118c0ebe3a4c37c0R162-R191

I think the applications should ideally accept both of the versions to avoid unexpected failures. The above place is how I handled that in SoftHSM.

Thanks! Implemented in a similar way, will be pushed soon

ssh-pkcs11.c

sshkey.c

beldmit · 2021-02-25T17:06:53Z

Force-pushed changes related to build without openssl and related to the comments by @Jakuje

Don't see the MasOS output so can't do anything with it :(

rahsinha2 · 2021-03-31T20:57:15Z

I reached out to Jakub about having trouble converting OPENSSL Ed25519 key to OPENSSH format when he pointed me out to the commit "Allow import Edwards keys from OpenSSL". I tried those changes by cloning the pkcs11_edwards branch and the conversion worked for me.

$ openssl genpkey -algorithm Ed25519 -out ed25519key.pem
$ ssh-keygen -y -f ed25519key.pem
Load key "ed25519key.pem": invalid format
$

NOW USING branch: pkcs11_edwards
$ ../openssh-portable/ssh-keygen -y -f ed25519key.pem
ssh-ed25519 AAAA..............(ommitted)........................................ZB
$

Thanks for the help Jakub.

nisamuel · 2021-05-24T10:51:06Z

Hi Dmitry,
Do you know where is this pkcs11_edwards branch now ?

Thanks,
Nitin

beldmit · 2021-05-24T10:53:46Z

Hi Nitin

https://github.com/beldmit/openssh-portable/tree/pkcs11_edwards

nisamuel · 2021-05-24T11:39:13Z

Hi Dmitry,
I get this error when a client tries to connect to sshd (OpenSSH_8.0p1) using ed25519 hostkeys.
Any clues ?

debug2: ssh_ed25519_verify: crypto_sign_ed25519_open failed: -1
ssh_dispatch_run_fatal: Connection to 10.196.156.163 port 22022: incorrect signature
openssh-8.5p1:=> ./ssh -V
OpenSSH_8.5p1, OpenSSL 1.0.2k-fips 26 Jan 2017

Thanks,
Nitin

beldmit · 2021-05-24T11:43:35Z

No clues. Which token do you use?

ssh-pkcs11.c

martelletto · 2021-05-24T12:11:48Z

this PR exposes ssh-add's 1024-byte signature test's incompatibility with OpenSC's 512-byte internal limit, since the data to be signed is passed to OpenSC undigested (consequently, ssh-add -T fails). it would be nice if both sides could agree on a limit, so that ssh-add -T works w/ opensc-pkcs11 eddsa keys.

Jakuje · 2021-05-24T16:14:22Z

this PR exposes ssh-add's 1024-byte signature test's incompatibility with OpenSC's 512-byte internal limit, since the data to be signed is passed to OpenSC undigested (consequently, ssh-add -T fails). it would be nice if both sides could agree on a limit, so that ssh-add -T works w/ opensc-pkcs11 eddsa keys.

Hmm. Did not know about this feature of ssh-add. This is indeed useful. For normal keys, it is not an issue as they go through hashing. We have the following PR on OpenSC side to allow large inputs so we should make sure it will get into the next release: OpenSC/OpenSC#2314

nisamuel · 2021-05-26T02:37:21Z

Hi Dmitry,
Here is the story...
I have a PEM key for Ed25519 from which I wanted to extract the pub key. I took your patch "Allow import Edwards keys from OpenSSL" alone (1 from the 8 commits) to get ssh-keygen -f <pem> -y working.
Thanks for that !!

  The pair now looks like --

firepower-2110:~# cat /mnt/disk0/.private/lina_sshd/ctxt0/ssh_host_eddsa_key.pub
ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIO6kAVZLmePdXfsca2MGuGSjrzvT7QvVZ3cbaFZy3EXx

firepower-2110:~# cat /mnt/disk0/.private/lina_sshd/ctxt0/ssh_host_eddsa_key // input PEM
-----BEGIN PRIVATE KEY-----
MC4CAQAwBQYDK2VwBCIEIG2Z9l0q+9au2Mun6dA2dwPXVs8mDcYAQSPuo+/1IXae
-----END PRIVATE KEY-----

  The above pvtkey is loaded into  OpenSSH_8.0p1  sshd which it used to sign the kex HASH and sent in kex reply.
  The problem now is that a remote client trying to connect to this sshd fails to verify the signature  using the pubkey, as in --

CLIENT:
debug2: ssh_ed25519_verify: crypto_sign_ed25519_open failed: -1
ssh_dispatch_run_fatal: Connection to x.y.z.z port 22022: incorrect signature

A private ed25519 key on a normal linux host looks larger than the one generated from PEM above--

linux:~# cat /etc/ssh/ssh_host_ed25519_key
-----BEGIN OPENSSH PRIVATE KEY-----
b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAMwAAAAtzc2gtZW
QyNTUxOQAAACBhArefq3UNoXUQzWm84lNXZn0khT4OfVyfqvJSzLMwmwAAAJiIKZ2BiCmd
gQAAAAtzc2gtZWQyNTUxOQAAACBhArefq3UNoXUQzWm84lNXZn0khT4OfVyfqvJSzLMwmw
AAAECgfh9gYXZZFTD99yiE6RssIfgALC5/i6NX61a1rJgK+2ECt5+rdQ2hdRDNabziU1dm
fSSFPg59XJ+q8lLMszCbAAAAEXJvb3RAaW50ZWwteDg2LTY0AQIDBA==
-----END OPENSSH PRIVATE KEY-----

Is there something amiss in the pair that ssh-keygen generated from the PEM I provided ?

Thanks,
Nitin

beldmit · 2021-05-26T05:46:43Z

Hello Nitin

First of all I hope you didn't publish your real keys, just experimental ones.

I'll try to check but not immediately

Jakuje · 2021-05-26T08:35:26Z

Note the headers:

-----BEGIN OPENSSH PRIVATE KEY-----

and

-----BEGIN PRIVATE KEY-----

The first one is OpenSSH format and the other is PEM.

The PEM file looks fine:

[jjelen@t490s Downloads]$ openssl pkey -in - -text
-----BEGIN PRIVATE KEY-----
MC4CAQAwBQYDK2VwBCIEIG2Z9l0q+9au2Mun6dA2dwPXVs8mDcYAQSPuo+/1IXae
-----END PRIVATE KEY-----
-----BEGIN PRIVATE KEY-----
MC4CAQAwBQYDK2VwBCIEIG2Z9l0q+9au2Mun6dA2dwPXVs8mDcYAQSPuo+/1IXae
-----END PRIVATE KEY-----
ED25519 Private-Key:
priv:
    6d:99:f6:5d:2a:fb:d6:ae:d8:cb:a7:e9:d0:36:77:
    03:d7:56:cf:26:0d:c6:00:41:23:ee:a3:ef:f5:21:
    76:9e
pub:
    ee:a4:01:56:4b:99:e3:dd:5d:fb:1c:6b:63:06:b8:
    64:a3:af:3b:d3:ed:0b:d5:67:77:1b:68:56:72:dc:
    45:f1

But for some reason, the values in the OpenSSH format look different

$ base64 -d | hexdump -C
b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAMwAAAAtzc2gtZW
QyNTUxOQAAACBhArefq3UNoXUQzWm84lNXZn0khT4OfVyfqvJSzLMwmwAAAJiIKZ2BiCmd
gQAAAAtzc2gtZWQyNTUxOQAAACBhArefq3UNoXUQzWm84lNXZn0khT4OfVyfqvJSzLMwmw
AAAECgfh9gYXZZFTD99yiE6RssIfgALC5/i6NX61a1rJgK+2ECt5+rdQ2hdRDNabziU1dm
fSSFPg59XJ+q8lLMszCbAAAAEXJvb3RAaW50ZWwteDg2LTY0AQIDBA==
00000000  6f 70 65 6e 73 73 68 2d  6b 65 79 2d 76 31 00 00  |openssh-key-v1..|
00000010  00 00 04 6e 6f 6e 65 00  00 00 04 6e 6f 6e 65 00  |...none....none.|
00000020  00 00 00 00 00 00 01 00  00 00 33 00 00 00 0b 73  |..........3....s|
00000030  73 68 2d 65 64 32 35 35  31 39 00 00 00 20 61 02  |sh-ed25519... a.|
00000040  b7 9f ab 75 0d a1 75 10  cd 69 bc e2 53 57 66 7d  |...u..u..i..SWf}|
00000050  24 85 3e 0e 7d 5c 9f aa  f2 52 cc b3 30 9b 00 00  |$.>.}\...R..0...|
00000060  00 98 88 29 9d 81 88 29  9d 81 00 00 00 0b 73 73  |...)...)......ss|
00000070  68 2d 65 64 32 35 35 31  39 00 00 00 20 61 02 b7  |h-ed25519... a..|
00000080  9f ab 75 0d a1 75 10 cd  69 bc e2 53 57 66 7d 24  |..u..u..i..SWf}$|
00000090  85 3e 0e 7d 5c 9f aa f2  52 cc b3 30 9b 00 00 00  |.>.}\...R..0....|
000000a0  40 a0 7e 1f 60 61 76 59  15 30 fd f7 28 84 e9 1b  |@.~.`avY.0..(...|
000000b0  2c 21 f8 00 2c 2e 7f 8b  a3 57 eb 56 b5 ac 98 0a  |,!..,....W.V....|
000000c0  fb 61 02 b7 9f ab 75 0d  a1 75 10 cd 69 bc e2 53  |.a....u..u..i..S|
000000d0  57 66 7d 24 85 3e 0e 7d  5c 9f aa f2 52 cc b3 30  |Wf}$.>.}\...R..0|
000000e0  9b 00 00 00 11 72 6f 6f  74 40 69 6e 74 65 6c 2d  |.....root@intel-|
000000f0  78 38 36 2d 36 34 01 02  03 04                    |x86-64....|

I was looking into the following part, which should be a public key

61 02 b7 9f ab 75 0d a1 75 10  cd 69 bc e2 53 57 66 7d 24 85 3e 0e 7d 5c 9f aa  f2 52 cc b3 30 9b

so indeed if the PEM key was generated from this OpenSSH keye, there is probably something wrong as the values do not match

beldmit · 2021-05-26T08:49:04Z

I will check as the self-tests passed with my patch.

beldmit · 2021-05-28T15:05:39Z

openssl pkey -in test_ed25519.pem -text
gives me the output

MC4CAQAwBQYDK2VwBCIEIG2Z9l0q+9au2Mun6dA2dwPXVs8mDcYAQSPuo+/1IXae
-----END PRIVATE KEY-----
ED25519 Private-Key:
priv:
    6d:99:f6:5d:2a:fb:d6:ae:d8:cb:a7:e9:d0:36:77:
    03:d7:56:cf:26:0d:c6:00:41:23:ee:a3:ef:f5:21:
    76:9e
pub:
    ee:a4:01:56:4b:99:e3:dd:5d:fb:1c:6b:63:06:b8:
    64:a3:af:3b:d3:ed:0b:d5:67:77:1b:68:56:72:dc:
    45:f1

After extracting the pubkey with locally built openssh via ./ssh-keygen -y -f test_ed25519.pem
I get

ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIO6kAVZLmePdXfsca2MGuGSjrzvT7QvVZ3cbaFZy3EXx

Decoding the value, I get

$ base64 -d|hexdump -C
AAAAC3NzaC1lZDI1NTE5AAAAIO6kAVZLmePdXfsca2MGuGSjrzvT7QvVZ3cbaFZy3EXx

00000000  00 00 00 0b 73 73 68 2d  65 64 32 35 35 31 39 00  |....ssh-ed25519.|
00000010  00 00 20 ee a4 01 56 4b  99 e3 dd 5d fb 1c 6b 63  |.. ...VK...]..kc|
00000020  06 b8 64 a3 af 3b d3 ed  0b d5 67 77 1b 68 56 72  |..d..;....gw.hVr|
00000030  dc 45 f1                                          |.E.|

that are exactly the same bytes.

beldmit · 2021-05-29T12:14:31Z

@martelletto would you mind to test the attached patch with opensc?
opensc.txt

martelletto · 2021-05-29T14:13:44Z

@beldmit The diff works and looks good at a glance. I will give it a closer look early next week.

beldmit · 2021-05-29T14:58:38Z

Great!

martelletto · 2021-06-01T09:10:51Z

@beldmit Just like we call d2i_ASN1_OCTET_STRING() on CKA_EC_POINT in pkcs11_fetch_eddsa_pubkey(), we need to call i2d_ASN1_OCTET_STRING() on CKA_EC_POINT when looking up the keys in eddsa_do_sign_pkcs11(). I had this modification on my local tree when I applied your patch, which is why it worked for me. Two questions (or more):

Which PKCS#11 provider did you use to test the functionality, where CKA_EC_POINT is not an ASN1-encoded octet string? Shouldn't we make sure this PKCS#11 provider and OpenSC align their interfaces first, to avoid implementing compatibility quirks in ssh-pkcs11.c? This inconsistency on the format of CKA_EC_POINT will make eddsa_do_sign_pkcs11() particularly (and unnecessarily) complex to implement;
Is there a reason eddsa_do_sign_pkcs11() populates key_filter inside the TAILQ_FOREACH() loop, or could we move the code to the function's prelude and perform those steps once?

beldmit · 2021-06-01T09:31:56Z

@beldmit Just like we call d2i_ASN1_OCTET_STRING() on CKA_EC_POINT in pkcs11_fetch_eddsa_pubkey(), we need to call i2d_ASN1_OCTET_STRING() on CKA_EC_POINT when looking up the keys in eddsa_do_sign_pkcs11(). I had this modification on my local tree when I applied your patch, which is why it worked for me. Two questions (or more):

May I ask you to commit it against the top of my patch? Totally missed it.

Which PKCS#11 provider did you use to test the functionality, where CKA_EC_POINT is not an ASN1-encoded octet string? Shouldn't we make sure this PKCS#11 provider and OpenSC align their interfaces first, to avoid implementing compatibility quirks in ssh-pkcs11.c? This inconsistency on the format of CKA_EC_POINT will make eddsa_do_sign_pkcs11() particularly (and unnecessarily) complex to implement;

I tested it against SoftHSM as it was the only available implementation I had.

Is there a reason eddsa_do_sign_pkcs11() populates key_filter inside the TAILQ_FOREACH() loop, or could we move the code to the function's prelude and perform those steps once?

Unfortunately yes. For RSA and ECDSA this information is cached via the RSA_METHOD/EC_KEY_METHOD structures.
See pkcs11_rsa_wrap and pkcs11_ecdsa_wrap for the details. The Edwards curves don't have such specifics structures to store the data so I had to implement some search.

martelletto · 2021-06-02T11:38:35Z

May I ask you to commit it against the top of my patch? Totally missed it.

I've placed a commit with suggestions here: martelletto@afe336c. I couldn't push to your branch (permission denied). Please feel free to squash/cherry-pick/modify as you see fit.

I tested it against SoftHSM as it was the only available implementation I had.

I was a bit puzzled by this, so I had a look. There's a difference in the way ECDSA and EDDSA keys are imported by SoftHSM2's softhsm2-util: ECDSA keys have their CKA_EC_POINT attribute set to an DER-encoded blob, while EDDSA keys have their CKA_EC_POINT attribute set to a raw octet string. AFAICT, the PKCS#11 3.0 spec is clear: CKA_EC_POINT should be DER-encoded in both cases. I suggest we bring this to the attention of SoftHSM's developers, drop support for it in this PR, and move on (that's what's implemented in my 'suggestions' commit above).

beldmit · 2021-06-02T14:05:13Z

@martelletto, do you have any contact with SoftHSM developers?

martelletto · 2021-06-02T15:28:40Z

@beldmit No. :(

nisamuel · 2021-06-09T06:25:18Z

Hi @beldmit ,
Thanks for your response 2 weeks back --
"
openssl pkey -in test_ed25519.pem -text
gives me the output
...
...
"

Have you tried importing an Ed25519 PEM key or mine on your sshd, followed by testing a client connecting to it ?
My observation is that the client fails to verify the kex signature from the sshd that imported these keys.
Any help is appreciated.

mfriedl · 2021-06-16T09:24:23Z

ssh-agent.c

-		goto send;
+
+	if ((sshkey_type_plain(id->key->type) == KEY_ED25519) && (id->key->flags & SSHKEY_FLAG_EXT)) {
+		if ((r = sshkey_sign_pkcs11(id->key, &signature, &slen,


why do you need to add this function? RSA and ECDSA do not need this.

Both RSA and ECDSA refer to the custom OpenSSL method so offload to pkcs11 happens implicitly. EdDSA does not have such API on the OpenSSL level so I had to offload it explicitly.

Jakuje reviewed Feb 25, 2021

View changes

beldmit force-pushed the pkcs11_edwards branch from bff4208 to c40d146 Compare February 25, 2021 17:05

Jakuje approved these changes Mar 1, 2021

View changes

beldmit mentioned this pull request Apr 13, 2021

Conversion of SSH key with passphrase into OpenSSL PEM format | 'unable to load Private Key' openssl/openssl#14854

Closed

martelletto reviewed May 24, 2021

View changes

ssh-pkcs11.c Outdated Show resolved Hide resolved

beldmit force-pushed the pkcs11_edwards branch from c40d146 to 7185f5d Compare May 28, 2021 13:28

beldmit added 8 commits June 1, 2021 11:07

Necessary constants for Edwards PKCS#11 support

5e1e43d

Detect necessary openssl functions

2ca1077

Allow import Edwards keys from OpenSSL

f9b61bb

Implement the PKCS#11 operations with Ed25519 keys

edaa415

Implement the Edwards PKCS#11 operations in client

40cf5e1

Call the PKCS#11 operations for PKCS#11 Edwards keys

fc988df

Call the PKCS#11 signature for PKCS#11 keys

cfbbdbb

Tests for Edwards PKCS#11 keys and import

f6ace4b

Implement the PKCS#11 operations with Ed25519 keys, OpenSC compatibility

69d9c0c

beldmit force-pushed the pkcs11_edwards branch from cd55fab to 69d9c0c Compare June 1, 2021 09:08

beldmit mentioned this pull request Jun 3, 2021

Ed25519 public key representation opendnssec/SoftHSMv2#634

Open

mfriedl reviewed Jun 16, 2021

View changes

Jakuje mentioned this pull request Dec 16, 2022

Add support for EdDSA keys latchset/pkcs11-provider#145

Closed

Pkcs11 Edwards support #230

Pkcs11 Edwards support #230

beldmit commented Feb 25, 2021

Jakuje Feb 25, 2021

beldmit Feb 25, 2021

beldmit commented Feb 25, 2021

rahsinha2 commented Mar 31, 2021

nisamuel commented May 24, 2021

beldmit commented May 24, 2021

nisamuel commented May 24, 2021

beldmit commented May 24, 2021

martelletto commented May 24, 2021

Jakuje commented May 24, 2021 •

edited

nisamuel commented May 26, 2021

beldmit commented May 26, 2021

Jakuje commented May 26, 2021

beldmit commented May 26, 2021

beldmit commented May 28, 2021

beldmit commented May 29, 2021

martelletto commented May 29, 2021

beldmit commented May 29, 2021

martelletto commented Jun 1, 2021

beldmit commented Jun 1, 2021 •

edited

martelletto commented Jun 2, 2021 •

edited

beldmit commented Jun 2, 2021

martelletto commented Jun 2, 2021

nisamuel commented Jun 9, 2021

mfriedl Jun 16, 2021

beldmit Jun 16, 2021

Pkcs11 Edwards support #230

Are you sure you want to change the base?

Pkcs11 Edwards support #230

Conversation

beldmit commented Feb 25, 2021

Jakuje Feb 25, 2021

Choose a reason for hiding this comment

beldmit Feb 25, 2021

Choose a reason for hiding this comment

beldmit commented Feb 25, 2021

rahsinha2 commented Mar 31, 2021

nisamuel commented May 24, 2021

beldmit commented May 24, 2021

nisamuel commented May 24, 2021

beldmit commented May 24, 2021

martelletto commented May 24, 2021

Jakuje commented May 24, 2021 • edited

nisamuel commented May 26, 2021

beldmit commented May 26, 2021

Jakuje commented May 26, 2021

beldmit commented May 26, 2021

beldmit commented May 28, 2021

beldmit commented May 29, 2021

martelletto commented May 29, 2021

beldmit commented May 29, 2021

martelletto commented Jun 1, 2021

beldmit commented Jun 1, 2021 • edited

martelletto commented Jun 2, 2021 • edited

beldmit commented Jun 2, 2021

martelletto commented Jun 2, 2021

nisamuel commented Jun 9, 2021

mfriedl Jun 16, 2021

Choose a reason for hiding this comment

beldmit Jun 16, 2021

Choose a reason for hiding this comment

Jakuje commented May 24, 2021 •

edited

beldmit commented Jun 1, 2021 •

edited

martelletto commented Jun 2, 2021 •

edited