Make makeCredential() more precise. #347

jyasskin · 2017-02-15T01:28:10Z

I've linked a lot more terms, reordered explanations to be clearer, and
specified some missing behavior.

This fixes #263 and #273, and improves #254 and #270.

annevk · 2017-02-15T08:58:28Z

It seems #273 is the more elaborate issue.

annevk · 2017-02-15T08:59:41Z

index.bs

+    : {{challenge}}
+    :: The [=base64url encoding=] of {{attestationChallenge}}
+    : {{origin}}
+    :: |rpId|


rpId is not an origin though. It can be a host per the above steps.

Yep, I haven't tackled #259 yet, and I think that'll be easier once w3c/html#769 (splitting the document.domain setter) is fixed. So fixing this will probably be a follow-up. That said, it's probably a good idea to specify this as one of the serializations of an origin, pretending |rpId| is already an origin.

w3c/html o_O

We'll get the fix in whatwg/html; w3c is just where the bug happened to get filed due to #257.

annevk · 2017-02-23T08:54:05Z

index.bs

+    : {{ClientData/extensions}}
+    :: <code>{{options}}.{{ScopedCredentialOptions/extensions}}</code>
+
+10. Let |clientDataJSON| be the UTF-8 encoding of the result of calling the


You should link UTF-8 encoding to https://encoding.spec.whatwg.org/#utf-8-encode.

equalsJeffH

thanks @jyasskin, this seems to me to be an overall improvement to the prior state. I have some questions/suggestions inline.

equalsJeffH · 2017-02-22T22:52:47Z

index.bs

@@ -62,6 +66,7 @@ spec: WebCryptoAPI; urlPrefix: https://www.w3.org/TR/WebCryptoAPI/
 </pre> <!-- class=anchors -->

 <pre class="link-defaults">
+spec:infra; type:dfn; text:list


