-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Metadata service support #208
Metadata service support #208
Conversation
8acd292
to
5851044
Compare
Wow, very nice! I will try provide some feedback when I can. |
@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 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.
Again, this is a tremendous effort and a great ton of high complexity, can't thank you enough! Sorry this took me so long to jump on it and review 🙏 |
Just chiming in to say agree with this. 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. |
Thanks for the review @brauliomartinezlm ! To answer your questions:
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.
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.
It's implementing step 15 of verifying the credential registration. But like @grzuy already suggested the coupling should be fairly minimal, something like a
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 |
Yeah, that's reasonable 👍
Had a similar potential outcome in mind 👍
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.
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. |
ae0e21e
to
765036e
Compare
1fb58a9
to
6c064fb
Compare
6c064fb
to
7eb8df4
Compare
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. |
7eb8df4
to
cde7540
Compare
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.
cde7540
to
ec5f7a5
Compare
With this commit, all the metadata server tests in the FIDO conformance tools pass
ec5f7a5
to
b89cf5c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
|
||
def get_with_token(uri) | ||
if @token && [email protected]? | ||
uri.path += "/" unless uri.path.end_with?("/") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😅
def get(uri) | ||
get = Net::HTTP::Get.new(uri, DEFAULT_HEADERS) | ||
response = http(uri).request(get) | ||
response.value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Raising an error, but seeing it again I agree it can be made a bit more explicit. Will change :)
module Coercer | ||
class AssumedValue | ||
def initialize(assume) | ||
@assume = assume |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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".
|
||
it "has the expected attributes" do | ||
expect(subject).to have_attributes( | ||
authenticator_version: 2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I implemented the textual descriptions from section 5 quite literally and I didn't even notice the spec mentioned it, haha! Will add.
Hi @bdewater, Thanks for all your hard work on this. Heads up:
UPDATE: Just created a |
b90a129
to
7defc20
Compare
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 We also agreed the best would be to have it turned OFF by default. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 :) |
@@ -13,3 +13,5 @@ | |||
/Gemfile.lock | |||
/gemfiles/*.gemfile.lock | |||
.byebug_history | |||
|
|||
/spec/conformance/metadata.zip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't having any effect because the file is also included in the diff here.
The intention was to ignore right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 So I think this could be something like:
FWIW, there's a JS package similar to this: https://www.npmjs.com/package/mds-client. @bdewater Thoughts? |
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:
|
Thank you!
Nice. So how would that work? Is SoloKeys or any other manufacturer also providing a source implementing that spec?
Sounds good. |
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 |
Excellent. |
Closes #175
Addresses #66 for packed and U2F attestation formats.
Some notes:
json_accessor
helper to make working with JSON'scamelCase
and Ruby'ssnake_case
less cumbersome, and dealing with data types JSON doesn't know about. Could easily be extended with anoptional: true
keyword arg to raise for specified mandatory elements but so far I'm not really convinced of the needserver.rb
for how the conformance tools test for authenticators listed as compromisedcertificate_in_use?
calls in the attestation response classes since the chain can then be verified using aX509::TrustStore
using the root certificates from the metadata service, which includes this check