STR: visit a site with an EV certificate (e.g. bugzilla.mozilla.org) and then shift-refresh (clearing the cache). Expected results: the extended green EV bar should still be displayed Actual results: the EV indicator falls back to the DV indicator I bisected this to changeset 4adb7daf5033 from bug 1264562.

Comment on attachment 8806508 [details] bug 1313491 - add basic tests that PSM sets the right security state during session resumption https://reviewboard.mozilla.org/r/89904/#review90138 ::: security/manager/ssl/tests/unit/test_session_resumption.js:49 (Diff revision 1) > + ok(!sslStatus.isUntrusted, > + "expired.example.com should not have isUntrusted set"); > + } > + ); > + > + // This test is similar, except with extended validation. Could you maybe explain a little better exactly what the EV part of the test achieves? The two add_connection_test calls look basically the same. It's wasn't clear to me on reading this part of the test what was being tested.

Comment on attachment 8806507 [details] bug 1313491 - include more context when determining EV status https://reviewboard.mozilla.org/r/89868/#review90166

Comment on attachment 8806507 [details] bug 1313491 - include more context when determining EV status https://reviewboard.mozilla.org/r/89868/#review90404 Cool, much nicer! ::: security/manager/ssl/SSLServerCertVerification.cpp:1375 (Diff revision 1) > + RememberCertErrorsTable::GetInstance().RememberCertHasError(infoObject, > + nullptr, > + SECSuccess); Follow-up material: Might be nice to create a new method dedicated to clearing cert errors, since IMO it's confusing that method named `RememberCertHasError()` can actually clear an existing error. We should probably also rename `LookupCertErrorBits()` to `UpdateCertErrorBits()` or something. ::: security/manager/ssl/SSLServerCertVerification.cpp:1385 (Diff revision 1) > certificateTransparencyInfo); > > // The connection may get terminated, for example, if the server requires > // a client cert. Let's provide a minimal SSLStatus > // to the caller that contains at least the cert and its status. > + RefPtr<nsSSLStatus> status(infoObject->SSLStatus()); A prexisting issue, but it looks like this file is actually missing a few includes: ```cpp #include "nsSSLStatus.h" #include "nsNSSCertificate.h" #include "mozilla/RefPtr.h" #include "SharedCertVerifier.h" #include "TransportSecurityInfo.h" // For RememberCertErrorsTable ``` ::: security/manager/ssl/SSLServerCertVerification.cpp:1402 (Diff revision 1) > } > > + RefPtr<nsNSSCertificate> nsc = nsNSSCertificate::Create(cert.get()); > status->SetServerCert(nsc, evStatus); > MOZ_LOG(gPIPNSSLog, LogLevel::Debug, > - ("AuthCertificate setting NEW cert %p\n", nsc.get())); > + ("AuthCertificate setting NEW cert %p\n", nsc.get())); Nit: Might as well get rid of the unnecessary \n while we're touching this line. ::: security/manager/ssl/nsNSSCallbacks.cpp:1106 (Diff revision 1) > + > + if (!sslStatus || !fd || !infoObject) { > + return; > + } > + > + UniqueCERTCertificate cert(SSL_PeerCertificate(fd)); Seems like this deserves an assert and null check. ::: security/manager/ssl/nsNSSCallbacks.cpp:1108 (Diff revision 1) > + return; > + } > + > + UniqueCERTCertificate cert(SSL_PeerCertificate(fd)); > + > + RefPtr<SharedCertVerifier> certVerifier(GetDefaultCertVerifier()); A prexisting issue, but it looks like this file is actually missing a few includes: ```cpp #include "mozilla/RefPtr.h" #include "SharedCertVerifier.h" #include "nsNSSCertificate.h" ``` ::: security/manager/ssl/nsNSSCallbacks.cpp:1116 (Diff revision 1) > + if (!certVerifier) { > + return; > + } > + > + // We don't own these pointers. > + const SECItemArray* csa = SSL_PeerStapledOCSPResponses(fd); Nit: I guess this was copied from from AuthCertificateHook(), but maybe we could name `csa` something else? At least for me it's not a really intuitive name. ::: security/manager/ssl/nsNSSCallbacks.cpp:1156 (Diff revision 1) > + &evOidPolicy); > + > + RefPtr<nsNSSCertificate> nssc(nsNSSCertificate::Create(cert.get())); > + if (rv == Success && evOidPolicy != SEC_OID_UNKNOWN) { > + MOZ_LOG(gPIPNSSLog, LogLevel::Debug, > + ("HandshakeCallback using NEW cert %p (is EV)\n", nssc.get())); Nit: Get rid of the unnecessary \n. Same thing below. ::: security/manager/ssl/nsNSSCertificate.h:42 (Diff revision 1) > > friend class nsNSSCertificateFakeTransport; > > - explicit nsNSSCertificate(CERTCertificate* cert, SECOidTag* evOidPolicy = nullptr); > + explicit nsNSSCertificate(CERTCertificate* cert); > nsNSSCertificate(); > - static nsNSSCertificate* Create(CERTCertificate*cert = nullptr, > + static nsNSSCertificate* Create(CERTCertificate*cert = nullptr); Nit: Space after *. ::: security/manager/ssl/nsSSLStatus.cpp:13 (Diff revision 1) > #include "nsIClassInfoImpl.h" > -#include "nsIObjectOutputStream.h" > #include "nsIObjectInputStream.h" > +#include "nsIObjectOutputStream.h" > +#include "nsNSSCertificate.h" > +#include "plstr.h" I think we can get rid of this include now.

