Metadata service support #208

bdewater · 2019-05-20T23:50:09Z

Closes #175
Addresses #66 for packed and U2F attestation formats.

Some notes:

The CRL checking logic should probably extracted to the JWT gem
json_accessor helper to make working with JSON's camelCase and Ruby's snake_case less cumbersome, and dealing with data types JSON doesn't know about. Could easily be extended with an optional: true keyword arg to raise for specified mandatory elements but so far I'm not really convinced of the need
Policy (see processing rules) for acceptable authenticators is delegated to the application for now, see server.rb for how the conformance tools test for authenticators listed as compromised
remove the certificate_in_use? calls in the attestation response classes since the chain can then be verified using a X509::TrustStore using the root certificates from the metadata service, which includes this check

grzuy · 2019-05-21T12:50:53Z

Wow, very nice!

I will try provide some feedback when I can.

brauliomartinezlm · 2019-06-22T20:27:46Z

@bdewater I have finally had proper time to understand this more, go over Fido MDS docs and derived RFCs that I needed to refresh to give a more appropriate review.

I have not gone in depth on checking we're covering all the steps but have identified many of them in the processed described in 3.1.8 Metadata TOC object processing rules of the aforementioned doc. I figure it was not the goal of my first pass on this draft, but I'll cover it on my final stamp.

First of all, I can't tell you how much appreciation I have on you taking on such a complex and extensive part we're missing on our verification flows. This probably took you a lot of efforts and it's undoubtedly an excellent piece of work.

I don't have any concern or red flags on what I have reviewed. I didn't go to the details at this stage. The implementation and the architecture look great to me so far. I do have questions on which I would like to pick you brain a bit.

How is the gem user going to fire up the process of bootstrapping and populating the metadata store?
How is the gem user going to keep the metadata store up to date and perform necessary maintenance? Are you thinking of some side processing for this? I don't see this tied or fired by any webauthn existing flow, it will need to be something completely separate.
We need to minimize roundtrips for fetching metadata statements from authenticator vendors, so I guess we will be using a cache? What do you have in mind in such regard?
How are we going to persist the TOCs, dictionary of entries, etc? I guess this is coupled to the previous question.
Do you see this client living inside the gem and being extracted later? I keep seeing symptoms of this being an isolated component, with separate flows and contained complexity that lead me to think on this living outside the gem itself. I'm not suggesting we extract it now but there are certain flows (downloading and refreshing the store) that may grow into something worth considering it. My questions were already rumbling around store persistence and scheduled data refreshing (that might even mean running workers or other side processing).

Again, this is a tremendous effort and a great ton of high complexity, can't thank you enough!
Let me know if I can help on anything, or wanna chat any specifics in Gitter, I'd be glad to help keep this effort moving forward with all the capacity I can dedicate (sadly, sometimes is not much :( ).

Sorry this took me so long to jump on it and review 🙏

grzuy · 2019-06-24T18:03:05Z

Some notes:

the metadata client does a lot and some of the CRL checking logic should probably extracted - I wasn't sure yet what that should look like, once a second use case shows to help guide forming the abstraction perhaps

Just chiming in to say agree with this.
Also like that you kept all the metadata client code namespaced with the Metadata constant.

In general, I would try to keep all FIDO MDS service logic as disconnected as possible to the WebAuthn core, which is the direction I see you are already taking. If we could connect them with a few method calls only, that would be ideal. Can even be converted into a separate library if we want in the future. Similar to https://www.npmjs.com/package/mds-client.

bdewater · 2019-06-24T20:01:18Z

Thanks for the review @brauliomartinezlm ! To answer your questions:

How is the gem user going to fire up the process of bootstrapping and populating the metadata store?

The gem will need to be configured with the MDS API key and a cache backend. I'm planning to add a simple in-memory backend for testing and and conformance testing, but RPs wishing to roll this out to production should really write a Redis/memcached/Active Record/etc adapter suitable for their application.

How is the gem user going to keep the metadata store up to date and perform necessary maintenance? Are you thinking of some side processing for this?

I was thinking the cache could work in a similar manner as Rails' implementation. It could fetch data on demand (TOC first, the detailed statement if needed), but it should also be possible to have a daily background job fetch all the MDS data to ensure there's always cached data to work with.

I don't see this tied or fired by any webauthn existing flow, it will need to be something completely separate.

It's implementing step 15 of verifying the credential registration. But like @grzuy already suggested the coupling should be fairly minimal, something like a valid_attestation_trustworthiness? method on AuthenticatorAttestationResponse that calls to the cache, gated behind the configuration.verify_attestation_statement value.

Do you see this client living inside the gem and being extracted later? I keep seeing symptoms of this being an isolated component, with separate flows and contained complexity that lead me to think on this living outside the gem itself.

Maybe, but given this being tied to registration where the RP wishes to request and verify attestation I feel there's also a case to be made for it staying a part of the gem, even if it's minimally coupled. I'm not seeing the value of a standalone client just yet but maybe you can convince me otherwise 🙂

After this is done I'll focus on upstreaming the x5c JWT verification and CRL handling from the Metadata::Client to https://github.com/jwt/ruby-jwt first.

brauliomartinezlm · 2019-06-29T12:46:26Z

The gem will need to be configured with the MDS API key and a cache backend. I'm planning to add a simple in-memory backend for testing and and conformance testing, but RPs wishing to roll this out to production should really write a Redis/memcached/Active Record/etc adapter suitable for their application.

Yeah, that's reasonable 👍

I was thinking the cache could work in a similar manner as Rails' implementation. It could fetch data on demand (TOC first, the detailed statement if needed), but it should also be possible to have a daily background job fetch all the MDS data to ensure there's always cached data to work with.

Had a similar potential outcome in mind 👍

It's implementing step 15 of verifying the credential registration. But like @grzuy already suggested the coupling should be fairly minimal, something like a valid_attestation_trustworthiness? method on AuthenticatorAttestationResponse that calls to the cache, gated behind the configuration.verify_attestation_statement value.

Yeah, that's perfect. Keeping it as decoupled as that will give us the flexibility we need to take any decision in the future about this part of the gem's arch.

Maybe, but given this being tied to registration where the RP wishes to request and verify attestation I feel there's also a case to be made for it staying a part of the gem, even if it's minimally coupled. I'm not seeing the value of a standalone client just yet but maybe you can convince me otherwise 🙂

When I said symptoms on this being potentially extracted I was specifically thinking on the 2 first answers I quoted in this reply. Making the gem user plug backends and backgrounds jobs. I'm not in a point where I'd try to convince the rest on making it standalone client from scratch. I'm more than fine with keep building it inside the gem and come together to a conclusion with more cards on the table and having a shared decision in such move if that ever happens at all.

bdewater · 2019-08-08T12:46:28Z

This is ready for review! 🎉

I'll merge #234 soon and test with a Surface 4 to address the TODO in the TPM specs, but I think the overall direction is good.

bdewater · 2019-08-16T01:04:26Z

Decided to drop the TPM commit here - the Fido MDS is of no use at the moment here but we can use the TPM manufacturer root certificates to verify this instead. I'll open a separate PR for this.

Implements most of https://fidoalliance.org/specs/fido-v2.0-rd-20180702/fido-metadata-service-v2.0-rd-20180702.html Interestingly the MDS server currently does not correctly implement the standard; TOC entry hashes for statements are padded base64url while they should be unpadded. The conformance server however uses unpadded encoding correctly.

With this commit, all the metadata server tests in the FIDO conformance tools pass

bdewater

Thanks for the review and kind words @brauliomartinezlm !

I'd like to ask what you would think about running the whole MDS topic as opt-in for a while for the gem's users.

Strictly speaking that already is the case, browsers don't ask for attestation by default 😉 #219 also provides a way to enforce that server side.

It's fair amount of heavy lifting that some gem users might not want or be ready to add, and for them, I'd like to give them the chance to keep doing the more basic stuff we were doing with the attestation statement and that didn't involve a cache backend, nor making requests to fido and authenticator manufacturers urls behind the curtains.

I see your point regarding easy of use, but I also feel asking for attestation is a 'heavy' feature in itself* (user must allow sharing "make and model of the security key" with RP) and the current implementation is security theatre for most formats. I'll think about this a bit more, but right now I'm leaning more towards a black/white solution where you either opt-out or -in completely into verifying attestation properly as described by the standard.

_{*: considering the user must consent to sharing "make and model" with the RP (unless pre-allowed by enterprise policies), and the use cases mentioned here are are not typical for consumers but rather for (large) companies, financial institutions and governments.}

iii. Give us time to work on documentation and gem user adoption for all the theory and conceptual background behind this feature.

💯 I'll get started on a markdown file for here in the repo on and example implementation in https://github.com/cedarcode/webauthn-rails-demo-app

bdewater · 2019-09-02T00:13:06Z

lib/webauthn/metadata/client.rb

+
+ def get_with_token(uri)
+ if @token && [email protected]?
+ uri.path += "/" unless uri.path.end_with?("/")


