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

credential ID returned by authenticatorGetAssertion() is optional if allowList has exactly one member #472

Closed
equalsJeffH opened this issue May 23, 2017 · 13 comments · Fixed by #498

Comments

@equalsJeffH
Copy link
Contributor

The authenticatorGetAssertion operation says:

On successful completion, the authenticator returns to the user agent:
* The identifier of the credential used to generate the signature.
* The authenticator data used to generate the signature.
* The assertion signature.

However, the CTAP spec says in 4.2 authenticatorGetAssertion:

On success, the authenticator must return the following structure in its response:

Member name  Data type   Required?  Definition
credential   Credential  Optional   Credential whose private key was used to 
                                     generate the assertion. May be
                                     omitted if the whitelist has exactly 
                                     one Credential.
[...]

..which would seem to be an optimization for CTAP where it does not have to return as many bytes (in what may be a common case).

Update WebAuthn to reflect this?

@equalsJeffH
Copy link
Contributor Author

Hm, if the authnr's returning of Credential ID is optional in the case where the webauthn client invokes authenticatorGetAssertion() with exactly one credential descriptor in allowCredentials[], then in this case the client needs to maintain state -- the Cred ID -- until the call to authenticatorGetAssertion() returns and then set {{PublicKeyCredential/[[identifier]]}} to the saved value, yes? And that saved value needs to be in some global-to-the-window object (or roughly equivalent) I'm guessing?

Is this a case of "snapshotting" ?

I took a stab at doing this in 31a8f85

/cc @bzbarsky @jyasskin @jcjones

@bzbarsky
Copy link

Please re-cc me if there's a specific question for me.

@equalsJeffH
Copy link
Contributor Author

@bzbarsky yes, the above questions are for you if you have time to consider them (the other guys are out-of-pocket for a while). thanks.

@bzbarsky
Copy link

I guess I just don't understand the questions then. They seem to be about details of authn that I'm not familiar with, not about the general platform integration aspects....

@equalsJeffH
Copy link
Contributor Author

equalsJeffH commented Jun 19, 2017

The questions are about how to save state (a particular value) from before the invocation of an async operation, until the async operation returns, and then combine the saved value with the async op's returned values. abstracted out, it's like this:

if a given sequence |A| has exactly one value, then let |S| be a new ArrayBuffer, created using |global|'s %ArrayBuffer%, and containing the bytes of |A|[0].
[..invoke async op.....op returns..]
Create a new ArrayBuffer, using global|'s %ArrayBuffer%. If |S| exists, set the value of the new ArrayBuffer to be the bytes of |S|. Otherwise, set the value of the new ArrayBuffer to be the bytes of the fooBar returned from the op. [...]

Is the above more or less correctly specified, or not? thanks for your help.

@bzbarsky
Copy link

So basically you want to pass some sort of data through the async op? I'm not sure what the best current spec language for that is. @domenic ?

@domenic
Copy link
Contributor

domenic commented Jun 19, 2017

I'm having a hard time understanding the above, and I tried reading 31a8f85, but without context it's not very intelligible to me. At first glance those sentences seem OK-ish, although generally you want to be more explicit about going in-parallel and then posting a task to get back to the main thread. But it seems like you're taking care to not create or manipulate main-thread objects during your off-main-thread async work, so that's good at least.

@equalsJeffH
Copy link
Contributor Author

So basically you want to pass some sort of data through the async op?

Well, not "through" the async op. Rather, want to remember something "globally" (on the "main thread" ?) "outside" the async op.

So, thanks guys -- I take that as meaning that at a first-order approximation the new language is OK enough for now.

wrt..

generally you want to be more explicit about going in-parallel and then posting a task to get back to the main thread.

might you be able to point to a spec that does the above, that we might learn from?

thanks again.

@domenic
Copy link
Contributor

domenic commented Jun 19, 2017

@equalsJeffH
Copy link
Contributor Author

Ok thanks. Is not "posting a task to get back to the main thread" part of inherent Promise machinery? In the case of the alg discussed here, it is called-into by navigator.credentials.get() wherein the Promise resolution/rejection is handled.

@domenic
Copy link
Contributor

domenic commented Jun 20, 2017

If the only thing you do back on the main thread is resolve/reject a promise with a pre-existing value (usually undefined), then we have defined them to post a task. (Although that is somewhat controversial/buggy; see w3ctag/promises-guide#52.)

But if you do anything else back on the main thread, such as creating an object, you need to post a task to do that.

Now that I've been linked to https://w3c.github.io/webauthn/#getAssertion, I see a few problems:

  • "Let global be the PublicKeyCredential's interface object's environment settings object’s global object." should probably just be "Let global be the PublicKeyCredential's relevant global object". Sorry this stuff is so convoluted.
  • Step 17 states a task source but it's going in parallel; it's not posting any tasks. Instead step 18 "If any authenticator indicates success" should be posting the task (and stating the task source) since it needs to go back to the main thread to create the object.
  • The algorithm is theoretically synchronous as stated at the top, but it goes in parallel. But it appears we're already in parallel (i.e. on the background thread) when it's called, by https://w3c.github.io/webappsec-credential-management/#abstract-opdef-request-a-credential step 5.9? It seems like the separation here is not working that well.

I think I would advise:

  • The algorithm stays synchronous
  • Anywhere it says "in parallel" gets removed. It's doing all its work on the background thread already.
  • It never returns actual JS objects or messes with actual JS globals. Instead it returns the necessary data to create one. (Perhaps as an Infra struct.)
    • This also needs to be done for the exceptions, actually.
  • The "request a credential" operation, after synchronously retrieving the error code or data necessary to create a credential, posts a task (going "back to the main thread") to actually do the object/exception creation and resolve/reject the promise.

You can see some pretty similar spec-code in WebAssembly/design#1093 (preview). That PR's processing/success/failure steps are probably not quite necessary in your case, so no need to emulate that somewhat inside-out structure, but hopefully it conveys the way in which you weave between threads.

Hope this helps... I do feel every time I end up in this repository I see you tackling with some of the hardest issues in spec-writing, so my heart goes out to you.

@equalsJeffH
Copy link
Contributor Author

Thanks for the review & guidance @domenic !

@domenic noted in irc #whatwg that when he says "post a task", he means "[=queue a task=]"

@domenic wrote:

  • The algorithm stays synchronous
  • Anywhere it says "in parallel" gets removed. It's doing all its work on the background thread already.

Ok.

It never returns actual JS objects or messes with actual JS globals. Instead it returns the necessary data to create one. (Perhaps as an Infra struct.)

Ok, will look to WebAssembly/design#1093 (preview) for guidance.

  • The "request a credential" operation,

ah, you are referring to webappsec-credential-management/#abstract-opdef-request-a-credential here it seems.

after synchronously retrieving the error code or data necessary to create a credential, [queues] a task (going "back to the main thread") to actually do the object/exception creation and resolve/reject the promise.

ok, though IIUC, webappsec-credential-management/#abstract-opdef-request-a-credential does not itself presently "queue a task...to actually do the object/exception creation and resolve/reject the promise", yes?

I am thinking that all the "object/exception creation and resolve/reject the promise" functionality could be specified in the webauthn spec in a fashion similar to WebAssembly PR #1093's approach.

WDYT?

/cc @mikewest

@equalsJeffH
Copy link
Contributor Author

equalsJeffH commented Jun 27, 2017

see also issue #254 "There is no "current settings object" in algorithm steps that are executing in parallel" for additional context wrt what's at issue here. see especially #254 (comment)

equalsJeffH pushed a commit that referenced this issue Aug 25, 2017
equalsJeffH pushed a commit that referenced this issue Sep 29, 2017
equalsJeffH pushed a commit that referenced this issue Sep 29, 2017
equalsJeffH pushed a commit that referenced this issue Sep 29, 2017
equalsJeffH added a commit that referenced this issue Oct 19, 2017
* do not call authenticatorMakeCredential() with separate |rpId| fixes #466

* credID returned by authnrGetAssn() is optional if allowCreds has exactly 1 member fixes #472

* fixup global object reference per domenic, improves #472

* indent 4.1.4 step 18et al to clarify relation to prior step

* fix line indent

* do not call authenticatorMakeCredential() with separate |rpId| fixes #466

* credID returned by authnrGetAssn() is optional if allowCreds has exactly 1 member fixes #472

* fixup global object reference per domenic, improves #472

* indent 4.1.4 step 18et al to clarify relation to prior step

* fix line indent

* post rebase-on-master, fix dangling MakeCredentialOptions

* fix error in resolving rebase conflicts

* further rebase conflict resolution error fixups

* convert switch steps to colon-denotation

* tag 'while'

* primary changes for improving #472 mostly complete

* further issue #472 cleanups

* del 'cancel the timer' from #creatCredential fixes #535

* polish constructResultantCredentialCallback method description

* incorp comments from mikewest at webappsec-credential-management/pull/100

* rebased onto master

* credID returned by authnrGetAssn() is optional if allowCreds has exactly 1 member fixes #472

* fixup global object reference per domenic, improves #472

* indent 4.1.4 step 18et al to clarify relation to prior step

* fix line indent

* do not call authenticatorMakeCredential() with separate |rpId| fixes #466

* credID returned by authnrGetAssn() is optional if allowCreds has exactly 1 member fixes #472

* post rebase-on-master, fix dangling MakeCredentialOptions

* fix error in resolving rebase conflicts

* convert switch steps to colon-denotation

* tag 'while'

* primary changes for improving #472 mostly complete

* further issue #472 cleanups

* polish constructResultantCredentialCallback method description

* incorp comments from mikewest at webappsec-credential-management/pull/100

* fix indents make BS happy, add some periods

* fix code tags placement

* correct bugs in prior merge conflict resolution, doh

* rm 'the bytes of'

* add missing @@EDITOR-ANCHOR-01A

* auto-number some steps

* re- fix #466 (due to merge-from-master), fix #536

* eliminate callback and just return an algorithm from #createCredential

* continue fix conflicts from merge from master

* fix a couple of issue #466 stragglers in #op-make-cred

* revert to prior AuthenticationExtensions language per jyasskin

* add inline spec issue pointing to issue #657

* minor cleanups, remove issue wrt not explicitly returning |credentialCreationData|

* fix annoying bikeshed warning wrt 'rpEntity'

* correctly fix warning as well as other incorrect markup
equalsJeffH pushed a commit that referenced this issue Nov 8, 2017
* do not call authenticatorMakeCredential() with separate |rpId| fixes #466

* credID returned by authnrGetAssn() is optional if allowCreds has exactly 1 member fixes #472

* fixup global object reference per domenic, improves #472

* indent 4.1.4 step 18et al to clarify relation to prior step

* fix line indent

* do not call authenticatorMakeCredential() with separate |rpId| fixes #466

* credID returned by authnrGetAssn() is optional if allowCreds has exactly 1 member fixes #472

* fixup global object reference per domenic, improves #472

* indent 4.1.4 step 18et al to clarify relation to prior step

* fix line indent

* post rebase-on-master, fix dangling MakeCredentialOptions

* fix error in resolving rebase conflicts

* further rebase conflict resolution error fixups

* convert switch steps to colon-denotation

* tag 'while'

* primary changes for improving #472 mostly complete

* further issue #472 cleanups

* del 'cancel the timer' from #creatCredential fixes #535

* polish constructResultantCredentialCallback method description

* marked authenticator model section as non-normative

* marked relying party operation section as non-normative

* fix proper subset tweak

* Added abort signal object and steps to webauthn

* fixed a minor issue with linking

* add minor edits to focus on the main things

* getting the blank line correct

* Added a example section to explain how abort should be used

* fix up example

* committing before computer dies

* updated grammars of the example based on feedback

* update example text

* Updated with the section on switching tab; complete the PR

* minor tweak

* finished polishing the spec

* whoops one leftover

* finally figured out how to remove last two linking errors

* take out abortsignal from extension; edit promise rejection
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants