1426305 - Firefox for Android should store derived key material rather than the master key `kB`

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Firefox Accounts, enhancement, P1)

Description

[Ryan Kelly [:rfkelly]]

This is the Android part of Bug 1426304.  Since we will soon start deriving non-sync-related keys from kB, sync clients should avoid storing kB directly, and instead store the minimal set of derived keys necessary to talk to sync:

* 64 bytes for the sync key bundle [1]:

  kSync = HKDF(kB, undefined, "identity.mozilla.com/picl/v1/oldsync", 64)

* 16 bytes for the tokenserver's X-Client-State header [2]:

  kXCS = SHA256(kB)[:16]

Comment 1

[Ryan Kelly [:rfkelly]]

(The [X] footnote references there are a copy-paste fail, please ignore)

Comment 2

[:Grisha Kruglov]

At a first glance, this looks fairly straightforward. We store kB as part of either Cohabiting or Married states; this work is then a two-part effort:
- move state logic away from operating directly on kB and toward operating on the two derived keys
- bump state machine version to provide a transition path for existing clients (read serialized state representation with a kB, produce states with two derived keys instead)

We'll need to ensure that at the end of this transition kB isn't stored on disk as part of the serialized state, but I think regular state machine flow gets us that for free - final state is written to disk, erasing what we had prior.

Comment 3

[:Grisha Kruglov]

Attachment: Bug 1426305 - Migrate FxA state machine to store only derived keys, not kB itself

Review commit: https://reviewboard.mozilla.org/r/208958/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/208958/

Comment 4

[:Grisha Kruglov]

Comment on attachment 8938211 [details]
Bug 1426305 - Migrate FxA state machine to store only derived keys, not kB itself

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208958/diff/1-2/

Comment 5

[Nick Alexander :nalexander [he/him]]

Grisha: I might get to this before January, but I can't guarantee it.  Flag somebody else if it can't wait.  Merry Christmas!

Comment 6

[:Grisha Kruglov]

No worries, there's no rush here at all. Enjoy your holidays, Nick!

Comment 7

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8938211 [details]
Bug 1426305 - Migrate FxA state machine to store only derived keys, not kB itself

https://reviewboard.mozilla.org/r/208958/#review215472

This looks good.  I expected a new state to migrate to, etc, but we built in a migration option with the version codes that works too.

My only thought is that we should store X-Client-State as `byte[]`, since I don't trust `String` and in particular encodings and Java.  (Yes, Android is _always_ UTF-8, but I still don't trust it.)  I'm willing to let it be, though.

Give some thought to what has to happen to `GET /keys` and the interaction with `fxa-content-server` when this code finally accepts these artifacts directly from the server, OAuth style, rather than with the key fetching logic here.  That's clearly the end game.

Finally, it would be a reasonable oxidation target to extract the iOS state machine (it's the cleanest) and glue it into first Android (since they're conceptually the same) and then Desktop (which is much harder, since they share no ideological ancestry).  It seems that the hard part here is abstracting over the network (since we want different libraries across platforms, and Rust's HTTP client is probably not enough for Desktop, which wants Necko) and storage (ditto).  Food for thought.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/login/Engaged.java:72
(Diff revision 2)
> +          kSync = FxAccountUtils.deriveSyncKey(kB);
> +          kXCS = FxAccountUtils.computeClientState(kB);
>            if (FxAccountUtils.LOG_PERSONAL_INFORMATION) {
>              FxAccountUtils.pii(LOG_TAG, "Fetched kA: " + Utils.byte2Hex(result.kA));
>              FxAccountUtils.pii(LOG_TAG, "And wrapkB: " + Utils.byte2Hex(result.wrapkB));
> -            FxAccountUtils.pii(LOG_TAG, "Giving kB : " + Utils.byte2Hex(kB));
> +            FxAccountUtils.pii(LOG_TAG, "Giving derived kSync: " + Utils.byte2Hex(kSync));

I think it's worth logging `kB` here still, since it's the thing that can be compared.  (If the derivations are wrong, you'll immediately want the `kB` logging to figure out what's happened.)

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/login/Married.java:96
(Diff revision 2)
>      }
>      return assertion;
>    }
>  
>    public KeyBundle getSyncKeyBundle() throws InvalidKeyException, NoSuchAlgorithmException, UnsupportedEncodingException {
>      // TODO Document this choice for deriving from kB.

nit: I think this TODO is satisfied.

Comment 8

[:Grisha Kruglov]

(In reply to Nick Alexander :nalexander (less responsive until Jan 3, 2018) from comment #7)
> Comment on attachment 8938211 [details]
> Bug 1426305 - Migrate FxA state machine to store only derived keys, not kB
> itself
> 
> https://reviewboard.mozilla.org/r/208958/#review215472
> 
> This looks good.  I expected a new state to migrate to, etc, but we built in
> a migration option with the version codes that works too.
> 
> My only thought is that we should store X-Client-State as `byte[]`, since I
> don't trust `String` and in particular encodings and Java.  (Yes, Android is
> _always_ UTF-8, but I still don't trust it.)  I'm willing to let it be,
> though.

We'll end up dumping X-Client-State into a String at the point of persistence, and when building our headers, so keeping it in byte[] up to those events is probably of a limited value?

> Finally, it would be a reasonable oxidation target to extract the iOS state
> machine (it's the cleanest) and glue it into first Android (since they're
> conceptually the same) and then Desktop (which is much harder, since they
> share no ideological ancestry).  It seems that the hard part here is
> abstracting over the network (since we want different libraries across
> platforms, and Rust's HTTP client is probably not enough for Desktop, which
> wants Necko) and storage (ditto).  Food for thought.

Indeed, a unified FxA state machine would have definite value. I have a feeling a project akin to what you describe might fit into our medium-term plans - esp. given a parallel "SDK-all-the-things" effort by the FxA team.

Comment 9

[:Grisha Kruglov]

Comment on attachment 8938211 [details]
Bug 1426305 - Migrate FxA state machine to store only derived keys, not kB itself

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208958/diff/2-3/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a05a37c15ce1
Migrate FxA state machine to store only derived keys, not kB itself r=nalexander

Comment 11

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/a05a37c15ce1

Comment 12

[Vlad Filippov (:vladikoff)]

Awesome that this landed! Thank you!

Comment 13

[Ryan Kelly [:rfkelly]]

In IRC discussion, it came up that this change might cause us issues if we ever want to add the `chrome.storage.sync` API on mobile.  Desktop will be storing those keys as a separate derived secret [1] but here on Android, we'd have to ask the user to sign in again in order to get the new derived secrets.

Are we likely to ever want to add `chrome.storage.sync` on Andoid, and if so, should we modify this to store the necessary derived secrets just in case?

(ni? :grisha in the first instance, but I don't really know who I should direct the "are we likely to..?" part of that question towards)

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1426306

Comment 14

[Nick Alexander :nalexander [he/him]]

(In reply to Ryan Kelly [:rfkelly] from comment #13)
> In IRC discussion, it came up that this change might cause us issues if we
> ever want to add the `chrome.storage.sync` API on mobile.  Desktop will be
> storing those keys as a separate derived secret [1] but here on Android,
> we'd have to ask the user to sign in again in order to get the new derived
> secrets.

I thought about this while reviewing, and concluded that we _want_ the user to sign in (using an OAuth flow) for each new service that they use.  That is, you shouldn't be able to silently opt a user in to a potentially information-leaking service without a credential verification loop.

To me, this is a big part of the OAuth flow story, and why I've tried to support the "get keys via OAuth" work where possible.  We need to uniformize this stuff and stop special casing Sync quite so much, and this is a step in that direction.

Just FYI: Android's FxA implementation has support for OAuth scopes, etc, and we use them for profile service authentication.  It's not robust and would need a lot of work to add new services, but it's guided some of the decisions we made.

> Are we likely to ever want to add `chrome.storage.sync` on Andoid, and if
> so, should we modify this to store the necessary derived secrets just in
> case?

I think yes, we should eventually implement this on mobile.

> (ni? :grisha in the first instance, but I don't really know who I should
> direct the "are we likely to..?" part of that question towards)

andym for the technical side right now, and abovens for product direction, perhaps?

> 
> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1426306

Comment 15

[Ryan Kelly [:rfkelly]]

> we _want_ the user to sign in (using an OAuth flow) for each new service that they use.

Agreed.  The one potential wrinkle in this case is that `chrome.storage.sync` is a bit of a hybrid - a separate service using its own keys and oauth token, but conceptually "part of sync" from the user's perspective :-/

Comment 16

[:Grisha Kruglov]

I think it is an unfortunate oversight on my part that this patch landed without an easy path forward for 'chrome.storage.sync' that doesn't involve a rather convoluted UX flow. I can't think of a non-terrible user experience that involves installing a WE which uses chrome.storage.sync, which is supposed to sync against a logged-in Firefox Account, and that requires user to login into their Firefox Account before it becomes functional. Or wait until the next sync (so that keys are derived). Perhaps we could kick-off a sync at the point of installation of a WE w/ 'storage' permission, and derive keys whenever we see a kB. Still, very clumsy.

This isn't a problem if we never support chrome.storage.sync for WE on Fennec, or if that support comes as part of another re-incarnation of Fennec.

AFAIK, there are no concrete plans to implement chrome.storage.sync support for Fennec - but that's largely a function of resource allocation & perceived product benefit, and an overall state of WEs on mobile. Personally, I'd love to see it happen - this has a potential of unblocking tight, integrated community-driven user work flows.

As for the matter at hand, I think we have four options:
0) consider 'chrome.storage.sync' an entirely separate entity, as Nick suggests, and figure out a decent flow when the time comes.
1) consider the API intrinsically tied to Sync, but ignore the problem, placing a bet that mobile support for 'chrome.storage.sync' won't be added, not soon enough to matter, or that we'll work out the quirky UX around this in some way (see first paragraph).
2) extend this patch to derive & store the necessary key (simple), and take care of the already-upgraded Nightly users, somehow. The simplest way to do that, I think, is to conditionally move those users into a Separated state on upgrade, forcing a login.

I'm leaning toward (2), thinking that a little bit of work now can save a headache in the future.

Comment 17

[Nick Alexander :nalexander [he/him]]

(In reply to :Grisha Kruglov from comment #16)
> I think it is an unfortunate oversight on my part that this patch landed
> without an easy path forward for 'chrome.storage.sync' that doesn't involve
> a rather convoluted UX flow. I can't think of a non-terrible user experience
> that involves installing a WE which uses chrome.storage.sync, which is
> supposed to sync against a logged-in Firefox Account, and that requires user
> to login into their Firefox Account before it becomes functional. Or wait
> until the next sync (so that keys are derived). Perhaps we could kick-off a
> sync at the point of installation of a WE w/ 'storage' permission, and
> derive keys whenever we see a kB. Still, very clumsy.
> 
> This isn't a problem if we never support chrome.storage.sync for WE on
> Fennec, or if that support comes as part of another re-incarnation of Fennec.
> 
> AFAIK, there are no concrete plans to implement chrome.storage.sync support
> for Fennec - but that's largely a function of resource allocation &
> perceived product benefit, and an overall state of WEs on mobile.
> Personally, I'd love to see it happen - this has a potential of unblocking
> tight, integrated community-driven user work flows.
> 
> As for the matter at hand, I think we have four options:
> 0) consider 'chrome.storage.sync' an entirely separate entity, as Nick
> suggests, and figure out a decent flow when the time comes.
> 1) consider the API intrinsically tied to Sync, but ignore the problem,
> placing a bet that mobile support for 'chrome.storage.sync' won't be added,
> not soon enough to matter, or that we'll work out the quirky UX around this
> in some way (see first paragraph).
> 2) extend this patch to derive & store the necessary key (simple), and take
> care of the already-upgraded Nightly users, somehow. The simplest way to do
> that, I think, is to conditionally move those users into a Separated state
> on upgrade, forcing a login.
> 
> I'm leaning toward (2), thinking that a little bit of work now can save a
> headache in the future.

