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

various issues with AppId extension #491

Closed
equalsJeffH opened this issue Jun 7, 2017 · 10 comments
Closed

various issues with AppId extension #491

equalsJeffH opened this issue Jun 7, 2017 · 10 comments

Comments

@equalsJeffH
Copy link
Contributor

There are various issues with the AppId extension as presently written:

  1. why is the appId extension used only during get() (which in this case is impl'd by PublicKeyCredential’s [[DiscoverFromExternalSource]](options) method)? If browsers will initially be using U2F authenticators (authnrs) underneath WebAuthn, will they not need to pass an appId down to the authnr during create() (which in this case is impl'd by PublicKeyCredential’s [[Create]](options) method) ?

    The description of the appid extension says:

    This authentication extension allows Relying Parties that have previously registered a credential using the legacy FIDO JavaScript APIs to request an assertion.

    But if the initial authnrs are U2F, will they not also need to perform registration?

  2. the rpId is not syntactically the same (at this time) as the appId. The RP ID is "a valid domain string", while the appid is a serialized origin. So one cannot simply overwrite the RP ID (at this time) with an appid.

  3. "client extension processing" references step 3 of {#getAssertion} though I believe that is now step 7 of the latter alg.

  4. "client extension processing" says in part:

    Replace the calculation of rpId in Step 7 of {#getAssertion} with the following procedure: The client uses the value of appid to perform the AppId validation procedure (as defined by [FIDO-APPID]). If valid, the value of rpId for all client processing should be replaced by the value of appid.

    But we cannot do simply that because of the syntactic differences between rpId and appid.

(off-the-cuff unvalidated) solution options:

S1. alter RP ID syntax to be serialized origin

S2. have appid extn add additional "appid" dictionary item to the options passed into both [Create] and [DiscoverFromExternalSource]

@nadalin
Copy link
Contributor

nadalin commented Sep 7, 2017

@leshi Please review and if you are still interested in this please update, if not please close this

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

nadalin commented Nov 9, 2017

@equalsJeffH please verify

@agl
Copy link
Contributor

agl commented Nov 30, 2017

why is the appId extension used only during get()

I think the motivation is that sites may have many security keys that were registered with the U2F API, but don't wish to force re-registration when switching to webauthn. The tokens will expect the appId hash to be the U2F appId, however, and not the RP ID.

Newly registered keys can be all-webauthn and the appId hash at the token level can be a hash of the RP ID.

As for the questions about appIds vs RP IDs: I don't think we want to change the format of webauthn RP IDs at this point. So adding an "appid" dictionary item would work, or else just noting that poking an appId into an RP ID slot doesn't quite work but implementations will know what it means.

@emlun
Copy link
Member

emlun commented Dec 5, 2017

@agl is correct, the AppID is not needed for new registrations.

To elaborate: the application parameter sent to U2F authenticators is an opaque binary array containing the SHA-256 hash of the AppID; the validation of the AppID is done by the client platform. U2F authenticators are therefore compatible with WebAuthn RP IDs for new registrations since they only see the hash. The difference in format between RP IDs and AppIDs can be handled fully by the client.

A U2F credential registered via WebAuthn wouldn't be usable with an RP that only knows about U2F, but since credentials aren't shared between RPs this is not a problem. Likewise the credential wouldn't be usable with a client that knows only about U2F, but since that client wouldn't itself be compatible with the RP this is also not a problem.

Therefore the AppID is not necessary for new registrations.


What could cause trouble, though, is point (4):

If valid, the value of rpId for all client processing should be replaced by the value of appid.

At present the rpId seems to not be used in any other client extension processing, but this could possibly cause confusion with future extensions. Then again the RP would be aware since it has to explicitly opt into this behaviour, so I guess it would only matter if other extensions for some reason expect rpId to be of the RP ID format.

@leshi
Copy link
Contributor

leshi commented Dec 5, 2017

rpId cannot be replaced by appId because the RP could have provided several credentials in the allowList -- some that were registered using the legacy u2f APIs (and hence need the appid extension to override their rpId) and some that were registered using webauthn API (and hence need to keep their rpId). This needs to be fixed.

Regarding "causing confusing with future extensions", I think that's a future problem. Future extensions can cause problems with any part of the spec (not just rpId processing) since extensions have no limits as to what they can do. In any case, let's not worry about future extensions here.

@arnar
Copy link
Contributor

arnar commented Dec 5, 2017

I think this can be fixed with more precise language in the definition of the extension, by spelling out the specific purpose (support existing U2F registrations), and that the appId is to be used when talking to U2F devices only. The presence of an rpId becomes irrelevant in those cases, hence we can drop the language about replacing rpId with appId.

@leshi
Copy link
Contributor

leshi commented Dec 5, 2017

Any chance you have cycles to fix that? :-P

@arnar
Copy link
Contributor

arnar commented Dec 5, 2017

Yep, catching up to make sure I don't make it worse. Jeff you can add me to assignees if that helps with tracking.

@emlun
Copy link
Member

emlun commented Dec 5, 2017

@leshi Oh right, that is a good point. Though I think in practice most users will probably have only one authenticator, which mitigates the impact. The RP could also somewhat work around it by trying both U2F and Webauthn credentials in two separate getAssertion calls, but it would be a pretty ugly user experience.

We could fix this by making the extension value a [{ credentialId, appId }...] list, or a { appId: [credentialId...] } object or the like, but we'd hoped that the next releases would have no breaking changes. I can't say whether or not we'll do that.

@equalsJeffH
Copy link
Contributor Author

equalsJeffH commented Dec 6, 2017

on 6-Dec-2017 webauthn call: @agl asserts that in the case of deployed RPs using U2F, they will have existing registered users that are registered with AppID hash, and the new webauthn api interactions will be using RP ID hash and thus problems ensue.

@agl asserts that this extension needs majjor surgery and will undertake that from a U2F compatibility perspective. See PR #723.

@agl agl assigned agl and unassigned equalsJeffH Dec 6, 2017
agl added a commit to agl/webauthn that referenced this issue Dec 7, 2017
This change clarifies the the behaviour of the `appid` client extension
and removes the client extension output.

Fixes w3c#491.
agl added a commit to agl/webauthn that referenced this issue Dec 8, 2017
This change clarifies the the behaviour of the `appid` client extension
and removes the client extension output.

Fixes w3c#491.
agl added a commit to agl/webauthn that referenced this issue Dec 8, 2017
This change clarifies the the behaviour of the `appid` client extension
and removes the client extension output.

Fixes w3c#491.
@agl agl closed this as completed in #723 Jan 5, 2018
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

7 participants