Add "infra" to the list of dependencies in {#dependencies}?

Bikeshed does provide an [INFRA] in https://api.csswg.org/bikeshed/?url=https://raw.githubusercontent.com/jyasskin/webauthn/clean-make-credential/index.bs#normative with the used terms listed in https://api.csswg.org/bikeshed/?url=https://raw.githubusercontent.com/jyasskin/webauthn/clean-make-credential/index.bs#index-defined-elsewhere. I can add it redundantly to #dependencies, or we could plan to remove that whole section in favor of the automated sections.

I usually state them upfront so readers know what kind of things to expect, but maybe we should stop doing that? Probably worth revisiting at some point.

@annevk said:

I usually state them upfront so readers know what kind of things to expect, but maybe we should stop doing that? Probably worth revisiting at some point

yeah, something to think about. of course i was lame and didn't search further down in doc for mentions/links to [INFRA]. [ perhaps also bikeshed could eventually construct a {#dependencies} section nearer front of specs. ]

Would y'all file an issue in https://github.com/tabatkins/bikeshed/issues about generating a dependencies section earlier in the document? For now ... I'm going to take this thread as an indication not to add "infra" to #dependencies unless y'all say otherwise.

@jyasskin

Would y'all file an issue in https://github.com/tabatkins/bikeshed/issues about generating a dependencies section earlier in the document?

done: speced/bikeshed#937

I'm going to take this thread as an indication not to add "infra" to #dependencies unless y'all say otherwise.

ok, tho perhaps Dependencies section ought to have link to #index-defined-elsewhere ?

equalsJeffH · 2017-02-22T22:54:44Z

index.bs

-    Then asynchronously continue executing the following steps. If any fatal error is encountered in this process other than the
-    ones enumerated below, cancel the timer, reject |promise| with a DOMException whose name is "{{UnknownError}}", and terminate
-    this algorithm.
+    Issue: Put some constraints on the "reasonable range".


hm, maybe we should only add issues such as this to the spec if there are corresponding issues filed in https://github.com/w3c/webauthn/issues/ and said in-spec-issue explicitly links to the issue-in-github -- i.e., if any in-spec-issues remain at time of merge, they should have corresponding webauthn/issues/ filed.

e.g., some of the below in-spec-issues might be resolved by incorporating review feedback.

Yeah, that sounds right. I'm hoping to resolve most of the new Issues using your feedback during the review, but I'll file official issues for anything we can't resolve here.

sounds good, thx.

equalsJeffH · 2017-02-23T01:14:56Z

index.bs

+    cancel the timer started in step 2, return [=a promise rejected with=] with
+    a {{DOMException}} whose name is "{{NotSupportedError}}", and terminate this
+    algorithm.
+


add..
Issue(#266): is WRT the above step.
..?

I've incorporated a fix for #266.

equalsJeffH · 2017-02-23T19:32:39Z

index.bs

+        an error occurs during this procedure, then [=continue=].
+    - [=list/Append=] a new object of type {{ScopedCredentialParameters}} to
+        |normalizedParameters|, with |type| set to
+        <code>|current|.{{ScopedCredentialParameters/type}}</code> and


add..
Issue(#265): is WRT the above step.
..? (and perhaps just go ahead and incorp the proposed solution?)

I've incorporated the proposed solution.

yay, tho there might be a modest bug in last substep: https://github.com/w3c/webauthn/pull/347/files#r103042454 ?

equalsJeffH · 2017-02-23T22:12:29Z

index.bs

+    <a>present</a>, then [=map/for each=] |extension| → |argument| of
+    <code>{{options}}.{{ScopedCredentialOptions/extensions}}</code>:
+    1. If |extension| is not supported by this client platform, [=continue=].
+    2. Let |result| be the result of running |extension|'s [=client processing=] algorithm on |argument|.


s/algorithm/algorithm, if any,/

We'd need to define what to do with clientExtensions for an extension with no client processing algorithm. https://w3c.github.io/webauthn/#extensions says

Clients wishing to support the widest possible range of extensions may choose to pass through any extensions that they do not recognize to authenticators, generating the authenticator argument by simply encoding the client argument in CBOR.

I'd read that as a default definition of an extension's client processing, but I guess it's actually one of 3 alternatives for this step:

The extension's defined client processing.

CBOR-encode argument.

Continue.

see https://github.com/w3c/webauthn/pull/347/files#r103043381 for possible resolution...

equalsJeffH · 2017-02-23T23:01:48Z

index.bs

+    :: <code>{{options}}.{{ScopedCredentialOptions/extensions}}</code>
+
+10. Let |clientDataJSON| be the UTF-8 encoding of the result of calling the
+    original value of {{JSON/stringify|JSON.stringify}} on |clientData|.


what is meant by "calling the original value of JSON.stringify"? The term "original" does not appear in or around https://tc39.github.io/ecma262/#sec-json.stringify

It should have been "initial", matching https://www.w3.org/2001/tag/doc/promises-guide/#a-new-promise. It's a fuzzy term, but it's meant to defend against the web page assigning a new value to window.JSON or to JSON.stringify.

ah, ok, thx.

equalsJeffH · 2017-02-23T23:16:47Z

index.bs

+14. [=set/For each=] |authenticator| in |currentlyAvailableAuthenticators|:
+    1. Let |excludeList| be a new [=list=].
+    2. [=list/For each=] credential |C| in <code>{{options}}.{{ScopedCredentialOptions/excludeList}}</code>:
+        1. If |C| has an empty {{transports}} list, [=list/append=] it to


equalsJeffH · 2017-02-23T23:22:29Z

index.bs

+        2. If |authenticator| is connected over a transport not mentioned in
+            <code>|C|.{{transports}}</code>, the client MAY [=continue=].
+
+            Issue: I'm not sure this captures the intent of the original wording.


upon review it seems to me it does capture the original intent, tho would be good to get @vijaybh's (at least) review.

equalsJeffH · 2017-02-23T23:28:58Z

index.bs

-    this client platform, to produce the extension data that needs to be sent to the authenticator. If an error is encountered
-    while processing an extension, skip that extension and do not produce any extension data for it. Call the result of this
-    processing |clientExtensions|.
+    Issue: Should "process" be "algorithm", or does it actually mean an OS


seems to me "process" should be "algorithm".

Changed, but @vijaybh should double-check.

equalsJeffH · 2017-02-23T23:30:34Z

index.bs

-    while processing an extension, skip that extension and do not produce any extension data for it. Call the result of this
-    processing |clientExtensions|.
+    Issue: Should "process" be "algorithm", or does it actually mean an OS
+    process? What kinds of fatal errors are you worried about? I suggest we just


It seems to me a case can be made that any possible errors are covered by the "If any authenticator returns an error status", but perhaps @vijaybh was worried about various platform-level errors that might percolate up in real life, eg when calling authenticatorCancel (e.g., authnr is now unreachable) and intends for that sentence to be a catch-all.

I've linked a lot more terms, reordered explanations to be clearer, and specified some missing behavior. This fixes w3c#273 and improves w3c#270.

This fixes w3c#263 and fixes half of w3c#254.

equalsJeffH · 2017-02-24T21:51:29Z

index.bs

+    :: UA-chosen
+
+        Issue: We need *some* constraints on the possible hash algorithms, or
+        else sites will fail on unusual UAs.


pick from this set: {"SHA-256", "SHA-384", "SHA-512"}, c.f. webcrypto ?

I'm inclined to keep that policy decision out of this PR. :) I've filed an issue and referred to it from here.

equalsJeffH · 2017-02-24T22:01:31Z

index.bs

+        an error occurs during this procedure, then [=continue=].
+    1. [=list/Append=] the pair of
+        <code>|current|.{{ScopedCredentialParameters/type}}</code> and
+        |algorithm| to |normalizedAlgorithm|.


s/normalizedAlgorithm/normalizedParameters/ ?

equalsJeffH · 2017-02-24T22:06:32Z

index.bs

+1. If the {{ScopedCredentialOptions/extensions}} member of {{options}} is
+    <a>present</a>, then [=map/for each=] |extension| → |argument| of
+    <code>{{options}}.{{ScopedCredentialOptions/extensions}}</code>:
+    1. If |extension| is not supported by this client platform, [=continue=].


given #347 (comment), should this substep be something like..

If |extension| is not supported by this client platform, either [=continue=], or optionally CBOR-encode |argument| as a byte string, and go to Step 4.

..@vijaybh ?

I've added some text along those lines, and a new issue to make it better.

jyasskin · 2017-02-24T23:37:05Z

index.bs

+    :: |clientExtensions|
+
+1. Let |clientDataJSON| be the [=UTF-8 encoding=] of the result of calling the
+    initial value of {{JSON/stringify|JSON.stringify}} on |clientData|.


Note that this fixes #274 by precisely specifying the JSON format. Do UAs need to use the flexibility that was here before?

No, we don't want to allow for multiple JSON implementations (and then later have that end up being required due to some silliness/webness).

You might want to make this change centrally in https://w3c.github.io/webauthn/#clientdata-clientdatajson and reference that here. Maybe we should also change the variable name so that we can say something like "Generate the [=clientDataJSON=] and call it |J|."

annevk · 2017-02-25T06:32:44Z

(Please don't use usernames in commit messages anymore. The spam is annoying and can persist rather long.)

jyasskin · 2017-02-28T08:42:58Z

I'm at a C++ committee meeting this week and on vacation next week, so if you want any more changes to this PR, either please feel free to make them yourself, or I'll get to them the week of March 13.

equalsJeffH · 2017-02-28T21:08:54Z

LGTM, thanks @jyasskin.

vijaybh · 2017-02-24T23:39:40Z

index.bs

-    Then asynchronously continue executing the following steps. If any fatal error is encountered in this process other than the
-    ones enumerated below, cancel the timer, reject |promise| with a {{DOMException}} whose name is "{{UnknownError}}", and terminate
-    this algorithm.
+    Issue: Put some constraints on the "reasonable range".


Can we use Github to track issues and not put them in the document? Over time it becomes painful to track these inline issues.

see thread here: #347 (comment)

Straw man: I chose [15,120] seconds as the range for timeouts in the first Firefox implementation. Under 15 seconds seems too short to dig out a security key, and making things blink for more than two minutes seemed cruel.

vijaybh · 2017-02-24T23:44:00Z

index.bs


 1. Set |callerOrigin| to the <a>current settings object</a>'s <a>origin</a>. If |callerOrigin| is
-    an <a>opaque origin</a>, reject |promise| with a {{DOMException}} whose name is "{{NotAllowedError}}", and
+    an <a>opaque origin</a>, [=reject=] |promise| with a {{DOMException}} whose name is "{{NotAllowedError}}", and


Alternatively, just get rid of |promise| entirely, use [=a promise rejected with=] here and in other error cases, and use [=a new promise=] when going async below.

vijaybh · 2017-02-28T23:55:02Z

index.bs

+    <code>{{options}}.{{ScopedCredentialOptions/extensions}}</code>:
+    1. If |extension| is not supported by this client platform, then either:
+        * [=Continue=], or
+        * Let |result| be a CBOR ([[!RFC7049]]) encoding of |extension|.


The consensus among implementers was that we do not want to blindly pass extensions along since this may break a contract the UA has with the user, e.g. you may blindly pass along a location extension when the user has asked to block location info. So maybe we should have [=Continue=] as the only option.

vijaybh · 2017-02-28T23:59:22Z

index.bs

+    : {{origin}}
+    :: The [=unicode serialization of an origin|unicode serialization=] of |rpId|
+    : {{hashAlg}}
+    :: UA-chosen


We've intentionally avoided using "UA" anywhere in this spec. Suggest "Hash algorithm for generating clientDataHash, by the client". Maybe we could recommend SHA-256 for maximum interop.

vijaybh · 2017-03-01T00:01:20Z

index.bs

+    initial value of {{JSON/stringify|JSON.stringify}} on |clientData|.
+
+    Issue: Some extensions contain ArrayBuffers, which don't stringify well.
+    What's the intent here?


I think we should base64url encode those just as we do with the challenge. Need to update client processing sections of extensions, maybe this should be part of #363.

vijaybh · 2017-03-01T16:34:54Z

index.bs

+    :: |clientExtensions|
+
+1. Let |clientDataJSON| be the [=UTF-8 encoding=] of the result of calling the
+    initial value of {{JSON/stringify|JSON.stringify}} on |clientData|.


You might want to make this change centrally in https://w3c.github.io/webauthn/#clientdata-clientdatajson and reference that here. Maybe we should also change the variable name so that we can say something like "Generate the [=clientDataJSON=] and call it |J|."

vijaybh · 2017-03-01T16:43:29Z

index.bs

+    1. [=list/For each=] credential |C| in <code>{{options}}.{{ScopedCredentialOptions/excludeList}}</code>:
+        1. If |C| has an empty {{transports}} list, [=list/append=] |C| to
+            |excludeList| and [=continue=].
+        1. If |authenticator| is connected over a transport not mentioned in


This is an "else" from the previous point, right? This seems a bit unwieldy.

Perhaps we could replace all these three sub-bullets with two:

If |C|.{{transports}} is non-empty and does not contain the transport that |authenticator| is connected over, the client MAY [=continue=].

[=list/Append=] |C| to |excludeList|.

vijaybh · 2017-03-01T16:48:31Z

index.bs

-    any), to create a {{ClientData}} structure representing this request. Choose a hash algorithm for {{ClientData/hashAlg}} and
-    compute the {{ScopedCredentialInfo/clientDataJSON}} and its <a>clientDataHash</a>.
+    Issue: What kinds of fatal errors are you worried about? I suggest we just
+    remove that sentence.


OK. The initial intention was to guard against things like low-level communication breakdowns between client and authenticator, but maybe that becomes part of "any authenticator returns an error" now.

vijaybh · 2017-03-01T16:57:08Z

index.bs

+    "{{NotAllowedError}}".
+
+    Issue: {{NotAllowedError}} seems incorrect for at least the timeout and
+    cancelled exit conditions above.


Suggested alternative? If you have one we can just put it in with this PR or open a new issue.

I've filed #376 for this.

vijaybh · 2017-03-01T16:59:13Z

index.bs

@@ -542,7 +643,7 @@ When this method is invoked, the user agent MUST execute the following algorithm
        execute a platform-specific procedure to determine which, if any, credentials listed in {{AssertionOptions/allowList}}
        might be present on this authenticator, and set |credentialList| to this filtered list. If no such filtering is
        possible, set |credentialList| to an empty list.


Should that be [=list=]? Or were you planning to make getAssertion more precise along similar lines in a new PR?

jcjones

On call we agreed to address the comments in a follow-up PR. This looks good to merge, so we don't have to rebase it.

vijaybh · 2017-03-01T20:04:36Z

As discussed on the call, merging this now. I will send out a new PR with changes reflecting my comments.

As per my comments on #347

* Refine makeCredential description As per my comments on #347 * Incorporate feedback from JeffH, part 1 * Incorporate feedback from JeffH, part 2 Use "client data" and "authenticator data" instead of "ClientData" and "authenticatorData". * Incorporate feedback from JeffH, part 3 Tag all instances of "client data", "authenticator data" and "attestation data".

* Refine makeCredential description As per my comments on #347 * Incorporate feedback from JeffH, part 1 * [=scoped credential(s)=] * [=authenticator=] * [=[RP]=] * [=Conforming User Agent=] * [=[RPS]=] * [=authenticators=] * [=Web Authentication API=] * [=Registration=] * [=Authentication=] * {{makeCredential()}} and {{getAssertion()}} * [=User Verification=] * [=Authentication Assertion=] * [=authorization gesture=] * [=user consent=] * '<a>...</a>' to be '[=...=]' thru terminology section * Incorporate feedback from JeffH, part 2 Use "client data" and "authenticator data" instead of "ClientData" and "authenticatorData". * Incorporate feedback from JeffH, part 3 Tag all instances of "client data", "authenticator data" and "attestation data". * [=present=] * [=attestation object=] * [=effective domain=] * <a>...</a> replacement up to {#iface-credentialInfo} * <a>...</a> replacement up to {#attestation-formats} * <a>...</a> replacement thru rest of spec * refined regex and caught <a>...</a> stragglers * Fix two typos and some locally anomalous line lengths Typos - weird character in "Subsequently" on line 84, "user verification" missing on line 104 (old) / 102 (new).

jyasskin changed the title ~~Fix up makeCredential() to be as precise as a web spec.~~ Make makeCredential() more precise. Feb 15, 2017

jyasskin force-pushed the clean-make-credential branch 2 times, most recently from a1c674c to f06fd29 Compare February 15, 2017 04:33

annevk reviewed Feb 15, 2017

View reviewed changes

jyasskin force-pushed the clean-make-credential branch 2 times, most recently from 47b9c22 to 02b0bb2 Compare February 16, 2017 01:02

jyasskin mentioned this pull request Feb 17, 2017

Add gesture verification parameter to option in both makeC and getA #348

Merged

equalsJeffH self-requested a review February 22, 2017 05:28

jcjones self-requested a review February 22, 2017 18:18

jyasskin force-pushed the clean-make-credential branch from 6388eb1 to 1af52d3 Compare February 22, 2017 22:06

annevk reviewed Feb 23, 2017

View reviewed changes

equalsJeffH suggested changes Feb 23, 2017

View reviewed changes

equalsJeffH requested a review from vijaybh February 23, 2017 23:44

equalsJeffH added this to the WD-05 milestone Feb 23, 2017

jyasskin added 3 commits February 24, 2017 10:45

Make makeCredential() more precise.

b69aefa

I've linked a lot more terms, reordered explanations to be clearer, and specified some missing behavior. This fixes w3c#273 and improves w3c#270.

Treat rpId as an origin.

c172b17

Go parallel later in makeCredential().

7a0d27a

This fixes w3c#263 and fixes half of w3c#254.

jyasskin force-pushed the clean-make-credential branch 6 times, most recently from c403edc to 3268d0c Compare February 24, 2017 20:25

equalsJeffH reviewed Feb 24, 2017

View reviewed changes

jyasskin added 2 commits February 24, 2017 14:39

Fix w3c#265.

7ff341d

Fix w3c#266.

1ca3523

jyasskin mentioned this pull request Feb 24, 2017

Crypto algorithm agility: e.g., Specify the set of hash algorithms UAs can select between. #362

Closed

jyasskin mentioned this pull request Feb 24, 2017

Define extension client processing more carefully. #363

Closed

jyasskin commented Feb 24, 2017

View reviewed changes

jyasskin added 3 commits February 24, 2017 22:52

Fix annevk's and equalsJeffH's comments.

788ec72

Refer to w3c#362.

99b6ec5

Improve processing of unsupported extensions.

62a5ac0

jyasskin force-pushed the clean-make-credential branch from 71b9520 to 62a5ac0 Compare February 25, 2017 06:53

equalsJeffH approved these changes Feb 28, 2017

View reviewed changes

nadalin added the subtype:CTAP label Feb 28, 2017

vijaybh suggested changes Mar 1, 2017

View reviewed changes

equalsJeffH added the subtype:algorithms/WebIDL label Mar 1, 2017

jcjones approved these changes Mar 1, 2017

View reviewed changes

vijaybh approved these changes Mar 1, 2017

View reviewed changes

vijaybh merged commit 546f82f into w3c:master Mar 1, 2017

WebAuthnBot pushed a commit that referenced this pull request Mar 1, 2017

Built by Travis-CI: 546f82f Make makeCredential() more precise. (#347)

951a765

This was referenced Mar 1, 2017

What does normalizedParameters actually contain in makeCredential? #265

Closed

What does "If normalizedAlgorithm is empty" mean? #266

Closed

vijaybh added a commit that referenced this pull request Mar 2, 2017

Refine makeCredential description

66a296a

As per my comments on #347

vijaybh mentioned this pull request Mar 2, 2017

Refine makeCredential description #369

Merged

This was referenced Mar 2, 2017

Creation of ScopedCredentialInfo in makeCredential should probably be more explicit #277

Closed

Constrain the "reasonable range" of timeouts #364

Closed

jyasskin deleted the clean-make-credential branch March 3, 2017 05:50

This was referenced Mar 6, 2017

Exception handling in cryptoParameters processing needs to be clarified #264

Closed

Should attestationChallenge be snapshotted in makeCredential, or used before going async? #272

Closed

equalsJeffH mentioned this pull request Mar 9, 2017

fixup getAssertion, polish algorithms #371

Merged

Make makeCredential() more precise. #347

Make makeCredential() more precise. #347

Conversation

jyasskin commented Feb 15, 2017 • edited Loading

annevk commented Feb 15, 2017

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 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

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

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

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

Choose a reason for hiding this comment

annevk commented Feb 25, 2017

jyasskin commented Feb 28, 2017

equalsJeffH commented Feb 28, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcjones left a comment

Choose a reason for hiding this comment

vijaybh commented Mar 1, 2017

jyasskin commented Feb 15, 2017 •

edited

Loading