Integrate the FIDO U2F JS API with the u2f-hid-rs library

RESOLVED FIXED in Firefox 57

Status

()

enhancement
P3
normal
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: jcj, Assigned: jcj)

Tracking

(Depends on 1 bug, Blocks 2 bugs)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [webauthn])

Attachments

(3 attachments, 2 obsolete attachments)

The FIDO U2F Token interactions require middleware between the FIDO U2F JS API (Bug 1231681) and the USB HID driver (Bug 1198330). As of the time this is being written, the expectation is for this to be implemented in the dom/u2f/USBToken.cpp file from the U2F JS API bug.

The state machines written to control this interaction must be carefully designed to protect user privacy, so that the U2F JS API does not become a mechanism for javascript fingerprinting.
Component: DOM: Security → DOM: Device Interfaces
Whiteboard: [webauthn]
Note to those following along: This would be to change dom/u2f/U2F.cpp to roughly function the same as dom/webauthn/U2FTokenManager.cpp [1], instead of synchronously calling mAuthenticator->Sign(), using a TokenManager and calling mTokenManagerImpl->Sign with MozPromises.

That should be what's necessary to enable FIDO U2F hardware support in Gecko and go a long way to closing out Bug 
1065729.

[1] http://searchfox.org/mozilla-central/source/dom/webauthn/U2FTokenManager.cpp
Severity: normal → enhancement
Priority: -- → P3
Summary: Implement FIDO U2F Token State Machine → Integrate the FIDO U2F JS API with the u2f-hid-rs library
Duplicate of this bug: 1354330
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8901399 [details]
Bug 1245527 - Rewrite U2F.cpp to use U2FTokenManager

David, I'd love your feedback on the approach of this patch, and what - if anything - could be split apart for easy review.

There are a two major outstanding things still WIP here:

1) The tests all need to be touched / revised to handle U2F behaving async now.
2) The U2FManager.cpp implementation does not coordinate with WebAuthnManager.cpp, meaning that if both are live at once, u2f_hid_rs can be accessed simultaneously by both. Which is probably bad. Right now we handle singletonization in WebAuthnManager, but perhaps it should move (in another patch) out to U2FTokenManager?

The really good news is, this patch combined with the u2f-hid-rs patches produces a well-functioning U2F implementation. I haven't exercised it fully yet, but hey, hardware tokens functioning on at least a couple websites, in Firefox. Woohoo!
Attachment #8901399 - Flags: feedback?(dkeeler)
Comment on attachment 8901399 [details]
Bug 1245527 - Rewrite U2F.cpp to use U2FTokenManager

https://reviewboard.mozilla.org/r/172868/#review178754

Yeah, I don't really see a way to simplify this. It might even make sense to just `hg rm` the original implementation, commit, and then `hg add` new new implementation.
At a higher level, though, I don't really understand why we're supporting u2f and webauthn. Isn't the whole point of webauthn that it's more flexible as to what backing implementation you're using? (As in, we can have u2f physically back the webauthn api?) (Or am I misunderstanding what this patch does?)

