Garret Robinson notices that going to https://kuix.de:5146 fives sec_error_ocsp_unauthorized_request instead of sec_error_expired certificate. I checked again and on nightly on linux(debug) getting an intermetent error. On OSX the error is on nightly (34) and aurora(33) but not on beta (32).

Hg blame + Manual checks lead me to say that this was due to changeset c21ea604d839 ( bug 1033563) part 1: https://bugzilla.mozilla.org/attachment.cgi?id=8450013

Keeler assigned to you as you r+ the change. Please let me know if you differ on my diagnosis.

Clarifiation By manual check I mean I did a couple of builds and went to the site above OK on revision 911d02f2c02a (error expired) not OK on revision c21ea604d839 (error ocsp)

This is almost definitely caused by part 1 of bug 1033563. I can look at this next week.

I spent a little time investigating this to see if it would result in a major change. It seems like the change is very simple, but I haven't written a test for it. See this attachment; with this patch, I can add a cert error override for https://kuix.de:5146/.

[Tracking Requested - why for this release]:

David, are you planning to propose a fix this bug in 33? Beta 6 goes to build today. Thanks

Well, Brian wrote this patch, so he should probably drive this. FWIW, I had a look and the patch is probably fine as-is. It just needs a test or two.

I agree my patch is great, but I probably won't have time to write tests for this within the Firefox 33 timeframe.

This tests the patch you wrote. Without your patch, it fails in CheckRevocation. With the patch, it passes.

David, Brian, Beta 8 go to build is today. Are you planning to land these changes today? Thanks

Comment on attachment 8494677 [details] [diff] [review] test Let's see if we can get this reviewed and landed today.

Comment on attachment 8494677 [details] [diff] [review] test Review of attachment 8494677 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/pkix/test/gtest/pkixbuild_tests.cpp @@ +300,5 @@ > nullptr/*stapledOCSPResponse*/)); > } > } > + > +class ExpiredCertTrustDomain : public TrustDomain This class needs a comment, like "Mock class initialized with a root DER that is only used in GetCertTrust." @@ +308,5 @@ > + { > + EXPECT_EQ(Success, rootCert.Init(rootDER.data(), rootDER.length())); > + } > + > + virtual Result GetCertTrust(EndEntityOrCA, const CertPolicyId&, Are these unused params because of the superclass? At least name them EndEntityOrCA ignoredEE or something. @@ +323,5 @@ > + > + virtual Result FindIssuer(Input encodedIssuerName, > + IssuerChecker& checker, Time time) > + { > + bool keepGoing; explicitly initialize to false? @@ +368,5 @@ > +{ > + const char* rootCN = "Root CA"; > + ScopedTestKeyPair rootKey; > + ByteString rootDER(CreateCert(rootCN, rootCN, EndEntityOrCA::MustBeCA, > + nullptr, rootKey, nullptr)); safer to check CreateCert == SECSuccess before converting to ByteString, or is that already handled in the conversion? @@ +378,5 @@ > + ByteString issuerDER(CNToDERName(rootCN)); > + EXPECT_NE(ENCODING_FAILED, issuerDER); > + ByteString subjectDER(CNToDERName("Expired End-Entity Cert")); > + EXPECT_NE(ENCODING_FAILED, subjectDER); > + ByteString extensions[2]; It looks like this is ignored if it is null. Are these extensions being used implicitly in the test below? If not, maybe better to pass nullptr.

(In reply to [:mmc] Monica Chew (please use needinfo) from comment #14) Thanks for the review. I've addressed most of your comments with comments in the patch. Unfortunately, now I'm getting gtest failures on windows, so I'm looking into that. https://tbpl.mozilla.org/?tree=Try&rev=787ce0cf396c > @@ +368,5 @@ > > +{ > > + const char* rootCN = "Root CA"; > > + ScopedTestKeyPair rootKey; > > + ByteString rootDER(CreateCert(rootCN, rootCN, EndEntityOrCA::MustBeCA, > > + nullptr, rootKey, nullptr)); > > safer to check CreateCert == SECSuccess before converting to ByteString, or > is that already handled in the conversion? There's no conversion here - CreateCert returns a ByteString. Unless I'm misunderstanding your comment? > @@ +378,5 @@ > > + ByteString issuerDER(CNToDERName(rootCN)); > > + EXPECT_NE(ENCODING_FAILED, issuerDER); > > + ByteString subjectDER(CNToDERName("Expired End-Entity Cert")); > > + EXPECT_NE(ENCODING_FAILED, subjectDER); > > + ByteString extensions[2]; > > It looks like this is ignored if it is null. Are these extensions being used > implicitly in the test below? If not, maybe better to pass nullptr. So it seems. I changed this to nullptr, but I'm getting BAD_DER failures. I'm wondering if it's related to this...

In that case, please feel free to ignore the comment about extensios since we're trying to get this into beta.

Nuts - that wasn't it anyway: https://tbpl.mozilla.org/?tree=Try&rev=cb74aedca078 I'll have to figure this out tomorrow.

Ok - looks like I figured it out: https://tbpl.mozilla.org/?tree=Try&rev=31a4cee56f2b (For future reference, having Inputs as member variables doesn't work very well, since they can't be re-used. I'm not sure why there would be a platform-specific difference, but oh well.) https://hg.mozilla.org/integration/mozilla-inbound/rev/f7d98f2e35fd https://hg.mozilla.org/integration/mozilla-inbound/rev/165c3fd176ec

https://hg.mozilla.org/mozilla-central/rev/f7d98f2e35fd https://hg.mozilla.org/mozilla-central/rev/165c3fd176ec

Approval Request Comment [Feature/regressing bug #]: mozilla::pkix refactoring [User impact if declined]: some sites with expired certificates will no longer be browsable (it basically depends on if the CA that issued the certificate is still keeping track of it) [Describe test coverage new/current, TBPL]: has a test (next patch) [Risks and why]: this patch is fairly small, so there shouldn't be much risk [String/UUID change made/needed]: none

Approval Request Comment (see previous comment - this patch is the test for this fix)

Comment on attachment 8499077 [details] [diff] [review] patch 1/2 for beta Approved to decrease the breakage in term of certificates support. David, any reason you didn't ask for an aurora uplift?

https://hg.mozilla.org/releases/mozilla-beta/rev/d8ebbb47a75e https://hg.mozilla.org/releases/mozilla-beta/rev/9150826eaf1b

(In reply to Sylvestre Ledru [:sylvestre] from comment #22) > Comment on attachment 8499077 [details] [diff] [review] > patch 1/2 for beta > > Approved to decrease the breakage in term of certificates support. > > David, any reason you didn't ask for an aurora uplift? Thanks, Sylvestre. I'm still working on an aurora-specific patch, but I wanted to get the beta patch out the door as soon as it was ready, since that's more urgent.

David - Can you please get the Aurora patch completed and nommed this week while 34 is still on Aurora?

Comment on attachment 8473185 [details] [diff] [review] do-not-check-revocation-for-expired-certs.patch Approval Request Comment (same as comment 20) This applies on aurora, so no branch-specific patch needed in this case. The test does need an aurora-specific patch, which I'll be uploading shortly.

Approval Request Comment (see previous comment - this is the accompanying test patch)

Comment on attachment 8473185 [details] [diff] [review] do-not-check-revocation-for-expired-certs.patch Thanks. Aurora+

https://hg.mozilla.org/releases/mozilla-aurora/rev/5f8dea9b01ac https://hg.mozilla.org/releases/mozilla-aurora/rev/013c6a8663ab

Backed out for gcc 4.4 bustage. https://hg.mozilla.org/releases/mozilla-aurora/rev/49bd5bad84d2 https://treeherder.mozilla.org/ui/logviewer.html#?job_id=303716&repo=mozilla-aurora

I think I fixed this: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=78b6f7d474cf So: https://hg.mozilla.org/releases/mozilla-aurora/rev/db731c467d75 https://hg.mozilla.org/releases/mozilla-aurora/rev/3e7f5082d9ec

For posterity.

Thanks :)