feat: add API methods for providing received extension information #317

santiagorodriguez96 · 2020-06-02T15:59:01Z

What

Add the possibility of accessing to the client and authenticator extensions outputs in the PublicKeyCredential's models, so that it can be manually validated by the user according to their expectations.

Why

Extracted from WebAuthn spec in the sections 7.1 Registering a New Credential (step 17) and 7.2 Verifying an Authentication Assertion (step 18):

Verify that the values of the client extension outputs in clientExtensionResults and the authenticator extension outputs in the extensions in authData are as expected, considering the client extension input values that were given in options.extensions and any specific policy of the Relying Party regarding unsolicited extensions, i.e., those that were not specified as part of options.extensions. In the general case, the meaning of "are as expected" is specific to the Relying Party and which extensions are in use.

Note: Client platforms MAY enact local policy that sets additional authenticator extensions or client extensions and thus cause values to appear in the authenticator extension outputs or client extension outputs that were not originally specified as part of options.extensions. Relying Parties MUST be prepared to handle such situations, whether it be to ignore the unsolicited extensions or reject the attestation. The Relying Party can make this decision based on local policy and the extensions in use.

Note: Since all extensions are OPTIONAL for both the client and the authenticator, the Relying Party MUST also be prepared to handle cases where none or not all of the requested extensions were acted upon.

spec/webauthn/public_key_credential_spec.rb

santiagorodriguez96 · 2020-06-02T20:09:32Z

The above was an accidental delete of this thread. Sorry about that.

spec/webauthn/public_key_credential_spec.rb

santiagorodriguez96 · 2020-06-03T13:22:17Z

spec/webauthn/public_key_credential_spec.rb

+ let(:attestation_response) do
+ allow_any_instance_of(WebAuthn::FakeAuthenticator::AuthenticatorData)
+ .to receive(:extensions)
+ .and_return(nil)


The following discussion was accidentally deleted by me in an attempt to separate different discussions into different threads – originally it was placed after this comment, in the same thread.
Turns out that if you delete the first message of a thread, you'll actually delete the whole tread 🙃
I was able to re-create it thanks to @marceloeloelo 🥳

The initial comment was:

Is not that clear to me if in this case extensions should return nil or {} 😕

I don't think is possible that one can initialize an WebAuthn::FakeAuthenticator::AuthenticatorData with extensions being nil, given that it has { "fakeExtension" => "fakeExtensionValue" } as the default value.

But, on the other hand, the test fails if we use {} as blank authentication extension input, given that the fake authenticator will consider that extension data is received – extension_data will encode in CBOR the object {} and later extension_data_included_bit will return 1.
So, a little tweak has to be made to this model if we agree that we should consider {} as blank exceptions.

To avoid doing changes to the fake authenticator data I just went with the first one but I would say that we should consider going for the second one and do that little refactor.

To which @bdewater replied:

You can initialize an WebAuthn::FakeAuthenticator::AuthenticatorData with extensions: nil. It's only when the extensions key is not specified by the caller the default value is used. You can think of of Ruby keyword arguments as a hash with default values being merged with caller values :)

Based on my reading of CTAP 2.0 the current behaviour seems correct, extensions are optional and it's presence indicates means they should be processed. It seems it is unlikely the result will be the equivalent of an empty Ruby hash in real life 🤔

Finally, my response was:

You can initialize an WebAuthn::FakeAuthenticator::AuthenticatorData with extensions: nil. It's only when the extensions key is not specified by the caller the default value is used. You can think of of Ruby keyword arguments as a hash with default values being merged with caller values :)

Oh I see, that makes sense! Thanks for the clarification! 🙂

Based on my reading of CTAP 2.0 the current behaviour seems correct, extensions are optional and it's presence indicates means they should be processed. It seems it is unlikely the result will be the equivalent of an empty Ruby hash in real life 🤔

Good to know! Thanks for clarifying that! I supposed that the current behavior was correct that's why I was hesitant to make that refactor without consulting to you guys first 😄

grzuy

Thank you!!

Great to see this happening :-)

spec/webauthn/public_key_credential_spec.rb

grzuy

Thanks for the updates!
Left a few comments.

santiagorodriguez96 · 2020-06-16T17:29:52Z

spec/webauthn/public_key_credential_spec.rb

+ response = client.create(
+ challenge: raw_challenge,
+ extensions: { "txAuthSimple" => "Could you please verify yourself?" }
+ )["response"]


