Add EdDSA support #55

bdewater · 2020-05-16T05:32:09Z

This relies generic pkey support currently in OpenSSL gem master, slated to be released as 3.0 (ruby/openssl#329). Closes #48

EdDSA in COSE is 'pure' and takes no hash: https://tools.ietf.org/html/rfc8152#section-8.2

The ASN1 structure is described in https://tools.ietf.org/html/rfc5958 and some experimentation of returned values by the OpenSSL bindings using https://lapo.it/asn1js/

spec/cose/sign1_spec.rb

bdewater · 2020-05-16T14:01:03Z

Marked this as draft for now. While the fiddling with ASN1 works I think the OpenSSL gem would benefit from exposing some library functions. I posted a message upstream.

grzuy · 2020-05-16T17:46:51Z

Nice! 👏

grzuy

Thank you!

lib/cose/algorithm/eddsa.rb

lib/cose/key/okp.rb

grzuy · 2020-07-04T19:43:56Z

lib/cose/key/okp.rb

+        rescue OpenSSL::PKey::PKeyError
+          asymmetric_key = pkey.public_to_der
+          public_key_bit_string = OpenSSL::ASN1.decode(asymmetric_key).value.last.value
+          attributes.merge!({ x: public_key_bit_string })


Do you know if this results in the RFC 8032 encoding form?

Yes, but rubber ducking here in public just in case (Cunningham's law 😉)

Taking Ed25519 test 2 vectors from https://www.rfc-editor.org/rfc/rfc8032#section-7 (just like the OpenSSL gem test suite):

priv_pem = <<~EOF -----BEGIN PRIVATE KEY----- MC4CAQAwBQYDK2VwBCIEIEzNCJso/5banbbDRuwRTg9bijGfNaumJNqM9u1PuKb7 -----END PRIVATE KEY----- EOF priv = OpenSSL::PKey.read(priv_pem) cose_priv = COSE::Key::OKP.from_pkey(priv) public_key_hex = cose_priv.x.unpack1("H*") puts public_key_hex.chars.each_slice(32).map(&:join).join("\n") # 3d4017c3e843895a92b70aa74d1b7ebc # 9c982ccf2ec4968cc0cd55f12af4660c

SamSaffron · 2022-10-12T04:52:36Z

@bdewater curious on what is blocking this, perhaps it can be merged as an "optional" protocol and users that want to consume it need to make sure they have the right version of openssl installed?

I am noticing some failures in the wild now https://github.com/solokeys/solo1 is -8 by default.

SamSaffron · 2022-10-12T05:04:51Z

I wonder if another option is leaning on rbnacl? would that make distribution easier?

bdewater · 2022-10-12T13:24:54Z

I forgot about this! I'll rebase it later today and see where things stand with OpenSSL gem 3.0 now that it's out.

I am noticing some failures in the wild now https://github.com/solokeys/solo1 is -8 by default.

🤔 doesn't it support -7 (ES256)? IIRC that's required for authenticators by the WebAuthn specification, you shouldn't see -8 unless you specify it in pubKeyCredParams?

SamSaffron · 2022-10-12T23:30:50Z

doesn't it support -7 (ES256)? IIRC that's required for authenticators by the WebAuthn specification, you shouldn't see -8 unless you specify it in pubKeyCredParams?

Great call @bdewater ...

@gschlager did some research and it turns out that dropping -258 and -259 from our list resolves the issue, so the issue we were experiencing was not even due to -8

bdewater · 2022-10-13T01:47:37Z

Rebased, I'll try this branch in the latest alpha of webauthn-ruby and with a modern Yubikey in the next few days to make sure it's all working as expected. I'm a little hazy on some details here after 2 years 😅

SamSaffron · 2022-10-13T03:57:11Z

Thanks so much @bdewater !

bdewater · 2022-10-14T14:59:31Z

@gschlager did some research and it turns out that dropping -258 and -259 from our list resolves the issue, so the issue we were experiencing was not even due to -8

If that's a bug in this gem, I'd appreciate a bug report (or confirmation it wasn't) 😄 . I don't know which authenticators support -258 and -259 at the moment.

santiagorodriguez96

Hey @bdewater! This is amazing! Thank you so much! 👏

I tried giving this a try using webauthn-rails-demo-app and webauthn-ruby latest alpha. I also used a YubiKey Security Key C NFC with firmware version v5.4.3. I pulled up a branch with the changes I made in order to be able to test this so you can take a look at them if you want 🙂 – maybe I did something wrong?

I had no problem registering the key, but I did see an error when trying to authenticate with it:

Error log

Started POST "/session/callback" for ::1 at 2022-10-21 14:51:23 -0300
Processing by SessionsController#callback as JSON
Parameters: {"type"=>"public-key", "id"=>"_uuj5ZnqKVbZ8Y9l14K46LFy2msjU3fwKCFOVRzStZeXucOxGUJNctzrOcjSqMq1_8HRUb3sr6S--C1gNpTzvE3VGAUf7qRXyOlZ2kL_3W5pBF6FN8jg3uUjrWr8MtPbt4UtviwwO11BBkD0Nm6iKrCsXr4BF_Qrra1iwNYn2OQ", "rawId"=>"_uuj5ZnqKVbZ8Y9l14K46LFy2msjU3fwKCFOVRzStZeXucOxGUJNctzrOcjSqMq1_8HRUb3sr6S--C1gNpTzvE3VGAUf7qRXyOlZ2kL_3W5pBF6FN8jg3uUjrWr8MtPbt4UtviwwO11BBkD0Nm6iKrCsXr4BF_Qrra1iwNYn2OQ", "response"=>{"clientDataJSON"=>"eyJ0eXBlIjoid2ViYXV0aG4uZ2V0IiwiY2hhbGxlbmdlIjoiRnBwMXJaSG9UVGZhTXdTV2pYaGk1RzR5X3Bpc2xPdG16aWp1SG1qY1VyOCIsIm9yaWdpbiI6Imh0dHA6Ly9sb2NhbGhvc3Q6MzAwMCIsImNyb3NzT3JpZ2luIjpmYWxzZSwib3RoZXJfa2V5c19jYW5fYmVfYWRkZWRfaGVyZSI6ImRvIG5vdCBjb21wYXJlIGNsaWVudERhdGFKU09OIGFnYWluc3QgYSB0ZW1wbGF0ZS4gU2VlIGh0dHBzOi8vZ29vLmdsL3lhYlBleCJ9", "authenticatorData"=>"SZYN5YgOjGh0NBcPZHZgW4_krrmihjLHmVzzuoMdl2MBAAAACA", "signature"=>"dpwa1CgNWylpsbM3cIQBZURasn9m7JWMGZNSJ1Ig7a92eDqqeERbKtMXeOnt8iZ3FKpzgiw7-rlhyQZ6EdiFBA", "userHandle"=>nil}, "session"=>{"type"=>"public-key", "id"=>"_uuj5ZnqKVbZ8Y9l14K46LFy2msjU3fwKCFOVRzStZeXucOxGUJNctzrOcjSqMq1_8HRUb3sr6S--C1gNpTzvE3VGAUf7qRXyOlZ2kL_3W5pBF6FN8jg3uUjrWr8MtPbt4UtviwwO11BBkD0Nm6iKrCsXr4BF_Qrra1iwNYn2OQ", "rawId"=>"_uuj5ZnqKVbZ8Y9l14K46LFy2msjU3fwKCFOVRzStZeXucOxGUJNctzrOcjSqMq1_8HRUb3sr6S--C1gNpTzvE3VGAUf7qRXyOlZ2kL_3W5pBF6FN8jg3uUjrWr8MtPbt4UtviwwO11BBkD0Nm6iKrCsXr4BF_Qrra1iwNYn2OQ", "response"=>{"clientDataJSON"=>"eyJ0eXBlIjoid2ViYXV0aG4uZ2V0IiwiY2hhbGxlbmdlIjoiRnBwMXJaSG9UVGZhTXdTV2pYaGk1RzR5X3Bpc2xPdG16aWp1SG1qY1VyOCIsIm9yaWdpbiI6Imh0dHA6Ly9sb2NhbGhvc3Q6MzAwMCIsImNyb3NzT3JpZ2luIjpmYWxzZSwib3RoZXJfa2V5c19jYW5fYmVfYWRkZWRfaGVyZSI6ImRvIG5vdCBjb21wYXJlIGNsaWVudERhdGFKU09OIGFnYWluc3QgYSB0ZW1wbGF0ZS4gU2VlIGh0dHBzOi8vZ29vLmdsL3lhYlBleCJ9", "authenticatorData"=>"SZYN5YgOjGh0NBcPZHZgW4_krrmihjLHmVzzuoMdl2MBAAAACA", "signature"=>"dpwa1CgNWylpsbM3cIQBZURasn9m7JWMGZNSJ1Ig7a92eDqqeERbKtMXeOnt8iZ3FKpzgiw7-rlhyQZ6EdiFBA", "userHandle"=>nil}}}
User Load (0.7ms)  SELECT "users".* FROM "users" WHERE "users"."username" = $1 LIMIT $2  [["username", "Yorick"], ["LIMIT", 1]]
↳ app/controllers/sessions_controller.rb:28:in `callback'
Credential Load (0.7ms)  SELECT "credentials".* FROM "credentials" WHERE "credentials"."user_id" = $1 AND "credentials"."external_id" = $2 LIMIT $3  [["user_id", 5], ["external_id", "/uuj5ZnqKVbZ8Y9l14K46LFy2msjU3fwKCFOVRzStZeXucOxGUJNctzrOcjSqMq1/8HRUb3sr6S++C1gNpTzvE3VGAUf7qRXyOlZ2kL/3W5pBF6FN8jg3uUjrWr8MtPbt4UtviwwO11BBkD0Nm6iKrCsXr4BF/Qrra1iwNYn2OQ="], ["LIMIT", 1]]
↳ app/controllers/sessions_controller.rb:32:in `callback'
Completed 500 Internal Server Error in 15ms (ActiveRecord: 3.2ms | Allocations: 7728)



NoMethodError (undefined method `to_pkey' for #<OpenSSL::PKey::PKey:0x00007fd568d18950 oid=ED25519>
Did you mean?  to_query):

app/controllers/sessions_controller.rb:35:in `callback'

Digging in a little bit, seems that the error is caused in COSE::Algorithm::EdDSA#valid_signature?, where we are receiving a OpenSSL::PKey::PKey as key and calling .to_pkey to it, which raises the undefined method error.

Worth noting that this same key works correctly – both for registration and authentication – if I remove the restriction for the relying party to only accept EdDSA.

lib/cose/algorithm/eddsa.rb

spec/cose/key/okp_spec.rb

bdewater · 2022-10-21T21:28:00Z

Thank you! The demo app was giving me some trouble to get working and I didn't have the bandwidth to fix it (or build a new one, it's been on my wish list 😁). I committed your suggestions :)

Worth noting that this same key works correctly – both for registration and authentication – if I remove the restriction for the relying party to only accept EdDSA.

You probably got an ES256 (-7) key in that case.

santiagorodriguez96

I think this is good to go! Looks great. Thanks again for your effort on this! 💪

Thank you! The demo app was giving me some trouble to get working and I didn't have the bandwidth to fix it (or build a new one, it's been on my wish list 😁). I committed your suggestions :)

