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

Make makeCredential() more precise. #347

Merged
merged 8 commits into from
Mar 1, 2017
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
217 changes: 163 additions & 54 deletions index.bs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ Markup Shorthands: css off, markdown on

<pre class="anchors">

spec: ECMAScript; urlPrefix: https://tc39.github.io/ecma262/#
type: method
for: JSON; text: stringify; url: sec-json.stringify

<!-- spec: HTML; urlPrefix: https://html.spec.whatwg.org/multipage/ -->
spec: HTML52; urlPrefix: https://w3c.github.io/html/
type: dfn
Expand All @@ -53,6 +57,10 @@ spec: HTML52; urlPrefix: https://w3c.github.io/html/
type: interface
text: Navigator

spec: TokenBinding; urlPrefix: https://tools.ietf.org/html/draft-ietf-tokbind-protocol-13#
type: dfn
text: Token Binding ID; url: section-3.2

spec: WebCryptoAPI; urlPrefix: https://www.w3.org/TR/WebCryptoAPI/
type: dfn
text: normalizing an algorithm; url: dfn-normalize-an-algorithm
Expand All @@ -61,6 +69,7 @@ spec: WebCryptoAPI; urlPrefix: https://www.w3.org/TR/WebCryptoAPI/
</pre> <!-- class=anchors -->

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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. ]

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

spec:webidl; type:interface; text:Promise
</pre>

Expand Down Expand Up @@ -161,7 +170,8 @@ or a combination of both.

## Dependencies ## {#dependencies}

This specification relies on several other underlying specifications.
This specification relies on several other underlying specifications, listed
below and in [[#index-defined-elsewhere]].

: Base64url encoding
:: The term <dfn>Base64url Encoding</dfn> refers to the base64 encoding using the URL- and filename-safe character set defined
Expand Down Expand Up @@ -372,6 +382,7 @@ underlying platform, which may involve data storage managed by the browser or th
approve this operation. On success, the promise will be resolved with a {{ScopedCredentialInfo}} object describing the newly
created credential.

<div class="note">
This method takes the following parameters:

<ul dfn-type="argument" dfn-for="WebAuthentication/makeCredential(accountInformation, cryptoParameters, attestationChallenge, options)">
Expand All @@ -397,6 +408,7 @@ This method takes the following parameters:
[[#credential-options]].

</ul>
</div>

When this method is invoked, the user agent MUST execute the following algorithm:

Expand All @@ -405,13 +417,10 @@ When this method is invoked, the user agent MUST execute the following algorithm
value. If {{ScopedCredentialOptions/timeout}} was not specified, then set |adjustedTimeout| to a platform-specific
default.

1. Let |promise| be [=a new Promise=]. Return |promise| and start a timer for |adjustedTimeout| milliseconds.
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".
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

see thread here: #347 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


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
Copy link
Contributor

Choose a reason for hiding this comment

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

But you deleted the previous step where |promise| was instantiated, so at this point it is undefined. I suspect what you want to do is keep the part about creating |promise| in the step above this one, and only go async by returning |promise| later on.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

terminate this algorithm.

1. If the {{ScopedCredentialOptions/rpId}} member of {{options}} is not <a>present</a>, then set |rpId| to
Expand All @@ -424,58 +433,150 @@ When this method is invoked, the user agent MUST execute the following algorithm
terminate this algorithm.
1. Set |rpId| to the {{ScopedCredentialOptions/rpId}}.

1. Process each element of {{cryptoParameters}} using the following steps, to produce a new sequence |normalizedParameters|.
- Let |current| be the currently selected element of {{cryptoParameters}}.
- If `current.type` does not contain a {{ScopedCredentialType}} supported by this implementation, then stop processing
|current| and move on to the next element in {{cryptoParameters}}.
- Let |normalizedAlgorithm| be the result of <a>normalizing an algorithm</a> [[!WebCryptoAPI]],
with |alg| set to `current.algorithm` and |op| set to 'generateKey'. If an error occurs during this
procedure, then stop processing |current| and move on to the next element in {{cryptoParameters}}.
- Add a new object of type {{ScopedCredentialParameters}} to |normalizedParameters|, with |type| set to `current.type` and
|algorithm| set to |normalizedAlgorithm|.

1. If |normalizedAlgorithm| is empty and {{cryptoParameters}} was not empty, cancel the timer started in step 2, reject
|promise| with a DOMException whose name is "{{NotSupportedError}}", and terminate this algorithm.

1. If the {{ScopedCredentialOptions/extensions}} member of {{options}} is <a>present</a>, process any extensions supported by
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(w3c/webauthn#259): The rest of this algorithm assumes |rpId| is an
[=origin=], but the above step sometimes produces a string.

1. Let |normalizedParameters| be a new [=list=] whose [=list/items=] are pairs of
ScopedCredentialType and a [=dictionary=] type (as returned by [=normalizing
an algorithm=]).
1. [=list/For each=] |current| of {{cryptoParameters}}:
1. If <code>|current|.{{ScopedCredentialParameters/type}}</code> does not
contain a {{ScopedCredentialType}} supported by this implementation,
then [=continue=].
1. Let |normalizedAlgorithm| be the result of <a>normalizing an algorithm</a>
[[!WebCryptoAPI]], with |alg| set to
<code>|current|.{{algorithm}}</code> and |op| set to `"generateKey"`. If
an error occurs during this procedure, then [=continue=].
1. [=list/Append=] the pair of
<code>|current|.{{ScopedCredentialParameters/type}}</code> and
|normalizedAlgorithm| to |normalizedParameters|.

1. If |normalizedParameters| is empty and {{cryptoParameters}} was not empty,
cancel the timer started in step 2, return [=a promise rejected with=] with
a {{DOMException}} whose name is "{{NotSupportedError}}", and terminate this
algorithm.

1. Let |clientExtensions| be a new [=list=].
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, then either:
* [=Continue=], or
* Let |result| be a CBOR ([[!RFC7049]]) encoding of |extension|.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


Issue(w3c/webauthn#363): Define this encoding more precisely.
1. Otherwise, let |result| be the result of running |extension|'s [=client
processing=] algorithm on |argument|. If the algorithm returned an
error, [=continue=].

Issue(w3c/webauthn#363): Ensure all extensions define a client
processing algorithm.
1. [=list/Append=] |result| to |clientExtensions|.

1. Let |clientData| be a new {{ClientData}} instance whose fields are:
: {{challenge}}
:: The [=base64url encoding=] of {{attestationChallenge}}
: {{origin}}
:: The [=unicode serialization of an origin|unicode serialization=] of |rpId|
: {{hashAlg}}
:: UA-chosen
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


Issue(w3c/webauthn#362): We need *some* constraints on the possible hash
algorithms, or else sites will fail on unusual UAs.
: {{tokenBinding}}
:: The [=Token Binding ID=] associated with |callerOrigin| (if any)

Issue(w3c/webauthn#360): Make sure this association was set up properly.
: {{ClientData/extensions}}
:: |clientExtensions|

1. Let |clientDataJSON| be the [=UTF-8 encoding=] of the result of calling the
initial value of {{JSON/stringify|JSON.stringify}} on |clientData|.
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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


Issue: Some extensions contain ArrayBuffers, which don't stringify well.
What's the intent here?
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

1. Let |clientDataHash| be the hash of |clientDataJSON| using
<code>|clientData|.{{hashAlg}}</code>.

1. Let |issuedRequests| and |currentlyAvailableAuthenticators| be new [=ordered
sets=].

1. For each |authenticator| currently available on this platform, if
<code>{{options}}.{{ScopedCredentialOptions/attachment}}</code> is not
[=present=] or its value matches |authenticator|'s attachment modality,
[=set/append=] |authenticator| to |currentlyAvailableAuthenticators|.

1. [=set/For each=] |authenticator| in |currentlyAvailableAuthenticators|:
1. Let |excludeList| be a new [=list=].
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
Copy link
Contributor

Choose a reason for hiding this comment

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

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:

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

<code>|C|.{{transports}}</code>, the client MAY [=continue=].

Issue: I'm not sure this captures the intent of the original wording.
1. [=list/Append=] |C| to |excludeList|.
1. [=In parallel=], invoke the <a>authenticatorMakeCredential</a> operation
on |authenticator| with |rpId|, |clientDataHash|,
{{accountInformation}}, |normalizedParameters|, |excludeList| and
|clientExtensions| as parameters.
1. [=set/Append=] |authenticator| to |issuedRequests|.

1. Let |promise| be [=a new promise=]. Return |promise| and start a timer for |adjustedTimeout| milliseconds.
Then execute the following steps [=in parallel=]. If any fatal error is encountered in this algorithm other than the
ones enumerated below, cancel the timer, [=reject=] |promise| with a DOMException whose name is "{{UnknownError}}", and terminate
this algorithm.

1. Use {{attestationChallenge}}, |callerOrigin| and |rpId|, along with the token binding key associated with |callerOrigin| (if
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


1. Initialize |issuedRequests| and |currentlyAvailableAuthenticators| to empty lists.
1. While |issuedRequests| is not empty, perform the following actions depending upon the |adjustedTimeout| timer and responses
from the authenticators:
<dl class="switch">

1. For each authenticator currently available on this platform, add the authenticator to |currentlyAvailableAuthenticators|
unless the {{ScopedCredentialOptions/attachment}} member of {{options}} is <a>present</a>. In that case, let |attachment|
be {{ScopedCredentialOptions/attachment}}, and add the authenticator to |currentlyAvailableAuthenticators| if its attachment
modality matches |attachment|.
<dt>If the |adjustedTimeout| timer expires,</dt>

1. For each authenticator in |currentlyAvailableAuthenticators|: asynchronously invoke the <a>authenticatorMakeCredential</a>
operation on that authenticator with |rpId|, <a>clientDataHash</a>, {{accountInformation}}, |normalizedParameters|,
{{ScopedCredentialOptions/excludeList}} and |clientExtensions| as parameters. Add a corresponding entry to |issuedRequests|.
- For each credential |C| in the {{ScopedCredentialOptions/excludeList}} member of {{options}} that has a non-empty
|transports| list, optionally use only the specified transports to test for the existence of |C|.
<dd>[=set/For each=]
|authenticator| in |issuedRequests| invoke the
<a>authenticatorCancel</a> operation on |authenticator| and
[=set/remove=] |authenticator| from |issuedRequests|.</dd>

1. While |issuedRequests| is not empty, perform the following actions depending upon the |adjustedTimeout| timer and responses
from the authenticators:
- If the |adjustedTimeout| timer expires, then for each entry in |issuedRequests| invoke the <a>authenticatorCancel</a>
operation on that authenticator and remove its entry from the list.
- If any authenticator returns a status indicating that the user cancelled the operation, delete that authenticator's
entry from |issuedRequests|. For each remaining entry in |issuedRequests| invoke the <a>authenticatorCancel</a>
operation on that authenticator and remove its entry from the list.
- If any authenticator returns an error status, delete the corresponding entry from |issuedRequests|.
- If any authenticator indicates success:
- Remove this authenticator's entry from |issuedRequests|.
- Create a new {{ScopedCredentialInfo}} object named |value| and populate its fields with the values returned from the
authenticator as well as the {{ScopedCredentialInfo/clientDataJSON}} computed earlier.
- For each remaining entry in |issuedRequests| invoke the <a>authenticatorCancel</a> operation on that authenticator and
remove its entry from the list.
- Resolve |promise| with |value| and terminate this algorithm.
<dt>If any |authenticator| returns a status indicating that the user cancelled the operation,</dt>

1. Reject |promise| with a {{DOMException}} whose name is "{{NotAllowedError}}", and terminate this algorithm.
<dd>
1. [=set/Remove=] |authenticator| from |issuedRequests|.
2. [=set/For each=] remaining |authenticator| in |issuedRequests| invoke
the <a>authenticatorCancel</a> operation on |authenticator| and
[=set/remove=] it from |issuedRequests|.

</dd>

<dt>If any |authenticator| returns an error status,</dt>

<dd>[=set/Remove=] |authenticator| from |issuedRequests|.</dd>

<dt>If any |authenticator| indicates success,</dt>

<dd>
1. [=set/Remove=] |authenticator| from |issuedRequests|.
2. Let |value| be a new {{ScopedCredentialInfo}} object whose fields are:
: {{ScopedCredentialInfo/clientDataJSON}}
:: A new {{ArrayBuffer}} containing the bytes of |clientDataJSON|.
: {{ScopedCredentialInfo/attestationObject}}
:: A new {{ArrayBuffer}} containing the bytes of the value returned
from the successful [=authenticatorMakeCredential=] operation
3. [=set/For each=] remaining |authenticator| in |issuedRequests| invoke the
<a>authenticatorCancel</a> operation on |authenticator| and
[=set/remove=] it from |issuedRequests|.
4. [=Resolve=] |promise| with |value| and terminate this algorithm.

</dd>

1. [=Reject=] |promise| with a {{DOMException}} whose name is
"{{NotAllowedError}}".

Issue: {{NotAllowedError}} seems incorrect for at least the timeout and
cancelled exit conditions above.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I've filed #376 for this.


During the above process, the user agent SHOULD show some UI to the user to guide them in the process of selecting and
authorizing an authenticator.
Expand Down Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

- For each credential C within the |credentialList| that has a non-empty |transports| list, optionally use only the
- For each credential C within the |credentialList| that has a non-empty {{transports}} list, optionally use only the
specified transports to get assertions using credential C.
- If the above filtering process concludes that none of the credentials on the {{AssertionOptions/allowList}} can possibly
be on this authenticator, do not perform any of the following steps for this authenticator, and proceed to the next
Expand Down Expand Up @@ -2115,7 +2216,7 @@ Note: Extensions should aim to define authenticator arguments that are as small
over low-bandwidth links such as Bluetooth Low-Energy or NFC.


## Extending client processing ## {#extension-client-processing}
## Extending <dfn>client processing</dfn> ## {#extension-client-processing}

Extensions may define additional processing requirements on the client platform during the creation of credentials or the
generation of an assertion. In order for the <a>[RP]</a> to verify the processing took place, or if the processing has a result
Expand Down Expand Up @@ -2932,6 +3033,14 @@ Brad Hill, Jing Jin, Anne van Kesteren, Giridhar Mandyam, Axel Nennker, Yaron Sh
"href": "https://tools.ietf.org/html/draft-greevenbosch-appsawg-cbor-cddl",
"status": "Internet Draft (work in progress)",
"date": "21 September 2016"
},

"TokenBinding": {
"authors": ["A. Popov", "M. Nystroem", "D. Balfanz", "A. Langley", "J. Hodges"],
"title": "The Token Binding Protocol Version 1.0",
"href": "https://tools.ietf.org/html/draft-ietf-tokbind-protocol-13",
"status": "Internet-Draft",
"date": "February 16, 2017"
}
}
</pre>