971347 - autoconfig vulnerable to active MITM attacks for all domains (including the ones in ISPDB)

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, enhancement)

Description

[tagnaq]

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:24.0) Gecko/20100101 Firefox/24.0 (Beta/Release)
Build ID: 20100101

Steps to reproduce:

autoconfig [1] performs multiple steps in the following order:

- tb-install-dir/isp/example.com.xml on the harddisk
- check for autoconfig.example.com (insecure HTTP)
- look up of "example.com" in the ISPDB (HTTPS protected)
- look up "MX example.com" in DNS, and for mx1.mail.hoster.com, look up "hoster.com" in the ISPDB
- try to guess (imap.example.com, smtp.example.com etc.)

Due to the fact that the insecure HTTP request happens *before* the secure ISPDB HTTPS request, an active attacker can deliver his malicious client config to Thunderbird even if there would be an entry in Mozilla's ISPDB.

I assume Thunderbird doesn't query the ISPDB if autoconfig.example.com delivered already an (insecure) xml file.

I would suggest to change the order to:

0) file lookup
1) fetch from ISP ((try)HTTPS)
2) fetch from Mozilla's ISPDB (HTTPS)
3) fetch from ISP (HTTP)
...

This might significantly increases the number of requests that Mozilla's ISPDB has to handle (but these requests only happen during account setup not on every TB startup).


https://developer.mozilla.org/en-US/docs/Mozilla/Thunderbird/Autoconfiguration

Comment 1

[tagnaq]

To avoid downgrade attacks, Thunderbird would have to skip (3) when the TCP connection to Mozilla's ISP fails, otherwise an attacker can simply block (1) and (2) at the TCP level to trick Thunderbird into using insecure HTTP fetches (3).

Comment 2

[tagnaq]

s/connection to Mozilla's ISP fails/connection to Mozilla's ISPDB fails/

Comment 3

[Ben Bucksch (:BenB)]

> Thunderbird would have to skip (3) when the TCP connection to Mozilla's ISP fails

FTR, we can see that by the result code from XHR, and the wrapper FetchHTTP nicely returns that as error code from HTTP error codes, so we can differentiate between "config not found" (404), HTTP server broken (HTTP 5xx) and TCP connection broken (IIRC FetchHTTP error code < 0).

> I would suggest to change the order to:
> 0) file lookup
> 1) fetch from ISP ((try)HTTPS)
> 2) fetch from Mozilla's ISPDB (HTTPS)
> 3) fetch from ISP (HTTP)

In step 1, we'd only try https://autoconfig.<emaildomain>/... , and *not* https://<emaildomain>/.well-know/... ,  while step 3 would continue to try http://autoconfig.<emaildomain>/... and http://<emaildomain>/.well-know/...

That sounds like a good idea.

Just a few things that I'm considering, to answer this:
* It's one more HTTP call. However, if the DNS record autoconfig.<domain> doesn't exist, the HTTP call will fail very quickly, and the expensive part is the DNS lookup, which we'll do in Step 3 anyway, and which we currently already do in step 1. So, if measured, I don't think this slows things down much. It will probably actually speed up overall, because configs that are in ISPDB won't check http://<emaildomain>/.well-know/... anymore.
* We wanted to give ISPs priority, out of the idea of decentralization. The ISPDB was intended only as a short-term stop-gap. However, it turned out to be a huge success for all parties involved - end users, ISPs [1], even other email clients that use our database, but don't implement fetchISP.
* We wanted to value ISP control over Mozilla's control. In principle, Mozilla shouldn't have the right *over* the ISP. This could - in theory - even be an attack from the US government on foreign people. Thus, I wanted to give ISPs a way out and to trump Mozilla's configs. Again, the day-to-day reality is very different: See bug 537649 and countless big webmail providers (too many to list - pretty much all big names) that publicly announce only plaintext configs, but do offer SSL servers, just didn't want all users to use them. In reality, it was me who's forcing more secure configs over the ISP's will. So, the attack scenario can also go the other way: A government forcing an ISP to downgrade one of their users (esp. because we send the email address in the request, which makes targetted attacks trivial), and even creating false CA certs (which are accepted by one of the many TB root CAs) and MITM attacks on their users in their country only.
* Getting a CA-signed certs is a real problem for many ISPs. The bigger hosters couldn't, because they need one cert per customer domain (or all customer domains in one cert), and that's not feasible. The small entities can't, because they often don't have a cert and getting one is too complicated [1].

[1] Most ISPs are happy for TB to be set up easily for their customers, but will not cooperate with us to make autoconfig work, neither by setting up an autoconfig XML on their web server, nor by changing mail server configs (e.g. to accept email address as username). There's a gap between customer support and server administration, and between mail server admin and web server admin. In reality, I'm having big trouble getting people to use the fetch ISP mechanism at all.

* It will complicate the code a little bit, because it has fetchHTTP in 2 different places of the order.

All things considered, I think this is a reasonable move from the current situation.
* It will not notably slow down and will probably speed up
* It doesn't change the theory, because the ISP can still override
* It makes things slightly more SSLy than now
* It keeps the case "ISP has config, ISPDB does not" and "ISP cannot offer config over SSL" working
* Downside: slighly less pretty code

Comment 4

[Ben Bucksch (:BenB)]

Thinking about this more, I think the best solution is to parallelize the requests. I wanted to do that anyway.

Fetches:
* https://autoconfig.<emaildomain>/...config-v1.1.xml?emailaddress=<username>@<emaildomain> .
* Mozilla ISPDB via https
* http://autoconfig.<emaildomain>/...config-v1.1.xml (no email address!)
* http://<emaildomain>/.well-know/mail/config-v1.1.xml
* MX lookup via Mozilla MX service via HTTPS
  * When returning: All steps above for MX domain.

Rules:
1. If ISP HTTPS call has a result, use that
2. If Mozilla ISPDB call has a result, use that
3. If Mozilla ISPDB call comes back with a connection problem,
   will ignore the result of the HTTP calls.
4. If MX call has a result, and ISP HTTPS call has a result for MX domain, use that
5. If MX call has a result, and Mozilla ISPDB call has a result for MX domain, use that
6. If ISP HTTP non-SSL autoconfig call has a result, use that
7. If ISP HTTP non-SSL webserver call has a result, use that
8. If MX call has a result, and ISP HTTP autoconfig call has a result for MX domain, use that
9. If MX call has a result, and ISP HTTP webserver call has a result for MX domain, use that


How about this? tagnaq?

Comment 5

[Ben Bucksch (:BenB)]

* Drop rules 8 and 9. We currently don't do that either.
* Rule 5 we don't currently do, but it's something hosters have been asking for, because it's difficult for them to set DNS A records for all their customers. They should be able to get a cert for autoconfig.hosterdomain.com, though.
* Rules 6 and 7 (ISP fetch without SSL) are still important for small domains. We don't want them in ISPDB, and they can't afford (and maintain) a cert.
* Add rule 0: local file, of course.

Comment 6

[James Renken]

(In reply to Ben Bucksch (:BenB) from comment #3)
> > Thunderbird would have to skip (3) when the TCP connection to Mozilla's ISP fails
> 
> FTR, we can see that by the result code from XHR, and the wrapper FetchHTTP
> nicely returns that as error code from HTTP error codes, so we can
> differentiate between "config not found" (404), HTTP server broken (HTTP
> 5xx) and TCP connection broken (IIRC FetchHTTP error code < 0).

I fear this would do little to mitigate an active MitM attack where the attacker controls the network. S/he could easily hijack, not merely interrupt, the TCP session.

With low-skill attackers hijacking unsecured WiFi networks more and more often, I have a hard time recommending Thunderbird when it is vulnerable to trivially easy attacks by default.

> * Getting a CA-signed certs is a real problem for many ISPs. The bigger
> hosters couldn't, because they need one cert per customer domain (or all
> customer domains in one cert), and that's not feasible. The small entities
> can't, because they often don't have a cert and getting one is too
> complicated [1].

Would it be feasible for Thunderbird to connect to autoconfig.<hosterdomain> (and expect that as the SSL/TLS certificate's CN) where autoconfig.<emaildomain> is a CNAME to autoconfig.<hosterdomain> rather than an A? (OTOH, there seems to be a consensus that this is undesirable behavior - e.g. bug 139467.)

Comment 7

[Ben Bucksch (:BenB)]

> I fear this would do little to mitigate an active MitM attack where the
> attacker controls the network. S/he could easily hijack, not merely interrupt,
> the TCP session.

Not with HTTPS. The ISPDB is https, and this bug is talking about ISPDB.
My proposal would at least secure the ISPDB further.

> autoconfig.<emaildomain> is a CNAME to autoconfig.<hosterdomain> rather than an A?

No, hoster cannot change DNS for all customer domains, even if he hosts the DNS. At least that's what hosters told me.

> I have a hard time recommending Thunderbird when it is vulnerable to trivially easy attacks by default.

Please keep things in perspective. There are mitigating factors, which are very much intentional and by design:
* This is a one-time setup action. You're unlikely to set up your accounts for the first time while on the road.
* The config is shown to the end user and he has to approve the config.
* If the IMAP config is not SSL or the SSL cert is self-signed, we warn very boldly. Thus, an attacker needs both a real domain and a real SSL cert.
* The alternative is typically to search on Google and the ISP's website for the right config, which is no less vulnerable.

Comment 8

[Ben Bucksch (:BenB)]

Nevermind the last part of my comment. It doesn't belong in this technical bug.

Comment 9

[Alice Wonder]

The issue for big hosters could be solved by using a DNS SRV record to point to the location of the autoconfigure appropriate for the domain.

The big hoster then would only need a single certificate for that domain, and to create the SRV record in the client's zone file, but they currently have to that anyway with an A/AAAA record.

Using an SRV record would also allow it to be more robust, as alternate configuration servers could be specified in case one of them was down.

-=-

At the very least, Thunderbird needs to advise the user that the auto-configuration information was retrieved from an insecure source, and allow the user to inspect the results before submitting authentication information.

Comment 10

[Alice Wonder]

(In reply to tagnaq from comment #0)

> 
> I would suggest to change the order to:
> 
> 0) file lookup
> 1) fetch from ISP ((try)HTTPS)
> 2) fetch from Mozilla's ISPDB (HTTPS)
> 3) fetch from ISP (HTTP)
> ...
> 

I would suggest

0) fetch from (HTTPS) autoconfig.domain.tld
1) fetch from (HTTPS) Mozilla's ISPDB
2) fetch from (HTTPS) url pointed to in SRV record, if exists
3) fetch from local file, if exists

Local file should be last. That prevents stale or trojan config data being used if the current verifiable (HTTPS x509) data can be retrieved.

Comment 11

[ISHIKAWA, Chiaki]

Here is a tangentical comment:
I have observed over the last few years that
during |make mozmill| test suite run of comm-central TB, 
the autoconfiguration always fail (test failures) on one PC, while on another PC, it seems to succeed sometimes.
I am curious what the differences of environment (64-bit linux fails all the time while 32-bit linux seems to work sometimes).
Anyway, maybe someone digging into the issue of bugzilla entry, and THEN seeing if the current test suite written in mozmill checks the behavior of TB correctly regarding autocofiguration including the fallback would be able to deduce if there is an
inherent limitation of the current test about the autoconfiguration OUTSIDE mozilla compilation farm, etc.

TIA

Comment 12

[Alice Wonder]

I'm not sure what your issue is but Thunderbird auto-configuration works for on on 64-bit Thunderbird as distributed by CentOS for Linux and on 64-bit for Windows using the Mozilla provided binaries.

But since you are referencing the test suite, I wonder if you are talking about something different than client auto-configuration when creating a new e-mail address?

Comment 13

[ISHIKAWA, Chiaki]

(In reply to Alice Wonder from comment #12)
> I'm not sure what your issue is but Thunderbird auto-configuration works for
> on on 64-bit Thunderbird as distributed by CentOS for Linux and on 64-bit
> for Windows using the Mozilla provided binaries.
> 
> But since you are referencing the test suite, I wonder if you are talking
> about something different than client auto-configuration when creating a new
> e-mail address?

There are test programs within TB's test suite that is invoked |make mozmill|, which exercises the actions you refer to and make sure TB is supposed to operate as expected.

On one of my PC where 64-bit linux instance runs, the test almost always fail for the autoconfiguration and I am not sure what is wrong: but there must be a very few minority group of would be users who also experience possibly the real world failure of autoconfiguration (due to mysterious network time out errors).
On the other hand, on another PC where linux32 bit instance runs, the same test works fine. 

This seeming difference on two platforms has baffled me for a few years now.

I don't think the test falls back to the local file/cache thingy as far as I can tell.

TIA

Comment 14

[u]

Has there been any progress on implementing this change into recent versions of Thunderbird by any chance?

Comment 15

[u]

Attachment: 0001-Optionally-skip-guessing-plaintext-protocols.patch

[Patch 1/7]
Optionally skip guessing plaintext protocols.

Setting mailnews.auto_config_ssl_only to True prevents all testing of
plaintext protocols when guessing during autoconfiguration.

Comment 16

[u]

Attachment: 0002-Optionally-skip-insecure-database-autoconfiguration-.patch

[patch 2/7] Optionally skip insecure database autoconfiguration lookup.

Setting mailnews.auto_config_ssl_only to True makes autoconfiguration
skip database lookups if mailnews.auto_config_url isn't HTTPS.

Comment 17

[u]

Attachment: 0003-Optionally-skip-insecure-DNS-MX-autoconfiguration-lo.patch

[patch 3/7] Optionally skip insecure DNS MX autoconfiguration lookup.
 
Setting mailnews.auto_config_ssl_only to True makes autoconfiguration
skip DNS MX lookup during autoconfiguration.

Comment 18

[u]

Attachment: 0004-Make-ISP-autoconfiguration-lookup-first-try-https-th.patch

[patch 4/7] Make ISP autoconfiguration lookup first try https, then
http if we allow insecure protocols.

Setting mailnews.auto_config_ssl_only to True makes autoconfiguration
use only https for ISP lookup.

Comment 19

[u]

Attachment: 0005-Add-checkbox-for-toggling-mailnews.auto_config_ssl_o.patch

[patch 5/7] Add checkbox for toggling mailnews.auto_config_ssl_only.

Comment 20

[u]

Attachment: 0006-Optionally-skip-fetched-configs-using-plaintext-prot.patch

[patch 6/7] Optionally skip fetched configs using plaintext protocols.

Setting mailnews.auto_config_ssl_only to True discards any
configuration using plaintext protocols read from XML configuration
files during autoconfiguration.

Comment 21

[u]

Attachment: 0007-Make-FetchHTTP-silently-fail-on-SSL-cert-errors.patch

[patch 7/7] Make FetchHTTP silently fail on SSL cert errors.

XMLHttpRequest just throws an error for self-signed certificates
without the possibility to add an exception. For more, see this bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=507645

Comment 22

[u]

Hi,

Years ago, long before this ticket was created, Tails developers wrote a
set of patches for Thunderbird which offer users the choice to accept only
secure protocols when using the autoconfiguration wizard.
This is why these patches do not follow the design devised on this
ticket. (The patches do not modify the behaviour of manual configuration.)

As noted on https://trac.torproject.org/projects/tor/ticket/10836, users
who use Torbirdy to send Email through the Tor Network, can not use the
autoconfiguration dialog because it has been disabled for the reasons
outlined in https://bugzilla.mozilla.org/show_bug.cgi?id=971347
(Autoconfig vulnerable to active MITM attacks for all domains (including
the ones in ISPDB)): "disabling the autoconfig dialog does users a
dis-service not only in convenience and usability (in the literal sense
of the word), but more importantly in security, because we know about
SSL configs that users might not know or find."

However, we think that security is not only a necessary option for
Torbirdy users, but for all Thunderbird users.
Our patches introduce a configuration variable
(mailnews.auto_config.ssl_only). By default it is set to true, and
allows using only secure protocols for the domain and ISPDB lookups as
well as for the actual mail account configuration. In detail this means:
1. to prevent all testing of plaintext protocols when guessing during
autoconfiguration.
2. to make autoconfiguration skip database lookups if
mailnews.auto_config_url isn't HTTPS.
3. to make ISP autoconfiguration lookups first try https, then http if
we allow insecure protocols.
4. to discard any configuration using plaintext protocols read from XML
configuration files during autoconfiguration.

In addition, even when plaintext protocols are allowed, the patches make
us prefer secure options if available. Without the patches, the first
one read from the config is used.

User choice is represented by a checkbox which is visible before the
wizard actually talks to the network and can be deactivated as needed,
for example if one needs to setup a .onion or a .i2p address, which
might not have SSL certificates, and where the transport layer ensures
end-to-end authentication and encryption or when using an email service
from the 90s that does not support SSL/TLS (or when SSL/TLS does not
work for other reasons, e.g. when visiting $SSL_BLOCKING_COUNTRY or
whatever).

Another hidden preference in our patches
(mailnews.auto_config.dns_mx_lookup.enabled) makes autoconfiguration
skip DNS MX lookup if set to false. Checking or unchecking the GUI
checkbox won't affect this. It is true by default, but TorBirdy should
of course set it to false.

A further possible improvement that we're willing to implement, if
needed would be that whenever the box is unchecked, a warning could be
shown, to enable users to make the right choice and ensure that they
question if they are being targeted through a "downgrade-to-non-TLS"
attack.

We think that there's no good reason not to provide TLS anymore today,
especially with the birth of LetsEncrypt and we'd like to submit these
patches for inclusion. We think that using secure protocols should be
opt-out, and this we think that setting ssl_only to true by default is
the correct way to go. This would make everybody safe, but still allow
people who use older/different methods to connect to their mail servers
to do that, if they wish so.

https://trac.torproject.org/projects/tor/ticket/10836#comment:17
suggests that one can now disable the non-TLS fetch from ISP, but as
this is a hidden preference, which simply avoids sending email addresses
in cleartext, we think that our patches don't actually overlap with this
modification.

Here are some basic user stories for these patches:

  As a user
  When configuring my existing email account in Thunderbird
  using the account creation wizard
  I want Thunderbird to only use secure (SSL/TLS) protocols by default
  when guessing or retrieving the mail server configuration
  In order to prevent an attacker from MitM:ing me
  and serving me a malicious mail server configuration.

  As a user
  When configuring my existing email account in Thunderbird
  using the account creation wizard
  I want Thunderbird to only accept secure (SSL/TLS) configurations by
default
  when guessing or retrieving mail server configurations
  In order to avoid transmitting account credentials in cleartext.

We've rebased and tested these patches on Icedove 38.5.0, see here:
https://git-tails.immerda.ch/icedove/log/?h=secure_account_creation-38.5.0-1
(and in the attachments.)

We've also successfully built Debian packages with these patches and successfully
tested them in Debian Sid.

For the TorBirdy use case, we also posted a patch adding SOCKS support to
the autoconfiguration guessing, solving Mozilla bug #669238.

Hence, with all the patches, TorBirdy should be able to safely use the
full autoconfiguration suite, except DNS MX lookup.

Cheers,
u. from the Tails team.

Comment 23

[Magnus Melin [:mkmelin]]

Thx for submitting these patches. You're not supposed to review+ patches like that though
See http://developer.mozilla.org/en/docs/Getting_your_patch_in_the_tree#Getting_the_patch_reviewed

About the flow: instead of a pref that very few users have any idea of, maybe better to just try the secure options first, then if no success ask the user if trying insecurely is ok.

Comment 24

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Magnus Melin from comment #23)
> About the flow: instead of a pref that very few users have any idea of,
> maybe better to just try the secure options first, then if no success ask
> the user if trying insecurely is ok.

Just want to do a drive-by "from the perspective of the Firefox OS email app" info dump:

These days, a mail server that lacks valid certificates is either:
A) A really sucky mail provider.  These unfortunately do still exist, but have increasingly less ability to rationalize their suckiness now that Let's Encrypt exists.
B) A user under attack.

