J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Description • 10 years ago

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

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 1 • 10 years ago

This will be used for the encryption part only.  Authentication will happen in bug 1103120.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 2 • 10 years ago

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3bea5c796d57

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 4 • 10 years ago

/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

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 5 • 10 years ago

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3c54a1126eea

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 6 • 10 years ago

/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

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 7 • 10 years ago

Updated to fix some test failures.

	Paul Rouget [:paul]
	Comment 8 • 10 years ago

https://reviewboard.mozilla.org/r/1121/#review693

Ship It!

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 9 • 10 years ago

Comment on attachment 8531190 [details]
MozReview Request: bz://1059001/jryans

Paul gave r+ for his portion.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 10 • 10 years ago

Fixed test failure.

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a4fe1c27d627

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 11 • 10 years ago

https://reviewboard.mozilla.org/r/1111/#review717

Ship It!

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 12 • 10 years ago

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"?

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 13 • 10 years ago

https://reviewboard.mozilla.org/r/1115/#review721

Ship It!

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 14 • 10 years ago

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?

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 15 • 10 years ago

https://reviewboard.mozilla.org/r/1119/#review731

Ship It!

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 16 • 10 years ago

https://reviewboard.mozilla.org/r/1109/#review733

Ship It!

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 17 • 10 years ago

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.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 18 • 10 years ago

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

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 19 • 10 years ago

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.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 20 • 10 years ago

Made review changes and fixed a path issue on Fennec.

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

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews)
	Comment 21 • 10 years ago

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.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 22 • 10 years ago

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.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 23 • 10 years ago

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=573c68faedf9

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 24 • 10 years ago

remote:   https://hg.mozilla.org/integration/fx-team/rev/0a424f45dc23
remote:   https://hg.mozilla.org/integration/fx-team/rev/b16c58046543
remote:   https://hg.mozilla.org/integration/fx-team/rev/20a3c5ad55c6
remote:   https://hg.mozilla.org/integration/fx-team/rev/40e8d75da54f
remote:   https://hg.mozilla.org/integration/fx-team/rev/5fc8a787af0f
remote:   https://hg.mozilla.org/integration/fx-team/rev/aa53dedec65f

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 25 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/0a424f45dc23
https://hg.mozilla.org/mozilla-central/rev/b16c58046543
https://hg.mozilla.org/mozilla-central/rev/20a3c5ad55c6
https://hg.mozilla.org/mozilla-central/rev/40e8d75da54f
https://hg.mozilla.org/mozilla-central/rev/5fc8a787af0f
https://hg.mozilla.org/mozilla-central/rev/aa53dedec65f