Closed Bug 1309284 Opened 8 years ago Closed 7 years ago

Implementing the WebAuthn JS API

Categories

(Core :: DOM: Device Interfaces, enhancement, P2)

Product:
Core
Component:
DOM: Device Interfaces
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: jcj, Assigned: jcj)

References

(Blocks 1 open bug,
URL
)

Details

Attachments

(3 files, 2 obsolete files)

Bug 1309284 - Implement W3C Web Authentication JS API [part 1]
58 bytes, text/x-review-board-request
keeler
: review+
qdot
: review+
Details
Bug 1309284 - WebAuthn JS API [part 2]: Bugfixes from testing
58 bytes, text/x-review-board-request
keeler
: review+
Details
Bug 1309284 - WebAuthn JS API [part 3]: Support origin relax algorithm
58 bytes, text/x-review-board-request
keeler
: review+
Details 
This bug is to implement the JS API defined for Web Authentication by the WebAuthn WG at W3C: https://www.w3.org/TR/webauthn/ .
Assignee: nobody → jjones
Blocks: webauthn
URL: https://www.w3.org/TR/webauthn/
Severity: normal → enhancement
Status: NEW → ASSIGNED
OS: Unspecified → All
Priority: -- → P2
Hardware: Unspecified → All
Comment on attachment 8816308 [details]
Bug 1309284 - Implement W3C Web Authentication JS API [part 1]

Kyle, David:

This is ~1,200 lines of code as a starting point to implementing W3C Web Authentication. It has a list of TODOs in it, most notably that there's no tests (yet). Given the size, I wanted to get this in front of you (and others Chrome/Edge) in an preview form to get early feedback.

Do check the commit message, but basically:

1) Is this interaction with the WebIDL OK? I'm planning, per :qdot's suggestion, to migrate the nsIU2FToken API to be async and use callbacks, so this code was designed to assume that would happen soon (tm), and for now it all happens on one thread -- but uses promises/mozpromises where relevant.

2) Is the use of nsICryptoHash correct? It seems to produce correct values, but hey.

3) Kyle - the spec for Navigator.Authentication _doesn't_ call for it to be able to [Throws], but I added that because I'm not sure how else to handle conditions where we fail to initialize in WebAuthentication::Init(). I guess it could return a valid object with mInitialized=false, so further usage returns promises that error... but that seems... late? I don't know if this is a spec issue actually, or a PEBKAC on my part.

Otherwise, I'm going to point some other WebAuthn contributors to this to look at the weird way I've mashed U2F and WebAuthn's types together, so we can hopefully come to a conclusion of what the right thing to do is.

Thanks!
Attachment #8816308 - Flags: feedback?(kyle)
Attachment #8816308 - Flags: feedback?(dkeeler)
(In reply to J.C. Jones [:jcj] from comment #3)
> Comment on attachment 8816308 [details]
> Bug 1309284 - [WIP] Implement W3C Web Authentication JS API f?keeler f?qdot

> 3) Kyle - the spec for Navigator.Authentication _doesn't_ call for it to be
> able to [Throws], but I added that because I'm not sure how else to handle
> conditions where we fail to initialize in WebAuthentication::Init(). I guess
> it could return a valid object with mInitialized=false, so further usage
> returns promises that error... but that seems... late? I don't know if this
> is a spec issue actually, or a PEBKAC on my part.

Why do we need an init function? At the point of getting the WebAuth object on navigator, we shouldn't be doing anything other than exposing an interface object. Nothing should fail until as late as possible, meaning a call to makeCredential/getAssertion/etc, where you have a promise to fail already.
Comment on attachment 8816308 [details]
Bug 1309284 - Implement W3C Web Authentication JS API [part 1]

https://reviewboard.mozilla.org/r/97088/#review97874

Things look generally ok for the WebIDL boilerplate, but see other comment about init function. We need to figure out what the IPC and parent side of things are going to look like before we can really get going on this, because things like the U2F functions in WebAuthentication.cpp have a bunch of calls that will be async over IPC (token operations like register/sign, etc).

