Andrew Sutherland [:asuth] (he/him) Reporter
	Description • 12 years ago

+++ This bug was initially created as a clone of Bug #733573 +++

Bug 733573 currently includes support for unencrypted cleartext connections and starting a connection with SSL/TLS active from the get-go, but does not include upgrade-to-TLS from unencrypted cleartext.

Both IMAP and SMTP support a variant of this; in IMAP, it is the STARTTLS command.

This was initially deferred from bug 733573 because it was not required for immediate needs (the known IMAP servers of interest that support STARTTLS also support SSL/TLS from the get-go) and there were complications related to buffering and use of the TLS upgrade.

The MozTCPSocket implementation ended up changing its buffering solution by the time it landed, so this is believed to be pretty simple.  The key implementation need is to make sure that all buffers filled/enqueued prior to the call to startTLS on MozTCPSocket get flushed before the TLS upgrade is performed.  Ideally, all buffers written to after calling startTLS will appropriately wait for the TLS negotiation to occcur as well.

In terms of configuration, the way this worked before startTLS support was removed was that 'starttls' is passed as the crypto option instead of (Boolean)true, but the gaia e-mail client is flexible about this.

NB: Since MozTCPSocket moved from b2g/ to dom/, it might be appropriate to move this bug, but since I'm cloning the bug, I'm leaving it b2g for now in the hopes someone else has a better idea of where to migrate it to.

Because there are no known servers that only support TLS upgrade (and not initial SSL/TLS), I don't think this should be a b2g blocker, but I also think that we really need to do this now so we don't end up with a partial TCP implementation and while the code is fresh in the minds of the authors/reviewers.

	Andrew Overholt [:overholt]
	Comment 1 • 12 years ago

Donovan, do you think this will make feature freeze?  Thanks.

	Donovan Preston [:fzzzy]
	Comment 2 • 12 years ago

No.

	Andrew Overholt [:overholt]
	Comment 3 • 12 years ago

I spoke with Andrew Sutherland about this and while it's low-hanging fruit that would be nice to have, it's not a blocker for the B2G mail app for v1 so I'm removing the blocker+.  As always, if anyone disagrees, feel free to re-nom.

	Faramarz Rashed
	Comment 4 • 12 years ago

We just covered this in the Gaia meeting. Just wanting to make sure that this doesn't mean we won't have a secure way of emailing; and Andrew S. assured us we would.  Andrew, can you confirm in this bug please?

	Donovan Preston [:fzzzy]
	Comment 5 • 12 years ago

SSL works without this bug; this bug is only for TLS, which is creating a socket unencrypted and then upgrading it to encrypted.

	Andrew Sutherland [:asuth] (he/him) Reporter
	Comment 6 • 12 years ago

(In reply to Faramarz (:faramarz) from comment #4)
> We just covered this in the Gaia meeting. Just wanting to make sure that
> this doesn't mean we won't have a secure way of emailing; and Andrew S.
> assured us we would.  Andrew, can you confirm in this bug please?

Yes.  We are establishing encrypted connections (technically TLS which has succeeded SSL, but 'SSL' in our parlance) for IMAP and SMTP using the TCP API.

Although we support unencrypted/plaintext connections in the back-end, this is primarily for testing.  We are attempting to make it impossible to create an account that is unencrypted because the risks are simply so high given that the devices may be used over suspect wi-fi networks, etc.

	Andrew Sutherland [:asuth] (he/him) Reporter
	Comment 7 • 12 years ago

(ps: where I said plaintext I meant cleartext.)

	Patrick McManus [:mcmanus]
	Comment 8 • 12 years ago

(In reply to Andrew Sutherland (:asuth) from comment #6)

> Although we support unencrypted/plaintext connections in the back-end, this
> is primarily for testing.  We are attempting to make it impossible to create
> an account that is unencrypted because the risks are simply so high given
> that the devices may be used over suspect wi-fi networks, etc.

+1 awesome!

	Andrew Sutherland [:asuth] (he/him) Reporter
	Comment 9 • 12 years ago

Ben Bucksh did some fantastic digging into the Thunderbird ISP database on a post to the tb-planning list to look for servers that may require STARTTLS because of a lack of pure SSL support.  It also covers entries that appear to lack crypto support entirely or may require POP3:
https://mail.mozilla.org/pipermail/tb-planning/2013-January/002579.html

The relevant bits are:
===
comcast.net - SMTP is STARTTLS only, only POP3
email.it - SMTP is STARTTLS only
ewetel.de - SMTP is STARTTLS only
hotmail.com - SMTP is STARTTLS only, only POP3
some unheard-of Japanese ISPs - STARTTLS only
inbox.lt, inbox.lv - SMTP is STARTTLS only
kabelmail.de - POP3 only
posteo.de - STARTTLS only
telenor.dk (with lots of domains) - STARTTLS only
skynet.be - SMTP is STARTTLS only
sympatico.ca - SMTP is STARTTLS only
t-online.de - SMTP is STARTTLS only (they are huge!)
web.de - SMTP is STARTTLS only (they are huge!)
uol.com.br - SMTP is STARTTLS only (that's your target group, right?)
===

===
Please note that I generally tried to find and list all working configs, not only those that are officially supported, so if there's no SSL server listed, then most likely there simply is no working one (at least at the time when we did the config).

I just double-checked some of the important configs that have STARTTLS, but no SSL:

 * T-Online securesmtp.t-online.de works with SSL on port 465, so we
   could add that. I only checked whether the server responds, but not
   whether the certificate is OK and a login actually works.
 * ditto uol.com.br
 * smtp.web.de SSL 465 does not work. web.de is one of the biggest mail
   providers in Germany, and in the top 5 world-wide for Thunderbird.
 * smtpauth.peoplepc.com SSL 465 blackholes, no response at all
 * ditto earthlink, mindspring.com 
===

Nominating for tracking since this is not currently tracked.

	Alex Keybl [:akeybl]
	Comment 10 • 12 years ago

New feature, unless this is a blocker for a future product/version, we won't be taking in v1.x.

	Andreas Gal :gal
	Comment 11 • 12 years ago

I am adding a comment pointing to this bug to imap.js to use node-like starttls emulation for this.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 12 • 12 years ago

Needed for competitive parity, I think.  If we can emulate it then p2.

	Florian Bender
	Comment 13 • 12 years ago

For T-Online: it's a branch of Telekom which Mozilla is partnering with. So you might be able to do something here on the provider side. 

This will probably not work for WEB.DE. It's mostly used for Freemail, though there is a Premium option (which only few people use). The Freemail product is inherently bad (e. g. they only offer 12MB Mail storage unless you install their crappy toolbar). I guess they'll have little motivation to upgrade any of their software for the free users (their Premium product provides IMAP and maybe SSL/TLS without STARTTLS). However, WEB.DE focuses on the German market iirc thus it should probably not be high prio.

	Honza Bambas (:mayhemer)
	Comment 14 • 12 years ago

I think this should not be that hard to implement with the architecture we have.

	Honza Bambas (:mayhemer)
	Comment 15 • 11 years ago

What is the priority of this bug?

	Donovan Preston [:fzzzy]
	Comment 16 • 11 years ago

(In reply to Honza Bambas (:mayhemer) from comment #15)
> What is the priority of this bug?

What do you think it should be? When this bug was first opened we decided it wasn't life or death because regular ssl worked, and doing this properly would have been difficult the way the code was originally written. Now that the socket code uses MultiplexStream, it should be straightforward to implement this by setting a flag, creating a new MultiplexStream to store new sent strings in, waiting until the old insecure MultiplexStream is empty, and then performing the TLS upgrade.

	Andrew Sutherland [:asuth] (he/him) Reporter
	Comment 17 • 11 years ago

Peter, we would like to prioritize this (and the related e-mail bug 847032) for v1.2 for e-mail.  I'm abusing needinfo here for e-mail feature triage.

	Peter Dolanjski [:pdol]
	Comment 18 • 11 years ago

Nom'd for blocking 1.2 (Koi).

	Honza Bambas (:mayhemer)
	Comment 19 • 11 years ago

(In reply to Peter Dolanjski [:pdol] from comment #18)
> Nom'd for blocking 1.2 (Koi).

Thanks.  What is the 1.2 deadline or where can I find the dates (there is wiki page for this I think) ?

(In reply to Donovan Preston from comment #16)
> (In reply to Honza Bambas (:mayhemer) from comment #15)
> > What is the priority of this bug?
> 
> What do you think it should be? ... then performing the TLS upgrade.

I was asking for priority.  I know how to implement it, but the thing is that someone has to do it.  I wanted to know whether I should preserve some cycles for this soon or not.

	Vincent
	Comment 20 • 11 years ago

Could you work on it Honza Bambas? You could solve https://bugzilla.mozilla.org/show_bug.cgi?id=847032

	Patrick Wang (Chih-Kai Wang) (:kk1fff) Assignee
	Comment 21 • 11 years ago

Is anyone working on this? If not, can I take this bug?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 22 • 11 years ago

Patrick, I don't think anyone is working on this. But please coordinate the implementation strategy with Honza since he module peer for this code. It would also be good to keep Donovan in the loop since he wrote the original TCPSocket implementation.

	Vincent
	Comment 24 • 11 years ago

(In reply to Patrick Wang [:kk1fff] from comment #23)
> Created attachment 789416 [details] [diff] [review]
> Implement startTLS in MozTCPSocket

OMG, thousand thanks to you, Patrick Wang. Waited for so long time! Thank you again.

	Honza Bambas (:mayhemer)
	Comment 25 • 11 years ago

Comment on attachment 789416 [details] [diff] [review]
Implement startTLS in MozTCPSocket

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

That's it.

Test?

::: dom/network/interfaces/nsIDOMTCPSocket.idl
@@ +73,5 @@
>     */
>    nsIDOMTCPServerSocket listen(in unsigned short localPort, [optional] in jsval options,
>                                 [optional] in unsigned short backlog);
>  
> +  void startTLS();

IID change

::: dom/network/src/TCPSocket.js
@@ +157,5 @@
>    _socketBridge: null,
>  
> +  // StartTLS
> +  _waitingForStartTLS: false,
> +  _pendingData: [],

_pendingDataAfterStartTLS ?

@@ +265,5 @@
> +            if (self._pendingData.length > 0) {
> +              for (let i = 0; i < self._pendingData.length; i++) {
> +                self._multiplexStream.appendStream(self._pendingData[i]);
> +              }
> +              self._pendingData.length = 0;

clear() ?

@@ +270,5 @@
> +              self._ensureCopying();
> +            } else if (self._waitingForDrain) {
> +              self._waitingForDrain = false;
> +              self.callListener("drain");
> +            }

this is duplicated.  thinking of just 'return;' after self._ensureCopying(); and when no pending data let this method execution continue.  Up to you.

	Andrew Sutherland [:asuth] (he/him) Reporter
	Comment 26 • 11 years ago

Before we removed startTLS support from the original TCPSocket landing, we only allowed for startTLS upgrade if useSSL was 'starttls'.  Is there an underlying resource overhead for asking for startTLS support for a connection that will only ever be cleartext TCP connection?

Also, should we migrate to be closer to the ongoing raw sockets spec work?  Right now, the TCPSocket constructor spec calls for the third argument to be an options dictionary: http://www.w3.org/2012/sysapps/raw-sockets/#dictionary-tcpoptions

The encryption stuff isn't spec'ed well there, but presumably we can impact that.  It seems like 'useSecureTransport' there isn't thought out and you would need to explicitly indicate SSL or starttls support, presumably via a string value like 'ssl' or 'starttls'.  Or use something like 'initial' or 'upgrade' to indicate how we would go secure since we're really using TLS and it's a question of whether we fundamentally require encryption from the get-go or will negotiate up to it after a chat with the server.

	Patrick Wang (Chih-Kai Wang) (:kk1fff) Assignee
	Comment 27 • 11 years ago

(In reply to Andrew Sutherland (:asuth) from comment #26)
> Before we removed startTLS support from the original TCPSocket landing, we
> only allowed for startTLS upgrade if useSSL was 'starttls'.  Is there an
> underlying resource overhead for asking for startTLS support for a
> connection that will only ever be cleartext TCP connection?
> 
> Also, should we migrate to be closer to the ongoing raw sockets spec work? 
> Right now, the TCPSocket constructor spec calls for the third argument to be
> an options dictionary:
> http://www.w3.org/2012/sysapps/raw-sockets/#dictionary-tcpoptions
> 
> The encryption stuff isn't spec'ed well there, but presumably we can impact
> that.  It seems like 'useSecureTransport' there isn't thought out and you
> would need to explicitly indicate SSL or starttls support, presumably via a
> string value like 'ssl' or 'starttls'.  Or use something like 'initial' or
> 'upgrade' to indicate how we would go secure since we're really using TLS
> and it's a question of whether we fundamentally require encryption from the
> get-go or will negotiate up to it after a chat with the server.

Sorry, I missed this comment.

A starttls socket in gecko is more like an SSL socket, I believe it introduces more overhead than a plain socket does. But I think it will be more straightforward for people who use MozTCPSocket to upgrade any insecure socket to secure by calling a method like startTLS. As we already use SSL socket a lot in our code, I believe the overhead should be acceptable. I suggest that we just use startTLS socket in any MozTCPSocket that isn't created with ssl.

So we have two questions:
1. Should we change the option name to 'useSecureTransport' to make our interface be closer to raw socket spec?
2. Should provide a way to create a TCPSocket that won't go secure?

Honza, do you have any thoughts?

	Honza Bambas (:mayhemer)
	Comment 28 • 11 years ago

(In reply to Patrick Wang [:kk1fff] from comment #27)
> (In reply to Andrew Sutherland (:asuth) from comment #26)
> > Before we removed startTLS support from the original TCPSocket landing, we
> > only allowed for startTLS upgrade if useSSL was 'starttls'.  Is there an
> > underlying resource overhead for asking for startTLS support for a
> > connection that will only ever be cleartext TCP connection?
> > 
> > Also, should we migrate to be closer to the ongoing raw sockets spec work? 
> > Right now, the TCPSocket constructor spec calls for the third argument to be
> > an options dictionary:
> > http://www.w3.org/2012/sysapps/raw-sockets/#dictionary-tcpoptions
> > 
> > The encryption stuff isn't spec'ed well there, but presumably we can impact
> > that.  It seems like 'useSecureTransport' there isn't thought out and you
> > would need to explicitly indicate SSL or starttls support, presumably via a
> > string value like 'ssl' or 'starttls'.  Or use something like 'initial' or
> > 'upgrade' to indicate how we would go secure since we're really using TLS
> > and it's a question of whether we fundamentally require encryption from the
> > get-go or will negotiate up to it after a chat with the server.
> 
> Sorry, I missed this comment.
> 
> A starttls socket in gecko is more like an SSL socket, I believe it
> introduces more overhead than a plain socket does. But I think it will be
> more straightforward for people who use MozTCPSocket to upgrade any insecure
> socket to secure by calling a method like startTLS. As we already use SSL
> socket a lot in our code, I believe the overhead should be acceptable. I
> suggest that we just use startTLS socket in any MozTCPSocket that isn't
> created with ssl.
> 

I don't understand the concerns.  What overhead are you talking about?

> So we have two questions:
> 1. Should we change the option name to 'useSecureTransport' to make our
> interface be closer to raw socket spec?

For spec conformation ask :sicking

> 2. Should provide a way to create a TCPSocket that won't go secure?

What would be a reason??

When creating a socket you create it either as raw or ssl (i.e. ssl is setup on the socket sooner then any data go out or in).  When the socket is raw, you can - depending on protocol demands - layer it with ssl with startTLS().  I don't see any other use case.

Maybe I just don't understand what you are asking me for.

	Andrew Sutherland [:asuth] (he/him) Reporter
	Comment 29 • 11 years ago

(In reply to Honza Bambas (:mayhemer) from comment #28)
> I don't understand the concerns.  What overhead are you talking about?

The question is basically: Is memoryUse(unencrypted TCP without the ability to call startTLS()) roughly the same as memoryUse(unencrypted TCP with startTLS upgrade possible but never called)?

Presumably there is some difference, and presumably the difference is small enough to not matter.  I just thought it was worth checking.

	Honza Bambas (:mayhemer)
	Comment 30 • 11 years ago

(In reply to Andrew Sutherland (:asuth) from comment #29)
> (In reply to Honza Bambas (:mayhemer) from comment #28)
> > I don't understand the concerns.  What overhead are you talking about?
> 
> The question is basically: Is memoryUse(unencrypted TCP without the ability
> to call startTLS()) roughly the same as memoryUse(unencrypted TCP with
> startTLS upgrade possible but never called)?
> 
> Presumably there is some difference, and presumably the difference is small
> enough to not matter.  I just thought it was worth checking.

There is absolutely no difference for the two cases.

	Patrick Wang (Chih-Kai Wang) (:kk1fff) Assignee
	Comment 31 • 11 years ago

Jonas, based on comment 26, in raw socket spec (http://www.w3.org/2012/sysapps/raw-sockets/#dictionary-tcpoptions), the option name of enabling secure is 'useSecureTransport', while the name is 'useSSL' in our implementation. Should we also change the option name to move closer to the spec?

	Patrick Wang (Chih-Kai Wang) (:kk1fff) Assignee
	Comment 32 • 11 years ago

I suggest we keep useSSL for now, we can always file a bug to change the option name when we are ready to make this change. Andrew, what do you think?

	Andrew Sutherland [:asuth] (he/him) Reporter
	Comment 33 • 11 years ago

Fine with me.  In general we probably want to maintain backwards compatibility anyways.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 34 • 11 years ago

The long term goal is definitely to align fully with the spec. Including removing properties that are not in the spec, and dropping support for the current API.

We can do that separately when we unprefix the API though. But of course the smaller the differences between the prefixed and unprefixed API, the easier migration will be.

So I would prefer to align with the spec as soon as we can. And we should definitely do it when we're adding new features.

Of course, if we think that our names/APIs are better than what's in the spec, please do provide feedback to the spec. We should definitely not be shy about doing that.

	Patrick Wang (Chih-Kai Wang) (:kk1fff) Assignee
	Comment 35 • 11 years ago

Thanks, Jonas. I will update the option name.

	Patrick Wang (Chih-Kai Wang) (:kk1fff) Assignee
	Comment 36 • 11 years ago

(In reply to Honza Bambas (:mayhemer) from comment #25)
> @@ +265,5 @@
> > +            if (self._pendingData.length > 0) {
> > +              for (let i = 0; i < self._pendingData.length; i++) {
> > +                self._multiplexStream.appendStream(self._pendingData[i]);
> > +              }
> > +              self._pendingData.length = 0;
> 
> clear() ?

I didn't find clear() method. my intention is to clear the array.

	Patrick Wang (Chih-Kai Wang) (:kk1fff) Assignee
	Comment 38 • 11 years ago

Change in Gaia. In this patch I just add 'useSecureTransport' in option. I think we can remove useSSL after Gecko change landed.

	Thinker Li [:sinker]
	Comment 39 • 11 years ago

I think the API worked here is too simplify what TLS is.  It seems stick to serve the requirement of the mail app.  But, by considering common use cases of TLS, we need to provide the ways to configure TLS itself.  For example, to implement an VPN protocol, it at least needs to install or select the certificate you want to use.

	Andrew Sutherland [:asuth] (he/him) Reporter
	Comment 40 • 11 years ago

(In reply to Thinker Li [:sinker] from comment #39)
> I think the API worked here is too simplify what TLS is.

It's probably best to directly discuss this as part of the standards process.  I think https://github.com/sysapps/raw-sockets/issues/10 or another issue there is probably the right place for that.  Or on http://lists.w3.org/Archives/Public/public-sysapps/

	Andrew Sutherland [:asuth] (he/him) Reporter
	Comment 41 • 11 years ago

Comment on attachment 796068 [details]
Patch: Part 2 - Use useSecureTransport option in Gaia.

Thanks for the patch.  Unfortunately, the files in gaia/apps/email/js/ext/ are compiled byproducts from the gaia-email-libs-and-more repo.  I've prepared an appropriate patch with an added comment for gaia-email-libs-and-more and landed it there and on gaia after testing a b2g-desktop build with your gecko patch and without it with the gaia patch applied.

landed:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/241/
https://github.com/mozilla-b2g/gaia-email-libs-and-more/commit/5bb84b91e4c6cdfdc9164e6d14242b7548d4bd75

https://github.com/mozilla-b2g/gaia/pull/11796
https://github.com/mozilla-b2g/gaia/commit/d5e7d4c39dee68dc7f3558980804ce8ea0015790

We'll take care of removing the old useSSL instance after the gecko patch lands and when we implement startTLS support/usage from the e-mail app.

	Andrew Sutherland [:asuth] (he/him) Reporter
	Comment 42 • 11 years ago

Additional note: previous TCPSocket changes have broken the Firefox OS simulator's adb logic, but I just checked https://github.com/mykmelez/r2d2b2g/blob/master/addon/lib/adb/adb-socket.js and no SSL is used, so I think that covers the consumers I know of.  (I do not profess to know all consumers outside of gaia, however.)

	Honza Bambas (:mayhemer)
	Comment 43 • 11 years ago

(In reply to Honza Bambas (:mayhemer) from comment #30)
> (In reply to Andrew Sutherland (:asuth) from comment #29)
> > The question is basically: Is memoryUse(unencrypted TCP without the ability
> > to call startTLS()) roughly the same as memoryUse(unencrypted TCP with
> > startTLS upgrade possible but never called)?
> 
> There is absolutely no difference for the two cases.

I've double checked and there is a difference...  

For "startTLS" we add a ssl socket over the tcp socket, but set it up to do not start the negotiation.  So, there is more memory consumption per socket.

Based on that it would be better to have an option to distinguish between startTLS and no-security.

Other option could be to simply add an ssl socket over the tco socket in your JS implementation of startTLS().  But I'm not sure how complicated and even possible would be to do this purely in JS.

Sorry, I somehow misinterpreted the question at the first place.

	Honza Bambas (:mayhemer)
	Comment 44 • 11 years ago

Comment on attachment 796066 [details] [diff] [review]
Patch: Part 1: Implement startTLS in MozTCPSocket

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

r+.  Comment 43 can be fixed in a followup, but since it changes the API we might want to ship consistently.  It means the startTLS option should be in the same release as this patch is, at least.

::: dom/network/src/TCPSocket.js
@@ +212,5 @@
>    set onclose(f) {
>      this._onclose = f;
>    },
>  
> +  _activeTLS: function() {

_activateTLS ?

@@ +229,1 @@
>      }

As I wrote in comment 43.

@@ +265,5 @@
> +            if (self._pendingDataAfterStartTLS.length > 0) {
> +              for (let i = 0; i < self._pendingDataAfterStartTLS.length; i++) {
> +                self._multiplexStream.appendStream(self._pendingDataAfterStartTLS[i]);
> +              }
> +              self._pendingDataAfterStartTLS.length = 0;

FYI: you can also do:

while (self._pendingDataAfterStartTLS.length)
  self._multiplexStream.appendStream(self._pendingDataAfterStartTLS.shift());

Up to you.

@@ +488,5 @@
> +      throw new Error("Socket not open.");
> +    }
> +    if (this._ssl == 'ssl') {
> +      // Already SSL
> +      return;

I have not read the spec, but shouldn't we throw?

	Patrick Wang (Chih-Kai Wang) (:kk1fff) Assignee
	Comment 45 • 11 years ago

(In reply to Honza Bambas (:mayhemer) from comment #44)
> @@ +488,5 @@
> > +      throw new Error("Socket not open.");
> > +    }
> > +    if (this._ssl == 'ssl') {
> > +      // Already SSL
> > +      return;
> 
> I have not read the spec, but shouldn't we throw?

Under current discussion thread (https://github.com/sysapps/raw-sockets/issues/10). The error is expected to be reported in the Promise upgradeToSecure() returns.

	Patrick Wang (Chih-Kai Wang) (:kk1fff) Assignee
	Comment 46 • 11 years ago

Per talked with Jonas on IRC, I will change the method name to upgradeToSecure(), which is proposed in the discussion thread, and leave the return type void in this first implementation.

	Patrick Wang (Chih-Kai Wang) (:kk1fff) Assignee
	Comment 47 • 11 years ago

Address comment 44 and comment 46. Jonas, would you take a look at the interface change?

	Honza Bambas (:mayhemer)
	Comment 48 • 11 years ago

Comment on attachment 797086 [details] [diff] [review]
Patch: Implement startTLS in MozTCPSocket v.3

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

Andrew, you have not addressed comment 43.

	Honza Bambas (:mayhemer)
	Comment 49 • 11 years ago

(In reply to Honza Bambas (:mayhemer) from comment #48)
> Comment on attachment 797086 [details] [diff] [review]
> Patch: Implement startTLS in MozTCPSocket v.3
> 
> Review of attachment 797086 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Andrew, you have not addressed comment 43.

(By means of what is your plan, at least).

	Andrew Sutherland [:asuth] (he/him) Reporter
	Comment 50 • 11 years ago

(In reply to Honza Bambas (:mayhemer) from comment #49)
> > Andrew, you have not addressed comment 43.
> 
> (By means of what is your plan, at least).

Did you mean Patrick?  I don't have much knowledge of the network stack in terms of what's exposed to JS for crypto layering; I've always asked for 'starttls' crypto when opening a socket in XPCOM land.

I do think that the security argument can be made that you should know when opening a connection whether you are going to try and perform a TLS upgrade since "only upgrade to TLS if the insecure connection which could just be an attacker says that TLS-upgrade is available" is not safe.  Which is an argument for requiring useSecureTransport to be set to 'starttls' in that case, or something like it.  The spec's TCPOptions dictionary seems like it will need to be updated to something like that in the future.

	Patrick Wang (Chih-Kai Wang) (:kk1fff) Assignee
	Comment 51 • 11 years ago

(In reply to Honza Bambas (:mayhemer) from comment #49)
> (In reply to Honza Bambas (:mayhemer) from comment #48)
> > Comment on attachment 797086 [details] [diff] [review]
> 
> (By means of what is your plan, at least).

I will file a follow up to provide an interface to create a non-secure TCPSocket. We can do this through changing |useSecureTransport| to a string or using an additional boolean in TCPOptions as comment 26 mentioned.

	Honza Bambas (:mayhemer)
	Comment 52 • 11 years ago

(In reply to Andrew Sutherland (:asuth) from comment #50)
> Did you mean Patrick?  I don't have much knowledge of the network stack in

Ah!  Yes, sorry :)

	Honza Bambas (:mayhemer)
	Comment 53 • 11 years ago

(In reply to Patrick Wang [:kk1fff] from comment #51)
> I will file a follow up to provide an interface to create a non-secure
> TCPSocket. We can do this through changing |useSecureTransport| to a string
> or using an additional boolean in TCPOptions as comment 26 mentioned.

Thanks!  And again, sorry for the first confusion about this.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 54 • 11 years ago

Comment on attachment 797086 [details] [diff] [review]
Patch: Implement startTLS in MozTCPSocket v.3

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

What happens if you call send() right after calling upgradeToSecure(), but before the actual upgrade has had time to happen? Does it result in an exception? Does the data get sent once the upgrade has happened? What happens if the upgrade fails, does the data get sent unsecurely?

I sort of like what I think the spec proposes, which is that when you call upgradeToSecure() we change readystate to "upgrading". If you call send() during that state an exception is thrown. Once the upgrade is successful or fails, the readystate goes back to "open". We should also somewhere on the API expose if the current connection is secure or not. Either by making secure connections use "opensecure" as readystate, or by having a separate .secure property or some such.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 55 • 11 years ago

Alternatively, it might be easier to handle a failed upgrade by simply disconnecting the connection. That way no data is inadvertently sent over an insecure connection. If we do that we might not need the "upgrading" readystate.

	Honza Bambas (:mayhemer)
	Comment 56 • 11 years ago

(In reply to Jonas Sicking (:sicking) from comment #54)
> Comment on attachment 797086 [details] [diff] [review]
> Patch: Implement startTLS in MozTCPSocket v.3
> 
> Review of attachment 797086 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What happens if you call send() right after calling upgradeToSecure(), but
> before the actual upgrade has had time to happen? 

The data are written to the secure socket that starts the negotiation.  This is how PSM/NSS secure socket is written.

To start the update you actually need to write data to the socket.

> Does it result in an
> exception? 

Obviously no.

> Does the data get sent once the upgrade has happened? 

Yes.

> What
> happens if the upgrade fails, does the data get sent unsecurely?

No.  If the secure connection fails from whatever reason (often because of cert issues or cipher negotiation issues) you get an error and the socket closes.

> 
> I sort of like what I think the spec proposes, which is that when you call
> upgradeToSecure() we change readystate to "upgrading". If you call send()
> during that state an exception is thrown. Once the upgrade is successful or
> fails, the readystate goes back to "open". 

Is there a notification specified when the socket is again at "open" state?

It would mean we drive the ssl negotiation by pushing 0 bytes to the socket while getting WOULD_BLOCK.  When we get OK, the update is done.  When we ERROR update (ssl connection) has failed.

I'm not sure this is fully doable in JS, but I may check.  If not, it might just complicate the TCPSocket code.

> We should also somewhere on the
> API expose if the current connection is secure or not. Either by making
> secure connections use "opensecure" as readystate, or by having a separate
> .secure property or some such.

Agree.

(In reply to Jonas Sicking (:sicking) from comment #55)
> Alternatively, it might be easier to handle a failed upgrade by simply
> disconnecting the connection. 

That is the behavior.


> That way no data is inadvertently sent over an
> insecure connection. If we do that we might not need the "upgrading"
> readystate.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 57 • 11 years ago

Current approach sounds good then. Would be good to expose if we are on a secure connection or not through a property separate from readyState. But we might already do that? Either way that can be a separate bug/patch

	Patrick Wang (Chih-Kai Wang) (:kk1fff) Assignee
	Comment 58 • 11 years ago

(In reply to Jonas Sicking (:sicking) from comment #57)
> Current approach sounds good then. Would be good to expose if we are on a
> secure connection or not through a property separate from readyState. But we
> might already do that? Either way that can be a separate bug/patch

Current readyState doesn't contain the secure state, I will file a bug for reporting the state of secure connection.

	Patrick Wang (Chih-Kai Wang) (:kk1fff) Assignee
	Comment 60 • 11 years ago

https://hg.mozilla.org/integration/b2g-inbound/rev/1000c23df772

	Ed Morley [:emorley]
	Comment 61 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/1000c23df772

	Donovan Preston [:fzzzy]
	Comment 62 • 11 years ago

Woohoo! Awesome that this is fixed! Thanks Patrick and everyone who helped!

	Andrew Sutherland [:asuth] (he/him) Reporter
	Comment 63 • 11 years ago

This works in single-process SSL operation but regressed multi-process SSL operation.  This caused bug 912147.

The problem is in TCPSocket.prototype.open:

    if (this._inChild) {
      that._socketBridge = Cc["@mozilla.org/tcp-socket-child;1"]
                             .createInstance(Ci.nsITCPSocketChild);
      that._socketBridge.open(that, host, port, !!that._ssl,
                              that._binaryType, this.useWin, this.useWin || this);
      return that;
    }

Since that._ssl will be set to 'ssl', and !!'ssl' becomes `true`, then in the child, _createTransport will receive an sslMode of `true`, which is not `=== 'ssl'`, so it will create the socket in startTLS mode.


It's unclear to me if this patch should be backed out and re-landed or a new follow-up bug filed.  It is pretty clear that we'll need to get SSL tests in the tree for this (bug 466524).  I'm speculatively reopening.

	Andrew Sutherland [:asuth] (he/him) Reporter
	Comment 64 • 11 years ago

Sorry, jet-lag problem analysis fail.

The problem looks to be in TCPSocketParentIntermediary.  

That looks like:

  open: function(aParentSide, aHost, aPort, aUseSSL, aBinaryType) {
    let baseSocket = Cc["@mozilla.org/tcp-socket;1"].createInstance(Ci.nsIDOMTCPSocket);
    let socket = baseSocket.open(aHost, aPort, {useSSL: aUseSSL, binaryType: aBinaryType});

The problem there is that useSSL should have been changed to useSecureTransport.  Probably.  Jet-lag, you know?  But definitely we are not establishing an SSL connection at the other end.  Definitely.

	Andrew Sutherland [:asuth] (he/him) Reporter
	Comment 65 • 11 years ago

Here's the patch that fixes things for me.

	Andrew Sutherland [:asuth] (he/him) Reporter
	Comment 66 • 11 years ago

Backed out per advice from Fabrice (and since this is a weekend so a review of my suggested fix might not be fast enough for us to have working Firefox OS builds for this work week):
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bf2b6cf8d73

	Patrick Wang (Chih-Kai Wang) (:kk1fff) Assignee
	Comment 67 • 11 years ago

Comment on attachment 801213 [details] [diff] [review]
[interdiff between v.checkin and atchm 801231] fix-tcpsocket-v1.diff

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

Sorry, I've missed that when changing the option name. Thanks for catching it, Andrew!

	Patrick Wang (Chih-Kai Wang) (:kk1fff) Assignee
	Comment 68 • 11 years ago

Having grep-ed for all 'useSSL' in tcpsocket's implementation, the code I actually modified is only the line that Andrew had mentioned. I have tested SSL connections in OOP and inproc, they both works.

	Honza Bambas (:mayhemer)
	Comment 69 • 11 years ago

Comment on attachment 801231 [details] [diff] [review]
Patch: Implement startTLS in MozTCPSocket

If this fixes the problem, then go ahead (no time and resources to test this my self).

This is just a prove that tests must go in WITH the patch and NOT AFTER.

	Patrick Wang (Chih-Kai Wang) (:kk1fff) Assignee
	Comment 70 • 11 years ago

re-land: https://hg.mozilla.org/integration/b2g-inbound/rev/9edc229b7d09

	Ryan VanderMeulen [:RyanVM]
	Comment 71 • 11 years ago

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

	Andrew Sutherland [:asuth] (he/him) Reporter
	Comment 72 • 11 years ago

Bug 914520 is a leo+'d blocker to support German ISPs that only support startTLS SMTP upgrades on port 587 and do not support (initial) SSL on port 465.  This bug should be marked leo+ for clarity.

I have prepared an uplift patch and am testing it now; the existing patch does not apply cleanly because mozilla-b2g18 does not have the TCP server socket changes.

The changes of the uplift patch from the mozilla-central landing are:
- New UUIDs have been generated so anyone using the UUIDs directly would not be confused.
- Sticking with useSSL instead of useSecureTransport.  My rationale is that since this is such a late-landing change for v1.1, the de facto interface should not be changed.

	Andrew Sutherland [:asuth] (he/him) Reporter
	Comment 73 • 11 years ago

xpcshell tests for a b2g-desktop build work.  Having trouble getting gaia to work in b2g at all.  Will spin an unagi build and verify on the unagi.

	Andrew Sutherland [:asuth] (he/him) Reporter
	Comment 74 • 11 years ago

I performed testing on an unagi device using the patched version of the e-mail app (in addition to the xpcshell-tests).

Landed (on the authority of blocking-b2g=leo+ bug 914520):
https://hg.mozilla.org/releases/mozilla-b2g18/rev/bebaca30ea30

	Dylan Oliver [:doliver]
	Comment 75 • 11 years ago

triage: leo+, raised as high priority during work week (already landed)

	Ryan VanderMeulen [:RyanVM]
	Comment 76 • 11 years ago

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/bebaca30ea30

	Jeremie Patonnier :Jeremie
	Comment 77 • 11 years ago

Documented here:
https://developer.mozilla.org/en-US/docs/Web/API/TCPSocket.upgradeToSecure