Closed Bug 1323339 Opened 9 years ago Closed 8 years ago

Implement IPC and Parent Process portions of WebAuthn API

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: qdot, Assigned: qdot)

References

(Blocks 1 open bug)

Details

(Whiteboard: [webauthn])

Attachments

(7 files)

Bug 1323339 - IPDL for WebAuthn Protocol and PBackground additions
59 bytes, text/x-review-board-request
baku
: review+
jcj
: review+
Details
Bug 1323339 - Cleanup of WebAuthn DOM classes
59 bytes, text/x-review-board-request
baku
: review+
jcj
: review+
Details
Bug 1323339 - Turn WebAuthenication into a simple DOM class that passes to WebAuthnManager
59 bytes, text/x-review-board-request
baku
: review+
jcj
: review+
Details
Bug 1323339 - Add WebAuthnManager and support IPC Child classes
59 bytes, text/x-review-board-request
baku
: review+
jcj
: review+
Details
Bug 1323339 - Add U2FTokenManager class and support IPC Parent classes
59 bytes, text/x-review-board-request
baku
: review+
jcj
: review+
Details
Bug 1323339 - Add U2FSoftToken Manager
59 bytes, text/x-review-board-request
baku
: review+
jcj
: review+
Details
Bug 1323339 - Modify WebAuthn mochitests to work with IPC API
59 bytes, text/x-review-board-request
baku
: review+
jcj
: review+
Details
Implements the IPDL protocols and parent process hardware management for WebAuthn Protocol. Exposed to content via bug 1309284.
Does the dispatching to the different HW protocols happen in the parent process with one ParentWebAuthenMakeCredential or are you planning to have different calls to the parent like ParentWebAuthnUsbMakeCredential and ParentWebAuthnBtMakecredential? Does this bug really depend on the HID Bug1298838 ? I think that the Parent Process messaging could be done in parallel especially if there is only one message to the parent that transfers all the parameters from JS to the parent process. Thanks
(In reply to Axel Nennker from comment #1) > Does the dispatching to the different HW protocols happen in the parent > process with one ParentWebAuthenMakeCredential or are you planning to have > different calls to the parent like ParentWebAuthnUsbMakeCredential and > ParentWebAuthnBtMakecredential? Pretty sure qdot's implementing it as a single call to the parent process, and all registered authenticators then get the message fanned out from there. > Does this bug really depend on the HID Bug1298838 ? > I think that the Parent Process messaging could be done in parallel > especially if there is only one message to the parent that transfers all the > parameters from JS to the parent process. No, that's a mistake, it's actually blocking Bug 1298838. Fixed.
Blocks: 1298838
Status: NEW → ASSIGNED
No longer depends on: 1298838
OS: Unspecified → All
Hardware: Unspecified → All
Whiteboard: [webauthn]
jcj, you might want to glance through these patches before I start getting them ready for review, as I've munged the spec related comments you had in there pretty badly. Most of that was due to dividing processing between child and parent processes, as well as just optimizing the logic of a few things, since spec function order didn't necessarily lay out best execution order. I can get this stuff cleaned up, just need to know what your preferences are since you're gonna have to work in this code too after it lands. :)
Flags: needinfo?(jjones)
Hey qdot - I have taken a look, and it's entirely fine to write-off the spec-related comments for now. The draft has changed enough that all of that code needs an update to ensure it's still doing the right things, and we can adjust the comments then. Whenever it seems like the comment-ordering is misleading, feel free to delete. It should be plenty obvious which algorithm is which without them. Thanks!
Flags: needinfo?(jjones)
Having fixed up comments and commit messages, first 4 patches are now in for review. Continuing to work on parent side since I still feel like it's kinda messy. Most of the comments taken from the spec are still correct, so I just left them in. However, I did remove the indexes from them, as we don't want to be stuck having to sync with spec order constantly. Also, I tried to check for this but there may be places where code is repeated that I either missed bringing the comment along, or it shouldn't be repeated anyways. Feel free to point that out in review.
Comment on attachment 8855582 [details] Bug 1323339 - IPDL for WebAuthn Protocol and PBackground additions https://reviewboard.mozilla.org/r/127428/#review133086 ::: dom/webauthn/PWebAuthnTransaction.ipdl:37 (Diff revision 2) > +}; > + > +struct WebAuthnTransactionInfo { > + uint8_t[] RpIdHash; > + uint8_t[] ClientDataHash; > + uint32_t Timeout; Nit which we can fix in a latter commit to avoid rebasing: let's include the units since this is an ambiguous type (millis, presumably?) ::: dom/webauthn/PWebAuthnTransaction.ipdl:52 (Diff revision 2) > + async RequestSign(WebAuthnTransactionInfo aTransactionInfo); > + async RequestCancel(); > + child: > + async ConfirmRegister(uint8_t[] RegBuffer, uint8_t[] SigBuffer); > + async ConfirmSign(uint8_t[] CredentialID, uint8_t[] ReplyBuffer); > + async Cancel(nsresult Error); Observation: Using nsresult here seems like it's going to require us to add some kind of mapping between U2F error types and nsresult enumerations. But that's probably fine, and can always change it later.
Attachment #8855582 - Flags: review?(jjones) → review+
Comment on attachment 8855583 [details] Bug 1323339 - Cleanup of WebAuthn DOM classes https://reviewboard.mozilla.org/r/127430/#review133088
Attachment #8855583 - Flags: review?(jjones) → review+
Comment on attachment 8855584 [details] Bug 1323339 - Turn WebAuthenication into a simple DOM class that passes to WebAuthnManager https://reviewboard.mozilla.org/r/127432/#review133098
Attachment #8855584 - Flags: review?(jjones) → review+
Sorry for the odd review order here. I've still gotta clean up some pref fetching in the soft token manager patch, then it should be ready. Hopefully tomorrow or Wednesday.
Comment on attachment 8855585 [details] Bug 1323339 - Add WebAuthnManager and support IPC Child classes https://reviewboard.mozilla.org/r/127434/#review133906 ::: dom/webauthn/WebAuthnManager.cpp:167 (Diff revision 3) > +} > + > +void > +WebAuthnManager::MaybeClearTransaction() > +{ > + mInfo.reset(); Nit: It feels a little weird to wipe `mInfo` here but leave `mClientData` populated. ::: dom/webauthn/WebAuthnManager.cpp:220 (Diff revision 3) > + const Account& aAccountInformation, > + const Sequence<ScopedCredentialParameters>& aCryptoParameters, > + const ArrayBufferViewOrArrayBuffer& aChallenge, > + const ScopedCredentialOptions& aOptions) > +{ > + MOZ_ASSERT(aParent); It'd be good to assert what threads are used here. ::: dom/webauthn/WebAuthnManager.cpp:614 (Diff revision 3) > + > +void > +WebAuthnManager::FinishMakeCredential(nsTArray<uint8_t>& aRegBuffer, > + nsTArray<uint8_t>& aSigBuffer) > +{ > + MOZ_ASSERT(mTransactionPromise); Can we assert what thread we're on here, too? This is only accessible from the IPC threadset, right?
Attachment #8855585 - Flags: review?(jjones) → review+
Comment on attachment 8855588 [details] Bug 1323339 - Modify WebAuthn mochitests to work with IPC API https://reviewboard.mozilla.org/r/127440/#review133926
Attachment #8855588 - Flags: review?(jjones) → review+
Comment on attachment 8855586 [details] Bug 1323339 - Add U2FTokenManager class and support IPC Parent classes https://reviewboard.mozilla.org/r/127436/#review133946 I think this will be clearer when the following patch is ready for review, too. As-is I just probably highlighted things that are fixed in that patch -- sorry if so! ::: dom/webauthn/U2FTokenManager.cpp:135 (Diff revision 3) > + > +void > +U2FTokenManager::Register(WebAuthnTransactionParent* aTransactionParent, > + const WebAuthnTransactionInfo& aTransactionInfo) > +{ > + this->Run(aTransactionParent, It seems to me that there should be something here clearing `mInfo` in the way that `MaybeClearTransaction()` is used in WebAuthn. ::: dom/webauthn/U2FTokenManager.cpp:160 (Diff revision 3) > + > + mTransactionParent = aTransactionParent; > + mInfo = Some(aTransactionInfo); > + > + // Use InvokeAsync to retrieve preferences on the main thread, then bounce > + // back to the background thread to continue U2F operations. Saves having to Copy/paste issue? Maybe "saves having to have multiple runnable classes"? ::: dom/webauthn/U2FTokenManager.cpp:166 (Diff revision 3) > + // multiple runnable classes. > + InvokeAsync(AbstractThread::MainThread(), __func__, []() { > + MOZ_ASSERT(XRE_IsParentProcess()); > + MOZ_ASSERT(NS_IsMainThread()); > + nsTArray<bool> prefs; > + //TODO: Get preferences for token managers we should use. I think we talked about this TODO on IRC. ::: dom/webauthn/U2FTokenManager.cpp:193 (Diff revision 3) > + ); > +} > + > +void > +U2FTokenManager::DoRegister() > +{ Probably should assert that these run on the `mOwningThread`.
Attachment #8855586 - Flags: review?(jjones) → review-
Comment on attachment 8855582 [details] Bug 1323339 - IPDL for WebAuthn Protocol and PBackground additions https://reviewboard.mozilla.org/r/127428/#review134292
Attachment #8855582 - Flags: review?(amarchesini) → review+
Comment on attachment 8855583 [details] Bug 1323339 - Cleanup of WebAuthn DOM classes https://reviewboard.mozilla.org/r/127430/#review134294
Attachment #8855583 - Flags: review?(amarchesini) → review+
Comment on attachment 8855584 [details] Bug 1323339 - Turn WebAuthenication into a simple DOM class that passes to WebAuthnManager https://reviewboard.mozilla.org/r/127432/#review134296
Attachment #8855584 - Flags: review?(amarchesini) → review+
Comment on attachment 8855585 [details] Bug 1323339 - Add WebAuthnManager and support IPC Child classes https://reviewboard.mozilla.org/r/127434/#review134298 You must improve the the shutting down of the child actor before landing this patch. ::: dom/webauthn/WebAuthnManager.cpp:168 (Diff revision 3) > + > +void > +WebAuthnManager::MaybeClearTransaction() > +{ > + mInfo.reset(); > + if (mTransactionPromise) { don't do this if(). Just simply: mTransactionPromise = nullptr; ::: dom/webauthn/WebAuthnManager.cpp:171 (Diff revision 3) > +{ > + mInfo.reset(); > + if (mTransactionPromise) { > + mTransactionPromise = nullptr; > + } > + if (mChild) { mChild could be gone at this time. See ActorDestroyed comments. ::: dom/webauthn/WebAuthnManager.cpp:445 (Diff revision 3) > +} > + > +void > +WebAuthnManager::StartRegister() { > + MOZ_ASSERT(mChild); > + mChild->SendRequestRegister(mInfo.ref()); same comment about IsActive(). See below. ::: dom/webauthn/WebAuthnManager.cpp:450 (Diff revision 3) > + mChild->SendRequestRegister(mInfo.ref()); > +} > + > +void > +WebAuthnManager::StartSign() { > + MOZ_ASSERT(mChild); same comment about IsActive(). See below. ::: dom/webauthn/WebAuthnManager.cpp:747 (Diff revision 3) > + > +void > +WebAuthnManager::Cancel(const nsresult& aError) > +{ > + if (mChild) { > + mChild->SendRequestCancel(); You should do: if (mChild&& mChild->IsAlive()) { mChild->SendRequestCancel(); } Where isAlive() returns true if ActorDestroyed() has not been called yet. Or you make ActorDestroyed() to nullify the pointer of mChild in WebAuthnManager. Up to you. ::: dom/webauthn/WebAuthnTransactionChild.h:22 (Diff revision 3) > + */ > + > +namespace mozilla { > +namespace dom { > + > +class WebAuthnTransactionChild final : public PWebAuthnTransactionChild { { in a new line ::: dom/webauthn/WebAuthnTransactionChild.h:22 (Diff revision 3) > + */ > + > +namespace mozilla { > +namespace dom { > + > +class WebAuthnTransactionChild final : public PWebAuthnTransactionChild { in general, no virtual for final classes. ::: dom/webauthn/WebAuthnTransactionChild.h:30 (Diff revision 3) > + WebAuthnTransactionChild() {} > + virtual mozilla::ipc::IPCResult RecvConfirmRegister(nsTArray<uint8_t>&& aRegBuffer, > + nsTArray<uint8_t>&& aSigBuffer) override; > + virtual mozilla::ipc::IPCResult RecvConfirmSign(nsTArray<uint8_t>&& aCredentialId, > + nsTArray<uint8_t>&& aBuffer) override; > + virtual mozilla::ipc::IPCResult RecvCancel(const nsresult& aError) override; Implements ActorDestroyed(). We don't want to call Send__delete_ if ActorDestroyed has been called. ::: dom/webauthn/WebAuthnTransactionChild.h:32 (Diff revision 3) > + nsTArray<uint8_t>&& aSigBuffer) override; > + virtual mozilla::ipc::IPCResult RecvConfirmSign(nsTArray<uint8_t>&& aCredentialId, > + nsTArray<uint8_t>&& aBuffer) override; > + virtual mozilla::ipc::IPCResult RecvCancel(const nsresult& aError) override; > +private: > + virtual ~WebAuthnTransactionChild() {} ~WebAuthnTransactionChild() = default;
Attachment #8855585 - Flags: review?(amarchesini) → review+
Comment on attachment 8855586 [details] Bug 1323339 - Add U2FTokenManager class and support IPC Parent classes https://reviewboard.mozilla.org/r/127436/#review134304 ::: dom/webauthn/U2FTokenManager.h:41 (Diff revision 3) > + }; > + NS_INLINE_DECL_REFCOUNTING(U2FTokenManager) > + static U2FTokenManager* GetOrCreate(); > + static U2FTokenManager* Get(); > + void Init(); > + void Register(WebAuthnTransactionParent* aTransactionParent, If you implement unregister() the parent actor doesn't need to be refcounted. ::: dom/webauthn/U2FTokenManager.cpp:108 (Diff revision 3) > +U2FTokenManager::~U2FTokenManager() > +{ > +} > + > +void > +U2FTokenManager::Init() why this? remove it :) ::: dom/webauthn/U2FTokenManager.cpp:169 (Diff revision 3) > + MOZ_ASSERT(NS_IsMainThread()); > + nsTArray<bool> prefs; > + //TODO: Get preferences for token managers we should use. > + return PrefPromise::CreateAndResolve(prefs, __func__); > + })->Then(mOwningThread, __func__, > + [aTransactionType](const nsTArray<bool>& bools) { strange indentation... ::: dom/webauthn/U2FTokenManager.cpp:176 (Diff revision 3) > + // If the manager disappears before we finish out the promise, > + // it'll have already cancelled the operation. Just return. > + if (!mgr) { > + return; > + } > + mgr->Init(); we don't need to initialize it, right? ::: dom/webauthn/U2FTokenManager.cpp:196 (Diff revision 3) > +void > +U2FTokenManager::DoRegister() > +{ > + MOZ_LOG(gU2FTokenManagerLog, LogLevel::Debug, ("U2FAuthRegister")); > + MOZ_ASSERT(mInfo.isSome()); > + Unused << mTransactionParent->SendCancel(NS_ERROR_DOM_NOT_ALLOWED_ERR); What about if this has been destroyed in the meantime? ::: dom/webauthn/U2FTokenManager.cpp:203 (Diff revision 3) > + > +void U2FTokenManager::DoSign() > +{ > + MOZ_LOG(gU2FTokenManagerLog, LogLevel::Debug, ("U2FAuthSign")); > + MOZ_ASSERT(mInfo.isSome()); > + Unused << mTransactionParent->SendCancel(NS_ERROR_DOM_NOT_ALLOWED_ERR); same here. ::: dom/webauthn/U2FTokenTransport.h:19 (Diff revision 3) > + */ > + > +namespace mozilla { > +namespace dom { > + > +class U2FTokenTransport { { in a new line ::: dom/webauthn/U2FTokenTransport.h:32 (Diff revision 3) > + virtual nsresult Sign(nsTArray<uint8_t>& aApplication, > + nsTArray<uint8_t>& aChallenge, > + nsTArray<uint8_t>& aKeyHandle, > + /* out */ nsTArray<uint8_t>& aSignature) = 0; > +protected: > + virtual ~U2FTokenTransport() {} = default ::: dom/webauthn/WebAuthnTransactionParent.h:21 (Diff revision 3) > + */ > + > +namespace mozilla { > +namespace dom { > + > +class WebAuthnTransactionParent final : public PWebAuthnTransactionParent { { in a new line ::: dom/webauthn/WebAuthnTransactionParent.h:30 (Diff revision 3) > + virtual mozilla::ipc::IPCResult > + RecvRequestRegister(const WebAuthnTransactionInfo& aTransactionInfo) override; > + virtual mozilla::ipc::IPCResult > + RecvRequestSign(const WebAuthnTransactionInfo& aTransactionInfo) override; > + virtual mozilla::ipc::IPCResult RecvRequestCancel() override; > + virtual void ActorDestroy(ActorDestroyReason aWhy) override {} This cannot be {}. You must be able to deal with a crash in the content process. ::: dom/webauthn/WebAuthnTransactionParent.h:32 (Diff revision 3) > + virtual mozilla::ipc::IPCResult > + RecvRequestSign(const WebAuthnTransactionInfo& aTransactionInfo) override; > + virtual mozilla::ipc::IPCResult RecvRequestCancel() override; > + virtual void ActorDestroy(ActorDestroyReason aWhy) override {} > +private: > + virtual ~WebAuthnTransactionParent() {} 1. = default 2. no virtual for final classes ::: ipc/glue/BackgroundParentImpl.cpp:877 (Diff revision 3) > } > > +dom::PWebAuthnTransactionParent* > +BackgroundParentImpl::AllocPWebAuthnTransactionParent() > +{ > + RefPtr<dom::WebAuthnTransactionParent> parent = This doesn't need to be refcounted.
Attachment #8855586 - Flags: review?(amarchesini) → review-
Comment on attachment 8855588 [details] Bug 1323339 - Modify WebAuthn mochitests to work with IPC API https://reviewboard.mozilla.org/r/127440/#review134308
Attachment #8855588 - Flags: review?(amarchesini) → review+
Priority: -- → P3
Comment on attachment 8855582 [details] Bug 1323339 - IPDL for WebAuthn Protocol and PBackground additions https://reviewboard.mozilla.org/r/127428/#review133086 > Nit which we can fix in a latter commit to avoid rebasing: let's include the units since this is an ambiguous type (millis, presumably?) I can just take care of that now, rebasing isn't a big deal since I've got other changes to make anyways. > Observation: Using nsresult here seems like it's going to require us to add some kind of mapping between U2F error types and nsresult enumerations. But that's probably fine, and can always change it later. Ok, I designed this with too much info hidden in the parent process I think. I'll file a followup for this.
Comment on attachment 8855585 [details] Bug 1323339 - Add WebAuthnManager and support IPC Child classes https://reviewboard.mozilla.org/r/127434/#review133906 > Nit: It feels a little weird to wipe `mInfo` here but leave `mClientData` populated. Fixed. Made it into a Maybe too, so that we can actually represent "no data". > It'd be good to assert what threads are used here. I put thread checks into Get/GetOrCreate, which, along with RefPtr thread checks, should keep us safe.
Comment on attachment 8855585 [details] Bug 1323339 - Add WebAuthnManager and support IPC Child classes https://reviewboard.mozilla.org/r/127434/#review134298 > You should do: > > if (mChild&& mChild->IsAlive()) { > mChild->SendRequestCancel(); > } > > Where isAlive() returns true if ActorDestroyed() has not been called yet. Or you make ActorDestroyed() to nullify the pointer of mChild in WebAuthnManager. > Up to you. Ok. Went with the ActorDestroyed() option. That should fix the pointer checks that were already in there.
New patches are up. I pushed the U2FTokenManager code for handling U2FSoftTokenManager setup from the (still unfinished :( ) Patch 6 back into Patch 5, in hopes the logic might make a bit more sense.
Comment on attachment 8855587 [details] Bug 1323339 - Add U2FSoftToken Manager https://reviewboard.mozilla.org/r/127438/#review138194 ::: dom/webauthn/U2FSoftTokenManager.cpp:801 (Diff revision 5) > + counterItem.data[3] = (mCounter >> 0) & 0xFF; > + uint32_t counter = mCounter; > + AbstractThread::MainThread()->Dispatch(NS_NewRunnableFunction( > + [counter] () { > + MOZ_ASSERT(NS_IsMainThread()); > + Preferences::SetUint(PREF_U2F_NSSTOKEN_COUNTER, counter); Note: This is potentially racy (if there are multiple requests in-flight) with respect to the counter being monotonic, but that's OK for the test version of the soft token. WebAuthn's spec enforces only one request in-flight at a time, so this should only be a U2F problem anyway.
Attachment #8855587 - Flags: review?(jjones) → review+
Comment on attachment 8855586 [details] Bug 1323339 - Add U2FTokenManager class and support IPC Parent classes https://reviewboard.mozilla.org/r/127436/#review138226 This looks much better, thank you. I don't mind the style nit and TODO going into a final commit if that's easier. ::: dom/webauthn/U2FTokenManager.h:30 (Diff revision 4) > + > +class U2FTokenTransport; > +class U2FSoftTokenManager; > +class WebAuthnTransactionParent; > + > +class U2FTokenManager final { Style nit: U2FSoftTokenManager.h puts the class' open brace on a new line. ::: dom/webauthn/U2FTokenManager.cpp:46 (Diff revision 4) > + > +// Class to avoid having to set up a full observer to listening on the xpcom > +// thread shutdown event. Holds a promise that it resolves on deletion of the > +// object, can be passed to ClearOnShutdown on Main Thread. > +// > +// TODO: Generalize and move to ClearOnShutdown if safe? We should get a bug for this TODO
Attachment #8855586 - Flags: review?(jjones) → review+
Comment on attachment 8855586 [details] Bug 1323339 - Add U2FTokenManager class and support IPC Parent classes https://reviewboard.mozilla.org/r/127436/#review134304 > strange indentation... Tried to fix this, but am not real sure how this should line up.
Comment on attachment 8855586 [details] Bug 1323339 - Add U2FTokenManager class and support IPC Parent classes https://reviewboard.mozilla.org/r/127436/#review138226 > We should get a bug for this TODO yeah, I'm waiting to hear back from baku on that before I move forward with it (that's why the ?). ClearOnShutdown is mostly his code anyways, I just want to make sure I'm not missing anything obvious in whether I should/shouldn't do that.
Comment on attachment 8855586 [details] Bug 1323339 - Add U2FTokenManager class and support IPC Parent classes https://reviewboard.mozilla.org/r/127436/#review137690 Looks good, but I would like to see what you think about the preferences values and the initialize(). ::: dom/webauthn/U2FSoftTokenManager.h:38 (Diff revision 4) > + /* out */ nsTArray<uint8_t>& aSignature) override; > + nsresult IsRegistered(nsTArray<uint8_t>& aKeyHandle, > + nsTArray<uint8_t>& aAppParam, > + bool& aResult); > +private: > + ~U2FSoftTokenManager(); = default? ::: dom/webauthn/U2FTokenManager.h:30 (Diff revision 4) > + > +class U2FTokenTransport; > +class U2FSoftTokenManager; > +class WebAuthnTransactionParent; > + > +class U2FTokenManager final { { in a new line ::: dom/webauthn/U2FTokenManager.h:31 (Diff revision 4) > +class U2FTokenTransport; > +class U2FSoftTokenManager; > +class WebAuthnTransactionParent; > + > +class U2FTokenManager final { > + struct U2FPrefs { same here ::: dom/webauthn/U2FTokenManager.h:53 (Diff revision 4) > + const WebAuthnTransactionInfo& aTransactionInfo); > + void DoRegister(const U2FPrefs& prefs); > + void DoSign(const U2FPrefs& prefs); > + void MaybeClearTransaction(WebAuthnTransactionParent* aParent); > +private: > + U2FTokenManager(); you _must_ set mTransactionParent to null here. ::: dom/webauthn/U2FTokenManager.h:61 (Diff revision 4) > + TransactionType aTransactionType, > + const WebAuthnTransactionInfo& aTransactionInfo); > + void Cancel(const nsresult& aError); > + RefPtr<PrefPromise> mPrefPromise; > + nsTArray<RefPtr<U2FTokenTransport>> mTransports; > + WebAuthnTransactionParent* mTransactionParent; Write a comment explaining why this raw pointer is safe. ::: dom/webauthn/U2FTokenManager.cpp:46 (Diff revision 4) > + > +// Class to avoid having to set up a full observer to listening on the xpcom > +// thread shutdown event. Holds a promise that it resolves on deletion of the > +// object, can be passed to ClearOnShutdown on Main Thread. > +// > +// TODO: Generalize and move to ClearOnShutdown if safe? It is safe if the initialization happens on the main-thread. But how do you want to generalize it? Are you thinking on a "Resolve/RejectOnShutdown()" ? ::: dom/webauthn/U2FTokenManager.cpp:80 (Diff revision 4) > + return gBG->GetPromise(); > + })->Then(mOwningThread, __func__, []() { > + gU2FTokenManager = nullptr; > + }, > + []() { > + }); Usually I have a different approach for this. See: https://dxr.mozilla.org/mozilla-central/source/dom/file/ipc/IPCBlobInputStreamStorage.cpp#32 The singleton is initialized in nsLayoutStatics::Initialize(), that runs on the main-thread. Then I just implement: U2FTokenManager::Get() In Initialize() I call ClearOnShutdown() Would be nice to generalize it, but without a mutex, this becomes racy. Another completely approach would be a ClearOnShutdown thread-safe, able to take a reference of the object, and dispatch a runnable to the main-thread where the 'real' ClearOnShutdown is called. ::: dom/webauthn/U2FTokenManager.cpp:121 (Diff revision 4) > + > +void > +U2FTokenManager::Register(WebAuthnTransactionParent* aTransactionParent, > + const WebAuthnTransactionInfo& aTransactionInfo) > +{ > + this->Run(aTransactionParent, remove this-> ::: dom/webauthn/U2FTokenManager.cpp:130 (Diff revision 4) > + > +void > +U2FTokenManager::Sign(WebAuthnTransactionParent* aTransactionParent, > + const WebAuthnTransactionInfo& aTransactionInfo) > +{ > + this->Run(aTransactionParent, remove this-> ::: dom/webauthn/U2FTokenManager.cpp:136 (Diff revision 4) > + U2FTokenManager::TransactionType::SignTransaction, > + aTransactionInfo); > +} > + > +void > +U2FTokenManager::Run(WebAuthnTransactionParent* aTransactionParent, Maybe 'Execute' instead of 'Run' ? Run sounds like a Runnable class. ::: dom/webauthn/U2FTokenManager.cpp:153 (Diff revision 4) > + // create multiple runnable classes. > + InvokeAsync(AbstractThread::MainThread(), __func__, []() { > + MOZ_ASSERT(XRE_IsParentProcess()); > + MOZ_ASSERT(NS_IsMainThread()); > + U2FTokenManager::U2FPrefs u2fPrefs; > + u2fPrefs.softTokenEnabled = Preferences::GetBool(PREF_WEBAUTHN_SOFTTOKEN_ENABLED); Can we avoid this extra main-thread tasks taking this prefs in the Initialize() function? Maybe if you need to change the prefs value, you can have a mutex to protect those, and have a observer. When they change, you lock, and take the new values. Then add some getters methods to lock + take the cached values. In this way, here you just need to do: if (...) mgr->DoRegister(); else mgr->DoSign(); } and the prefs can be taken synchronously in any thread calling the getters().
Attachment #8855586 - Flags: review?(amarchesini) → review-
Comment on attachment 8855587 [details] Bug 1323339 - Add U2FSoftToken Manager https://reviewboard.mozilla.org/r/127438/#review138764 ::: dom/webauthn/U2FSoftTokenManager.cpp:148 (Diff revision 5) > + /*out*/ UniqueSECKEYPrivateKey& aPrivKey, > + /*out*/ UniqueSECKEYPublicKey& aPubKey, > + const nsNSSShutDownPreventionLock&) > +{ > + MOZ_ASSERT(aSlot); > + if (!aSlot) { NS_WARN_IF ::: dom/webauthn/U2FSoftTokenManager.cpp:153 (Diff revision 5) > + if (!aSlot) { > + return NS_ERROR_INVALID_ARG; > + } > + > + UniquePLArenaPool arena(PORT_NewArena(DER_DEFAULT_CHUNKSIZE)); > + if (!arena) { NS_WARN_IF ::: dom/webauthn/U2FSoftTokenManager.cpp:159 (Diff revision 5) > + return NS_ERROR_OUT_OF_MEMORY; > + } > + > + // Set the curve parameters; keyParams belongs to the arena memory space > + SECItem* keyParams = CreateECParamsForCurve(kEcAlgorithm, arena.get()); > + if (!keyParams) { NS_WARN_IF ::: dom/webauthn/U2FSoftTokenManager.cpp:173 (Diff revision 5) > + PK11_GenerateKeyPair(aSlot.get(), mechanism, keyParams, &pubKeyRaw, > + /* ephemeral */ false, false, > + /* wincx */ nullptr)); > + aPubKey = UniqueSECKEYPublicKey(pubKeyRaw); > + pubKeyRaw = nullptr; > + if (!aPrivKey.get() || !aPubKey.get()) { NS_WARN_IF ::: dom/webauthn/U2FSoftTokenManager.cpp:178 (Diff revision 5) > + if (!aPrivKey.get() || !aPubKey.get()) { > + return NS_ERROR_FAILURE; > + } > + > + // Check that the public key has the correct length > + if (aPubKey->u.ec.publicValue.len != kPublicKeyLen) { NS_WARN_IF :) ::: dom/webauthn/U2FSoftTokenManager.cpp:187 (Diff revision 5) > + return NS_OK; > +} > + > +nsresult > +U2FSoftTokenManager::GetOrCreateWrappingKey(const UniquePK11SlotInfo& aSlot, > + const nsNSSShutDownPreventionLock& locker) indentation ::: dom/webauthn/U2FSoftTokenManager.cpp:190 (Diff revision 5) > +nsresult > +U2FSoftTokenManager::GetOrCreateWrappingKey(const UniquePK11SlotInfo& aSlot, > + const nsNSSShutDownPreventionLock& locker) > +{ > + MOZ_ASSERT(aSlot); > + if (!aSlot) { NS_WARN_IF ::: dom/webauthn/U2FSoftTokenManager.cpp:219 (Diff revision 5) > + /* attributes */ PK11_ATTR_TOKEN | > + PK11_ATTR_PRIVATE, > + /* wincx */ nullptr)); > + > + if (!mWrappingKey) { > + MOZ_LOG(gNSSTokenLog, LogLevel::Warning, indentation ::: dom/webauthn/U2FSoftTokenManager.cpp:227 (Diff revision 5) > + } > + > + SECStatus srv = PK11_SetSymKeyNickname(mWrappingKey.get(), > + mSecretNickname.get()); > + if (srv != SECSuccess) { > + MOZ_LOG(gNSSTokenLog, LogLevel::Warning, indentation ::: dom/webauthn/U2FSoftTokenManager.cpp:251 (Diff revision 5) > + /*out*/ UniqueSECKEYPrivateKey& aAttestPrivKey, > + /*out*/ UniqueCERTCertificate& aAttestCert, > + const nsNSSShutDownPreventionLock& locker) > +{ > + MOZ_ASSERT(aSlot); > + if (!aSlot) { NS_WARN_IF ::: dom/webauthn/U2FSoftTokenManager.cpp:333 (Diff revision 5) > + PLArenaPool* arena = aAttestCert->arena; > + > + srv = SECOID_SetAlgorithmID(arena, &aAttestCert->signature, > + SEC_OID_ANSIX962_ECDSA_SHA256_SIGNATURE, > + /* wincx */ nullptr); > + if (srv != SECSuccess) { yeah.. NS_WARN_IF everywhere you return an error. It makes easier the debugging
Attachment #8855587 - Flags: review?(amarchesini) → review+
Comment on attachment 8855586 [details] Bug 1323339 - Add U2FTokenManager class and support IPC Parent classes https://reviewboard.mozilla.org/r/127436/#review137690 > = default? That changes in the next patch to be more than a default constructor, so just gonna leave it as is. > It is safe if the initialization happens on the main-thread. But how do you want to generalize it? > Are you thinking on a "Resolve/RejectOnShutdown()" ? Just moving things to startup initialization like you recommended. > Can we avoid this extra main-thread tasks taking this prefs in the Initialize() function? > > Maybe if you need to change the prefs value, you can have a mutex to protect those, and have a observer. When they change, you lock, and take the new values. Then add some getters methods to lock + take the cached values. In this way, here you just need to do: > > if (...) > mgr->DoRegister(); > else > mgr->DoSign(); > } > > and the prefs can be taken synchronously in any thread calling the getters(). Yeah, the main goal was to avoid the boilerplate of having to create and return runnable classes. This idea works too and should keep that goal, so I'll go with it.
Comment on attachment 8855586 [details] Bug 1323339 - Add U2FTokenManager class and support IPC Parent classes https://reviewboard.mozilla.org/r/127436/#review140020
Attachment #8855586 - Flags: review?(amarchesini) → review+
Pushed by kmachulis@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c569c8ed68bc IPDL for WebAuthn Protocol and PBackground additions; r=baku,jcj https://hg.mozilla.org/integration/autoland/rev/1ea996669446 Cleanup of WebAuthn DOM classes; r=baku,jcj https://hg.mozilla.org/integration/autoland/rev/8963562767f7 Turn WebAuthenication into a simple DOM class that passes to WebAuthnManager; r=baku,jcj https://hg.mozilla.org/integration/autoland/rev/09ca3b67a359 Add WebAuthnManager and support IPC Child classes; r=baku,jcj https://hg.mozilla.org/integration/autoland/rev/cdc1a8372229 Add U2FTokenManager class and support IPC Parent classes; r=baku,jcj https://hg.mozilla.org/integration/autoland/rev/8e1d29e25f4d Add U2FSoftToken Manager; r=baku,jcj https://hg.mozilla.org/integration/autoland/rev/fa99347a09fb Modify WebAuthn mochitests to work with IPC API; r=baku,jcj
Backed out for bustage at dom/webauthn/WebAuthnManager.cpp:433: https://hg.mozilla.org/integration/autoland/rev/b371eb0faf3ee2418b3d90e55e23f4fd8001213a https://hg.mozilla.org/integration/autoland/rev/0d734918ad2df7ed4b59a63bd6ad9c365176e8b6 https://hg.mozilla.org/integration/autoland/rev/0e1665546b6e5259eca8f9c41fea0adf36969997 https://hg.mozilla.org/integration/autoland/rev/e459e30e34db5d157021c564dd6bf53821a9e51f https://hg.mozilla.org/integration/autoland/rev/5241d15cc4670de6017d7d5a9cc41dbb1e0beb42 https://hg.mozilla.org/integration/autoland/rev/4087c6078b3d0d040e5ef556e626ba6e27549896 https://hg.mozilla.org/integration/autoland/rev/a29a3e8e16bce20e1c0df2d97ea70cc214b0d7d6 Push showing bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=964f96b08cc65c87a432f84914fed12a04aab052&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=97410922&repo=autoland [task 2017-05-08T19:13:19.580119Z] 19:13:19 INFO - In file included from /home/worker/workspace/build/src/obj-firefox/dom/webauthn/Unified_cpp_dom_webauthn0.cpp:74: [task 2017-05-08T19:13:19.580247Z] 19:13:19 INFO - /home/worker/workspace/build/src/dom/webauthn/WebAuthnManager.cpp:433:13: error: Refcounted variable 'this' of type 'mozilla::dom::WebAuthnManager' cannot be captured by a lambda [task 2017-05-08T19:13:19.580289Z] 19:13:19 INFO - this->StartRegister(); [task 2017-05-08T19:13:19.580315Z] 19:13:19 INFO - ^ [task 2017-05-08T19:13:19.580402Z] 19:13:19 INFO - /home/worker/workspace/build/src/dom/webauthn/WebAuthnManager.cpp:433:13: note: Please consider using a smart pointer [task 2017-05-08T19:13:19.580497Z] 19:13:19 INFO - /home/worker/workspace/build/src/dom/webauthn/WebAuthnManager.cpp:436:13: error: Refcounted variable 'this' of type 'mozilla::dom::WebAuthnManager' cannot be captured by a lambda [task 2017-05-08T19:13:19.580538Z] 19:13:19 INFO - this->MaybeClearTransaction(); [task 2017-05-08T19:13:19.580572Z] 19:13:19 INFO - ^ [task 2017-05-08T19:13:19.580639Z] 19:13:19 INFO - /home/worker/workspace/build/src/dom/webauthn/WebAuthnManager.cpp:436:13: note: Please consider using a smart pointer [task 2017-05-08T19:13:19.580725Z] 19:13:19 INFO - /home/worker/workspace/build/src/dom/webauthn/WebAuthnManager.cpp:601:13: error: Refcounted variable 'this' of type 'mozilla::dom::WebAuthnManager' cannot be captured by a lambda [task 2017-05-08T19:13:19.580767Z] 19:13:19 INFO - this->StartSign(); [task 2017-05-08T19:13:19.580804Z] 19:13:19 INFO - ^ [task 2017-05-08T19:13:19.580870Z] 19:13:19 INFO - /home/worker/workspace/build/src/dom/webauthn/WebAuthnManager.cpp:601:13: note: Please consider using a smart pointer [task 2017-05-08T19:13:19.580960Z] 19:13:19 INFO - /home/worker/workspace/build/src/dom/webauthn/WebAuthnManager.cpp:604:13: error: Refcounted variable 'this' of type 'mozilla::dom::WebAuthnManager' cannot be captured by a lambda [task 2017-05-08T19:13:19.581004Z] 19:13:19 INFO - this->MaybeClearTransaction(); [task 2017-05-08T19:13:19.581037Z] 19:13:19 INFO - ^ [task 2017-05-08T19:13:19.581099Z] 19:13:19 INFO - /home/worker/workspace/build/src/dom/webauthn/WebAuthnManager.cpp:604:13: note: Please consider using a smart pointer [task 2017-05-08T19:13:19.581168Z] 19:13:19 INFO - /home/worker/workspace/build/src/dom/webauthn/WebAuthnManager.cpp:777:3: error: 'AddRef' cannot be called on the return value of 'operator->' [task 2017-05-08T19:13:19.581207Z] 19:13:19 INFO - mChild->AddRef(); [task 2017-05-08T19:13:19.583571Z] 19:13:19 INFO - ^ [task 2017-05-08T19:13:19.583622Z] 19:13:19 INFO - 5 errors generated.
Flags: needinfo?(kyle)
Pushed by kmachulis@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c092c00a7907 IPDL for WebAuthn Protocol and PBackground additions; r=jcj r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/af732e815170 Cleanup of WebAuthn DOM classes; r=jcj r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/abdbe7c77b7c Turn WebAuthenication into a simple DOM class that passes to WebAuthnManager; r=jcj r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/d81e2c47f219 Add WebAuthnManager and support IPC Child classes; r=jcj r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/51a430f5d1e5 Add U2FTokenManager class and support IPC Parent classes; r=jcj r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/cbf095629867 Add U2FSoftToken Manager; r=baku r=jcj https://hg.mozilla.org/integration/mozilla-inbound/rev/2f7004fb4631 Modify WebAuthn mochitests to work with IPC API; r=jcj r=baku
Relanded with fixes to static analysis issues. seems to have stuck this time.
Flags: needinfo?(kyle)
Too late to fix for 53. I think it looks a little scary to bring to 54 beta so I'm wontfixing there also. Kyle, if you disagree, let me know (or just request uplift). Thanks!
status-firefox53: affected → wontfix
status-firefox54: --- → wontfix
Flags: needinfo?(kyle)
AFAIK, there's no reason this should need uplift. I think we're aiming for 56 for the API release, so tracking this for 55 should be fine. ni?'ing jcj just to make sure.
Flags: needinfo?(kyle) → needinfo?(jjones)
You'd think I'd know how all this release process works, but I don't. Nevertheless, this patch is relevant for work yet-to-come, and is all aiming to be in by 57, so if we're going to track [webauthn] things, that'd be the right release. But this, yeah, definitely no desire to uplift it.
Flags: needinfo?(jjones)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: