Only exclude CredIDs matching the RPID #651

jovasco · 2017-10-16T16:22:48Z

Only credentials in the exclude credentials list that match this RPID should result in a not allowed error.

Preview | Diff

Only credentials in the exclude credentials list that match this RPID should result in a not allowed error.

emlun · 2017-10-16T16:44:08Z

~~Does anyone object to marking this as non-substantive to let @jovasco through the check?~~

emlun · 2017-10-16T16:47:04Z

~~If not, I'll do so at 2017-10-17T16:00Z.~~

emlun · 2017-10-16T23:12:54Z

Scratch that, I'll leave this to the requested reviewers instead. :)

equalsJeffH

this issue intersects with work occuring in PR #498. AFAICT this change is OK, shall I address it in PR #498?

equalsJeffH · 2017-10-16T23:33:52Z

index.bs

-1. Check if a credential matching an [=list/item=] of |excludeCredentialDescriptorList| is present on this authenticator. If
-    so, return an error code equivalent to "{{NotAllowedError}}" and terminate the operation.
+1. Check if a credential matching an [=list/item=] of |excludeCredentialDescriptorList| is present on this authenticator for
+    the supplied |rpId|. If so, return an error code equivalent to "{{NotAllowedError}}" and terminate the operation.


actually, the discrete |rpId| parameter is disappearing in PR #498 because it is duplicated in rpEntity.id. That fix is in PR #498. I'd also word this a bit differently.

Wel, I am not a native English speaker :),

No worries -- is there a submitted issue corresponding to this PR ? if not please submit one, and I can/will address in pr #498

Ok, I submitted issue #654 for this.

sigh -- @jovasco had already submitted issue #650 which I'd missed. thx.

It has to be rpEntity.id. Apart from that it looks fine to me.

@akshayku Fixed.

equalsJeffH · 2017-10-18T18:31:59Z

@jovasco sub'd issue #650 for this -- if another(s) can double-check that this change is correct, I'll address issue #654 in PR #498 and we can close this PR.

cc: @jcjones @AngeloKai @leshi

jcjones · 2017-10-19T19:22:19Z

I disagree with adding this into the algorithm steps, as it implies checking the rpId of a credential before using it -- which is impossible for the implementing browser to do, as PublicKeyCredentialDescriptor doesn't provide a way to check that.

There should be a correspondence between a Credential ID and an RP ID, but it has to be managed by the authenticator. Some authenticators might use business logic here ("I made this CredID for X.Y, but this is for Z.Y, nope!"), some might use cryptography to mix the RP ID into the Credential ID's byte array. That's an implementation detail.

IMO this should go into the security considerations section instead. Maybe something to the effect of:

Authenticators, when presented a Credential ID for an RP ID, SHOULD 
ensure that the Credential ID was created for this RP ID before accepting
it for use.

jcjones · 2017-10-19T19:29:22Z

Apologies, I misunderstood which section this was actually being placed into -- this does make sense for the authenticator's makeCredential algorithm. I'll properly review.

jcjones

Reads fine to me.

jcjones · 2017-10-19T19:34:26Z

Marked as non substantive for IPR from ash-nazg.

equalsJeffH · 2017-10-19T19:59:15Z

Ok, since this is not as clear-cut as I'd thought wrt the language to use, I will not address this (and issue #650) in PR #498. will comment further in this PR.

akshayku · 2017-10-21T19:06:33Z

index.bs

-1. Check if a credential matching an [=list/item=] of |excludeCredentialDescriptorList| is present on this authenticator. If
-    so, return an error code equivalent to "{{NotAllowedError}}" and terminate the operation.
+1. Check if a credential matching an [=list/item=] of |excludeCredentialDescriptorList| is present on this authenticator for
+    the supplied |rpId|. If so, return an error code equivalent to "{{NotAllowedError}}" and terminate the operation.


It has to be rpEntity.id. Apart from that it looks fine to me.

equalsJeffH

please see comment below on refining the matching language in this step.

equalsJeffH · 2017-10-23T17:09:17Z

index.bs

-    so, return an error code equivalent to "{{NotAllowedError}}" and terminate the operation.
+1. Check if a credential matching an [=list/item=] of |excludeCredentialDescriptorList| is present on this authenticator for
+    the supplied <code>|rpEntity|.{{PublicKeyCredentialRpEntity/id}}</code>. If so, return an error code equivalent to 
+    "{{NotAllowedError}}" and terminate the operation.


this step, where we are matching creds bound to an authnr to credDescriptors, is similar to step 16.2 in section #discover-from-external-source. Note that the latter step matches on rpId, credential ID, and cred type.

Should not this authenticatorMakeCredential step similarly match on the same items?

e.g.:
1. Check if a credential matching an [=list/item=] of |excludeCredentialDescriptorList| is bound to this authenticator by matching with <code>|rpEntity|.{{PublicKeyCredentialRpEntity/id}}</code>, <code>|excludeCredentialDescriptorList|.{{PublicKeyCredentialDescriptor/id}}</code>, and <code>|excludeCredentialDescriptorList|.{{PublicKeyCredentialDescriptor/type}}</code>. If so, return an error code equivalent to "{{NotAllowedError}}" and terminate the operation.

I believe that is inherent in checking [=list/item] but the check for the rpID is not. It perfectly possible that an item is present on the authenticator but for a different rpID while if ID and type are invalid, this cannot be a credential on this authenticator.

It seems you @jovasco are saying that we do not need to be explicit regarding the definition of "matching" in this context. However, the present language in step 16.2 in section #discover-from-external-source is the result of addressing issue #280. I would not be surprised to have a similar issue raised here down the road if we are not explicit. Is there any harm/errors with the proposed language above?

@equalsJeffH Alright, if we do this, we better do it right:

Check if a credential matching an [=list/item=] of |excludeCredentialDescriptorList| is bound to this authenticator with the following procedure:

verify that the |excludeCredentialDescriptorList|.{{PublicKeyCredentialDescriptor/id}} was generated by this Authenticator.

verify that the |excludeCredentialDescriptorList|.{{PublicKeyCredentialDescriptor/type}} matches the |excludeCredentialDescriptorList|.{{PublicKeyCredentialDescriptor/id}}

verify that |rpEntity|.{{PublicKeyCredentialRpEntity/id}} is identical to the one used to create the |excludeCredentialDescriptorList|.{{PublicKeyCredentialDescriptor/id}}.

If all of these check succeed, return an error code equivalent to "{{NotAllowedError}}" and terminate the operation.

If you agree I will update the pull request for both make crednetial and getassertion.

@jovasco proposed:

Check if a credential matching an [=list/item=] of |excludeCredentialDescriptorList| is bound to this authenticator with the following procedure:

hm, I think i see whay you are getting at -- i.e., being even more explicit -- though, this does not seem correct to me...in the above, do you mean to also say "...for each item of |excludeCredentialDescriptorList| ?

i. verify that the <code>|excludeCredentialDescriptorList|. {{PublicKeyCredentialDescriptor/id}}</code> was generated by this Authenticator.

in the above, "generated by this authnr" is not what we are looking for. Rather, we are trying to ascertain whether there exists a cred bound to this authnr having a credential ID matching |excludeCredentialDescriptorList|.{{PublicKeyCredentialDescriptor/id}}.

ii. verify that the <code>|excludeCredentialDescriptorList|. {{PublicKeyCredentialDescriptor/type}}</code> matches the <code>|excludeCredentialDescriptorList|.{{PublicKeyCredentialDescriptor/id}}</code>

in the above, do you mean to say something like "if there exists a cred bound to this authnr whose {{PublicKeyCredentialDescriptor/type}} value matches |excludeCredentialDescriptorList|.{{PublicKeyCredentialDescriptor/type}} ?

iii. verify that <code>|rpEntity|.{{PublicKeyCredentialRpEntity/id}}</code> is identical to the one used to create the <code>|excludeCredentialDescriptorList|. {{PublicKeyCredentialDescriptor/id}}</code>.

in the above, do you mean to say something like "if there exists a cred bound to this authnr whose RP ID matches |excludeCredentialDescriptorList|.{{PublicKeyCredentialDescriptor/id}}" ?

Perhaps the below works ?

3. Check if any credential, bound to this authenticator and matching an [=list/item=] of |excludeCredentialDescriptorList|, exists. A match occurs if a credential matches all of an |excludeCredentialDescriptorList| item's <code>|rpEntity|. {{PublicKeyCredentialRpEntity/id}}</code>, <code>|excludeCredentialDescriptorList|. {{PublicKeyCredentialDescriptor/id}}</code>, and <code>|excludeCredentialDescriptorList|.{{PublicKeyCredentialDescriptor/type}}</code>. If so, return an error code equivalent to "{{NotAllowedError}}" and terminate the operation.

@equalsJeffH I modified it slightly. rpId is not part of the exclude list item and I found the language a bit tortured.

equalsJeffH · 2017-10-23T17:10:54Z

review of this is here: #651 (review)

equalsJeffH · 2017-10-23T17:13:38Z

Do we similarly need to refine step 3 of the authenticatorGetAssertion operation (#op-get-assertion)?

akshayku · 2017-10-23T17:20:03Z

@equalsJeffH Yes, we do.

jovasco · 2017-10-23T17:37:59Z

@equalsJeffH Yes.

jovasco · 2017-10-23T17:42:55Z

Fixed.

nadalin · 2017-10-23T18:26:00Z

@akshayku Have you reviewed the changes yet as we would like to merge this

akshayku

Looks good to me

selfissued

This looks fine to me.

equalsJeffH · 2017-10-23T21:48:52Z

i still have a question here: #651 (comment)

equalsJeffH · 2017-10-31T21:27:16Z

reply to @jovasco here: #651 (comment)

jcjones · 2017-11-01T17:31:11Z

I'm willing to take over this PR to resolve @equalsJeffH 's comments if @jovasco does not have time to do it this week. I can do it on Friday, 3 Nov.

Also added a reference to the list item back in the correct place.

equalsJeffH · 2017-11-02T22:46:47Z

thanks, this looks good to me.

equalsJeffH

LGTM, thanks!

Only exclude CredIDs matching the RPID

d879b79

Only credentials in the exclude credentials list that match this RPID should result in a not allowed error.

jovasco mentioned this pull request Oct 16, 2017

authenticatorMakeCredential operation credential filtering #650

Closed

equalsJeffH reviewed Oct 16, 2017

View reviewed changes

equalsJeffH mentioned this pull request Oct 18, 2017

authenticatorMakeCredential: Only exclude CredIDs matching the RPID #654

Closed

equalsJeffH added this to the WD-07 milestone Oct 18, 2017

equalsJeffH added the type:technical label Oct 18, 2017

jcjones approved these changes Oct 19, 2017

View reviewed changes

akshayku requested changes Oct 21, 2017

View reviewed changes

Change |rpId| to rpEntity.id

be4c139

equalsJeffH reviewed Oct 23, 2017

View reviewed changes

Johan Verrept added 2 commits October 23, 2017 19:40

Fix getAssertion too.

4794adb

In this case it is |rpId|

a242e6a

nadalin assigned jovasco Oct 23, 2017

akshayku approved these changes Oct 23, 2017

View reviewed changes

selfissued approved these changes Oct 23, 2017

View reviewed changes

Johan Verrept added 4 commits November 2, 2017 16:40

Improved language.

af04111

Fix up makeCredential with the same fixes.

558c013

Also added a reference to the list item back in the correct place.

Update index.bs

e051327

Fix indent.

6e5f27f

equalsJeffH approved these changes Nov 2, 2017

View reviewed changes

jcjones approved these changes Nov 3, 2017

View reviewed changes

jcjones merged commit 625bd7a into w3c:master Nov 3, 2017

WebAuthnBot pushed a commit that referenced this pull request Nov 3, 2017

Built by Travis-CI: 625bd7a Merge pull request #651 from jovasco/patch-1

ca6a49e

equalsJeffH mentioned this pull request Jan 19, 2018

Privacy across Account IDs #204

Closed

Only exclude CredIDs matching the RPID #651

Only exclude CredIDs matching the RPID #651

Uh oh!

Conversation

jovasco commented Oct 16, 2017 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emlun commented Oct 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emlun commented Oct 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emlun commented Oct 16, 2017

Uh oh!

equalsJeffH left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

equalsJeffH commented Oct 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcjones commented Oct 19, 2017

Uh oh!

jcjones commented Oct 19, 2017

Uh oh!

jcjones left a comment

Choose a reason for hiding this comment

Uh oh!

jcjones commented Oct 19, 2017

Uh oh!

equalsJeffH commented Oct 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

equalsJeffH left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

equalsJeffH commented Oct 23, 2017

Uh oh!

equalsJeffH commented Oct 23, 2017

Uh oh!

akshayku commented Oct 23, 2017

Uh oh!

jovasco commented Oct 23, 2017

Uh oh!

jovasco commented Oct 23, 2017

Uh oh!

nadalin commented Oct 23, 2017

Uh oh!

akshayku left a comment

Choose a reason for hiding this comment

Uh oh!

selfissued left a comment

Choose a reason for hiding this comment

jovasco commented Oct 16, 2017 •

edited by pr-preview bot

Loading

emlun commented Oct 16, 2017 •

edited

Loading

emlun commented Oct 16, 2017 •

edited

Loading

equalsJeffH commented Oct 18, 2017 •

edited

Loading