Describe how authenticators unique and find credential sources. #623

jyasskin · 2017-10-10T21:28:47Z

This PR builds on #620. Just look at the last commit (50d8d1b) to see what's new here. @leshi, @rlin1, @akshayku, you were cc'ed on #602 (comment), which this patch implements.

This happens to fix a maybe-bug where the authenticator didn't check that a
decrypted credential ID came from the right RP.

It's also much more precise about the distinction between a credential
descriptor and a credential or credential source.

This still only improves #578, but it makes it clearer where we'll put that fix.

Preview (#authenticato…) (#op-make-cred) (#op-get-asser…) | Diff

rlin1 · 2017-10-11T20:21:54Z

I think we should use the term Credential for the authentication key pair, i.e. the thing that gets created by authenticatorMakeCredential.
And likewise we should continue calling the thing assertion that is sent to the RP and then verified by the RP.

rlin1

We are currently using the term credential for the authentication kay pair and assertion for the one-time data sent to and then verified by the RP.

Re-defining credential to refer to the assertion is a BIG change. That would have to cover much more than what is currently covered.

rlin1 · 2017-10-11T20:06:14Z

index.bs

@@ -324,13 +325,60 @@ The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "S
 :: A user agent implementing, in conjunction with the underlying platform, the [=Web Authentication API=] and algorithms
    given in this specification, and handling communication between [=authenticators=] and [=[RPS]=].

+: <dfn>Credential ID</dfn>
+:: A probabilistically-unique [=byte sequence=] identifying a [=public key credential source=].


the way we use credential in the spec today suggests that credential actually refers to the authentication key pair and not the authentication assertion.
authenticatorMakeCredential makes a credential i.e. the key pair.

when ever we mean the assertion we use the term assertion (not credential)

Credential ID is already defined in the spec at this point. This addition can be deleted at this point.

rlin1 · 2017-10-11T20:09:48Z

index.bs

+
+    A [=public key credential=] is created by the [=authenticatorGetAssertion=] operation using a [=public key credential
+    source=] created by a previous [=authenticatorMakeCredential=] call. The [=[RP]=] verifies it using the [=credential public
+    key=] returned from that [=authenticatorMakeCredential=] call via {{PublicKeyCredential/[[Create]]()}}.


I think this is confusing: when I call Credential.create I would expect to get a credential back - not a credential source.

rlin1 · 2017-10-11T20:10:44Z

index.bs

@@ -940,7 +977,7 @@ When this method is invoked, the user agent MUST execute the following algorithm
        2.  Let |value| be a new {{PublicKeyCredential}} associated with |global| whose fields are:

            :   {{PublicKeyCredential/[[identifier]]}}
-            ::  A new {{ArrayBuffer}}, created using |global|'s [=%ArrayBuffer%=], containing the bytes of the credential ID
+            ::  A new {{ArrayBuffer}}, created using |global|'s [=%ArrayBuffer%=], containing the bytes of the [=credential ID=]


if we use credential source to refer to the public key/private key, the credential ID should be a credential source ID.

rlin1 · 2017-10-11T20:15:50Z

index.bs


 On successful completion of this operation, the authenticator returns the [=attestation object=] to the client.


-### The <dfn>authenticatorGetAssertion</dfn> operation ### {#op-get-assertion}
+<h4 id="op-get-assertion" algorithm> The <dfn>authenticatorGetAssertion</dfn> operation</h4>


should be renamed to authenticatorGetCredential (if we stick to the redefinition of credential)

rlin1 · 2017-10-11T20:16:52Z

index.bs

+: |hash|
+:: The [=hash of the serialized client data=], provided by the client.
+: |allowCredentialDescriptorList|
+:: An optional list of {{PublicKeyCredentialDescriptor}}s describing credentials acceptable to the [=[RP]=] (possibly filtered


would have to be renamed to PublicKeyCrdentialSourceDescriptor describing credential sources acceptable...

rlin1 · 2017-10-11T20:17:50Z

index.bs

+    - |selectedCredential|.[=public key credential source/id=]
+    - |authenticatorData|
+    - |signature|
+    - |selectedCredential|.[=public key credential source/userHandle=]

 If the authenticator cannot find any credential corresponding to the specified [=[RP]=] that matches the specified criteria, it


would have to be renamed to credential source corresponding ...

rlin1 · 2017-10-11T20:18:13Z

index.bs

@@ -1845,7 +1943,7 @@ credential. It has the following format:
    </tr>
    <tr>
        <td>L</td>
-        <td>Credential ID</td>
+        <td>[=Credential ID=]</td>


would have to be renamed to CredentialSource ID

rlin1 · 2017-10-11T20:19:56Z

index.bs

@@ -3483,10 +3581,10 @@ credential.

 9. If an assertion was successfully generated and returned,
    - The script sends the assertion to the server.
-    - The server examines the assertion, extracts the credential ID, looks up the registered
+    - The server examines the assertion, extracts the [=credential ID=], looks up the registered
        credential public key it is database, and verifies the assertion's authentication signature.


would have to be rephrased to ... and verifies the credential's signature

jyasskin · 2017-10-11T22:31:18Z

I've replied to @rlin1's comments in #620 (comment), since they're really about that change.

nadalin · 2017-10-17T08:18:40Z

@akshayku @leshi Please review before 10/18

This also redefines "Public Key Credential" to be the thing presented to the RP, as a willful violation of RFC4949. Credential ID is defined to explicitly include the possibility that it's the encrypted Credential Source.

This happens to fix a maybe-bug where the authenticator didn't check that a decrypted credential ID came from the right RP. It's also much more precise about the distinction between a credential descriptor and a credential or credential source.

selfissued

This looks OK to me.

equalsJeffH

Overall, I wonder about this being too prescriptive for authenticators and their implementers. One aspect is that I would venture to guess that such implementers do not commonly use WebIDL and thus wonder whether stipulating algorithms in detail via WebIDL is appropriate for that audience.

Another aspect is wondering whether/how this matches-up with the CTAP spec -- tho it does not have to be an exact match: we are ostensibly being purposely hand-wavy wrt authenticator implementations here in the webauthn spec, e.g., to give platform authnr implementers wiggle-room (i.e., those folks not necessarily depending directly on CTAP).

That said, the additions made here to the authenticatorMakeCredential and authenticatorGetAssertion operations seem correct.

Tho, I have a comment below wrt placement of the formal definition of the proposed credentials map and the credential id lookup algorithm.

equalsJeffH · 2017-10-25T21:26:09Z

index.bs

+    1. If |credSource|.[=public key credential source/id=] is |credentialId|, return |credSource|.
+1. Return `null`.
+
+</div>


I suggest we keep all the gory formal definitions and algorithmic stuff down in subsections below this introductory prose.

BTW, I continue to wish to move this algorithm to it's own section. I did not do that as a part of the major merge-from-master catchup, but will do now if there's no objections.

equalsJeffH · 2017-10-25T21:44:28Z

Reviewed: #623 (review)

equalsJeffH · 2017-12-06T21:22:42Z

bump: @jyasskin: #623 (review)

equalsJeffH · 2017-12-06T21:26:57Z

getting input from folks who are involved in platform authenticator APIs (i.e., not only CTAP) would be good here: #623 (review)

cc: @emlun @akshayku @jcjones @leshi @rlin1

emlun

Looks good overall to me, but I agree with @equalsJeffH on moving the definition of the lookup algorithm.

emlun · 2017-12-07T15:54:13Z

index.bs

+    1. If |credSource|.[=public key credential source/id=] is |credentialId|, return |credSource|.
+1. Return `null`.
+
+</div>


equalsJeffH · 2018-01-29T23:24:02Z

Am still working on merge-from-master. the portions in "6.2.2. The authenticatorGetAssertion operation" are tricky. more news on the morrow...

equalsJeffH · 2018-01-30T20:51:54Z

This is ready for re-review. I merged-from-master and believe it is appropriately faithful to @jyasskin's original PR modulo master having evolved since then.

my issues with this have been addressed.

emlun · 2018-01-31T08:48:43Z

index.bs

+    |rpId|.
+1. If |credentialOptions| is now empty, return an error code equivalent to "{{NotAllowedError}}" and terminate the operation.
+
+1. If |allowCredentialDescriptorList| was not supplied, set it to a list of all credentials stored for |rpId| (as determined by


It looks to me like steps 2-6 and steps 7-9 do the same thing, no? In that case, we should delete steps 7-9 and change |allowCredentialDescriptorList| to |credentialOptions| in step 10.

oops, how embarrassing, me thinks ur correct.

emlun · 2018-01-31T08:49:52Z

index.bs

-    the [=authenticator=] if it has its own output capability, or by the user agent otherwise. The prompt SHOULD display the
-    |rpId| and any additional displayable data associated with |selectedCredential|, if possible.
-
-    If |requireUserVerification| is `true`, the method of obtaining [=user consent=] MUST include [=user verification=].


The requireUserVerification parameter is now unused. Is this intentional?

good catch, thx.

emlun · 2018-01-31T08:50:13Z

index.bs

-
-    If |requireUserVerification| is `true`, the method of obtaining [=user consent=] MUST include [=user verification=].
-
-    If |requireUserPresence| is `true`, the method of obtaining [=user consent=] MUST include a [=test of user presence=].


The requireUserPresence parameter is now unused. Is this intentional?

good catch, thx.

emlun · 2018-01-31T08:55:43Z

index.bs

-        - The [=user handle=] associated with |selectedCredential|, if available.
+        Return to the user agent:
+            - |selectedCredential|.[=public key credential source/id=], if either a list of credentials of length 2 or greater was
+                supplied by the client, or no such list was supplied.


If my above comment about allowCredentialDescriptorList is correct, we should reference allowCredentialDescriptorList (which is then untouched at this point) here and in the Note: below.

emlun · 2018-01-31T09:01:03Z

index.bs

+    
+            - |authenticatorData|
+            - |signature|
+            - |selectedCredential|.[=public key credential source/userHandle=]


CTAP stores the user handle only for client-side-resident private key credentials, so it seems misleading to imply that it must always be returned (unless we point out at the definition site that the userHandle field is nullable). See also the client's getAssertion method.

emlun · 2018-01-31T09:04:47Z

index.bs

+        ::  The [=Relying Party Identifier=], for the [=[RP]=] this [=public key credential source=] is associated with.
+
+        :   <dfn>userHandle</dfn>
+        ::  The [=user handle=] associated when this [=public key credential source=] was created.


I think we need to point out that this field is nullable.

equalsJeffH

there's one change I wish to make now that the major merge-from-master catchup is done.

equalsJeffH · 2018-01-31T18:04:58Z

index.bs

+    1. If |credSource|.[=public key credential source/id=] is |credentialId|, return |credSource|.
+1. Return `null`.
+
+</div>


BTW, I continue to wish to move this algorithm to it's own section. I did not do that as a part of the major merge-from-master catchup, but will do now if there's no objections.

selfissued

This looks good to me now. It's clearly an improvement over the existing text.

equalsJeffH · 2018-01-31T22:23:38Z

@emlun's comments are addressed I think in 48de8b2

equalsJeffH · 2018-01-31T22:36:02Z

I'm more confident @emlun's comments are addressed now given the further commits.

emlun

Thanks @equalsJeffH, this looks good to me except one last thing... :)

emlun · 2018-01-31T22:46:21Z

index.bs

@@ -2351,7 +2396,7 @@ It takes the following input parameters:
 : |allowCredentialDescriptorList|
 :: An optional [=list=] of {{PublicKeyCredentialDescriptor}}s describing credentials acceptable to the [=[RP]=] (possibly filtered
    by the client), if any.
-: |requireUserPresence|
+: <var ignore>requireUserPresence</var>


This <var> is now defined and ignored in two places... :)

if I remove the ignore here, i get the annoying "only used once" warning.

If you remove just one, yes, but not if you remove both this one and #623 (comment) .

yeah, i was gonna try removing it from both next but was running out door to go sit in traffic....u r of course correct, fixed now in
14335b1

emlun · 2018-01-31T22:46:33Z

index.bs


    If |requireUserVerification| is `true`, the method of obtaining [=user consent=] MUST include [=user verification=].

-    If |requireUserPresence| is `true`, the method of obtaining [=user consent=] MUST include a [=test of user presence=].
+    If <var ignore>requireUserPresence</var> is `true`, the method of obtaining [=user consent=] MUST include a 


This <var> is now defined and ignored in two places... :)

equalsJeffH · 2018-02-01T23:55:12Z

just pushed new commit dc50b3f that moves the op-lookup-credsource-by-credid alg to new subsection.

emlun

👍

equalsJeffH · 2018-02-05T17:31:46Z

Given the approvals, I'll merge this later today 5-Feb-2018 in the afternoon PT if there's no objections. thx.

jyasskin requested review from leshi, rlin1 and akshayku October 10, 2017 21:28

jyasskin self-assigned this Oct 10, 2017

rlin1 requested changes Oct 11, 2017

View reviewed changes

jyasskin mentioned this pull request Oct 11, 2017

Define Public Key Credential Source and Credential ID. #620

Merged

selfissued added this to the WD-07 milestone Oct 12, 2017

jyasskin force-pushed the put-credentials-in-a-map branch from ef99d3a to 715a9a3 Compare October 12, 2017 16:58

jyasskin added 4 commits October 23, 2017 10:42

Define Public Key Credential Source and Credential ID.

fb72770

This also redefines "Public Key Credential" to be the thing presented to the RP, as a willful violation of RFC4949. Credential ID is defined to explicitly include the possibility that it's the encrypted Credential Source.

Link "credential ID".

bade774

Allow hashes as credential IDs.

493dd3c

jyasskin force-pushed the put-credentials-in-a-map branch from 715a9a3 to 50d8d1b Compare October 23, 2017 18:46

