Closed Bug 982591 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox29 --- wontfix
firefox30 --- fixed
firefox31 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

Attachments

(1 file, 1 obsolete file)

+++ 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
Wrong dependency order. I will work on this bug once bug 966434 has been finished the refactoring.
No longer blocks: 966434
Depends on: 966434
Summary: Update TPS to retrieve keys from server instead of hard-codind them → Update TPS to retrieve keys from server instead of hard-coding them
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.
Flags: needinfo?(ckarlof)
Jed, do you have an idea?
Flags: needinfo?(jparsons)
Thanks for the answers on IRC. A patch will be uploaded in a bit.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Flags: needinfo?(jparsons)
Flags: needinfo?(ckarlof)
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #8394427 - Flags: review?(warner-bugzilla)
Blocks: 986190
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).
Attachment #8394427 - Flags: review?(warner-bugzilla) → review+
Attached patch Patch v2Splinter Review
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
Attachment #8394427 - Attachment is obsolete: true
Attachment #8394600 - Flags: review+
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?
Flags: needinfo?(warner-bugzilla)
Flags: in-testsuite+
Target Milestone: --- → mozilla31
https://hg.mozilla.org/mozilla-central/rev/584c7a6d5a09
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
(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.
Flags: needinfo?(warner-bugzilla)
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
Attachment #8394600 - Flags: approval-mozilla-aurora?
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.
Flags: needinfo?(edwong)
Attachment #8394600 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Flags: needinfo?(edwong)
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.
Duplicate of this bug: 987091
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.