Encapsulate WebAuthn::PublicKey behaviour #284

ssuttner · 2019-11-29T15:18:02Z

WebAuthn has some custom logic to account for backward compatibility on the deserialization of stored public keys in older formats. See this comment.

Up until now, this logic is was not properly encapsulated so that it could be reused from other parts of the gem, or even to be used as a public API.

Motivated by #222 and in collaboration with @padulafacundo a new method is now exposed to be able to check that a stored public key abides by the expected formatting.
If WebAuthn::PublicKey.deserialize(stored_public_key) succeeds, then a user can be sure the key is valid.

ssuttner · 2019-11-29T15:19:19Z

lib/webauthn/authenticator_assertion_response.rb

@@ -42,7 +42,7 @@ def initialize(authenticator_data:, signature:, user_handle: nil, **options)
 def verify(expected_challenge, expected_origin = nil, public_key:, sign_count:, user_verification: nil,
 rp_id: nil)
 super(expected_challenge, expected_origin, user_verification: user_verification, rp_id: rp_id)
- verify_item(:signature, credential_cose_key(public_key))
+ verify_item(:signature, WebAuthn::PublicKey.deserialize(public_key))


Is there any other part of the gem where we should be calling WebAuthn::PublicKey.deserialize that I'm missing?

Should this

webauthn-ruby/lib/webauthn/authenticator_data/attested_credential_data.rb

Lines 55 to 59 in cdc101c

def valid_credential_public_key?

cose_key = COSE::Key.deserialize(public_key)

!!cose_key.alg

end

be

WebAuthn::PublicKey.deserialize(public_key).valid?

instead?

My question was more directed to places where we actually needed to account for the two formats.

Answering the question you are now asking: I think it could!
But, I think I'd like to have a deeper discussion on whether all the usages of COSE::Key.deserialize should be replaced with WebAuthn::PublickKey.deserialize or not.

It's a pretty interesting discussion to have.
Do we want to make it part of this PR, or discuss separately?
I'm fine either way.

My question was more directed to places where we actually needed to account for the two formats.

Answering the question you are now asking: I think it could!
But, I think I'd like to have a deeper discussion on whether all the usages of COSE::Key.deserialize should be replaced with WebAuthn::PublickKey.deserialize or not.

