Closed Bug 689428 Opened 8 years ago Closed 8 years ago

Implement KeyExchange v3 in jpakeclient.js

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: philikon, Assigned: philikon)

References

Details

(Whiteboard: [fixed in services][qa-])

Attachments

(3 files, 5 obsolete files)

For "J-PAKE first" (bug 675823) we need to implement the KeyExchange protocol v3 (as spec'ed out in bug 684074).
Attachment #562644 - Flags: review?(rnewman)
Attachment #562644 - Attachment description: Part 1 (v1): Fix style in jpakeclient.js → Part 0 (v1): Fix style in jpakeclient.js
This implements the KeyExchange v3 draft spec (bug 684074). It also implements the additions in v2 now (sending and processing the right ETag bits for PUT requests).

This is fully functional, it's just missing unit tests for edge cases that were added in v2 and v3.
Attachment #562648 - Flags: feedback?(rnewman)
This makes the Firefox UI work with the new JPAKEClient API. Since Fennec doesn't support sending credentials atm, it doesn't need to be updated.

Note that this doesn't actually implement "J-PAKE first" yet. This merely fixes the UI so it doesn't break with the API changes. Bug 675823 will do the "J-PAKE first" bit, but with this we can at least land and test these changes.
Attachment #562649 - Flags: review?(rnewman)
Attachment #562644 - Flags: review?(rnewman) → review+
Comment on attachment 562648 [details] [diff] [review]
Part 1 (WIP): Implement KeyExchange v3

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

Looking good. Changes seem broadly non-intrusive, so I'll gladly give f+ without going over the implementation alongside protocol docs, but I'll do that tomorrow. Should be a pretty straightforward r+ when the tests are done.

::: services/sync/modules/constants.js
@@ +184,5 @@
>  JPAKE_ERROR_NODATA:                    "jpake.error.nodata",
>  JPAKE_ERROR_KEYMISMATCH:               "jpake.error.keymismatch",
>  JPAKE_ERROR_WRONGMESSAGE:              "jpake.error.wrongmessage",
>  JPAKE_ERROR_USERABORT:                 "jpake.error.userabort",
> +JPAKE_ERROR_DELAY_NOT_SUPPORTED:       "jpake.error.delay_not_supported",

Humph. I don't like the inconsistency_between thetwonamingconventions. I prefer the underscores, but I don't know if the old error codes can be changed…

::: services/sync/modules/jpakeclient.js
@@ +83,5 @@
> + *                                   | verify against local known value
> + *
> + *   At this point Desktop knows whether the PIN was entered correctly.
> + *   If it wasn't, Desktop deletes the session. If it was, the account
> + *   setup can proceed. If Desktop doesn't have an account set up yet,

Move "yet" to before "have", please.

@@ +107,5 @@
>   *     that didn't provide the PIN.
>   * 
> + *   onPaired() -- Called when the device pairing has been established and
> + *     we're ready to send the credentials over. To do that, the controller
> + *     can (and should) call the 'sendAndComplete()' at any time after this.

"the 'sendAndComplete()'" -- "method" missing, or "the" is surplus.

"can (and should)" seems ambiguous, and "at any time" seems incorrect (timeouts?). I'd rather see

  "the controller must call 'sendAndComplete()' or abort the session within the timeout duration"

or whatever else is strictly true.

(Sorry to be nitpicky, but docs are important!)

@@ +209,5 @@
>                  this._complete)();
>    },
>  
> +  /**
> +   * Initiative pairing based on the PIN entered by the user.

s/Initiative/Initiate

@@ +215,5 @@
> +   * This is typically called on desktop devices where typing is easier than
> +   * on mobile.
> +   * 
> +   * @param pin
> +   *        12 character string (in human friendly base32) containing the PIN

s/human /human-

@@ +291,5 @@
>      }
>  
>      if (error == JPAKE_ERROR_CHANNEL
>          || error == JPAKE_ERROR_NETWORK
>          || error == JPAKE_ERROR_NODATA) {

Can we rejig these to follow the "break after operator" style? (In the cleanup patch, ideally.)

::: services/sync/tests/unit/test_jpakeclient.js
@@ +11,5 @@
>   * Simple server.
>   */
>  
>  function check_headers(request) {
> +  let stack = Components.stack.caller;

Ooh! New trick!

*steals*

@@ +181,5 @@
>    let rec = new JPAKEClient({
>      displayPIN: function displayPIN(pin) {
>        _("Received PIN " + pin + ". Entering it in the other computer...");
>        this.cid = pin.slice(JPAKE_LENGTH_SECRET);
> +      Utils.nextTick(function() { snd.pairWithPIN(pin); });

I gently prefer the explicit "false" as the second argument to pairWithPIN. No big deal if you disagree.

@@ +263,5 @@
>        secret = [char for each (char in secret)].reverse().join("");
>        let new_pin = secret + this.cid;
>        _("Received PIN " + pin + ", but I'm entering " + new_pin);
>  
> +      Utils.nextTick(function() { snd.pairWithPIN(new_pin); });

Ditto.

@@ +348,5 @@
>      },
>      displayPIN: function displayPIN(pin) {
>        _("Received PIN " + pin + ". Entering it in the other computer...");
>        this.cid = pin.slice(JPAKE_LENGTH_SECRET);
> +      Utils.nextTick(function() { snd.pairWithPIN(pin); });

Ditto.

@@ +360,5 @@
>  
>  add_test(function test_wrongmessage() {
>    let cid = new_channel();
> +  let channel = channels[cid];
> +  channel.data = JSON.stringify({type: "receiver2", version: 3, payload: {}});

Constant for version, plz.

::: services/sync/tests/unit/xpcshell.ini
@@ +42,5 @@
>  [test_interval_triggers.js]
>  [test_jpakeclient.js]
>  # Bug 618233: this test produces random failures on Windows 7.
>  # Bug 676978: test hangs on Android (see also testing/xpcshell/xpcshell.ini)
> +#skip-if = os == "win" || os == "android"

*ahem*
Attachment #562648 - Flags: feedback?(rnewman) → feedback+
Comment on attachment 562649 [details] [diff] [review]
Part 2 (v1): Update Firefox UI to the new JPAKEClient API

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

::: browser/base/content/syncAddDevice.js
@@ +112,5 @@
> +      onPaired: function onPaired() {
> +        let credentials = {account: Weave.Service.account,
> +                           password: Weave.Service.password,
> +                           synckey: Weave.Service.passphrase,
> +                           serverURL: Weave.Service.serverURL};

Can we line up the "Weave"s? *aesthete wince*

@@ +138,5 @@
>      this.pin1.disabled = this.pin2.disabled = this.pin3.disabled = true;
>      this.wizard.canAdvance = false;
>  
>      let pin = this.pin1.value + this.pin2.value + this.pin3.value;
> +    jpakeclient.pairWithPIN(pin);

The part of me that wears belt, braces, and staples his pants to his abdomen would prefer

  let expectDelay = false;
  jpakeclient.pairWithPIN(pin, expectDelay);

-- presumably expectDelay at some point will be true or computed, and this is a step to fill JS's lack of named arguments, and act as a kind of informal doc. But I won't press the point!
Attachment #562649 - Flags: review?(rnewman) → review+
Address rnewman's review comment: also fix || operator wrapping style.
Attachment #562644 - Attachment is obsolete: true
Address rnewman's feedback comments.
Attachment #562648 - Attachment is obsolete: true
Address rnewman's review comments.
Attachment #562649 - Attachment is obsolete: true
Comment on attachment 562808 [details] [diff] [review]
Part 0 (v2): Fix style in jpakeclient.js

UR RUBBER STAMPS, I HAZ THEM
Attachment #562808 - Flags: review+
Attachment #562810 - Flags: review+
Tests all fleshed out (and simplified with a BaseController).

(Note: the WIPs contained a brainfart about accepting any 412 Precondition Failed that the server sends. That's obviously not correct. We should only do that if we attempt to PUT again after a network failure or something similar. We don't do that right now, so we shouldn't continue if our only PUT fails with 412 Precondition Failed. In fact, it would be a sign that the channel has been tampered with. See also bug 624714.)
Attachment #563984 - Flags: review?(rnewman)
Blocks: 624714
Attachment #562809 - Attachment is obsolete: true
Comment on attachment 563984 [details] [diff] [review]
Part 1 (v1): Implement KeyExchange v3

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

Just nits.

::: services/sync/tests/unit/test_jpakeclient.js
@@ +89,5 @@
>      response.bodyOutputStream.write(this.data, this.data.length);
>  
>      // Automatically clear the channel after 6 successful GETs.
>      this.getCount += 1;
> +    if (this.getCount == 6) {

Magic number…

@@ +235,5 @@
>  });
>  
>  
> +add_test(function test_firstMsgMaxTries() {
> +  _("Test that receiver can wait longer for the first message..");

s/\.\././

@@ +277,5 @@
> +    __proto__: BaseController,
> +    onPaired: function onPaired() {
> +      // For the purpose of the tests, the poll interval is 50ms and
> +      // we're polling up to 5 times for the last exchange (as opposed
> +      // to 2 times for other exchanges). So let's pretend it took

"other" isn't strictly true -- for 3 exchanges, we allow 5, 2, 5.

@@ +278,5 @@
> +    onPaired: function onPaired() {
> +      // For the purpose of the tests, the poll interval is 50ms and
> +      // we're polling up to 5 times for the last exchange (as opposed
> +      // to 2 times for other exchanges). So let's pretend it took
> +      // 150ms to come up with the final payload.

", which should require three polls."

@@ +279,5 @@
> +      // For the purpose of the tests, the poll interval is 50ms and
> +      // we're polling up to 5 times for the last exchange (as opposed
> +      // to 2 times for other exchanges). So let's pretend it took
> +      // 150ms to come up with the final payload.
> +      _("Pairing successful, waiting 159ms to send final payload.");

s/159/150

@@ +456,5 @@
> +  let channel = channels[cid];
> +  channel.data = JSON.stringify({type: "receiver1",
> +                                 version: KEYEXCHANGE_VERSION,
> +                                 payload: {}});
> +  // This naughty server doesn't supply ETag (well, it supplies empty one.)

s/\.)/)./
Attachment #563984 - Flags: review?(rnewman) → review+
Addressed nits. Please do a final review of the KeyExchange v3 implementation.
Attachment #563984 - Attachment is obsolete: true
Attachment #564010 - Flags: review?(rnewman)
Comment on attachment 564010 [details] [diff] [review]
Part 1 (v2): Implement KeyExchange v3

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

This looks good to me.

As we discussed, the user-flow.rst doc (and presumably the rest of the spec) needs some careful consideration around its inclusion of the 412/200 mechanism: certainly this should be a 'could' not a 'should' on behalf of the client (particularly given that the primary implementation elects not to implement this behavior!), but I'm also not convinced this was adequately reviewed.

The spec could do with some additional love, perhaps phrasing it in terms of a generalized sequence of message exchanges. This would reduce some of the redundant repeated text, and make it easier to map to a code implementation.

I will find or file a bug for this later.

::: services/sync/modules/jpakeclient.js
@@ +113,3 @@
>   *   onComplete(data) -- Called after transfer has been completed. On
>   *     the sending side this is called with no parameter and as soon as the
>   *     data has been uploaded, which this doesn't mean the receiving side

Should be:

  data has been uploaded. This does not mean that the receiving side

@@ +148,5 @@
>    this._log = Log4Moz.repository.getLogger("Sync.JPAKEClient");
>    this._log.level = Log4Moz.Level[Svc.Prefs.get(
>      "log.logger.service.jpakeclient", "Debug")];
>  
>    this._serverUrl = Svc.Prefs.get("jpake.serverURL");

Can we please rename this attribute to _serverURL?

Same applies to _channelUrl.
Attachment #564010 - Flags: review?(rnewman) → review+
Blocks: sync2sm
No longer blocks: sync2sm
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.