This is just a simple implementation of the code for faking the extensions. Right now, you require an extension when creating the request – either for authentication or registration – and that extension is returned with the same value from the request in both client and authenticator extension outputs.
I was thinking about doing something more realistic that processes each extension input and returns an output that is more similar to what each extension would return if you were using it in a real case based on the documentation, using some kind of ExtensionProcesser. I gave the idea some thinking but decided to keep it simple.

It's worth mention that before these changes, If you call the methods make_credential and get_assertion from FakeAuthenticator – the same goes for create and get from FakeClient which uses those methods – you have to specifically require for an extension in order to then receive an output in the authenticator data. That was not happening before, as we were not specifying the extensions key when initializing AuthenticatorData, so the default extension { "fakeExtension" => "fakeExtensionValue" } was being used.
IMO I like it better this way since no extension output is returned unless you specifically require one, but we can be potentially breaking some tests with this changes 🙂

This is just a simple implementation of the code for faking the extensions. Right now, you require an extension when creating the request – either for authentication or registration – and that extension is returned with the same value from the request in both client and authenticator extension outputs.
I was thinking about doing something more realistic that processes each extension input and returns an output that is more similar to what each extension would return if you were using it in a real case based on the documentation, using some kind of ExtensionProcesser. I gave the idea some thinking but decided to keep it simple.

This is an interesting/cool idea to consider.
For now I agree with you that we can keep it simple.

It's worth mention that before these changes, If you call the methods make_credential and get_assertion from FakeAuthenticator – the same goes for create and get from FakeClient which uses those methods – you have to specifically require for an extension in order to then receive an output in the authenticator data. That was not happening before, as we were not specifying the extensions key when initializing AuthenticatorData, so the default extension { "fakeExtension" => "fakeExtensionValue" } was being used.
IMO I like it better this way since no extension output is returned unless you specifically require one, but we can be potentially breaking some tests with this changes slightly_smiling_face

Makes sense, no extension output by default.

Thanks for the write up.

grzuy

Thanks for the updates!

Looks good - just a minor comment regarding the faking arguments.

We're close :-) 👏

grzuy · 2020-06-16T23:31:49Z

lib/webauthn/fake_client.rb

