Status

defect
RESOLVED FIXED
5 years ago
Last year

People

(Reporter: jryans, Assigned: jryans)

Tracking

(Blocks 1 bug)

unspecified
Firefox 37
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 1 obsolete attachment)

As part WiFi debugging, we need a version of RDP sockets over a TLS connection.

This may also involves pieces of the auth process as well.

Design: https://wiki.mozilla.org/DevTools/WiFi_Debugging/Design
Depends on: 1058997
Blocks: wifi-rdp
Blocks: 1103120
This will be used for the encryption part only.  Authentication will happen in bug 1103120.
Attachment #8531190 - Flags: review?(paul)
Attachment #8531190 - Flags: review?(past)
/r/1111 - Bug 1059001 - Part 1a: Separate create and open listener. r=past
/r/1113 - Bug 1059001 - Part 1b: Update openListener callsites. r=past
/r/1115 - Bug 1059001 - Part 2: Move discovery into socket listener. r=past
/r/1117 - Bug 1059001 - Part 3: Add encryption socket option. r=past
/r/1119 - Bug 1059001 - Part 4: Pass encryption through via discovery. r=past
/r/1121 - Bug 1059001 - Part 5: Reset connection options before connecting. r=paul

Pull down these commits:

hg pull review -r 4c48e5855a9794a3f7de7fc11464c565cdcddcd8
/r/1111 - Bug 1059001 - Part 1a: Separate create and open listener. r=past
/r/1113 - Bug 1059001 - Part 1b: Update openListener callsites. r=past
/r/1115 - Bug 1059001 - Part 2: Move discovery into socket listener. r=past
/r/1117 - Bug 1059001 - Part 3: Add encryption socket option. r=past
/r/1119 - Bug 1059001 - Part 4: Pass encryption through via discovery. r=past
/r/1121 - Bug 1059001 - Part 5: Reset connection options before connecting. r=paul

Pull down these commits:

hg pull review -r bce3ef5fce9b56e30d6b36f70051802af969e68c
Updated to fix some test failures.
Comment on attachment 8531190 [details]
MozReview Request: bz://1059001/jryans

Paul gave r+ for his portion.
Attachment #8531190 - Flags: review?(paul) → review+
https://reviewboard.mozilla.org/r/1113/#review719

::: b2g/chrome/content/devtools/debugger.js
(Diff revision 2)
> +      this._listener.portOrPath = -1 /* any available port */;

Not a fan of the more traditional "-1; // any available port"?
https://reviewboard.mozilla.org/r/1117/#review723

Is there someone more familiar with the TLS APIs that could take an additional look at the socket creation and connection acceptance bits? mgoodwin or dkeeler perhaps?

::: toolkit/devtools/transport/tests/unit/test_dbgsocket.js
(Diff revision 2)
>  function test_socket_shutdown()

This should be a generator function.

::: toolkit/devtools/transport/tests/unit/test_dbgsocket.js
(Diff revision 2)
> -      // machines it may just time out.

I think it would be useful to keep the comment, as it may not be immediately clear why it would be expected for the connection to timeout.

::: toolkit/devtools/security/socket.js
(Diff revision 2)
>      DevToolsUtils.reportException("socketConnect", e);

This should now become "_attemptConnect".

::: toolkit/devtools/security/socket.js
(Diff revision 2)
> +    let { input, output } = _attemptConnect({ host, port, encryption });

I find the gratuitous blocks confusing and as far as I can tell they only exist to allow the destructuring assignment with |let| work. If that is actually the case, I would prefer a slightly more verbose assignment instead.

Also, why not move the connection attempt into a separate function if we are using it twice?

::: toolkit/devtools/security/socket.js
(Diff revision 2)
> +    }, e => {

We don't currently handle errors from this promise. Better move the rejection handler in a separate .then(null, e => ...).

::: toolkit/devtools/security/tests/unit/test_encryption.js
(Diff revision 2)
> +function run_test() {

Nit: brief comment about the test's purpose please.

::: toolkit/devtools/security/socket.js
(Diff revision 2)
> +Cc["@mozilla.org/psm;1"].getService(Ci.nsISupports);

Is this cheap enough for us to do unconditionally even if no TLS connection happens?
Attachment #8531190 - Flags: review?(past) → review+
https://reviewboard.mozilla.org/r/1113/#review737

> Not a fan of the more traditional "-1; // any available port"?

Eh, I guess I was thinking of it like a magic function argument, where it's nice to put the comment inline with the value.  Not too worried about the style myself, but can change if you'd like.
Blocks: 1109285
Comment on attachment 8531190 [details]
MozReview Request: bz://1059001/jryans

dkeeler, Panos has reviewed this code from the DevTools team, but we would like to make sure the TLS socket creation and acceptance pieces are being done correctly.

Can you give feedback on these parts?  The relevant pieces are in the part 3 patch.  Specifically, the changes to socket.js[1] in |DebuggerSocket.connect| and the |SecurityObserver| that checks the server-side handshake.

[1]: https://reviewboard.mozilla.org/r/1117/diff/2/?file=16777#10
Attachment #8531190 - Flags: feedback?(dkeeler)
https://reviewboard.mozilla.org/r/1117/#review745

I've flagged dkeeler for feedback on this patch.  Also, there will be a general security review of the feature once the auth pieces land.

> Is this cheap enough for us to do unconditionally even if no TLS connection happens?

I spent a while trying to do this lazily, or later on in the process, but all such attempts led to crashes.  It seems NSS (which is what this will load) is a bit fragile in this respect.

This file (security/socket) is loaded lazily by all consumers, so I think that will need to be good enough for now.  For example, it would not be loaded by each app on b2g, only the root server.  I filed bug 1109285 to investigate further.

> I think it would be useful to keep the comment, as it may not be immediately clear why it would be expected for the connection to timeout.

Makes sense, restored.

> This should be a generator function.

Changed.

> We don't currently handle errors from this promise. Better move the rejection handler in a separate .then(null, e => ...).

Moved to separate rejection handler.

> This should now become "_attemptConnect".

Fixed.

> I find the gratuitous blocks confusing and as far as I can tell they only exist to allow the destructuring assignment with |let| work. If that is actually the case, I would prefer a slightly more verbose assignment instead.
> 
> Also, why not move the connection attempt into a separate function if we are using it twice?

Yeah, I was trying to balance readibilty and comprehension vs. making another function.  I don't feel very strongly either way, so your suggestions are fine with me.

I've added a new |_attemptTransport| function to contain the repeated pieces, and removed the usage of inner blocks.
Made review changes and fixed a path issue on Fennec.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8de2f2fda0cf
https://reviewboard.mozilla.org/r/1117/#review807

LGTM in general - I just had a couple of questions.

::: toolkit/devtools/security/socket.js
(Diff revision 2)
> +              .SSLStatus.QueryInterface(Ci.nsISSLStatus).serverCert;

nsISSLStatusProvider.SSLStatus already is an nsISSLStatus, so you shouldn't need to QI to it, right?

::: toolkit/devtools/security/socket.js
(Diff revision 2)
> + * override.  This implies that we take on the burden of authentication for

Out of curiosity, where does the authentication happen? Somewhere higher in the UI?

::: toolkit/devtools/security/socket.js
(Diff revision 2)
> +    if (clientStatus.tlsVersionUsed < Ci.nsITLSClientStatus.TLS_VERSION_1_2) {

I would probably actually use != instead of < here.
Attachment #8531190 - Flags: feedback?(dkeeler) → feedback+
https://reviewboard.mozilla.org/r/1117/#review823

Thanks for taking a look!

> Out of curiosity, where does the authentication happen? Somewhere higher in the UI?

It will be done via a QR code scanning scheme in the UI, but this has not landed yet.  That's my next step, in bug 1103120.  The feature is disabled by default until I've got that part in.

> nsISSLStatusProvider.SSLStatus already is an nsISSLStatus, so you shouldn't need to QI to it, right?

Ah, you're right, I've removed it.

> I would probably actually use != instead of < here.

Okay, being stricter should be fine, fixed.
Depends on: 1113035
Attachment #8531190 - Attachment is obsolete: true
Attachment #8618291 - Flags: review+
Attachment #8618292 - Flags: review+
Attachment #8618293 - Flags: review+
Attachment #8618294 - Flags: review+
Attachment #8618296 - Flags: review+
Attachment #8618297 - Flags: review+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.