1207996 - [TV] Implement JPAKE authentication over TLS socket server

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::TV::System, defect)

Description

[Eric Tsai (no more review request accepted)]

This is a meta bug for TV remote control PIN code pairing

Comment 1

[Luke Chang [:lchang] (inactive)]

I've created a WIKI page for documenting the architecture of Remote Control. Please refer to [1] for the flowchart drafts of this pairing mechanism.

[1] https://wiki.mozilla.org/Firefox_OS/Remote_Control#Pairing

Comment 2

[Eric Tsai (no more review request accepted)]

Modify for Gecko features

Comment 3

[Eric Tsai (no more review request accepted)]

Attachment: Gecko patch for PIN code pairing function

Hi Fabrice, please help to review this patch for pairing. The patch depends on https://bugzilla.mozilla.org/attachment.cgi?id=8673027 and gaia patches from :lchang at bug 1215075 and bug 1215076

Comment 4

[Eric Tsai (no more review request accepted)]

Attachment: 0001-Bug-1207996-Add-PIN-code-pairing.patch

Gecko patch for PIN code pairing function, fix server IP in settings

Hi Fabrice, please help to review this patch for pairing. The patch depends on https://bugzilla.mozilla.org/attachment.cgi?id=8673027 and gaia patches from :lchang at bug 1215075 and bug 1215076

Comment 5

[Eric Tsai (no more review request accepted)]

Comment on attachment 8676013 [details] [diff] [review]
0001-Bug-1207996-Add-PIN-code-pairing.patch

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

Switch reviewer to schien

Comment 6

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8676013 [details] [diff] [review]
0001-Bug-1207996-Add-PIN-code-pairing.patch

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

Discussed with @etsai, this patch will be updated according to the modification in bug 1197749. Cancel r? for now.

::: b2g/components/RemoteControlService.jsm
@@ +85,5 @@
> +function sendChromeEvent(action, details)
> +{
> +  details.action = action;
> +  SystemAppProxy._sendCustomEvent(REMOTE_CONTROL_EVENT, details);
> +}

Not need to create this short, one-time-only function.

Comment 7

[Eric Tsai (no more review request accepted)]

Attachment: 0001-Bug-1207996-Add-PIN-code-pairing.patch

Update patch follow bug 1197749 and 1197751's coding style. Add description, remove one-shot function.

Comment 8

[Eric Tsai (no more review request accepted)]

Attachment: 0001-Bug-1207996-Add-PIN-code-pairing.patch

After discuss with :lchang, handle event if and only if the request if valid.

Comment 9

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8688188 [details] [diff] [review]
0001-Bug-1207996-Add-PIN-code-pairing.patch

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

The patch is not clearly divide all the pairing related logic from bug 1197749, which means I need to cross reference two patches while reviewing this patch.
Please help create a clear patch to reduce review effort.

Or, you can use the MozReview so that I can see full source code in one place.
https://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview.html