Either the production or conformance MDS environment needed this. It was a bit wacky and resulted in HTTP 404s otherwise, I'll add a comment since I don't know exactly anymore either 😅

bdewater · 2019-09-02T00:13:56Z

lib/webauthn/metadata/client.rb

+ def get(uri)
+ get = Net::HTTP::Get.new(uri, DEFAULT_HEADERS)
+ response = http(uri).request(get)
+ response.value


Raising an error, but seeing it again I agree it can be made a bit more explicit. Will change :)

bdewater · 2019-09-02T00:18:03Z

lib/webauthn/metadata/coercer/assumed_value.rb

+ module Coercer
+ class AssumedValue
+ def initialize(assume)
+ @assume = assume


There's a few references to "assumed value" in that document :) e.g:

protocolFamily of type DOMString
The FIDO protocol family. The values "uaf", "u2f", and "fido2" are supported. If this field is missing, the assumed protocol family is "uaf".

bdewater · 2019-09-02T00:24:59Z

spec/webauthn/metadata/statement_spec.rb

+
+ it "has the expected attributes" do
+ expect(subject).to have_attributes(
+ authenticator_version: 2,


I implemented the textual descriptions from section 5 quite literally and I didn't even notice the spec mentioned it, haha! Will add.

grzuy · 2019-09-02T14:12:44Z

Hi @bdewater,

Thanks for all your hard work on this.

Heads up:

Planning to and want to review this myself :-)
Seeing it possibly included in a >= v2.1 release, not v2.0. I expect v2.0 to include new WebAuthn::PublicKeyCredential API and not a lot more. After some time with that released and stable we can continue pushing new features, like this one.

UPDATE: Just created a v2.0 milestone to provide more clarity.

grzuy · 2019-10-31T14:14:24Z

I see your point regarding easy of use, but I also feel asking for attestation is a 'heavy' feature in itself* (user must allow sharing "make and model of the security key" with RP) and the current implementation is security theatre for most formats. I'll think about this a bit more, but right now I'm leaning more towards a black/white solution where you either opt-out or -in completely into verifying attestation properly as described by the standard.

For the record, there was an offline discussion a while ago with @bdewater and @brauliomartinezlm in which we decided we would only want to have an all-or-nothing config setting that turns ON/OFF attestation verification completely. No value in keeping the "partial" attestation verification we are providing as of v2.0.0 as an option.

We also agreed the best would be to have it turned OFF by default.

grzuy · 2019-10-31T17:19:20Z

Hi @bdewater,

We decided to start merging all changes that are part of #66 into an integration branch (attestation_trustworthiness), instead of master.

Then, when feature complete, we merge into master.

grzuy

Thank you @bdewater,
This is excellent work.

More than ready to be merged into attestation_trustworthiness epic branch and continue polishing all thing related to attestation there.

bdewater · 2019-10-31T17:35:56Z

Hey @grzuy! Working on a bigger feature branch sounds like a good idea. As mentioned on Gitter I ran into a few small issues that need fixing for successful integration in the Rails demo app. I'm aiming to push those fixes and the example PR up this weekend and we can continue from there :)

grzuy · 2019-10-31T20:04:09Z

.gitignore