I think in most cases "Yes", given that the WebAuthn spec builds extra domain logic on top of COSE Keys (in https://www.w3.org/TR/webauthn-2/#sctn-attested-credential-data). So ideally I imagine no references to COSE key other than from WebAuthn::PublicKey, so that every "webauthn specific key logic" is contained just there.

For example, by moving the validation of the "alg" to WebAuthn::PublicKey#valid?.

It's a pretty interesting discussion to have.
Do we want to make it part of this PR, or discuss separately?

Separate PR is fine.
This provides value already in it's current form I think.

I'm fine either way.

Perfect! Let's keep working on this on separate PRs.

ssuttner · 2019-11-29T15:20:25Z

spec/webauthn/public_key_spec.rb

+ end
+ let(:cose_public_key) do
+ Base64.urlsafe_decode64(
+ "pQECAyYgASFYIPJKd_-Rl0QtQwbLggjGC_EbUFIMriCkdc2yuaukkBuNIlggaBsBjCwnMzFL7OUGJNm4b-HVpFNUa_NbsHGARuYKHfU"


Is there a better place to define this cose_public_key? Maybe the seeds?

grzuy

Nice!

Thanks @ssuttner and @padulafacundo 🎉

grzuy · 2019-11-29T17:52:14Z

lib/webauthn/authenticator_assertion_response.rb

 WebAuthn::SignatureVerifier
- .new(credential_cose_key.alg, credential_cose_key.to_pkey)
+ .new(webauthn_public_key.cose_key.alg, webauthn_public_key.pkey)


webauthn_public_key.alg?

Yeah, let's expose that method as well 👍
Good call out!

grzuy · 2019-11-29T17:53:43Z

lib/webauthn/authenticator_assertion_response.rb

@@ -2,11 +2,11 @@

 require "cose/algorithm"
 require "cose/key"
-require "webauthn/attestation_statement/fido_u2f/public_key"


Should we also remove cose requires above?

You are right!

grzuy · 2019-11-29T17:55:33Z

lib/webauthn/public_key.rb

+
+require "webauthn/attestation_statement/fido_u2f/public_key"
+require "cose/key"
+require "cose/key/ec2"


"cose/key" requires all keys already.

grzuy · 2019-11-29T18:06:12Z

lib/webauthn/public_key.rb

+ )
+ else
+ COSE::Key.deserialize(public_key)
+ end


I don't think rubocop has a cop for this, which I would enable if supported, but I think I prefer not having indentation depend of the length of a variable name. Smells a bit weird to me that I would need to update ~20 lines if I decide to change the name of cose_key variable to key e.g.

What do you think about?

diff --git a/lib/webauthn/public_key.rb b/lib/webauthn/public_key.rb index 65521bd..5fc2e99 100644 --- a/lib/webauthn/public_key.rb +++ b/lib/webauthn/public_key.rb @@ -8,26 +8,27 @@ require "cose/algorithm" module WebAuthn class PublicKey def self.deserialize(public_key) - cose_key = if WebAuthn::AttestationStatement::FidoU2f::PublicKey.uncompressed_point?(public_key) - # Gem version v1.11.0 and lower, used to behave so that Credential#public_key - # returned an EC P-256 uncompressed point. - # - # Because of https://github.com/cedarcode/webauthn-ruby/issues/137 this was changed - # and Credential#public_key started returning the unchanged COSE_Key formatted - # credentialPublicKey (as in https://www.w3.org/TR/webauthn/#credentialpublickey). - # - # Given that the credential public key is expected to be stored long-term by the gem - # user and later be passed as the public_key argument in the - # AuthenticatorAssertionResponse.verify call, we then need to support the two formats. - COSE::Key::EC2.new( - alg: COSE::Algorithm.by_name("ES256").id, - crv: 1, - x: public_key[1..32], - y: public_key[33..-1] - ) - else - COSE::Key.deserialize(public_key) - end + cose_key = + if WebAuthn::AttestationStatement::FidoU2f::PublicKey.uncompressed_point?(public_key) + # Gem version v1.11.0 and lower, used to behave so that Credential#public_key + # returned an EC P-256 uncompressed point. + # + # Because of https://github.com/cedarcode/webauthn-ruby/issues/137 this was changed + # and Credential#public_key started returning the unchanged COSE_Key formatted + # credentialPublicKey (as in https://www.w3.org/TR/webauthn/#credentialpublickey). + # + # Given that the credential public key is expected to be stored long-term by the gem + # user and later be passed as the public_key argument in the + # AuthenticatorAssertionResponse.verify call, we then need to support the two formats. + COSE::Key::EC2.new( + alg: COSE::Algorithm.by_name("ES256").id, + crv: 1, + x: public_key[1..32], + y: public_key[33..-1] + ) + else + COSE::Key.deserialize(public_key) + end new(cose_key: cose_key) end

There is not a rubocop rule agains what you just proposed.
But initially, I had:

cose_key = if WebAuthn::AttestationStatement::FidoU2f::PublicKey.uncompressed_point?(public_key) # Gem version v1.11.0 and lower, used to behave so that Credential#public_key # returned an EC P-256 uncompressed point. # # Because of https://github.com/cedarcode/webauthn-ruby/issues/137 this was changed # and Credential#public_key started returning the unchanged COSE_Key formatted # credentialPublicKey (as in https://www.w3.org/TR/webauthn/#credentialpublickey). # # Given that the credential public key is expected to be stored long-term by the gem # user and later be passed as the public_key argument in the # AuthenticatorAssertionResponse.verify call, we then need to support the two formats. COSE::Key::EC2.new( alg: COSE::Algorithm.by_name("ES256").id, crv: 1, x: public_key[1..32], y: public_key[33..-1] ) else COSE::Key.deserialize(public_key) end

And rubocop complained with the following errors:

lib/webauthn/public_key.rb:21:9: C: Layout/IndentationWidth: Use 2 (not -9) spaces for indentation. COSE::Key::EC2.new( ^^^^^^^^^ lib/webauthn/public_key.rb:27:7: C: Layout/ElseAlignment: Align else with if. else ^^^^ lib/webauthn/public_key.rb:29:7: W: Layout/EndAlignment: end at 29, 6 is not aligned with if at 10, 17. end ^^^

When I asked rubocop to fix it for me by using rubocop -a, I got the output that triggered this discussion.
I was just trying to abide by whatever rubocop suggested.

No worries, I agree with you on the benefits of refactoring it to not depend on the variable name.
Thanks for bringing this up! 👍

Oh, I see.

Didn't know that rubocop auto-corrected to that... :-/

Thanks for the update

grzuy · 2019-11-29T18:09:43Z

spec/webauthn/public_key_spec.rb

+ end
+ end
+ end
+end


grzuy · 2019-11-29T18:22:10Z

lib/webauthn/authenticator_assertion_response.rb

@@ -42,7 +42,7 @@ def initialize(authenticator_data:, signature:, user_handle: nil, **options)
 def verify(expected_challenge, expected_origin = nil, public_key:, sign_count:, user_verification: nil,
 rp_id: nil)
 super(expected_challenge, expected_origin, user_verification: user_verification, rp_id: rp_id)
- verify_item(:signature, credential_cose_key(public_key))
+ verify_item(:signature, WebAuthn::PublicKey.deserialize(public_key))


Should this

webauthn-ruby/lib/webauthn/authenticator_data/attested_credential_data.rb

Lines 55 to 59 in cdc101c

def valid_credential_public_key?

cose_key = COSE::Key.deserialize(public_key)

!!cose_key.alg

end

be

WebAuthn::PublicKey.deserialize(public_key).valid?

instead?

grzuy

Thanks again!

grzuy · 2019-12-17T14:14:07Z

Will merge after fixup squshing...

* this will allow to keep the code isolated and more maintainable * at the same time, the WebAuthn::PublicKey.deserialize interface will be available for public use

* clean up public interface to avoid extra method navigation when trying to fetch the 'alg' value

ssuttner added area: authentication verification area: CBOR parsing labels Nov 29, 2019

ssuttner requested a review from grzuy November 29, 2019 15:18

ssuttner commented Nov 29, 2019

View reviewed changes

ssuttner force-pushed the webauthn_keys branch from 672f1d1 to cdc101c Compare November 29, 2019 15:24

grzuy reviewed Nov 29, 2019

View reviewed changes

grzuy changed the title ~~Encapsulate Webauthn::PublicKey behaviour~~ Encapsulate WebAuthn::PublicKey behaviour Nov 29, 2019

grzuy self-requested a review December 16, 2019 14:15

grzuy approved these changes Dec 16, 2019

View reviewed changes

ssuttner added 3 commits December 17, 2019 11:37

refactor: create a class to deserialize stored public keys

b162625

* this will allow to keep the code isolated and more maintainable * at the same time, the WebAuthn::PublicKey.deserialize interface will be available for public use

refactor: use WebAuthn::PublicKey on assertion

ae94745

feat(WebAuthn::PublicKey): expose 'alg' method

b5a34ff

* clean up public interface to avoid extra method navigation when trying to fetch the 'alg' value

ssuttner force-pushed the webauthn_keys branch from eaa0b30 to b5a34ff Compare December 17, 2019 14:37

grzuy merged commit 114a96d into master Dec 17, 2019

grzuy deleted the webauthn_keys branch December 17, 2019 14:53

grzuy mentioned this pull request Jan 3, 2020

Method to validate a public key #222

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Encapsulate WebAuthn::PublicKey behaviour #284

Encapsulate WebAuthn::PublicKey behaviour #284

ssuttner commented Nov 29, 2019

ssuttner Nov 29, 2019

grzuy Nov 29, 2019

ssuttner Dec 4, 2019

grzuy Dec 16, 2019 •

edited

Loading

ssuttner Dec 17, 2019

ssuttner Nov 29, 2019

grzuy left a comment

grzuy Nov 29, 2019

ssuttner Dec 4, 2019

grzuy Nov 29, 2019

ssuttner Dec 4, 2019

grzuy Nov 29, 2019

grzuy Nov 29, 2019

ssuttner Dec 4, 2019

grzuy Dec 16, 2019

grzuy Nov 29, 2019

grzuy Nov 29, 2019

grzuy left a comment

grzuy commented Dec 17, 2019

	def valid_credential_public_key?
	cose_key = COSE::Key.deserialize(public_key)

	!!cose_key.alg
	end

Encapsulate WebAuthn::PublicKey behaviour #284

Encapsulate WebAuthn::PublicKey behaviour #284

Conversation

ssuttner commented Nov 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grzuy Dec 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grzuy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grzuy left a comment

Choose a reason for hiding this comment

grzuy commented Dec 17, 2019

grzuy Dec 16, 2019 •

edited

Loading