@@ -41,7 +42,8 @@ def create(
 client_data_hash: client_data_hash,
 user_present: user_present,
 user_verified: user_verified,
- attested_credential_data: attested_credential_data
+ attested_credential_data: attested_credential_data,
+ extensions: extensions


Nice! 👏

This extensions argument is being returned both as the authenticator extension outputs and the client extension outputs in the response from what I understand. That won't happen in reality, right?

Should we allow two arguments here instead? client_extension_outputs: nil and authenticator_extension_outputs: nil, so on top of being more real we can also have more flexibility and control when testing different cases.

This extensions argument is being returned both as the authenticator extension outputs and the client extension outputs in the response from what I understand. That won't happen in reality, right?

Exactly! Yeah I agree that it's not likely to happen though

Should we allow two arguments here instead? client_extension_outputs: nil and authenticator_extension_outputs: nil, so on top of being more real we can also have more flexibility and control when testing different cases.

That was one of the approaches that I took, but I decided not to stay with it because that's not so real either, given that in reality, if I understood correctly, the relying party only requests a client extension to the client and it's that client who later requests an authenticator extension to the authenticator depending on that client extension requested. So in the end, I decided to go with the above approach.
However, I agree that doing this approach will allow us to have more flexibility!

However, I agree that doing this approach will allow us to have more flexibility!

Yup.

I don't think I consider this a blocker though. We can improve later as needed.

lib/webauthn/fake_client.rb

spec/webauthn/public_key_credential_spec.rb

grzuy · 2020-06-16T23:38:08Z

spec/webauthn/public_key_credential_spec.rb

+ response = client.create(
+ challenge: raw_challenge,
+ extensions: { "txAuthSimple" => "Could you please verify yourself?" }
+ )["response"]


This is just a simple implementation of the code for faking the extensions. Right now, you require an extension when creating the request – either for authentication or registration – and that extension is returned with the same value from the request in both client and authenticator extension outputs.
I was thinking about doing something more realistic that processes each extension input and returns an output that is more similar to what each extension would return if you were using it in a real case based on the documentation, using some kind of ExtensionProcesser. I gave the idea some thinking but decided to keep it simple.

This is an interesting/cool idea to consider.
For now I agree with you that we can keep it simple.

It's worth mention that before these changes, If you call the methods make_credential and get_assertion from FakeAuthenticator – the same goes for create and get from FakeClient which uses those methods – you have to specifically require for an extension in order to then receive an output in the authenticator data. That was not happening before, as we were not specifying the extensions key when initializing AuthenticatorData, so the default extension { "fakeExtension" => "fakeExtensionValue" } was being used.
IMO I like it better this way since no extension output is returned unless you specifically require one, but we can be potentially breaking some tests with this changes slightly_smiling_face

Makes sense, no extension output by default.

Thanks for the write up.

santiagorodriguez96 · 2020-06-18T14:55:48Z

spec/webauthn/public_key_credential_spec.rb

+ context "are not received" do
+ let(:public_key_credential) do
+ WebAuthn::PublicKeyCredentialWithAttestation.new(
+ type: type,
+ id: id,
+ raw_id: raw_id,
+ client_extension_outputs: nil,
+ response: attestation_response
+ )
+ end
+
+ it "works" do
+ expect(public_key_credential.verify(challenge)).to be_truthy
+
+ expect(public_key_credential.client_extension_outputs).to be_nil


@grzuy @bdewater –
I still have doubts in terms of what should we return in client_extensions_outputs in each case. Right now we are just returning the same object that is being passed to us. So, it will be something like:

clientExtensionResults client_extension_outputs

nil nil

{} {}

{ "appid" => "true" } { "appid" => "true" }

{ appid: true } { appid: true }

<ActionController::Parameters {"appid"=>true} permitted: false> <ActionController::Parameters {"appid"=>true} permitted: false>

That doesn't smell right for me, so I was thinking that we should consider doing some kind of processing in order to get a more consistent output, something like:

clientExtensionResults client_extension_outputs

nil {}

{} {}

{ "appid" => "true" } { appid: true }

{ appid: true } { appid: true }

<ActionController::Parameters {"appid"=>true} permitted: false> { appid: true }

What do you guys think about this?

Good question...

I don't think we should try making the gem be "too smart" about the semantics of the extensions by doing any data normalization right now.

I am not saying we would never add any normalization, but I think we should release like this, and consider some normalization in the future as extensions mature in the ecosystem.

I want to avoid getting it wrong and needing to re-implement normalization later, meaning a breaking change.

Thanks for raising the concern :-)

grzuy

Thanks again!

grzuy · 2020-06-20T19:42:38Z

lib/webauthn/fake_client.rb

@@ -41,7 +42,8 @@ def create(
 client_data_hash: client_data_hash,
 user_present: user_present,
 user_verified: user_verified,
- attested_credential_data: attested_credential_data
+ attested_credential_data: attested_credential_data,
+ extensions: extensions


However, I agree that doing this approach will allow us to have more flexibility!

Yup.

I don't think I consider this a blocker though. We can improve later as needed.

grzuy · 2020-06-20T19:53:30Z

spec/webauthn/public_key_credential_spec.rb

+ context "are not received" do
+ let(:public_key_credential) do
+ WebAuthn::PublicKeyCredentialWithAttestation.new(
+ type: type,
+ id: id,
+ raw_id: raw_id,
+ client_extension_outputs: nil,
+ response: attestation_response
+ )
+ end
+
+ it "works" do
+ expect(public_key_credential.verify(challenge)).to be_truthy
+
+ expect(public_key_credential.client_extension_outputs).to be_nil


Good question...

I don't think we should try making the gem be "too smart" about the semantics of the extensions by doing any data normalization right now.

I am not saying we would never add any normalization, but I think we should release like this, and consider some normalization in the future as extensions mature in the ecosystem.

I want to avoid getting it wrong and needing to re-implement normalization later, meaning a breaking change.

Thanks for raising the concern :-)

Follow up for #317.

* docs: add documentation for extensions Follow up for #317. * docs: those extensions are defined in the spec - not necessatily implemented by browsers/authenticators * docs: link to published spec - not editor draft * docs: key away from Rails specifics in the documentation * docs: minor grammatic and formatting updates * docs: provide both extensions list - published and draft Co-authored-by: Gonzalo <[email protected]>

santiagorodriguez96 requested a review from grzuy June 2, 2020 15:59

santiagorodriguez96 commented Jun 2, 2020

View reviewed changes

spec/webauthn/public_key_credential_spec.rb Outdated Show resolved Hide resolved

cedarcode deleted a comment from bdewater Jun 2, 2020

