755988 - Mail Account Setup doesn't detect service on smtps (465)

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, defect, P1)

Description

[Yoshiaki Kasahara]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120515 Firefox/14.0a2
Build ID: 20120515042006

Steps to reproduce:

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120516 Thunderbird/14.0a2

I noticed that "Mail Account Setup" wizard cannot detect service running on port 465 with SSL/TLS.  I found the problem with version 12.0.1 (for Windows and Mac).  I tried Earlybird (14.0a2) for Windows but it was still broken.

I did the following steps:
(Assume an (internal) SMTP server running services on standard port 25/465/587)

* Select Account Settings -> Account Actions -> Add Mail Account
* Enter a name and Email address, and click "Continue"
* Thunderbird found configuration by trying common server names
* Click "Manual config" and change outgoing SSL setting from "STARTTLS" to "SSL/TLS"
  (The port number was automatically changed from 587 to 465)
* Click "Re-test"


Actual results:

Thunderbird reported "Thunderbird failed to find the settings for your email account".

If I reverted outgoing SSL setting to "STARTTLS" and clicked "Re-test", Thunderbird reported "The following settings were found by probing the given server".

I'm sure that the server is configured correctly, because I could manually edit "Outgoing Server (SMTP)" in Account Settings to use "SSL/TLS" for "Connection security" and send messages without problems.


Expected results:

Thunderbird should be able to find service on port 465 with SSL/TLS by probing the given server. Actually, version 9.0.1 worked as expected, so it is a regression.

I captured packets between the client and server using wireshark, and noticed that Thunderbird didn't send SSL client hello to port 465 after 3-way handshake, and just closed the connection.  imaps (993) and pop3s (995) are detected properly. I guess something is wrong with the service detection logic.

Comment 1

[Ben Bucksch (:BenB)]

We definitely do try to find servers on port 465. (This doesn't exclude the possibility of a bug.)

Does the server send the same certificate on both ports? We may ignore any servers with wrong certs, and servers can have different configurations on the 2 ports, so that is a possibility.

Comment 2

[Ben Bucksch (:BenB)]

CONFIRMED. I tried 3-4 ISPs that, according to our database, have SMTP servers on port 465, but when I changed pref mailnews.auto_config_url and ran guess config, it never found port 465, only STARTLS on port 587 or 25. Even manually setting the server to port 465 and SSL and clicking "Re-Test" failed to find the server.

bsmith, could this be a regression in SSL?

Comment 3

[Ben Bucksch (:BenB)]

This is definitely a regression. I tried a TB trunk from 2012-03-31, and it worked. The TB trunk from 2012-04-25 failed. So, it's a regression in that timeframe.

Bug 745950 falls in that time-frame, and changes code relevant to this, so could well be the culprit.

Reproduction:
1. Edit | Preferences... | Advanced | General | Config Editor ...
   Set pref "mailnews.auto_config_url" to "http://domain.invalid/" to force guess config
1. File | New | Existing Mail Account...
2. Realname: nonsense, Email address: [e-mail] , password: nonsense
   (Be sure to use @laposte.net as domain, because they have an SSL SMTP server at port 465,
   but no STARTTLS at SMTP.)
3. Continue and wait
4. Manual config

Expected result:
SMTP: smtp.laposte.net, port 465, SSL/TLS

Actual result:
SMTP: smtp.laposte.net, port 465, None (No encryption

Comment 4

[Ben Bucksch (:BenB)]

Sorry, I meant bug 721369

Comment 5

[Ben Bucksch (:BenB)]

> SMTP: smtp.laposte.net, port 465, None (No encryption)

And here I meant "..., port 587, None", of course. Sorry.

Comment 7

[Ben Bucksch (:BenB)]

As said, this is likely caused by bug 721369.
This bug is causing quite some grief. If somebody could investigate and fix this, this would be appreciated.

Comment 8

[:Irving Reid (No longer working on Firefox)]

Attachment: Handle both number and string values for encryption type code

It won't be easy to stop swearing at JavaScript for long enough to write this up...

The problem is a combination of two things. First, https://hg.mozilla.org/comm-central/rev/69177e1f55f4 commented out the probe for SSL support when we were dealing with a socket where the encryption type is UNKNOWN (-1).

Second, the switch statement at https://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/accountcreation/guessConfig.js#867 uses (under the hood) JavaScript's === comparator, which does NOT coerce strings to numbers. That means if you pass the number 2 in, you get back SSL, but if you pass in the string "2" you get back UNKNOWN

Third, when we read from our ISP database we get the number for the encryption type, and we initialize the manual settings UI with those numbers, and as long as the user doesn't change anything they stay numbers. BUT, if the user make a selection on the encryption type UI element, the value gets changed to a string.

So, if the user explicitly selects an encryption type of SSL in the manual settings UI, it gets converted to UNKNOWN by the JS switch() statement, and the UNKNOWN encryption type no longer probes for SSL support, so our config test fails.


Patch changes the switch() to a series of if statements so that string/integer coercion works and we correctly try SSL when the user explicitly chooses that in the UI.

Comment 9

[Ben Bucksch (:BenB)]

Comment on attachment 653920 [details] [diff] [review]
Handle both number and string values for encryption type code

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

Thanks, irving, for looking into this!

> https://hg.mozilla.org/comm-central/rev/69177e1f55f4 commented out the probe
> for SSL support when we were dealing with a socket where the encryption type is
> UNKNOWN (-1).

That looks wrong to me, even though I wrote the patch.
Yet, I am not in the habit of throwing in useless changes. I wonder why I did that. Could you check in the comments of bug 721369 why we did that?

Speaking of unneeded changes: Please make the patch minimal, removing all changes that are not strictly necessary to fix this bug. If you feel that other changes are useful, please attach them separately, one per purose (e.g. one only logging statements, but I don't think we need all these).

> BUT, if the user make a selection on the encryption type UI element,
> the value gets changed to a string.

Yeah, that's a bug. Please fix this part as well, though: Both 1) the ConvertSocketTypeToSSL() (switch vs. if|, I agree in preferring |if|), as well as 2) what the UI returns: We should never have "2" in that enum variable in the first place, that's already the bug, not just ConvertSocketTypeToSSL().

Thanks a lot again for fixing this! I can see how horrible that was to debug. It is a hideous bug.

Comment 10

[David Favor]

The real problem is having any of this Wizard code.

Every mail setup can be customized in many ways, making Wizard design complex
and as we've seen... many times broken so badly Thunderbird has to be rolled back
to 2.0.0.24 just to configure new accounts.

Please add a Manual Config button to the first panel of the Wizard which allows bypassing
the Wizard and going directly to the "Settings" for a given account ala the 2.0.0.24 way of
doing setups.

Comment 11

[:Irving Reid (No longer working on Firefox)]

Attachment: Additional logging to help diagnose the issue

Comment 12

[:Irving Reid (No longer working on Firefox)]

Attachment: Fix JavaScript strict errors in affected files

Always like to leave things cleaner than I found them...

Comment 13

[:Irving Reid (No longer working on Firefox)]

Attachment: Handle string/integer differences in the manual configuration UI

The code that copied config out of UI elements converted to integer for the incoming parameters but not for the outgoing. This patch adds parseInt to the outgoing parameters

Comment 14

[:Irving Reid (No longer working on Firefox)]

(In reply to Ben Bucksch (:BenB) from comment #9)
> Comment on attachment 653920 [details] [diff] [review]
> Handle both number and string values for encryption type code
> 
> Review of attachment 653920 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks, irving, for looking into this!
> 
> > https://hg.mozilla.org/comm-central/rev/69177e1f55f4 commented out the probe
> > for SSL support when we were dealing with a socket where the encryption type is
> > UNKNOWN (-1).
> 
> That looks wrong to me, even though I wrote the patch.
> Yet, I am not in the habit of throwing in useless changes. I wonder why I
> did that. Could you check in the comments of bug 721369 why we did that?

The code checked at https://bugzilla.mozilla.org/show_bug.cgi?id=721369#c53 is not the patch attached to the bug; you'll need to look at it yourself to figure out what's up. There are some remarks in https://bugzilla.mozilla.org/show_bug.cgi?id=721369#c51 about trying different combinations of probes, perhaps that is where the commented out lines came from?

Comment 15

[Ben Bucksch (:BenB)]

Comment on attachment 654271 [details] [diff] [review]
Handle string/integer differences in the manual configuration UI

+      config.outgoing.socketType = parseInt(e("outgoing_ssl").value);
+      config.outgoing.auth = parseInt(e("outgoing_authMethod").value);

Please use
... = santize.integer(e("outgoing_ssl").value);
like the other code.

Comment 16

[Ben Bucksch (:BenB)]

Comment on attachment 654253 [details] [diff] [review]
Additional logging to help diagnose the issue

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

I can see that this was useful for you during debugging, but I am not sure we need this in the code that we ship.
Particularly the guessConfig changes: I think the cancel/abort is being called for all other in-flight probes when we found a good host. Ditto when the user cancels the probe, you'll get 50 cancel messages, because we probe 50 hosts at the same time.

Comment 17

[Ben Bucksch (:BenB)]

(In reply to Ben Bucksch (:BenB) from comment #15)
> Comment on attachment 654271 [details] [diff] [review]
...
> Please use
> ... = santize.integer(e("outgoing_ssl").value);
> like the other code.

With this change: r+

Thanks, irving!

Comment 18

[:Irving Reid (No longer working on Firefox)]

Attachment: Fix JS Strict warnings, updated to not depend on debug patch

Carrying forward r=benb

Comment 19

[:Irving Reid (No longer working on Firefox)]

Attachment: Handle string/integer differences in the manual configuration UI, using sanitize.integer()

r=benb from https://bugzilla.mozilla.org/show_bug.cgi?id=755988#c17

Comment 20

[:Irving Reid (No longer working on Firefox)]

Pushed to comm-central:

https://hg.mozilla.org/comm-central/rev/82882c866072
https://hg.mozilla.org/comm-central/rev/115d3d4fa725

Comment 21

[Ben Bucksch (:BenB)]

Thanks a lot, irving, for fixing this! Much appreciated.

Comment 22

[alta88]

(In reply to :Irving Reid (No longer working on Firefox) from comment #8)
> Created attachment 653920 [details] [diff] [review]
> Handle both number and string values for encryption type code
> 
> It won't be easy to stop swearing at JavaScript for long enough to write
> this up...
> 
> The problem is a combination of two things. First,
> https://hg.mozilla.org/comm-central/rev/69177e1f55f4 commented out the probe
> for SSL support when we were dealing with a socket where the encryption type
> is UNKNOWN (-1).
> 

Is there a legitimate reason for SSL to still be commented out in guessContig.js here?
https://dxr.mozilla.org/comm-central/rev/706e5fdd301201b23e96be9826b7e5b953d11776/mail/components/accountcreation/content/guessConfig.js#773

Comment 23

[Ben Bucksch (:BenB)]

I can't remember anymore, sorry.