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

Add “appidExclude” extension. #1244

Merged
merged 6 commits into from Jul 31, 2019
Merged

Add “appidExclude” extension. #1244

merged 6 commits into from Jul 31, 2019

Conversation

agl
Copy link
Contributor

@agl agl commented Jun 24, 2019

This change adds a registration-only “appidExclude” extension that allows platforms to make excludeCredentials effective when transitioning from U2F.

It's a separate extension from “appid” because that extension was documented in level one as being authentication-only and some implementations thus reject it for registration. Also, having an “appid” extension that's effective during registration invites people to believe that it does the “obvious” thing and allows the creation of U2F credentials, which isn't true.

Fixes #1235.


Preview | Diff

This change adds a registration-only “appidExclude” extension that
allows platforms to make excludeCredentials effective when transitioning
from U2F.

It's a separate extension from “appid” because that extension was
documented in level one as being authentication-only and some
implementations thus reject it for registration. Also, having an
“appid” extension that's effective during registration invites people to
believe that it does the “obvious” thing and allows the creation of U2F
credentials, which isn't true.

Fixes w3c#1235.
Copy link
Contributor

@equalsJeffH equalsJeffH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx @agl -- overall looks workable tho I'm thinking we ought to tie link it more explicitly into the [create] alg and apply some polish...

index.bs Outdated Show resolved Hide resolved
index.bs Outdated
:: 1. Let |facetId| be the result of passing the caller's [=origin=] to the FIDO algorithm for [=determining the FacetID of a calling application=].
1. Let |appId| be the extension input.
1. Pass |facetId| and |appId| to the FIDO algorithm for [=determining if a caller's FacetID is authorized for an AppID=]. If that algorithm rejects |appId| then return a "{{SecurityError}}" {{DOMException}}.
1. For each authenticator that supports the U2F protocol, if |excludeCredentialDescriptorList| is not empty then, before invoking [=authenticatorMakeCredential=], attempt to obtain a U2F assertion (without user-presence), for each element of |excludeCredentialDescriptorList|, using the SHA-256 hash of |appId| as the application parameter. If unsuccessful, continue with invoking [=authenticatorMakeCredential=]. Otherwise, cease normal processing of this authenticator and indicate in a platform-specific manner that the authenticator is inapplicable. For example, this could be in the form of UI, or could involve requesting user-consent from the authenticator and, upon receipt, treating it as if the authenticator had returned {{InvalidStateError}}.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to dig around to figure out how this works. IIUC, it seems that steps 1 - 3 (above) probably ought to occur around step 19 of [[create]](), and step 4 (above) ought to occur either after or during Step 20.5 of [[create]](). Perhaps we oughta add links into those places in [[create]]() from the above. I have some alg-smithing suggestions for step 4 yet to write up. I can help with crafting this PR if you wish.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, there were a lot of commas in there. I've restructured this a bit, see if you like it better. You also have direct push access to this repo. (I.e. git remote add agl git@github.com:agl/webauthn.git then git push agl HEAD:appidexclude to push the current branch to this PR.)

Copy link
Member

@emlun emlun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but I think we should merge #1239 first and see if this needs updates to agree.

@equalsJeffH
Copy link
Contributor

agreed that we should merge PR #1239 first

@equalsJeffH
Copy link
Contributor

PR #1239 has been merged. Plus, I did a merge-from-master since master changed today.

Copy link
Member

@emlun emlun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor editorial polish suggestions:

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@emlun
Copy link
Member

emlun commented Jun 27, 2019

Perhaps we should define a proper term for "legacy credential", and use that in both this and the appid extension? I'm thinking something like:

  • AppID Credential: A credential scoped to a FIDO AppID, unlike an RP ID credential which is scoped to an RP ID. AppID credentials can be created via the FIDO U2F JavaScript API and cannot be created via the WebAuthn API, but the appid extension enables using them to generate WebAuthn assertion signatures.
  • RP ID Credential: A credential scoped to an RP ID. This is the default kind of credential in WebAuthn, and is the only kind of credential that can be created via the WebAuthn API. See also AppID credential.

apply @emluyn's polishing, thx!

Co-Authored-By: Emil Lundberg <emil@emlun.se>
@equalsJeffH
Copy link
Contributor

@emlun suggested "AppID Credential" and "RP ID Credential" terms....

hm... well, I spose I'm thinking that we already have a formal "public key credential" name as well as various informal monikers in use: "webauthn credential", "fido2 credential", "U2F credential"...

Since "legacy" is only used, in terms of U2F credentias and JS API, in the two extension definitions (AppId, appidExclude), I'm thinking things are defined clearly enough at this point and we ought not grant legacy U2F creds any further formal status in the context of the webauthn spec.

@emlun
Copy link
Member

emlun commented Jun 28, 2019

@equalsJeffH Alright, sounds good.

Copy link
Contributor

@equalsJeffH equalsJeffH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems ready to land, thx @agl!

@emlun emlun requested a review from jcjones July 1, 2019 10:10
@jcjones
Copy link
Contributor

jcjones commented Jul 3, 2019

As an extension, this is going to be best-effort. With no output, it's not going to be clear to the RP whether it was honored or not -- is that acceptable to RPs?

index.bs Show resolved Hide resolved
@agl
Copy link
Contributor Author

agl commented Jul 8, 2019

As an extension, this is going to be best-effort. With no output, it's not going to be clear to the RP whether it was honored or not -- is that acceptable to RPs?

I think the output of appid was a mistake and so I've got this extension without an output so far. @emlun suggests a constant true output, which isn't too expensive, but I still think it's superfluous.

As an RP transitioning to WebAuthn, if they don't care about duplicate registrations on a single authenticator then they're already happy, so let's assume that they do care. They'll include the current credential IDs in the exclude list and set this extension. Then, if there's an output from this extension, they get to find out, after the registration, whether this extension was processed or not. But what are they going to do with that information? Reject the registration because it might be duplicative and tell users to update their browser? That doesn't seem like something we want to work to support. Worse yet they might try some warning: “due to browser limitations we can't ensure that you didn't just register the same token again because it might duplicate some registration that you made before we switched to WebAuthn, so hopefully you didn't do that, right?”.

So I can only see bad things happening with this information, so why waste time plumbing it in?

Even if we do believe that either of the above are sensible, surely the RP would want to know about support before the registration operation? That would suggest putting it in #1219 .

@nadalin
Copy link
Contributor

nadalin commented Jul 10, 2019

@selfissued Please review

@agl
Copy link
Contributor Author

agl commented Jul 10, 2019

(J.C. suggests adding a string for this in the capability detection PR.)

@jcjones jcjones merged commit 4561e48 into w3c:master Jul 31, 2019
WebAuthnBot pushed a commit that referenced this pull request Jul 31, 2019
baloo pushed a commit to baloo/python-fido2 that referenced this pull request Aug 9, 2019
introduced in the spec on 2019/07/31

see: w3c/webauthn#1244
see: w3c/webauthn#1235

Signed-off-by: Arthur Gautier <baloo@gandi.net>
baloo pushed a commit to baloo/python-fido2 that referenced this pull request Aug 9, 2019
introduced in the spec on 2019/07/31

see: w3c/webauthn#1244
see: w3c/webauthn#1235

Signed-off-by: Arthur Gautier <baloo@gandi.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't exclude U2F credentals
6 participants