santiagorodriguez96 commented Jun 3, 2020

View reviewed changes

spec/webauthn/public_key_credential_spec.rb Outdated Show resolved Hide resolved

santiagorodriguez96 commented Jun 3, 2020

View reviewed changes

santiagorodriguez96 marked this pull request as ready for review June 3, 2020 14:17

grzuy reviewed Jun 4, 2020

View reviewed changes

santiagorodriguez96 requested a review from grzuy June 9, 2020 18:31

grzuy reviewed Jun 10, 2020

View reviewed changes

santiagorodriguez96 mentioned this pull request Jun 15, 2020

Unable to import createExtended() and getExtended() methods from webauthn-json/extended github/webauthn-json#20

Closed

santiagorodriguez96 added 3 commits June 16, 2020 14:07

feat: provide client extensions as an output

9840b69

feat: provide authenticator extensions as an output

d86cbeb

fixup! feat: provide authenticator extensions as an output

b62402e

santiagorodriguez96 force-pushed the provide_extensions_as_an_output_of_the_verification branch from 008b527 to b62402e Compare June 16, 2020 17:10

santiagorodriguez96 commented Jun 16, 2020

View reviewed changes

santiagorodriguez96 requested a review from grzuy June 16, 2020 17:48

grzuy approved these changes Jun 16, 2020

View reviewed changes

fixup! feat: provide client extensions as an output

fc9fb1b

santiagorodriguez96 requested a review from grzuy June 18, 2020 14:34

santiagorodriguez96 commented Jun 18, 2020

View reviewed changes

santiagorodriguez96 mentioned this pull request Jun 18, 2020

feat: add capability of handling appid extension #319

Merged

grzuy approved these changes Jun 20, 2020

View reviewed changes

santiagorodriguez96 merged commit 1d2f509 into master Jun 22, 2020

santiagorodriguez96 deleted the provide_extensions_as_an_output_of_the_verification branch June 22, 2020 14:21

santiagorodriguez96 mentioned this pull request Jun 23, 2020

docs: add documentation for extensions #323

Merged

santiagorodriguez96 added a commit that referenced this pull request Jun 23, 2020

docs: add documentation for extensions

3afa0cd

Follow up for #317.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add API methods for providing received extension information #317

feat: add API methods for providing received extension information #317

santiagorodriguez96 commented Jun 2, 2020

santiagorodriguez96 commented Jun 2, 2020 •

edited

Loading

santiagorodriguez96 Jun 3, 2020

santiagorodriguez96 Jun 3, 2020

santiagorodriguez96 Jun 3, 2020

santiagorodriguez96 Jun 3, 2020

grzuy left a comment •

edited

Loading

grzuy left a comment

santiagorodriguez96 Jun 16, 2020 •

edited

Loading

grzuy Jun 16, 2020

grzuy left a comment

grzuy Jun 16, 2020

santiagorodriguez96 Jun 17, 2020 •

edited

Loading

grzuy Jun 20, 2020

grzuy Jun 16, 2020

santiagorodriguez96 Jun 18, 2020 •

edited

Loading

grzuy Jun 20, 2020

grzuy left a comment

grzuy Jun 20, 2020

grzuy Jun 20, 2020

clientExtensionResults	client_extension_outputs
`nil`	`nil`
`{}`	`{}`
`{ "appid" => "true" }`	`{ "appid" => "true" }`
`{ appid: true }`	`{ appid: true }`
`<ActionController::Parameters {"appid"=>true} permitted: false>`	`<ActionController::Parameters {"appid"=>true} permitted: false>`

feat: add API methods for providing received extension information #317

feat: add API methods for providing received extension information #317

Conversation

santiagorodriguez96 commented Jun 2, 2020

What

Why

santiagorodriguez96 commented Jun 2, 2020 • edited Loading

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 • edited Loading

Choose a reason for hiding this comment

grzuy left a comment

Choose a reason for hiding this comment

santiagorodriguez96 Jun 16, 2020 • edited Loading

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

santiagorodriguez96 Jun 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

santiagorodriguez96 Jun 18, 2020 • edited Loading

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

santiagorodriguez96 commented Jun 2, 2020 •

edited

Loading

grzuy left a comment •

edited

Loading

santiagorodriguez96 Jun 16, 2020 •

edited

Loading

santiagorodriguez96 Jun 17, 2020 •

edited

Loading

santiagorodriguez96 Jun 18, 2020 •

edited

Loading