The heckCertOverridesbits function uses a substraction isntead of a bitwise maninulation for clearing overrides. This causes that when there are overrides than the currently enabled are found (such as when libpix is enabled or if the cert changes) the override service does not returned cleared even when that should be the case. Steps to reproduce (on a fresh checkout) 1. Enable libpkix verfication in /modules/libpref/src/init/all.js 2. recompile 3. Run the test_sdpy.js xpcshell test case (you need node.js). 4. The test will fail.

This a two byter.. already r+ by ryan smith, now asking Honza for second review so that I can land.

Comment on attachment 654831 [details] [diff] [review] Fix to the bug Please use r? and sr? to handle the two-review requirement in security/.

Comment on attachment 654831 [details] [diff] [review] Fix to the bug Review of attachment 654831 [details] [diff] [review]: ----------------------------------------------------------------- See also https://bugzilla.mozilla.org/show_bug.cgi?id=571034#c1. That bug is effectively a dup of this one, btw. sr=honzab.

Comment on attachment 654831 [details] [diff] [review] Fix to the bug [Approval Request Comment] Bug caused by (feature/regressing bug #): This is a longstanding bug. User impact if declined: The user may encounter the "certificate error" page two or three times (over a period of time) instead of just once, even if they override the error. (This is long-standing behavior.) Testing completed (on m-c, etc.): This just landed on inbound. Risk to taking this patch (and alternatives if risky): No risk. This is a trivial change that is an obviously improvement. String or UUID changes made by this patch: none.

(In reply to Brian Smith (:bsmith) from comment #7) > User impact if declined: The user may encounter the "certificate error" page > two or three times (over a period of time) instead of just once, even if > they override the error. (This is long-standing behavior.) Is the concern here that bug 650355 will make the certificate error much more common?

Comment on attachment 654831 [details] [diff] [review] Fix to the bug [Triage Comment] Approving for Aurora in preparation for Beta consideration.

(In reply to Alex Keybl [:akeybl] from comment #8) > (In reply to Brian Smith (:bsmith) from comment #7) > > User impact if declined: The user may encounter the "certificate error" page > > two or three times (over a period of time) instead of just once, even if > > they override the error. (This is long-standing behavior.) > > Is the concern here that bug 650355 will make the certificate error much > more common? Yes, that's right.

Comment on attachment 654831 [details] [diff] [review] Fix to the bug [Triage Comment] Let's get bugs like this nominated sooner in the release cycle. Approving for Beta since this is a trivial fix for what may become an annoying "regression". If you know of an md5 certificate site that QA can use to verify, please include here.

Wait, |!overrideBits| and not |~overrideBits|?

(In reply to :Ms2ger from comment #13) > Wait, |!overrideBits| and not |~overrideBits|? Camilo, please fix this ASAP. Thanks, Ms2ger.

Added two patches for you to choose. this one Assumes the first patch was not placed before

Comment on attachment 654831 [details] [diff] [review] Fix to the bug [Triage Comment] Given the churn in this bug, setting the aurora/beta flags back to ?. This patch was meant to be very low risk, but has since been reopened.

Comment on attachment 665548 [details] [diff] [review] fix-to-commited patch. Use bitwise NOT instead of logic NOT Review of attachment 665548 [details] [diff] [review]: ----------------------------------------------------------------- https://hg.mozilla.org/integration/mozilla-inbound/rev/5d87924d4760 https://hg.mozilla.org/releases/mozilla-aurora/rev/260a27f882b5 https://hg.mozilla.org/releases/mozilla-beta/rev/f95d57abe039 OK, I didn't see Alex's last comment until I'd already checked in the fixed version of the patch into all three channels. Shall I back them out? It was definitely silly that three different people missed "!" vs "~" in the patch but it's still very low risk. (Now even more risk, now that it is correct!)

Comment on attachment 665550 [details] [diff] [review] To patch before reverting. Fixing bitwise NOT https://hg.mozilla.org/releases/mozilla-aurora/rev/260a27f882b5 https://hg.mozilla.org/releases/mozilla-beta/rev/f95d57abe039 I already checked in this patch based on the previous approval for the one with the typo. See the previous comment. [Approval Request Comment] Bug caused by (feature/regressing bug #): longstanding issue User impact if declined: Under some conditions, certificate error pages may be shown more frequently than necessary and/or some certificate errors may not overridable. Testing completed (on m-c, etc.): Manual. Just landed. Risk to taking this patch (and alternatives if risky): Basically none (for real this time.) String or UUID changes made by this patch: none.

Comment on attachment 665550 [details] [diff] [review] To patch before reverting. Fixing bitwise NOT [Triage Comment] We're going to trust your intuition and leave it approved, to prevent a frustrating UX. Please instruct QA on the best way to verify and find regressions.

Pushed to Aurora: http://hg.mozilla.org/releases/mozilla-aurora/rev/260a27f882b5 Pushed to Beta: http://hg.mozilla.org/releases/mozilla-beta/rev/f95d57abe039

(In reply to Alex Keybl [:akeybl] from comment #20) > Please instruct QA on the best way to verify and find regressions. Brian - still waiting on this.

I think I'm gonna by one!

cviecco, please either write an automated test or respond on comment 23. If you write an automated test, I think you can do so in a mochitest by directly calling into nsICertOverrideService to set a cert override for some invalid cert that has only one bit set (e.g. expired) for a cert that has two problems (e.g. expired + untrusted issuer), then open up an SSL connection. The test should fail without your patch and it should succeed with your patch. See toolkit/mozapps/extensions/test/browser/browser_installssl.js for an example. For this, you need to generate a new certificate that has the two errors. This is good because I think you've already done similar work in bug 699874.

Was out of the office until today. Seems like writing a mochitest would be possible. And Yes I am going to get one of those shirts. Shall I open a follow bug for the tests?

Just use this bug.

BTW, you might not need a cert with two problems; it might be the case that you simply need a cert that has a different problem (e.g. expired) than what the override is for.

This tests the override service for all certoverride bits but only for 1. expired 2. unstrusted 3. expired and mismatch 4. unstrusted and mismatch Three cases are pending (adding mismatch only, expired and untrusted and all wrong), but those require changing the pgo cert db. This patch is just a preview.

Comment on attachment 667733 [details] [diff] [review] test-certoverrides Review of attachment 667733 [details] [diff] [review]: ----------------------------------------------------------------- Great work! Please describe what you did with pgocerts to modify the cert and key databases. Please attach a log showing the output of this test when your patch for this bug is not applied (i.e. when we use operator- instead of operator&), to show which tests fail (or add a link to a tryserver run that shows that this test fails without your fix.) ::: security/manager/ssl/tests/mochitest/bugs/test_certificate_overrides.html @@ +1,5 @@ > +<!DOCTYPE HTML> > +<html> > +<head> > + <title>Test certificate overrides</title> > + <script type="text/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script> Fix trailing whitespace highlighted by splinter everywhere in this file. Also, many parts of this file do not follow the style guide, regarding camelCase naming and whitespace conventions. Please fix that. Also, restrict the number of blank lines between functions to one. Remove commented-out code. @@ +15,5 @@ > +//const Cu = Components.utils; > +//const Cr = Components.results; > + > +var testHost=[]; > +testHost[1]="untrusted.example.com"; s/testHost/testHosts/ @@ +16,5 @@ > +//const Cr = Components.results; > + > +var testHost=[]; > +testHost[1]="untrusted.example.com"; > +//mismatch is 2 what does this comment mean? @@ +49,5 @@ > + }, > + QueryInterface: function(aIID) { > + if (aIID.equals(Ci.nsIBadCertListener2) || > + aIID.equals(Ci.nsIInterfaceRequestor) || > + aIID.equals(Ci.nsISupports)) Add braces around body of statement to make things easier to read. @@ +56,5 @@ > + }, > + notifyCertProblem: function(socketInfo, sslStatus, targetHost) { > + certerrorbits = (sslStatus.QueryInterface(Ci.nsISSLStatus).isUntrusted ) | > + (sslStatus.QueryInterface(Ci.nsISSLStatus).isDomainMismatch <<1 ) | > + (sslStatus.QueryInterface(Ci.nsISSLStatus).isNotValidAtThisTime <<2 ); QueryInterface once. certErrorBits = 0; if (sslStatus.isUntrusted) certErrorBits |= Ci.nsICertOverrideService.ERROR_UNTRUSTED; ... This way, we don't have to worry about whether ERROR_UNTRUSTED is 1, etc. @@ +77,5 @@ > + } else { > + url = "https://" + host + "/"; > + } > + req.open("GET", url, false); > + req.channel.notificationCallbacks = new CertOverrideListener(host, port, bits); None of the above lines in this block should be in the try block. We want the test to fail if any of them fail. @@ +78,5 @@ > + url = "https://" + host + "/"; > + } > + req.open("GET", url, false); > + req.channel.notificationCallbacks = new CertOverrideListener(host, port, bits); > + req.send(null); missing ok(false, "the request should have failed"); @@ +80,5 @@ > + req.open("GET", url, false); > + req.channel.notificationCallbacks = new CertOverrideListener(host, port, bits); > + req.send(null); > + } catch (e) { > + // This will fail since the server is not trusted yet missing ok(true, ...); @@ +85,5 @@ > + } > + } > + > + > +/////////////////////// remove this line and extra blank lines. @@ +87,5 @@ > + > + > +/////////////////////// > + > +function xhrMustSucceed(domain,message){ why not: function xhr(domain, message, mustSucceed) and... @@ +96,5 @@ > + try{ > + req.open("GET", "https://"+domain+"/",false); > + req.send(null); > + var headers = req.getAllResponseHeaders(); > + ok(true, "Page Load success "+message); ok(mustSuceeded, "Page load success " + message); and... @@ +99,5 @@ > + var headers = req.getAllResponseHeaders(); > + ok(true, "Page Load success "+message); > + } > + catch(err){ > + ok(false, "Page failed to load it should succeed message="+message); ok(!mustSucceed, "page load failed " + message); and... @@ +103,5 @@ > + ok(false, "Page failed to load it should succeed message="+message); > + }; > +} > + > +function xhrMustFail(domain,message){ ...remove this function. @@ +119,5 @@ > + }; > +} > + > + > +function doConnectTests(overridebits){ Write a comment about what this function is supposed to do. Instead of having this function loop through the test hosts, why not put the loop inside testCertOverrides, so that this function would take (testHost, overrideBits) as parameters? This would make it clearer that we're testing the cross product of hosts * override bits, and it would make this function simpler. @@ +121,5 @@ > + > + > +function doConnectTests(overridebits){ > + var cos = Cc["@mozilla.org/security/certoverride;1"]. > + getService(Ci.nsICertOverrideService); might as well make cos a global variable that is initialized in the beginning, so we don't have to repeat this in every function. @@ +123,5 @@ > +function doConnectTests(overridebits){ > + var cos = Cc["@mozilla.org/security/certoverride;1"]. > + getService(Ci.nsICertOverrideService); > + for(var i=1;i<8;i++){ > + if(typeof testHost[i] == 'undefined'){ Why? @@ +138,5 @@ > + else{ > + xhrMustFail(host,"override host="+host+status_info); > + } > + //reset overrides > + cos.clearValidityOverride(host,443); isn't this redundant? @@ +144,5 @@ > + > +} > + > + > +function checkHostErrorbits(hostindex) { capitalization in name. What is this function supposed to do, exactly? Please add a comment. and, why not: function checkHostErrorBits(host) so that this function doesn't need to know about the array at all. @@ +148,5 @@ > +function checkHostErrorbits(hostindex) { > + var port =443; > + var bits = 15; > + > + if(typeof testHost[hostindex] == 'undefined'){ Why? @@ +167,5 @@ > + url = "https://" + host + "/"; > + } > + req.open("GET", url, false); > + req.channel.notificationCallbacks = new CertOverrideListener(host, port, bits); > + req.send(null); ok(false, ...); @@ +169,5 @@ > + req.open("GET", url, false); > + req.channel.notificationCallbacks = new CertOverrideListener(host, port, bits); > + req.send(null); > + } catch (e) { > + // This will fail since the server is not trusted yet ok(true, ...); @@ +174,5 @@ > + } > + //now compare > + is(certerrorbits,hostindex,"errorbits="+certerrorbits); > + //and reset host the state > + var cos = Cc["@mozilla.org/security/certoverride;1"]. indention. @@ +178,5 @@ > + var cos = Cc["@mozilla.org/security/certoverride;1"]. > + getService(Ci.nsICertOverrideService); > + cos.clearValidityOverride(host,443); > + } > + Too many blank lines (here and elsewhere). @@ +182,5 @@ > + > + > + > +function testCertOverrides(){ > + for (var i=1;i<8;i++){ var allBits = Ci.nsICertOverrideService.ERROR_UNTRUSTED | Ci.nsICertOverrideService.ERROR_MISMATCH | Ci.nsICertOverrideService.ERROR_TIME; // Loop through every combination of overrides for (var i = 1; i < allBits; i++) { @@ +194,5 @@ > + > +function onWindowLoad() > +{ > +/* > +*/ :(

Comment on attachment 667733 [details] [diff] [review] test-certoverrides Review of attachment 667733 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/tests/mochitest/bugs/test_certificate_overrides.html @@ +16,5 @@ > +//const Cr = Components.results; > + > +var testHost=[]; > +testHost[1]="untrusted.example.com"; > +//mismatch is 2 personal comment (what is the bit value of mismatched?) , removed. @@ +49,5 @@ > + }, > + QueryInterface: function(aIID) { > + if (aIID.equals(Ci.nsIBadCertListener2) || > + aIID.equals(Ci.nsIInterfaceRequestor) || > + aIID.equals(Ci.nsISupports)) done @@ +56,5 @@ > + }, > + notifyCertProblem: function(socketInfo, sslStatus, targetHost) { > + certerrorbits = (sslStatus.QueryInterface(Ci.nsISSLStatus).isUntrusted ) | > + (sslStatus.QueryInterface(Ci.nsISSLStatus).isDomainMismatch <<1 ) | > + (sslStatus.QueryInterface(Ci.nsISSLStatus).isNotValidAtThisTime <<2 ); We still need to worry for the hostindex, but I like this as it removes one more place to look. @@ +77,5 @@ > + } else { > + url = "https://" + host + "/"; > + } > + req.open("GET", url, false); > + req.channel.notificationCallbacks = new CertOverrideListener(host, port, bits); agreed.done @@ +85,5 @@ > + } > + } > + > + > +/////////////////////// ack, done @@ +87,5 @@ > + > + > +/////////////////////// > + > +function xhrMustSucceed(domain,message){ makes sense. Will do @@ +119,5 @@ > + }; > +} > + > + > +function doConnectTests(overridebits){ agreed @@ +121,5 @@ > + > + > +function doConnectTests(overridebits){ > + var cos = Cc["@mozilla.org/security/certoverride;1"]. > + getService(Ci.nsICertOverrideService); done, now global const. @@ +123,5 @@ > +function doConnectTests(overridebits){ > + var cos = Cc["@mozilla.org/security/certoverride;1"]. > + getService(Ci.nsICertOverrideService); > + for(var i=1;i<8;i++){ > + if(typeof testHost[i] == 'undefined'){ testHost was sparse at one moment. @@ +138,5 @@ > + else{ > + xhrMustFail(host,"override host="+host+status_info); > + } > + //reset overrides > + cos.clearValidityOverride(host,443); no, actually the first one would be the reduntant. as I assume that none of the hosts have overrides to begin with. @@ +144,5 @@ > + > +} > + > + > +function checkHostErrorbits(hostindex) { The function would then need the expected errors (they match the host index) and the signarue would be function checkHostErrors(host,expectedErrors). @@ +148,5 @@ > +function checkHostErrorbits(hostindex) { > + var port =443; > + var bits = 15; > + > + if(typeof testHost[hostindex] == 'undefined'){ because initial versions of the hostindex was not filled completelly (missing tesing cases) and one more extra check seemed innocuous. @@ +167,5 @@ > + url = "https://" + host + "/"; > + } > + req.open("GET", url, false); > + req.channel.notificationCallbacks = new CertOverrideListener(host, port, bits); > + req.send(null); Yep missed that one @@ +169,5 @@ > + req.open("GET", url, false); > + req.channel.notificationCallbacks = new CertOverrideListener(host, port, bits); > + req.send(null); > + } catch (e) { > + // This will fail since the server is not trusted yet I can add it but is seems redundant. @@ +182,5 @@ > + > + > + > +function testCertOverrides(){ > + for (var i=1;i<8;i++){ ok for (var i = 1; i <= allBits; i++) (or 'for (var i = 1; i < allBits+1; i++)' ) @@ +194,5 @@ > + > +function onWindowLoad() > +{ > +/* > +*/ So sad...... Yes I agree

Comment on attachment 667733 [details] [diff] [review] test-certoverrides Review of attachment 667733 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/tests/mochitest/bugs/test_certificate_overrides.html @@ +138,5 @@ > + else{ > + xhrMustFail(host,"override host="+host+status_info); > + } > + //reset overrides > + cos.clearValidityOverride(host,443); Tests should always assume that the previous test messed up and didn't clean up after itself (because some tests are buggy and don't clean up properly). So, you should clear all the overrides at the very start of the test as well. @@ +182,5 @@ > + > + > + > +function testCertOverrides(){ > + for (var i=1;i<8;i++){ Good catch. (I prefer <=, but I am not one to nit-pick.)

This is the output of running the test with: 'TEST_PATH=security/ssl/bugs/test_certificate_overrides.html make -C obj-ff-dbg/ mochitest-chrome > /tmp/badout.txt' the things to notice are the expected fail readings, notice 12 test fail.

Comment on attachment 668195 [details] [diff] [review] test-certoverrides v2.2 Review of attachment 668195 [details] [diff] [review]: ----------------------------------------------------------------- Great work. r+ with comments addressed. ::: security/manager/ssl/tests/mochitest/bugs/test_certificate_overrides.html @@ +24,5 @@ > +testHost[ em | eu ] = "mismatch.untrusted.example.com"; > +testHost[ et ] = "expired.example.com"; > +testHost[ et | eu ] = "untrusted-expired.example.com"; > +testHost[ et | em ] = "mismatch.expired.example.com"; > +testHost[ et | em | eu ] = "mismatch.untrusted-expired.example.com"; You make testing for cert overrides so pretty! @@ +96,5 @@ > + var req = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"] > + .createInstance(Ci.nsIXMLHttpRequest); > + > + try { > + req.open("GET", "https://"+domain+"/", false); spaces around "+" (two places). @@ +98,5 @@ > + > + try { > + req.open("GET", "https://"+domain+"/", false); > + req.send(null); > + var headers = req.getAllResponseHeaders(); why are you grabbing the headers? @@ +106,5 @@ > + } > +} > + > +function checkHostConnect(hostIndex, overrideBits) { > + if (typeof testHost[hostIndex] == 'undefined') { can this happen? @@ +115,5 @@ > + var statusMessage = " hostIndex =" + hostIndex + " overrideBits=" + overrideBits; > + > + addCertOverride(host, 443, overrideBits); > + if (0 == (hostIndex & ~overrideBits)) { > + xhrConnect(host,"override host=" + host + statusMessage,true); whitespace. @@ +118,5 @@ > + if (0 == (hostIndex & ~overrideBits)) { > + xhrConnect(host,"override host=" + host + statusMessage,true); > + } > + else { > + xhrConnect(host,"override host=" + host + statusMessage,false); whitespace. @@ +145,5 @@ > + } catch (e) { > + // This will fail since the server is not trusted yet > + } > + //now compare > + is(certErrorBits,expectedErrorBits,"errorbits=" +certErrorBits + " host=" + host); spaces. @@ +150,5 @@ > + //and reset host the state > + cos.clearValidityOverride(host, port); > +} > + > +function testCertOverrides(){ () {

Comment on attachment 668195 [details] [diff] [review] test-certoverrides v2.2 Review of attachment 668195 [details] [diff] [review]: ----------------------------------------------------------------- Should I submit a new version with fixed or wait for Honza's sr? ::: security/manager/ssl/tests/mochitest/bugs/test_certificate_overrides.html @@ +96,5 @@ > + var req = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"] > + .createInstance(Ci.nsIXMLHttpRequest); > + > + try { > + req.open("GET", "https://"+domain+"/", false); Ack. Done @@ +98,5 @@ > + > + try { > + req.open("GET", "https://"+domain+"/", false); > + req.send(null); > + var headers = req.getAllResponseHeaders(); It was for the failure, but it should be removed. @@ +106,5 @@ > + } > +} > + > +function checkHostConnect(hostIndex, overrideBits) { > + if (typeof testHost[hostIndex] == 'undefined') { it should never happen, is just a fail safe in case we ever try to use an invalid hostIndex. I can remove if you wish. @@ +115,5 @@ > + var statusMessage = " hostIndex =" + hostIndex + " overrideBits=" + overrideBits; > + > + addCertOverride(host, 443, overrideBits); > + if (0 == (hostIndex & ~overrideBits)) { > + xhrConnect(host,"override host=" + host + statusMessage,true); ACK. Done @@ +118,5 @@ > + if (0 == (hostIndex & ~overrideBits)) { > + xhrConnect(host,"override host=" + host + statusMessage,true); > + } > + else { > + xhrConnect(host,"override host=" + host + statusMessage,false); ACK. Done @@ +145,5 @@ > + } catch (e) { > + // This will fail since the server is not trusted yet > + } > + //now compare > + is(certErrorBits,expectedErrorBits,"errorbits=" +certErrorBits + " host=" + host); ACK. (grrrr, what is that utility that you mentioned to help me catch these?) @@ +150,5 @@ > + //and reset host the state > + cos.clearValidityOverride(host, port); > +} > + > +function testCertOverrides(){ ACK, Done

Comment on attachment 670126 [details] [diff] [review] test-certoverrides v3 one typeo removing review requests

Camilo, please don't obsolete the current patch, you would destroy my splinter comments. Thanks.

(In reply to Honza Bambas (:mayhemer) from comment #45) > Camilo, please don't obsolete the current patch, you would destroy my > splinter comments. Thanks. OK. Thank you

Comment on attachment 670405 [details] [diff] [review] test-certoverrides v3.1 Review of attachment 670405 [details] [diff] [review]: ----------------------------------------------------------------- Overall, this is a very good piece of work! Please regenerate the certificates (where applicable) to expire not sooner then in 100 years. Many formatting nits, please strictly follow this formatting, mainly missing or unnecessary white spaces and CRs: gGlobalVariable; function name() { // Comment has to be a sentence. variable = 0; for (var i = 0; i < max; ++i) { } } Please provide certutil (or any other) commands you used to generate the certs, bug comment is enough. ::: build/pgo/server-locations.txt @@ +97,5 @@ > https://expired.example.com:443 privileged,cert=expired > https://requestclientcert.example.com:443 privileged,clientauth=request > https://requireclientcert.example.com:443 privileged,clientauth=require > +https://mismatch.expired.example.com:443 privileged,cert=expired > +https://mismatch.untrusted.example.com:443 privileged,cert=untrusted You would be OK with the self signed cert we already have, but up to you. @@ +98,5 @@ > https://requestclientcert.example.com:443 privileged,clientauth=request > https://requireclientcert.example.com:443 privileged,clientauth=require > +https://mismatch.expired.example.com:443 privileged,cert=expired > +https://mismatch.untrusted.example.com:443 privileged,cert=untrusted > +https://mismatch.example.com:443 privileged,cert=staticname I think you are OK with using nocert.example.com host here. Again, no need for a new cert here. ::: security/manager/ssl/tests/mochitest/bugs/test_certificate_overrides.html @@ +26,5 @@ > +testHost[ et | eu ] = "untrusted-expired.example.com"; > +testHost[ et | em ] = "mismatch.expired.example.com"; > +testHost[ et | em | eu ] = "mismatch.untrusted-expired.example.com"; > + > +var certErrorBits; call this gCertErrorBits. @@ +100,5 @@ > + req.open("GET", "https://" + domain + "/", false); > + req.send(null); > + ok(expectedSuccess, "Page Load success " + message + " expected=" + expectedSuccess); > + } catch (err) { > + ok(!expectedSuccess, "Page failed to load" + message + " expected=" + expectedSuccess); Check spaces in logs... @@ +113,5 @@ > + var host = testHost[hostIndex]; > + var statusMessage = " hostIndex =" + hostIndex + " overrideBits=" + overrideBits; > + > + addCertOverride(host, 443, overrideBits); > + if (0 == (hostIndex & ~overrideBits)) { You can just put this condition to the last arg of xhrConnect. @@ +141,5 @@ > + req.channel.notificationCallbacks = new CertOverrideListener(host, port, bits); > + req.send(null); > + ok(false, "Connection to host" + host + "succeeded when it should have failed"); > + } catch (e) { > + // This will fail since the server is not trusted yet "Failure here is expected" is a bit better comment, but up to you. @@ +144,5 @@ > + } catch (e) { > + // This will fail since the server is not trusted yet > + } > + //now compare > + is(certErrorBits, expectedErrorBits, "errorbits=" +certErrorBits + " host=" + host); I think you can do this check in checkHostConnect() when hostIndex == overrideBits after you call addCertOverride(). Then (g)certErrorBits must be equal to hostIndex, right? This code is mostly a copy of what addCertOverride does. Since this test makes a lot of requests (7*7+7=56) we may save those 7 at least. @@ +145,5 @@ > + // This will fail since the server is not trusted yet > + } > + //now compare > + is(certErrorBits, expectedErrorBits, "errorbits=" +certErrorBits + " host=" + host); > + //and reset host the state wording

Addresses Comments by Honza To generate the new cert: cd build/pgo/certs/ ../../../obj-ff-dbg/dist/bin/certutil -S -n "untrustedandexpired" -s "CN=untrusted-expired.example.com" -c "Unknown CA" -t "P,," -k rsa -g 1024 -m 98743 -v 0 -d .

Comment on attachment 670872 [details] [diff] [review] test-certoverrides v4.2 Addressed review of 3.1. -Renamed the certerror bits into gCertErrorBits. -Integrated the checks into a single function. (and this is why I am calling this a review) (removed 7 connections) -Changed style. -Fixed spaces in two of the log output strings. -Remove one unnecessary cert from the database. (I am still using 'the untrusted cert' instead of the self-signed for the untrusted cert test). -Added an extra test for the self-signed case.

Given there is an automated test for this, is there any necessity for QA to do a verification? If so, please advise on what we can check for in terms of regressions.

Comment on attachment 665550 [details] [diff] [review] To patch before reverting. Fixing bitwise NOT This was fixed on all channels already using the other patch.

Comment on attachment 670872 [details] [diff] [review] test-certoverrides v4.2 Review of attachment 670872 [details] [diff] [review]: ----------------------------------------------------------------- Dropping r, still not for to be landed. You didn't regenerate the 'untrusted' and 'Unknown CA' certs. Those will expire in 2019. Please fix the last formatting issues before landing please. I personally liked some blank lines you have removed, but that is up to you. ::: security/manager/ssl/tests/mochitest/bugs/test_certificate_overrides.html @@ +14,5 @@ > + getService(Ci.nsICertOverrideService); > +const eu = Ci.nsICertOverrideService.ERROR_UNTRUSTED; > +const em = Ci.nsICertOverrideService.ERROR_MISMATCH; > +const et = Ci.nsICertOverrideService.ERROR_TIME; > +//note: the host index matches the expected error. // Note: the host index matches the expected error. @@ +46,5 @@ > + }, > + QueryInterface: function(aIID) { > + if (aIID.equals(Ci.nsIBadCertListener2) || > + aIID.equals(Ci.nsIInterfaceRequestor) || > + aIID.equals(Ci.nsISupports)){ aIID.equals(Ci.nsISupports)) { Space before { @@ +82,5 @@ > + req.open("GET", url, false); > + req.channel.notificationCallbacks = new CertOverrideListener(host, port, bits); > + try { > + req.send(null); > + ok(false, "Connection to host" + host + "succeeded when it should have failed"); "Connection to host " + host + " succeeded when it should have failed" @@ +103,5 @@ > +} > + > +function checkHostConnect(host, overrideBits, successExpected, overridesMustEqualError) > +{ > + var statusMessage = " overrideBits=" + overrideBits; Why is this variable needed? @@ +105,5 @@ > +function checkHostConnect(host, overrideBits, successExpected, overridesMustEqualError) > +{ > + var statusMessage = " overrideBits=" + overrideBits; > + addCertOverride(host, 443, overrideBits); > + if(overridesMustEqualError) { if (overridesMustEqualError) { Space after if. @@ +107,5 @@ > + var statusMessage = " overrideBits=" + overrideBits; > + addCertOverride(host, 443, overrideBits); > + if(overridesMustEqualError) { > + is(gCertErrorBits, overrideBits, "Reported Error match: errorbits=" + gCertErrorBits + " host=" + host); > + } Does it make any sense to check here that gCertErrorBits & ~overrideBits == 0 in an else branch? @@ +124,5 @@ > + for (var j = 1; j < testHost.length; ++j){ > + var expectedError = j; > + var successExpected = (0 == (expectedError & ~overrideBits)); > + var errorMustMatch = (overrideBits == expectedError); > + checkHostConnect(testHost[expectedError], i, successExpected, errorMustMatch); pass overrideBits instead of i? and maybe testHost[j], but up to you. @@ +127,5 @@ > + var errorMustMatch = (overrideBits == expectedError); > + checkHostConnect(testHost[expectedError], i, successExpected, errorMustMatch); > + } > + } > + //now we test the self-signed. Must return an overridable untrusted error. // Now we test the self-signed. Must return an overridable untrusted error. @@ +129,5 @@ > + } > + } > + //now we test the self-signed. Must return an overridable untrusted error. > + cos.clearValidityOverride("self-signed.example.com", 443); > + checkHostConnect("self-signed.example.com", eu , successExpected, true); checkHostConnect("self-signed.example.com", eu, successExpected, true); Unnecessary white space after eu.

(In reply to Honza Bambas (:mayhemer) from comment #54) > Please fix the last formatting issues before landing please. I thought "before resubmitting" of course.

Comment on attachment 670872 [details] [diff] [review] test-certoverrides v4.2 Review of attachment 670872 [details] [diff] [review]: ----------------------------------------------------------------- I disagree with the regeneration of both 'untrusted' and 'Unknown CA' certs as these where in the database before this change. If we desire to update the expiration of certs this should be in a separate bug. ::: security/manager/ssl/tests/mochitest/bugs/test_certificate_overrides.html @@ +14,5 @@ > + getService(Ci.nsICertOverrideService); > +const eu = Ci.nsICertOverrideService.ERROR_UNTRUSTED; > +const em = Ci.nsICertOverrideService.ERROR_MISMATCH; > +const et = Ci.nsICertOverrideService.ERROR_TIME; > +//note: the host index matches the expected error. OK. Agreed. fixed @@ +46,5 @@ > + }, > + QueryInterface: function(aIID) { > + if (aIID.equals(Ci.nsIBadCertListener2) || > + aIID.equals(Ci.nsIInterfaceRequestor) || > + aIID.equals(Ci.nsISupports)){ Agreed Done. @@ +82,5 @@ > + req.open("GET", url, false); > + req.channel.notificationCallbacks = new CertOverrideListener(host, port, bits); > + try { > + req.send(null); > + ok(false, "Connection to host" + host + "succeeded when it should have failed"); Fixed @@ +103,5 @@ > +} > + > +function checkHostConnect(host, overrideBits, successExpected, overridesMustEqualError) > +{ > + var statusMessage = " overrideBits=" + overrideBits; Is not really needed. Just added it for readability. Can remove if you wish. @@ +105,5 @@ > +function checkHostConnect(host, overrideBits, successExpected, overridesMustEqualError) > +{ > + var statusMessage = " overrideBits=" + overrideBits; > + addCertOverride(host, 443, overrideBits); > + if(overridesMustEqualError) { Done @@ +107,5 @@ > + var statusMessage = " overrideBits=" + overrideBits; > + addCertOverride(host, 443, overrideBits); > + if(overridesMustEqualError) { > + is(gCertErrorBits, overrideBits, "Reported Error match: errorbits=" + gCertErrorBits + " host=" + host); > + } I think that is unncecesary. @@ +127,5 @@ > + var errorMustMatch = (overrideBits == expectedError); > + checkHostConnect(testHost[expectedError], i, successExpected, errorMustMatch); > + } > + } > + //now we test the self-signed. Must return an overridable untrusted error. Fixed. @@ +129,5 @@ > + } > + } > + //now we test the self-signed. Must return an overridable untrusted error. > + cos.clearValidityOverride("self-signed.example.com", 443); > + checkHostConnect("self-signed.example.com", eu , successExpected, true); Done fixed.

Solves issues on JS of version 4.2. Does NOT update the db from version 4.2 (ie does not update the certs already in the db).

Addresses comments for 4.2. Does not change the certdb from 4.2 (does not update certs already in the cert db).

(In reply to Camilo Viecco (:cviecco) from comment #56) > Comment on attachment 670872 [details] [diff] [review] > test-certoverrides v4.2 > > Review of attachment 670872 [details] [diff] [review]: > ----------------------------------------------------------------- > > I disagree with the regeneration of both 'untrusted' and 'Unknown CA' certs > as these > where in the database before this change. Ah, I wasn't aware. You had to enlighten me the last time already :)

Comment on attachment 676209 [details] [diff] [review] test-certoverrides v5 Review of attachment 676209 [details] [diff] [review]: ----------------------------------------------------------------- r=honzab with few last little nits. Thanks for all the updates and this pretty good test! ::: security/manager/ssl/tests/mochitest/bugs/test_certificate_overrides.html @@ +16,5 @@ > +const eu = Ci.nsICertOverrideService.ERROR_UNTRUSTED; > +const em = Ci.nsICertOverrideService.ERROR_MISMATCH; > +const et = Ci.nsICertOverrideService.ERROR_TIME; > + > +//Note: the host index matches the expected error. Space between // and Note (it is coding convention to have a space after //. @@ +106,5 @@ > +} > + > +function checkHostConnect(host, overrideBits, successExpected, overridesMustEqualError) > +{ > + var statusMessage = " overrideBits=" + overrideBits; For me this var was more confusing, but up to you to leave or remove. @@ +130,5 @@ > + var errorMustMatch = (overrideBits == expectedError); > + checkHostConnect(testHost[j], overrideBits, successExpected, errorMustMatch); > + } > + } > + //Now we test the self-signed. Must return an overridable untrusted error. Same here.

Fixed nits from v5 Thank you Honza and Brian. This now has r+ from both of you. I am now pushing it to try and once it passes I will mark it for chekin.

Comment on attachment 677044 [details] [diff] [review] test-certoverrides v6 https://hg.mozilla.org/integration/mozilla-inbound/rev/2fb35fde527d

Is there anything specific that we can do here to verify this? See comment 52.

Marking as [qa-] since there are still no answers for comment 52, and this bug fix does have an automated test verifying it.