Comment on attachment 8806508 [details] bug 1313491 - add basic tests that PSM sets the right security state during session resumption https://reviewboard.mozilla.org/r/89904/#review90406 LGTM. ::: security/manager/ssl/tests/unit/bad_certs/ev-test-intermediate.pem.certspec:6 (Diff revision 1) > issuer:evroot > -subject:anyPolicy-int-path-int > +subject:ev-test-intermediate > issuerKey:ev > extension:basicConstraints:cA, > extension:keyUsage:cRLSign,keyCertSign > -extension:authorityInformationAccess:http://www.example.com:8888/anyPolicy-int-path-int/ > +extension:authorityInformationAccess:http://localhost:8888/ev-test-intermediate/ Hmm, why the www.example.com -> localhost change? Is it to save us having to set the "network.dns.localDomains" pref? ::: security/manager/ssl/tests/unit/test_session_resumption.js:60 (Diff revision 1) > + add_connection_test("ev-test.example.com", PRErrorCodeSuccess, null, > + (transportSecurityInfo) => { > + ok(!(transportSecurityInfo.securityState & > + Ci.nsIWebProgressListener.STATE_CERT_USER_OVERRIDDEN), > + "ev-test.example.com should not have STATE_CERT_USER_OVERRIDDEN flag"); > + let sslStatus = transportSecurityInfo > + .QueryInterface(Ci.nsISSLStatusProvider) > + .SSLStatus; > + ok(!sslStatus.isDomainMismatch, > + "ev-test.example.com should not have isDomainMismatch set"); > + ok(!sslStatus.isNotValidAtThisTime, > + "ev-test.example.com should not have isNotValidAtThisTime set"); > + ok(!sslStatus.isUntrusted, > + "ev-test.example.com should not have isUntrusted set"); > + ok(!gEVExpected || sslStatus.isExtendedValidation, > + "ev-test.example.com should have isExtendedValidation set " + > + "(or this is a non-debug build)"); > + } > + ); Nit: It might be clearer to dedupe this block of code and the one above to make it clearer we expect the exact same results.

Comment on attachment 8806507 [details] bug 1313491 - include more context when determining EV status https://reviewboard.mozilla.org/r/89868/#review90940 This looks all right and proper to me.

Comment on attachment 8806508 [details] bug 1313491 - add basic tests that PSM sets the right security state during session resumption https://reviewboard.mozilla.org/r/89904/#review91028 ::: security/manager/ssl/tests/unit/test_session_resumption.js:49 (Diff revision 1) > + ok(!sslStatus.isUntrusted, > + "expired.example.com should not have isUntrusted set"); > + } > + ); > + > + // This test is similar, except with extended validation. Maybe it'd be clearer to split `run_test()` into two tests: `run_resume_non_ev_with_override()` and `run_resume_ev()` ?

Comment on attachment 8806507 [details] bug 1313491 - include more context when determining EV status https://reviewboard.mozilla.org/r/89868/#review90404 Thanks for the reviews, all! > Follow-up material: Might be nice to create a new method dedicated to clearing cert errors, since IMO it's confusing that method named `RememberCertHasError()` can actually clear an existing error. > > We should probably also rename `LookupCertErrorBits()` to `UpdateCertErrorBits()` or something. Sure - filed bug 1316070. > A prexisting issue, but it looks like this file is actually missing a few includes: > ```cpp > #include "nsSSLStatus.h" > #include "nsNSSCertificate.h" > #include "mozilla/RefPtr.h" > #include "SharedCertVerifier.h" > #include "TransportSecurityInfo.h" // For RememberCertErrorsTable > ``` Ok - added. > Nit: Might as well get rid of the unnecessary \n while we're touching this line. Sounds good. > Seems like this deserves an assert and null check. Good call. > A prexisting issue, but it looks like this file is actually missing a few includes: > ```cpp > #include "mozilla/RefPtr.h" > #include "SharedCertVerifier.h" > #include "nsNSSCertificate.h" > ``` Ok - added (also sorted). > Nit: I guess this was copied from from AuthCertificateHook(), but maybe we could name `csa` something else? At least for me it's not a really intuitive name. I can't remember why I called it that, but yeah, it's not a great name. > Nit: Get rid of the unnecessary \n. Same thing below. Sounds good. > Nit: Space after *. Fixed. > I think we can get rid of this include now. Sounds good.

Comment on attachment 8806508 [details] bug 1313491 - add basic tests that PSM sets the right security state during session resumption https://reviewboard.mozilla.org/r/89904/#review90138 > Could you maybe explain a little better exactly what the EV part of the test achieves? The two add_connection_test calls look basically the same. It's wasn't clear to me on reading this part of the test what was being tested. I added more documentation in addition to refactoring the code a bit.

Comment on attachment 8806508 [details] bug 1313491 - add basic tests that PSM sets the right security state during session resumption https://reviewboard.mozilla.org/r/89904/#review90406 > Hmm, why the www.example.com -> localhost change? Is it to save us having to set the "network.dns.localDomains" pref? Yes - unfortunately add_connection_test *sets* "network.dns.localDomains" rather than appending to it. That needs fixing, but I was concerned about making too many changes with this patch, so I'd rather address that as a follow-up. I filed bug 1316076 for this. > Nit: It might be clearer to dedupe this block of code and the one above to make it clearer we expect the exact same results. Good call.

Pushed by dkeeler@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1c76db819ee6 include more context when determining EV status r=Cykesiopka,jcj,mgoodwin https://hg.mozilla.org/integration/autoland/rev/bd9fb3865606 add basic tests that PSM sets the right security state during session resumption r=Cykesiopka,jcj,mgoodwin

https://hg.mozilla.org/mozilla-central/rev/1c76db819ee6 https://hg.mozilla.org/mozilla-central/rev/bd9fb3865606