fixup getAssertion, polish algorithms by equalsJeffH · Pull Request #371 · w3c/webauthn

equalsJeffH · 2017-03-04T01:55:56Z

This initial set of commits applies @jyasskin's techniques from makeCredential() to getAssertion().

More commits to follow (to fix various [subtype:algorithms] issues I've signed up for), tho review of these initial commits is most welcome in the meantime.

vijaybh · 2017-03-06T06:09:37Z

-    - Asynchronously invoke the [=authenticatorGetAssertion=] operation on this authenticator with |rpId|,
-        |clientDataHash|, |credentialList|, and |clientExtensions| as parameters.
-    - Add an entry to |issuedRequests|, corresponding to this request.
+    1. [=list/For each=] credential |C| in <code>{{options}}.{{AssertionOptions/allowList}}</code> that is present on this 


How can the client tell if a credential is on a particular authenticator? This is only possible in certain circumstances. Maybe we should flip this. Something like "For each credential in allowList, append to credentialList. Some clients may be able to determine, by client-specific methods, the presence or absence of credentials on certain authenticators. In this case, the client MAY skip appending those credentials which it knows to not be on the authenticator."

Thoughts?

thx, yeah, i trimmed too much out of the prior step 8, have attempted to backfit things, pls review new commit(s).

vijaybh · 2017-03-06T06:11:43Z

+    1. If |credentialList| [=list/is empty=] then [=continue=].
+
+    1. [=In parallel=], [=list/for each=] credential |C| in |credentialList|:
+        1. If |C| has a non-empty {{transports}} list, the client MAY, [=list/for each=] |transport| in {{transports}}, 


This means that if a credential has say "USB, BTLE, NFC", then it will get invoked three times, once on each transport? This seems like overkill. We could instead say the client SHOULD select a transport from that list, and otherwise it can pick any one transport to contact the authenticator over.

I have attempted to state this, assuming I understood what you meant by the "otherwise" clause. pls review new commit(s).

vijaybh · 2017-03-06T06:15:23Z

+                1. {{ScopedCredential/type}} whose value is the {{ScopedCredentialType}} representing this [=scoped 
+                    credential=]'s type.
+                1. {{ScopedCredential/id}} whose value is a new {{ArrayBuffer}} containing the bytes of the crendential ID 
+                    returned from the successful [=authenticatorMakeCredential=] operation.


You mean authenticatorGetAssertion? This also returns a credential ID.

vijaybh · 2017-03-06T06:15:36Z

+            :: A new {{ScopedCredential}} object whose fields are:
+                1. {{ScopedCredential/type}} whose value is the {{ScopedCredentialType}} representing this [=scoped 
+                    credential=]'s type.
+                1. {{ScopedCredential/id}} whose value is a new {{ArrayBuffer}} containing the bytes of the crendential ID 


Typo - "crendential".

vijaybh · 2017-03-06T06:43:21Z

We should also check back to see if the following are addressed by this PR and the previous revisions to makeCredential: #251, #253, #254, #255, #258, #276, #279, #280, #281 and #282.

vijaybh · 2017-03-07T23:56:33Z

Noticed that you converted a few authenticator functions to <a></a> markup from [= =]. Not sure if this is intentional. Other than that. LGTM.

equalsJeffH · 2017-03-08T00:22:04Z

Noticed that you converted a few authenticator functions to <a></a> markup from [= =]. Not sure if this is intentional.

accident-when-resolving-conflicts :-/ thx.

Other than that. LGTM.

k, thx.

we should also check back to see if the following are addressed by this PR and the previous revisions to makeCredential: #251, #253, #254, #255, #258, #276, #279, #280, #281 and #282.

Yeah, what I meant by..

More commits to follow (to fix various [subtype:algorithms] issues I've signed up for)

..is the former, including fixing them in this branch and PR if they are not already fixed -- that ok ?

vijaybh · 2017-03-08T06:36:13Z

Works for me. LMK when you think we should merge this branch (for the current changes it would be nice to get Jeffrey or JC to review as well).

equalsJeffH · 2017-03-08T15:54:41Z

for the current changes it would be nice to get Jeffrey or JC to review as well

Agreed -- @jcjones is already in the reviewers list -- I tried to add @jyasskin but he does not appear in the drop-down menu, tho he is now listed as a wg member -- perhaps he lacks "write perms" to the repo and that is why?

…ion fixups

vijaybh · 2017-03-08T18:15:20Z

    challenge value (e.g., by using
    <a href="https://developer.android.com/reference/android/security/keystore/KeyGenParameterSpec.Builder.html#setAttestationChallenge(byte%5B%5D)">
-    setAttestationChallenge</a>),
+    setAttestationChallenge=]),


This seems bad

yeah -- bikeshed barfed on me, i will fix..