@@ -13,3 +13,5 @@
 /Gemfile.lock
 /gemfiles/*.gemfile.lock
 .byebug_history
+
+/spec/conformance/metadata.zip


This isn't having any effect because the file is also included in the diff here.

The intention was to ignore right?

That must've gone wrong during a rebase, it was not meant to be included :( It's governed by the conformance tools terms of service and not open source.

grzuy · 2019-11-08T17:34:49Z

Do you see this client living inside the gem and being extracted later? I keep seeing symptoms of this being an isolated component, with separate flows and contained complexity that lead me to think on this living outside the gem itself.

Maybe, but given this being tied to registration where the RP wishes to request and verify attestation I feel there's also a case to be made for it staying a part of the gem, even if it's minimally coupled. I'm not seeing the value of a standalone client just yet but maybe you can convince me otherwise slightly_smiling_face

Hi @bdewater, I've been thinking more and more about this and I think I am quite convinced that we want, at least, the metadata service client to be separate library/gem. I am not sure about the store yet, but the client for sure.

I think is also arguable that the WebAuthn::Metadata::Client should not be namespaced with WebAuthn. This is a client consuming the FIDO Metadata Service v2, which is more broad than WebAuthn (FIDO 2), it also contains metadata for FIDO UAF and FIDO U2F. Not only that, it was actually born as the FIDO UAF Authenticator Metadata Service v1.0, without support for FIDO2 at the time.

So I think this could be something like:

gem "fido_metadata_service" and FIDOMetadataService::Client; or
gem "fido_mds" and FIDOMDS::Client; or
gem "mds-client and MDS::Client or something similar

FWIW, there's a JS package similar to this: https://www.npmjs.com/package/mds-client.

@bdewater Thoughts?

bdewater · 2019-11-10T15:31:51Z

I agree with the namespacing but from a practical standpoint I didn't feel it was that big of a deal initially. I'd be happy to extract it and make a gem if you do, given our recent Gitter chat it seems to make sense to have the new gem support multiple publishers too (i.e. FIDO, SoloKeys).

The cache/store layer I do think is in the right place, for both practical reasons and this bit from the specification:

It is expected that most authenticators will support a small number of attestation types and attestation statement formats, while Relying Parties will decide what attestation types are acceptable to them by policy. Relying Parties will also need to understand the characteristics of the authenticators that they trust, based on information they have about these authenticators. For example, the FIDO Metadata Service provides one way to access such information.

grzuy · 2019-11-11T15:53:12Z

I agree with the namespacing but from a practical standpoint I didn't feel it was that big of a deal initially. I'd be happy to extract it and make a gem if you do,

Thank you!
I am quite convinced we should have it separate.

given our recent Gitter chat it seems to make sense to have the new gem support multiple publishers too (i.e. FIDO, SoloKeys).

Nice.

So how would that work?
I was assumming the "FIDO Metadata Service" client would only know how to consume a web source that follows the https://fidoalliance.org/specs/fido-v2.0-id-20180227/fido-metadata-service-v2.0-id-20180227.html spec.

Is SoloKeys or any other manufacturer also providing a source implementing that spec?

The cache/store layer I do think is in the right place, for both practical reasons and this bit from the specification:

It is expected that most authenticators will support a small number of attestation types and attestation statement formats, while Relying Parties will decide what attestation types are acceptable to them by policy. Relying Parties will also need to understand the characteristics of the authenticators that they trust, based on information they have about these authenticators. For example, the FIDO Metadata Service provides one way to access such information.

Sounds good.

bdewater · 2019-11-13T16:44:19Z

So how would that work?
I was assumming the "FIDO Metadata Service" client would only know how to consume a web source that follows the https://fidoalliance.org/specs/fido-v2.0-id-20180227/fido-metadata-service-v2.0-id-20180227.html spec.
Is SoloKeys or any other manufacturer also providing a source implementing that spec?

Mostly but not entirely: https://github.com/solokeys/solo/tree/master/metadata - I'll think about this a little more, but in the mean time users could download these files by hand and add them to the cache if they wish to trust this vendor.

In the mean time, here's the gem extraction of this PR: https://github.com/bdewater/fido_metadata

grzuy · 2019-11-13T19:45:20Z

Excellent.
Thank you very much!

bdewater force-pushed the metadata-service branch from 8acd292 to 5851044 Compare May 20, 2019 23:54

bdewater mentioned this pull request May 24, 2019

Support x5c JWT header as specified in RFC-7515 (JWS) jwt/ruby-jwt#308

Closed

bdewater force-pushed the metadata-service branch 6 times, most recently from ae0e21e to 765036e Compare July 1, 2019 05:56

bdewater force-pushed the metadata-service branch from 1fb58a9 to 6c064fb Compare July 6, 2019 20:31

This was referenced Jul 10, 2019

Checking AAGUID when attestation type is "direct" #239

Closed

Expose AAGUID and attestationCertificateKey for MDS lookup #234

Merged

bdewater force-pushed the metadata-service branch from 6c064fb to 7eb8df4 Compare August 8, 2019 12:37

bdewater marked this pull request as ready for review August 8, 2019 12:38

bdewater force-pushed the metadata-service branch from 7eb8df4 to cde7540 Compare August 16, 2019 01:02

bdewater mentioned this pull request Aug 16, 2019

Verify TPM attestation certificate trustworthiness #253

Merged

bdewater added 4 commits August 20, 2019 20:15

feat: add certificate revocation check to metadata client

ef8e8b1

feat: parse metadata TOC objects

96d5046

feat: parse metadata statements

9aa4e1d

bdewater force-pushed the metadata-service branch from cde7540 to ec5f7a5 Compare August 21, 2019 02:36

bdewater added 2 commits August 20, 2019 22:45

feat: use metadata to retrieve root CAs for verifying packed attestation

905c771

With this commit, all the metadata server tests in the FIDO conformance tools pass

feat: use metadata to retrieve root CAs for verifying U2F attestation

b89cf5c

bdewater force-pushed the metadata-service branch from ec5f7a5 to b89cf5c Compare August 21, 2019 02:47

bdewater commented Sep 2, 2019

View reviewed changes

grzuy requested review from grzuy and removed request for grzuy September 2, 2019 14:05

Add draft and somewhat aspirational metadata docs

7defc20

bdewater force-pushed the metadata-service branch from b90a129 to 7defc20 Compare September 19, 2019 01:46

grzuy added area: attestation area: fido-u2f attestation area: packed attestation labels Oct 8, 2019

bdewater mentioned this pull request Oct 21, 2019

Helper to retrieve CRL URIs from a certificate ruby/openssl#275

Merged

grzuy mentioned this pull request Oct 24, 2019

Verify attestation trustworthiness #66

Closed

grzuy changed the base branch from master to attestation_trustworthiness October 31, 2019 17:19

Merge branch 'master' into metadata-service

f318ffb

grzuy approved these changes Oct 31, 2019

View reviewed changes

grzuy mentioned this pull request Oct 31, 2019

Complete attestation verification support #280

Closed

10 tasks

test: fix chain validation for packed_spec/fido_u2f_spec

c22ff2d

grzuy merged commit 0f0947c into cedarcode:attestation_trustworthiness Oct 31, 2019

grzuy reviewed Oct 31, 2019

View reviewed changes

This was referenced Nov 1, 2019

WIP: Attestation POC cedarcode/webauthn-rails-demo-app#113

Closed

Add x5c header key finder jwt/ruby-jwt#338

Merged

bdewater deleted the metadata-service branch November 13, 2019 16:44

padulafacundo mentioned this pull request Nov 15, 2019

Attestation trust path validation #282

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Metadata service support #208

Metadata service support #208

bdewater commented May 20, 2019 •

edited

Loading

grzuy commented May 21, 2019

brauliomartinezlm commented Jun 22, 2019

grzuy commented Jun 24, 2019 •

edited

Loading

bdewater commented Jun 24, 2019

brauliomartinezlm commented Jun 29, 2019

bdewater commented Aug 8, 2019

bdewater commented Aug 16, 2019

bdewater left a comment

bdewater Sep 2, 2019

bdewater Sep 2, 2019

bdewater Sep 2, 2019

bdewater Sep 2, 2019

grzuy commented Sep 2, 2019 •

edited

Loading

grzuy commented Oct 31, 2019 •

edited

Loading

grzuy commented Oct 31, 2019

grzuy left a comment

bdewater commented Oct 31, 2019

grzuy Oct 31, 2019

bdewater Oct 31, 2019

grzuy commented Nov 8, 2019 •

edited

Loading

bdewater commented Nov 10, 2019

grzuy commented Nov 11, 2019 •

edited

Loading

bdewater commented Nov 13, 2019

grzuy commented Nov 13, 2019

Metadata service support #208

Metadata service support #208

Conversation

bdewater commented May 20, 2019 • edited Loading

grzuy commented May 21, 2019

brauliomartinezlm commented Jun 22, 2019

grzuy commented Jun 24, 2019 • edited Loading

bdewater commented Jun 24, 2019

brauliomartinezlm commented Jun 29, 2019

bdewater commented Aug 8, 2019

bdewater commented Aug 16, 2019

bdewater left a comment

Choose a reason for hiding this comment

bdewater Sep 2, 2019

Choose a reason for hiding this comment

bdewater Sep 2, 2019

Choose a reason for hiding this comment

bdewater Sep 2, 2019

Choose a reason for hiding this comment

bdewater Sep 2, 2019

Choose a reason for hiding this comment

grzuy commented Sep 2, 2019 • edited Loading

grzuy commented Oct 31, 2019 • edited Loading

grzuy commented Oct 31, 2019

grzuy left a comment

Choose a reason for hiding this comment

bdewater commented Oct 31, 2019

grzuy Oct 31, 2019

Choose a reason for hiding this comment

bdewater Oct 31, 2019

Choose a reason for hiding this comment

grzuy commented Nov 8, 2019 • edited Loading

bdewater commented Nov 10, 2019

grzuy commented Nov 11, 2019 • edited Loading

bdewater commented Nov 13, 2019

grzuy commented Nov 13, 2019

bdewater commented May 20, 2019 •

edited

Loading

grzuy commented Jun 24, 2019 •

edited

Loading

grzuy commented Sep 2, 2019 •

edited

Loading

grzuy commented Oct 31, 2019 •

edited

Loading

grzuy commented Nov 8, 2019 •

edited

Loading

grzuy commented Nov 11, 2019 •

edited

Loading