982591 - Update TPS to retrieve keys from server instead of hard-coding them

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

+++ This bug was initially created as a clone of Bug #981921 +++

Once bug 981921 has been fixed, we can update tps to retrieve the real keys from the server. Right now those have been hard-coded:

http://mxr.mozilla.org/mozilla-central/source/services/sync/tps/extensions/tps/modules/fxaccounts.jsm#36

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Wrong dependency order. I will work on this bug once bug 966434 has been finished the refactoring.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Chris, I have a problem here and I don't know how to continue. So I added the code to retrieve the keys. That works fine and I get keyA and wrapKB. I can decode keyA and use it but I fail for wrapKB. Reason is that I need getUserAccountData().unwrapBKey. I'm not sure where to get it from. There is the fxAccounts instance but I cannot access the getUserAccountData() method because it is undefined.

So what shall I do in the following file:
http://mxr.mozilla.org/mozilla-central/source/services/sync/tps/extensions/tps/resource/fxaccounts.jsm#36

What I have right now:

    client.signIn(email, password, true).then(credentials => {
      // Retrieve keys for the signed in account
      client.accountKeys(credentials.keyFetchToken).then((bundle) => {
??? ==> let data = yield fxAccounts.internal.getUserAccountData();
        let kB_hex = CryptoUtils.xor(CommonUtils.hexToBytes(data.unwrapBKey),
                             wrapKB);

        credentials.kA = CommonUtils.bytesAsHex(bundle.kA);
        credentials.kB = CommonUtils.bytesAsHex(kB_hex);

The code in question I got from an existing unit test, but I cannot access fxAccounts.internal here.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Jed, do you have an idea?

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks for the answers on IRC. A patch will be uploaded in a bit.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v1

Comment 6

[Brian Warner [:warner :bwarner]]

Comment on attachment 8394427 [details] [diff] [review]
Patch v1

Review of attachment 8394427 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great! r+ with that one little docs change.

::: services/fxaccounts/FxAccounts.jsm
@@ +366,1 @@
>     *          keyFetchToken: an unused keyFetchToken

while we're at it, let's change this to say "a keyFetchToken which has not yet been used", to avoid implying that this function doesn't use the token. (keyFetchTokens are single-use).

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v2

Updated patch for the jsdoc comment. For safety I pushed it to try. Will land it later when all is green.

https://tbpl.mozilla.org/?tree=Try&rev=92f21914f9ac

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/584c7a6d5a09

If all sticks I will request backports. I think it would be good to even have this patch landed on beta. Brian what do you think?

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/584c7a6d5a09

Comment 10

[Brian Warner [:warner :bwarner]]

(In reply to Henrik Skupin (:whimboo) from comment #8)

> If all sticks I will request backports. I think it would be good to even
> have this patch landed on beta. Brian what do you think?

Eh, if you need it for testing beta, sure, but I don't think anything needs it.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8394600 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): None
User impact if declined: None, its for testing only
Testing completed (on m-c, etc.): Automated tests are passing and TPS can login correctly
Risk to taking this patch (and alternatives if risky): None, most of it is only used by testing code
String or IDL/UUID changes made by this patch: None

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Edwin, what would be your testing strategy in terms of TPS for beta? Do you want to see CI runs for that branch? If yes, we would have to backport this patch to beta.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2bf56d62dadc

Comment 14

[Edwin Wong [:edwong]]

Re: comment #12 - getting TPS running on beta is low priority at this time.  The highest priority is getting TPS running with all tests passing as we want any regressions to be fixed prior to fx29 release. So we don't need CI to run on Beta for the Fx29 release.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks Edwin. I will keep the firefox-29 affected flag, so we can query for non-backported patches later. I will continue with my work on m-c and m-a.