Last Comment Bug 60377 - [RFE] Support IMAP STARTTLS command
: [RFE] Support IMAP STARTTLS command
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Networking: IMAP (show other bugs)
: Trunk
: All All
: P3 enhancement with 16 votes (vote)
: ---
Assigned To: David :Bienvenu
: esther
Mentors:
http://www.ietf.org/rfc/rfc2595.txt
: 239057 (view as bug list)
Depends on:
Blocks: 270483
  Show dependency treegraph
 
Reported: 2000-11-16 13:25 PST by Andrew Taylor
Modified: 2009-01-22 10:17 PST (History)
24 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
proposed fix (6.22 KB, patch)
2005-01-20 13:40 PST, David :Bienvenu
mscott: superreview+
Details | Diff | Splinter Review
fix incorporating Christian's comments (9.15 KB, patch)
2005-01-21 17:37 PST, David :Bienvenu
mscott: superreview+
Details | Diff | Splinter Review
fix TLS and implement LOGINDISABLED (6.89 KB, patch)
2005-01-28 15:50 PST, David :Bienvenu
mscott: superreview+
Details | Diff | Splinter Review

Description Andrew Taylor 2000-11-16 13:25:08 PST
Add support for the STARTTLS IMAP command, which allows a client to request the
communication channel be changed from cleartext to SSL/TLS after a session has
started.  This IMAP extension is documented in RFC 2595, available here:
http://www.imc.org/rfc2595.

The Linux imapd included in the imap-2000 package from Redhat supports this
extension.
Comment 1 Keyser Sose 2000-12-18 17:44:36 PST
Marking NEW so someone will look at it.
Comment 2 Kai Engert (:kaie) 2003-03-03 12:52:04 PST
Marking fixed, we have been supporting this feature for a long time already.
Comment 3 Andrew Taylor 2003-03-03 13:17:42 PST
How does one enable this feature?  When I connect to a STARTTLS enabled server
with 1.3b, mozilla doesn't initiate the secure connection.

Looking at the network traffic, I see mozilla do an IMAP CAPABILITY command. 
The server responds with a CAPABILITY string including the STARTTLS feature. 
Mozilla then procedes to do the usual login stuff.  I would expect mozilla to
send a STARTTLS message to the server immediately after the CAPABILITY.

I also tried setting the "Use Secure Connection (SSL)" checkbox and connecting
on port 143, but that results in an SSL certificate error.
Comment 4 Kai Engert (:kaie) 2003-03-03 13:22:51 PST
Sorry, my fault. I misread this request, I was thinking about SMTP, not IMAP...

Reopening, not yet fixed.
Comment 5 Ere Maijala (slow) 2003-03-31 12:48:07 PST
We should also support capability LOGINDISABLED. It tells that the server won't
accept plain-text LOGIN and requires STARTTLS.
Comment 6 (not reading, please use seth@sspitzer.org instead) 2003-04-07 23:54:43 PDT
cavin was just asking me about this today.

we do have imap over ssl, but not starttls support.  (we do have it for smtp)

you'll want to take a look at nsImapProtocol::SetupWithUrl() (and
http://lxr.mozilla.org/mozilla/source/mailnews/compose/src/nsSmtpProtocol.cpp)
for the places to start looking.

