Closed Bug 959919 Opened 6 years ago Closed 6 years ago

Adapt FxA token server client to include X-Client-State header

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: rnewman, Assigned: jedp)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 2 obsolete files)

Desktop equivalent of Android's Bug 959915.

This will allow us to receive a new node assignment when our generated key changes.

[qa-], because this will be tested implicitly.
linking the bug with the spec in it for completeness
Depends on: 959441
Assignee: nobody → jparsons
The conclusion of this sync-dev thread

  https://mail.mozilla.org/pipermail/sync-dev/2014-January/000764.html

leads me to believe that I should send this with every request to the token server:

  HKDF(BASE64URLSAFE(FIRST-16-BYTES(SHA256(kB))), salt=emailUTF8, context=KW("X-Client-State"), 16)

Is that correct?  Is there a more spec-like spec somewhere that I ought to refer to?
Thanks,
j
Flags: needinfo?(nalexander)
(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #2)
> The conclusion of this sync-dev thread
> 
>   https://mail.mozilla.org/pipermail/sync-dev/2014-January/000764.html
> 
> leads me to believe that I should send this with every request to the token
> server:
> 
>   HKDF(BASE64URLSAFE(FIRST-16-BYTES(SHA256(kB))), salt=emailUTF8,
> context=KW("X-Client-State"), 16)
> 
> Is that correct?  Is there a more spec-like spec somewhere that I ought to
> refer to?
> Thanks,
> j

No spec. Things are crazy!

Let's do Nick's proposal:

https://mail.mozilla.org/pipermail/sync-dev/2014-January/000772.html

BASE64URLSAFE(FIRST-16-BYTES(SHA256(kB)))
Flags: needinfo?(nalexander)
Confirmed with rfk on irc that the resulting header looks like what he'd like

Nick, how does this look as a way to compute the header?  Is this what you're doing on Android?
Attachment #8367031 - Flags: feedback?(nalexander)
Comment on attachment 8367031 [details] [diff] [review]
959919-x-client-state-header.patch

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

Yeah, looks fine.  I'll test with this vector on Android.
Attachment #8367031 - Flags: feedback?(nalexander) → feedback+
Comment on attachment 8367031 [details] [diff] [review]
959919-x-client-state-header.patch

Hi, Gregory, do you mind taking a look at this?

Summary:
- Adds an X-Client-State header
- removes unused conditionsAccepted arg (that's done by fxa now)
- provides way to add arbitrary headers to getToken request
- fixes bug in common utils encodeBase64URL
- adds tests for sha256 and header computation

thanks!
j
Attachment #8367031 - Flags: review?(gps)
Things changed!

LOWERCASE_HEX_ENCODE(FIRST-16-BYTES(SHA256(kB)))
Attached patch Send X-Client-State header (obsolete) — Splinter Review
Small change in the encoding scheme (per comment 8)

Otherwise, same summary as in comment 7.

Leaving in the fix to encodeBase64Url.

Thanks!
j
Attachment #8367031 - Attachment is obsolete: true
Attachment #8367031 - Flags: review?(gps)
Attachment #8367045 - Flags: review?(gps)
Comment on attachment 8367045 [details] [diff] [review]
Send X-Client-State header

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

::: services/sync/modules/browserid_identity.js
@@ +67,5 @@
> +   * Compute the X-Client-State header given kB.
> +   * Return HEX(FIRST-16-BYTES(SHA256(kB)))
> +   */
> +  _computeXClientState: function(kB) {
> +    return CommonUtils.bytesAsHex(this._sha256(kB).slice(0, 16), false);

This function expects kB to be a byte array, because CryptoUtils.digestBytes does.

@@ +317,5 @@
>    _fetchTokenForUser: function(userData) {
>      let tokenServerURI = Svc.Prefs.get("tokenServerURI");
>      let log = this._log;
>      let client = this._tokenServerClient;
> +    let headers = {"X-Client-State": this._computeXClientState(userData.kB)};

If userData.kB is hex-encoded here, make sure you decode it.

::: services/sync/tests/unit/test_browserid_identity.js
@@ +137,5 @@
> +  let kB = "fd5c747806c07ce0b9d69dcfea144663e630b65ec4963596a22f24910d7dd15d";
> +  let bidUser = new BrowserIDManager();
> +  do_check_eq(kB.length, 64);
> +
> +  let header = bidUser._computeXClientState(kB);

But this test is passing in a 64-'byte' string. Which is wrong. You need to hex-decode the kB test string here.
Attachment #8367045 - Flags: review?(gps) → review-
(In reply to Richard Newman [:rnewman] from comment #10)
> @@ +317,5 @@
> >    _fetchTokenForUser: function(userData) {
> >      let tokenServerURI = Svc.Prefs.get("tokenServerURI");
> >      let log = this._log;
> >      let client = this._tokenServerClient;
> > +    let headers = {"X-Client-State": this._computeXClientState(userData.kB)};
> 
> If userData.kB is hex-encoded here, make sure you decode it.

Jed, do as _fetchSyncKeyBundle does it:

>      // both Jelly and FxAccounts give us kA/kB as hex.
>      let kB = Utils.hexToBytes(userData.kB);
Thank you both.  I got myself mixed up yesterday.  Fixing now.
This is the test I'm using on Android.

  @Test
  public void testClientState() throws Exception {
    final String hexKB = "fd5c747806c07ce0b9d69dcfea144663e630b65ec4963596a22f24910d7dd15d";
    final byte[] byteKB = Utils.hex2Byte(hexKB);
    final String clientState = FxAccountUtils.computeClientState(byteKB);
    final String expected = "6ae94683571c7a7c54dab4700aa3995f";
    Assert.assertEquals(expected, clientState);
  }

with

  public static String computeClientState(byte[] kB) throws NoSuchAlgorithmException {
    byte[] sha256 = Utils.sha256(kB);
    byte[] truncated = new byte[16];
    System.arraycopy(sha256, 0, truncated, 0, 16);
    return Utils.byte2Hex(truncated);    // Already lowercase.
  }
Excellent.  Thanks, Richard.
Comment on attachment 8367640 [details] [diff] [review]
Send X-Client-State header

Hi, Richard,

I think this straightens out the encoding snafu.  Is it looking better now?

Thanks for the test vectors and for your review,
j
Attachment #8367640 - Flags: review?(rnewman)
Comment on attachment 8367640 [details] [diff] [review]
Send X-Client-State header

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

Looks good!
Attachment #8367640 - Flags: review?(rnewman) → review+
Sweet.  Thank you, Richard.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/9ff6690506a6
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [qa-] → [qa-][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/9ff6690506a6
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [qa-][fixed-in-fx-team] → [qa-]
Target Milestone: --- → mozilla29
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.