That's interesting! 👀 Out of curiosity do you feel that there is something missing from the current demo app? I have some bandwidth right now so I'm open to help there 🙂 Also, a couple of years ago I was working on a demo app for WebAuthn as a second factor that I had to drop off but I'm planning to retake it and release it soon.

Gemfile

santiagorodriguez96 · 2022-10-24T14:56:13Z

lib/cose/algorithm/eddsa.rb

+      end
+
+      def valid_signature?(key, signature, verification_data)
+        pkey = to_pkey(key)


Not a blocker for this PR: I see that we are indirectly testing it in spec/cose/sign_spec.rb and spec/cose/sign1_spec.rb but I wonder if having COSE::Algorithm::SignatureAlgorithm.verify? more thoroughly tested could have helped us to find the error earlier by making the tests to fail.

Seems that it's the idea for the method to support receiving both a COSE::Key or an OpenSSL::PKey, so maybe it's a good idea to test both scenarios? It feels to me that it could help as documentation for the method as well.

brauliomartinezlm · 2022-10-26T01:03:16Z

@bdewater @santiagorodriguez96 feel free to merge this at this point when you think it's ready. I'm happy to cut a release with this before EOW (going on vacation for 10 days on Saturday) after it's merged.

Thank you for doing this @bdewater and @santiagorodriguez96 for helping on the testing and tweaking! ❤️

santiagorodriguez96

LGTM 👍

bdewater · 2022-10-28T00:30:03Z

@brauliomartinezlm I can't merge in this repo - and have a good vacation! :)

brauliomartinezlm · 2022-10-28T01:28:21Z

I can't merge in this repo

@bdewater apologies. Just sent you an invite 🙂

and have a good vacation! :)

Thank you!

I'll do release tomorrow for this!

bdewater force-pushed the eddsa branch from ab08466 to 654bec6 Compare May 16, 2020 05:58

bdewater commented May 16, 2020

View reviewed changes

spec/cose/sign1_spec.rb Outdated Show resolved Hide resolved

bdewater marked this pull request as draft May 16, 2020 13:35

bdewater mentioned this pull request May 16, 2020

pkey: add more support for 'generic' pkey types ruby/openssl#329

Merged

grzuy self-requested a review May 16, 2020 17:47

bdewater force-pushed the eddsa branch from 654bec6 to 8b82dd2 Compare May 21, 2020 02:41

bdewater mentioned this pull request Jun 8, 2020

Support EdDSA credential keys cedarcode/webauthn-ruby#276

Closed

grzuy reviewed Jul 4, 2020

View reviewed changes

bdewater mentioned this pull request Jun 24, 2022

Add support for OpenSSL 3.0 #61

Merged

bdewater closed this Oct 13, 2022

bdewater force-pushed the eddsa branch from 8b82dd2 to abc42a1 Compare October 13, 2022 00:44

bdewater reopened this Oct 13, 2022

bdewater marked this pull request as ready for review October 13, 2022 01:07

bdewater force-pushed the eddsa branch 2 times, most recently from 183e854 to 0e68121 Compare October 13, 2022 01:21

santiagorodriguez96 reviewed Oct 21, 2022

View reviewed changes

lib/cose/algorithm/eddsa.rb Outdated Show resolved Hide resolved

spec/cose/key/okp_spec.rb Outdated Show resolved Hide resolved

bdewater force-pushed the eddsa branch from 0e68121 to 4961d4b Compare October 21, 2022 21:25

santiagorodriguez96 approved these changes Oct 24, 2022

View reviewed changes

Add EdDSA support

42d3f04

bdewater force-pushed the eddsa branch from 4961d4b to 42d3f04 Compare October 27, 2022 02:18

santiagorodriguez96 approved these changes Oct 27, 2022

View reviewed changes

santiagorodriguez96 merged commit 3c28ad3 into cedarcode:master Oct 28, 2022

bdewater deleted the eddsa branch October 28, 2022 15:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add EdDSA support #55

Add EdDSA support #55

bdewater commented May 16, 2020 •

edited

Loading

bdewater commented May 16, 2020

grzuy commented May 16, 2020

grzuy left a comment

grzuy Jul 4, 2020

bdewater Oct 17, 2022

SamSaffron commented Oct 12, 2022 •

edited

Loading

SamSaffron commented Oct 12, 2022

bdewater commented Oct 12, 2022 •

edited

Loading

SamSaffron commented Oct 12, 2022

bdewater commented Oct 13, 2022

SamSaffron commented Oct 13, 2022

bdewater commented Oct 14, 2022 •

edited

Loading

santiagorodriguez96 left a comment

bdewater commented Oct 21, 2022

santiagorodriguez96 left a comment

santiagorodriguez96 Oct 24, 2022

brauliomartinezlm commented Oct 26, 2022

santiagorodriguez96 left a comment

bdewater commented Oct 28, 2022

brauliomartinezlm commented Oct 28, 2022 •

edited

Loading

Add EdDSA support #55

Add EdDSA support #55

Conversation

bdewater commented May 16, 2020 • edited Loading

bdewater commented May 16, 2020

grzuy commented May 16, 2020

grzuy left a comment

Choose a reason for hiding this comment

grzuy Jul 4, 2020

Choose a reason for hiding this comment

bdewater Oct 17, 2022

Choose a reason for hiding this comment

SamSaffron commented Oct 12, 2022 • edited Loading

SamSaffron commented Oct 12, 2022

bdewater commented Oct 12, 2022 • edited Loading

SamSaffron commented Oct 12, 2022

bdewater commented Oct 13, 2022

SamSaffron commented Oct 13, 2022

bdewater commented Oct 14, 2022 • edited Loading

santiagorodriguez96 left a comment

Choose a reason for hiding this comment

bdewater commented Oct 21, 2022

santiagorodriguez96 left a comment

Choose a reason for hiding this comment

santiagorodriguez96 Oct 24, 2022

Choose a reason for hiding this comment

brauliomartinezlm commented Oct 26, 2022

santiagorodriguez96 left a comment

Choose a reason for hiding this comment

bdewater commented Oct 28, 2022

brauliomartinezlm commented Oct 28, 2022 • edited Loading

bdewater commented May 16, 2020 •

edited

Loading

SamSaffron commented Oct 12, 2022 •

edited

Loading

bdewater commented Oct 12, 2022 •

edited

Loading

bdewater commented Oct 14, 2022 •

edited

Loading

brauliomartinezlm commented Oct 28, 2022 •

edited

Loading