The following root certificate has expired, so please remove EV treatment for it. CN = Buypass Class 3 CA 1 O = Buypass AS-983163327 C = NO SHA-1 Fingerprint: 61:57:3A:11:DF:0E:D8:7E:D5:92:65:22:EA:D0:56:D7:44:B3:23:71

Bug 1164609 - Remove EV treatment for expired Buypass Class 3 CA 1 root certificate. Also restores assert that appears to have been mistakenly commented out in https://hg.mozilla.org/mozilla-central/rev/1dc5cbdf0dd8#l4.12 .

Due to the commented out assert, this is just a soft-blocker to Bug 1190794, but it doesn't hurt to add the relation I guess. https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c9fd8b0916c

(In reply to Cykesiopka from comment #1) > Also restores assert that appears to have been mistakenly commented out in > https://hg.mozilla.org/mozilla-central/rev/1dc5cbdf0dd8#l4.12 . I'm not sure it was mistakenly commented out. Please check with Keeler.

(In reply to Kathleen Wilson from comment #3) > (In reply to Cykesiopka from comment #1) > > Also restores assert that appears to have been mistakenly commented out in > > https://hg.mozilla.org/mozilla-central/rev/1dc5cbdf0dd8#l4.12 . > > I'm not sure it was mistakenly commented out. Please check with Keeler. It turns out it was. The behavior we want is for it to fail on debug builds so we know about it if we make a mistake changing the EV list. It will continue to work without failing on release builds.

Comment on attachment 8643549 [details] MozReview Request: Bug 1164609 - Remove EV treatment for expired Buypass Class 3 CA 1 root certificate. https://reviewboard.mozilla.org/r/15111/#review13609 Great - r=me. ::: security/certverifier/ExtendedValidation.cpp:1241 (Diff revision 1) > // version of system NSS is installed). We will just silently avoid This comment should be updated to reflect that we have a debug assertion but that we silently continue on release builds. ::: security/certverifier/ExtendedValidation.cpp:1251 (Diff revision 1) > - //PR_NOT_REACHED("Could not find EV root in NSS storage"); > + PR_NOT_REACHED("Could not find EV root in NSS storage"); It probably is a good idea to have this so we catch any mistakes we might make when modifying the EV root list. However, I think MOZ_ASSERT(true, ...) might be more appropriate than PR_NOT_REACHED. Also it's distressing that this got commented-out at all, but that's a completely different issue.

(In reply to David Keeler [:keeler] (use needinfo?) from comment #5) > However, I think MOZ_ASSERT(true, ...) > might be more appropriate than PR_NOT_REACHED. I assume you mean MOZ_ASSERT(false, ...)? I'm also unsure what the practical difference between MOZ_ASSERT and PR_NOT_REACHED is - they both seem to do the same thing. Is it just a "don't use PR_NOT_REACHED in new code" thing? (My searches for this information came up empty, sorry!)

(In reply to Cykesiopka from comment #6) > (In reply to David Keeler [:keeler] (use needinfo?) from comment #5) > > However, I think MOZ_ASSERT(true, ...) > > might be more appropriate than PR_NOT_REACHED. > > I assume you mean MOZ_ASSERT(false, ...)? Whoops :) Yes, that's what I meant. > I'm also unsure what the practical difference between MOZ_ASSERT and > PR_NOT_REACHED is - they both seem to do the same thing. Is it just a "don't > use PR_NOT_REACHED in new code" thing? > (My searches for this information came up empty, sorry!) Yeah, I guess they're roughly the same. My thought was that PR_NOT_REACHED indicates that that code is logically unreachable in a way that the compiler can't necessarily deduce, but I think it commonly is used interchangeably with PR_ASSERT or MOZ_ASSERT. In any case, I contacted the developer who accidentally commented out the line and we're reverting that change in bug 1166323, so changing the PR_NOT_REACHED to a MOZ_ASSERT seems less important now (although you're welcome to if you want).

(In reply to David Keeler [:keeler] (use needinfo?) from comment #7) > Yeah, I guess they're roughly the same. My thought was that PR_NOT_REACHED > indicates that that code is logically unreachable in a way that the compiler > can't necessarily deduce, but I think it commonly is used interchangeably > with PR_ASSERT or MOZ_ASSERT. I see, thanks for the explanation. > In any case, I contacted the developer who accidentally commented out the > line and we're reverting that change in bug 1166323, so changing the > PR_NOT_REACHED to a MOZ_ASSERT seems less important now (although you're > welcome to if you want). OK, I'll leave the assert alone then.

+ Update comment to note that debug builds assert, release builds silently continue - Revert assert uncommenting change

Thanks for the review. (Try push is in comment 2)