If the user is under attack, as mentioned earlier in this thread, the attacker can easily force the secure attempts to fail, forcing the dialog to come up.  The problem with all dialogs of this ilk is that no matter how scary the dialog is, from the user's perspective it frequently boils down to "Get your email? [Yes] [No]."

For the Firefox OS gaia mail app we opted to do the following:
- The autoconfig flow only uses https lookups and only accepts connections using TLS.  This includes ignoring all ISPDB entries that say to not use TLS.
- (It's also impossible to add certificate exceptions, but there are complicating system-level factors.)
- The only way to create an account without TLS enabled is to use the manual account creation flow.  If you select such an option, you get a brief dialog that scare-warns you and suggests you can get a better email provider with more details available at https://support.mozilla.org/en-US/kb/switch-secure-email-provider-firefox-os.  Our spiel text is:
  - Are you sure?
  - Other people may be able to see your passwords and emails or control your account.
  - There are multiple, free email providers that offer more secure, encrypted connections.
  - [Learn more] [I understand]

The motivating rationale is basically:
- Make it easy for the user to do the safe/secure thing.
- Make it a hassle for the user to do the dangerous thing.  While frustrating the user is never an explicit goal, there are a few upsides:
  - It makes it more difficult for word-of-mouth bad-advice to shoot the user in the foot.  For example, Thunderbird's certificate warning dialogs are simple enough to be annoying but easy enough to bypass that a user can be instructed to just click through, permanently accepting an attacker's certificate.
  - Potentially result in support calls to the mail provider to help incentivize them to exert the minimal effort required to take an important security step for themselves and their users.
  - Potentially help encourage the user to switch to a better mail provider.
- At least try and inform the user.

Comment 25

[Magnus Melin [:mkmelin]]

For desktop though, I'd expect there to be a lot of cases where ISPs don't bother to set up any mail security (since the mail service you get is just a side product), and a lot of companies not setting up security because the only way you can access the company mail servers is from intranet through VPN.

Comment 26

[Andrew Sutherland [:asuth] (he/him)]

Indeed, as I said, it does happen.  But is it worth optimizing for the insecure case at the cost of the secure case?  It's also worth noting that, at least as long as it's only me doing the reviews, the Thunderbird ISPDB no longer accepts new entries with insecure settings.  So in the cases you cite, it's possible manual configuration would end up being needed in those cases anyways.  (The exception is Thunderbird's autoconfig process does do subdomain guessing that may succeed.)

Comment 27

[Magnus Melin [:mkmelin]]

Those cases would likely be through heuristics I suppose. I'm all for *optimizing* for secure, but in the end it's not that likely you're really MITM-attacked the one time you set up your account to your small server, so we should keep the insecure checks too.

Comment 28

[Andrew Sutherland [:asuth] (he/him)]

