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
Conversation
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.
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.
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}}. |
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 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.
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.
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.)
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.
Looks good to me, but I think we should merge #1239 first and see if this needs updates to agree.
agreed that we should merge PR #1239 first |
PR #1239 has been merged. Plus, I did a merge-from-master since master changed today. |
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.
Some minor editorial polish suggestions:
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:
|
apply @emluyn's polishing, thx! Co-Authored-By: Emil Lundberg <emil@emlun.se>
@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. |
@equalsJeffH Alright, sounds good. |
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 seems ready to land, thx @agl!
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 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 . |
@selfissued Please review |
(J.C. suggests adding a string for this in the capability detection PR.) |
introduced in the spec on 2019/07/31 see: w3c/webauthn#1244 see: w3c/webauthn#1235 Signed-off-by: Arthur Gautier <baloo@gandi.net>
introduced in the spec on 2019/07/31 see: w3c/webauthn#1244 see: w3c/webauthn#1235 Signed-off-by: Arthur Gautier <baloo@gandi.net>
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