1547096 - remove uses of nsIBadCertListener2 in comm-central

Status: RESOLVED FIXED

(MailNews Core :: General, task)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

nsIBadCertListener2 is going away. The last uses of it are in comm-central:

https://searchfox.org/comm-central/rev/01f50ef57ff126a8a8146f9091a44d3ff0b31d4e/calendar/base/modules/utils/calProviderUtils.jsm#172
https://searchfox.org/comm-central/rev/01f50ef57ff126a8a8146f9091a44d3ff0b31d4e/chat/modules/socket.jsm#492
https://searchfox.org/comm-central/rev/01f50ef57ff126a8a8146f9091a44d3ff0b31d4e/mail/base/content/mailWindow.js#621
https://searchfox.org/comm-central/rev/01f50ef57ff126a8a8146f9091a44d3ff0b31d4e/mail/components/accountcreation/content/MyBadCertHandler.js#12
https://searchfox.org/comm-central/rev/01f50ef57ff126a8a8146f9091a44d3ff0b31d4e/mail/components/accountcreation/content/verifyConfig.js#281
https://searchfox.org/comm-central/rev/01f50ef57ff126a8a8146f9091a44d3ff0b31d4e/suite/mailnews/content/mailTasksOverlay.js#212

Equivalent functionality can be achieved by checking the status of any channel/request that gets sent off. If the status is a failing code that is an overridable certificate error, the add certificate exception dialog can be shown in the same way it is today.

e.g. if you have an onStopRequest or onerror handler that gives you a request and a status code (or if you still have a handle on the original request/channel), you can do something like:

if (status != Cr.NS_OK) {
  let nssErrorsService = Cc["@mozilla.org/nss_errors_service;1"]
                           .getService(Ci.nsINSSErrorsService);
  let isCertError = false;
  try {
    let errorType = nssErrorsService.getErrorClass(status);
    if (errorType == nsINSSErrorsService.ERROR_CLASS_BAD_CERT) {
      isCertError = true;
    }
  } catch (e) {
    // nsINSSErrorsService.getErrorClass throws if given a non-TLS, non-cert error, so ignore this
  }

  if (isCertError && request.channel && request.channel.securityInfo) {
    let secInfo = req.channel.securityInfo.QueryInterface(Ci.nsITransportSecurityInfo);
    informUserOfCertError(secInfo.serverCert);
  }
}

Comment 1

[Jorg K (CEST = GMT+2)]

Dana, what's the ETA for the removal? Kai will start work again soon.

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Not before 68 branches, but ideally as soon as we can after that. This particularly affects some work the necko team is doing (socket process project).

Comment 3

[Kai Engert (:KaiE:)]

Attachment: test1.patch

