846581 - certificate override mechanisms for tests: use onreadystatechange, not nsIBadCertListener2

Status: RESOLVED FIXED

(Toolkit :: General, enhancement, P1)

Description

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

+++ This bug was initially created as a clone of Bug #750421 +++

Some tests use an XHR with an nsIBadCertListener2 implementation to add a certificate override. We're working on removing nsIBadCertListener2, and these can be replaced by getting the certificate in onreadystatechange and adding the override there. This does make for some fun asynchronous callback hoop-jumping, but it's all javascript anyway, so I don't think that's a problem.

Comment 1

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

Attachment: patch

Does this look like a reasonable approach?

Comment 2

[Honza Bambas (:mayhemer)]

Just let you know, we do the request just to easily get instance of nsIX509Cert.  We can create it by e.g. loading the cert from a file.  Other option is to add the server cert certification authority (or the cert it self, when it's self signed) as a trusted cert to the profile NSS database.  AFAIK, this is ensured for mochitests.

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

To expand on what Honza said: In many cases these tests are using the bad cert listener because whoever wrote the test did not know how to write the test so that it used a valid (at least, valid for the duration of the test) certificate. It is relatively simple for mochitest.

I created an xpcshell test which shows how a temporarily-trusted root can be added for xpcshell in https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/unit/test_signed_apps.js.

In toolkit/mozapps/extensions/test/browser, the test is written in that weird way because the person who wrote the test is using that network connection to get the actual certificates so a cert override can be added for them. But we can instead just, like Honza said, load the certs for the *.example.com sites from a file as test_signed_apps.js does.

test_spdy.js should do the same. (It is really bad for test_spdy.js to be using the bad cert listener approach anyway, because our SPDY implementation works differently when there is a cert error vs. when there isn't, at least with respect to connection coalescing.)

cviecco wrote test_cert_overrides so he may have ideas about how to modify that test to avoid needed to use nsIBadCertListener2, but I think it is the same as the toolkit case.

Also, Honza did related work in bug 466524 to create a small framework for SSL testing in xpcshell.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 719758 [details] [diff] [review]
patch

(In reply to Brian Smith (:bsmith) from comment #3)
> In toolkit/mozapps/extensions/test/browser, the test is written in that
> weird way because the person who wrote the test is using that network
> connection to get the actual certificates so a cert override can be added
> for them. But we can instead just, like Honza said, load the certs for the
> *.example.com sites from a file as test_signed_apps.js does.

To clarify this further: it seems like the technique you are using does work fine for this test, because it needs to use cert overrides anyway. That is, there isn't any need to change what you did to load the cert from a file like test_signed_apps.js does, if you don't want to. But...

> test_spdy.js should do the same. (It is really bad for test_spdy.js to be
> using the bad cert listener approach anyway, because our SPDY implementation
> works differently when there is a cert error vs. when there isn't, at least
> with respect to connection coalescing.)

It would be better to have test_spdy.js use a certificate that is actually trusted, like test_signed_apps.js does.

> cviecco wrote test_cert_overrides so he may have ideas about how to modify
> that test to avoid needed to use nsIBadCertListener2, but I think it is the
> same as the toolkit case.

So, whichever technique you use for toolkit/ should work for this too.

Comment 5

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

Attachment: patch v2

Ok - this is much better. My understanding is the security/ and toolkit/ tests are specifically testing overrides, so I've continued to use the cert override service there, just without the callback nonsense.
The spdy test setup was a bit weird - it claimed to be using a server cert and a corresponding ca cert, but the server cert was self-signed and the "ca cert" was actually a csr. Also, the server cert had expired. So, I made a fake ca and signed a server cert (both good until 2043). Then, I modified the spdy test to load up the ca file (well, a copy of it - they're in completely different directories) and add it as a trusted root. So, not only does this get rid of a use of nsIBadCertListener2, it tests spdy in a more real-world situation.

Comment 6

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

Comment on attachment 730213 [details] [diff] [review]
patch v2

Ted, since it looks like you were involved with the initial implementation of the spdy test, I would appreciate if you took a look at this. Most of the context you'll need is in comment 5. Thanks!

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 730213 [details] [diff] [review]
patch v2

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

You're probably better off with hurley here. I reviewed harness changes to add the SPDY tests, but I don't really know much about the tests themselves. If you can't get a review elsewhere I'll take a look.

Comment 8

[u408661]

Comment on attachment 730213 [details] [diff] [review]
patch v2

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

This seems sane to me, just a couple questions:

(1) Will the test continue to run even after the cert in it expires? It seems bad to have tests start failing just because a particular date has been reached (even if that date *is* 30 years out, misconfigurations can happen). This isn't a showstopper, just something to keep in the back of our collective mind (assuming xpcshell and gecko don't go the way of the dodo within 30 years)
(2) I don't see a try run associated with this patch. Is there one? I remember there being some odd interactions that needed ironing out with the spdy test when we first deployed on the infrastructure, so we want to make sure nothing like that has happened with these new changes.

f+ from me, bsmith can handle the r+, since he's a necko peer.

Comment 9

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

Thanks for the feedback. I can add a check that fails the test with an informative message if it's 2043. I really, really hope we're not still using xpcshell/gecko in 30 years.
I usually do a try run after getting reviews in case I need to change anything. FWIW, the tests I've changed pass on my local machine.

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 730213 [details] [diff] [review]
patch v2

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

::: netwerk/test/unit/test_spdy.js
@@ +285,5 @@
> +                  .createInstance(Ci.nsIFileInputStream);
> +  fstream.init(filename, -1, 0, 0);
> +  var data = NetUtil.readInputStreamToString(fstream, fstream.available());
> +  fstream.close();
> +  return data;

Put this in head_psm.js

@@ +297,3 @@
>  
> +  var der = readFile(certFile);
> +  certdb.addCert(der, "CTu,,", "spdy-test-root");

Based on the other stuff I reviewed today, perhaps it is worth putting a "addCertFromFile" function in head_psm.js that encapsulates these four lines of code.

Also, the name parameter is not needed, is it?

::: security/manager/ssl/tests/mochitest/bugs/test_certificate_overrides.html
@@ +40,5 @@
>      url = "https://" + host + ":" + port + "/";
>    } else {
>      url = "https://" + host + "/";
>    }
> +  req.open("GET", url, true);

Why did you change the sync XHR to async XHR? AFAICT, sync XHR should work because it spins the event loop like you are doing manually.

In general, it seems like the only thing you should need to change here is changing the use of req.channel.notificationCallbacks to the use of req.onerror.

@@ +42,5 @@
>      url = "https://" + host + "/";
>    }
> +  req.open("GET", url, true);
> +  var done = false;
> +  req.onreadystatechange = function() {

Instead of onreadystatechange, why not use onerror to make this clearer?

See https://developer.mozilla.org/en-US/docs/How_to_check_the_security_state_of_an_XMLHTTPRequest_over_SSL

::: toolkit/mozapps/extensions/test/browser/head.js
@@ +489,4 @@
>  // Add overrides for the bad certificates
>  function addCertOverride(host, bits) {
>    var req = new XMLHttpRequest();
> +  req.open("GET", "https://" + host + "/", true);

Same questions as above.

Is there a way to put this in a reusable place? If we create a security/mangaer/tests/unit/head_cert_overrides.js[m], then I think there should be some way to a way to reuse it here so that we don't have the copy-pasta.

Comment 11

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

Thanks for the review - I have some questions, though.

(In reply to Brian Smith (:bsmith) from comment #10)
> Comment on attachment 730213 [details] [diff] [review]
> ::: netwerk/test/unit/test_spdy.js
> @@ +285,5 @@
> > +                  .createInstance(Ci.nsIFileInputStream);
> > +  fstream.init(filename, -1, 0, 0);
> > +  var data = NetUtil.readInputStreamToString(fstream, fstream.available());
> > +  fstream.close();
> > +  return data;
> 
> Put this in head_psm.js

It's there now, but since this test is in netwerk/test/unit and head_psm.js is in security/manager/..., I don't know of an easy way to share between the two. Would it make sense to move this test? Should that shared code be hoisted even farther up the sharing chain? (i.e. should it be in the test framework itself?)

> ::: security/manager/ssl/tests/mochitest/bugs/test_certificate_overrides.html
> ::: toolkit/mozapps/extensions/test/browser/head.js
> @@ +489,4 @@
> >  // Add overrides for the bad certificates
> >  function addCertOverride(host, bits) {
> >    var req = new XMLHttpRequest();
> > +  req.open("GET", "https://" + host + "/", true);
> 
> Is there a way to put this in a reusable place? If we create a
> security/mangaer/tests/unit/head_cert_overrides.js[m], then I think there
> should be some way to a way to reuse it here so that we don't have the
> copy-pasta.

We might be able to make a cert_overrides.jsm or a psm_utils.jsm or something that can be imported from anywhere - should I look into this?

Comment 12

[u408661]

(In reply to David Keeler (:keeler) from comment #11)
> Thanks for the review - I have some questions, though.
> 
> (In reply to Brian Smith (:bsmith) from comment #10)
> > Comment on attachment 730213 [details] [diff] [review]
> > ::: netwerk/test/unit/test_spdy.js
> > @@ +285,5 @@
> > > +                  .createInstance(Ci.nsIFileInputStream);
> > > +  fstream.init(filename, -1, 0, 0);
> > > +  var data = NetUtil.readInputStreamToString(fstream, fstream.available());
> > > +  fstream.close();
> > > +  return data;
> > 
> > Put this in head_psm.js
> 
> It's there now, but since this test is in netwerk/test/unit and head_psm.js
> is in security/manager/..., I don't know of an easy way to share between the
> two. Would it make sense to move this test? Should that shared code be
> hoisted even farther up the sharing chain? (i.e. should it be in the test
> framework itself?)

My $0.02 - the shared code should be somewhere that everyone can use it (whether it's in the global xpcshell head.js or a cert_overrides.jsm like you mentioned later is less important, though having a jsm seems cleaner). It doesn't make sense to put the spdy test (which tests networking) in with the PSM tests *just* because it needs a cert override.

Comment 13

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

Attachment: patch v3

* changed to use onerror
* made spdy tests use head_psm.js
* refactored addCertOverride into newly added CertUtils.jsm

Comment 14

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 759860 [details] [diff] [review]
patch v3

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

Please request re-review when you answer the questions.

::: netwerk/test/unit/test_spdy.js
@@ +347,5 @@
>    var oldPref = prefs.getIntPref("network.http.speculative-parallel-limit");
>    prefs.setIntPref("network.http.speculative-parallel-limit", 0);
>  
> +  var certdb = Cc["@mozilla.org/security/x509certdb;1"].getService(Ci.nsIX509CertDB);
> +  addCertFromFile(certdb, "spdy-ca.der", "CTu,,");

Question: I guess this spdy-ca.der root is just a "good for the purposes of testing SPDY CA" and that there is a corresponding "good for the purposes of testing SPDY" end-entity certificate.

But, do we really need to have a new good EE cert and a new good root cert for each test? Couldn't we just have a single "good for the purposes of testing things that require a trusted CA" root that signs a corresponding EE certificate, that we use for all tests that require such things? If so, don't we already have one (or several) such CA and EE certificates? For example, don't we have one that is used for testing OCSP stapling that we could also reuse here?

I think it would be great if we could add a "addTestCA()" function to head_psm.js that any test could call to automatically get a trusted CA.

Now, maybe the SPDY test is an exception because the server is node.js and requires openssl? If so, it is understandable to do something special for it.

Nit: Perhaps addCertFromFile should just do the Cc["@mozilla.org/security/x509certdb;1"].getService(Ci.nsIX509CertDB) itself.

::: testing/modules/CertUtils.jsm
@@ +11,5 @@
> +let Cc = Components.classes;
> +let Ci = Components.interfaces;
> +
> +let CertUtils = {
> +  addCertOverride: function CertUtils_addCertOverride(host, bits, port) {

Please document the reason for doing the XHR here. AFAICT, the only reason we're doing the XHR is because we don't know the cert since the caller doesn't pass it in.

Comment 15

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Brian Smith (:briansmith), was [e-mail] (:bsmith) from comment #14)
> Now, maybe the SPDY test is an exception because the server is node.js and
> requires openssl? If so, it is understandable to do something special for it.

Thinking about this more: The only thing we should need to do special for the SPDY test is export the key and cert into an OpenSSL-friendly format so that it can be used by the node.js-based server.

Comment 16

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

Attachment: bug 846581 - avoid nsIBadCertListener2 in toolkit install/update tests r?mossop

This patch changes how some toolkit mochitest-browser-chrome tests set up
certificate error overrides to avoid using nsIBadCertListener2 (basically it
follows the approach of exceptionDialog.js and uses the onerror callback of the
XHR).

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/494abd5b60b8
avoid nsIBadCertListener2 in toolkit install/update tests r=mossop

Comment 18

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/494abd5b60b8