Closed Bug 954534 Opened 10 years ago Closed 10 years ago

Use Firefox untrusted cert dialog for "SSL Handshake failed" errors

Categories

(Chat Core :: General, defect)

defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bugzilla, Assigned: clokep)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 11 obsolete files)

17.97 KB, image/png
Details
18.19 KB, image/png
florian
: review+
Details
10.95 KB, patch
florian
: review+
Details | Diff | Splinter Review
7.74 KB, patch
florian
: review+
Details | Diff | Splinter Review
*** Original post on bio 1100 by Demian Marty <d.marty AT io-market.com> at 2011-10-21 06:19:00 UTC ***

Fehler: Couldn't look up SRV record. Der DNS-Name ist nicht vorhanden. (9003).

Quelldatei: http://hg.instantbird.org/instantbird/raw-file/73f480ee09a7/purple/libpurple/dnssrv.c
Zeile: 589
Quelltext:
dnssrv: res_main_thread_cb


Fehler: Handshake failed  (-8156)

Quelldatei: http://hg.instantbird.org/instantbird/raw-file/73f480ee09a7/purple/libpurple/ssl-nss.c
Zeile: 342
Quelltext:
nss: ssl_nss_handshake_cb


Resolved with replace of the file 'purple.dll' of the last release.
*** Original post on bio 1100 at 2011-10-21 10:17:58 UTC ***

Are you trying to connect to a XMPP with a self-signed certificate?
*** Original post on bio 1100 by Demian Marty <d.marty AT io-market.com> at 2011-10-21 10:53:09 UTC ***

Yes, I try to connect to our inhouse hosted server, and it has a self signed certificate.
Could I override the certificate warning somewhere in the config?
*** Original post on bio 1100 at 2011-10-21 12:08:12 UTC ***

The better solution is to import the certificate (or the certificate authority). You can do this in Preferences -> Advanced -> Encryption -> View Certificates by selecting the right tab (Server or Authority, usually) and clicking "Import".
*** Original post on bio 1100 at 2011-10-21 12:09:48 UTC ***

It seems a more verbose/helpful error message may be required for SSL handshake errors? (i.e. "these are some common causes, try this to fix")
*** Original post on bio 1100 at 2011-10-21 12:12:14 UTC ***

Oh, and afaik you can in fact turn off certificate checking: From Preferences -> Advanced -> General, go to the config editor and set purple.ssl.check_certificates to false.
*** Original post on bio 1100 at 2011-10-21 17:21:12 UTC ***

