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

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Richard Newman [:rnewman]]

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.

Comment 1

[Ryan Kelly [:rfkelly]]

linking the bug with the spec in it for completeness

Comment 2

[Jed Parsons (use needinfo, please) [:jedp, :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

Comment 3

[Chris Karlof [:ckarlof]]

Related: https://github.com/mozilla-services/tokenserver/issues/25

Comment 4

[Chris Karlof [:ckarlof]]

(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)))

Comment 5

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: 959919-x-client-state-header.patch

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?

Comment 6

[Nick Alexander :nalexander [he/him]]

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.

Comment 7

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

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

Comment 8

[Chris Karlof [:ckarlof]]

Things changed!

LOWERCASE_HEX_ENCODE(FIRST-16-BYTES(SHA256(kB)))

Comment 9

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Send X-Client-State header

Small change in the encoding scheme (per comment 8)

Otherwise, same summary as in comment 7.

Leaving in the fix to encodeBase64Url.

Thanks!
j

Comment 10

[Richard Newman [:rnewman]]

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.

Comment 11

[Chris Karlof [:ckarlof]]

(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);

Comment 12

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Thank you both.  I got myself mixed up yesterday.  Fixing now.

Comment 13

[Richard Newman [:rnewman]]

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.
  }

Comment 14

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Excellent.  Thanks, Richard.

Comment 15

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Send X-Client-State header

Comment 16

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

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

Comment 17

[Richard Newman [:rnewman]]

Comment on attachment 8367640 [details] [diff] [review]
Send X-Client-State header

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

Looks good!

Comment 18

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Sweet.  Thank you, Richard.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/9ff6690506a6

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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