::: dom/u2f/U2FManager.h:18
(Diff revision 1)
> +#include "mozilla/dom/PWebAuthnTransaction.h"
> +#include "nsIDOMEventListener.h"
> +#include "nsIIPCBackgroundChildCreateCallback.h"
> +
> +/*
> + * Content process manager for the WebAuthn protocol. Created on calls to the

Does this all need to be updated to refer to "U2F" or is "WebAuthn" still the right terminology?

::: dom/u2f/U2FManager.h:59
(Diff revision 1)
> +class U2FTransactionInfo;
> +
> +typedef MozPromise<nsString, ErrorCode, false> U2FPromise;
> +
> +class U2FManager final : public nsIIPCBackgroundChildCreateCallback,
> +                         public nsIDOMEventListener

nit: ',' on this line

::: dom/u2f/U2FManager.cpp:515
(Diff revision 1)
> +{
> +  MOZ_CRASH("We shouldn't be here!");
> +}
> +
> +}
> +}

nit: ending namespace comments
Comment on attachment 8901398 [details]
Bug 1245527 - Use PWebAuthnTransactionParent superclass for U2FTokenManager

https://reviewboard.mozilla.org/r/172866/#review178764
Attachment #8901398 - Flags: review?(dkeeler) → review+
Comment on attachment 8901399 [details]
Bug 1245527 - Rewrite U2F.cpp to use U2FTokenManager

If the backing library doesn't do its own resource synchronization, then yeah, we need to make sure we structure things so that two APIs aren't using it at the same time and causing confusion.
Attachment #8901399 - Flags: feedback?(dkeeler) → feedback+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(In reply to David Keeler [:keeler] (use needinfo?) from comment #8)
> Comment on attachment 8901399 [details]
> Bug 1245527 - Rewrite U2F.cpp to use U2FTokenManager
> 
> If the backing library doesn't do its own resource synchronization, then
> yeah, we need to make sure we structure things so that two APIs aren't using
> it at the same time and causing confusion.

Good news! The library's synchronization handles it OK, experimentally.

https://u2f.bin.coffee/index2.html races u2f and webauthn's create credential methods. Seems to work OK on Windows and OSX for hardware and soft token.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8901399 - Flags: review?(ttaubert)
Attachment #8901399 - Flags: review?(dkeeler)
Attachment #8902530 - Flags: review?(ttaubert)
There's an unstable build for this patch available here: https://wiki.mozilla.org/Security/CryptoEngineering#Unstable_Build:_30_August_2017
Assignee: nobody → jjones
Status: NEW → ASSIGNED
Comment on attachment 8901399 [details]
Bug 1245527 - Rewrite U2F.cpp to use U2FTokenManager

https://reviewboard.mozilla.org/r/172868/#review179594

+2.2k -> r=me!
(No but seriously, I've been staring at U2F.{h,cpp} and the tests for a few hours now, and it

::: dom/u2f/U2F.h:70
(Diff revision 3)
> +  Sign(const nsAString& aAppId,
> +       const nsAString& aChallenge,
> +       const Sequence<RegisteredKey>& aRegisteredKeys,
> +       U2FSignCallback& aCallback,
> +       const Optional<Nullable<int32_t>>& opt_aTimeoutSeconds,
> +       ErrorResult& aRv);

nit: let's have a blank line before this one

::: dom/u2f/U2F.cpp:65
(Diff revision 3)
> +  return NS_OK;
> +}
> +
> +static void
> +RegisteredKeysToScopedCredentialList(const nsAString& aAppId,
> +                            const nsTArray<RegisteredKey>& aKeys,

I think for long arguments like this that don't wrap well, we can just put them 2-space indented from the beginning of the line.

::: dom/u2f/U2F.cpp:76
(Diff revision 3)
> +        key.mVersion.Value() != kRequiredU2FVersion) {
> +      continue;
> +    }
> +
> +    // If this key's mAppId doesn't match the invocation, we can't handle it.
> +    if (key.mAppId.WasPassed() && key.mAppId.Value() != aAppId) {

I guess operator== is defined for nsAString? (unfortunately our #define string magic makes it hard to double-check...)

::: dom/u2f/U2F.cpp:131
(Diff revision 3)
> +                           nullptr, nullptr);      // ignore path
> +  if (NS_FAILED(rv)) {
> +    return ErrorCode::BAD_REQUEST;
> +  }
> +
> +  nsAutoCString appIdScheme(Substring(appIdUrl, appIdSchemePos, appIdSchemeLen));

Hmmm - is there a reason we want to construct these sort-of by hand rather than creating a new nsIURI and calling the getters on the various parts of the URI?

::: dom/u2f/U2F.cpp:168
(Diff revision 3)
> +  if (aCb.isNothing()) {
> +    return;
> +  }
> +
> +  ErrorResult error;
> +  aCb.ref()->Call(aResp, error);

So this needs to be main-thread-only also, right?

::: dom/u2f/U2F.cpp:225
(Diff revision 3)
> +              const Sequence<RegisterRequest>& aRegisterRequests,
> +              const Sequence<RegisteredKey>& aRegisteredKeys,
> +              U2FRegisterCallback& aCallback,
> +              const Optional<Nullable<int32_t>>& opt_aTimeoutSeconds,
> +              ErrorResult& aRv)
> +{

We should probably assert this is main-thread-only, right?

::: dom/u2f/U2F.cpp:283
(Diff revision 3)
> +  // Build the exclusion list, if any
> +  nsTArray<WebAuthnScopedCredentialDescriptor> excludeList;
> +  RegisteredKeysToScopedCredentialList(adjustedAppId, aRegisteredKeys,
> +                                       excludeList);
> +
> +  auto& localCb = mRegisterCallback;

Are we certain the U2F object can't go away before this promise resolves? (I'm thinking if the user toggles the pref while the register is in flight? Or maybe closes the page...?)

::: dom/u2f/tests/test_multiple_keys.html:71
(Diff revision 3)
> +  await promiseU2FRegister(window.location.origin, [regRequest], [], function(aRegResponse) {
> +    testState.key2 = keyHandleFromRegResponse(aRegResponse);
> +  });
> +
> +  await promiseU2FRegister(window.location.origin, [regRequest],
> +                           [invalidKey, testState.key1], function(aRegResponse) {

So if we just pass invalidKey it should register a new key? Or am I misunderstanding? Maybe we should add a test in any case.

::: dom/u2f/tests/test_multiple_keys.html:91
(Diff revision 3)
> +    isnot(aSignResponse.clientData, undefined, "The signing provided clientData with two keys");
> +  });
> +
> +  // Just a key that came from a random profile; syntactically valid but not
> +  // unwrappable.
> +  var invalidKey = {

Lexically this is used before it's declared. Also s/var/let/?

::: dom/u2f/tests/test_multiple_keys.html:102
(Diff revision 3)
> +                    [invalidKey, testState.key2], function(aSignResponse) {
> +    is(aSignResponse.errorCode, 0, "The signing did not error when given an invalid key");
> +    isnot(aSignResponse.clientData, undefined, "The signing provided clientData even when given an invalid key");
> +  });
> +
> +  await promiseU2FSign(window.location.origin, bytesToBase64UrlSafe(challenge),

Maybe add a testcase where we only pass the invalid key?

::: dom/u2f/tests/test_register_sign.html:112
(Diff revision 3)
> +    return state.attestationCert.getPublicKey();
> +  }).then(function(attestationPublicKey) {
> +    var signedData = assembleRegistrationSignedData(state.appParam, state.challengeParam, state.keyHandleBytes, state.publicKeyBytes);
> +    return verifySignature(attestationPublicKey, signedData, state.attestationSig);
> +  }).then(function(verified) {
> +    console.log("No error verifying signature");

not sure this is necessary - shouldn't the ok(...) on the next line cause output?

::: dom/u2f/tests/test_register_sign.html:152
(Diff revision 3)
> +    state.signResponse = signResponse;
> +  });
> +
> +  // Make sure this signature op worked, bailing early if it failed.
> +  is(state.signResponse.errorCode, 0, "The signing did not error");
> +  isnot(state.signResponse.clientData, undefined, "The signing did not provide client data");

Shouldn't this be "The signing did provide client data"?
Attachment #8901399 - Flags: review+
(Assignee)

Comment 18

2 years ago
mozreview-review-reply
Comment on attachment 8901399 [details]
Bug 1245527 - Rewrite U2F.cpp to use U2FTokenManager

https://reviewboard.mozilla.org/r/172868/#review179594

> I guess operator== is defined for nsAString? (unfortunately our #define string magic makes it hard to double-check...)

It clearly is due to this passing tests, but I'm going to go ahead and change the logic to `Equals(...)` because it took me 5 minutes to decide I can't find where `operator==` is defined in our codebase to do it (because it actually is inheriting from the langauge `string`) so... `.Equals(...)`!

> Hmmm - is there a reason we want to construct these sort-of by hand rather than creating a new nsIURI and calling the getters on the various parts of the URI?

I guess I don't know how to do that. I'll give it a shot, though.

> So this needs to be main-thread-only also, right?

Yes, and it does no harm to enforce it here, too.

> We should probably assert this is main-thread-only, right?

Yup, thanks!

> Are we certain the U2F object can't go away before this promise resolves? (I'm thinking if the user toggles the pref while the register is in flight? Or maybe closes the page...?)

That _might_ be possible under certain circumstances. The solution is then to make these MozPromises a little more complicated and use their `Track` method to provide a mechanism to disable the promises when the U2F object is being destructed -- or if `mEventTarget` ever gets changed out from underneath it (which it shouldn't.)

OK, this will need some thought.

> So if we just pass invalidKey it should register a new key? Or am I misunderstanding? Maybe we should add a test in any case.

You're right; test added.

> Shouldn't this be "The signing did provide client data"?

Yep... heh.

Comment 19

2 years ago
mozreview-review
Comment on attachment 8902530 [details]
Bug 1245527 - Remove NSS U2F SoftToken

https://reviewboard.mozilla.org/r/174130/#review180938

Lots of red. LGTM.
Attachment #8902530 - Flags: review?(ttaubert) → review+

Comment 20

2 years ago
mozreview-review
Comment on attachment 8901399 [details]
Bug 1245527 - Rewrite U2F.cpp to use U2FTokenManager

https://reviewboard.mozilla.org/r/172868/#review180940

::: dom/u2f/U2FManager.h:102
(Diff revision 3)
> +  typedef MozPromise<nsresult, nsresult, false> BackgroundActorPromise;
> +
> +  RefPtr<BackgroundActorPromise> GetOrCreateBackgroundActor();
> +
> +  // Promise representing transaction status.
> +  MozPromiseHolder<U2FPromise> mTransactionPromise;

Is there a reason you're using a MozPromiseHolder<> instead of just a RefPtr<> like in WebAuthn?

::: dom/u2f/U2FManager.cpp:42
(Diff revision 3)
> +static nsresult
> +HashCString(nsICryptoHash* aHashService, const nsACString& aIn,
> +            /* out */ CryptoBuffer& aOut)

Should we put this somewhere we can share it between U2F and WebAuthn?

::: dom/u2f/U2FManager.cpp:54
(Diff revision 3)
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +
> +  rv = aHashService->Update(
> +    reinterpret_cast<const uint8_t*>(aIn.BeginReading()),aIn.Length());

nit: space between arguments

::: dom/u2f/U2FManager.cpp:89
(Diff revision 3)
> +static void
> +StopListeningForVisibilityEvents(nsPIDOMWindowInner* aParent,
> +                                 U2FManager* aListener)

We could share a lot of code like this if we had a BaseAuthManager (or the like) that would be the base for the {U2F,WebAuthn}Manager classes.

We might consolidate this in a follow-up if we want. Maybe that doesn't make a lot of sense though as we'd want to get rid of U2F in the future?

::: dom/u2f/U2FManager.cpp:195
(Diff revision 3)
> +U2FManager::Register(nsPIDOMWindowInner* aParent, const nsCString& aRpId,
> +  const nsCString& aClientDataJSON, const uint32_t& aTimeoutMillis,
> +  const nsTArray<WebAuthnScopedCredentialDescriptor>& aExcludeList)

nit: the formatting doesn't match what we usually do for long argument lists

::: dom/u2f/U2FManager.cpp:207
(Diff revision 3)
> +
> +  nsresult srv;
> +  nsCOMPtr<nsICryptoHash> hashService =
> +    do_CreateInstance(NS_CRYPTO_HASH_CONTRACTID, &srv);
> +  if (NS_WARN_IF(NS_FAILED(srv))) {
> +    return U2FPromise::CreateAndReject(ErrorCode::OTHER_ERROR, __func__).forget();

Shouldn't we also use `aParent` to construct the promise?

```
nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(aParent);
RefPtr<Promise> promise = Promise::Create(global, rv);

if (failure) {
  promise->MaybeReject(NS_ERROR_OUT_OF_MEMORY);
  return promise.forget();
}
```

::: dom/u2f/U2FManager.cpp:298
(Diff revision 3)
> +
> +  nsresult srv;
> +  nsCOMPtr<nsICryptoHash> hashService =
> +    do_CreateInstance(NS_CRYPTO_HASH_CONTRACTID, &srv);
> +  if (NS_WARN_IF(NS_FAILED(srv))) {
> +    return U2FPromise::CreateAndReject(ErrorCode::OTHER_ERROR, __func__).forget();

Same here about `Promise::Create`. `Promise::CreateAndReject().forget()` is a pattern I can't find anywhere else.

::: dom/u2f/U2FManager.cpp:301
(Diff revision 3)
> +  CryptoBuffer rpIdHash;
> +  if (!rpIdHash.SetLength(SHA256_LENGTH, fallible)) {
> +    return U2FPromise::CreateAndReject(ErrorCode::OTHER_ERROR, __func__).forget();
> +  }
> +  srv = HashCString(hashService, aRpId, rpIdHash);
> +  if (NS_WARN_IF(NS_FAILED(srv))) {
> +    return U2FPromise::CreateAndReject(ErrorCode::OTHER_ERROR, __func__).forget();
> +  }
> +
> +  CryptoBuffer clientDataHash;
> +  if (!clientDataHash.SetLength(SHA256_LENGTH, fallible)) {
> +    return U2FPromise::CreateAndReject(ErrorCode::OTHER_ERROR, __func__).forget();
> +  }
> +  srv = HashCString(hashService, aClientDataJSON, clientDataHash);
> +  if (NS_WARN_IF(NS_FAILED(srv))) {
> +    return U2FPromise::CreateAndReject(ErrorCode::OTHER_ERROR, __func__).forget();
> +  }
> +
> +  if (MOZ_LOG_TEST(gU2FLog, LogLevel::Debug)) {
> +    nsString base64;
> +    Unused << NS_WARN_IF(NS_FAILED(rpIdHash.ToJwkBase64(base64)));
> +
> +    MOZ_LOG(gU2FLog, LogLevel::Debug,
> +            ("dom::U2FManager::Sign::RpID: %s",
> +              aRpId.get()));
> +
> +    MOZ_LOG(gU2FLog, LogLevel::Debug,
> +            ("dom::U2FManager::Sign::Rp ID Hash (base64): %s",
> +              NS_ConvertUTF16toUTF8(base64).get()));
> +
> +    Unused << NS_WARN_IF(NS_FAILED(clientDataHash.ToJwkBase64(base64)));
> +
> +    MOZ_LOG(gU2FLog, LogLevel::Debug,
> +            ("dom::U2FManager::Sign::Client Data JSON: %s",
> +              aClientDataJSON.get()));
> +
> +    MOZ_LOG(gU2FLog, LogLevel::Debug,
> +            ("dom::U2FManager::Sign::Client Data Hash (base64): %s",
> +              NS_ConvertUTF16toUTF8(base64).get()));
> +  }

Could we pull this out? Maybe together with the WebAuthnTransactionInfo below? This seems to be exactly the same as for ::Register().

::: dom/u2f/U2FTransactionChild.h:13
(Diff revision 3)
> +#define mozilla_dom_U2FTransactionChild_h
> +
> +#include "mozilla/dom/PWebAuthnTransactionChild.h"
> +
> +/*
> + * Child process IPC implementation for WebAuthn API. Receives results of

"for U2F API."

::: dom/u2f/U2FTransactionChild.h:14
(Diff revision 3)
> + * WebAuthn transactions from the parent process, and sends them to the
> + * WebAuthnManager either cancel the transaction, or be formatted and relayed to

More WebAuthn mentions :)

::: dom/u2f/U2FTransactionChild.h:15
(Diff revision 3)
> +#include "mozilla/dom/PWebAuthnTransactionChild.h"
> +
> +/*
> + * Child process IPC implementation for WebAuthn API. Receives results of
> + * WebAuthn transactions from the parent process, and sends them to the
> + * WebAuthnManager either cancel the transaction, or be formatted and relayed to

"to either cancel..."
Attachment #8901399 - Flags: review?(ttaubert)

Comment 21

2 years ago
mozreview-review
Comment on attachment 8901399 [details]
Bug 1245527 - Rewrite U2F.cpp to use U2FTokenManager

https://reviewboard.mozilla.org/r/172868/#review180966

::: dom/u2f/U2F.cpp:283
(Diff revision 3)
> +  // Build the exclusion list, if any
> +  nsTArray<WebAuthnScopedCredentialDescriptor> excludeList;
> +  RegisteredKeysToScopedCredentialList(adjustedAppId, aRegisteredKeys,
> +                                       excludeList);
> +
> +  auto& localCb = mRegisterCallback;

Yeah, I think a MozPromiseRequestHolder is the right thing to use here. We should then call Disconnect() upon destruction, but also whenever Register() or Sign() is called. Like we do in the U2FTokenManager for m{Register,Sign}Promise.
(Assignee)

Comment 22

2 years ago
mozreview-review-reply
Comment on attachment 8901399 [details]
Bug 1245527 - Rewrite U2F.cpp to use U2FTokenManager

https://reviewboard.mozilla.org/r/172868/#review179594

Thanks, Keeler!

> I guess I don't know how to do that. I'll give it a shot, though.

Oh hey, I do know how to do that. Thanks!

> That _might_ be possible under certain circumstances. The solution is then to make these MozPromises a little more complicated and use their `Track` method to provide a mechanism to disable the promises when the U2F object is being destructed -- or if `mEventTarget` ever gets changed out from underneath it (which it shouldn't.)
> 
> OK, this will need some thought.

While ugly, I can ensure the safety of this without the `Track` method (and all of its added baggage) by calling the Maybe functions' `reset` methods in the destructor. Since we're working with references to those methods in the callbacks, that will stop them from calling stale references.
(Assignee)

Comment 23

2 years ago
mozreview-review-reply
Comment on attachment 8901399 [details]
Bug 1245527 - Rewrite U2F.cpp to use U2FTokenManager

https://reviewboard.mozilla.org/r/172868/#review180940

Thanks for the review, Tim!

> Is there a reason you're using a MozPromiseHolder<> instead of just a RefPtr<> like in WebAuthn?

Because it's a MozPromise, rather than a JS::Promimse. See `dom/webauthn/U2FTokenManager.h`.

> Should we put this somewhere we can share it between U2F and WebAuthn?

Yes, good idea. I've made a U2FUtil.h file. It seems like some of WebAuthnUtil.cpp could move there, but while that logically makes sense, the U2F-related code in there is not actually useful to `dom/u2f/` so I'm going to keep it simple for now.

> We could share a lot of code like this if we had a BaseAuthManager (or the like) that would be the base for the {U2F,WebAuthn}Manager classes.
> 
> We might consolidate this in a follow-up if we want. Maybe that doesn't make a lot of sense though as we'd want to get rid of U2F in the future?

I don't think we'll get rid of U2F anytime soon, but I also agree that this would be a better follow-up for the sake of momentum. I've opened Bug 1396907.

> nit: the formatting doesn't match what we usually do for long argument lists

lol... See the above comment by Keeler reocmmending this.

I can add a newline between aClientDataJson and aTimeoutMillis, I suppose...

> Shouldn't we also use `aParent` to construct the promise?
> 
> ```
> nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(aParent);
> RefPtr<Promise> promise = Promise::Create(global, rv);
> 
> if (failure) {
>   promise->MaybeReject(NS_ERROR_OUT_OF_MEMORY);
>   return promise.forget();
> }
> ```

If it were a JS::Promise, yes. MozPromises work this way.

> Same here about `Promise::Create`. `Promise::CreateAndReject().forget()` is a pattern I can't find anywhere else.

Admittedly there isn't much that returns the `already_AddRefed` form, but there are tons of them returning the `RefPtr`. See `dom/webauthn/U2FSoftTokenManager.cpp` or all the `CreateAndReject` instances in `dom/media/platforms/agnostic/`

> Could we pull this out? Maybe together with the WebAuthnTransactionInfo below? This seems to be exactly the same as for ::Register().

Great idea, thanks! Go-go `PopulateTransactionInfo()`
(Assignee)

Comment 24

2 years ago
mozreview-review-reply
Comment on attachment 8901399 [details]
Bug 1245527 - Rewrite U2F.cpp to use U2FTokenManager

https://reviewboard.mozilla.org/r/172868/#review180966

> Yeah, I think a MozPromiseRequestHolder is the right thing to use here. We should then call Disconnect() upon destruction, but also whenever Register() or Sign() is called. Like we do in the U2FTokenManager for m{Register,Sign}Promise.

Yes, though it's a lot of boilerplate for basically killing the `Maybe`s, so I instead used `Maybe.reset` in the destuctor. That accomplishes the same thing.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(In reply to J.C. Jones [:jcj] from comment #23)
> > Is there a reason you're using a MozPromiseHolder<> instead of just a RefPtr<> like in WebAuthn?
> 
> Because it's a MozPromise, rather than a JS::Promimse. See
> `dom/webauthn/U2FTokenManager.h`.

I knew I missed something.

> > nit: the formatting doesn't match what we usually do for long argument lists
> 
> lol... See the above comment by Keeler reocmmending this.
> 
> I can add a newline between aClientDataJson and aTimeoutMillis, I suppose...

Oops. I don't actually care that much about this :)
(In reply to J.C. Jones [:jcj] from comment #24)
> > Yeah, I think a MozPromiseRequestHolder is the right thing to use here. We should then call Disconnect() upon destruction, but also whenever Register() or Sign() is called. Like we do in the U2FTokenManager for m{Register,Sign}Promise.
> 
> Yes, though it's a lot of boilerplate for basically killing the `Maybe`s, so
> I instead used `Maybe.reset` in the destuctor. That accomplishes the same
> thing.

Does it though? You're capturing mRegisterCallback or mSignCallback in the lambdas and IIUIC we potentially access this->m{Sign,Register}Callback from ExecuteCallback(), even after destruction of the U2F instance.

I think only through a MozPromiseRequestHolder and calling Disconnect() we could prevent the lambdas from being executed after destruction. By doing so we could also "cancel" these promises whenever Register() is called while waiting on Sign() or vice-versa. The token manager does that already, I think, but it would probably be nice to have this here as well, in case a promise resolution is in-flight.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 34

2 years ago
mozreview-review
Comment on attachment 8901399 [details]
Bug 1245527 - Rewrite U2F.cpp to use U2FTokenManager

https://reviewboard.mozilla.org/r/172868/#review182768

LGTM. r=me if you untangle the PromiseRequestHolder a little bit as described below :)

::: dom/u2f/U2F.cpp:160
(Diff revision 5)
> +  if (aPromise.Exists()) {
> +    aPromise.Complete();
> +  }

We should call `Complete()` that asserts `Exists() == true`. Anything else would mean `Disconnect()` doesn't work or the callback was called twice. That would assert when `Register()` or `Sign()` fail early but see below...

::: dom/u2f/U2F.cpp:295
(Diff revision 5)
> +                      ("dom::U2F::Register::Promise::Resolve, response was %s",
> +                        NS_ConvertUTF16toUTF8(aResponse).get()));
> +              RegisterResponse response;
> +              response.Init(aResponse);
> +
> +              ExecuteCallback(response, localCb, localReqHolder);

How about not passing `localReqHolder` to `ExecuteCallback()` and just calling `localReqHolder.Complete()` right here in the closures? `ExecuteCallback()` shouldn't need to know about that...
Attachment #8901399 - Flags: review?(ttaubert) → review+
(Assignee)

Comment 35

2 years ago
mozreview-review-reply
Comment on attachment 8901399 [details]
Bug 1245527 - Rewrite U2F.cpp to use U2FTokenManager

https://reviewboard.mozilla.org/r/172868/#review182768

Thanks so much!

> We should call `Complete()` that asserts `Exists() == true`. Anything else would mean `Disconnect()` doesn't work or the callback was called twice. That would assert when `Register()` or `Sign()` fail early but see below...

I didn't understand, but I do now! Thanks.

> How about not passing `localReqHolder` to `ExecuteCallback()` and just calling `localReqHolder.Complete()` right here in the closures? `ExecuteCallback()` shouldn't need to know about that...

By your comments combined, I understand your brilliance! I agree.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8902506 - Attachment is obsolete: true
No real changes since the last green try run, so marking checkin-needed. Thanks, everyone!
Keywords: checkin-needed

Comment 41

2 years ago
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 3 changesets with 37 changes to 37 files
remote: 
remote: sync-messages.ini altered in changeset dd082909c6f6 without IPC peer review
remote: 
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: 
remote: Changes to sync-messages.ini in this repo require review from a IPC peer in the form of r=...
remote: This is to ensure that we behave responsibly by not adding sync IPC messages that cause performance issues needlessly. We appreciate your understanding..
remote: 
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.d_webidl hook failed
abort: push failed on remote
Oh boy. Good news is, I deleted all the sync IPC messages. I'll find an IPC peer. :)
Keywords: checkin-needed
Jed: The third patch removes all the U2F sync IPC-using code from tree, switching to the async forms that qDot put together. It's a very red patch. Thanks!
Comment on attachment 8902530 [details]
Bug 1245527 - Remove NSS U2F SoftToken

https://reviewboard.mozilla.org/r/174130/#review182912

rs=me.
Attachment #8902530 - Flags: review?(jld) → review+

Comment 45

2 years ago
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 3 changesets with 37 changes to 37 files
remote: 
remote: sync-messages.ini altered in changeset 31c3886b400c without IPC peer review
remote: 
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: 
remote: Changes to sync-messages.ini in this repo require review from a IPC peer in the form of r=...
remote: This is to ensure that we behave responsibly by not adding sync IPC messages that cause performance issues needlessly. We appreciate your understanding..
remote: 
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.d_webidl hook failed
abort: push failed on remote
Hi Ryan,

Jed's an IPC peer according to [1]; what am I doing wrong?

Thanks!

[1] https://wiki.mozilla.org/Modules/Core
Flags: needinfo?(ryanvm)
Looks like the hg hook is looking for "jed" instead of "jld"
https://hg.mozilla.org/hgcustom/version-control-tools/file/default/hghooks/mozhghooks/prevent_webidl_changes.py#l86

You can't make this up...
Flags: needinfo?(ryanvm)
I'm going to push this to inbound for now. Can you please file a follow-up bug in "Developer Services::Mercurial: hg.mozilla.org" to update the hook?

Comment 49

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/be63e73426b4
Use PWebAuthnTransactionParent superclass for U2FTokenManager. r=keeler
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6a5de8d1246
Rewrite U2F.cpp to use U2FTokenManager. r=keeler, r=ttaubert
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ee1f7aebd62
Remove NSS U2F SoftToken. r=ttaubert, r=jed
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/c3eea543172c for a pair of inconvenient problems.

Apparently the reason you don't see any other uses of "prefs =" in mochitest.ini is because you can only use it when runByManifest is set, which it isn't for Android, so you get the obscure https://treeherder.mozilla.org/logviewer.html#?job_id=129762223&repo=mozilla-inbound when you try.

You also have leaks on ASan in every browser-chrome chunk, but because bug 1395424, that's it, "you have leaks." Good luck fixing those leaks of... something, because https://treeherder.mozilla.org/logviewer.html#?job_id=129762588&repo=mozilla-inbound is all you get to go on.
Depends on: 1395424
Oh boy. Thanks, Philor. At least I know what I'm doing next week!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8906784 - Attachment is obsolete: true
Attachment #8906784 - Flags: review?(dkeeler)
Comment on attachment 8901399 [details]
Bug 1245527 - Rewrite U2F.cpp to use U2FTokenManager

https://reviewboard.mozilla.org/r/172868/#review183552

I had a look at the refactored tests - looks good except for some minor nits (mainly use of var or let depending on if you're in the top-level scope or not).

::: dom/u2f/tests/frame_appid_facet.html:12
(Diff revision 9)
>  <body>
> -<p>Test for AppID / FacetID behavior for FIDO Universal Second Factor</p>
> +<p>AppID / Facet checks</p>
>  <script class="testbody" type="text/javascript">
>  "use strict";
>  
> -local_is(window.location.origin, "https://example.com", "Is loaded correctly");
> +async function doTests() {

Do we not still want the sanity check? (or am I missing it?)

::: dom/u2f/tests/frame_appid_facet.html:13
(Diff revision 9)
>  <script class="testbody" type="text/javascript">
>  "use strict";
>  
> -local_is(window.location.origin, "https://example.com", "Is loaded correctly");
> +async function doTests() {
> -
> -var version = "U2F_V2";
> +  var version = "U2F_V2";

nit: since this isn't top-level any more, I think technically the style would be to use 'let' instead of 'var'

::: dom/u2f/tests/frame_appid_facet.html:34
(Diff revision 9)
> -}], [], function(res){
> +    }], [], function(res){
> -  local_is(res.errorCode, 0, "Empty AppID should work.");
> +      local_is(res.errorCode, 0, "Empty AppID should work.");
> -  local_completeTest();
> -});
> +  });
>  
> -// Test: Correct TLD, but incorrect scheme
> +  await promiseU2FRegister("http://example.com/appId", [{

Might be nice to keep the comments?

::: dom/u2f/tests/frame_appid_facet_subdomain.html:8
(Diff revision 9)
>  <head>
> -  <script src="u2futil.js"></script>
> +  <script type="text/javascript" src="frame_utils.js"></script>
> +  <script type="text/javascript" src="u2futil.js"></script>
>  </head>
>  <body>
> -<p>Test for AppID / FacetID behavior for FIDO Universal Second Factor</p>
> +<p>AppID / FacetID behavior check</p>

Maybe add something about subdomain?

::: dom/u2f/tests/frame_multiple_keys.html:41
(Diff revision 9)
>    key2: null,
>  }
>  
> +// Just a key that came from a random profile; syntactically valid but not
> +// unwrappable.
> +let invalidKey = {

nit: s/let/var/ at the top level

::: dom/u2f/tests/u2futil.js:3
(Diff revision 9)
> -// Used by local_addTest() / local_completeTest()
> -var _countCompletions = 0;
> -var _expectedCompletions = 0;
> +function promiseU2FRegister(aAppId, aChallenges, aExcludedKeys, aFunc) {
> +  return new Promise(function(resolve, reject) {
> +      u2f.register(aAppId, aChallenges, aExcludedKeys, function(res){

nit: space before '{'

::: dom/u2f/tests/u2futil.js:12
(Diff revision 9)
> -    ok(false, "Unexpected message in the test harness: " + event.data)
> -  }
> +}
> +
> +function promiseU2FSign(aAppId, aChallenge, aAllowedKeys, aFunc) {
> +  return new Promise(function(resolve, reject) {
> +      u2f.sign(aAppId, aChallenge, aAllowedKeys, function(res){

space before '{'
(Assignee)

Comment 59

2 years ago
mozreview-review-reply
Comment on attachment 8901399 [details]
Bug 1245527 - Rewrite U2F.cpp to use U2FTokenManager

https://reviewboard.mozilla.org/r/172868/#review183552

This is great, thanks again, Keeler! All these things are way more obvious looking at the old-vs-new side-by-side, which I hadn't been doing since before I had basically re-written them. Anynway, thanks!
Kicked off the m-bc tests on your ASan build on try and they look good, did you change anything? :)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=70951ba7ee39c107084dda9329c61c98b3bd369f
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Tim - thanks for doing that, you'll have to show me how! I did not change anything, actually, so that the ASAN build isn't showing a leak maybe means it wasn't me? I audited all the heap-usage in the patches on the plane yesterday and couldn't find anything, so it's pleasing to know maybe I wasn't nuts.

This try run shows I fixed Android: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e1445bc4ff2ddef22fba11ed795ebc92118dfc0

This try run shows that we're not having leaks in the ASAN mochitests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=70951ba7ee39c107084dda9329c61c98b3bd369f

NOTE TO SHERIFF: ReviewBoard still shows :jld 's r+ on the commit that touches the IPC code, but Bugzilla isn't reflecting it.

Marking checkin-needed.
Keywords: checkin-needed

Comment 64

2 years ago
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 3 changesets with 33 changes to 33 files
remote: 
remote: sync-messages.ini altered in changeset 2373be94ec56 without IPC peer review
remote: 
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: 
remote: Changes to sync-messages.ini in this repo require review from a IPC peer in the form of r=...
remote: This is to ensure that we behave responsibly by not adding sync IPC messages that cause performance issues needlessly. We appreciate your understanding..
remote: 
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.d_webidl hook failed
abort: push failed on remote
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 68

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f33559018906
Use PWebAuthnTransactionParent superclass for U2FTokenManager. r=keeler
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd315914f198
Rewrite U2F.cpp to use U2FTokenManager. r=keeler, r=ttaubert
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd7e4852bd06
Remove NSS U2F SoftToken. r=ttaubert, r=jed
Keywords: checkin-needed
Depends on: 1401872
You need to log in before you can comment on or make changes to this bug.