helpwante, but re-assign to cavin.  (better to rot on his list, than mscott.)
Comment 7 Rui Carmo 2003-07-22 16:24:34 PDT
Well, i've noticed a worrying trend in corporate firewalls - port 993 has become a "forbidden" port 
in ultra-paranoid firewall setups (since most security people these days subscribe to the "we'd 
rather not have encrypted traffic leave our firewall" or the "let them use STARTTLS, Exchange 
supports it" schools of thought).

Any chance this bug may get bumped to a slightly higher priority?

Assigned to someone on Thunderbird who is willing to merge ESMTP and IMAP support?

(I can't delve into the code that deep yet)
Comment 8 David :Bienvenu 2003-10-02 11:05:46 PDT
taking - shouldn't be that hard (famous last words :-) )
Comment 9 Christian Eyrich 2003-10-02 12:05:05 PDT
Oh, so bug 218902 is a dupe in case of IMAP. Should we change that bug to POP only?
Comment 10 David :Bienvenu 2003-10-02 12:09:59 PDT
yes, done.
Comment 11 Christian Eyrich 2003-10-03 07:45:53 PDT
I posted a patch v2 in bug 218902. Because the relevant UI of POP is the same as
for IMAP, anyone interested in this bug should have a look at that proposal.
Comment 12 John Robinson 2004-02-17 10:37:59 PST
Christian - the pop3 patch looks good to me. Just to add a few more spanners in
the works though... It looks like you still don't support STARTTLS moving to
SSL, only STARTTLS moving to TLS, though I may well have that wrong if the TLS
code negotiatiates whatever's required to support either/both. I note that
Mulberry offers the two as different options, however. As noted, the imapd from
Redhat supports this, and it's the University of Washington imapd; the UW popd
supports this too, and these may well be the most common imap/pop daemons out
there on the 'net :-)

I wish I was a better programmer to have a go at this, but I get lost with SSL
etc just in the documentation; I just know I want my servers to run SSL/TLS and
my client software to support it too.

Anyway, thanks for all your good work, guys!
Comment 13 Christian Eyrich 2004-02-17 11:07:01 PST
The already available SSL supports SSL and TLS and so does STARTTLS. SMTP's
STARTTLS also supports both.
Comment 14 Christian Eyrich 2004-04-05 07:00:37 PDT
*** Bug 239057 has been marked as a duplicate of this bug. ***
Comment 15 Sean Brandt 2004-06-11 12:23:18 PDT
What's the status of this issue? It's been new, a patch for pop3 is available,
but no work since Feb.
Comment 16 Michael Zimmermann 2004-12-09 06:11:50 PST
Any chance to see further work on this bug? Just asking, because I do have
servers that only support imap with tsl and not imaps.
Comment 17 MZM 2005-01-20 06:14:46 PST
I was just planing to force office move to moz based IMAP client. Now it looks,
I will need some other (propertary) e-mail app. Any chances to get fixed this
bug? Is somebody workin on it?
Comment 18 Christian Eyrich 2005-01-20 06:52:36 PST
(In reply to comment #17)
> I was just planing to force office move to moz based IMAP client. Now it looks,
> I will need some other (propertary) e-mail app.

Do you really need STARTTLS? Though we should definitely support it, most
servers use IMAP over SSL/TLS on a special port (mostly 993).
Comment 19 Peter M. Jansson 2005-01-20 11:23:04 PST
(In reply to comment #18)
> Do you really need STARTTLS? Though we should definitely support it, most
> servers use IMAP over SSL/TLS on a special port (mostly 993).

I'm not sure which "most" you mean, but UW-IMAPD, Cyrus, Courier, Dovecot,
Oracle Collaboration Suite and Exchange all support STARTTLS, and I'd be happier
if I didn't need the extra firewall hole.
Comment 20 David :Bienvenu 2005-01-20 11:24:52 PST
yes, this isn't hard to do.
Comment 21 RL "Bob" Morgan 2005-01-20 12:15:11 PST
(In reply to comment #18)
> (In reply to comment #17)
> > I was just planing to force office move to moz based IMAP client. Now it looks,
> > I will need some other (propertary) e-mail app.
> 
> Do you really need STARTTLS? Though we should definitely support it, most
> servers use IMAP over SSL/TLS on a special port (mostly 993).

(1)  STARTTLS is the method specified in the IMAP spec set (RFC 2595 to be
specific) for support of SSL/TLS.  There is no official standing for the
separate-port approach.  Most people do it because they're operating by analogy
with https, but it's a non-standard hack.  Seems to me Mozilla talks a lot about
promoting standards.  Being realistic by supporting the separate-port hack is
fine, but not being standards-compliant (especially when it's easy and your
users are asking for it and servers all support it) is just not right.

(2)  The real point is that to use the separate-port hack, the user has to go
into bizarre setup land and configure their client to use it.  The real win of
STARTTLS is that the client can have a default policy of "use TLS if server
offers it", and *it will just work* without the user having to do anything.  The
way it is now, sites who want their clients to just work without having them
configure stuff *can't use SSL/TLS* and probably run with password-in-the-clear
for this reason.  This is sad and completely avoidable.
Comment 22 William Winton 2005-01-20 12:33:51 PST
We do need this fixed.  Many hosting providers running IMAP servers already
support STARTTLS (like Courier and Exchange).  So the admins of those sites are
less likely to graft on SSL (via stunnel?) support.
Comment 23 Christian Eyrich 2005-01-20 12:51:36 PST
(In reply to comment #21)
> (1)  STARTTLS is the method specified in the IMAP spec set (RFC 2595 to be
> specific) for support of SSL/TLS.  There is no official standing for the
> separate-port approach.  Most people do it because they're operating by analogy
> with https, but it's a non-standard hack.  Seems to me Mozilla talks a lot about
> promoting standards.

Well, I know that and fully agree. See my comment #11, if I would know the IMAP
code I've already done it.
But telling David that maybe has some effect. :)
Comment 24 Christian Eyrich 2005-01-20 12:53:33 PST
(In reply to comment #20)
> yes, this isn't hard to do.

I know a guy who wrote the following some comments above:
> taking - shouldn't be that hard (famous last words :-) )
Comment 25 David :Bienvenu 2005-01-20 13:40:29 PST
Created attachment 171919 [details] [diff] [review]
proposed fix

STARTTLS for IMAP, used by default if server supports STARTTLS, and user hasn't
turned useTLS pref off
Comment 26 Scott MacGregor 2005-01-20 13:57:02 PST
Comment on attachment 171919 [details] [diff] [review]
proposed fix

1) bump the IID in nsIMsgIncomingServer

2) will adding mail.server.default.useTLS have any negative effect on non imap
servers?
Comment 27 David :Bienvenu 2005-01-20 14:13:43 PST
thx, I'll bump the IID. 

 mail.server.default.useTLS won't affect other servers, yet. When we do starttls
for pop3, I would expect that pref to be able to turn it off.
Comment 28 Christian Eyrich 2005-01-20 15:12:56 PST
(In reply to comment #27)
> thx, I'll bump the IID. 
> 
> mail.server.default.useTLS won't affect other servers, yet. When we do
> starttls for pop3, I would expect that pref to be able to turn it off.

In my patch for bug 218902 (POP3 case) I introduced mail.server.default.useSSL
resp. useSSL with type int.
That to get rid of isSecure and being able of holding all four states in one
pref as we it for SMTP. I waited over one year with that POP3 patch to
synchronize the changes (e.g. ns(I)MsgIncomingServer) with the IMAP case. The UI
part could also be shared.

So it would be nice if we could talk about that before your checkin.
Comment 29 David :Bienvenu 2005-01-20 15:30:05 PST
I'm all for sharing...however, imap is a little different (simpler). The server
tells us if it supports STARTTLS so we don't have to guess...And, the always use
TLS, even when not available, doesn't make sense to me in the imap context. And
finally, if you want me to be able to use the settings for that pref, they can't
be in the pop3 protocol code, but need to be more general, like in
nsIMsgIncomingServer.idl.

My thinking was that IMAP would always use TLS, if available, and maybe we'd
have a pref UI to turn it off, maybe in advanced settings. But we can try to
merge the approaches somehow...
Comment 30 Christian Eyrich 2005-01-21 05:40:08 PST
(In reply to comment #29)
> I'm all for sharing...however, imap is a little different (simpler). The
> server tells us if it supports STARTTLS so we don't have to guess...And, the
> always use TLS, even when not available, doesn't make sense to me in the imap
> context.

The POP code doesn't has to guess too since STLS in CAPA tells the client if
STARTTLS is available.
To me "always use TLS" makes sense. If the user only wants to communicate over
encrypted and authenticated link we should obey him. Working on the SMTP over
SSL patch I read the following argumentation a few times:
What happens if an attacker hijacks the connection to the server? "TLS if
available" would just use a normal link when the attacker doesn't offer STARTTLS
on his machine while the always option would fail. And it would also fail if the
offerst TLS because of the wrong certificate.

> And finally, if you want me to be able to use the settings for that pref,
> they can't be in the pop3 protocol code, but need to be more general, like in
> nsIMsgIncomingServer.idl.

Sure - look at my patch, I used ns(I)MsgIncomingServer.

> My thinking was that IMAP would always use TLS, if available, and maybe we'd
> have a pref UI to turn it off, maybe in advanced settings. But we can try to
> merge the approaches somehow...

And the current behaviour of using a different port and POP/IMAP over SSL?
Having two options/prefs (isSecure and useTLS) will be confusing. From your
patch it looks ssl will be used if activated even if useTLS is too - I don't
think that's ok.
The UI in my POP patch could also be used for IMAP and is the same as for SMTP,
so it would be consistent.
Comment 31 David :Bienvenu 2005-01-21 11:07:39 PST
I understand your points about why all 4 settings are useful in some scenarios,
but I'm not convinced they're useful enough to clutter up the UI. But that's
relatively orthogonal since I can do the backend without worrying about the
front end (i.e., respect the prefs, independent of the UI for setting them)

My point about nsIMsgIncomingServer.idl is that the enum is not defined there.
You have this:

+  /* should we use a secure channel? */
+  attribute long useSSL;
+

but no indication of what the four values for useSSL are. That is defined in
nsPop3Protocol.h :

+typedef enum _UseSSL {
+    PREF_SECURE_NEVER = 0,
+    PREF_SECURE_TRY_STLS = 1,
+    PREF_SECURE_ALWAYS_STLS = 2,
+    PREF_SECURE_ALWAYS_POP3S = 3
+} UseSSL;
+

where I can't get at them. What I meant was the enum values need to be in
nsIMsgIncomingServer.idl. I don't want to duplicate them in the imap code.

So I'm going to add an enum for the socketType to my changes to
nsIMsgIncomingServer.idl, and switch over to that in the imap code. I think the
name useSSL is not sufficiently descriptive - I think socketType is better, and
matches what necko calls the arg passed to CreateTransport. The alternative is
connectionType, but I think socketType is OK.

Thoughts?
Comment 32 Christian Eyrich 2005-01-21 12:15:30 PST
(In reply to comment #31)
> but no indication of what the four values for useSSL are. That is defined in
> nsPop3Protocol.h

Ah, now I got it. You're right.

> I think the name useSSL is not sufficiently descriptive - I think socketType
> is better, and matches what necko calls the arg passed to CreateTransport.

That's fine for me too.
Comment 33 David :Bienvenu 2005-01-21 12:23:48 PST
hmm, if we use one pref, useTLS, or socketType, to subsume isSecure, then we've
got to worry about forwards and backwards compatibility. I.e., we need to read
the  old value of isSecure and set socketType accordingly, but we only want to
do once.  Then we'd need to convert all the callers of GetIsSecure to use
socketType, or change the impl of GetIsSecure to check the socketType
value...But the biggest concern is migrating the isSecure pref to the new
aggregrate pref. That's a bit of a pain because you need to add a version pref
that says we've upgraded the prefs, and we need to find a place in the code to
put the pref upgrading...
Comment 34 Christian Eyrich 2005-01-21 14:26:07 PST
(In reply to comment #33)
> hmm, if we use one pref, useTLS, or socketType, to subsume isSecure, then
> we've got to worry about forwards and backwards compatibility.

Yes, I know.

I hoped we could ignore compatibility in this point. But maybe we can just
ignore forwards compatibility but be backwards compatible if we keep the
isSecure pref and read it in GetSocketType. If isSecure is false we can use the
socketType value, is isSecure true, socketType is set to 3 AND isSecure set to
false.

Calls of GetIsSecure and SetIsSecure for POP and IMAP would have to be converted.
Comment 35 David :Bienvenu 2005-01-21 15:38:25 PST
>If isSecure is false we can use the socketType value, if isSecure true,
>socketType is set to 3 AND isSecure set to false.

Forwards compatibility is an issue, especially for developers, but also for
people who try new builds and then go back to an older build, so I don't think
we can go clearing that pref in prefs.js on upgrade.  We could only pay
attention to isSecure if the new pref had its default value (currently TryTLS)
So I'd propose the following logic:

NS_IMETHODIMP nsMsgIncomingServer::GetSocketType(PRInt32 *aSocketType)
{
  nsCAutoString fullPrefName;
  getPrefName(m_serverKey.get(), "socketType", fullPrefName);
  nsresult rv = m_prefBranch->GetIntPref(fullPrefName.get(), aSocketType);

  // socketType is set to default value. Look at isSecure setting
  if (NS_FAILED(rv))
  {
    PRBool isSecure;
    rv = getBoolPref("isSecure", &isSecure);
    if (NS_SUCCEEDED(rv) && isSecure)
    {
       *aSocketType = nsIMsgIncomingServer::useSSL;
       SetSocketType(*aSocketType);
    }
    else
    {
       getDefaultIntPref("socketType", aSocketType);
    }
  }
  return rv;
  
}

After this code runs, if isSecure was true, that pref will effectively be
ignored going forward and we'll use the socket type pref. If it was false, we'll
use the socketType value going forward. This is all less than ideal because if
the socketType is set to the default, we'll still try to read the isSecure pref,
though it won't have any effect unless it has been changed to true.  There are a
couple alternatives - adding a socket version upgrade pref, which we would have
to check every time, and would be written to prefs.js, and having a default
value for socketType that wasn't valid, so we'd detect the invalid value and
upgrade to a valid value. That's a bit ugly, because the account settings code
would also have to worry about that, and we'd end up always writing out a value
for the pref.

So I think my proposal might be the least bad, though I'm not crazy about it and
open to suggestions that don't break old profiles.
Comment 36 John Robinson 2005-01-21 17:19:21 PST
(In reply to comment #30)
> What happens if an attacker hijacks the connection to the server? "TLS if
> available" would just use a normal link when the attacker doesn't offer STARTTLS
> on his machine while the always option would fail. And it would also fail if the
> offerst TLS because of the wrong certificate.

Well, just my 2c here, but I'd quite like to see this option (and perhaps others
around POP3 and SMTP) work on a configuration-time or first-time basis, not vary
their behaviour per connection; that is to say, if I pick "TLS if available" and
it works first time, then the stored setting ought to end up being "TLS always"
or at least I ought to get a warning if TLS suddenly stops working. Also, as
already noted, I ought to get a warning if the certificate was the wrong one or
had changed (the same as any decent SSH client). As I'm not much of a coder (not
C/C++/Java on a megaproject like this, anyway), this is probably something I
shouldn't be asking for here, but thank you all for your good work!
Comment 37 David :Bienvenu 2005-01-21 17:37:02 PST
Created attachment 172054 [details] [diff] [review]
fix incorporating Christian's comments

this expands on the old patch, adopting Christian's comments about using one
pref, socketType.
Comment 38 Christian Eyrich 2005-01-22 07:39:29 PST
(In reply to comment #37)
> Created an attachment (id=172054) [edit]
> fix incorporating Christian's comments

That handling in GetSocketType() is good. What about also adjust SetSocketType()
so isSecure is set according to socketType? E.g.
if (aSocketType == useSSL)
  SetIntValue("isSecure", true);
if (aSocketType == defaultSocket)
  SetIntValue("isSecure", false);

Switching between builds back and forth is nice but while being careful in
changing interfaces and prefs we shouldn't overrate compatibility forever.
Relevance of isSecure pref will decrease with every future version.
Comment 39 David :Bienvenu 2005-01-22 08:07:30 PST
wouldn't it just be:

SetBoolValue("isSecure", aSocketType == useSSL);

But this would seem to prolong the relevancy of "isSecure", which undermines
your point a little :-) I'm mostly concerned about upgrading and not diddling
the older build's pref under normal situations. I don't mind if the prefs
eventually diverge, if that makes sense.

But that reminds me - the account settings UI code doesn't call GetSocketType,
does it? It looks at the pref directly. So either you need to add a call to
GetSocketType in there so the socketType pref gets automatically upgraded,
before the account settings code loads the pref, or you need to duplicate that
logic. I think just adding a call to GetSocketType with a comment about why
would be the easiest thing to do. 
Comment 40 Christian Eyrich 2005-01-22 11:48:50 PST
(In reply to comment #39)
> SetBoolValue("isSecure", aSocketType == useSSL);

Hmpf, of course.

> But this would seem to prolong the relevancy of "isSecure", which undermines
> your point a little :-)

As I wrote being careful is good, so do what's possible but only without
extraordinary costs.

> But that reminds me - the account settings UI code doesn't call GetSocketType,
> does it? It looks at the pref directly. So either you need to add a call to
> GetSocketType in there so the socketType pref gets automatically upgraded,
> before the account settings code loads the pref, or you need to duplicate that
> logic.

The patch needs to be overhauled anyway, I'll take care of that.
Comment 41 David :Bienvenu 2005-01-24 09:36:03 PST
fixed on trunk.

Thinking about it again, I think you were right:

if (aSocketType == useSSL)
  SetBoolValue("isSecure", true);
if (aSocketType == defaultSocket)
  SetBoolValue("isSecure", false);

the other choices might end up turning off SSL in the older builds if the user
decided they wanted TLS in the newer build...but I haven't checked in anything
yet to do with setting isSecure.
Comment 42 Christian Eyrich 2005-01-27 06:43:55 PST
Though I don't fully understand the IMAP code and can't test with a STARTTLS
able server, I doubt this is fixed.

I can't find a place in the IMAP code where the command "STARTTLS" is issued or
connection is switched from unencrypted to encrypted.
And I also think 
 && (capability & kHasStartTLSCapability)
on nsImapProtocol.cpp#765 is wrong since at the time the transport is created,
kHasStartTLSCapability can't already be set (resp. if capability infos are
cached it can't be set at least for the first connection in a session).
Comment 43 Roland Stuehmer 2005-01-27 07:30:43 PST
UI controls would be nice, too, once your at it.
Comment 44 Christian Eyrich 2005-01-27 07:38:12 PST
(In reply to comment #43)
> UI controls would be nice, too, once your at it.

The UI is shared between POP3 and IMAP and included in patch of bug 218902.
Please let me know there what you think.
Comment 45 David :Bienvenu 2005-01-27 09:46:58 PST
you could be right, but,

+          connectionType = "starttls";

and connectionType is passed to CreateTransport. You're right that the very
first time you connect to an imap server, we can't know the capabilities so we
won't use TLS if you say to use it when available, but since we write the
capabilities to prefs.js, we should TLS every time after that, even with the
first connection after restart...

I don't know what we have to do to set the connection as encrypted. I thought
necko did that...
Comment 46 David :Bienvenu 2005-01-27 10:07:17 PST
you're right about sending the command STARTTLS, I misread that comment -
reopening...
Comment 47 Christian Eyrich 2005-01-27 10:10:17 PST
(In reply to comment #45)
This comment might be unnecessary after reopening, but since I put all the work
in it, I'll post it nevertheless. :)

> but since we write the capabilities to prefs.js, we should TLS every time
> after that, even with the first connection after restart...

Ah ok, POP doesn't cache CAPA informations.
But given the following I still think the line cited is unnecessary.

> I don't know what we have to do to set the connection as encrypted. I thought
> necko did that...

I only know what SMTP does. But since it also (indirectly) passes "starttls" to
CreateTransport() but doesn't connect encrypted I think as follows:
1. if types is nsnull, no encryption is used and can't be enabled afterwards.
2. if type is "ssl", encryption is used right from the start
3. if types is "starttls", no encryption is used but can switched afterwards.

SMTP code and my POP patch do this by calling sslControl->StartTLS(), see
nsSmtpProtocol::SendTLSResponse().

From RFC 2595 chapter 3, it's clear the client has to tell the server the wish
to start TLS while in unencrypted state and switches after positive response.
Comment 48 Christian Eyrich 2005-01-28 06:34:58 PST
I noted another problem. When setting socketType using SetSocketType(), it will
clear the pref if the value is the default value (in SetIntValue()).
If isSecure is set, socketType will be returned as useSSL when GetSocketType()
is called next time. I.e. the value the user sets isn't always the value used!

One workarounds would be adding
  SetBoolValue("isSecure", aSocketType == useSSL);
in SetSocketType(). This has two drawbacks: the one you noted in comment #41 and
it won't work if we use the UI Roland proposed in bug 218902 comment #19. It
would use isSecure for the checkbox and socketType for the radios.

Please let's talk about that before continuing with the backend.
Comment 49 David :Bienvenu 2005-01-28 11:05:19 PST
I think the imap backend work can continue, since the imap backend is mostly
orthogonal, unless you think the current GetSocketType call is unworkable. All
the imap backend has to do is respect the GetSocketType return value correctly.
What prefs implement that return value is orthogonal.

I think your suggestion of this code avoids the problem I mentioned in #c41 and
the one you mention in #c48 :

if (aSocketType == useSSL)
  SetBoolValue("isSecure", true);
if (aSocketType == defaultSocket)
  SetBoolValue("isSecure", false);

Re the UI approach, I'm OK with keeping the isSecure pref - in other words, I
think we should pick the best UI for the user, and make that determine what
pref(s) we set.
Comment 50 Christian Eyrich 2005-01-28 13:54:53 PST
(In reply to comment #49)
> I think the imap backend work can continue, since the imap backend is mostly
> orthogonal, unless you think the current GetSocketType call is unworkable.
> All the imap backend has to do is respect the GetSocketType return value
> correctly.

I meant the code in GetSocketType() and it's usage in SetupWithUrl(). My plan is
doing the following (shortened):
PRBool isSecure = PR_FALSE;
m_socketType = nsIMsgIncomingServer::defaultSocket;
server->GetIsSecure(&m_isSecure);
if (isSecure)
  server->GetSocketType(&m_socketType);

That way we'd only respect socketType if is secure connection is switched on in
general.

> Re the UI approach, I'm OK with keeping the isSecure pref - in other words, I
> think we should pick the best UI for the user, and make that determine what
> pref(s) we set.

If we're going to use isSecure in the UI in order to switch on/off any
encryption (serverTypeElement.disabled == !isSecure) it's wrong manipulating
isSecure in any other place. So the code mentioned in #c41 and #c48 would be wrong.
SetSocketType() shouldn't use SetIntValue() but directly write the pref without
clearing it if the value to write is the default.
Comment 51 David :Bienvenu 2005-01-28 15:15:32 PST
That doesn't seem consistent. Use TLS if available, for example, is not secure,
if TLS is not available.
Comment 52 David :Bienvenu 2005-01-28 15:50:59 PST
Created attachment 172757 [details] [diff] [review]
fix TLS and implement LOGINDISABLED

this should get TLS working, and implements LOGINDISABLED, which is required if
you implement STARTTLS. Re isSecure, it really needs to be orthogonal to the
sockettype, since we use isSecure for the port number, among other things!
Comment 53 Christian Eyrich 2005-01-29 05:03:00 PST
(In reply to comment #51)
> That doesn't seem consistent. Use TLS if available, for example, is not secure,
> if TLS is not available.

What means the UI Roland proposed isn't useable.

> Re isSecure, it really needs to be orthogonal to the sockettype, since we use
> isSecure for the port number, among other things!

What means
if (aSocketType == useSSL)
  SetBoolValue("isSecure", true);
if (aSocketType == defaultSocket)
  SetBoolValue("isSecure", false);
in SetSocketType() is necessary AND it shouldn't use SetIntValue() in any case.
Comment 54 David :Bienvenu 2005-01-29 12:55:37 PST
>What means the UI Roland proposed isn't useable.

Yes, I think it's just wrong...we'd have to remove the "use tls if available"
option to make that workable, and since I think that should be the default
option, I don't think we should remove it :-)

yes, I think this code is fine:

if (aSocketType == useSSL)
  SetBoolValue("isSecure", true);
if (aSocketType == defaultSocket)
  SetBoolValue("isSecure", false);
Comment 55 David :Bienvenu 2005-02-02 11:27:18 PST
Comment on attachment 172757 [details] [diff] [review]
fix TLS and implement LOGINDISABLED

please ignore the timeout part, which comes from another patch...

I'm going to turn off tls by default for now, since there are issues with
security dialogs popping up because some isps send certs that don't match the
host name or something like that...
Comment 56 David :Bienvenu 2005-02-03 08:01:13 PST
fix checked in - and as I said earlier, I also changed the default to non-secure
because TLS by default will have issues. We might want to add TLS to the new
account UI so the user can turn on TLS when defining an account.
Comment 57 Roland Stuehmer 2005-02-03 09:28:28 PST
(In reply to comment #56)
> We might want to add TLS to the new account UI so the user
> can turn on TLS when defining an account.

Good idea, that's bug 80919, fyi.

Comment 58 neil@parkwaycc.co.uk 2005-02-09 07:08:51 PST
Comment on attachment 172054 [details] [diff] [review]
fix incorporating Christian's comments

>+  // socketType is set to default value. Look at isSecure setting
>+  if (NS_FAILED(rv))
>+  {
>+    PRBool isSecure;
>+    rv = GetBoolValue("isSecure", &isSecure);
>+    if (NS_SUCCEEDED(rv) && isSecure)
>+    {
>+       *aSocketType = nsIMsgIncomingServer::useSSL;
>+       SetSocketType(*aSocketType);
>+    }
>+    else
>+    {
>+       getDefaultIntPref("socketType", aSocketType);
>+    }
This doesn't do what you expect, because GetBoolValue never fails.
Comment 59 RL "Bob" Morgan 2005-02-11 21:48:20 PST
(In reply to comment #55)
> (From update of attachment 172757 [details] [diff] [review] [edit])
> please ignore the timeout part, which comes from another patch...
> 
> I'm going to turn off tls by default for now, since there are issues with
> security dialogs popping up because some isps send certs that don't match the
> host name or something like that...

I humbly submit that this default is profoundly mistaken and unfortunately
completely undercuts all the fine work on implementing this feature.  The whole
point of STARTTLS is that client and server can negotiate, on the existing
service port, to obtain the required level of security.  If the client by
default *refuses* to do STARTTLS even when offered by the server then we are
back to forcing the user to get involved in this negotiation by clicking on
mysterious security options.  The design of STARTTLS (with capability
advertising) permits a site like ours that actually wants to protect our users'
passwords (ie, any rational site) to require TLS, and have it work with clients
that can negotiate it, while still permitting these same clients, with the same
default setting, to work just fine on other sites that don't care to use TLS.

Your reasoning about avoiding security popups just doesn't make sense.  If we
have misconfigured TLS on our site so users see popups, then the users *will see
them anyway*, after they have been forced to check the mysterious security
settings.  You haven't protected them from anything, in fact just the reverse. 
The sites you are catering to with this default are those that permit clear
connections, but have TLS turned on and misconfigured.  Do these brain-damaged
sites really deserve to control the default and make life miserable for well-run
sites?

So please please please make "TLS, if available" the default setting.  Please. 
I'd reopen this bug if I could but I can't.
Comment 60 David :Bienvenu 2005-02-12 07:52:39 PST
the problem is that the security dialog hangs the app sometimes. Until that's
fixed, I really don't want to turn this on by default.
Comment 61 RL "Bob" Morgan 2005-03-04 10:08:30 PST
(In reply to comment #60)
> the problem is that the security dialog hangs the app sometimes. Until that's
> fixed, I really don't want to turn this on by default.

OK, is there a bug to track on that?

If this happens often enough to affect this default, it seems like it would be a
showstopper for providing any TLS support (and if it doesn't happen that often,
then it shouldn't affect this default).

Note You need to log in before you can comment on or make changes to this bug.