(In reply to Dana Keeler (she/her) (use needinfo) (:keeler for reviews) from comment #0)

Equivalent functionality can be achieved by checking the status of any channel/request that gets sent off. If the status is a failing code that is an overridable certificate error, the add certificate exception dialog can be shown in the same way it is today.

e.g. if you have an onStopRequest or onerror handler that gives you a request and a status code (or if you still have a handle on the original request/channel), you can do something like:

I assume you're suggesting nsIRequestObserver.onStopRequest

I tried the attached patch, but with an interactive test, the onStopRequest and onStartRequest functions aren't reached. (nsIBadCertListener2.notifyCertProblem was reached.)

I guess we need to register the handlers differently?

Comment 4

[Kai Engert (:KaiE:)]

I couldn't find any code that was converted from nsIBadCertListener2.notifyCertProblem to use something else.

For example in bug 1531958, it appears you no longer test for bad cert events.

Comment 5

[Kai Engert (:KaiE:)]

(I meant: you no longer listen for bad cert events.)

Comment 6

[Kai Engert (:KaiE:)]

So currently, TB can have a single listener for notifyCertProblem in JS. Several of TB's network protocols are implemented in C++. I think we'll need to change each of the C++ProtocolImplementation::OnStopRequest methods to check for errors, and send our own notification from C++ to JS land.

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Here's one example of moving from notifyCertProblem to an onerror handler, but I'm not sure how applicable it will be to most of comm-central: https://hg.mozilla.org/mozilla-central/rev/494abd5b60b8#l3.22

Comment 8

[Magnus Melin [:mkmelin]]

A valid certificate problem that occurs all the sudden (for a known host) should be very uncommon for mail, I'd assume (in comparison to web servers).

I guess the "new" behaviour would be that the connection just fails?

An alternative, and more general approach to poking around in the protocol implementations could be to have a trouble shooter to help you find the issue for connection issues. Part of that would be checking certificates.

OTOH, from a quick look we might be able to just add needed logic to nsMsgProtocol::OnStopRequest (and some slight adjustments)

Comment 9

[Kai Engert (:KaiE:)]

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

A valid certificate problem that occurs all the sudden (for a known host) should be very uncommon for mail, I'd assume (in comparison to web servers).

Common scenarios are:

• server certificate expired because it hasn't been renewed in time
• captive portal at a hotel
• MITM attempt at a WiFi hotspot

I guess the "new" behaviour would be that the connection just fails?

You mean silently fails? I think the user should get informed when a connection fails. And just saying "connection failed" doesn't help the user to investigate the cause. If we want users to understand what's going on, or to get help, they need to be able to learn about the details of the failure. That's what that listener is about. Today, we can get a push notification from the core SSL/TLS communication to the JS user interface, which includes all the interesting details. The intended change removes this convenience. Instead, Thunderbird must identify all code locations in which SSL/TLS connections are performed, and setup appropriate failure handling, that queries those details from the core SSL/TLS.

An alternative, and more general approach to poking around in the protocol implementations could be to have a trouble shooter to help you find the issue for connection issues. Part of that would be checking certificates.

IMHO that isn't simpler. You'd require a central place in Thunderbird that keeps track of all the various connections, and which is able to iterate over all of them. For some protocols, performing the SSL/TLS handshake requires knowledge of the protocol. For example, with all STARTTLS variants, it's necessary to start with a protocol specific plaintext handshake, and only afterwards, if the initial conversation with the server has reached a certain point, in which the server has shown its ability, the socket can switch to start the SSL/TLS protocol.

IMHO, if we don't get a callback, adding the error handling into the protocol code is necessary. We don't need individual user interaction for each protocol. Each protocol implementation only needs the code to check for the kind of connection failure, and if it's found to be related to a bad certificate, call common Thunderbird code that handles SSL/TLS errors.

OTOH, from a quick look we might be able to just add needed logic to nsMsgProtocol::OnStopRequest (and some slight adjustments)

Thanks for this hint, I'll have a look.

Comment 10

[Kai Engert (:KaiE:)]

Note this isn't just related to mail messages. It's also about chat protocols, accessing calendar servers, RSS feeds, and any other network connections that Thunderbird might perform.

Comment 11

[Magnus Melin [:mkmelin]]

(In reply to Kai Engert (:kaie:) from comment #9)

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

A valid certificate problem that occurs all the sudden (for a known host) should be very uncommon for mail, I'd assume (in comparison to web servers).

Common scenarios are:

• server certificate expired because it hasn't been renewed in time
• captive portal at a hotel

Firefox has handling for this nowadays. I'm not sure how/if Thunderbird hooks into that, but we should.

• MITM attempt at a WiFi hotspot

Let's not help with that ;)

I guess the "new" behaviour would be that the connection just fails?

You mean silently fails?

Well, not silently but with a normal connection failed message, yeah.

IMHO that isn't simpler. You'd require a central place in Thunderbird that
keeps track of all the various connections,

We would only test the affected account. We're already doing much of that in account setup so most of the code should already exist to some degree.

Comment 12

[alta88]

(In reply to Kai Engert (:kaie:) from comment #10)

Note this isn't just related to mail messages. It's also about chat protocols, accessing calendar servers, RSS feeds, and any other network connections that Thunderbird might perform.

More granular detection and error messaging, along with easier user override UI was added to feeds in Bug 497488.

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Kai, any updates here?

Comment 14

[Kai Engert (:KaiE:)]

Not yet. I'll look into this now and try to push this forward.
Also adding BenC to CC.

Comment 15

[Kai Engert (:KaiE:)]

To unblock, we could potentially just remove the existing code that implements the bad cert handler interface, and leave that unfixed initially.

Users of TB nightly and beta who only connect to well maintained servers wouldn't be affected by that change.

Only users who have a need to connect o servers with bad certificates. Maybe it's acceptable to have that broken for a while and fix it one after the other?

(IIUC SeaMonkey hasn't been able to build in a longer time. We have to decide if we simply disable the respective code in the suite directory, or if we'd simply leave that code untouched. I guess we'd need a code review to touch the suite directory, so it suite doesn't work anyway, maybe it's best to not touch it.)

Comment 16

[Kai Engert (:KaiE:)]

I've filed 4 bugs to track the different functional areas, see the dependency list:

• chat
• calendar
• mail account wizard (bad certs during probing or account setup)
• mail reader window (bad certs after an account has already been configured)

Comment 17

[Kai Engert (:KaiE:)]

A first test showed that the above approach could work. Apparently none of the current Thunderbird tests require that overriding works, so removing the ability to override shouldn't break our tests:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5df88c6c4edd088b9861f61874926a5c4603bfa0

I've submitted a second try build with a cleaner removal patch for all platforms:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=28598e79c0371e1fef632bd1e7593924187d8768

Magnus, Jörg, any concerns about this suggestion? (Remove first, fix later, in the separate bugs.)

Comment 18

[Kai Engert (:KaiE:)]

(In reply to Kai Engert (:kaie:) from comment #17)

I've submitted a second try build with a cleaner removal patch for all platforms:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=28598e79c0371e1fef632bd1e7593924187d8768

Canceled most of above, and submitted an artifact try build instead:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8d4595986477681b613b1fcd91bb7713098a6439

Comment 19

[Kai Engert (:KaiE:)]

Attachment: 1547096-v2.patch

Comment 20

[Jorg K (CEST = GMT+2)]

Comment on attachment 9103468 [details] [diff] [review]
1547096-v2.patch

Looks OK to me if you fix the linting issue ;-) - I'm surprised Xpcshell works in artifact mode, it failed last I tried. Never mind the GTests, they won't be affected. Thanks for moving this forward.

Comment 21

[Kai Engert (:KaiE:)]

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

Looks OK to me if you fix the linting issue

That was a whitespace change to trigger try acceptance. It isn't in this patch, which was accepting by lint in
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=28598e79c0371e1fef632bd1e7593924187d8768

Comment 22

[Kai Engert (:KaiE:)]

Dana, how soon do you need this landing? I see you have a couple of other bugs still blocking the removal.

Comment 23

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Ideally as soon as possible. I think this is the only open bug blocking bug 1468230.

Comment 24

[Magnus Melin [:mkmelin]]

(In reply to Kai Engert (:kaie:) from comment #17)

Magnus, Jörg, any concerns about this suggestion? (Remove first, fix later, in the separate bugs.)

I think that's alright.

Comment 25

[Jorg K (CEST = GMT+2)]

I'll land it within the next 12 hours.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/dee2511f17fb
Remove use of nsIBadCertListener2 in comm-central. r=mkmelin