(In reply to comment #4)
> It seems a more verbose/helpful error message may be required for SSL handshake
> errors? (i.e. "these are some common causes, try this to fix")

Even better, clokep in IRC:

The real solution though is to pop up the "untrusted cert" dialogue that Firefox has.
*** Original post on bio 1100 at 2012-02-13 19:57:19 UTC ***

Confirming.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: XMPP protocol login Error: SSL Handshake failed → Use Firefox untrusted cert dialog for "SSL Handshake failed" errors
Blocks: 954548
*** Original post on bio 1100 by gpsychosis <gpsychosis AT gmail.com> at 2012-11-04 20:12:47 UTC ***

(In reply to comment #7)
> (In reply to comment #4)
> > It seems a more verbose/helpful error message may be required for SSL handshake
> > errors? (i.e. "these are some common causes, try this to fix")
> 
> Even better, clokep in IRC:
> 
> The real solution though is to pop up the "untrusted cert" dialogue that
> Firefox has.

I am interested in seeing this functionality implemented so I don't have to toggle all certificate checking to false.
*** Original post on bio 1100 at 2012-11-04 21:26:06 UTC ***

(In reply to comment #11)

> I am interested in seeing this functionality implemented so I don't have to
> toggle all certificate checking to false.

You can already add exceptions from the preference window (the UI for it is admittedly difficult to find though).
*** Original post on bio 1100 by gpsychosis <gpsychosis AT gmail.com> at 2012-11-04 22:26:35 UTC ***

In reply to comment #12)
> (In reply to comment #11)
> 
> > I am interested in seeing this functionality implemented so I don't have to
> > toggle all certificate checking to false.
> 
> You can already add exceptions from the preference window (the UI for it is
> admittedly difficult to find though).

I am also experiencing issues with one particular server that doesn't even have a certificate at all, and exceptions I set in preferences (on top of having toggled the global certificate check to false) need to be deleted and re-set at regular intervals.

The message I get from this server is: "Error: The certificate issuer's certificate has expired. Check your system date and time."

Receiving a prompt when this happens and/or finding a way to permanently use SSL for one server would make this a whole lot less tedious.
*** Original post on bio 1100 at 2012-12-20 22:19:04 UTC ***

Playing with this a little bit, it need to implement nsIBadCertListener2 in a better way, see [1]. This can then bring up the Necko prompt about cert handling as in [2].

I ran the following and the error console and I got a pretty prompt that mostly does what we want:
openDialog('chrome://pippki/content/exceptionDialog.xul','','chrome,centerscreen,modal', {exceptionAdded: false, prefetchCert: true, location: "http://google.com"})

I'm unsure if we would need to reconnect afterward, etc. Do we have a public test case I can try against?

[1] http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindow.js#507
[2] http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindow.js#541
Attached patch WIP, chat/ part v1 (obsolete) — Splinter Review
*** Original post on bio 1100 as attmnt 2215 at 2013-02-01 01:35:00 UTC ***

This is the chat/ and purple/ parts of this patch, the purple part almost definitely needs some work, but this worked for an XMPP account. We'll definitely want to abstract this a bit, but I wanted some feedback!
Attachment #8353978 - Flags: review?(florian)
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Attached patch Patch, instantbird/ part v1 (obsolete) — Splinter Review
*** Original post on bio 1100 as attmnt 2216 at 2013-02-01 01:38:00 UTC ***

Instantbird part, adds an extra button to the account manager that opens up the cert dialog.
Attachment #8353979 - Flags: review?(florian)
Attached image Account manager with cert error (obsolete) —
*** Original post on bio 1100 as attmnt 2217 at 2013-02-01 01:42:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
*** Original post on bio 1100 as attmnt 2218 at 2013-02-01 01:43:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
*** Original post on bio 1100 at 2013-02-01 13:30:00 UTC ***

Shouldn't onBadCertificate be in jsProtoHelper?

I doubt the patch you uploaded corresponds to the screenshot (eg the string for the button is wrong)?

For the UI, maybe there should be an equally prominent button to "Add certificate" that opens the relevant pane of Preferences? Otherwise many users will probably assume adding an exception is what they should do and all they can do. Of course this is tricky as a growing number of buttons isn't ideal either. (I'm tempted to say "account.xml needs redesigning anyway" but we all know that already ;)
*** Original post on bio 1100 at 2013-02-01 13:37:54 UTC ***

(In reply to comment #20)
> Shouldn't onBadCertificate be in jsProtoHelper?
(In reply to comment #16)
> Created attachment 8353978 [details] [diff] [review] (bio-attmnt 2215) [details]
> WIP, chat/ part v1
> We'll definitely want to abstract this a bit, but I wanted some feedback!

> I doubt the patch you uploaded corresponds to the screenshot (eg the string for
> the button is wrong)?
It does, exactly. No differences in between. Why is the string "wrong"?
> 
> For the UI, maybe there should be an equally prominent button to "Add
> certificate" that opens the relevant pane of Preferences? Otherwise many users
> will probably assume adding an exception is what they should do and all they
> can do.

Firefox doesn't give that option. Isn't it what they should do (assuming they still want to connect)? Why is adding a certificate "better" than adding an exception?
*** Original post on bio 1100 at 2013-02-01 13:49:43 UTC ***

(In reply to comment #21)
> It does, exactly. No differences in between. Why is the string "wrong"?
(As it turns out (via IRC) it was the changes to the dtd file that were obsolete)

> > For the UI, maybe there should be an equally prominent button to "Add
> > certificate" that opens the relevant pane of Preferences? Otherwise many users
> > will probably assume adding an exception is what they should do and all they
> > can do.
> 
> Firefox doesn't give that option. Isn't it what they should do (assuming they
> still want to connect)? Why is adding a certificate "better" than adding an
> exception?

It was just my impression from people on #instantbird facing cert problems that usually the right thing to do for them was to import a missing cert. You're right that for a browser, this is not something you would prefer a user to do, for IM I'm not sure.
Comment on attachment 8353979 [details] [diff] [review]
Patch, instantbird/ part v1

*** Original change on bio 1100 attmnt 2216 at 2013-02-12 17:31:23 UTC ***

Feedback from IRC:
 - Don't add another button to the account dialog, add a link at the end of the error that clearly shows it is related.
Attachment #8353979 - Flags: review?(florian) → review+
Comment on attachment 8353978 [details] [diff] [review]
WIP, chat/ part v1

*** Original change on bio 1100 attmnt 2215 at 2013-02-12 17:32:38 UTC ***

Feedback from IRC:
 - Abstract code to jsProtoHelper.
 - The information required isn't available from libpurple, so ignore this feature for it.
Attachment #8353978 - Flags: review?(florian) → review+
*** Original post on bio 1100 as attmnt 2228 at 2013-02-14 23:38:00 UTC ***

I'd like a UI review  on this...it doesn't look great, but I'm unsure exactly how to tweak it...
Attachment #8353991 - Flags: review?(florian)
Comment on attachment 8353991 [details]
Account manager with cert error as link

*** Original change on bio 1100 attmnt 2228 at 2013-02-15 08:23:54 UTC ***

While I agree that this doesn't really look great, I don't have any idea of what to improve, and I think it's already good enough for what you really want to achieve here. If someone has ideas later to polish it further, it's always possible to handle that in follow-up bugs :-).

Thanks for working on this long standing issue by the way! :-)
Attachment #8353991 - Flags: review?(florian) → review+
Attached patch Patch, instantbird/ part v2 (obsolete) — Splinter Review
*** Original post on bio 1100 as attmnt 2229 at 2013-02-15 11:38:00 UTC ***

Here's the patch for it being a link. I still need to work on the backend patch a bit.
Attachment #8353992 - Flags: review?(florian)
Comment on attachment 8353979 [details] [diff] [review]
Patch, instantbird/ part v1

*** Original change on bio 1100 attmnt 2216 at 2013-02-15 11:38:03 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353979 - Attachment is obsolete: true
Comment on attachment 8353980 [details]
Account manager with cert error

*** Original change on bio 1100 attmnt 2217 at 2013-02-15 11:38:03 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353980 - Attachment is obsolete: true
*** Original post on bio 1100 at 2013-02-15 15:23:11 UTC ***

(In reply to comment #27)
> Created attachment 8353992 [details] [diff] [review] (bio-attmnt 2229) [details]
> Patch, instantbird/ part v2
> 
> Here's the patch for it being a link. I still need to work on the backend patch
> a bit.

There's a "Components.utils.reportError("FOO");" in the "accounts.js" part that needs to be removed.
Attached patch Patch, chat/ part v2 (obsolete) — Splinter Review
*** Original post on bio 1100 as attmnt 2232 at 2013-02-16 02:36:00 UTC ***

The IRC part is under tested, I don't have a server to test against.
Attachment #8353995 - Flags: review?(florian)
Comment on attachment 8353978 [details] [diff] [review]
WIP, chat/ part v1

*** Original change on bio 1100 attmnt 2215 at 2013-02-16 02:36:13 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353978 - Attachment is obsolete: true
Comment on attachment 8353992 [details] [diff] [review]
Patch, instantbird/ part v2

*** Original change on bio 1100 attmnt 2229 at 2013-03-12 22:30:01 UTC ***


>+            <xul:label class="addException text-link"
>+                       onclick="gAccountManager.addException()"
>+                       value="&certmgr.addException.label;"/>

There's <!ENTITY certmgr.addException.accesskey       "x"> in certManager.dtd. Is there a reason for not using it? (maybe label don't take accesskeys?)

>--- a/instantbird/content/accounts.css
>+++ b/instantbird/content/accounts.css
>@@ -19,13 +19,15 @@ richlistitem:not([state="disconnected"])
> richlistitem:not([state="disconnecting"]) .disconnecting,
> richlistitem:not([error="true"]) .error,
> richlistitem:not([error="true"]) .errorIcon,
> richlistitem:not([state="disconnected"]) .error,
> richlistitem[error="true"] .disconnected,
> richlistitem[selected="true"]:not([state="disconnected"]) .connectButton,
> richlistitem[selected="true"][state="disconnected"] .disconnectButton,
> richlistitem[selected="true"][state="disconnecting"] .disconnectButton,
>+richlistitem[selected="true"]:not([certError="true"]) .addException,
>+richlistitem:not([selected="true"]) .addException,
> richlistitem:not([selected="true"]) .autoSignOn,
> richlistitem:not([reconnectPending="true"]) description[anonid="reconnect"]

So many descendant selectors :(. I'm not going to ask you to cleanup this mess here thought :).

>diff --git a/instantbird/content/accounts.js b/instantbird/content/accounts.js

>+  addException: function am_addException() {
>+    Components.utils.reportError("FOO");

Remove.


>--- a/instantbird/locales/en-US/chrome/instantbird/accounts.dtd
>+++ b/instantbird/locales/en-US/chrome/instantbird/accounts.dtd

I think you intended to revert the changes to this file.

Looks good otherwise, but I haven't tested it.
Attachment #8353992 - Flags: review?(florian) → review-
Comment on attachment 8353995 [details] [diff] [review]
Patch, chat/ part v2

*** Original change on bio 1100 attmnt 2232 at 2013-03-12 23:08:38 UTC ***

>diff --git a/chat/modules/jsProtoHelper.jsm b/chat/modules/jsProtoHelper.jsm

>+  handleBadCertificate: function(aSocket, aStatus) {
>+    let error = Ci.prplIAccount.ERROR_CERT_OTHER_ERROR;
>+    if (!aStatus)
>+      error = Ci.prplIAccount.ERROR_CERT_NOT_PROVIDED;
>+    else if (aStatus.isUntrusted) {
>+      if (aStatus.serverCert.isSelfSigned)
>+        error = Ci.prplIAccount.ERROR_CERT_SELF_SIGNED;
>+      else
>+        error = Ci.prplIAccount.ERROR_CERT_UNTRUSTED;
>+    }
>+    else if (aStatus.isNotValidAtThisTime)
>+      error = Ci.prplIAccount.ERROR_CERT_EXPIRED;
>+    else if (aStatus.isDomainMismatch)
>+      error = Ci.prplIAccount.ERROR_CERT_HOSTNAME_MISMATCH;
>+    // XXX ERROR_CERT_NOT_ACTIVATED
>+    // XXX ERROR_CERT_FINGERPRINT_MISMATCH
>+
>+    Components.utils.reportError(aSocket + " " + aSocket.targetSite);

You meant to remove this, didn't you?

>diff --git a/chat/modules/socket.jsm b/chat/modules/socket.jsm

>       let nssErrorsService =
>         Cc["@mozilla.org/nss_errors_service;1"].getService(Ci.nsINSSErrorsService);
>-      if (aStatus <= nssErrorsService.getXPCOMFromNSSError(nssErrorsService.NSS_SEC_ERROR_BASE) &&
>-          aStatus >= nssErrorsService.getXPCOMFromNSSError(nssErrorsService.NSS_SEC_ERROR_LIMIT - 1)) {
>-        this.onBadCertificate(nssErrorsService.getErrorMessage(aStatus));
>+      if ((aStatus <= nssErrorsService.getXPCOMFromNSSError(nssErrorsService.NSS_SEC_ERROR_BASE) &&
>+           aStatus >= nssErrorsService.getXPCOMFromNSSError(nssErrorsService.NSS_SEC_ERROR_LIMIT - 1)) ||
>+          (aStatus <= nssErrorsService.getXPCOMFromNSSError(nssErrorsService.NSS_SSL_ERROR_BASE) &&
>+           aStatus >= nssErrorsService.getXPCOMFromNSSError(nssErrorsService.NSS_SSL_ERROR_LIMIT - 1))) {
>+        this.onBadCertificate(aStatus, nssErrorsService.getErrorMessage(aStatus));

If nssErrorsService.getErrorClass(aStatus) is nssErrorsService.ERROR_CLASS_SSL_PROTOCOL, I think we want to disconnect the account with Ci.prplIAccount.ERROR_ENCRYPTION_ERROR (which will cause reconnection timers to be fired).

Note: I may be wrong here, as all the interfaces related to NSS are a bit confusing.

We should figure out how to handle broken certificates for prpls using http.jsm and do it for Twitter, but you don't have to do it here. The r- is because of the 2 above comments only.

I haven't tested this at all, but it looks reasonable otherwise.
Attachment #8353995 - Flags: review?(florian)
Attachment #8353995 - Flags: review-
Attachment #8353995 - Flags: feedback+
Attached patch Patch, chat/ part v3 (obsolete) — Splinter Review
*** Original post on bio 1100 as attmnt 2270 at 2013-03-13 01:58:00 UTC ***

(In reply to comment #31)
> Comment on attachment 8353995 [details] [diff] [review] (bio-attmnt 2232) [details]
> Patch, chat/ part v2
> 
> >diff --git a/chat/modules/socket.jsm b/chat/modules/socket.jsm
> If nssErrorsService.getErrorClass(aStatus) is
> nssErrorsService.ERROR_CLASS_SSL_PROTOCOL, I think we want to disconnect the
> account with Ci.prplIAccount.ERROR_ENCRYPTION_ERROR (which will cause
> reconnection timers to be fired).
I added this check into jsProtoHelper.jsm, not socket.jsm since socket.jsm doesn't actually deal with accounts at all.

> Note: I may be wrong here, as all the interfaces related to NSS are a bit
> confusing.
I have no way to test this, but it seems like the right thing to do.
Attachment #8354035 - Flags: review?(florian)
Comment on attachment 8353995 [details] [diff] [review]
Patch, chat/ part v2

*** Original change on bio 1100 attmnt 2232 at 2013-03-13 01:58:53 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353995 - Attachment is obsolete: true
Attached patch Patch, instantbird/ part v3 (obsolete) — Splinter Review
*** Original post on bio 1100 as attmnt 2271 at 2013-03-13 02:01:00 UTC ***

(In reply to comment #30)
> Comment on attachment 8353992 [details] [diff] [review] (bio-attmnt 2229) [details]
> Patch, instantbird/ part v2
> 
> 
> >+            <xul:label class="addException text-link"
> >+                       onclick="gAccountManager.addException()"
> >+                       value="&certmgr.addException.label;"/>
> 
> There's <!ENTITY certmgr.addException.accesskey       "x"> in certManager.dtd.
> Is there a reason for not using it? (maybe label don't take accesskeys?)
Apparently labels can use an accesskey (according to MDN), but after adding it...it doesn't seem to do anything. I have a feeling the account binding is stealing my key presses? I've left it in this patch.
Attachment #8354036 - Flags: review?(florian)
Comment on attachment 8353992 [details] [diff] [review]
Patch, instantbird/ part v2

*** Original change on bio 1100 attmnt 2229 at 2013-03-13 02:01:13 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353992 - Attachment is obsolete: true
Comment on attachment 8354036 [details] [diff] [review]
Patch, instantbird/ part v3

*** Original change on bio 1100 attmnt 2271 at 2013-03-13 09:42:02 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354036 - Flags: review?(florian) → review+
Comment on attachment 8354035 [details] [diff] [review]
Patch, chat/ part v3

*** Original change on bio 1100 attmnt 2270 at 2013-03-13 10:12:56 UTC ***

Would it be more readable to write the code this way?
  handleBadCertificate: function(aSocket, aStatus) {
    if (aSocket.targetSite)
      this._connectionTarget = aSocket.targetSite;

    if (!aStatus)
      return Ci.prplIAccount.ERROR_CERT_NOT_PROVIDED;

    if (aStatus.isUntrusted) {
      if (aStatus.serverCert.isSelfSigned)
        return Ci.prplIAccount.ERROR_CERT_SELF_SIGNED;
      else
        return Ci.prplIAccount.ERROR_CERT_UNTRUSTED;
    }

    if (aStatus.isNotValidAtThisTime)
      return Ci.prplIAccount.ERROR_CERT_EXPIRED;

    if (aStatus.isDomainMismatch)
      return Ci.prplIAccount.ERROR_CERT_HOSTNAME_MISMATCH;

    let nssErrorsService = Cc["@mozilla.org/nss_errors_service;1"]
                             .getService(Ci.nsINSSErrorsService);
    if (nssErrorsService.getErrorClass(aStatus) ==
        nssErrorsService.ERROR_CLASS_SSL_PROTOCOL) {
      error = Ci.prplIAccount.ERROR_ENCRYPTION_ERROR;
    }

    // XXX ERROR_CERT_NOT_ACTIVATED
    // XXX ERROR_CERT_FINGERPRINT_MISMATCH

    return Ci.prplIAccount.ERROR_CERT_OTHER_ERROR;
  },



onBadCertificate could have a third aIsSSLError argument with the value nssErrorsService.getErrorClass(aStatus) == nssErrorsService.ERROR_CLASS_SSL_PROTOCOL
That would avoid doing a second Cc["@mozilla.org/nss_errors_service;1"].getService(Ci.nsINSSErrorsService) in jsProtoHelper.


Except if I'm missing something, the aStatus value in onStopRequest is an nsresult value, not an nsISSLStatus instance. (This comment is the reason for the r-, the previous two are just nits.)

The only place in our code when I see an nsISSLStatus instance is the aStatus parameter of notifyCertProblem (I may have missed others).
Attachment #8354035 - Flags: review?(florian) → review-
Attached patch Patch, chat/ part v4 (obsolete) — Splinter Review
*** Original post on bio 1100 as attmnt 2285 at 2013-03-16 18:01:00 UTC ***

> Except if I'm missing something, the aStatus value in onStopRequest is an
> nsresult value, not an nsISSLStatus instance. (This comment is the reason for
> the r-, the previous two are just nits.)

This is a pretty major flaw, you're right. I've refactored the code a bit to handle this properly.
Attachment #8354050 - Flags: review?(florian)
Comment on attachment 8354035 [details] [diff] [review]
Patch, chat/ part v3

*** Original change on bio 1100 attmnt 2270 at 2013-03-16 18:01:33 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354035 - Attachment is obsolete: true
Comment on attachment 8354050 [details] [diff] [review]
Patch, chat/ part v4

*** Original change on bio 1100 attmnt 2285 at 2013-03-19 23:19:11 UTC ***

>diff --git a/chat/modules/jsProtoHelper.jsm b/chat/modules/jsProtoHelper.jsm

>+  handleBadCertificate: function(aSocket, aIsSslError) {
>+    if (aSocket.targetSite)
>+      this._connectionTarget = aSocket.targetSite;
>+
>+    if (aIsSslError)
>+      return Ci.prplIAccount.ERROR_ENCRYPTION_ERROR;
>+

Nit: Adding |let sslStatus = aSocket.sslStatus;| here will reduce some duplication:

>+    if (!aSocket.sslStatus)
>+      return Ci.prplIAccount.ERROR_CERT_NOT_PROVIDED;
>+
>+    if (aSocket.sslStatus.isUntrusted) {
>+      if (aSocket.sslStatus.serverCert.QueryInterface(Ci.nsIX509Cert3)
>+                 .isSelfSigned)
>+        return Ci.prplIAccount.ERROR_CERT_SELF_SIGNED;
>+      else
>+        return Ci.prplIAccount.ERROR_CERT_UNTRUSTED;
>+    }
>+
>+    if (aSocket.sslStatus.isNotValidAtThisTime)
>+      return Ci.prplIAccount.ERROR_CERT_EXPIRED;
>+
>+    if (aSocket.sslStatus.isDomainMismatch)
>+      return Ci.prplIAccount.ERROR_CERT_HOSTNAME_MISMATCH;

|aSocket.sslStatus| is present 5 times in the current patch.

>diff --git a/chat/modules/socket.jsm b/chat/modules/socket.jsm

>  * Users should "subclass" this object, i.e. set their .__proto__ to be it. And
>  * then implement:
>  *   onConnection()
>  *   onConnectionHeard()
>  *   onConnectionTimedOut()
>  *   onConnectionReset()
>- *   onBadCertificate(AString aNSSErrorMessage)
>+ *   onBadCertificate(nsISSLStatus aStatus, AString aNSSErrorMessage)

The first parameters seems to now be:
boolean aIsSslError

>diff --git a/chat/protocols/irc/irc.js b/chat/protocols/irc/irc.js

>-  onBadCertificate: function(aNSSErrorMessage) {
>+  onBadCertificate: function(aStatus, aNSSErrorMessage) {
>     this.ERROR("bad certificate: " + aNSSErrorMessage);
>-    this._account.gotDisconnected(Ci.prplIAccount.ERROR_CERT_OTHER_ERROR,
>-                                  aNSSErrorMessage);
>+    let error = this._account.handleBadCertificate(this, aStatus);

Rename aStatus to aIsSslError here to reduce the possible confusion when reading this code.

>diff --git a/chat/protocols/xmpp/xmpp-session.jsm b/chat/protocols/xmpp/xmpp-session.jsm

>-  onBadCertificate: function(aNSSErrorMessage) {
>-    this.onError(Ci.prplIAccount.ERROR_CERT_OTHER_ERROR, aNSSErrorMessage);
>+  onBadCertificate: function(aStatus, aNSSErrorMessage) {
>+    let error = this._account.handleBadCertificate(this, aStatus);

Same here.

I still haven't tried this, but I have good hopes that the next update of the patch will be ready to go :).
Attachment #8354050 - Flags: review?(florian) → review-
Attached patch Patch, chat/ part v5 (obsolete) — Splinter Review
*** Original post on bio 1100 as attmnt 2290 at 2013-03-19 23:29:00 UTC ***

This also adds comments to socket.jsm about what targetSite and sslStatus are.
Attachment #8354055 - Flags: review?(florian)
Comment on attachment 8354050 [details] [diff] [review]
Patch, chat/ part v4

*** Original change on bio 1100 attmnt 2285 at 2013-03-19 23:29:54 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354050 - Attachment is obsolete: true
Attached patch Patch, chat/ part v6 (obsolete) — Splinter Review
*** Original post on bio 1100 as attmnt 2291 at 2013-03-19 23:46:00 UTC ***

Fixes some nits from IRC.
Attachment #8354056 - Flags: review?(florian)
Comment on attachment 8354055 [details] [diff] [review]
Patch, chat/ part v5

*** Original change on bio 1100 attmnt 2290 at 2013-03-19 23:46:24 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354055 - Attachment is obsolete: true
Attachment #8354055 - Flags: review?(florian)
Comment on attachment 8354056 [details] [diff] [review]
Patch, chat/ part v6

*** Original change on bio 1100 attmnt 2291 at 2013-03-20 23:02:00 UTC ***

Figured it'd be good to get another set of eyes on this!
Attachment #8354056 - Flags: feedback?(bugzilla)
Comment on attachment 8354056 [details] [diff] [review]
Patch, chat/ part v6

*** Original change on bio 1100 attmnt 2291 by mook.moz+bugs.instantbird AT gmail.com at 2013-03-21 05:06:06 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354056 - Flags: feedback?(bugzilla) → feedback+
*** Original post on bio 1100 by Mook <mook.moz+bugs.instantbird AT gmail.com> at 2013-03-21 05:06:06 UTC ***

Comment on attachment 8354056 [details] [diff] [review] (bio-attmnt 2291)
Patch, chat/ part v6

For testing IRC, you can just connect to concrete.mozilla.org port 6697 - it's
just the name for irc.mozilla.org that isn't in the certificate :)  (Similar
tricks for freenode etc. probably works too.)

>Bug 954534 (bio 1100) - Use Firefox untrusted cert dialog for "SSL Handshake failed" errors: chat/ part, r=fqueze.
I'm going to guess this is actually "toolkit", not "firefox"?

I think I'm gonna need to read the UI bits too to make sense of this...

>diff --git a/chat/components/public/imIAccount.idl b/chat/components/public/imIAccount.idl

>+  /* When a certificate error occurs, the target to get the certificate from.
>+     This is only valid when connectionErrorReason is one of ERROR_CERT_*. */
>+  readonly attribute AUTF8String connectionTarget;
What does "target" mean here?  (Reading this IDL, I wouldn't know how to get the
cert given this information.)  An example might be useful, "www.example.com:443".
Explicitly state that this may be empty or whatever (for libpurple prpls)

>diff --git a/chat/modules/jsProtoHelper.jsm b/chat/modules/jsProtoHelper.jsm

>+  handleBadCertificate: function(aSocket, aIsSslError) {
API style question: would it be better to make aIsSslError something more like
an enum?  I guess it doesn't really work in JS... :|

>+    if (sslStatus.isUntrusted) {
>+      if (sslStatus.serverCert.QueryInterface(Ci.nsIX509Cert3).isSelfSigned)
I'd be defensive and use if (sslStatus.serverCert instanceof Ci.nsIX509Cert3 && sslStatus.serverCert.isSelfSigned)

>+    if (sslStatus.isNotValidAtThisTime)
>+      return Ci.prplIAccount.ERROR_CERT_EXPIRED;
>+
>+    if (sslStatus.isDomainMismatch)
>+      return Ci.prplIAccount.ERROR_CERT_HOSTNAME_MISMATCH;
>+
>+    // XXX ERROR_CERT_NOT_ACTIVATED
This is probably also isNotValidAtThisTime, looking at
sslStatus.serverCert.validity.notBefore < Date.now() * 1000

>diff --git a/chat/modules/socket.jsm b/chat/modules/socket.jsm

>  * Users should "subclass" this object, i.e. set their .__proto__ to be it. And
>  * then implement:
Unrelated comment: Object.create!

>@@ -400,36 +407,48 @@ const Socket = {
>     delete this.isConnected;
>     if (aStatus == NS_ERROR_NET_RESET)
>       this.onConnectionReset();
>     else if (aStatus == NS_ERROR_NET_TIMEOUT)
>       this.onConnectionTimedOut();
>     else if (aStatus) {
    else if (!Components.isSuccessCode(aStatus)) {

>+      if ((aStatus <= nssErrorsService.getXPCOMFromNSSError(nssErrorsService.NSS_SEC_ERROR_BASE) &&
>+           aStatus >= nssErrorsService.getXPCOMFromNSSError(nssErrorsService.NSS_SEC_ERROR_LIMIT - 1)) ||
>+          (aStatus <= nssErrorsService.getXPCOMFromNSSError(nssErrorsService.NSS_SSL_ERROR_BASE) &&
>+           aStatus >= nssErrorsService.getXPCOMFromNSSError(nssErrorsService.NSS_SSL_ERROR_LIMIT - 1))) {
I think it's clearer to port the correct C++ macros to JS:
    const NS_ERROR_MODULE_BASE_OFFSET = 0x45;
    const NS_ERROR_MODULE_SECURITY = 21;
    const NS_ERROR_GET_MODULE = function(err) ((err >> 16) - NS_ERROR_MODULE_BASE_OFFSET) & 0x1fff;
    if (NS_ERROR_GET_MODULE(aStatus) == NS_ERROR_MODULE_SECURITY) { ...

>+        this.onBadCertificate(nssErrorsService.getErrorClass(aStatus) ==
>+                              nssErrorsService.ERROR_CLASS_SSL_PROTOCOL,
>+                              nssErrorsService.getErrorMessage(aStatus));
Is it useful to pass in aStatus so it has a chance of doing more interesting things?
(e.g. suppress specific errors errors)

>+  notifyCertProblem: function(aSocketInfo, aStatus, aTargetSite) {
>+    this.targetSite = aTargetSite;
>+    this.sslStatus = aStatus;
Does this just kinda hang around forever (until the next problem)?

>diff --git a/chat/protocols/irc/irc.js b/chat/protocols/irc/irc.js

>+  onBadCertificate: function(aIsSslError, aNSSErrorMessage) {
>     this.ERROR("bad certificate: " + aNSSErrorMessage);
probably want to print out this.targetSite too; also, this message is incorrect
if aIsSslError.

>diff --git a/purple/purplexpcom/src/purpleAccount.cpp b/purple/purplexpcom/src/purpleAccount.cpp

>+NS_IMETHODIMP purpleAccount::GetConnectionTarget(nsACString & aConnectionTarget)
>+{
>+  aConnectionTarget = "";
You want either aConnectionTarget.Truncate(); or (I think it's better)
aConnectionTarget.SetIsVoid(true);
*** Original post on bio 1100 by Mook <mook.moz+bugs.instantbird AT gmail.com> at 2013-03-21 05:18:32 UTC ***

Having skimmed the UI part:
- This dialog actually comes from PSM, not toolkit.  Yay pedantry.
- The add exception UI should probably not exist for libpurple prpls, since you can't get hostname / port from it.
- Having the Connect button and the Add Exception link look so different is strange; they're equivalent-class actions in my mind.

I'd say something encouraging about how the patch is awesome, but I currently don't need to connect to any broken-SSL IM/chat servers :)
Attached patch Patch, chat/ part v7 (obsolete) — Splinter Review
*** Original post on bio 1100 as attmnt 2296 at 2013-03-22 21:15:00 UTC ***

(In reply to comment #40)
> For testing IRC, you can just connect to concrete.mozilla.org port 6697 - it's
> just the name for irc.mozilla.org that isn't in the certificate :)  (Similar
> tricks for freenode etc. probably works too.)
Thanks! :)

> >diff --git a/chat/modules/jsProtoHelper.jsm b/chat/modules/jsProtoHelper.jsm
> 
> >+  handleBadCertificate: function(aSocket, aIsSslError) {
> API style question: would it be better to make aIsSslError something more like
> an enum?  I guess it doesn't really work in JS... :|
I don't think this makes sense currently.

> >+      if ((aStatus <= nssErrorsService.getXPCOMFromNSSError(nssErrorsService.NSS_SEC_ERROR_BASE) &&
> >+           aStatus >= nssErrorsService.getXPCOMFromNSSError(nssErrorsService.NSS_SEC_ERROR_LIMIT - 1)) ||
> >+          (aStatus <= nssErrorsService.getXPCOMFromNSSError(nssErrorsService.NSS_SSL_ERROR_BASE) &&
> >+           aStatus >= nssErrorsService.getXPCOMFromNSSError(nssErrorsService.NSS_SSL_ERROR_LIMIT - 1))) {
> I think it's clearer to port the correct C++ macros to JS:
>     const NS_ERROR_MODULE_BASE_OFFSET = 0x45;
>     const NS_ERROR_MODULE_SECURITY = 21;
>     const NS_ERROR_GET_MODULE = function(err) ((err >> 16) -
> NS_ERROR_MODULE_BASE_OFFSET) & 0x1fff;
>     if (NS_ERROR_GET_MODULE(aStatus) == NS_ERROR_MODULE_SECURITY) { ...
I think Florian and I agree that this is more confusing.

> >+        this.onBadCertificate(nssErrorsService.getErrorClass(aStatus) ==
> >+                              nssErrorsService.ERROR_CLASS_SSL_PROTOCOL,
> >+                              nssErrorsService.getErrorMessage(aStatus));
> Is it useful to pass in aStatus so it has a chance of doing more interesting
> things?
> (e.g. suppress specific errors errors)
It could be, but I don't want to complicate things unnecessarily. I think this would be a good idea if we need the extra information.

> >+  notifyCertProblem: function(aSocketInfo, aStatus, aTargetSite) {
> >+    this.targetSite = aTargetSite;
> >+    this.sslStatus = aStatus;
> Does this just kinda hang around forever (until the next problem)?
Yes, do you think this would be an issue? Maybe we should clear it on connect.

The rest of your comments I agree with, thanks for the review!
Attachment #8354061 - Flags: review?(florian)
Comment on attachment 8354056 [details] [diff] [review]
Patch, chat/ part v6

*** Original change on bio 1100 attmnt 2291 at 2013-03-22 21:15:56 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354056 - Attachment is obsolete: true
Attachment #8354056 - Flags: review?(florian)
*** Original post on bio 1100 at 2013-03-22 21:29:54 UTC ***

(In reply to comment #42)

> > >+  notifyCertProblem: function(aSocketInfo, aStatus, aTargetSite) {
> > >+    this.targetSite = aTargetSite;
> > >+    this.sslStatus = aStatus;
> > Does this just kinda hang around forever (until the next problem)?
> Yes, do you think this would be an issue? Maybe we should clear it on connect.

I saw this too when reviewing the patch, but I didn't mention it in my review comments because we usually drop the whole Socket object when the connection of an account stops. I think the API has been designed with the expectation that we don't re-use Socket instances, and that we just drop them after a connection was closed, or failed.
Comment on attachment 8354061 [details] [diff] [review]
Patch, chat/ part v7

*** Original change on bio 1100 attmnt 2296 at 2013-04-05 22:51:43 UTC ***

This looks good. My only comment is that I would like to either have an explanation of why we need to store Socket.targetSite, or have that replaced with:
this._connectionTarget = aSocket.host + ":" + aSocket.port;

These 2 review comments from Mook look like they haven't been addressed:
(from comment #40)

> >@@ -400,36 +407,48 @@ const Socket = {
> >     delete this.isConnected;
> >     if (aStatus == NS_ERROR_NET_RESET)
> >       this.onConnectionReset();
> >     else if (aStatus == NS_ERROR_NET_TIMEOUT)
> >       this.onConnectionTimedOut();
> >     else if (aStatus) {
>     else if (!Components.isSuccessCode(aStatus)) {


> >diff --git a/chat/protocols/irc/irc.js b/chat/protocols/irc/irc.js
> 
> >+  onBadCertificate: function(aIsSslError, aNSSErrorMessage) {
> >     this.ERROR("bad certificate: " + aNSSErrorMessage);
> [...] this message is incorrect if aIsSslError.
Attachment #8354061 - Flags: review?(florian) → review-
*** Original post on bio 1100 as attmnt 2340 at 2013-04-10 22:59:00 UTC ***

This meets the review comments from comment #44, sorry I had missed Mook's earlier! This removes targetSite as it seems to be equal to host:port.
Attachment #8354107 - Flags: review?(florian)
Comment on attachment 8354061 [details] [diff] [review]
Patch, chat/ part v7

*** Original change on bio 1100 attmnt 2296 at 2013-04-10 22:59:15 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354061 - Attachment is obsolete: true
*** Original post on bio 1100 as attmnt 2342 at 2013-04-11 02:50:00 UTC ***

Fixes the issues seen at http://log.bezut.info/instantbird/130410/#m654
Attachment #8354109 - Flags: review?(florian)
Comment on attachment 8354036 [details] [diff] [review]
Patch, instantbird/ part v3

*** Original change on bio 1100 attmnt 2271 at 2013-04-11 02:50:49 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354036 - Attachment is obsolete: true
Comment on attachment 8354107 [details] [diff] [review]
Patch, chat/ part v8

*** Original change on bio 1100 attmnt 2340 at 2013-04-11 07:27:17 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354107 - Flags: review?(florian) → review+
Comment on attachment 8354109 [details] [diff] [review]
Patch, instantbird/ part v4

*** Original change on bio 1100 attmnt 2342 at 2013-04-11 07:27:24 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354109 - Flags: review?(florian) → review+
*** Original post on bio 1100 at 2013-04-11 07:32:31 UTC ***

http://hg.instantbird.org/instantbird/rev/371f856510bc
http://hg.instantbird.org/instantbird/rev/dd0e6e6e7afe

Thanks for this great improvement that will save us from lots of support requests :-).

Note: I'm resolving this as FIXED, but this unfortunately doesn't fix the issue in comment 0 which was about libpurple's xmpp prpl; I'm afraid that specific case is wontfix and just depends on bug 955019 (bio 1589).
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
OS: Windows XP → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → 1.4
Is this released? I think I hit this on 1.5 on Win32.
I've just tried Win32 nightly and I still get "SSL Handshake failed". Is there a way to debug what is happening? I know that the certificate of the XMPP server I've been trying to connect is not trusted.
(In reply to Rade Martinović from comment #64)
> I've just tried Win32 nightly and I still get "SSL Handshake failed". Is
> there a way to debug what is happening? I know that the certificate of the
> XMPP server I've been trying to connect is not trusted.

This only works for the JavaScript implementation of XMPP, which is not used by default in Instantbird. You need to import the certificate in Tools > Options > Advanced > Encryption > "View Certificates" > Servers > Import...
I would love to import the cert. that my company is using, but I don't know how to get my hands on it. I will ask our IT guys, but they'll probably say "but we told you to use Spark IM".
(In reply to Rade Martinović from comment #66)
> I would love to import the cert. that my company is using, but I don't know
> how to get my hands on it. I will ask our IT guys, but they'll probably say
> "but we told you to use Spark IM".

Ah, sorry...there's an easier way: you can import the certificate in Tools > Options > Advanced > Encryption > "View Certificates" > Servers > Add Exception..., then type in the server/port you connect to (e.g. https://yourcompany.com:5222). You'll want to keep the https:// in front to have it do an SSL connection. If we need to discuss this more we should take it out of this bug, which is fixed. You can hop on IRC and talk to us in #instantbird on irc.mozilla.org or email our mailing list https://lists.mozilla.org/listinfo/support-instantbird
You need to log in before you can comment on or make changes to this bug.