Is it the case that the set of derivations for 'c.s.s' is already known?  Then sure, let's save that bit of information to ease transition in the future.  But clearly the set of derivations is not closed and known completely, so we shouldn't work too hard to make this happen.

In general, I think that a Web Extension that wants to use 'c.s.s' should have to do something to get the user to opt-in to that syncing, possibly including an OAuth flow via the browser, but perhaps that ship has sailed.

Comment 18

[Ryan Kelly [:rfkelly]]

> andym for the technical side right now, and abovens for product direction, perhaps?

+Andy and +Andreas for context; please see Comment 13 for the initial question.

> 1) consider the API intrinsically tied to Sync, but ignore the problem, placing a bet that mobile
> support for 'chrome.storage.sync' won't be added, not soon enough to matter, or that we'll work
> out the quirky UX around this in some way (see first paragraph)

I'm leaning towards this option, mostly because I think we'll eventually need to solve a generalized version of this quirky UX problem anyway.  If we add syncing of a new data type, we really should find some way to ask the user's permission before enabling it, and it's a short step from there to prompting them for their password.

That said...

> 2) extend this patch to derive & store the necessary key (simple), and take care of the already-upgraded Nightly users,
> somehow. The simplest way to do that, I think, is to conditionally move those users into a Separated state on upgrade,
> forcing a login.

There's likely no harm in doing this just in case (apart from the slightly dirty feeling of storing secrets that we don't need).  Perhaps we could even delay the "move users without these keys into a Separated state" part until if and when we actually need those keys, at which point we might have some better UX for them anyway?

Comment 19

[Andy McKay]

(In reply to Ryan Kelly [:rfkelly] from comment #18)
> > andym for the technical side right now, and abovens for product direction, perhaps?
> 
> +Andy and +Andreas for context; please see Comment 13 for the initial
> question.

We would like to implement storage.sync on Android, that's bug 1316442. 

However even at P3 at this point it's pretty low on our priorities to implement and if we did implement it we'd likely need the help of a bunch of people on this bug, so you'd get plenty of notice. I would be surprised if we worked it on in 2018 to be honest.