selfissued approved these changes Oct 23, 2017

View reviewed changes

equalsJeffH previously requested changes Oct 25, 2017

View reviewed changes

equalsJeffH modified the milestones: WD-07, CR Nov 1, 2017

jyasskin mentioned this pull request Nov 1, 2017

Sensible limits for RP and User Entity fields. #667

Merged

equalsJeffH requested review from emlun and jcjones December 6, 2017 21:23

emlun reviewed Dec 7, 2017

View reviewed changes

nadalin assigned akshayku Dec 13, 2017

nadalin removed the request for review from akshayku December 13, 2017 18:21

nadalin unassigned jyasskin Dec 13, 2017

JeffH added 6 commits January 29, 2018 11:09

first pass merge-from-upstream-master-and-fix-conflicts

257b3d6

Merge branch 'master' into put-credentials-in-a-map

50528aa

finish merge-from-master and fixup dangling internal crossrefs

bda9ba8

restore masthead

31efa65

restore clientDataHash rather than tbsHash in U2F attstn format

69a7a14

fixing rendering issue

e7a1d2b

JeffH added 2 commits January 30, 2018 11:26

fixup merge-from-master loose ends by hand

5f47c4c

fix var ignore issue

5254ef9

emlun requested changes Jan 31, 2018

View reviewed changes

Merge branch 'master' into put-credentials-in-a-map

f46d5de

equalsJeffH reviewed Jan 31, 2018

View reviewed changes

selfissued approved these changes Jan 31, 2018

View reviewed changes

equalsJeffH assigned equalsJeffH and unassigned akshayku Jan 31, 2018

JeffH added 2 commits January 31, 2018 13:13

Merge branch 'master' into put-credentials-in-a-map

546398e

address emlun's comments, thx!

48de8b2

JeffH added 2 commits January 31, 2018 14:31

catch straggler from emlun's comments, mea culpa

1a9ccb3

ignore a var make bikeshed happier

369202a

emlun requested changes Jan 31, 2018

View reviewed changes

move op-lookup-credsource-by-credid alg to new subsection

dc50b3f

dont need ignore no more

14335b1

emlun approved these changes Feb 2, 2018

View reviewed changes

equalsJeffH merged commit 4f1a3ba into w3c:master Feb 6, 2018


		If \|requireUserVerification\| is `true`, the method of obtaining [=user consent=] MUST include [=user verification=].

		If \|requireUserPresence\| is `true`, the method of obtaining [=user consent=] MUST include a [=test of user presence=].

Describe how authenticators unique and find credential sources. #623

Describe how authenticators unique and find credential sources. #623

Conversation

jyasskin commented Oct 10, 2017 • edited by pr-preview bot Loading

rlin1 commented Oct 11, 2017

rlin1 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyasskin commented Oct 11, 2017

nadalin commented Oct 17, 2017

selfissued left a comment

Choose a reason for hiding this comment

equalsJeffH left a comment

Choose a reason for hiding this comment

equalsJeffH Oct 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

equalsJeffH commented Oct 25, 2017

equalsJeffH commented Dec 6, 2017

equalsJeffH commented Dec 6, 2017

emlun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

equalsJeffH commented Jan 29, 2018

equalsJeffH commented Jan 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emlun Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

equalsJeffH left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

selfissued left a comment

Choose a reason for hiding this comment

equalsJeffH commented Jan 31, 2018

equalsJeffH commented Jan 31, 2018

emlun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

equalsJeffH commented Feb 1, 2018

emlun left a comment

Choose a reason for hiding this comment

equalsJeffH commented Feb 5, 2018

jyasskin commented Oct 10, 2017 •

edited by pr-preview bot

Loading

rlin1 left a comment •

edited

Loading

equalsJeffH Oct 25, 2017 •

edited

Loading

emlun Jan 31, 2018 •

edited

Loading