::: b2g/components/RemoteControlService.jsm
@@ +359,4 @@
>    },
>  
>    _isValidUUID: function(uuid) {
> +      return (uuid in this._uuids);

nit: indentation

@@ +389,5 @@
> +    if ( width > 0 )
> +    {
> +      return new Array( width + (/\./.test( number ) ? 2 : 1) ).join( '0' ) + number;
> +    }
> +    return number + ""; // always return a string

why not simply prepend '0's if the length of number.toString() is less than width?

@@ +451,5 @@
> +         // Send pairing.html to start pairing
> +         if (this._pairingRequired == false ||
> +             (request.hasHeader("Cookie") &&
> +              this._isValidUUID (decodeURIComponent(request.getHeader("Cookie")).substring(5)))) {
> +           return "/client.html";

This code looks duplicated in client.sjs. It means you have multiple call path to exam pairing token. Please rearrange this code to a better place. It's also weird to see UUID/pairing related function being exposed to sandbox.

::: b2g/remotecontrol/client.sjs
@@ +464,5 @@
>  function handleRequest(request, response)
>  {
> +  var requestIsValid = (isPairingRequired() == false ||
> +                             (request.hasHeader("Cookie") &&
> +                              isValidUUID (decodeURIComponent(request.getHeader("Cookie")).substring(5))));

nit: reform the statement to avoid compound expression and reduce the indentation level. for example:

if (isPairingRequired()) {
  let cookie = request.hasHeader("Cookie") ? request.getHeader("Cookie")
                                           : undefined;
  if (!cookie || isValidUUID(decodeURIComponenet(cookie.substring(5))) {
    // ... redirect to pairing procedure.
    return;
  }
}

//... continue the normal procedure

::: b2g/remotecontrol/pairing.sjs
@@ +42,5 @@
> +{
> +  var queryString = decodeURIComponent(request.queryString.replace(/\+/g, "%20"));
> +
> +  // Split JSON header "message=" and parse event
> +  var event = JSON.parse(queryString.substring(8));

Using substring is too hacky. You can use something like https://stackoverflow.com/a/2091331 to dealing with query string.

Comment 10

[Josh Cheng [:josh]]

Remote COntrol descoped from 2.5

Comment 11

[Josh Cheng [:josh]]

Hi Tori,
Will there be any UX spec change due to new use case of remote control?
Thanks!

Comment 12

[Tori Chen [:tori]]

Yes, I will modify the UX spec soon.

Comment 13

[Tori Chen [:tori]]

Hi Mike, 

Please check the latest spec for remote control here. This version(v1.1) was reviewed and approved by SC and Eric. Thanks!

https://drive.google.com/folderview?id=0B4dMhI4hp32OY0tfN3JvenhySWs&usp=sharing

Comment 14

[Mike Lien[:mlien]]

(In reply to Tori Chen [:tori] from comment #13)
> Hi Mike, 
> 
> Please check the latest spec for remote control here. This version(v1.1) was
> reviewed and approved by SC and Eric. Thanks!
> 
> https://drive.google.com/
> folderview?id=0B4dMhI4hp32OY0tfN3JvenhySWs&usp=sharing

According to this UX spec, I'll start to create test plan (bug 1269580). BTW, do you know when will the new visual spec come out?

Comment 15

[Tori Chen [:tori]]

There is already visual spec here: https://drive.google.com/open?id=0B4K8q1qWmtAvalFxMHQ4Uktsb2c
I'll ask Peko to update it based on new UX spec, but there shouldn't be too much different besides to removing TV part.

Comment 16

[Tori Chen [:tori]]

A minor change on spec after discussion with Eric this Monday. If user enter correct PIN code, show success message, redirect, and "clear" the pin code(V1.2, p12).

https://drive.google.com/folderview?id=0B4dMhI4hp32OY0tfN3JvenhySWs&usp=sharing

Comment 17

[Eric Tsai (no more review request accepted)]

Attachment: Patch 1. JPAKE authentication over TLS socket server

Hi :schien, I would like invite you to give functional review on b2g. After that, I will open a sec-review bug for implementation review to ensure both gecko and fennec implementations are OK.

Comment 18

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8755295 [details] [review]
Patch 1. JPAKE authentication over TLS socket server

r+ with my comment addressed.

Comment 19

[Eric Tsai (no more review request accepted)]

Comment on attachment 8755295 [details] [review]
Patch 1. JPAKE authentication over TLS socket server

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 1207996
User impact if declined: No function
Testing completed: self-tested
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: N/A

Comment 20

[Josh Cheng [:josh]]

Comment on attachment 8755295 [details] [review]
Patch 1. JPAKE authentication over TLS socket server

Approve for TV 2.6

Comment 21

[Eric Tsai (no more review request accepted)]

Hi Gary, please help to merge to 2.6 branch, thanks!

Comment 22

[Gary Chen [:xeonchen]]

https://github.com/mozilla-b2g/gecko-b2g/commit/1f9da1c47090cd29110df577ea1708cf6531c299

Comment 23

[Gary Chen [:xeonchen]]

Since this bug is for TV, I suppose this is a 2.6 only bug.