should be fixed as of commit eee29f9

jcjones · 2017-03-08T21:07:02Z

I'll finish my review of this this afternoon.

equalsJeffH · 2017-03-08T22:09:48Z

note that I'm still pushing commits to this branch -- will signal when I'm done doing so (in the next day or two)

jcjones · 2017-03-08T23:08:33Z

I've gone in detail through the algorithm updates, comparing with my current implementation even - it looks really good, it's a lot clearer with the switch notation since all of that is operating in parallel. I don't see anything in this rev that cries out for changing.

vijaybh · 2017-03-10T19:14:10Z

-    them. By default, the RP ID for a WebAuthn operation is set to the [=current settings object=]'s
-    [=origin=]. This default can be overridden by the caller subject to certain restrictions, as specified in
+    them. By default, the RP ID for a WebAuthn operation is set to the [=relevant settings object=] of the {{WebAuthentication}}
+    object's [=origin=]. This default can be overridden by the caller subject to certain restrictions, as specified in


The RP ID is set to the relevant settings object? Or did you mean the RP ID is set to the origin specified by the WebAuthentication object's relevant settings object?

I suspect you mean (relevant settings object of the Webauthentication object)'s origin, but I don't think most readers would put the parentheses there :)

yes, the latter.

vijaybh · 2017-03-10T19:15:13Z

-    return [=a promise rejected with=] a {{DOMException}} whose name is "{{NotAllowedError}}", and terminate this algorithm.
+1. Set |global| to this {{WebAuthentication}} object's [=global object|environment settings object's global object=].
+
+1. Set |callerOrigin| to the [=relevant settings object=] of this {{WebAuthentication}} object's [=origin=]. If |callerOrigin| 


See above. I think this should be "origin specified by the WebAuthentication object's relevant settings object".

doh. thanks, done.

vijaybh · 2017-03-10T19:16:47Z


    - The <dfn>rpId</dfn> parameter explicitly specifies the RP ID that the credential should be associated with. If it is
-        omitted, the RP ID will be set to the [=current settings object=]'s [=origin=].
+        omitted, the RP ID will be set to the [=relevant settings object=] of the {{WebAuthentication}} object's [=origin=].


See previous comment on this construct.

vijaybh · 2017-03-10T19:17:12Z


    - The optional <dfn>rpId</dfn> parameter specifies the rpId claimed by the caller. If it is omitted, it will be assumed to
-        be equal to the [=current settings object=]'s [=origin=].
+        be equal to the [=relevant settings object=] of the {{WebAuthentication}} object's [=origin=].


Another one.

vijaybh · 2017-03-11T06:04:05Z

LGTM. @jcjones, would you also take a look and we can merge if you're happy with the changes?

equalsJeffH · 2017-03-13T16:08:19Z

this PR was accidentally closed, reopening...

equalsJeffH · 2017-03-13T17:22:33Z

@jcjones @vijaybh - pls note that commit c49cb0f attempts resolution of #277 and is presently only applied to makeCredential(). can apply to getAssertion() as well if it looks reasonable. I poked through JC's firefox code to try to assess but am unable to so assess as yet due to unfamiliarity with FF guts. ultimately, @jyasskin @bzbarsky should probably also review.

jcjones

I don't see any blocking issues here, and it's a great cleanup. My comments are all style nits, feel free to address them or not as you see fit!

r=me

jcjones · 2017-03-14T00:13:28Z


 1. Let |promise| be [=a new promise=]. Return |promise| and start a timer for |adjustedTimeout| milliseconds. Then execute the
-    following steps [=in parallel=].
+    following steps [=in parallel=]. The [=task source=] for these [=tasks=] is the [=dom manipulation task source=].


It seems to mean to me that if the user has many types of authenticators available, this could starve the dom manipulation task thread pool. Probably not a big deal, but may be an implementation quirk to handle.

ok. let's leave it as =dom manipulation task source=] for now -- most all users will likely have only one or maybe two authenticators (we are surmizing) -- and we can use a different task source or invent a new task source if needed as @bzbarsky noted in #277 (comment)

jcjones · 2017-03-14T00:15:19Z

+    :: The [=recognized algorithm name=] of the hash algorithm selected by the client for generating the
+        [=hash of the serialized client data=]
+    : {{tokenBinding}}
+    :: The [=Token Binding ID=] associated with |callerOrigin| (if any)


Is it clear what encoding should be done here if there's no token binding ID?

tokenBinding is an optional member of the ClientData dictionary, so the implication here is it would not appear in the dictionary if a Token Binding ID is unanavailable, thus the (if any) qualification. Perhaps we can make that more clear.

ok, I attempted modest clarification in 1c84e8b

jcjones · 2017-03-14T00:25:29Z

+    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=].


Should this line end in a colon, then, since it's sort-of starting another block?

By that logic, maybe we should up the indentation level, too?

hm. I am sympathetic, tho @jyasskin did not do such in makeCredential(), so I'm inclined to leave it as-is for now.

jcjones · 2017-03-14T00:30:58Z

Oh, and since I just hit "submit" too early - the explicit global you've added in c49cb0f for ScopedCredentialInfo looks correct to me, in that we don't need to worry then about ScopedCredentialInfo outliving the WebAuthn call. I'm pretty sure it's right, based on my previous conversations about what to do there.

equalsJeffH · 2017-03-14T00:33:51Z

@jcjones said:

the explicit global you've added in c49cb0f for ScopedCredentialInfo looks correct to me, in that we don't need to worry then about ScopedCredentialInfo outliving the WebAuthn call. I'm pretty sure it's right, based on my previous conversations about what to do there.

Ok, if it's the correct global, then I wonder about the prose used to state its use, hence asking for @jyasskin & @bzbarsky input... :)

jcjones · 2017-03-14T15:45:42Z

LGTM. If we're wrong about the source of the global object, I figure that's a minor change on top of this much more substantive one.

JeffH added 2 commits March 3, 2017 17:34

make getAssertion() more precise a la makeCredential

1729597

merge from master, fix conflicts

afec72d

equalsJeffH added subtype:algorithms/WebIDL type:technical labels Mar 4, 2017

equalsJeffH added this to the WD-05 milestone Mar 4, 2017

equalsJeffH self-assigned this Mar 4, 2017

equalsJeffH requested review from jcjones and vijaybh March 4, 2017 01:55

vijaybh reviewed Mar 6, 2017

View reviewed changes

fixes per vijaybh's review.

43fd00b

vijaybh approved these changes Mar 7, 2017

View reviewed changes

'was specd'=>'present/not present' fixes #251. + markup canonicalizat…

9aaa4a9

…ion fixups

vijaybh reviewed Mar 8, 2017

View reviewed changes

JeffH added 2 commits March 8, 2017 11:38

fixes some of the prior 'fixes' (haste made waste)

eee29f9

fixes #267 wrt (not) empty lists

90e0d0f

fix #271

b1a9ce7

equalsJeffH mentioned this pull request Mar 8, 2017

ScopedCredentialDescriptor can contain BufferSources that are processed async #276

Closed

JeffH added 2 commits March 8, 2017 13:53

fix another 'empty' thing

4343b7d

Merge branch 'master' into jeffh-fixup-getA-polish-algs

847bc09

AngeloKai mentioned this pull request Mar 8, 2017

Throw NotFoundError when no authenticator is available #350

Merged

JeffH added 2 commits March 9, 2017 11:03

update acks

b892756

attempt resolution of #277

c49cb0f

vijaybh reviewed Mar 10, 2017

View reviewed changes

fixing relevant origin bugs, thx vijaybh!

bf6d618

AngeloKai closed this Mar 13, 2017

AngeloKai deleted the jeffh-fixup-getA-polish-algs branch March 13, 2017 13:53

equalsJeffH restored the jeffh-fixup-getA-polish-algs branch March 13, 2017 15:15

equalsJeffH reopened this Mar 13, 2017

jcjones approved these changes Mar 14, 2017

View reviewed changes

JeffH added 2 commits March 14, 2017 08:26

polish token binding occurances

1c84e8b

apply global and task src to getA() fixes #277

e200156

vijaybh merged commit 985f88e into master Mar 14, 2017

vijaybh deleted the jeffh-fixup-getA-polish-algs branch March 14, 2017 16:22

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

Built by Travis-CI: 985f88e fixup getAssertion, polish algorithms (#371)

e272170

This was referenced Mar 14, 2017

Please ensure that all ArrayBuffer creations are done explicitly, in the correct global #293

Closed

"might be present on this authenticator" could use a clearer definition #280

Closed

equalsJeffH mentioned this pull request Oct 16, 2017

fixup algs contd 3 #498

Merged

Conversation

equalsJeffH commented Mar 4, 2017

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!

Choose a reason for hiding this comment

Uh oh!

vijaybh commented Mar 6, 2017

Uh oh!

vijaybh commented Mar 7, 2017

Uh oh!

equalsJeffH commented Mar 8, 2017

Uh oh!

vijaybh commented Mar 8, 2017

Uh oh!

equalsJeffH commented Mar 8, 2017

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!

jcjones commented Mar 8, 2017

Uh oh!

equalsJeffH commented Mar 8, 2017

Uh oh!

jcjones commented Mar 8, 2017

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vijaybh commented Mar 11, 2017

Uh oh!

equalsJeffH commented Mar 13, 2017

Uh oh!

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

Uh oh!

Uh oh!

jcjones 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

equalsJeffH commented Mar 13, 2017 •

edited

Loading