Skip to content
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

attachment is only explicitly used in create() #420

Closed
equalsJeffH opened this issue Apr 21, 2017 · 8 comments
Closed

attachment is only explicitly used in create() #420

equalsJeffH opened this issue Apr 21, 2017 · 8 comments

Comments

@equalsJeffH
Copy link
Contributor

equalsJeffH commented Apr 21, 2017

The description of MakeCredentialOptions.attachment (of type Attachment) is:

This member contains authenticator attachment descriptions, which are used as an additional constraint on which authenticators are eligible to participate in a create() or get() operation.

Also, in the description of the Attachment type, there is this first brief example in the final parag:

...there are use-cases where only platform authenticators are acceptable to a Relying Party...As a concrete example of [this], a credential on a platform authenticator may be used by Relying Parties to quickly and conveniently reauthenticate the user with a minimum of friction, e.g., the user will not have to dig around in their pocket for their key fob or phone.

Finally, there is no option/parameter of type Attachment provided to ScopedCredential::[[DiscoverFromExternalSource]](options) (see: https://w3c.github.io/webauthn/#getAssertion)

@equalsJeffH
Copy link
Contributor Author

equalsJeffH commented Apr 21, 2017

[ oops, did not mean to hit [Comment] yet, sorry, so continuing with the issue submission here, I've also edited the initial description, so please do not rely on the emailed copy ]

Is the example (above) expected to work due to..

(a) the RP knowing, from credential creation & registration, that it has a cred for the user backed by a platform-attached authnr and thus is able to place just that one cred in the options.allowList in the call to PublicKeyCredential::[[DiscoverFromExternalSource]](options) (aka navigator.credentials.get()) ?

I.e., rather than, (b) being able to state "use any cred registered with this RP that has an attachment of platform" ?

I do not think we necessarily need to have (b), but if (a) is the expected way for the use case to work we may want to clarify that in the spec.

If we were to want (b), we'd have to do some work.

@equalsJeffH equalsJeffH self-assigned this Nov 9, 2017
@nadalin
Copy link
Contributor

nadalin commented Nov 9, 2017

@equalsJeffH please verify

@equalsJeffH
Copy link
Contributor Author

Yes, this remains a valid issue -- need feedback from @jyasskin @christiaanbrand @emlun @AngeloKai @akshayku wrt is the answer (a) or (b) ?

@emlun
Copy link
Member

emlun commented Nov 21, 2017

For what it's worth (a) is not possible in the first factor use case (no allowCredentials given to credentials.get()), so we need this if we want to support attachment selection there.

That said, I frankly can't see a use case for why an RP would ever want to allow an attachment mode for registration but not assertion. I don't see in what way that would be better than just sending allowList: ["platformCredA", "roamingCredB"] so the client picks the platform credential if it happens to be available. I can see how an RP could reasonably want to disable registering, say, wireless authenticators, but I don't see the benefit in the assertion case. I think it would only serve to frustrate the user if the RP can tell them they can't use that authenticator to log in this time. EDIT: If the user has only registered an authenticator that's cumbersome to use - and the RP has allowed them to do that in the first place - that's the user's own fault choice, IMO, and there's nothing stopping the RP from popping up a "Hey, wanna register this device for easier login next time?" if they detect a platform authenticator is available.

@emlun
Copy link
Member

emlun commented Nov 21, 2017

Oh wait, looks like I mixed this up with transport modes. I think the same argument applies, though. That might actually be even less useful - the use case for roaming authenticators in the example already has this as an implicit limitation, so I don't see why it would be useful to also impose it as an artificial limitation; and the use case for platform authenticators is IMO better solved as described in my previous post.

@christiaanbrand
Copy link

I vote (a) and I agree with @emlun that this doesn’t work for resident credentials, but it doesn’t need to. This is to solve for a typical reauth scenario where the RP only want to register a credential on a local “platform” authenticator since part of the security model is the fact that the authenticator is built-in (ie. it’s really used as a 2nd factor; the cookie identifying the platform is the first factor). In this case the RP will always have the credentialID since it has a handle to the device (via a session cookie, etc) and can use the allowList.

@emlun
Copy link
Member

emlun commented Nov 29, 2017

Do we have consensus that passing authenticator selection parameters to credentials.get() is not needed? If so, I'll go ahead and write up a PR to resolve the inconsistencies @equalsJeffH mentioned.

@equalsJeffH
Copy link
Contributor Author

PR #420 is merged so closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants