MasterKale / SimpleWebAuthn

WebAuthn, Simplified. A collection of TypeScript-first libraries for simpler WebAuthn integration. Supports modern browsers, Node, Deno, and more.
https://simplewebauthn.dev
MIT License
1.62k stars 137 forks source link

verifyRegistrationResponse fails if not metadata was found #155

Closed MaKleSoft closed 3 years ago

MaKleSoft commented 3 years ago

When trying to register the an authenticator using fingerprint on my Pixel 3a I get

 Error: No metadata statement found for aaguid "b93fd961-f2e6-462f-b122-82002247de78"

Looks like MetadataService.getStatement always throws if no meta data was found: https://github.com/MasterKale/SimpleWebAuthn/blob/master/packages/server/src/services/metadataService.ts#L138

Is this intentional? Because then this code block doesn't really make sense: https://github.com/MasterKale/SimpleWebAuthn/blob/master/packages/server/src/registration/verifications/verifyAndroidSafetyNet.ts#L94-L110

My guess is that getStatement should probably just return null if no metadata was found for a given aaguid, no?

MasterKale commented 3 years ago

To confirm, you've opted in to using MetadataService, right? Failing on a missing statement for a given AAGUID always seemed onerous to me, but I wasn't able to pass FIDO Conformance Tests without that restriction.

Perhaps I finally add an option to simply return undefined instead so the response verification can continue with regular cert path validation using the baked-in root feet. I'll make this the default so that the error is something you have to opt into since it's more strict.

MaKleSoft commented 3 years ago

To confirm, you've opted in to using MetadataService, right?

Yeah, but mostly because I'm using getStatement to get the description of the authenticator device, which currently isn't possible without also opting into the verification part. I actually don't mind also using MetadataService for verification but the thing is that afaik most platform authenticators simply aren't listed in mds so the verification will always fail for those. So it would be nice to either

MaKleSoft commented 3 years ago

Actually I just saw that in v4 you're now exporting the BaseMetadataService class, so I guess I could just initialise my own instance and opt out of mds verification otherwise. However I think it's still worth thinking about implementing a fallback mechanism in case no mds entry is found (unless that goes against the idea of using mds in the first place).

MasterKale commented 3 years ago

Hey @MaKleSoft I've got PR #157 waiting in the wings but I'm unsure what to call the flag. It's currently called allowUnrecognizedAAGUID but I'm not confident it's the best name for the option. Do you have any suggestions? 🤔

MaKleSoft commented 3 years ago

How about a verificationMode enum / union-type string instead? Something like

export type VerificationMode = "none" | "default" | "strict";

? Using verificationMode: "none" would be helpful for the use-case I described earlier where someone wants to fetch metadata only for logging or display purposes. Not sure what to call the "middle tier". Maybe something more descriptive like fallback? Or perhaps you can come up with even more options for the verification behaviour :) That's the nice things about using an enum instead of a boolean - it leaves you the option of adding additional modes in the future.

MasterKale commented 3 years ago

How about a verificationMode enum / union-type string instead?

Good call, I'd considered this earlier but was still indecisive on what exactly I needed to change about MetadataService to make things more permissive. A boolean seemed the simplest, but then I remembered that that's what led to there being a requireResidentKey: boolean and "residentKey: ResidentKeyRequirement + instructions on what to set requireResidentKey to for a given residentKey" in L2 of the WebAuthn spec (because in L1 it made sense for resident key requirement to be a simple yes/no!)

I settled on this:

// Allow MetadataService to accommodate unregistered AAGUIDs ("permissive"), or only allow
// registered AAGUIDs ("strict"). Currently primarily impacts how `getStatement()` operates
type VerificationMode = 'permissive' | 'strict';

An optional verificationMode can be set when calling MetadataService.initialize(opts); to control how the service behaves. It will default to "strict" because that's how MetadataService currently operates and I don't want this to be a breaking change.

MasterKale commented 3 years ago

This feature is now available in the new @simplewebauthn/server@4.1.0.