::: dom/base/Navigator.cpp:2200
(Diff revision 2)
>  
> +WebAuthentication*
> +Navigator::GetAuthentication(ErrorResult& aRv)
> +{
> +  if (!mAuthentication) {
> +    ErrorResult error;

Not used?

::: dom/u2f/WebAuthentication.cpp:290
(Diff revision 2)
> +// to produce the result of the WebAuthn MakeCredential method. The exact
> +// mapping of U2F data fields to WebAuthn data fields is still a matter of
> +// ongoing discussion, and this should not be taken as anything but a point-in-
> +// time possibility.
> +void
> +WebAuthentication::U2FAuthMakeCredential(

Where will cached registration data be held? Will we have access to it in the child process, or will this be held in the parent process, in which case we'll need to build that into the IPC protocols?

::: dom/u2f/WebAuthentication.cpp:291
(Diff revision 2)
> +// mapping of U2F data fields to WebAuthn data fields is still a matter of
> +// ongoing discussion, and this should not be taken as anything but a point-in-
> +// time possibility.
> +void
> +WebAuthentication::U2FAuthMakeCredential(
> +             const RefPtr<CredentialRequest>& aRequest,

Indentation?
Attachment #8816308 - Flags: review-
Comment on attachment 8816308 [details]
Bug 1309284 - Implement W3C Web Authentication JS API [part 1]

https://reviewboard.mozilla.org/r/97088/#review97874

Thanks for the review, :qdot! I've reworked the initialization as you suggested. I've also been hacking on tests on-and-off for the workweek, but those are still to come.

I'll upload a diff that changes the Init behavior. See below for my comments on the indentation (and maybe a to-do for clang-format) and caching.

> Not used?

Yeah, good catch. That said, this code changes a lot without the [Throws] and by making init lazy, so this gets fixed by disappearing. :)

> Where will cached registration data be held? Will we have access to it in the child process, or will this be held in the parent process, in which case we'll need to build that into the IPC protocols?

I'm not sure what you mean; there's no registration data caching necessary, as state is kept in the aAllowList used for GetAssertion().

So, I don't think there's any need for state at all, not at this level.

> Indentation?

This is matching the rest of the module, and parts of PSM, that shift-left to constrain to the 80-char limit. :franziskus suggested I should just adopt clang-format for the whole module, which I might do as a follow-on.
Comment on attachment 8816308 [details]
Bug 1309284 - Implement W3C Web Authentication JS API [part 1]

https://reviewboard.mozilla.org/r/97088/#review98718

The use of the pkix::Input/Reader looks fine (maybe a little inefficient, although hopefully a sufficiently advanced compiler will optimize that to a memcpy). I didn't do a full review of the actual algorithm/functionality this will eventually provide, but things look good in general.

::: dom/u2f/ScopedCredential.h:23
(Diff revision 3)
> +
> +namespace mozilla {
> +namespace dom {
> +
> +class ScopedCredential final : public nsISupports,
> +                               public nsWrapperCache

style nit: ',' on this line under the ':'

::: dom/u2f/WebAuthentication.cpp:57
(Diff revision 3)
> +            CryptoBuffer& aOut)
> +{
> +  MOZ_ASSERT(aHashService);
> +
> +  nsresult rv = aHashService->Init(nsICryptoHash::SHA256);
> +  if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }

Technically the style would be to have the return on its own line.

::: dom/u2f/WebAuthentication.cpp:64
(Diff revision 3)
> +  rv = aHashService->Update(
> +         reinterpret_cast<const uint8_t*>(aIn.BeginReading()),aIn.Length());
> +  if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> +
> +  nsAutoCString fullHash;
> +  rv = aHashService->Finish(false /* based64 result */, fullHash);

This illustrates why boolean arguments are often not great. To me, this comment is unclear: is it saying we will get a base64 result, or just that the boolean parameter controls whether or not we get a base64 result, meaning that we won't in this case since it's false? Let's just have a more fleshed-out comment like "Passing false here means we will get a binary result rather than a base64-encoded result." or something.

::: dom/u2f/WebAuthentication.cpp:119
(Diff revision 3)
> +
> +  return NS_OK;
> +}
> +
> +static nsresult
> +ReadToCryptoBuffer(pkix::Reader& aSrc, CryptoBuffer& aDest, uint32_t aLen)

Maybe truncate aDest first?

::: dom/u2f/WebAuthentication.cpp:162
(Diff revision 3)
> +  return NS_OK;
> +}
> +
> +static nsresult
> +U2FDecomposeRegistrationResponse(const CryptoBuffer& aResponse,
> +                                 CryptoBuffer& aPubKeyBuf,

These are all out parameters, right? It's nice to annotate them as /*out*/ in that case.

::: dom/u2f/WebAuthentication.cpp:184
(Diff revision 3)
> +  u2fResponse.Init(aResponse.Elements(), aResponse.Length());
> +
> +  pkix::Reader input(u2fResponse);
> +
> +  uint8_t b;
> +  if (NS_WARN_IF(input.Read(b) != pkix::Success)) {

aResponse is untrusted data, right? In that case, it doesn't make sense to NS_WARN_IF any of these operations fail, because we could have been fed malformed input.
Comment on attachment 8816308 [details]
Bug 1309284 - Implement W3C Web Authentication JS API [part 1]

https://reviewboard.mozilla.org/r/97088/#review99986

::: dom/u2f/WebAuthentication.cpp:221
(Diff revisions 3 - 4)
>    }
>  
>    // We have to parse the ASN.1 SEQUENCE on the outside to determine the cert's
>    // length.
>    pkix::Input cert;
> -  if (NS_WARN_IF(pkix::der::ExpectTagAndGetValue(
> +  if (pkix::der::ExpectTagAndGetValue(input, pkix::der::SEQUENCE, cert) !=

nit: I think it's common to put the '!= pkix::Success' on the next line (indented two spaces)

::: dom/u2f/WebAuthentication.cpp:366
(Diff revision 4)
> +    const Sequence<ScopedCredentialDescriptor>& list = aExcludeList.Value();
> +
> +    for (const ScopedCredentialDescriptor& scd : list) {
> +      bool isRegistered = false;
> +
> +      uint8_t *data;

Is there a type we can use here that identifies this as non-owning-don't-free-this-data? Short of that, we should at least add a comment that this is the case (ScopedCredentialGetData could also be documented).

::: dom/u2f/WebAuthentication.cpp:474
(Diff revision 4)
> +
> +  MOZ_ASSERT(buffer);
> +  CryptoBuffer signatureData;
> +  if (NS_WARN_IF(!signatureData.Assign(buffer, bufferlen))) {
> +    free(buffer);
> +    aRequest->SetFailure(NS_ERROR_DOM_UNKNOWN_ERR);

Out of memory error?

::: dom/u2f/WebAuthentication.cpp:587
(Diff revision 4)
> +
> +    MOZ_ASSERT(buffer);
> +    CryptoBuffer signatureData;
> +    if (NS_WARN_IF(!signatureData.Assign(buffer, bufferlen))) {
> +      free(buffer);
> +      aRequest->SetFailure(NS_ERROR_DOM_UNKNOWN_ERR);

Out of memory?

::: dom/u2f/WebAuthentication.cpp:646
(Diff revision 4)
> +                  const ArrayBufferViewOrArrayBuffer& aChallenge,
> +                  const ScopedCredentialOptions& aOptions)
> +{
> +  MOZ_ASSERT(mParent);
> +  nsPIDOMWindowInner* window = GetParentObject();
> +  MOZ_ASSERT(window);

Remember that MOZ_ASSERT is disabled on all builds we actually ship (nightly through release), so if things like this could fail, we need to add a real null check.

::: dom/u2f/WebAuthentication.cpp:680
(Diff revision 4)
> +
> +  if (mOrigin.EqualsLiteral("null")) {
> +    // 4.1.1.3 If callerOrigin is an opaque origin, reject promise with a
> +    // DOMException whose name is "NotAllowedError", and terminate this
> +    // algorithm
> +    MOZ_LOG(gWebauthLog, LogLevel::Debug,("opaque origin"));

nit: space before second '('

::: dom/u2f/WebAuthentication.cpp:810
(Diff revision 4)
> +  // 4.1.1.10 For each authenticator currently available on this platform:
> +  // asynchronously invoke the authenticatorMakeCredential operation on that
> +  // authenticator with rpIdHash, clientDataHash, accountInformation,
> +  // normalizedParameters, excludeList and clientExtensions as parameters. Add a
> +  // corresponding entry to issuedRequests.
> +  for (size_t a = 0; a < mAuthenticators.Length(); ++a) {

For loops where we don't need the index, it might be nice to use the c++11 range-based for loop.
Attachment #8816308 - Flags: review?(dkeeler) → review+
Comment on attachment 8819356 [details]
Bug 1309284 - WebAuthn JS API [part 2]: Bugfixes from testing

https://reviewboard.mozilla.org/r/98940/#review100008

Might as well just fold this in with part 1.
Attachment #8819356 - Flags: review?(dkeeler) → review+
Comment on attachment 8819358 [details]
Bug 1309284 - WebAuthn JS API [part 3]: Support origin relax algorithm

https://reviewboard.mozilla.org/r/98944/#review100408
Attachment #8819358 - Flags: review?(dkeeler) → review+
Comment on attachment 8819359 [details]
Bug 1309284 - WebAuthn JS API [part 4]: Add Unit Tests

https://reviewboard.mozilla.org/r/98946/#review100410

This looks good, modulo some minor formatting/style nits. Also, it looks like this doesn't actually test the relaxing same origin functionality, unless I missed it?

::: dom/u2f/tests/test_webauthn_get_assertion.html:24
(Diff revision 1)
> +"use strict";
> +
> +// Execute the full-scope test
> +SimpleTest.waitForExplicitFinish();
> +
> +function arrivingHereIsGood(aResult){

Looks like this function isn't used in this test.

::: dom/u2f/tests/test_webauthn_get_assertion.html:29
(Diff revision 1)
> +function arrivingHereIsGood(aResult){
> +  ok(true, "Good result! Received a: " + aResult);
> +  return Promise.resolve();
> +}
> +
> +function arrivingHereIsBad(aResult){

nit: space between ) and {

::: dom/u2f/tests/test_webauthn_loopback.html:136
(Diff revision 1)
> +    .then(function(){
> +      // We should have errored here!
> +      ok(false, "The excludeList didn't stop a duplicate being created!");
> +      SimpleTest.finish();
> +    })
> +    .catch(function(aReason){

nit: watch out for '){' without the space in between

::: dom/u2f/tests/test_webauthn_sameorigin.html:4
(Diff revision 1)
> +<!DOCTYPE html>
> +<meta charset=utf-8>
> +<head>
> +  <title>Test for MakeCredential for W3C Web Authentication</title>

nit: update title / h1 below?

::: dom/u2f/tests/test_webauthn_sameorigin.html:55
(Diff revision 1)
> +SpecialPowers.pushPrefEnv({"set": [["security.webauth.w3c", true],
> +                                   ["security.webauth.u2f_enable_softtoken", true],
> +                                   ["security.webauth.u2f_enable_usbtoken", false]]},
> +function() {
> +  isnot(navigator.authentication, undefined, "WebAuthn API endpoint must exist");
> +  isnot(navigator.authentication.makeCredential, undefined, "WebAuthn makeCredential API endpoint must exist");

nit: watch out for long lines (>99 characters is considered long for JS, I think)
Attachment #8819359 - Flags: review?(dkeeler) → review+
Comment on attachment 8816308 [details]
Bug 1309284 - Implement W3C Web Authentication JS API [part 1]

https://reviewboard.mozilla.org/r/97088/#review101142

::: dom/u2f/WebAuthentication.cpp:279
(Diff revision 4)
> +
> +  if (NS_WARN_IF(mOrigin.IsEmpty())) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  // This only functions in e10s mode

Add a TODO here to remove this in bug 1323339, since IPC will live in PBackground so either mode should work fine.

::: dom/u2f/WebAuthnAsync.h:1
(Diff revision 4)
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Why is this not called WebAuthnRequest.h? Also, I'm not exactly sure what this file does? It looks like it relates to the token hardware.
Attachment #8816308 - Flags: review?(kyle) → review-
Comment on attachment 8816308 [details]
Bug 1309284 - Implement W3C Web Authentication JS API [part 1]

https://reviewboard.mozilla.org/r/97088/#review101144

Would like some clarification about WebAuthnAsync, but otherwise things are looking good. If we can get that cleared up this'll probably r+ pretty quick, though I expect more changes in bug 1323339 to adapt this to IPC.
Attachment #8819358 - Attachment is obsolete: true
Comment on attachment 8819357 [details]
Bug 1309284 - WebAuthn JS API [part 3]: Support origin relax algorithm

https://reviewboard.mozilla.org/r/98942/#review104010
Attachment #8819357 - Flags: review?(dkeeler) → review+
Comment on attachment 8816308 [details]
Bug 1309284 - Implement W3C Web Authentication JS API [part 1]

https://reviewboard.mozilla.org/r/97088/#review104014
Attachment #8816308 - Flags: review?(kyle) → review+
Attachment #8819359 - Attachment is obsolete: true
Moving the unit tests to Bug 1329802 to land after Bug 1286312; code is disabled by default, so marking checkin-needed to get this in the tree to unblock qdot.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6f8f397d5cd1
Implement W3C Web Authentication JS API [part 1] r=keeler,qdot
https://hg.mozilla.org/integration/autoland/rev/353ff937f3e0
WebAuthn JS API [part 2]: Bugfixes from testing r=keeler
https://hg.mozilla.org/integration/autoland/rev/482e54376042
WebAuthn JS API [part 3]: Support origin relax algorithm r=keeler
Keywords: checkin-needed
Blocks: 1333084
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: