Closed Bug 1245527 Opened 9 years ago Closed 7 years ago

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

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: jcj, Assigned: jcj)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [webauthn])

Attachments

(3 files, 2 obsolete files)

Bug 1245527 - Use PWebAuthnTransactionParent superclass for U2FTokenManager
59 bytes, text/x-review-board-request
keeler
: review+
Details
Bug 1245527 - Rewrite U2F.cpp to use U2FTokenManager
59 bytes, text/x-review-board-request
keeler
: review+
ttaubert
: review+
Details
Bug 1245527 - Remove NSS U2F SoftToken
59 bytes, text/x-review-board-request
ttaubert
: review+
Details
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.
Depends on: 1198330, 1231681
Blocks: 1065729
Component: DOM: Security → DOM: Device Interfaces
Depends on: 1298838
Whiteboard: [webauthn]
Blocks: 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
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+
(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.
Attachment #8901399 - Flags: review?(ttaubert)
Attachment #8901399 - Flags: review?(dkeeler)
Attachment #8902530 - Flags: review?(ttaubert)
See Also: → 1347374
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+
Blocks: 1395406
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 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 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 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.
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.
Blocks: 1396907
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()`
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.
(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 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+
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.
Attachment #8902506 - Attachment is obsolete: true
No real changes since the last green try run, so marking checkin-needed. Thanks, everyone!
Keywords: checkin-needed
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
Attachment #8902530 - Flags: review?(jld)
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+
Keywords: checkin-needed
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?
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!
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 '{'
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
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
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
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.

Attachment

General

Creator:
Created:
Updated:
Size: