1490671 - [Device Pairing Phase 1] Desktop Auth

Status: VERIFIED FIXED

(Firefox :: Sync, defect, P1)

Description

[Edouard Oger [:eoger]]

Attachment: Implementation plan

Let's implement a prototype with Firefox Desktop as Auth (supplier of keys).
See the milestone 3 from the "Implementation plan" attachment for more information.

Comment 1

[Ryan Kelly [:rfkelly]]

This work will also need a fxa-content-server patch, for which I've filed an issue here:

  https://github.com/mozilla/fxa-content-server/issues/6546

Comment 2

[Edouard Oger [:eoger]]

(In reply to Ryan Kelly [:rfkelly] from comment #1)
> This work will also need a fxa-content-server patch, for which I've filed an
> issue here:
> 
>   https://github.com/mozilla/fxa-content-server/issues/6546

Sounds good, let's update the prototype later once this is ready.

Comment 3

[Edouard Oger [:eoger]]

Attachment: Bug 1490671 - wip prototype

Comment 4

[Edouard Oger [:eoger]]

Attachment: Bug 1490671 p3 - Add FxA device pairing

Comment 5

[Edouard Oger [:eoger]]

Attachment: Bug 1490671 p1 - Add toolkit qrcode component

Comment 6

[Edouard Oger [:eoger]]

Attachment: Bug 1490671 p1 - Refactor CryptoUtils and add JWK/JWE methods to jwcrypto.

Depends on D6964

Comment 7

[Edouard Oger [:eoger]]

Attachment: Bug 1490671 - Add FxA device pairing.

Depends on D6965

Comment 8

[Edouard Oger [:eoger]]

TODO:
- add "fxaccounts:pair_heartbeat" message handling
- use channelID as base64url bytes.
- On decryption error -> display an error to the user

Comment 9

[Edouard Oger [:eoger]]

Patch updated with all review comments and the comment 8's todos.
Not really asking for review yet, as I'd like to test it with Shane's content server branch first.

Comment 10

[Ryan Kelly [:rfkelly]]

Paul, the "p1" patch in this bug is doing a refactor of some of the crypto-related code for Firefox Sync, and adding some utilities for working with JWEs.  Who should we get to take a look at this from a security perspective?

Comment 11

[Edouard Oger [:eoger]]

Thanks for the review Ryan, I've updated P1 with your comments.
P2 also got updated with some stuff but is in a limbo state until we decide on the protocol.
I think it would be fine to land p1 when we're ready in the meantime as it cleans a lot of things up.

Comment 12

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Ryan Kelly [:rfkelly] from comment #10)
> Paul, the "p1" patch in this bug is doing a refactor of some of the
> crypto-related code for Firefox Sync, and adding some utilities for working
> with JWEs.  Who should we get to take a look at this from a security
> perspective?

Hey Ryan, we can, to  start with at least. We have a security testing bug open for this in 1500024 but I haven't had a chance to look at it yet. I'll get someone on it this week, and bring in other security teams as necessary (we might need someone from  JC's team here perhaps, not sure).

Comment 13

[Edouard Oger [:eoger]]

Things to do before landing:

- Finish UX work: intro screen, error screen, strings.
- Make the pairing flow Nightly only: make it impossible to be enabled on other release channels.
- Cleanup (e.g. Remove the old key-data stuff)

Unless Shane objects I believe these patches are ready for formal review.

Comment 14

[Shane Tomlinson [:stomlinson]]

(In reply to Edouard Oger [:eoger] from comment #13)

> Unless Shane objects I believe these patches are ready for formal review.

Absolutely no objections from me!

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

There is a small chance this feature will be turned on in Nightly 65 and will be behind a nightly-only flag.

Comment 16

[Edouard Oger [:eoger]]

Attachment: Bug 1490671 p3 - Make FxA Pairing nightly only.

Depends on D6966

Comment 17

[Edouard Oger [:eoger]]

Patch updated. Things left to do:
- UX work: intro screen, error screens.
- Strings.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Safe to say this isn't targeting 65 at this point?

Comment 19

[Edouard Oger [:eoger]]

Yup probably 66

Comment 20

[Edouard Oger [:eoger]]

We're still trying to land this in 66.

Comment 21

[Edouard Oger [:eoger]]

Updated with the new crypto lib (do not land).
Things to do:
- Check compat with content server
- Update tests for both p1 and p2.

Comment 22

[Edouard Oger [:eoger]]

Patches updated with revamped UX [0] and latest protocol changes [1].
This is ready for final review.

[0] https://www.lucidchart.com/documents/view/9a420c19-1e92-42a5-8eae-908a442c1044/1
[1] https://www.lucidchart.com/documents/edit/1bc1b604-0047-4542-8827-ed8518b0433e

Comment 23

[Ryan Kelly [:rfkelly]]

Something that just occurred to me here is, do we need to make any changes to the handling of the "fxa_status" webchannel message as part of this work?

When accounts.firefox.com loads in the browser, it sends an "fxa_status" webchannel message to the browser in order to sync up the login state between the browser and web content. The browser code for handling this is here:

https://dxr.mozilla.org/mozilla-central/rev/c2593a3058afdfeaac5c990e18794ee8257afe99/services/fxaccounts/FxAccountsWebChannel.jsm#183

We currently have some logic that ignores this message when in private browsing mode, except if the user is trying to access account management from the sync settings page:

https://dxr.mozilla.org/mozilla-central/rev/c2593a3058afdfeaac5c990e18794ee8257afe99/services/fxaccounts/FxAccountsWebChannel.jsm#

IIUC, the pairing authority broker will depend on the "fxa_status" webchannel message in order to ensure it's using the same sessionToken as the browser itself, to do things like check the user's 2FA status. Do we need to add any logic to ensure that "fxa_status" is handled correctly when the user does the pairing flow in a private browsing window?

What we definitely do not want, is to have web content using one sessionToken for its side of the authority flow, and the browser using a different sessionToken for its side of the flow.

Vlad, thoughts on what might be necessary here from the web-content side of things?

Comment 24

[Vlad Filippov (:vladikoff)]

(In reply to Ryan Kelly [:rfkelly] from comment #23)

We looked at this and it looks pretty good. Basically the plan is to add a signal that it's a pairing flow (just like we signal 'sync' at this moment) for the fxa status message.
I'm tracking this as follow up PR to content server and a small patch to desktop. We also want to make sure to land it before pairing hits release. :eoger said it should be okay to land that fix separately as well.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/74922ea29d44
Add FxA device pairing. r=markh,rfkelly,vladikoff,flod

Comment 26

[Dorel Luca [:dluca]]

Backed out changeset 74922ea29d44 (bug 1490671) for browser-chrome failures in browser/components/preferences/in-content/tests/browser_sync_pairing.js

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=229335326&repo=autoland&lineNumber=37953

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=74922ea29d44e5a45eb54da31f0329c326e4588c

Backout:
https://hg.mozilla.org/integration/autoland/rev/66a777b305c798b6a788af47e59ab05964b569da

Comment 27

[Ryan Kelly [:rfkelly]]

FYI made a v1.0.1 release of fxa-pairing-channel with some small tweaks based on latest discussions:

https://github.com/mozilla/fxa-pairing-channel/releases/tag/v1.0.1

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9dea142f2cc0
Add FxA device pairing. r=markh,rfkelly,vladikoff,flod

Comment 29

[Edouard Oger [:eoger]]

Attachment: Bug 1490671 - (Follow-up) Apply some recent subdialog fixes.

Comment 30

[Edouard Oger [:eoger]]

Clearing ni?: Re-landed with test fixes and v1.0.1 of the fxa-pairing-channel.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/050422e845df
(Follow-up) Apply some recent subdialog fixes. r=vladikoff

Comment 32

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/9dea142f2cc0

Comment 33

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/050422e845df

Comment 34

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/050422e845df

Comment 35

[Liz Henry (:lizzard)]

Is this intended to ride with 67 to release or are you looking for beta uplift?

Comment 36

[Liz Henry (:lizzard)]

Wontfixing for now but let me know if you feel strongly that it needs to be in 66.

Comment 37

[Edouard Oger [:eoger]]

No uplift scheduled, thank you!

Comment 38

[Daniel Bodea [:danibodea]]

How exactly am I supposed to use this feature? Where do I find this information?
Information needed for validating this bug.

Comment 39

[Vlad Filippov (:vladikoff)]

(In reply to Bodea Daniel [:danibodea] from comment #38)

How exactly am I supposed to use this feature? Where do I find this information?
Information needed for validating this bug.

1. Download Android Reference Browser builds from the following:

arm: https://s3-us-west-2.amazonaws.com/fxa-dev-bucket/fenix-pair/mar18/app-geckoNightly-arm-release.apk
x86: https://s3-us-west-2.amazonaws.com/fxa-dev-bucket/fenix-pair/mar18/app-geckoNightly-x86-release.apk
arm64: https://s3-us-west-2.amazonaws.com/fxa-dev-bucket/fenix-pair/mar18/app-geckoNightly-aarch64-release.apk

2. Create a new Firefox Nightly Desktop profile, flip this pref to true: identity.fxaccounts.pairing.enabled

3. Login to Firefox Sync in Desktop

4. Open the Reference Browser Android, go to Settings -> Pair.

Here's a video guide from an older build: https://www.youtube.com/watch?v=hN-mpyEC7Pg

Comment 40

[Bogdan Surd, QA [:bsurd, NI]]

Hello,

Tested this with the latest builds and there are no major issues with this feature.

Could not test with the x86 device as it's got some technical problems but everything else worked as expected.