The problem is that optimizing for secure requires not doing the insecure checks.  The only parties served by the insecure checks are attackers and users with horrible mail servers.  We want 0% of users to be attacked and we want 0% of users to have horrible mail servers.  (And if your security for not succumbing to a MITM-attack requires not being MITM-attacked, then you don't actually have any security against MITM-attacks.)

In effect you're advocating for exposing the (made up number) 99% of users with secure mail servers to attack risk because of the (again, made up) 1% of users with insecure mail servers.  This is not a good compromise, even if both numbers were 50%.

There are engineering mitigations that can implemented to eliminate the ambiguity such as shipping the entire ISPDB with Thunderbird, but they are non-trivial and have their own complications.  And as we made the call for Firefox OS mail, it is arguably not time well-spent to focus engineering effort on optimizing for the insecure case which no user should be experiencing.  That effort is better spent helping the user by informing them that they can switch to (for example) Gmail and have gmail pull their email into their server so they can keep their old mail address and slowly migrate away.

Note that the reason I'm jumping in on this so strongly is that it looks like "u" is a potential new contributor[1] making a concerted effort to help improve Thunderbird's woeful autoconfig security, an area where I believe there are no current active contributors AFAICT.  And beyond my obvious disagreement with your stance, I'm worried that "u" may make the practical decision that it's easier to just maintain the patches downstream than jump through hoops to not actually improve Thunderbird security in the default case.  Or slightly less bad, that the patches will be taken but again there's no meaningful improvement in Thunderbird's default configuration.

I'm going to bow out of the thread now because I think I've made as cogent an argument as I'm going to make and since I'm not actually volunteering to help improve the Thunderbird codebase (I only hack on the pure-JS gaia mail backend these days), I may have already used up too much air time as-is.

1: I no longer watch the commit logs, "u" may be an existing contributor.

Comment 29

[u]

Hi,

thanks for your comments and thank you Magnus for fixing the review bits!

Comment 23, Magnus Melin
> About the flow: instead of a pref that very few users have any idea of, maybe better to just try the secure options first, then if no success ask the user if trying insecurely is ok.

We agree with Andrew Sutherland that users will see this prompt as "Get your email? [Yes] [No]". And everything else he says. We do not like the idea of offering this option at all, and we would not mind seeing the checkbox go, forcing users to do an advanced/manual configuration to use the insecure protocols.

The reason we added the checkbox was mainly (we'll discuss the other point later) because we were sure this point was going to be raised and some mechanism to allow auto configuring insecurely would have to be kept. We rejected the idea of a prompt, since we know users quickly train themselves to deal with them in a manner that allows them to proceed without contemplating the consequences. While a checkbox is fundamentally the same, it offers this option in a less "in your face" manner, and hopefully encourages a more thoughtful process for disabling it. Really, we need a UI/UX psychology expert here! :)

If any prompt is to be added it should, in our humble opinion, be a warning/confirmation that is shown when the checkbox is unticked.

Comment 27, Magnus Melin
> [...] in the end it's not that likely you're really MITM-attacked the one time you set up your account to your small server

From a security point of view, this is unfortunately a poor assumption:

* For immobile (e.g. desktop) users, if you are in a position to *ever* be MitM:ed, then you very likely are for initial setup too.

* As an attacker, the incentive for doing a "protocol downgrade" attack during the account setup is a lot larger than MitM:ing subsequent secure logins; while it is an active attack to block the secure tries, it is not detectable by the user, and after downgrading the users configuration to non-SSL/TLS, all subsequent logins can be passively eavesdropped. Compare this to an active MitM when SSL/TLS is enabled, which will trigger certificate warnings upon login etc.

* In any case, the threat model for using Tor with Thunderbird it must be assumed that there are MitM:ing attackers. One famous example is http://archive.wired.com/politics/security/news/2007/09/embassy_hacks?currentPage=1

For what it is worth, in the context of Tails, which is an amnesic operating system where all filesystem changes are dropped after shutting down, the account *will* be setup anew every boot. The reason we picked Thunderbird is because of the automatic configuration, which makes this setup just as easy as using a webmail interface, i.e. only enter username and password and you're done.

Comment 28, Andrew Sutherland 
> The only parties served by the insecure checks are attackers and users with horrible mail servers.

There is another group that is ignored: for Tor hidden services (.onion addresses) it is pretty common to provide services with the option of application-level encryption and authentication since hidden services offer that via the transport-layer. In this case using insecure automatic configuration is actually secure (and generally the only option), and we would like to support this.

In closing: at the moment all Thunderbird users employing the autoconfiguration are vulnerable to MitM. These patches fix this, while still offering a possibility to obtain the old behaviour.

We feel it is important to fix this security issue for all Thunderbird users as soon as possible and would like to not delay this improvement any further. That's why we are open to finding a compromise which would allow incorporating these patches. (Although we do not agree that it is a good option to have a pop-up, because of the aforementioned reasons, we'd implement that if you think that that's our only option.) However, we are quite fond of Andrew's proposal, which would keep Thunderbird "in sync" with Firefox OS: that is, to propose only secure options through the autoconfiguration and leave the insecure possibilities to those users who know what they're doing and who have opted for manual configuration.

Thus, our suggestion would be to kindly let us know what the preferred way to go would be, so that we can propose a fix which can be reviewed.

Thanks for your work! 

Cheers,
u.

PS: "u" is a real person, but I am acting here on behalf of the Tails core developers and these comments are actually the result of a collective effort.

Comment 30

[Andrew Sutherland [:asuth] (he/him)]

(In reply to u from comment #29)
> Comment 28, Andrew Sutherland 
> There is another group that is ignored: for Tor hidden services (.onion
> addresses) it is pretty common to provide services with the option of
> application-level encryption and authentication since hidden services offer
> that via the transport-layer. In this case using insecure automatic
> configuration is actually secure (and generally the only option), and we
> would like to support this.

I think it would make sense/be preferable if for autoconfig/ISPDB we did something like introduced an explicit <socketType>TOR</socketType> instead of overloading <socketType>plain</socketType>.  This would allow clients to filter appropriately, favoring TOR if they are TOR-enabled or ignoring it if they are not.  And without requiring the unaware clients to need to understand that ".onion" is magical as far as they are concerned.  This ideally avoids a whole class of confusion bugs in the configuration logic too.

This would arguably enable the ISPDB and self-hosters to advertise that they are hidden-service capable, etc.  I'm not sure if this helps the trust graph or not.

For context, the initial options and related information is available at https://developer.mozilla.org/en-US/docs/Mozilla/Thunderbird/Autoconfiguration/FileFormat/HowTo#Configuration_file_format

Comment 31

[u]

(In reply to Andrew Sutherland [:asuth] from comment #30)
> (In reply to u from comment #29)
> > Comment 28, Andrew Sutherland 
> > There is another group that is ignored: for Tor hidden services (.onion
> > addresses) it is pretty common to provide services with the option of
> > application-level encryption and authentication since hidden services offer
> > that via the transport-layer. In this case using insecure automatic
> > configuration is actually secure (and generally the only option), and we
> > would like to support this.
> 
> I think it would make sense/be preferable if for autoconfig/ISPDB we did
> something like introduced an explicit <socketType>TOR</socketType> instead
> of overloading <socketType>plain</socketType>.  This would allow clients to
> filter appropriately, favoring TOR if they are TOR-enabled or ignoring it if
> they are not.  And without requiring the unaware clients to need to
> understand that ".onion" is magical as far as they are concerned.  This
> ideally avoids a whole class of confusion bugs in the configuration logic
> too.

This seems to be a good additional feature which would make it easier for the user.
But it doesn't solve the issue in the general case, since we shouldn't assume 
that ISPs serve their own configuration or have submitted one to Mozilla's database;
in this case we'd have to use the guessing mechanism, which would be unaffected so
it would still rely on an option similar to the one we propose.

Furthermore, implementing something like what you propose requires quite a bit further
work, and possibly to coordinate with whoever maintains the configuration format, since
we would extend it. It would take a lot of additional time to do so, and we think that
we really need to  fix this sooner rather than later.

Comment 32

[Andrew Sutherland [:asuth] (he/him)]

(In reply to u from comment #31)
> Furthermore, implementing something like what you propose requires quite a
> bit further
> work, and possibly to coordinate with whoever maintains the configuration
> format, since
> we would extend it. It would take a lot of additional time to do so, and we
> think that
> we really need to  fix this sooner rather than later.

Clarification: I was speaking as the only current active reviewer for the Mozilla-hosted ISPDB and as the author of a mail client that does not use domain guessing and probing.  I am no longer a Thunderbird reviewer and other than the general opinions about autoconfig/the ISPDB, you should probably ignore my feedback :)

Having said that, I expect if you want these patches to move forward you need to request review on the patches from the relevant owners/peers listed at https://wiki.mozilla.org/Modules/Thunderbird and https://wiki.mozilla.org/Modules/MailNews_Core.  See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch for more details.

Comment 33

[u]

Hi,

tentatively assigning these patches for review now.

I'd have preferred that we'd agree upon a common strategy first, in order to provide secure lookup for all Thunderbird users, the bug being already 2 years old.. :)

Cheers!

Comment 34

[Magnus Melin [:mkmelin]]

Sorry for the delay reviewing this, there's a lot to think about.

Just some thoughts so far:
 - The checkbox is to require secure or not is no longer visible once the checks fail. I think it should to be de-checkable there
 - It might be preferable if you only checked is we should require secure in FetchHTTP, HostDetector and balied out for non secure protocols

Comment 35

[u]

(In reply to Magnus Melin from comment #34)

> Sorry for the delay reviewing this, there's a lot to think about.  Just some thoughts so far:

Thank you very much for your review! We are happy to see this moving forward.

> - The checkbox is to require secure or not is no longer visible once the checks fail. I think it should to be de-checkable there

This was not intentional. A fixup patch will be submitted soon.

> - It might be preferable if you only checked is we should require secure in FetchHTTP, HostDetector and balied out for non secure protocols

We are not sure we understand this part. Can you please clarify what you mean here?

Do you mean that we should only use the option for "FetchHTTP, HostDetector"? And, in order to understand better what you're aiming at, could you please also explain the rationale behind this? Securing *all* methods still seems important.

Comment 36

[u]

(In reply to u from comment #35)
> (In reply to Magnus Melin from comment #34)

> > - The checkbox is to require secure or not is no longer visible once the checks fail. I think it should to be de-checkable there
> 
> This was not intentional. A fixup patch will be submitted soon.

After redigging a bit, that was actually intentional. Sorry for the confusion. Let me explain:

Once the automatic checks fail, Thunderbird switches to manual mode. And in manual mode, we don't guess the configuration and do not retrieve Mozilla DB again. So here we do not need to force SSL/TLS anymore.
As for proposing to the user to use insecure protocols at that point in time, we think we still need to allow them to do so.

An option for better UX would eventually be to keep the checkbox on this screen but to grey it out.
What do you think?

Cheers!

Comment 37

[Magnus Melin [:mkmelin]]

(In reply to u from comment #35)

> > - It might be preferable if you only checked is we should require secure in FetchHTTP, HostDetector and balied out for non secure protocols
> We are not sure we understand this part. Can you please clarify what you
> mean here?

ATM you're disabling by handling it in the callers, I was wondering what you think about disabling it in the code that performs the fetchs instead (which should not be that hard since they are designed to potentially fail, you'd fake a fail if disabled).

Rationale: less logic change and more future proof as it would not then be as easy to allow an unsafe protocol to be tried

Comment 38

[Magnus Melin [:mkmelin]]

(In reply to u from comment #36)
> (In reply to u from comment #35)
> > (In reply to Magnus Melin from comment #34)
> 
> > > - The checkbox is to require secure or not is no longer visible once the checks fail. I think it should to be de-checkable there
> > 
> > This was not intentional. A fixup patch will be submitted soon.
> 
> After redigging a bit, that was actually intentional. Sorry for the
> confusion. Let me explain:
> 
> Once the automatic checks fail, Thunderbird switches to manual mode. And in
> manual mode, we don't guess the configuration and do not retrieve Mozilla DB
> again. So here we do not need to force SSL/TLS anymore.

Well you get to type in server name and choose Autodetect security settings so those are guessed again.

Comment 39

[u]

(In reply to Magnus Melin from comment #37)
> (In reply to u from comment #35)
> 
> > > - It might be preferable if you only checked is we should require secure in FetchHTTP, HostDetector and balied out for non secure protocols
> > We are not sure we understand this part. Can you please clarify what you
> > mean here?
> 
> ATM you're disabling by handling it in the callers, I was wondering what you
> think about disabling it in the code that performs the fetchs instead (which
> should not be that hard since they are designed to potentially fail, you'd
> fake a fail if disabled).
> 
> Rationale: less logic change and more future proof as it would not then be
> as easy to allow an unsafe protocol to be tried

Good points, we'll fix this! Thanks for explaining this a bit further.

Comment 40

[u]

(In reply to Magnus Melin from comment #38)
> (In reply to u from comment #36)
> > (In reply to u from comment #35)
> > > (In reply to Magnus Melin from comment #34)
> > 
> > > > - The checkbox is to require secure or not is no longer visible once the checks fail. I think it should to be de-checkable there
> > > 
> > > This was not intentional. A fixup patch will be submitted soon.
> > 
> > After redigging a bit, that was actually intentional. Sorry for the
> > confusion. Let me explain:
> > 
> > Once the automatic checks fail, Thunderbird switches to manual mode. And in
> > manual mode, we don't guess the configuration and do not retrieve Mozilla DB
> > again. So here we do not need to force SSL/TLS anymore.
> 
> Well you get to type in server name and choose Autodetect security settings
> so those are guessed again.

True, our option still has an effect when SSL == "Autodetect", even in manual configuration mode, so we should still show the checkbox. However, when checked and selecting SSL == "Plaintext" we will always have an error (since we'll try to probe with plaintext, which we do not allow), so probably this checkbox should control which options we allow in the SSL drop-down menu, i.e. "show Plaintext if and only if our option is diabled". 
What do you think?

Comment 41

[Magnus Melin [:mkmelin]]

That sounds good to me

Comment 42

[u]

Attachment: [PATCH 01/10] Optionally skip guessing plaintext protocols.

replaces previous patch 0001.

Comment 43

[u]

Attachment: [PATCH 02/10] Optionally skip insecure database autoconfiguration lookup.

Replaces previous patch 0002

Comment 44

[u]

Attachment: [PATCH 03/10] Optionally skip insecure DNS MX autoconfiguration lookup.

Comment 45

[u]

Attachment: [PATCH 04/10] Optionally skip insecure ISP autoconfiguration lookups.

Comment 46

[u]

Attachment: [PATCH 05/10] Add checkbox for toggling mailnews.auto_config.ssl_only.

Comment 47

[u]

Attachment: [PATCH 06/10] Optionally skip fetched configs using plaintext protocols.

Comment 48

[u]

Attachment: [PATCH 08/10] Simplification: move HostDetector ssl_only logic into callee.

Comment 49

[u]

Attachment: [PATCH 09/10] Move ssl_only logic into FetchHTTP.

Comment 50

[u]

Attachment: [PATCH 10/10] Don't hide the ssl_only checkbox in manual-edit mode.

Comment 51

[u]

There are now some modifications.

We've implemented the suggested changes in patches 8 to 10.

But it also appears that I've submitted an older version of the previous patches. Sorry about that.

In the previous patches 1-7 the new preference was called mailnews.auto_config_ssl_only but it changed  to mailnews.auto_config.ssl_only (notice the dot instead of underscore). Also, mailnews.auto_config.dns_mx_lookup.enabled is not present in the patches I've previously imported.

The changes are:
 * mailnews.auto_config_ssl_only => mailnews.auto_config.ssl_only
 * The above pref has the default True
 * DNS MX lookup is controller by its own pref mailnews.auto_config.dns_mx_lookup.enabled
 * The above pref has the default True
 * Improved commit messages

The patch on https://bugzilla.mozilla.org/show_bug.cgi?id=669238 is still the correct one. That's patch 7 of 10, which is missing here.

Comment 52

[u]

Comment on attachment 8724400 [details] [diff] [review]
0007-Make-FetchHTTP-silently-fail-on-SSL-cert-errors.patch

This patch is obsolete.

Comment 54

[u]

Hi Magnus, would you have time to review the improved patches you think? Cheers!

Comment 55

[Magnus Melin [:mkmelin]]

Comment on attachment 8754938 [details] [diff] [review]
[PATCH 09/10] Move ssl_only logic into FetchHTTP.

This part doesn't apply. Seems it's a diff from within your branch, not against comm-central tip. (You're moving mailnews.auto_config.ssl_only which doesn't exist yet)

Comment 56

[Magnus Melin [:mkmelin]]

Comment on attachment 8754926 [details] [diff] [review]
[PATCH 01/10] Optionally skip guessing plaintext protocols.

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

Not sure if it's this or one of the other patches, but it seems to never reach the "we didn't find any config for you" state, if there is no secure option available. 
(tried [e-mail])

::: mailnews/base/prefs/content/accountcreation/guessConfig.js
@@ +787,4 @@
>    if (protocol != UNKNOWN) {
> +    if (ssl == UNKNOWN) {
> +      var order = [
> +                   //getHostEntry(protocol, SSL, port),

we could remove these commented ones while we're touching this. (here and the other spots)

@@ +801,5 @@
> +                 getHostEntry(IMAP, TLS, port),
> +                 //getHostEntry(IMAP, SSL, port),
> +                 getHostEntry(POP, TLS, port)
> +                 //getHostEntry(POP, SSL, port)
> +                ];

... and with those removed it probably fits well on one line

var order = [ getHostEntry(IMAP, TLS, port), getHostEntry(POP, TLS, port) ];

Comment 57

[Magnus Melin [:mkmelin]]

Comment on attachment 8754928 [details] [diff] [review]
[PATCH 02/10] Optionally skip insecure database autoconfiguration  lookup.

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

::: mailnews/base/prefs/content/accountcreation/fetchConfig.js
@@ +121,5 @@
>  function fetchConfigFromDB(domain, successCallback, errorCallback)
>  {
>    let url = Services.prefs.getCharPref("mailnews.auto_config_url");
> +  if (Services.prefs.getBoolPref("mailnews.auto_config.ssl_only") &&
> +      url.indexOf("https://") != 0) {

url.startsWith("https://")

Comment 58

[Magnus Melin [:mkmelin]]

Comment on attachment 8754929 [details] [diff] [review]
[PATCH 03/10] Optionally skip insecure DNS MX autoconfiguration  lookup.

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

::: mailnews/base/prefs/content/accountcreation/fetchConfig.js
@@ +171,5 @@
>   */
>  function fetchConfigForMX(domain, successCallback, errorCallback)
>  {
> +  if (!Services.prefs.getBoolPref("mailnews.auto_config.dns_mx_lookup.enabled")) {
> +    // XXX We may not have to skip this method if we're using DNSSEC

nit: end with period (and lose the XXX)

::: mailnews/mailnews.js
@@ +806,5 @@
>  // Only allow fetching configs using secure means (HTTPS) and also reject
>  // non-secure (non-SSL/TLS) configurations
>  pref("mailnews.auto_config.ssl_only", true);
> +// DNS MX configuration lookup. Disabling this will break hosted
> +// domains, some vanirty comains, etc.

typos: vanity domains

Comment 59

[Magnus Melin [:mkmelin]]

Comment on attachment 8754932 [details] [diff] [review]
[PATCH 05/10] Add checkbox for toggling  mailnews.auto_config.ssl_only.

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

::: mailnews/base/prefs/content/accountcreation/emailWizard.js
@@ +228,5 @@
>        rememberPasswordE.disabled = true;
>      }
>  
> +    e("only_secure_protocols").checked =
> +      Application.prefs.getValue("mailnews.auto_config.ssl_only", false);

please use Services.prefs.getBoolPref instead (Application.prefs.getValue is going away)

@@ +527,5 @@
>  
> +  toggleSecureProtocols : function()
> +  {
> +    Application.prefs.setValue("mailnews.auto_config.ssl_only",
> +                               e("only_secure_protocols").checked);

Services.prefs.setBoolPref

Comment 60

[Magnus Melin [:mkmelin]]

Comment on attachment 8754937 [details] [diff] [review]
[PATCH 08/10] Simplification: move HostDetector ssl_only logic into  callee.

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

::: mailnews/base/prefs/content/accountcreation/guessConfig.js
@@ +808,4 @@
>    if (protocol != UNKNOWN) {
> +    if (ssl == UNKNOWN)
> +      return [getHostEntry(protocol, TLS, port),
> +              //getHostEntry(protocol, SSL, port),

here too remove the commented line, then it all fits on one line

Comment 61

[Magnus Melin [:mkmelin]]

Comment on attachment 8754939 [details] [diff] [review]
[PATCH 10/10] Don't hide the ssl_only checkbox in manual-edit mode.

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

::: mailnews/base/prefs/content/accountcreation/emailWizard.js
@@ +524,5 @@
>  
>    toggleSecureProtocols : function()
>    {
> +    var ssl_only = e("only_secure_protocols").checked;
> +    Application.prefs.setValue("mailnews.auto_config.ssl_only", ssl_only);

Services.prefs.setBoolPref

@@ +525,5 @@
>    toggleSecureProtocols : function()
>    {
> +    var ssl_only = e("only_secure_protocols").checked;
> +    Application.prefs.setValue("mailnews.auto_config.ssl_only", ssl_only);
> +    // This value is as "defined" in the XUL file, "similar" to

what value?

@@ +534,5 @@
> +        if (menu.getItemAtIndex(i).value == ssl_no_encryption) {
> +          menu.getItemAtIndex(i).hidden = ssl_only;
> +          // If the selected item becomes prohibited we need to switch
> +          // to some other option.
> +          var auto_detect_index = 0;

please be consistent with let and var. preferabley just use let for everything

Comment 62

[Magnus Melin [:mkmelin]]

Once you fix the issues I mentioned, please squash your patches into one. (Which is mozilla style.)

Comment 63

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Magnus Melin from comment #62)
> Once you fix the issues I mentioned, please squash your patches into one.
> (Which is mozilla style.)

This is Thunderbird code, so your call goes, but to be clear: it's extremely common in platform to separate patches out into logically distinct/coherent units and land them that way.  3, 5, 10, 20 patches, unsquashed.  But review changes and such do get squashed in.

Comment 64

[Andrew Sutherland [:asuth] (he/him)]

(Although obviously all the patches land in a single push.)

Comment 65

[Magnus Melin [:mkmelin]]

Yes, I don't have a strong opinion about it, but it's a lot of work to get the patches all in if there are 10 of them, then some need changes and you may have to put them in the correct order again for things to apply and such. It works better if everybody have commit access and probably review board helps...

BTW, there is no patch 7/10?

Comment 66

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Magnus Melin from comment #65)
> Yes, I don't have a strong opinion about it, but it's a lot of work to get
> the patches all in if there are 10 of them, then some need changes and you
> may have to put them in the correct order again for things to apply and
> such. It works better if everybody have commit access and probably review
> board helps...

The qimportbz mercurial extension lets you easily import one or more patches from bugs.  It is already part of version-control-tools that "mach mercurial-setup" can help configure.  It lives at https://hg.mozilla.org/hgcustom/version-control-tools/file/tip/hgext/qimportbz and there's some limited context/documentation at https://developer.mozilla.org/en-US/docs/Mozilla/Mercurial/Queues#Bugzilla

But the key thing is you can do "hg qimport bz:971347" and get:

1: [PATCH 01/10] Optionally skip guessing plaintext protocols.
   u: review? (mkmelin+mozilla)
2: [PATCH 02/10] Optionally skip insecure database autoconfiguration  lookup.
   u: review? (mkmelin+mozilla)
3: [PATCH 03/10] Optionally skip insecure DNS MX autoconfiguration  lookup.
   u: review? (mkmelin+mozilla)
4: [PATCH 04/10] Optionally skip insecure ISP autoconfiguration lookups.
   u: review? (mkmelin+mozilla)
5: [PATCH 05/10] Add checkbox for toggling  mailnews.auto_config.ssl_only.
   u: review? (mkmelin+mozilla)
6: [PATCH 06/10] Optionally skip fetched configs using plaintext  protocols.
   u: review? (mkmelin+mozilla)
7: [PATCH 08/10] Simplification: move HostDetector ssl_only logic into  callee.
   u: review? (mkmelin+mozilla)
8: [PATCH 09/10] Move ssl_only logic into FetchHTTP.
9: [PATCH 10/10] Don't hide the ssl_only checkbox in manual-edit mode.
   u: review? (mkmelin+mozilla)

Which patches do you want to import, and in which order? [Default is all]
(eg '1-3,5', or 's' to toggle the sort order between id & patch description)

Comment 67

[Magnus Melin [:mkmelin]]

Yes I use that all the time, that's why I had to rename the patches in this bug. Seems it can't really handle it when they all have the same filename (and its renaming prompt didn't help).

Comment 68

[u]

Dear Magnus,

thank you very much for your hard work!

We'll update the patches accordingly and if you prefer, we can prepare this as a single file. At Tails we like atomic commits, so that every small modification is well tracked. We've exported those patches directly from those commits. But I completely see that this makes reviewing much harder and thus, it can be changed. Your call.

Patch number 7/10 actually has been attached to this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=669238 (see https://bugzilla.mozilla.org/show_bug.cgi?id=971347#c51) Maybe you could also have a look at that one?

Cheers!
u.

Comment 69

[Magnus Melin [:mkmelin]]

Spreading up the patches over more than one bug is one thing that makes many patches and their possible dependencies difficult to handle. Also for possible uplifts it's easier to have one commit you handle, not 10. So I'd still prefer one patch.

Comment 70

[u]

Hi Magnus,

here we go! :)

Magnus Melin 2016-06-06 11:04:39 PDT, comment 55
> Comment on attachment 8754938 [details] [diff] [review] [diff] [review]
> [PATCH 09/10] Move ssl_only logic into FetchHTTP.
> 
> This part doesn't apply. Seems it's a diff from within your branch, not against comm-central tip. (You're moving mailnews.auto_config.ssl_only which doesn't exist yet)

This patch moves the (exact) code introduced in an earlier (and no patch in between touches this location) so it must apply if you apply the patches in the correct order.

Magnus Melin 2016-06-06 11:29:15 PDT, comment 56
> Not sure if it's this or one of the other patches, but it seems to never reach the "we didn't find any config for you" state, if there is no secure option available. (tried [e-mail])

Sorry! I found several issues in the patches that affects this and related areas (e.g. make the "Stop" button not work), with the first one being the most important one:
* Mark try as failed when we skip because of lack of encryption.
* Move error check into getMX().
* Make sure object isn't null before accessing field.
* Use the correct error callback.

Now if I try [e-mail] with only secure protocols, Icedove tells me that it failed to find a configuration and that we are in the manual edit mode. If I allow insecure protocols, I get isecure results for both POP and IMAP. I believe this is expected, and what you'd like to see.

Also, I added another patch, "Start over when toggling the secure protocols checkbox", that makes the checkbox behave like the email field when modified, i.e. we start over. The rationale is that this setting affects all (but one) steps during autoconfiguration so a re-run with the new setting applied is probably what the user wants.

Magnus Melin 2016-06-06 11:29:15 PDT, comment 56 
> Comment on attachment 8754926 [details] [diff] [review] [diff] [review]
> [PATCH 01/10] Optionally skip guessing plaintext protocols.

I'll skip what you say here since I revert the affected bits in a latter patch (when moving the logic into HostDetector instead, as you suggested). Hence I also ignore the (same) comments for that latter patch. When squashed it'll be like I never touched this part. :)

As for the rest of your review comments, they are addressed in the patch "Style and typo fixes after review".

Magnus Melin 2016-06-07 05:10:00 PDT, comment 69
> Spreading up the patches over more than one bug is one thing that makes many patches and their possible dependencies difficult to handle.

Ok, I include that patch as well then, so now this is a review for bug #669238 as well. But please check that bug for my response to your review of this patch. From now on I suppose you can post all review comments on this bug, for simplicity.

> Also for possible uplifts it's easier to have one commit you handle, not 10. So I'd still prefer one patch.

I've kept them splitted in series.patch so you can check what has been fixed incrementally since your previous review. But I also provided a squashed patch for the (hopeful!) final merge, and also if you really prefer to do the review from scratch. :)

--------------------------------------------------------------------------------

https://bugzilla.mozilla.org/show_bug.cgi?id=669238

Magnus Melin 2016-06-07 05:06:16 PDT, comment 13
> WHen there's no proxy available proxyInfo would be null. That doesn't seem to be handled?

If you look at how this patch modifies SocketUtil() you'll see that it *always* used null before the patch, and that in the end this value is passed to transportService.createTransport(), where null means "do not use a proxy".

As for the rest of your review comments, they are addressed in the patch "Style and typo fixes after review".

--------------------------------------------------------------------------------
Squashed commit message
--------------------------------------------------------------------------------

Disallow insecure protocols during autoconfiguration by default.

Introduce an option mailnews.auto_config.ssl_only (default:
true) which, when set, affects autoconfiguration as follows:

* Only allow Mozilla database lookups if the URL uses HTTPS.
* Only try ISP lookup over HTTPS.
* Discard configurations not using SSL/TLS or STARTTLS from the
  above two methods.
* Only probe SMTP/POP/IMAP with SSL/TLS and STARTTLS.

In other words, when the option is set, enforce secure protocols
both when doing lookups and in the resulting configuration. Without
this, there are several vectors for MitM during autoconfiguration.
In the autoconfiguration window the option can be toggled via a
checkbox so that legacy ISPs that only support plaintext protocols
are still supported. (Will-fix: #971347)

In order to better support privacy oriented tools like Tor, add
SOCKS support during the autoconfiguration probing step, which
previously was always performed unproxied even when a proxy was
configured. (Will-fix: #669238)

Also, introduce an option mailnews.auto_config.dns_mx_lookup.enabled
(default: true) which controls whether the DNS MX lookup step should
be performed during autoconfiguration.

Comment 71

[u]

Attachment: Disallow-insecure-protocols-during-autoconfiguration.patch

This patch is a squashed single patch of all commits.

Comment 72

[u]

Attachment: 0001-Optionally-skip-guessing-plaintext-protocols.patch

Comment 73

[u]

Attachment: 0002-Optionally-skip-insecure-database-autoconfiguration-.patch

Comment 74

[u]

Attachment: 0003-Optionally-skip-insecure-DNS-MX-autoconfiguration-lo.patch

Comment 75

[u]

Attachment: 0004-Optionally-skip-insecure-ISP-autoconfiguration-looku.patch

Comment 76

[u]

Attachment: 0005-Add-checkbox-for-toggling-mailnews.auto_config.ssl_o.patch

Comment 77

[u]

Attachment: 0006-Optionally-skip-fetched-configs-using-plaintext-prot.patch

Comment 78

[u]

Attachment: 0007-Add-SOCKS-proxy-support-for-account-autoconfiguratio.patch

Comment 79

[u]

Attachment: 0008-Simplification-move-HostDetector-ssl_only-logic-into.patch

Comment 80

[u]

Attachment: 0009-Move-ssl_only-logic-into-FetchHTTP.patch

Comment 81

[u]

Attachment: 0010-Don-t-hide-the-ssl_only-checkbox-in-manual-edit-mode.patch

Comment 82

[u]

Attachment: 0011-Style-and-typo-fixes-after-review.patch

Comment 83

[u]

Attachment: 0012-Use-the-correct-error-callback.patch

Comment 84

[u]

Attachment: 0013-Make-sure-object-isn-t-null-before-accessing-field.patch

Comment 85

[u]

Attachment: 0014-Move-error-check-into-getMX.patch

Comment 86

[u]

Attachment: 0015-Mark-try-as-failed-when-we-skip-because-of-lack-of-e.patch

Comment 87

[u]

Attachment: 0016-Start-over-when-toggling-the-secure-protocols-checkb.patch

Comment 88

[u]

Attachment: 0017-Rename-method.patch

Comment 89

[u]

Attachment: series.patch - contains 17 single patches as a series

Comment 90

[u]

Hi Magnus, 

sorry for the multiple attachments. I've tried to provide several formats of the patches, so you can pick what you prefer.

* This is a squashed commit: https://bugzilla.mozilla.org/attachment.cgi?id=8762026
* This is series of all single commits: https://bugzilla.mozilla.org/attachment.cgi?id=8762045
* The other patches are self explaining, single commits.

I've marked only the first one (squashed commit) as to be reviewed.

I hope this implementation is now more on top of your expectations :)

Please also note that we've added https://bugzilla.mozilla.org/attachment.cgi?id=8762033 here (and to the series and to the squashed commit) which fixes https://bugzilla.mozilla.org/show_bug.cgi?id=669238. I'll also add a comment on this bug. 

Cheers!

Comment 91

[u]

Oops. I copy pasted much more than intended into this comment, sorry. :)
The comment for https://bugzilla.mozilla.org/show_bug.cgi?id=669238 should not have been here, neither the commit message. Oh, well.

Comment 92

[Magnus Melin [:mkmelin]]

Comment on attachment 8762026 [details] [diff] [review]
Disallow-insecure-protocols-during-autoconfiguration.patch

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

So a few nits. But there is some real problem too as I can't set up a gmail account with this patch applied.

Exception@chrome://messenger/content/accountcreation/util.js:137:5
ServerException@chrome://messenger/content/accountcreation/fetchhttp.js:269:3
FetchHTTP.prototype._response@chrome://messenger/content/accountcreation/fetchhttp.js:197:19
FetchHTTP.prototype.start/request.onload@chrome://messenger/content/accountcreation/fetchhttp.js:123:35
EventHandlerNonNull*FetchHTTP.prototype.start@chrome://messenger/content/accountcreation/fetchhttp.js:123:5
fetchConfigFromDB@chrome://messenger/content/accountcreation/fetchConfig.js:143:3
EmailConfigWizard.prototype.findConfig/this._abortable</self._abortable<@chrome://messenger/content/accountcreation/emailWizard.js:621:31
fetchConfigFromISP/error@chrome://messenger/content/accountcreation/fetchConfig.js:99:9
fetchConfigFromISP/error/fetch<@chrome://messenger/content/accountcreation/fetchConfig.js:103:47
FetchHTTP.prototype._error@chrome://messenger/content/accountcreation/fetchhttp.js:218:7
FetchHTTP.prototype._response@chrome://messenger/content/accountcreation/fetchhttp.js:197:7
FetchHTTP.prototype.start/request.onload@chrome://messenger/content/accountcreation/fetchhttp.js:123:35
EventHandlerNonNull*FetchHTTP.prototype.start@chrome://messenger/content/accountcreation/fetchhttp.js:123:5
fetchConfigFromISP/error@chrome://messenger/content/accountcreation/fetchConfig.js:106:7
fetchConfigFromISP/fetch<@chrome://messenger/content/accountcreation/fetchConfig.js:110:43
FetchHTTP.prototype._error@chrome://messenger/content/accountcreation/fetchhttp.js:218:7
FetchHTTP.prototype._response@chrome://messenger/content/accountcreation/fetchhttp.js:197:7
FetchHTTP.prototype.start/request.onerror@chrome://messenger/content/accountcreation/fetchhttp.js:124:36
EventHandlerNonNull*FetchHTTP.prototype.start@chrome://messenger/content/accountcreation/fetchhttp.js:124:5
setTimeout handler*runAsync@chrome://messenger/content/accountcreation/util.js:45:3
fetchConfigFromDisk@chrome://messenger/content/accountcreation/fetchConfig.js:19:31
EmailConfigWizard.prototype.findConfig@chrome://messenger/content/accountcreation/emailWizard.js:592:23
EmailConfigWizard.prototype.onNext@chrome://messenger/content/accountcreation/emailWizard.js:572:5
oncommand@chrome://messenger/content/accountcreation/emailWizard.xul:1:1
Bad response content
-- Exception object --
+ _message (string) 'Bad response content'
+ stack (string) 3533 chars
+ code (number) -4
+ uri (string) 'https://live.mozillamessaging.com/autoconfig/v1.1/google.com'
+ constructor (function) 6 lines
+ message (string) 'Bad response content'
+ toString (function) 4 lines
*

::: mailnews/base/prefs/content/accountcreation/fetchConfig.js
@@ +74,5 @@
> +  let plaintext_url1 = "http://" + conf2;
> +  var urls = [secure_url0, secure_url1];
> +  if (!Services.prefs.getBoolPref("mailnews.auto_config.ssl_only")) {
> +    urls.push(plaintext_url0, plaintext_url1);
> +  }

zeros and ones in variable names are not so nice, and mixing indexes and numbers is also confusing

would you mind just not doing the intermediaries, like

var urls = ["http://" + conf, "http://" + conf2];

@@ +88,3 @@
>      {
>        successCallback(readFromXML(result));
> +    };

please format it like
let success = function(result) {

}

@@ +88,5 @@
>      {
>        successCallback(readFromXML(result));
> +    };
> +
> +  let error = function(i, e)

same thing here

Comment 93

[Magnus Melin [:mkmelin]]

Oh wait, that problem seems to be a new failure on trunk (see bug 1280682).

Comment 94

[u]

(In reply to Magnus Melin from comment #92)
> Comment on attachment 8762026 [details] [diff] [review]
> Disallow-insecure-protocols-during-autoconfiguration.patch

Thanks for the review!

> So a few nits. But there is some real problem too as I can't set up a gmail
> account with this patch applied.

The styles fixes have been incorporated into Disallow-insecure-protocols-during-autoconfiguration.patch (i.e. the one you want to merge), and can be viewed separately in 0018-Style-fixes-after-review.patch (for completeness).

Note that the patches shouldn't affect this FetchHTTP instance -- the only change from our patches that affects the Mozilla DB lookup is that we fail if it's not using HTTPS (luckily the default database URL is HTTPS).

Can you please confirm that this issue was not provoqued by our patches?

Comment 95

[u]

Attachment: 0018-Style-fixes-after-review.patch

Comment 96

[u]

Attachment: Disallow-insecure-protocols-during-autoconfiguration.patch

Comment 97

[Magnus Melin [:mkmelin]]

Yes that bug has now been fixed.

Hmm, toggling the "use only secure protocols" box doesn't start over anymore. I'm pretty sure one of your versions did. Do you see this?

Comment 98

[Magnus Melin [:mkmelin]]

Comment on attachment 8763407 [details] [diff] [review]
Disallow-insecure-protocols-during-autoconfiguration.patch

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

You will also have to adjust some tests, at least mailnews/base/test/unit/test_autoconfigXML.js, and maybe mozmill/account/test-mail-account-setup-wizard.js
See a try run at https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f82da364c8e7&selectedJob=19648

Comment 99

[u]

(In reply to Magnus Melin from comment #97)
> Yes that bug has now been fixed.
> 
> Hmm, toggling the "use only secure protocols" box doesn't start over
> anymore. I'm pretty sure one of your versions did. Do you see this?

Hi Magnus,

Thanks for trying to review this.
We'll get back to you when my team mate is back from vacation.
Sorry about that!
Cheers!

Comment 100

[u]

Attachment: 0001-Disallow-insecure-protocols-during-autoconfiguration.patch

Comment 101

[u]

Hi!

(In reply to Magnus Melin from comment #97) 
> Hmm, toggling the "use only secure protocols" box doesn't start over
> anymore. I'm pretty sure one of your versions did. Do you see this?

Juggling this many patches is apparently hard for us -- that patch was
dropped by mistake. Now the patch (https://bugzilla.mozilla.org/attachment.cgi?id=8766705) should be fine. I'm terribly sorry about this!

Comment 102

[u]

(In reply to Magnus Melin from comment #98)
> Comment on attachment 8763407 [details] [diff] [review]
> Disallow-insecure-protocols-during-autoconfiguration.patch
> 
> Review of attachment 8763407 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You will also have to adjust some tests, at least
> mailnews/base/test/unit/test_autoconfigXML.js,

I modified this test so it exercises both (w.r.t. the added pref) code
paths. I haven't been able to test it myself, though, but perhaps you
can send it through your infra and we see how it goes?

> and maybe
> mozmill/account/test-mail-account-setup-wizard.js

This test boils down to configuring an account with the XML from
mail/test/mozmill/account/xml/example.com which uses SSL for the
incoming server and STARTTLS for the outgoing server so nothing changes
due to our patches. Also, while it uses TAB to jump between elements in
the initial config screen, it never goes beyond the password field, so
the added checkbox has no influence. So, this test needs no adjustment.

Comment 103

[Magnus Melin [:mkmelin]]

Comment on attachment 8766705 [details] [diff] [review]
0001-Disallow-insecure-protocols-during-autoconfiguration.patch

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

THx, the "start over" thing is now working. Unfortunately the test-mail-account-setup-wizard.js still fails with this patch applied

(Also, please use unix line endings in the patch, hg didn't want to apply it before I converted the patch to unix line endings.)

Go to obj-dir/mail and run
make mozmill-one SOLO_TEST=account/test-mail-account-setup-wizard.js

TEST-UNEXPECTED-FAIL | /opt/comm-central/src/mail/test/mozmill/account/test-mail-account-setup-wizard.js | test-mail-account-setup-wizard.js::test_bad_password_uses_old_settings
TEST-START | /opt/comm-central/src/mail/test/mozmill/account/test-mail-account-setup-wizard.js | test_remember_password
TEST-PASS | /opt/comm-central/src/mail/test/mozmill/account/test-mail-account-setup-wizard.js | test-mail-account-setup-wizard.js::test_remember_password
TEST-START | /opt/comm-central/src/mail/test/mozmill/account/test-mail-account-setup-wizard.js | teardownModule
TEST-PASS | /opt/comm-central/src/mail/test/mozmill/account/test-mail-account-setup-wizard.js | test-mail-account-setup-wizard.js::teardownModule
INFO Passed: 3
INFO Failed: 2
INFO Skipped: 0
SUMMARY-PASS | test-mail-account-setup-wizard.js::setupModule
SUMMARY-UNEXPECTED-FAIL | test-mail-account-setup-wizard.js | test-mail-account-setup-wizard.js::test_mail_account_setup
  EXCEPTION: Timeout waiting for current config to become non-null
    at: utils.js line 447
       TimeoutError utils.js:447 13
       waitFor utils.js:485 11
       MozMillController.prototype.waitFor controller.js:687 3
       test_mail_account_setup/< test-mail-account-setup-wizard.js:93 5
       Runner.prototype.wrapper frame.js:580 7
       startTest test-window-helpers.js:317 11
       waitFor utils.js:478 5
       WindowWatcher_waitForModalDialog test-window-helpers.js:377 5
       wait_for_modal_dialog test-window-helpers.js:614 3
       open_mail_account_setup_wizard test-mail-account-setup-wizard.js:43 10
       test_mail_account_setup test-mail-account-setup-wizard.js:75 3
       Runner.prototype.wrapper frame.js:585 9
       Runner.prototype._runTestModule frame.js:655 9
       Runner.prototype.runTestModule frame.js:701 3
       Runner.prototype.runTestFile frame.js:534 3
       runTestFile frame.js:713 3
       Bridge.prototype._execFunction server.js:179 10
       Bridge.prototype.execFunction server.js:183 16
       Session.prototype.receive server.js:283 3
       AsyncRead.prototype.onDataAvailable server.js:88 3
  EXCEPTION: Timeout waiting for modal dialog to open.
    at: utils.js line 447
       TimeoutError utils.js:447 13
       waitFor utils.js:485 11
       WindowWatcher_waitForModalDialog test-window-helpers.js:377 5
       wait_for_modal_dialog test-window-helpers.js:614 3
       open_mail_account_setup_wizard test-mail-account-setup-wizard.js:43 10
       test_mail_account_setup test-mail-account-setup-wizard.js:75 3
       Runner.prototype.wrapper frame.js:585 9
       Runner.prototype._runTestModule frame.js:655 9
       Runner.prototype.runTestModule frame.js:701 3
       Runner.prototype.runTestFile frame.js:534 3
       runTestFile frame.js:713 3
       Bridge.prototype._execFunction server.js:179 10
       Bridge.prototype.execFunction server.js:183 16
       Session.prototype.receive server.js:283 3
       AsyncRead.prototype.onDataAvailable server.js:88 3
SUMMARY-UNEXPECTED-FAIL | test-mail-account-setup-wizard.js | test-mail-account-setup-wizard.js::test_bad_password_uses_old_settings
  EXCEPTION: Timeout waiting for create button to be visible and active
    at: utils.js line 447
       TimeoutError utils.js:447 13
       waitFor utils.js:485 11
       MozMillController.prototype.waitFor controller.js:687 3
       test_bad_password_uses_old_settings/< test-mail-account-setup-wizard.js:181 7
       Runner.prototype.wrapper frame.js:580 7
       startTest test-window-helpers.js:317 11
       waitFor utils.js:478 5
       WindowWatcher_waitForModalDialog test-window-helpers.js:377 5
       wait_for_modal_dialog test-window-helpers.js:614 3
       open_mail_account_setup_wizard test-mail-account-setup-wizard.js:43 10
       test_bad_password_uses_old_settings test-mail-account-setup-wizard.js:163 3
       Runner.prototype.wrapper frame.js:585 9
       Runner.prototype._runTestModule frame.js:655 9
       Runner.prototype.runTestModule frame.js:701 3
       Runner.prototype.runTestFile frame.js:534 3
       runTestFile frame.js:713 3
       Bridge.prototype._execFunction server.js:179 10
       Bridge.prototype.execFunction server.js:183 16
       Session.prototype.receive server.js:283 3
       AsyncRead.prototype.onDataAvailable server.js:88 3
SUMMARY-PASS | test-mail-account-setup-wizard.js::test_remember_password
SUMMARY-PASS | test-mail-account-setup-wizard.js::teardownModule
/opt/comm-central/src/mozilla/../mail/testsuite-targets.mk:42: recipe for target 'mozmill-one' failed
make: *** [mozmill-one] Error 1

::: mailnews/base/test/unit/test_autoconfigXML.js
@@ +226,5 @@
> +  for (let ssl_only in [true, false]) {
> +    Services.prefs.setBoolPref("mailnews.auto_config.ssl_only", ssl_only);
> +    var domParser = new DOMParser();
> +    var config = xmlReader.readFromXML(JXON.build(
> +      domParser.parseFromString(clientConfigXML, "text/xml")));

you don't need to read the config twice. 
Just read it once, and the 
 do_readFromXML_checks(config, true)
 do_readFromXML_checks(config, false)

Comment 104

[Magnus Melin [:mkmelin]]

u: any luck fixing the patch so the test would be happy?

Comment 105

[u]

Hello Magnus!

(In reply to Magnus Melin from comment #104)
> u: any luck fixing the patch so the test would be happy?

thanks for caring :) Yes, we managed to fix this. But we also found that there is a small problem with OAuth which we are trying to fix before resubmitting the patch.
I hope we can fix it soon and you can review the whole thing once and for all.

Cheers!

Comment 106

[u]

Attachment: 0001-Disallow-insecure-protocols-during-autoconfiguration.patch

Comment 107

[u]

Dear Magnus,

please find the new patch attached.

We also modified the patch in order to honor OAuth used by Gmail.
I hope this time we got everything right.

Cheers!
u.

Comment 108

[u]

Hi Magnus, do you think you'll be able to test this patch again? Thanks & cheers! u.

Comment 109

[Magnus Melin [:mkmelin]]

Comment on attachment 8791621 [details] [diff] [review]
0001-Disallow-insecure-protocols-during-autoconfiguration.patch

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

Unfortunately the test seems to still fail (in a different way), now ends in an "Incoming server already exists" error dialog.

Please try running 
cd obj-*
make mozmill-one SOLO_TEST=account/test-mail-account-setup-wizard.js

Also see the last part of my comment 103 (just read the config once).

Comment 110

[u]

Hi,

just to let you know that we haven't dropped the ball, we just have incredible issues getting the test suite to start (Thunderbird segfaults on start for that test, but not others). This could be something due to Debian's build toolchain, so we're gonna try in some other environment and will let you know soonish if it worked out :)

Cheers!

Comment 111

[Wayne Mery (:wsmwk)]

(In reply to u from comment #110)
> Hi,
> 
> just to let you know that we haven't dropped the ball, we just have
> incredible issues getting the test suite to start (Thunderbird segfaults on
> start for that test, but not others). This could be something due to
> Debian's build toolchain, so we're gonna try in some other environment and
> will let you know soonish if it worked out :)
> 
> Cheers!

Have you figured it out?

Comment 112

[alta88]

Attachment: Disallow-insecure-protocols-during-autoconfiguration.patch

Unbitrot and fix test issue. Auto account config is a raging mess and this is important to make it rage less.

Comment 113

[Ben Bucksch (:BenB)]

Comment on attachment 8920591 [details] [diff] [review]
Disallow-insecure-protocols-during-autoconfiguration.patch

alta88, unlike what you might think ("raging mess"), there is a lot of thought behind all the policies and the implentation, and this has been discussed to death for month before it was implemented, and shortly thereafter.

I am a strong proponent of SSL only mail servers, in fact my ISPDB probably has done a lot for forward this cause, including because I refused to use non-SSL configs whenever possible. So, I completely share your goal here.

We allowed non-SSL methods, because some hosting providers simply cannot give SSL configs for all domains. We also need the MX lookup, it's a critical part in finding configs, and DNS is still not secured (and even if it indeed uses DNSSEC, we have no way to verify that). So, this hole exists, we can't do anything about it. Like I said, this has all been discussed endlessly, you're not the first to bring it up, and there's simply nothing we can reasonably do about it.

Aside form that, there are still major ISPs out there that have only non-SSL mail servers. So, your change would break all these users. (And no, these users can't change the pref. The idea of the autoconfig is that everything is automatic.)

There's no point for a pref with default false, either. Anybody who would set the pref to SSL_only=true would also be able to take the necessary care to check the results and the config.

This is precisely why we show the result config, so that you can do that. The result UI is also explicit about which source the config comes from, i.e. whether it is the target domain or ISPDB or guessing. So, security-conscious users have a lot of information already. There's no need for a pref.

So, the change to default to SSL only config methods is denied.

Before you implement policy changes to autoconfig, please discuss them with me. This saves you time.

Comment 114

[alta88]

Comment on attachment 8920591 [details] [diff] [review]
Disallow-insecure-protocols-during-autoconfiguration.patch


I notice your memory has suddenly improved[1]. I did not ask your opinion because I have zero interest in it. The comments here and the archeology indicate you choose practicality in supporting insecure providers at the expense of security/privacy (while claiming otherwise). That is a FAIL.

What it also a FAIL, is continually[2] attempting to freeze a past decision, in another world, as immutable. Enough people disagree. There is no "breakage" here, merely an issue of convenience. Security trumps convenience period.

You also seem to exhibit extreme territoriality. There's no place for that, and you are neither an owner nor a peer of _any_ mail or mailnews module; I suggest you revisit how governance works. So I'm going to ask for mail/ owner/peer feedback. In future, please do not exhibit the poor etiquette of changing f/r flags that do not belong to you.

Firefox has wholehearedly embraced security/privacy hardening and has an entire intiative to port code from the Tor project; this patch is a contribution from people involved there.


[1] https://bugzilla.mozilla.org/show_bug.cgi?id=755988#c23
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=664633#c5

Comment 115

[Ben Bucksch (:BenB)]

> I did not ask your opinion because I have zero interest in it. 

alta88, you are very offensive.

As I explained to you, this is *not* my personal opinion. My personal opinion matches yours. The fact that we decided something else is rooted in facts outside of our control.

I've carefully considered and discussed this topic with the community for many months. My original position was the same as yours, so I understand your position. Yet, the facts and input from other parties spoke another language. The resulting decision we made is a balance between all opinions, actors and needs. There are no new facts that would change the decision.

We need the DNS MX lookup to have decent coverage. DNSSEC is still not here for all domains, nor can we validate it. So there's no point to kill other useful configuration sources, because the result would still be vulnerable to active attacks, and there's no way to solve this properly.

Remember that the user always sees, verifies and approves the specific config.

If you kill one config source, all you do is to let the user fall into the guessing mode, which is even more vulnerable to attack, and has seriously worse results than an authoritative information from a DB or ISP.

You propose to
a) kill non-SSL config sources (e.g. DNS MX), which would mean that we simply don't find a good config for many ISPs, including those that do offer SSL mail servers.
b) kill non-SSL configs (mail servers), which means that you unsupport many large ISPs. That's not an option, either.

It simply means that we do not find certain configs anymore.

This is not a question of personal opinion, but of fact. So, please stop making this a personal matter and the personal attacks against me.

> you are neither an owner nor a peer of _any_ mail or mailnews module

I'm part of the engineering council of Thunderbird, together with 3 other developers, which is the module owner of all of Thunderbird, or above the module owners. This is new since 3 months, so you may not be aware.
I am also the author of the code in question.
In both these capacities, I denied this patch the review.

Comment 116

[Jorg K (CEST = GMT+2)]

Comment on attachment 8920591 [details] [diff] [review]
Disallow-insecure-protocols-during-autoconfiguration.patch

Sorry, I can't give feedback here since I know nothing about the subject. As an aside, my German provider Hetzner isn't listed in the ISPDB, so I have to type in the server names every single time I set up a test account (which I do frequently).

On the governance side we are discussing to have mail/ and mailnews/ owned by three people: Magnus, Kent and myself, but that's not set in stone yet. We're also discussing the establishment of an Engineering Committee of which a few developers would be part. This committee would ultimately take a decision in cases like this.

BTW, Kent has a new address? [e-mail]?

Comment 117

[:aceman]

Comment on attachment 8920591 [details] [diff] [review]
Disallow-insecure-protocols-during-autoconfiguration.patch

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

Sorry, I don't know how the proposed SSL-only setup improves the security. Ben is the expert in this area.
Of course SSL/TLS connections would be preferred but not all servers have it. Also fetching the config via https is fine, but what if the server does not provide it? We still need to fetch via http and then the attack can be mounted there. Or is there a new flag in ISPDB which servers provide HTTPs and only should be connected via that? That still will not cover servers that provide their own file.
I see the patch add proxy support in some of the paths, maybe at least that part could be accepted?

Comment 118

[Ben Bucksch (:BenB)]

What aceman said.

> I see the patch add proxy support in some of the paths, maybe at least that part could be accepted?

Sure. guessconfig should use the same proxy config as the eventual mail server connections will use, esp for SOCKS. If it currently doesn't (I didn't test that), that's a bug and I'd be glad to see a specific patch for that merged.

Comment 119

[alta88]

(In reply to Ben Bucksch (:BenB) from comment #115)
> > I did not ask your opinion because I have zero interest in it. 
> 
> alta88, you are very offensive.
> 
> As I explained to you, this is *not* my personal opinion. My personal
> opinion matches yours. The fact that we decided something else is rooted in
> facts outside of our control.

The only materially relevant fact is what you advocate for, which in this bug means preventing a UI option to enable the user to choose a secure only auto configuration.  This is outrageously wrong.
 
Explain what compelled you to lay aside your claimed security/privacy opinion and not only accept but vocally advocate for the opposite.  Who is 'we'?

> 
> I've carefully considered and discussed this topic with the community for
> many months. My original position was the same as yours, so I understand
> your position. Yet, the facts and input from other parties spoke another
> language. The resulting decision we made is a balance between all opinions,
> actors and needs. There are no new facts that would change the decision.
> 

You attempt to portray this 'decision' as some grand collaborative effort, where there is no such evidence. If there is documentary evidence, show it. The archeology also indicates there is none. You are at best not operating in an open manner with this particular tack.

I have read the docs linked in Bug 64633 comment 20. Those are not that evidence, and wholly inadequate to show compelling evidence of 'facts beyond our control' or 'all opinions, actors and needs' (really a tiny handful of regulars). So I'll have to ask you to also stop aggrandizing.

You have actively quashed requests for security reviews in Bug 64633.  You don't seem to grasp what a security review entails: it means if we find a particular vulnerability, you will not release the code until the vector is eliminated; it _doesn't_ mean, we'll identify some holes, document them, and then ignore them.  


> We need the DNS MX lookup to have decent coverage. DNSSEC is still not here
> for all domains, nor can we validate it. So there's no point to kill other
> useful configuration sources, because the result would still be vulnerable
> to active attacks, and there's no way to solve this properly.
> 
> Remember that the user always sees, verifies and approves the specific
> config.
> 
> If you kill one config source, all you do is to let the user fall into the
> guessing mode, which is even more vulnerable to attack, and has seriously
> worse results than an authoritative information from a DB or ISP.
> 
> You propose to
> a) kill non-SSL config sources (e.g. DNS MX), which would mean that we
> simply don't find a good config for many ISPs, including those that do offer
> SSL mail servers.
> b) kill non-SSL configs (mail servers), which means that you unsupport many
> large ISPs. That's not an option, either.
> 

Stop misrepresenting the patch. It add a checkbox which, if unchecked at the user's choice, continues the same hackme friendly behavior as exists currently.

> It simply means that we do not find certain configs anymore.
> 

This is _completely_ unimportant at the expense of security. So what?  Firefox will not connect you to a site advertising only a deprecated encryption.

Further, autoconfig is broken, ragingly. And not just the UI ergonomic. Let's look at guessConfig.js. Servers that are SSL are never found[1 above]. Servers that advertise Kerberos or other clientside unconfigured option falsely create an account that will never connect if the user isn't set up.

Users already must enter the few details to set up an account for a great many cases, as before autoconfig.  But now, their autoconfig request has been splayed out there.

> This is not a question of personal opinion, but of fact. So, please stop
> making this a personal matter and the personal attacks against me.
> 

Stop misdirecting attacks on your decisions/position as personal. Be assured I am also not interested in whether criticism offends you.

As an aide, I will give you an example of a personal attack:
"alta88, you are very offensive."

> > you are neither an owner nor a peer of _any_ mail or mailnews module
> 
> I'm part of the engineering council of Thunderbird, together with 3 other
> developers, which is the module owner of all of Thunderbird, or above the
> module owners. This is new since 3 months, so you may not be aware.
> I am also the author of the code in question.

Irrelevant, and speaks to territoriality.

> In both these capacities, I denied this patch the review.

Comment 120

[alta88]

Gerv, if you have a moment, your take (official or unofficial) would be appreciated.

The issue here is adding a checkbox to Thunderbird new account autoconfig UI. If checked, only secure methods will be used (https/valid cert for self hosted autoconfig files; only secure config options in manual edit). If unchecked, autoconfig will operate as it does currently.

The patch was contributed by TorBirdy contributors.

Comment 121

[alta88]

> I'm part of the engineering council of Thunderbird, together with 3 other
> developers, which is the module owner of all of Thunderbird, or above the
> module owners. This is new since 3 months, so you may not be aware.

Reference tb-planning post "Engineering Council Plans" by R Kent James dated 7/18/2017. The salient details are:
B.3 "When a concrete decision in a technical area is needed, a key leadership group will make that decision by voting."
B.3 "The Engineering Council is not intended to replace the existing technical module owners and peers, or play a direct role in approving patches. The intent is to deal with issues that are larger than an individual patch."

You individually have no module ownership or authority outside a collective vote by members of that council. Nowhere is stated that being a member grants any individual module authority.

This patch is not even a technical or architectural question; it is one of UX and maybe product management. Regardless, again, stop assigning yourself imaginary authority. Read, understand, and follow the rules; it's unfit for a council member to fail at such governance basics.

Comment 122

[Magnus Melin [:mkmelin]]

(In reply to Ben Bucksch (:BenB) from comment #115)
> You propose to
> a) kill non-SSL config sources (e.g. DNS MX), which would mean that we
> simply don't find a good config for many ISPs, including those that do offer
> SSL mail servers.
> b) kill non-SSL configs (mail servers), which means that you unsupport many
> large ISPs. That's not an option, either.
> 
> It simply means that we do not find certain configs anymore.

The patch just make non-secure lookups harder. If it fails to find a config the first time (secure only), you can uncheck the box and thereby go with the active choice to use non-secure protocols.

Comment 123

[Gervase Markham [:gerv]]

alta88: I see nothing wrong with the operation of Thunderbird's governance processes in this bug. Torbirdy and Thunderbird may not have the same audience or design goals, and so may well not make the same trade-offs. This is fine. If you want to reduce your patch delta load, and this is a feature you use, then get the UI-less part checked in (by politely asking for review) and just carry a UI or pref patch.

Gerv

Comment 124

[Matt]

Despite opinions to the contrary,  users just want the software to do what it advertises,  first time and every time.  If you want to introduce a preference that makes detection of accounts less successful in the first instance then I ask those that make such a decision provide the support effort required to support the users in their quest to make the software work out of the box.

Please keep in mind that we have a significant level of small business operators connecting to their "own" domains.  SSL  is not provided free and most of the budget end of the market offer SSL only with an inappropriate certificate (the hosts domain).  You can argue all around the block.  The provider is remiss.  The business owner is not doing a good job on security.  The reality is the business owner does not care as long as those emails offering sales or sales leads keep coming.  They have no vested interest in fixing the PHP on say the WIX web site. We had, I am not sure if we still do, entries in the ISPDB that secured the connection with SSL and immediately errors about the server names.  ISP's often use the same servers for their employees mail (corporate)  so just because you get a response on an SSL connection does not mean it really works.

I would therefore ask that any requirement to use SSL be optional and NOT THE DEFAULT.  I can understand where the TOR folks are coming from,  but honestly most users just do not care.  In this case having the box and leaving it blank unless the user specifies the ore secure method would be my preferred approach.  Along with some user documentation that actually explains what happens when you click the option.

Comment 125

[Jorg K (CEST = GMT+2)]

https://bugzilla.mozilla.org/user_profile?login=rkent-allmailnews%40caspia.com
Last activity 2012-07-28 20:19:43 CEST

Comment 126

[u]

(In reply to alta88 from comment #120)
> The patch was contributed by TorBirdy contributors.

Actually, we are not working on Torbirdy, but on Tails (https://tails.boum.org).
Tails ships Thunderbird by default as Thunderbird is currently the most usable email client around, in our opinion. So - thanks for your work and thanks for considering using at least part of these patches.

Tails is being started over 20,000 times per day (see our monthly reports), and many of our users rely on Thunderbird for their security. And even outside of Tails, Thunderbird is widely used. 

In our opinion, users who do not want or cannot use SSL should actively make the choice to opt out, and the secure option should be the default. (The "users don't care" argument is unapplicable - even ignorant users should be secured by default by the tools we - the Free Software community - provide to them.)

We are interested in upstreaming these patches (which we ship in Tails by the way, if you want to try) because we'd like the entire Thunderbird community to profit from them. Unfortunately we were not able to get Thunderbird's test suite to run and hence these patches got stuck here :(. However, we are still very much interested in moving this forward. Can you let us know how we can help? Otherwise, can you provide the possibility for us to use the test suite (do you have an internal mechanism to push branches there which would be tested automatically for example?) - then we could try to further improve upon them.

Cheers!
u. for the Tails project

Comment 127

[Ben Bucksch (:BenB)]

> users who do not want or cannot use SSL should actively make the choice to opt out,
> and the secure option should be the default.

The account creation dialog will
a) offer SSL mail servers, if available
b) actively warn the user when using a non-SSL mail server - with a big, red dialog
   You can only proceed when you check the box "[x] I understand the risks". Impossible to ignore.
c) ask the user to confirm the mail server names

So, users have to actively opt out of SSL. Already as it is.

Note that Thunderbird is widely used in enterprise settings, and many enterprise network operators take the stance that the internal network is secured and that SSL on the local network is pointless. You can easily argue that they are wrong, but that's the most common situation.

Comment 128

[u]

Dear Ben,

(In reply to Ben Bucksch (:BenB) from comment #127)

> The account creation dialog will
> a) offer SSL mail servers, if available
> b) actively warn the user when using a non-SSL mail server - with a big, red
> dialog
>    You can only proceed when you check the box "[x] I understand the risks".
> Impossible to ignore.
> c) ask the user to confirm the mail server names
> 
> So, users have to actively opt out of SSL. Already as it is.

"As it is"? I'm not sure I do completely understand you, sorry. Are you talking about such an option mean in our patches or in a released Thunderbird version? Forgive my ignorance!

> Note that Thunderbird is widely used in enterprise settings, and many
> enterprise network operators take the stance that the internal network is
> secured and that SSL on the local network is pointless. You can easily argue
> that they are wrong, but that's the most common situation.

Understood. Obviously, for such situations there needs to be a fallback.

Comment 129

[u]

Hi Ben!

I've now seen the red warning in TB, that's a-ma-zing. I think there is now only the proxy part and that SSL is preferred over plaintext (if SSL exists, otherwise plaintext is selected) which still seem relevant from our initial patches - we'll adapt this and we'll keep you posted for another review :)

Thanks for your work!
u.

Comment 130

[Ben Bucksch (:BenB)]

> SSL is preferred over plaintext (if SSL exists, otherwise plaintext is selected)

That should already be the case.

Comment 131

[u]

Hi!

We've finally managed to get the test suite running and could verify that our new, reduced patchset does not throw any errors.

The previous approach of "one big patch" clearly failed, so let's try an incremental approach with one patch per commit (besides now there are several bugfixes that deserve their own commits).

Please review and merge this series up to the patch you are happy with; each commit is atomic and at most depends on earlier patches, so this is safe and makes sense, and we can return to the remaining patches later. In fact, in most cases if you dislike a particular patch you could just skip that one and at worst get a simple conflict in mailnews/mailnews.js when importing a later patch.

Here are some extra patch comments for the reviewer:

0001-Fix-the-prefilled-hostnames-for-manual-edit.patch
0002-Invalidate-config-when-restarting-autoconfiguration.patch
0003-Remove-buggy-debug-code.patch
0004-Fix-issue-when-mailnews.auto_config_url-is-empty.patch
0005-Document-that-mailnews.-auto_config-mx_service-_url-.patch
0006-Comment-pref.patch

These are bugfixes and minor improvements which I suspect you'll want to merge right away.

0007-Prefer-fetched-configurations-using-SSL-over-plainte.patch

There's quite some effort made to prefer SSL in other places, so this is arguably a bugfix as well.

0008-Add-SOCKS-proxy-support-for-account-autoconfiguratio.patch

Fixes: https://bugzilla.mozilla.org/show_bug.cgi?id=669238

0009-Generalize.patch
0010-Fix-outdated-error-handling.patch
0011-Improve-logging-for-fetchConfigFromISP-during-autoco.patch

These are nice on their own, but builds up to the next patch:

0012-Fetch-ISP-configuration-using-SSL-if-possible.patch

This opportunistic attempt to prevent MitM can easily be worked around (attacker blocks https, thunderbird downgrades to http and the attacker wins) but is better than nothing, and builds up to:

0013-Add-pref-to-force-SSL-for-ISP-fetch-during-autoconfi.patch

This adds an opt-in pref that fixes the above MitM completely.

0014-Add-pref-for-whether-we-accept-OAuth2-during-autocon.patch
0015-Use-overridden-user-agent-for-HTTP-s-during-autoconf.patch
0016-Add-pref-for-each-guess-timeout-during-autoconfigura.patch

These allows TorBirdy to greatly improve the autoconfiguration UX when using Tor.

0017-Improve-logging-of-guess-instances-during-autoconfig.patch

This one helped me debugging 0015 and 0016.

0018-Add-pref-for-whether-we-accept-plaintext-protocols-d.patch

Given the "red warning" that appears whenever a user tries to accept a plaintext protocol (see comment https://bugzilla.mozilla.org/show_bug.cgi?id=971347#c127), this patch is not critical for us any more.

FWIW, I've seen the test-mail-account-setup-wizard.js test pass with all these patches applied.

Please let us know if this fits your requirements :)

Cheers,
u.

Comment 132

[u]

Attachment: 0001-Fix-the-prefilled-hostnames-for-manual-edit.patch

Comment 133

[u]

Attachment: 0002-Invalidate-config-when-restarting-autoconfiguration.patch

Comment 134

[u]

Attachment: 0003-Remove-buggy-debug-code.patch

Comment 135

[u]

Attachment: 0004-Fix-issue-when-mailnews.auto_config_url-is-empty.patch

Comment 136

[u]

Attachment: 0005-Document-that-mailnews.-auto_config-mx_service-_url-.patch

Comment 137

[u]

Attachment: 0006-Comment-pref.patch

Comment 138

[u]

Attachment: 0007-Prefer-fetched-configurations-using-SSL-over-plainte.patch

Comment 139

[u]

Attachment: 0008-Add-SOCKS-proxy-support-for-account-autoconfiguratio.patch

Comment 140

[u]

Attachment: 0009-Generalize.patch

Comment 141

[u]

Attachment: 0010-Fix-outdated-error-handling.patch

Comment 142

[u]

Attachment: 0011-Improve-logging-for-fetchConfigFromISP-during-autoco.patch

Comment 143

[u]

Attachment: 0012-Fetch-ISP-configuration-using-SSL-if-possible.patch

Comment 144

[u]

Attachment: 0013-Add-pref-to-force-SSL-for-ISP-fetch-during-autoconfi.patch

Comment 145

[u]

Attachment: 0014-Add-pref-for-whether-we-accept-OAuth2-during-autocon.patch

Comment 146

[u]

Attachment: 0015-Use-overridden-user-agent-for-HTTP-s-during-autoconf.patch

Comment 147

[u]

Attachment: 0016-Add-pref-for-each-guess-timeout-during-autoconfigura.patch

Comment 148

[u]

Attachment: 0017-Improve-logging-of-guess-instances-during-autoconfig.patch

Comment 149

[u]

Attachment: 0018-Add-pref-for-whether-we-accept-plaintext-protocols-d.patch

Comment 150

[u]

These patches should invalidate alta88's patch "Disallow-insecure-protocols-during-autoconfiguration.patch".

Comment 151

[Ben Bucksch (:BenB)]

Comment on attachment 8964851 [details] [diff] [review]
0005-Document-that-mailnews.-auto_config-mx_service-_url-.patch

I don't think this comment is really necessary. It seems kind of obvious.
If you add it, please put the new sentence on its own line.

Comment 152

[Ben Bucksch (:BenB)]

Comment on attachment 8964853 [details] [diff] [review]
0007-Prefer-fetched-configurations-using-SSL-over-plainte.patch

Per specification of the config file, the first configuration is the default.
This patch would break it and violate the spec.

That spec deliberately let's the config file author determine the default. The author of the config file determines what's in there, anyway. If he wants plaintext to be the default, there's nothing we can do. If we pick SSL against his wish, the config file author would simply remove the SSL config entirely, to achieve his goal of keeping plaintext as the default. We'd end up worse than we are now. So, this patch is *not* increasing security, but actually decreasing it. Esp. for security-conscious users.

I think most or all configs in our database are preferring SSL anyway (I insisted on that), so this patch won't make much of a difference in reality anyway.

Comment 153

[Ben Bucksch (:BenB)]

Comment on attachment 8964854 [details] [diff] [review]
0008-Add-SOCKS-proxy-support-for-account-autoconfiguratio.patch

This is a useful addition and a good start. Thank you.

However, we make dozens of tries. Is it possible to cache the proxy lookup? Or does it really change based on each server name? In theory of the API, it might, but in reality, if it does, there's something really wrong. These are all mail servers, and they should all be on the same network. If they are on different networks or need different proxies, there's something phishy going on and we're better off *not* doing that.

So, I would make one proxy lookup and cache the proxy for all tries. That should avoid a slowdown caused by this patch.

Comment 154

[Ben Bucksch (:BenB)]

Comment on attachment 8964855 [details] [diff] [review]
0009-Generalize.patch

Good start, but doens't take it far enough.

If we really add more URLs, we need to parallelize the requests. Otherwise autoconfig will be too slow - it's already too slow as it is. The calls are currently successive. I think there is a bug on file to parallelize them (don't remember the bug number), and there is IIRC a suitable Abortable implementation, and I even once tried implementing that, but ran into bugs and gave up. Compare Bug 549040, which did the same for guessing.

Comment 155

[Ben Bucksch (:BenB)]

Comment on attachment 8964856 [details] [diff] [review]
0010-Fix-outdated-error-handling.patch

> .statusText doesn't throw any more

Yes, XmlHttpRequest probably changed behavior.

However, we always need to have a proper errorCode. 0 is normally success. So, please keep the -2.

Also, for clarity, please keep the normal case as straight code path, like so:
     errorCode = this._request.status;
     errorStr = this._request.statusText;
     if (errorCode == 0) {
         errorStr = getStringBundle( ...
     }

Otherwise fine. r=BenB with that changed.

Comment 156

[Ben Bucksch (:BenB)]

Comment on attachment 8964861 [details] [diff] [review]
0015-Use-overridden-user-agent-for-HTTP-s-during-autoconf.patch

Strange that "general.useragent.override" is not used by the HTTP library automatically.

I trust that you tested this and found it necessary. r=BenB

It's in the right place, too.

Comment 157

[Ben Bucksch (:BenB)]

Comment on attachment 8964862 [details] [diff] [review]
0016-Add-pref-for-each-guess-timeout-during-autoconfigura.patch

Makes sense. r=BenB

Comment 158

[Ben Bucksch (:BenB)]

Comment on attachment 8964864 [details] [diff] [review]
0017-Improve-logging-of-guess-instances-during-autoconfig.patch

Makes sense. The logging made sense when the probes were sequential, but with parallel probes, they are now useless. Your patch prefixes the debug line with the host, which makes it useful again.

Comment 159

[Ben Bucksch (:BenB)]

Comment on attachment 8964865 [details] [diff] [review]
0018-Add-pref-for-whether-we-accept-plaintext-protocols-d.patch

I'd call it mailnews.auto_config.ssl_only_mail_servers
(That would allow a mailnews.auto_config.ssl_only_config_servers)

I seriously doubt the usefulness of this pref, though. We already prefer SSL, if we have an SSL config. So, all your patch does it present *no* config to the user.

That said, I'm in favor of hidden prefs that serve special uses. That's what makes open-source strong.

Code is fine.

Once the pref name is changed, r=BenB

Comment 160

[Ben Bucksch (:BenB)]

Comment on attachment 8964858 [details] [diff] [review]
0012-Fetch-ISP-configuration-using-SSL-if-possible.patch

I'm generally in favor of this, but only after we parallelized the requests.

Comment 161

[Ben Bucksch (:BenB)]

Comment on attachment 8964859 [details] [diff] [review]
0013-Add-pref-to-force-SSL-for-ISP-fetch-during-autoconfi.patch

Please rename pref to mailnews.auto_config.ssl_only_config_servers

This also depends on previous patches that need rework.

When adding comments, please try to keep one sentence or idea per line,
as it was before.

Otherwise fine.

Comment 162

[Ben Bucksch (:BenB)]

Comment on attachment 8964860 [details] [diff] [review]
0014-Add-pref-for-whether-we-accept-OAuth2-during-autocon.patch

I'm curious: How can you work around OAuth2?
(I personally dislike OAuth2 very much)

Code:
Can you remove the code from readFromXML.js? The fact that we don't read any prefs there so far is a sign that this class does not implement policy, but it should be higher up.
How about leaving the logic in emailWizard.js, in the place that handles OAuth (which you already modify)? The logic could simply take the read config objects and filter out those that have OAuth.

However, I would recommend to use a more measured approach: If you have alternative non-OAuth configs, you could remove the OAuth configs from the list. If you only have OAuth configs, and you remove them, you left the user stranded. So, you might want to use OAuth anyway, even if it's not nice.

Comment 163

[Ben Bucksch (:BenB)]

Comment on attachment 8964845 [details] [diff] [review]
0001-Fix-the-prefilled-hostnames-for-manual-edit.patch

The dot in the front is deliberate. We're in manual config, because none of the servers we tried worked. The user has to manually find the right hostnames and enter them manually.

What we prefill is simply to save him a little bit of typing, to not have to repeat the domain name manually.

The hostnames are intentionally invalid, because the user should *not* simply try those. We *should* fail, if the user did not modify them. (If we autocorrect them, then that's a bug.)

So, this isn't a bug, but that's intentional.

Comment 164

[Ben Bucksch (:BenB)]

Comment on attachment 8964846 [details] [diff] [review]
0002-Invalidate-config-when-restarting-autoconfiguration.patch

This is not correct.

This function switchToMode() is a pure UI function that only adapts the UI to the current state, but does not change the state by itself.

You should take a look at the callers of switchToMode("start"). You will see that not all of them should clear the config. Which is the bug this patch would introduce.

If you end up with server hostname "a.a" instead of "b.b" after changing the email address, then something else in the logic long before that is wrong.

I think you want to add this._currentConfig = null; to function onStartOver() (and only there). This is called from onInputEmail(), which is called when the user changes the email address.

Comment 165

[Ben Bucksch (:BenB)]

Comment on attachment 8964850 [details] [diff] [review]
0004-Fix-issue-when-mailnews.auto_config_url-is-empty.patch

Yes. Whoever added
  if (!url.includes("{{domain}}"))
    url = url + domain;
should have done that after the |if (!url.length)| check. That change broke it.

Patch is correct. Thanks. r=BenB.

Comment 166

[Ben Bucksch (:BenB)]

Comment on attachment 8964857 [details] [diff] [review]
0011-Improve-logging-for-fetchConfigFromISP-during-autoco.patch

Not sure why that would be necessary, but fine with me. r=BenB

Comment 167

[Ben Bucksch (:BenB)]

Hello u,

I would like to thank you for the patchset. It's measured and reasonable. It's well coded, too.

Took me a while to review. I hope my review comments are useful. I've r- a few patches that need only a few changes.

The most difficult change necessary is probably to parallelize the HTTP requests. I consider it necessary to not extend the length of autoconfig even further. I would have added https long ago, if it wasn't for that.

If we do that, we could parallize all the HTTP requests, including ISPDB. You'd basically create a pool of requests and wait for the pool to finish. The pool and waiting would be an Abortable subclass, like SuccessiveAbortable already is.

Ideally, we'd add MX requests to the pool as well, but that's harder, because we need to find the MX first. What if the other calls all succeed before the MX call returns? The pool would have already completed and the MX calls not be made. Due to that, and because MX lookup is not always necessary, I would suggest to make the MX calls as a second pool, second step.

Ben

Comment 168

[Wayne Mery (:wsmwk)]

back to "u" for finishing

Comment 169

[u]

Thanks for asking for more info and thank you for your reviews.
I'm (finally) attaching a new patchset (that was a nice timely coincidence!) and commenting below.

Comment 151:
> Comment on attachment 8964851 [details] [diff] [review] [diff] [review]
> 0005-Document-that-mailnews.-auto_config-mx_service-_url-.patch
>
> I don't think this comment is really necessary. It seems kind of
> obvious. If you add it, please put the new sentence on its own line.

Fair enough, patch dropped.

Comment 161:
> 
> Comment on attachment 8964859 [details] [diff] [review] [diff] [review]
> 0013-Add-pref-to-force-SSL-for-ISP-fetch-during-autoconfi.patch
> 
> Please rename pref to mailnews.auto_config.ssl_only_config_servers

Patch updated!

Comment 163:
> Comment on attachment 8964845 [details] [diff] [review] [diff] [review]
> 0001-Fix-the-prefilled-hostnames-for-manual-edit.patch
>
> The dot in the front is deliberate. We're in manual config, because
> none of the servers we tried worked. The user has to manually find
> the right hostnames and enter them manually.
>
> What we prefill is simply to save him a little bit of typing, to not
> have to repeat the domain name manually.

I see. FWIW, note that as a user it confused me, and ended up costing
me more time than I ever expect to get back by not having to type that
single dot. Just sayin'! :)

> So, this isn't a bug, but that's intentional.

Patch dropped.

Comment 164:
> Comment on attachment 8964846 [details] [diff] [review] [diff] [review]
> 0002-Invalidate-config-when-restarting-autoconfiguration.patch
> 
> This is not correct.
> 
> [...]

Thanks! Patch updated accordingly!

Comment 154:
> Comment on attachment 8964855 [details] [diff] [review] [diff] [review]
> 0009-Generalize.patch
> 
> Good start, but doens't take it far enough.
>
> If we really add more URLs, we need to parallelize the
> requests. Otherwise autoconfig will be too slow - it's already too
> slow as it is. The calls are currently successive.

Thanks, that is a great idea (especially for my use case, since auto
configuration already is pretty slow when using Tor)! I reworked the
"Generalize" patch to an implementation of this.

Comment 155:
> 
> Comment on attachment 8964856 [details] [diff] [review] [diff] [review]
> 0010-Fix-outdated-error-handling.patch
> 
> > .statusText doesn't throw any more
> 
> Yes, XmlHttpRequest probably changed behavior.
>
> However, we always need to have a proper errorCode. 0 is normally
> success. So, please keep the -2.
> 
> Also, for clarity, please keep the normal case as straight code
> path, like so:
>      errorCode = this._request.status;
>      errorStr = this._request.statusText;
>      if (errorCode == 0) {
>          errorStr = getStringBundle( ...
>      }
> 
> Otherwise fine. r=BenB with that changed.

Patch updated!

Comment 156:
> 
> Comment on attachment 8964861 [details] [diff] [review] [diff] [review]
> 0015-Use-overridden-user-agent-for-HTTP-s-during-autoconf.patch
> 
> Strange that "general.useragent.override" is not used by the HTTP
> library automatically.
> 
> I trust that you tested this and found it necessary. r=BenB

When I originally tested if `XMLHttpRequest` respects that pref, I had
apparently misspelled "override" in the pref... thanks for being
vigilant, and sorry for the noise!

Patch dropped!

Comment 159:
> 
> Comment on attachment 8964865 [details] [diff] [review] [diff] [review]
> 0018-Add-pref-for-whether-we-accept-plaintext-protocols-d.patch
> 
> I'd call it mailnews.auto_config.ssl_only_mail_servers
> (That would allow a mailnews.auto_config.ssl_only_config_servers)

Patch updated!

> I seriously doubt the usefulness of this pref, though. We already
> prefer SSL, if we have an SSL config. So, all your patch does it
> present *no* config to the user.

That is a feature for users in situations where an unauthenticated
fetch could lead to a tampered configuration that compromises
security. As long as HTTP is available as a "fallback", we are
susceptible to a trivial downgrade attack: blocking HTTPS.

> That said, I'm in favor of hidden prefs that serve special
> uses. That's what makes open-source strong.

Well said! :)

> Once the pref name is changed, r=BenB

Patch updated!

Comment 153:
> Comment on attachment 8964854 [details] [diff] [review] [diff] [review]
> 0008-Add-SOCKS-proxy-support-for-account-autoconfiguratio.patch
> 
> This is a useful addition and a good start. Thank you.
>
> However, we make dozens of tries. Is it possible to cache the proxy
> lookup?

Yes, it's pretty easy. We can just move the for-loop starting the
probes inside onProxyAvailable instead. When resolving the (single)
proxy we do so on the first probe's url, assuming all other urls would
resolve to the same proxy, like you have proposed.

> Or does it really change based on each server name? In theory of the
> API, it might, but in reality, if it does, there's something really
> wrong. These are all mail servers, and they should all be on the
> same network. If they are on different networks or need different
> proxies, there's something phishy going on and we're better off
> *not* doing that.

There is `network.proxy.no_proxies_on` where a user could have put
`pop.domain` but not `imap.domain`, so one would use the proxy and not
the other. Also, there's the FoxyProxy extension, whose main use case
is to define rules where domains can resolve to different proxies --
this extension is no longer compatible with Thunderbird, but I still
mention it to demonstrate that users might have different expectations
and/or usecases, and that this situation can occur without anything
being "really wrong". Therefore I feel this is a risky assumption to
make, at least when the gains (minor performance) are so small.

> So, I would make one proxy lookup and cache the proxy for all
> tries. That should avoid a slowdown caused by this patch.

The proxy lookup is pretty fast (it's all local files after all). I
made some very crude benchmarking, recording the respective times
before each probe's call to `proxyService.asyncResolve` until the
corresponding `onProxyAvailable` is called, and each was ~20 ms. Since
these fetches happen in parallel, the delays happen in parallel as
well, so the actual total delay the user will experience is (worst
case) the highest single such delay (not the sum). Hence we can assume
that it's insignificant. Considering how much time guessing takes as a
whole, IMHO, this is premature optimization.

I personally don't care too much, since this won't affect my use
case. If you still disagree, just let me know and I'll provide a patch
with the approach I mentioned above (I have not implemented this
into the current patchset).

Comment 162:
> 
> Comment on attachment 8964860 [details] [diff] [review] [diff] [review]
> 0014-Add-pref-for-whether-we-accept-OAuth2-during-autocon.patch
> 
> I'm curious: How can you work around OAuth2?
> (I personally dislike OAuth2 very much)

If you are referencing my mention of "workaround" in the commit
message, what I meant was: "TorBirdy can set this new pref to discard
OAuth2 configs and most likely get password-cleartext instead". So I
guess "password-cleartext" is my workaround... :)

> Code:
> Can you remove the code from readFromXML.js? The fact that we don't
> read any prefs there so far is a sign that this class does not
> implement policy, but it should be higher up.

`readFromXML()` actually already implements policy by selecting a
single socketType and authentication option (see the comments "take
first that we support") -- if it didn't implement any policy it would
create one config for each combination and return the list to the
caller for any policy filtering. `readFromXML()` would require a
complete rewrite, as would all places where it is called. I'll be
honest, I'm not intrigued about doing that.

Actually, since there a single global policy (i.e. the state of prefs
at the time auto configuration is initiated) - and this is unlikely to
change - I think it is good that it is applied at the lowest level,
so it is not forgotten, misapplied or needlessly repeated by callers.

What do you think?

> How about leaving the logic in emailWizard.js, in the place that
> handles OAuth (which you already modify)? The logic could simply
> take the read config objects and filter out those that have OAuth.

By that point `readFromXML()` has already discarded other
authentication options (like I said above, it already implements
policy). It's possible that the alternative config could save us, but
the real solution would require the above mentioned rewrite of
`readFromXML()` and friends.

> However, I would recommend to use a more measured approach: If you
> have alternative non-OAuth configs, you could remove the OAuth
> configs from the list. If you only have OAuth configs, and you
> remove them, you left the user stranded. So, you might want to use
> OAuth anyway, even if it's not nice.

The users are not necessarily stranded since subsequent methods like
`guessConfig()` often does the trick. So my use case (OAuth2 is
guaranteed to fail) is made worse with your proposal, since it would
block the only method that could possibly work for me
(i.e. `guessConfig()`).

What do you think? Could we leave the patch as-is?

Comment 152:
> Comment on attachment 8964853 [details] [diff] [review] [diff] [review]
> 0007-Prefer-fetched-configurations-using-SSL-over-plainte.patch
> 
> Per specification of the config file, the first configuration is the
> default. This patch would break it and violate the spec.

Looking at
https://wiki.mozilla.org/Thunderbird:Autoconfiguration:ConfigFileFormat
I only see this type of default specified for incomingServer,
outgoingServer and authentication. (Overall, I find it unclear what
tags can be repeated or not, and what effect this might have). As far
as I can tell, the meaning of multiple socketType tag is currently
unspecified, so only those of us reading Thunderbird's source code
will realize that multiple ones are supported, and have a default.  I
strongly suppose that no implementation but Thunderbird's has taken it
into account. (Since you, the specification author, also wrote that
code :) ) So to me it still seems we can change this, and I think we
should!

When using SSL the relationship between port and socketType is
one-to-one (port standards). The specification is quite unclear in
this area: are two port tags allowed? If so, what does it mean and how
does it relate to the socketType tags? Looking at Thunderbird's
implementation (in `readFromXML()`) it is assumed that exactly one
port is given per incomingServer/outgoingServer. I agree with that,
and then it follows that only one socketType should be allowed, which
fixes the same problem. It seems like both STARTTLS and plaintext map
to the same port, so in that case it would make sense to allow two
socketType:s. But the idea of a server-side "default" doesn't seem
like a great idea to me. Correct me if I misunderstood something here.

I wonder, what does a "default" mean for protocol negotiation? Could
you please clarify what problem it is trying to solve? As far as I can
tell, the server side having a default (or any sort of
preference/priority) doesn't actually do much since both sides must
cooperate to actually pick it (think about the history of security
downgrade attacks). So, in the end we're in a situation where each
side is completely responsible to negotiate for their own desired
level of security: the server by letting clients know what it finds
acceptable; the client by picking its preferred option among those
advertised, or opt out if none is good enough. The fact that there is
no need for a server-controllable default/preference when doing manual
configuration is a clue to me that it doesn't belong in automatic
configuration either. It has always been the user's choice, and I see
no reason for this to change for an automated client. Robots deserve
equal rights as humans! :)

Note that we are discussing an edge case: in practice (I grep:ed
Mozilla's complete database), configuration authors specify exactly
one socketType per incomingServer/outgoingServer, so they make a
incomingServer for SSL and a separate one for plaintext, for
instance. However, I worry that some random service provider out there
could write its own config with something like:

    <incomingServer type="imap">
      ...
      <socketType>plain</socketType>
      <socketType>SSL</socketType>
      ...

and (reasonably, IMHO) expect the behavior to be "this IMAP server
supports plaintext and SSL, pick what you like", instead of "this IMAP
server supports plaintext and SSL, but always use plaintext if it is
supported". Avoiding gotchas with such serious security implications
seems like worthwhile aiming for in the config format.

> That spec deliberately let's the config file author determine the
> default. The author of the config file determines what's in there,
> anyway. If he wants plaintext to be the default, there's nothing we
> can do. If we pick SSL against his wish, the config file author
> would simply remove the SSL config entirely, to achieve his goal of
> keeping plaintext as the default. We'd end up worse than we are
> now. So, this patch is *not* increasing security, but actually
> decreasing it. Esp. for security-conscious users.

I am not convinced by this example:

* What would your service provider do with users that do manual
  configuration and (naturally) pick SSL? Also, how often do we see
  instructions for manual email configuration where server providers
  say "we offer both plaintext and SSL, but we prefer if you use
  plaintext and probably will disable SSL if you start using it"? I
  don't see why we should care about such a usecase.

* Why did the author add an SSL socketType option to begin with when
  they apparently only want to support plaintext? If adding SSL was
  not a mistake, then what was the purpose of adding it? The wishes of
  your example's service provider seems self-contradictory and don't
  appear like a valid use case to me.

* How is security decreased? Without my patch, our starting point is
  "no security at all" for all configs where plaintext is listed as
  the first socketType, so security is at worst staying at zero with
  my patch, but if there is SSL we will use it and have strong
  security. Sure, your example's server admin will then eventually
  disable SSL, returning us back to zero security, but at no point did
  we perform worse with my patch. Given the admin's behavior SSL is
  not an option in practice, so we're all just stuck with no security
  :(

> I think most or all configs in our database are preferring SSL
> anyway (I insisted on that), so this patch won't make much of a
> difference in reality anyway.

That is great! It doesn't say anything about the ones served by the
email providers themselves, however. Let's improve the code so we
don't have to worry about them!

What do you think about this reasoning?  If you do not agree, would
you accept a patch where the new behavior is locked behind a pref?

Comment 167:
> Hello u,
>
> I would like to thank you for the patchset. It's measured and
> reasonable. It's well coded, too.

Thanks!

> Took me a while to review.

No problem, it takes way longer for me to respond. :)

> I hope my review comments are useful. I've r- a few patches that
> need only a few changes.

Your review has been extremely useful! Thanks a lot!

> The most difficult change necessary is probably to parallelize the
> HTTP requests. I consider it necessary to not extend the length of
> autoconfig even further. I would have added https long ago, if it
> wasn't for that.

Agreed!

> If we do that, we could parallize all the HTTP requests, including
> ISPDB. You'd basically create a pool of requests and wait for the
> pool to finish. The pool and waiting would be an Abortable subclass,
> like SuccessiveAbortable already is.
> 
> Ideally, we'd add MX requests to the pool as well, but that's
> harder, because we need to find the MX first. What if the other
> calls all succeed before the MX call returns? The pool would have
> already completed and the MX calls not be made. Due to that, and
> because MX lookup is not always necessary, I would suggest to make
> the MX calls as a second pool, second step.

I agree that these optimizations would be very nice, but beyond
parallelizing `fetchConfigFromISP()` I am afraid I cannot spend more time on this.

I hope however, that we're getting close to something mergeable.

Cheers!

Comment 170

[u]

Attachment: 0001-Invalidate-config-when-restarting-autoconfiguration.patch

Comment 171

[u]

Attachment: 0002-Remove-buggy-debug-code.patch

Comment 172

[u]

Attachment: 0003-Fix-issue-when-mailnews.auto_config_url-is-empty.patch

Comment 173

[u]

Attachment: 0004-Comment-pref.patch

Comment 174

[u]

Attachment: 0005-Prefer-fetched-configurations-using-SSL-over-plainte.patch

Comment 175

[u]

Attachment: 0006-Add-SOCKS-proxy-support-for-account-autoconfiguratio.patch

Comment 176

[u]

Attachment: 0007-Parallelize-fetchConfigFromISP.patch

Comment 177

[u]

Attachment: 0008-Fix-outdated-error-handling.patch

Comment 178

[u]

Attachment: 0009-Improve-logging-for-fetchConfigFromISP-during-autoco.patch

Comment 179

[u]

Attachment: 0010-Fetch-ISP-configuration-using-SSL-if-possible.patch

Comment 180

[u]

Attachment: 0011-Add-pref-to-force-SSL-for-ISP-fetch-during-autoconfi.patch

Comment 181

[u]

Attachment: 0012-Add-pref-for-whether-we-accept-OAuth2-during-autocon.patch

Comment 182

[u]

Attachment: 0013-Add-pref-for-each-guess-timeout-during-autoconfigura.patch

Comment 183

[u]

Attachment: 0014-Improve-logging-of-guess-instances-during-autoconfig.patch

Comment 184

[u]

Attachment: 0015-Add-pref-for-whether-we-accept-plaintext-protocols-d.patch

Comment 185

[u]

Comment on attachment 9027140 [details] [diff] [review]
0001-Invalidate-config-when-restarting-autoconfiguration.patch

Dear Ben, I'm assigning you as a reviewer to only the first patch, simply to avoid spamming you with reviewing requests. Let me know if that works for you. Thanks!

Comment 186

[Ben Bucksch (:BenB)]

Hey u,

thanks for all the updates. Unfortunately, this was at a most inopportune moment, because there's a huge refactoring pending and in review right now, but hasn't landed yet. And all your patches will conflict. This is a little embarrassing, because your updates were triggered by us (wayne).

Can I ask that you hold on this and wait for the other patches to land and then to update the patches again? I'm really sorry for the extra work this caused you now.

Ben

Comment 187

[Ben Bucksch (:BenB)]

Comment on attachment 9027140 [details] [diff] [review]
0001-Invalidate-config-when-restarting-autoconfiguration.patch

Great patch. This was an annoying bug I've noticed, too.

(This will not conflict with my patch and can land now)

Comment 188

[u]

(In reply to Ben Bucksch (:BenB) from comment #186)

Hi Ben,

> thanks for all the updates. Unfortunately, this was at a most inopportune
> moment, because there's a huge refactoring pending and in review right now,
> but hasn't landed yet. And all your patches will conflict. This is a little
> embarrassing, because your updates were triggered by us (wayne).


ah that is very unfortunate indeed :(

I was planning to update the patches, it was not only triggered by wayne, no worries.

Comment 189

[Ben Bucksch (:BenB)]

Comment on attachment 9027148 [details] [diff] [review]
0007-Parallelize-fetchConfigFromISP.patch

Good start. But won't work, because you cannot call successCallback() more than once.

Good news is that I've implemented the parallelization already in the other patch that's pending.

Comment 190

[Jorg K (CEST = GMT+2)]

Can you fold this zoo of patches into - say - less than eight? This is creating a lot of noise and we don't really need one-line patches.

Comment 191

[Ben Bucksch (:BenB)]

@Jörg: This is actually great. I like it this way. Not all of them will be accepted, and this makes it much easier to separate.

Comment 192

[u]

Hi again, Ben!

Could you link to the code changes (diffs) for this refactoring? I'd like to as quickly as possibly understand how badly these conflicts have set us back.

> Comment on attachment 9027148 [details] [diff] [review] [diff] [review]
> 0007-Parallelize-fetchConfigFromISP.patch
>
> Good start. But won't work, because you cannot call
> successCallback() more than once.

Ah, well I must admit didn't look/test fetchConfigFromISP() very carefully at this commit -- I focused on getting everything correct in the final state in my patch series.

That said, I guess all that's missing is that _abortable.cancel() should be called when we reach successCallback(), so any pending fetches are aborted and cannot call successCallback() a second time.

FWIW, while I haven't tested each individual patch by itself, I have tested the submitted state of of the whole patch series extensively and there was never any unexpected second call to successCallback(). And it makes sense: with the "https > http"
constraint blocking http, the two https (or http, if https failed) fetches would *both* have to succeed for it to happen, and that could only happen if the config XML file is served on both of the paths we test (i.e. with or without ".well-known/autoconfig") which I didn't test for.  But the fix is the same, just call _abortable.cancel() in successCallback().

> Good news is that I've implemented the parallelization already in
> the other patch that's pending.

Sad that we had to duplicate this work! :_(

I'm eager to see how you did it, and if it will be easy to re-implement the "https > http" constraint.

> @Jörg: This is actually great. I like it this way. Not all of them
> will be accepted, and this makes it much easier to separate.

It would be cool if we could take this to the next level and actually start merging things you think are good (so a candidate for this is: 0001-Invalidate-config-when-restarting-autoconfiguration.patch) while I continue working on the other patches that still need revision. This would require making the dependencies between patches clearer, but I can do that. What do you think?

Comment 193

[Ben Bucksch (:BenB)]

> Could you link to the code changes (diffs) for this refactoring?

Bug 1500105

Comment 194

[Ben Bucksch (:BenB)]

https://phabricator.services.mozilla.com/D9215

Comment 195

[Ben Bucksch (:BenB)]

@u: The above mentioned patch landed.

Comment 196

[Jorg K (CEST = GMT+2)]

So, is this bug going to be taken to completion? Despite what BenB said, I'd like to see the patches rebased and bundled. I believe we can handle more than one line per patch. So for example, why don't you bundle anything that is clean-up into on patch to start off with? Like 1-5 could be one patch.

Comment 197

[u]

Hi,

We've updated our patch series. It was very straightforward to work with the new priority system, and having all methods run in parallel is such a time saver (especially when running over slow Tor circuits). Great job! \o/

Most of our patches are identical (modulo trivial things like bracket positions) to our previous patch submission. The ones that were rewritten from scratch are:

• 0004-Also-fetch-ISP-configuration-using-SSL.patch
• 0005-Make-use-of-non-SSL-Exchange-AutoDiscover-methods-op.patch

For the other still ~identical patches our Comment 169 [0] is still unanswered by you.

Below I list for each new patch which of our responses to your comments that you should look at:

• 0003-Prefer-fetched-configurations-using-SSL-over-plainte.patch → Commend 152 (so, in our Comment 169, look for our response to your Comment 152)
• 0006-Add-pref-for-whether-we-accept-OAuth2-during-autocon.patch → Comment 162
• 0007-Add-SOCKS-proxy-support-for-account-guessing.patch → Comment 153
• 0010-Add-pref-for-whether-to-accept-plaintext-protocols-d.patch → Comment 159:

The remaining four patches (0001, 0002, 0008, 0009) are, as far as I can tell, already reviewed and ACKed by you.

Cheers!

[0] https://bugzilla.mozilla.org/show_bug.cgi?id=971347#c169

Comment 198

[u]

I'll upload the new version of the patches in ~1h.

Comment 199

[u]

Attachment: 0001-Invalidate-config-when-restarting-autoconfiguration - Landed

Comment 200

[u]

Attachment: 0002-Add-comment-for-pref - Landed

Comment 201

[u]

Attachment: 0003-Prefer-fetched-configurations-using-SSL-over-plainte.patch

Comment 202

[u]

Attachment: 0004-Also-fetch-ISP-configuration-using-SSL.patch

Comment 203

[u]

Attachment: 0005-Make-use-of-non-SSL-Exchange-AutoDiscover-methods-op.patch

Comment 204

[u]

Attachment: 0006-Add-pref-for-whether-we-accept-OAuth2-during-autocon.patch

Comment 205

[u]

Attachment: 0007-Add-SOCKS-proxy-support-for-account-guessing.patch

Comment 206

[u]

Attachment: 0008-Add-pref-for-setting-the-autoconfiguration-guess-tim - Landed

Comment 207

[u]

Attachment: 0009-Improve-logging-of-guess-instances - Landed

Comment 208

[u]

Attachment: 0010-Add-pref-for-whether-to-accept-plaintext-protocols-d.patch

Comment 209

[u]

(In reply to Jorg K (GMT+1) from comment #196)

So, is this bug going to be taken to completion? Despite what BenB said, I'd like to see the patches rebased and bundled. I believe we can handle more than one line per patch. So for example, why don't you bundle anything that is clean-up into on patch to start off with? Like 1-5 could be one patch.

Hi Jorg,

I have seen your comment only today. So there is no bundling in what we propose today to lead this issue to completion. I think in general that patches are easier to apply if they're atomic - especially if many developers operate on the same code base and code changes happen often.

Comment 210

[Magnus Melin [:mkmelin]]

hi u: there has been significant bitrot of these patches. I understand you have the 10 tiny different commits, but this is an unusual process for us, so please just fold it into one patch that we can test out, review and get landed.

Comment 211

[u]

Attachment: secure-account-creation.patch All patches in one file

Comment 212

[u]

(In reply to Magnus Melin [:mkmelin] from comment #210)

hi u: there has been significant bitrot of these patches. I understand you have the 10 tiny different commits, but this is an unusual process for us, so please just fold it into one patch that we can test out, review and get landed.

That would be great! Here it is: https://bugzilla.mozilla.org/attachment.cgi?id=9056143

Comment 213

[Ben Bucksch (:BenB)]

Comment on attachment 9047425 [details] [diff] [review]
0003-Prefer-fetched-configurations-using-SSL-over-plainte.patch

This breaks the protocol. The protocol defines that the configs are listed in order of preference. The preference is defined by the one giving the configuration.

If the config author is dead-set on wanting to let most of its users use a non-SSL config, and therefore giving it precedence despite in existing SSL config, then he can list the non-SSL config first, while still giving the option to use the SSL config for users who really want SSL.

If you remove that logic, then his only choice will be to remove the SSL config entirely. That hurts mostly the security-conscious users. I.e. you hurt your own user group the most with this choice.

We had some ISPS who have SSL configs, but insisted that the non-SSL one is the default. From the perspective of the ISPDB, with this patch, we would have no way to fulfill that wish and would need to entirely remove the SSL config, if we don't want to risk a conflict with the ISP in question.

So, this patch strictly removes options and makes things worse.

I understand the idea, but it will actually backfire and do the opposite of what's intended.

It's better to change the configs in the ISPDB. If you see configs that list non-SSL before SSL, make a request to change the config order around.

r- for this one.

Comment 214

[Ben Bucksch (:BenB)]

Comment on attachment 9047426 [details] [diff] [review]
0004-Also-fetch-ISP-configuration-using-SSL.patch

Code changes are good. r+ for that. But a few changes for the pref, please:

1.

> mailnews.auto_config.ssl_only_config_servers

Please change to mailnews.auto_config.fetchFromISP.sslOnly

> +// Whether we will only allow SSL channels when fetching.

add ..."config from ISP"

2.

+// When false an active attacker can block non-SSL fetches and then
+// MitM the HTTP fetch, granting the attacker full control over the
+// client configuration.

I understand your idea, but everybody is missing 2 important points:

1. Using https only will violate the established protocol, and will not work in many cases. This is for a reason, for email hosters, who want to do a redirect from customer domains to their own config.
AutoDiscover protocol is also defined that way, for the same reason. And in practice, most AutoDiscover servers are doing it exactly like that: http on the customer domain, redirect to provider, which then returns a config via http.
I know it's insecure, but if you insist on https, you will simply not find a config, plain and simple.

2. Even with this set to true, you are still suspect to a MITM, due to the guess config.
And you cannot disable the guess config, because only 40-50% of the configs are in the DB and via ISP and the rest needs to be guessed. Guessing often leads to success, so we really need that. Internal email servers often have no SSL.

So, while I do not object to the preference itself, I do not think it's wise to set it. And if we add a comment, it needs to make these problems clear, not be an advertising of how stupid TB is.

So, I suggest the following comment:

// Allow only SSL channels when fetching config from ISP
// If false, an active attacker can block SSL fetches and then
// MITM the HTTP fetch, determining the config that is shown to the user.
// However:
// 1. The user still needs to explicitly approve the false config.
// 2. Most hosters that offer this ISP config do so on HTTP and not on HTTPS.
//    That's because they direct customer domains (HTTP) to their provider
//    config (HTTPS). If you set this to true, you simply break this mechanism.
//    You will simply not get most configs.
// 3. Even with this mechanism protected, there are still guess config and
//    AutoDiscover config mechanisms which have the exact same problem.
//    And we really need the guess config, it's the mechanism that provides
//    about 40% of the configs. Plus, many internal mail servers
//    have no SSL, so even with this preference set to true, you cannot
//    solve the problem.

Yes, it's a long comment, but it's a very complex problem. Nobody understands this who doesn't know the entirety of the problem. That's why this keeps coming up, and intelligent people keep arguing for it. I hope this comment would remove this as a FAQ.

With the above changes, and if you include this entire comment, this is r+.

3.
Minor nit: Please keep the "Allow..." instead of changing it to "Whether...". The Allow is more concrete (that the bool pref is positive) and easier to understand.

Comment 215

[Ben Bucksch (:BenB)]

Comment on attachment 9047427 [details] [diff] [review]
0005-Make-use-of-non-SSL-Exchange-AutoDiscover-methods-op.patch

This is a violation of the defined Exchange AutoDiscover protocol.

And it will not work. Most Exchange AutoDiscover servers respond on on that third HTTP-only URL from a customer domain, and then redirect to a HTTPS URL from the provider.

There already is an option to disable the entire protocol.

Comment 216

[Ben Bucksch (:BenB)]

Comment on attachment 9047428 [details] [diff] [review]
0006-Add-pref-for-whether-we-accept-OAuth2-during-autocon.patch

u, I would suggest you file a new bugzilla bug and include the workarounds for OAuth2 that you implemented, and merge these to Thunderbird as well. That would make this change useful for some Thunderbird users as well. As-is, it's not useful for anybody but you.

If merged, small changes, please:

> mailnews.auto_config.allow_oauth2

mailnews.auto_config.OAuth2.enabled

+          if (!allow_oauth2 && iO.auth == Ci.nsMsgAuthMethod.OAuth2) {
+            iO.auth = null;
+            continue;

Remove line `iO.auth = null;`. It's unnecessary.
Same for outgoing.

Comment 217

[Ben Bucksch (:BenB)]

Comment on attachment 9047429 [details] [diff] [review]
0007-Add-SOCKS-proxy-support-for-account-guessing.patch

This is a great change.

Just a small code structure comment: Could you please implement a
async function getProxy()
or
function getProxy(resultCallback)
which returns the proxyInfo or null?
It would contain both the ProxyResolveCallback, the first few lines of onProxyAvailable, plus proxyService.asyncResolve, but none of the code that was there before.

That way, the proxy business is entirely confined in one function and a) is easier readable, separating proxy code from the rest of the code and b) reusable in other code.

Comment 218

[Ben Bucksch (:BenB)]

Comment on attachment 9047430 [details] [diff] [review]
0008-Add-pref-for-setting-the-autoconfiguration-guess-tim - Landed

Nice

Comment 219

[Ben Bucksch (:BenB)]

Comment on attachment 9047431 [details] [diff] [review]
0009-Improve-logging-of-guess-instances - Landed

r+

Comment 220

[Ben Bucksch (:BenB)]

Comment on attachment 9047432 [details] [diff] [review]
0010-Add-pref-for-whether-to-accept-plaintext-protocols-d.patch

Frankly, I don't think this makes much sense. The warning that Thunderbird presents is already very very big. You get a dialog with red background (!). If the user accepts this, that is his choice.

There are valid configurations without SSL. E.g. my own mail server us on the LAN in the next room. With this pref set, you simply break these configs and leave the user alone with manual config.

> mailnews.auto_config.ssl_only_mail_servers

// Whether to allow non-SSL IMAP, POP3 and SMTP servers.
// If false, the user get a huge fat red warning for such servers,
// which explains the implications.
// If true, these servers are simply ignored and the user is dropped
// into manual config.
// It will prevent many local intranet servers (without SSL) to be found.
mailnews.auto_config.onlySSLMailServers

Comment 221

[Ben Bucksch (:BenB)]

Comment on attachment 9056143 [details] [diff] [review]
secure-account-creation.patch All patches in one file

Some of these patches need changes.

Jörg: If you land, please land these as *individual* patches. Each patch fixes a completely different aspect, and needs to be a separate commit. This is particularly important, because many of these are argurable and each aspect has a different tradeoff.

Comment 222

[Ben Bucksch (:BenB)]

@Jörg:
Patches with r+: You can land those.

@u:
Patches with r+: They can land as-is.
Patches with r- feedback+: The patch is mostly good, I accept the idea, but there are some minor code changes needed before they can land.
Patches with r- feedback-: Please drop those. They are a bad idea from Thunderbird perspective. You could keep them as local patches to TorBirdy.
Patches with r- and no feedback: I'm on the fence for these whether they are a good idea or should be dropped. And they need code changes, too.

Comment 223

[Jorg K (CEST = GMT+2)]

OK, I'll land the four r+ patches. In the future, please provide patches with a proper HG header. Also, these patches need manual massaging to apply, diff --git a/comm/mail/components/accountcreation/content/emailWizard.js ... is not correct, the patch/diff needs to be taken from the C-C repository, so there is no comm/. So next time, please supply then in an acceptable form.

Comment 224

[Jorg K (CEST = GMT+2)]

And of course splitting it all into small patches is a pain since, for example, part 8 doesn't apply if we don't use some of the earlier parts. So whilst in theory "pick and choose" is a nice idea, in practise it doesn't work, or someone else needs to do the work :-(

Comment 225

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/55dab518e79a
Invalidate config when restarting autoconfiguration. r=BenB
https://hg.mozilla.org/comm-central/rev/ee9a9a89e349
Add comment for pref mailnews.auto_config.guess.enabled. r=BenB
https://hg.mozilla.org/comm-central/rev/5196d1d1e81a
Add pref for setting the autoconfiguration guess timeout. r=BenB
https://hg.mozilla.org/comm-central/rev/b796a9af14c7
Improve logging of guess instances. r=BenB DONTBUILD

Comment 226

[Ben Bucksch (:BenB)]

@Jörg: Thanks for landing!

part 8 doesn't apply if we don't use some of the earlier parts. So whilst in theory "pick and choose" is a nice idea, in practise it doesn't work, or someone else needs to do the work :-(

Yes. If that happens, feel free to throw the patch back to u (patch author) to update it.

Comment 227

[Ben Bucksch (:BenB)]

@u: Could you please update patches:

• 0004 Also-fetch-ISP-configuration-using-SSL (which fixes the main focus of this bug)
• 0007 Add-SOCKS-proxy-support-for-account-guessing
  ?

Comment 228

[u]

@BenB : thank you for reviewing and merging some of these patches. We will work on them again soon, I will keep you posted once this is clear. (Do 4 weeks matter after the 5th anniversary of this bug? ;))

Comment 229

[Jorg K (CEST = GMT+2)]

Yes, since TB 68 goes to beta in about 10 days. Whatever lands before will be part of TB 68 beta and then TB 68 ESR. Whatever misses the date will wait for a year before it faces the users in TB 76 ESR.

Comment 230

[u]

Thanks, good to know!

Comment 231

[Ben Bucksch (:BenB)]

@u: If you can only revise the patches 0004 and 0007, we can get it in, and I think we can call this bug done. They address the original purpose of this bug. And the changes I requested are reasonably small. I'd be happy to bring this to conclusion.

Comment 232

[u]

@BenB: yes, we are trying to that in time for TB 68. I cannot yet promise we'll succeed.

Comment 233

[u]

Hi!

One notice before we go on: please don't change the author of the
patches. anonym and u are different people. (You're actually talking to the two of us at the same time :))

tl;dr: The two patches you requested have been updated to do exactly
what you asked for:

• 0001-Also-fetch-ISP-configuration-using-SSL.patch (was 0004)
• 0002-Add-SOCKS-proxy-support-for-account-guessing.patch (was 0007)

And we'd also urge you to consider
0003-Add-pref-for-guessing-only-SSL-configs.patch (see below for
context).

Comment 217:

Comment on attachment 9047429 [details] [diff] [review]
0007-Add-SOCKS-proxy-support-for-account-guessing.patch

Just a small code structure comment: Could you please implement a
async function getProxy()
or
function getProxy(resultCallback)
which returns the proxyInfo or null?

Ah, yes that makes a lot of sense! We went with the latter although
it's called doProxy() ("get" sounds like it should return the
proxyInfo, not pass it via the callback). If you have a better name
perhaps you can just change it yourself to avoid another patch
roundtrip (which we probably won't have time with before the freeze).

Comment 214:

Comment on attachment 9047426 [details] [diff] [review]
0004-Also-fetch-ISP-configuration-using-SSL.patch

[...]

+// When false an active attacker can block non-SSL fetches and then
+// MitM the HTTP fetch, granting the attacker full control over the
+// client configuration.

I understand your idea, but everybody is missing 2 important points:

We understand that when designing this protocol your top priority was
to maximize the chance of ending up with a working config. This is not
all users' priority. For some subsets of users (e.g. Tails' and
TorBirdy's) security is more important than config success. In fact,
successfully enabling an insecure config is probably the worst failure
mode imaginable for them. Please make room for this other perspective
when reading my comments below.

1. Using https only will violate the established protocol

Yes, the protocol is insecure by design, and this option breaks it, in
order to make it safe and thus usable for users that require
security.

and will not work in many cases.

Yes, we pay for the security by making insecure configs
unavailable. For users that require security, an insecure protocol
is unusable, so for them it works for no cases. Such users have no
better option than accepting a secure version of the protocol that
works for fewer cases -- those lost cases (i.e. the insecure ones) are
actually exactly the ones they absolutely want to avoid, so from their
perspective this is not a loss but a win.

I know it's insecure, but if you insist on https, you will simply
not find a config, plain and simple.

That is the exact, desired behavior for users that require security.

2. Even with this set to true, you are still suspect to a MITM, due
   to the guess config.

We mostly agree. You now seem to reject
0010-Add-pref-for-whether-to-accept-plaintext-protocols-d.patch, which
would fix this for those that enable the pref it added. You were
previously in favor of merging this patch (see comment 159) although
we agree to some extent that the changes to readFromXML.js are mostly
covered by the scary warning. So we have lifted only the parts about
forcing SSL when guessing into a third patch adding the guess.sslOnly
pref which will be go great together with fetchFromISP.sslOnly, as per
the updated comments in mailnews.js.

So, please also consider
0003-Add-pref-for-guessing-only-SSL-configs.patch.

Above we said "mostly agree" because we think that HTTP ISP fetch is a
lot worse. In that case the attacker can present a config using SSL
(i.e. there won't be a scary warning) but pointing to domains under the attacker's
control → game over for the user. In the guessing case the attacker
cannot present a configuration using SSL (unless the attacker can
break SSL for that domain somehow) so they are limited to plaintext
configs which results in the scary warning → the user should at least
be more suspicious.

Or it can be put like this: when guessing it is the user's client that
generates configs and tests them, so the attacker can only filter a lot
among these configs; with HTTP fetch, the attacker can create
arbitrary configs, which is fundamentally worse.

And you cannot disable the guess config,
because only 40-50% of the configs are in the DB and via ISP and the
rest needs to be guessed. Guessing often leads to success, so we
really need that. Internal email servers often have no SSL.

This is not true: for users that require security disabling insecure
features is a must. For them those 50-60% configs that can only be
obtained insecurely were never options to begin with, so your argument
doesn't apply to them.

However, with the third patch's guess.sslOnly pref set, this point is
completely moot since guessing SSL only makes it a safe method for
those users again. Yay!

Comment 234

[u]

Attachment: 0001-Also-fetch-ISP-configuration-using-SSL - Landed

Comment 235

[u]

Attachment: 0002-Add-SOCKS-proxy-support-for-account-guessing.patch

Comment 236

[u]

Attachment: 0003-Add-pref-for-guessing-only-SSL-configs - Landed

This is a new patch.

Comment 237

[Ben Bucksch (:BenB)]

Comment on attachment 9063503 [details] [diff] [review]
0002-Add-SOCKS-proxy-support-for-account-guessing.patch

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

I have 3 small comments. They would improve the code. But they do *not* block the landing of this patch.

r=BenB, with or without these changes

::: mail/components/accountcreation/content/guessConfig.js
@@ +436,4 @@
>        if (i == 0) // showing 50 servers at once is pointless
>          this.mProgressCallback(thisTry);
>  
> +      var proxyResultCallback = function(proxy) {

NIT: We write these as inline functions passed directly to the function call, no explicit function variable. However, this comment is made moot if the next comment is fixed.

@@ +455,5 @@
> +              thisTry.status = kFailed;
> +              me._checkFinished();
> +            });
> +      };
> +      doProxy(thisTry.hostname, proxyResultCallback);

Minor: As mentioned in the review 1 year ago, you're resolving the proxy for every round. That's dozens of times at the same time. Rather, the proxy should be resolved once, the nsIProxyInfo stored, and then re-used.

@@ +1087,5 @@
> +  var proxyService = Cc["@mozilla.org/network/protocol-proxy-service;1"]
> +                     .getService(Ci.nsIProtocolProxyService);
> +  // Use some arbitrary scheme just because it is required...
> +  var uri = Services.io.newURI("http://" + hostname, null, null);
> +  // ... we'll ignore it any way. We prefer SOCKS since that's the

NIT: If you ignore it anyway, then you might as well use "imap://" to be at least closer to reality.

Comment 238

[Ben Bucksch (:BenB)]

Comment on attachment 9063502 [details] [diff] [review]
0001-Also-fetch-ISP-configuration-using-SSL - Landed

Thanks for these changes!
r=BenB

Comment 239

[Ben Bucksch (:BenB)]

Comment on attachment 9063504 [details] [diff] [review]
0003-Add-pref-for-guessing-only-SSL-configs - Landed

I don't think this patch is very useful for normal Thunderbird users, but it shouldn't hurt in the current form, and I accept it to be nice to you and reduce the number of patches that you need to carry and adapt for new TB.

Thanks for the patience you had with us, and thanks for these nice patches! I really appreciated working with you.

Comment 240

[Jorg K (CEST = GMT+2)]

(In reply to u from comment #233)

One notice before we go on: please don't change the author of the
patches. anonym and u are different people. (You're actually talking to the two of us at the same time :))

Hmm, the patches that got landed all had "anonym <[e-mail]>" in the hg header. How are we supposed to know that the assignee of the bug, someone called "u" who also attached the patches, is in fact not the patch author? I've never seen a case like this before.

Comment 241

[Jorg K (CEST = GMT+2)]

Hold on, aren't we going to address comment #237?

Comment 242

[anonym]

Attachment: 0002-Add-SOCKS-proxy-support-for-account-guessing-fixup.patch

Comment 243

[anonym]

(In reply to Ben Bucksch (:BenB) from comment #237)

Comment on attachment 9063503 [details] [diff] [review]
0002-Add-SOCKS-proxy-support-for-account-guessing.patch

Review of attachment 9063503 [details] [diff] [review]:

[...]

Minor: As mentioned in the review 1 year ago, you're resolving the proxy for
every round. That's dozens of times at the same time. Rather, the proxy
should be resolved once, the nsIProxyInfo stored, and then re-used.

Fixed by fixup-0002-Add-SOCKS-proxy-support-for-account-guessing.patch. I have just done rudimentary testing, but it was a mechanical code transformation so I guess I'm confident. :)

@@ +1087,5 @@

• var proxyService = Cc["@mozilla.org/network/protocol-proxy-service;1"]

•                 .getService(Ci.nsIProtocolProxyService);
  

• // Use some arbitrary scheme just because it is required...
• var uri = Services.io.newURI("http://" + hostname, null, null);
• // ... we'll ignore it any way. We prefer SOCKS since that's the

NIT: If you ignore it anyway, then you might as well use "imap://" to be at
least closer to reality.

"imap://" triggers an internal error, that is why I picked "http://".

Comment 244

[anonym]

(In reply to Jorg K (GMT+2) from comment #240)

(In reply to u from comment #233)

One notice before we go on: please don't change the author of the
patches. anonym and u are different people. (You're actually talking to the two of us at the same time :))

Hmm, the patches that got landed all had "anonym <[e-mail]>" in the hg header. How are we supposed to know that the assignee of the bug, someone called "u" who also attached the patches, is in fact not the patch author? I've never seen a case like this before.

We are git-centric and assumed you would use something like git am which would import the meta-data from the patch. Sorry for the inconvenience!

Comment 245

[Ben Bucksch (:BenB)]

Comment on attachment 9063603 [details] [diff] [review]
0002-Add-SOCKS-proxy-support-for-account-guessing-fixup.patch

r=BenB

Thank you!

Comment 246

[Jorg K (CEST = GMT+2)]

Comment on attachment 9063603 [details] [diff] [review]
0002-Add-SOCKS-proxy-support-for-account-guessing-fixup.patch

This patch doesn't apply at all (plus the indentation is wrong in `doProxy()`. 001 and 003 have been a pain as well: Patch needed to me made to apply manually by correcting the incorrect paths. And frankly, whether you're GIT-centric or not doesn't matter, here we work with Mercurial and patches **must** apply and have a decent HG header :-(

Comment 247

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/36511bbb7713
Also fetch ISP configuration using SSL. r=BenB
https://hg.mozilla.org/comm-central/rev/8d4257424669
Add pref for guessing only SSL configs. r=BenB

Comment 248

[anonym]

Attachment: 0002-Add-SOCKS-proxy-support-for-account-guessing - Landed

Comment 249

[anonym]

(In reply to Jorg K (GMT+2) from comment #246)

Comment on attachment 9063603 [details] [diff] [review]
0002-Add-SOCKS-proxy-support-for-account-guessing-fixup.patch

This patch doesn't apply at all

Crap, I was unclear: fixup-0002 was supposed to be squashed into 0002 (that's why I didn't obsolete 0002). The 0002 I just attached is squashed already. Sorry for the inconvenience!

plus the indentation is wrong in doProxy().

I've removed one indentation level (i.e. two spaces) which I believe was the problem.

001 and 003 have been a pain as well: Patch needed to me made
to apply manually by correcting the incorrect paths. And frankly, whether
you're GIT-centric or not doesn't matter, here we work with Mercurial and
patches must apply and have a decent HG header :-(

Sorry about this!

I assume it's about "comm/" missing in the beginning of the paths? I don't even get that when using Mercurial to generate the patch so I had to manually edit the patch. I hope all is ok now, but I'm available on short notice to fix any remaining issues.

Comment 250

[Jorg K (CEST = GMT+2)]

You mean "merged" or "folded" when you say "squashed"? I tried applying the 002-fixup in top of the 002-original, but that didn't work either. Besides, the 002-original WAS made obsolete in comment #235.

No, the "comm/" MUST NOT be there since this is a patch for the comm-central repository which happens to reside in the "comm/" subdirectory of some other repository. I had to remove "comm/" from the patches. If you look at 001 and 003 you will see that "comm/" was inconsistently applied.

Never mind, I hope this will be the last manual comment in this bug.

Comment 251

[Ben Bucksch (:BenB)]

I had to remove "comm/" from the patches.

patch -p 2 or hg import -p 2

Comment 252

[Ben Bucksch (:BenB)]

diff --git a/mail/... b/comm/mail/...

Ah, I see what you mean.

Comment 253

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+2) from comment #250)

... I tried applying the 002-fixup in top of the 002-original, but that didn't work either.

That must have been my confusion, it worked now. The result is the final version attached here. Also, please always provide 8 lines of context in your patches using:

[diff]
git = 1
showfunc = 1
unified = 8

in your mercurial.ini.

Comment 254

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f40eb5ec3bfa
Add SOCKS proxy support for account guessing. r=BenB DONTBUILD

Comment 255

[Ben Bucksch (:BenB)]

Thanks to u and anonym@riseup for these good patches! And thanks for your patience and hanging in there over all this time.

Also thanks to Jörg for landing them.

Comment 256

[Jorg K (CEST = GMT+2)]

Landed with the following linting errors fixed.

  1085:5   error  Extra space after key 'onProxyAvailable'.  key-spacing (eslint)
  1085:5   error  Expected method shorthand.                 object-shorthand (eslint)
  1091:28  error  Missing semicolon.                         semi (eslint)
  1092:6   error  Missing trailing comma.                    comma-dangle (eslint)
  1097:13  error  newURI's last parameters are optional.     mozilla/no-useless-parameters (eslint)

Author and reviewer MUST always run linting before submitting or approving patches.

Just keep in mind that none of these patches would have been accepted for mozilla-central. Anything that doesn't cleanly apply or causes linting errors is rejected immediately. You're very lucky to have the (grumpy) comm-central sheriffing service which also serves as a clean-up maid for everyone :-(

Comment 257

[u]

Just keep in mind that none of these patches would have been accepted for mozilla-central. Anything that doesn't cleanly apply or causes linting errors is rejected immediately. You're very lucky to have the (grumpy) comm-central sheriffing service which also serves as a clean-up maid for everyone :-(

@jorgk: thank you very much! We very much appreciate your efforts :)

Comment 258

[u]

Unassigning myself.