Once OCSP get capabilities exist, NSSTrustDomain should allow OCSP POST fallback when the data received via GET is not authoritative (such as bad/empty/null/old response).

From Brian Smith: == Begin Quote == Besides testing this, we also need to test the interaction of the code with the HTTP cache: 1. Do we cache Good, Revoked, and Unknown responses in the HTTP cache? 2. Do we use Good responses from the HTTP cache? 3. When we've cached an Unknown (and Revoked?) response in the HTTP cache, do we fall back to HTTP POST? If/when the HTTP POST fails, do we continue to use the cached response? 4. Do we properly handle *expired* (according to thisUpdate/nextUpdate) responses that are still considered fresh according to HTTP? 5. Do we properly handle otherwise bad (e.g. OCSP response is not for the certificate in question) OCSP responses that are in the HTTP cache from previous requests? 6. When we fallback to POST, do we properly bypass the HTTP cache? We didn't turn OCSP GET on in Gecko back when it was added to NSS because we were missing the cache interaction tests I mentioned above. In particular, #3 and #4 are very important to ensure that we don't allow anybody to block our users from updating Firefox and/or addons by poisoning our HTTP cache with bad OCSP responses for our AUS and AMO servers. ==End Quote == We need to ensure we test 3 and 4. My initial tests show that when using mozilla::pkix we dont use the local HTTP cache.

Comment on attachment 8441448 [details] [diff] [review] add-post-fallback-ocsp-get1 Review of attachment 8441448 [details] [diff] [review]: ----------------------------------------------------------------- Looks like this will verify the OCSP response twice - this is technically a race condition that we want to avoid. Have you written some tests? I think it would be good to see those as well. Rather than have a separate OCSPGetIsAuthoritative function, CheckRevocation might have to look something like this: if (stapledResponse) { // attempt to verify/cache/use stapledResponse } if (cachedResponse) { // attempt to use cachedResponse } if (useGET) { // attempt to verify/cache/use response from GET (keep track of time elapsed) } // attempt to verify/cache/use response from POST ::: security/certverifier/NSSCertDBTrustDomain.cpp @@ +182,5 @@ > + CERTCertificate* issuerCert, PRTime time, > + uint16_t maxLifetimeInDays, > + TrustDomain& trustDomain, > + OCSPCache& aOCSPCache, > + /*out*/ SECStatus *srv) { nit: SECStatus* srv Also (and this isn't a big deal), maybe line up the last two lines more like this: OCSPCache& aOCSPCache, /*out*/ SECStatus *srv) { @@ +192,5 @@ > + } > + PRTime thisUpdate = 0; > + PRTime validThrough = 0; > + > + SECStatus rv = VerifyEncodedOCSPResponse(trustDomain, cert, issuerCert, time, Shouldn't this just use VerifyAndMaybeCacheEncodedOCSPResponse? @@ +197,5 @@ > + maxLifetimeInDays, response, > + &thisUpdate, &validThrough); > + PRErrorCode error = (rv == SECSuccess ? 0 : PR_GetError()); > + // If success or SEC_ERROR_REVOKED_CERTIFICATE or SEC_ERROR_OCSP_UNKNOWN_CERT > + // then the response is authoritative But it could also be an expired revoked response, right? I think it would be good to rebase this patch on top of the one in bug 997509 (which is almost ready to land - I just need to fix a windows test failure). @@ +204,5 @@ > + error == SEC_ERROR_OCSP_UNKNOWN_CERT) { > + if (aOCSPCache.Put(cert, issuerCert, error, thisUpdate, validThrough) > + != SECSuccess) { > + *srv = SECFailure; > + return true; // Dont keep trying? Failure here is probably only an out-of-memory error, so we don't want to keep trying to do anything. @@ +397,5 @@ > if (cachedResponseErrorCode == 0 || > cachedResponseErrorCode == SEC_ERROR_OCSP_UNKNOWN_CERT || > cachedResponseErrorCode == SEC_ERROR_OCSP_OLD_RESPONSE) { > + PRIntervalTime elapsed = 0; > + PRIntervalTime timeout = OCSPFetchingTypeToTimeoutTime(mOCSPFetching); nit: declare these closer to where they're used @@ +407,5 @@ > + if (mOCSPGetConfig == CertVerifier::ocsp_get_enabled) { > + PRIntervalTime startTime = PR_IntervalNow(); > + SECStatus rv; > + bool OCSPGetAttemptIsAuthoritative = > + OCSPGetIsAuthorative(arena.get(), url.get(), request, timeout, cert, If this were a member function you wouldn't have to pass around *this or mOCSPCache

(In reply to David Keeler (:keeler) [use needinfo?] from comment #3) > Comment on attachment 8441448 [details] [diff] [review] > add-post-fallback-ocsp-get1 > > Review of attachment 8441448 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks like this will verify the OCSP response twice - this is technically a > race condition that we want to avoid. We are verifying the GET and the POST response in the assumption that they are different. (because of intermediare caches/bad OCSP responder implementations). I dont consider this a race condition. > Have you written some tests? I think it would be good to see those as well. There is some coverage when using test_ocsp_method. I have another patch that enables ocsp get and many tests will need to be fixed, I was planning to make the tests part of the enable OCSP get bug. But lets have the fixes ready to go now. > Rather than have a separate OCSPGetIsAuthoritative function, CheckRevocation > might have to look something like this: > > if (stapledResponse) { > // attempt to verify/cache/use stapledResponse > } > if (cachedResponse) { > // attempt to use cachedResponse > } > if (useGET) { > // attempt to verify/cache/use response from GET (keep track of time > elapsed) > } > // attempt to verify/cache/use response from POST The logic for caching need to be different between GET and POST as in the case for GET old responses MUST not be cached but allow the logic to fallback. The CSPGetAttemptIsAuthoritative is basically a way to simplify the reading of the code as the structure is basically what you say here: Once it is decided that a network connection is done then: if(useGET) { try use GET } Then fallback to the exact same logic as before. One of my initial patches had rewritten the logic, but that was much harder to parse and that is why I did it this way. > > ::: security/certverifier/NSSCertDBTrustDomain.cpp > @@ +182,5 @@ > > + CERTCertificate* issuerCert, PRTime time, > > + uint16_t maxLifetimeInDays, > > + TrustDomain& trustDomain, > > + OCSPCache& aOCSPCache, > > + /*out*/ SECStatus *srv) { > > nit: SECStatus* srv > Also (and this isn't a big deal), maybe line up the last two lines more like > this: > > OCSPCache& aOCSPCache, > /*out*/ SECStatus *srv) { > > @@ +192,5 @@ > > + } > > + PRTime thisUpdate = 0; > > + PRTime validThrough = 0; > > + > > + SECStatus rv = VerifyEncodedOCSPResponse(trustDomain, cert, issuerCert, time, > > Shouldn't this just use VerifyAndMaybeCacheEncodedOCSPResponse? No, because it always tries to cache the response. When using get with fallback we want to only cache authoritative responses and cache other responses only on the fallback case. > > @@ +197,5 @@ > > + maxLifetimeInDays, response, > > + &thisUpdate, &validThrough); > > + PRErrorCode error = (rv == SECSuccess ? 0 : PR_GetError()); > > + // If success or SEC_ERROR_REVOKED_CERTIFICATE or SEC_ERROR_OCSP_UNKNOWN_CERT > > + // then the response is authoritative > > But it could also be an expired revoked response, right? I think it would be > good to rebase this patch on top of the one in bug 997509 (which is almost > ready to land - I just need to fix a windows test failure). Agreed. > > @@ +204,5 @@ > > + error == SEC_ERROR_OCSP_UNKNOWN_CERT) { > > + if (aOCSPCache.Put(cert, issuerCert, error, thisUpdate, validThrough) > > + != SECSuccess) { > > + *srv = SECFailure; > > + return true; // Dont keep trying? > > Failure here is probably only an out-of-memory error, so we don't want to > keep trying to do anything. > > @@ +397,5 @@ > > if (cachedResponseErrorCode == 0 || > > cachedResponseErrorCode == SEC_ERROR_OCSP_UNKNOWN_CERT || > > cachedResponseErrorCode == SEC_ERROR_OCSP_OLD_RESPONSE) { > > + PRIntervalTime elapsed = 0; > > + PRIntervalTime timeout = OCSPFetchingTypeToTimeoutTime(mOCSPFetching); > > nit: declare these closer to where they're used > > @@ +407,5 @@ > > + if (mOCSPGetConfig == CertVerifier::ocsp_get_enabled) { > > + PRIntervalTime startTime = PR_IntervalNow(); > > + SECStatus rv; > > + bool OCSPGetAttemptIsAuthoritative = > > + OCSPGetIsAuthorative(arena.get(), url.get(), request, timeout, cert, > > If this were a member function you wouldn't have to pass around *this or > mOCSPCache OK lets make this function a member.

(In reply to Camilo Viecco (:cviecco) from comment #4) > > Have you written some tests? I think it would be good to see those as well. > > There is some coverage when using test_ocsp_method. I have another patch that > enables ocsp get and many tests will need to be fixed, I was planning to > make the > tests part of the enable OCSP get bug. But lets have the fixes ready to go > now. I strongly disagree with this. The code and tests should land together. One of the purposes of the testing requirements we have is to ensure that we've verified the correct implementation of the code to a reasonable extent before we even ask for review, so that we maximize the reviewers' time efficiency--i.e. it's a form of self-review. The other purpose is to ensure that we don't end up maintaining broken code. But, if the code lands pref'd off without tests then we're forced to maintain it whether or not it's correct, which sucks.

Hi brian I agree with you. When I said 'lets have the fixes ready to go > now.' I meant lets land this with tests. But, if the code lands pref'd > off without tests then we're forced to maintain it whether or not it's > correct, which sucks. Agreed.

Comment on attachment 8445914 [details] [diff] [review] add-post-fallback-ocsp-get1 (v2) Review of attachment 8445914 [details] [diff] [review]: ----------------------------------------------------------------- Unfortunately, there's an issue in terms of dealing with expired responses (see the third comment). r- for now since we don't want to regress functionality in that sense. ::: security/certverifier/NSSCertDBTrustDomain.cpp @@ +174,5 @@ > return PR_SecondsToInterval(2); > } > > +bool > +NSSCertDBTrustDomain::OCSPGetIsAuthorative(PLArenaPool* arena, How about something like "TryOCSPUsingGET"? Otherwise the name isn't really descriptive in the sense that this function could cause network i/o. @@ +181,5 @@ > + PRIntervalTime timeout, > + const mozilla::pkix::CertID& certID, > + PRTime time, > + uint16_t maxLifetimeInDays, > + /*out*/ SECStatus* srv) { Outputs should be initialized to safe values in case of early return. @@ +196,5 @@ > + maxLifetimeInDays, *response, > + expired, &thisUpdate, &validThrough); > + PRErrorCode error = (rv == SECSuccess ? 0 : PR_GetError()); > + // If success or SEC_ERROR_REVOKED_CERTIFICATE or SEC_ERROR_OCSP_UNKNOWN_CERT > + // then the response is authoritative But not if it's expired, right? The problem is if there's a cached expired Revoked or Unknown response from GET, we want to attempt a POST. But, if that fails, then we want to use the Revoked/Unknown response. @@ +200,5 @@ > + // then the response is authoritative > + PR_LOG(gCertVerifierLog, PR_LOG_DEBUG, > + ("NSSCertDBTrustDomain: OCSP GET error=%d", error)); > + if (rv == SECSuccess || > + ( thisUpdate != 0 && ( Why would thisUpdate be 0? Also, there shouldn't be a space after the opening "(" and I think the trailing "(" should be with the statement it encloses. @@ +206,5 @@ > + error == SEC_ERROR_OCSP_UNKNOWN_CERT))) { > + if (mOCSPCache.Put(certID, error, thisUpdate, validThrough) > + != SECSuccess) { > + *srv = SECFailure; > + return true; // Dont keep trying? A comment like "// This only fails catastrophically. Don't fall back to POST." would be more informative. @@ +473,5 @@ > + > + if (mOCSPGetConfig == CertVerifier::ocsp_get_enabled) { > + PRIntervalTime startTime = PR_IntervalNow(); > + SECStatus rv; > + bool OCSPGetAttemptIsAuthoritative = nit: variables start with lowercase, so something like "ocspGetAttemptIsAuthoritative"

Comment on attachment 8445917 [details] [diff] [review] fix-tests-p1 Review of attachment 8445917 [details] [diff] [review]: ----------------------------------------------------------------- What's here looks good. However, we still need tests involving GET responses and the http cache. Also, there are more ocsp tests - shouldn't they also be run in both modes? Additionally, there should be a test that exercises getting an expired Unknown/Revoked response from the http (GET) cache, attempting a POST that fails, and falling back to using the Unknown/Revoked response. r- for now. ::: security/manager/ssl/tests/unit/test_ocsp_caching.js @@ +24,5 @@ > ++gFetchCount; > > do_print("gFetchCount: " + gFetchCount); > > + if (gFetchCount != gSuccessAfter) { This setup has always been confusing to me. I think we should do something more like this: if (shouldServeGoodResponse(...)) { // serve good response } else { // serve 500 error } @@ +68,5 @@ > add_test(function() { do_check_eq(gFetchCount, 0); run_next_test(); }); > > // A failure to retrieve an OCSP response must result in the cached Unkown > // response being recognized and honored. > + let failCount = useOCSPGET ? 2 : 1; But it's not the number of failures, right? It's the number of requests. So maybe numExpectedRequests? ::: security/manager/ssl/tests/unit/test_ocsp_fetch_method.js @@ +57,5 @@ > // GET does fallback on bad entry > add_test(function() { > clearOCSPCache(); > Services.prefs.setBoolPref("security.OCSP.GET.enabled", true); > // Bug 1016681 mozilla::pkix does not support fallback yet. nit: outdated comment ::: security/manager/ssl/tests/unit/test_ocsp_required.js @@ +27,1 @@ > ocspResponder.registerPrefixHandler("/", function (request, response) { These handlers should probably check that each request is using the expected method (this applies to all of the tests modified by this patch). ::: security/manager/ssl/tests/unit/test_ocsp_stapling_expired.js @@ +65,5 @@ > + add_tests_in_mode(false); > + add_tests_in_mode(true); > + > + add_test(function () { ocspResponder.stop(run_next_test); }); > + add_test(check_ocsp_stapling_telemetry); You can just do ocspResponder.stop(check_ocsp_stapling_telemetry); @@ +68,5 @@ > + add_test(function () { ocspResponder.stop(run_next_test); }); > + add_test(check_ocsp_stapling_telemetry); > + run_next_test(); > + > +} nit: the blank line before this brace should be after it instead @@ +71,5 @@ > + > +} > +function add_tests_in_mode(useOCSPGET) { > + // This test assumes that OCSPStaplingServer uses the same cert for > + // ocsp-stapling-unknown.example.com and ocsp-stapling-none.example.com. More than that - it assumes all hosts use the same certificate. @@ +165,5 @@ > // These tests are verifying that an valid but very old response > // is rejected as a valid stapled response, requiring a fetch > // from the ocsp responder. > add_ocsp_test("ocsp-stapling-ancient-valid.example.com", Cr.NS_OK, > + ocspResponseGood, 1); This is inconsistent - set expectedRequestCount to 1 and use it here and below. ::: security/manager/ssl/tests/unit/test_ocsp_url.js @@ +96,5 @@ > add_test(function() { > clearOCSPCache(); > + let ocspResponder = {}; > + if (useOCSPGET) { > + // The path is depdendent on the cert, but we can test we validate This comment is unclear to me. @@ +97,5 @@ > clearOCSPCache(); > + let ocspResponder = {}; > + if (useOCSPGET) { > + // The path is depdendent on the cert, but we can test we validate > + ocspResponder = startOCSPResponder(SERVER_PORT, "www.example.com", [], Isn't this equivalent to start_ocsp_responder(["no-path-url"], []); ? @@ +98,5 @@ > + let ocspResponder = {}; > + if (useOCSPGET) { > + // The path is depdendent on the cert, but we can test we validate > + ocspResponder = startOCSPResponder(SERVER_PORT, "www.example.com", [], > + "test_ocsp_url", ["no-path-url"], []); nit: indentation

Comment on attachment 8445914 [details] [diff] [review] add-post-fallback-ocsp-get1 (v2) Review of attachment 8445914 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/certverifier/NSSCertDBTrustDomain.cpp @@ +196,5 @@ > + maxLifetimeInDays, *response, > + expired, &thisUpdate, &validThrough); > + PRErrorCode error = (rv == SECSuccess ? 0 : PR_GetError()); > + // If success or SEC_ERROR_REVOKED_CERTIFICATE or SEC_ERROR_OCSP_UNKNOWN_CERT > + // then the response is authoritative This comment is wrong. If it is revoked lets use it, no point if it expired or not. @@ +200,5 @@ > + // then the response is authoritative > + PR_LOG(gCertVerifierLog, PR_LOG_DEBUG, > + ("NSSCertDBTrustDomain: OCSP GET error=%d", error)); > + if (rv == SECSuccess || > + ( thisUpdate != 0 && ( verifyEncodedOCSP response error callback is very peculiar (from pkix/pkix.h): 14 // The optional parameter thisUpdate will be the thisUpdate value of 115 // the encoded response if it is considered trustworthy. Only 116 // good, unknown, or revoked responses that verify correctly are considered 117 // trustworthy. If the response is not trustworthy, thisUpdate will be 0. 118 // Similarly, the optional parameter validThrough will be the time through 119 // which the encoded response is considered trustworthy (that is, if a response had a 120 // thisUpdate time of validThrough, it would be considered trustworthy). so if I dont check for thisUpdate !=0 it sometimes fails with SEC_ERROR_OCSP_UNKNOWN_CERT when the error was caused by other issues (thanks testing). Since I want this to be only for authorative (trustworthy) responses I need to check. Also notice that I am not modifying (the baroque) logic of OCSP Post so that the fallback will work as expected.

(In reply to Camilo Viecco (:cviecco) from comment #11) > ::: security/certverifier/NSSCertDBTrustDomain.cpp > @@ +196,5 @@ > > + maxLifetimeInDays, *response, > > + expired, &thisUpdate, &validThrough); > > + PRErrorCode error = (rv == SECSuccess ? 0 : PR_GetError()); > > + // If success or SEC_ERROR_REVOKED_CERTIFICATE or SEC_ERROR_OCSP_UNKNOWN_CERT > > + // then the response is authoritative > > This comment is wrong. If it is revoked lets use it, no point if it expired > or not. It may be the case that there's an expired Unknown (or Revoked) response in the HTTP cache (locally). It may also be the case that the OCSP responder has a newer Good response. We should attempt to fetch that new response if we can. > @@ +200,5 @@ > 14 // The optional parameter thisUpdate will be the thisUpdate value of > 115 // the encoded response if it is considered trustworthy. Only > 116 // good, unknown, or revoked responses that verify correctly are > considered > 117 // trustworthy. If the response is not trustworthy, thisUpdate will be 0. > 118 // Similarly, the optional parameter validThrough will be the time > through > 119 // which the encoded response is considered trustworthy (that is, if a > response had a > 120 // thisUpdate time of validThrough, it would be considered trustworthy). > > so if I dont check for thisUpdate !=0 it sometimes fails with > SEC_ERROR_OCSP_UNKNOWN_CERT when the error was caused by other issues > (thanks testing). Since I want this to be only for authorative (trustworthy) > responses I need to check. Then that's a bug. thisUpdate should only be 0 if, for example, decoding failed or the signature verification failed (in which case the error set should not be SEC_ERROR_OCSP_UNKNOWN_CERT).

Comment on attachment 8462828 [details] [diff] [review] add-post-fallback-ocsp-get1 (v3.1) Review of attachment 8462828 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/certverifier/NSSCertDBTrustDomain.cpp @@ +252,5 @@ > return PR_SecondsToInterval(2); > } > > +bool > +NSSCertDBTrustDomain::OCSPGetAttemptIsAuthorative( This is the best name I could think of. @@ +284,5 @@ > + if (rv == SECSuccess || > + // thisUpdate != 0 => implies response verifies an the error > + // code returned is actually useful > + (thisUpdate != 0 && ( > + error == SEC_ERROR_REVOKED_CERTIFICATE || //revoked are OK to expired s/to/to be/

Comment on attachment 8462828 [details] [diff] [review] add-post-fallback-ocsp-get1 (v3.1) Review of attachment 8462828 [details] [diff] [review]: ----------------------------------------------------------------- Be sure to coordinate with Brian so as to not bitrot each other. In fact, I recommend rebasing this on top of the patch that expands the Result enum (it makes error handling a lot easier). I still have some concerns about this patch. I think this is the approach we need to take: * if GET is enabled, attempt to fetch a response * if this results in an actionable response, act on it (this is what the patch does, which is good) * if this results in an expired Revoked or Unknown response, make a note of this but fall back to POST * if POST fails, return Revoked or Unknown as appropriate (basically, see how the cache works and treat GET as essentially another cache) As messy as this function is getting, I think it'll actually be better if you add this functionality in-line, rather than in a separate subroutine. ::: security/certverifier/NSSCertDBTrustDomain.cpp @@ +272,5 @@ > + } > + PRTime thisUpdate = 0; > + PRTime validThrough = 0; > + bool expired; > + SECStatus rv = VerifyEncodedOCSPResponse(*this, certID, time, This means we potentially verify this response twice, right? @@ +281,5 @@ > + // then the response is authoritative > + PR_LOG(gCertVerifierLog, PR_LOG_DEBUG, > + ("NSSCertDBTrustDomain: OCSP GET error=%d", error)); > + if (rv == SECSuccess || > + // thisUpdate != 0 => implies response verifies an the error nit: leave out the "=>" Also, I think there's a typo in this comment, because it's not clear to me. @@ +283,5 @@ > + ("NSSCertDBTrustDomain: OCSP GET error=%d", error)); > + if (rv == SECSuccess || > + // thisUpdate != 0 => implies response verifies an the error > + // code returned is actually useful > + (thisUpdate != 0 && ( Again, use expired, not the value of thisUpdate @@ +284,5 @@ > + if (rv == SECSuccess || > + // thisUpdate != 0 => implies response verifies an the error > + // code returned is actually useful > + (thisUpdate != 0 && ( > + error == SEC_ERROR_REVOKED_CERTIFICATE || //revoked are OK to expired Actually, I disagree. If the Revoked response is expired, we should fall back to POST. If that fails, we should return SEC_ERROR_REVOKED_CERTIFICATE.

Comment on attachment 8463599 [details] [diff] [review] fix-tests-p1 (v2.1) Review of attachment 8463599 [details] [diff] [review]: ----------------------------------------------------------------- For the tests where it doesn't matter if OCSP GET is enabled, I think it's a good idea to turn it on to test it in advance of turning it on in production. However, it's also a good idea to test the configuration we're actually shipping, so I still think each OCSP test should run in both modes. r=me with comments addressed. ::: security/manager/ssl/tests/unit/test_ocsp_caching.js @@ +93,5 @@ > }); > add_connection_test("ocsp-stapling-none.example.com", Cr.NS_OK, > clearSessionCache); > + add_test(function() { > + do_check_eq(gFetchCount, numExpectedRequests + 1); run_next_test(); }); since numExpectedRequests is different in different cases, I would just do this: do_check_eq(gFetchCount, useOCSPGET ? 3 : 2); ::: security/manager/ssl/tests/unit/test_ocsp_get_fallback.js @@ +4,5 @@ > +"use strict"; > + > +// In which we try to validate several ocsp responses, checking in particular > +// that we use the specified method for fetching ocsp. We also check what > +// POST fallback when an invalid GET response is received. nit: this last sentence is unclear @@ +26,5 @@ > + return checkCertErrorGeneric(certdb, cert, expected_error, > + certificateUsageSSLServer); > +} > + > + nit: this second blank line is probably unnecessary @@ +38,5 @@ > + Services.prefs.setCharPref("network.dns.localDomains", > + "www.example.com"); > + Services.prefs.setBoolPref("security.OCSP.GET.enabled", true); > + > + // GET does fallback on an entry for a bad/incorrect cert Isn't this the same test as in test_ocsp_fetch_method.js? Maybe just remove it from that file. @@ +68,5 @@ > + check_cert_err("a", SEC_ERROR_OCSP_UNKNOWN_CERT); > + ocspResponder.stop(run_next_test); > + }); > + > + nit: extra blank line @@ +109,5 @@ > + check_cert_err("a", 0); > + ocspResponder.stop(run_next_test); > + }); > + > + nit: extra blank line @@ +115,5 @@ > +} > + > + > + > + Camilo - I would appreciate it if you would pay a little closer attention to details like not having four unnecessary blank lines at the end of files. ::: security/manager/ssl/tests/unit/test_ocsp_get_fallback/generate.py @@ +1,1 @@ > +#!/usr/bin/python Rather than generating yet more certificates, are there any that already exist in the tree that can be used for this test? ::: security/manager/ssl/tests/unit/test_ocsp_get_fallback/noise @@ +1,1 @@ > +1406240043.54 \ No newline at end of file this file shouldn't need to be checked in ::: security/manager/ssl/tests/unit/test_ocsp_get_fallback/pkcs11.txt @@ +1,1 @@ > +library= this file shouldn't be necessary if a certificate can be re-used instead of generating a whole new set ::: security/manager/ssl/tests/unit/test_ocsp_get_fallback/pwfile @@ +1,1 @@ > + this file shouldn't need to be checked in ::: security/manager/ssl/tests/unit/test_ocsp_stapling_expired.js @@ +12,5 @@ > let gCurrentOCSPResponse = null; > let gOCSPRequestCount = 0; > > +function add_ocsp_test(aHost, aExpectedResult, aOCSPResponseToServe, > + expectedRequestCount) { nit: let's be consistent about naming conventions in nearby code: aExpectedRequestCount ::: security/manager/ssl/tests/unit/test_ocsp_url.js @@ +97,5 @@ > clearOCSPCache(); > + let ocspResponder = {}; > + if (useOCSPGET) { > + // The path is depdendent on the cert, but we can test we validate > + ocspResponder = startOCSPResponder(SERVER_PORT, "www.example.com", [], When we generate the certificate, we can generate the expected request for it, right? In fact, that seems like a good thing to test/check. ::: security/manager/ssl/tests/unit/xpcshell.ini @@ +96,5 @@ > [test_add_preexisting_cert.js] > [test_keysize.js] > +[test_ocsp_get_fallback.js] > +run-sequentially = hardcoded ports > + nit: unnecessary blank line

Marking this as having nobody working on it, as that's true. The state of this bug is that we're happy to take an (updated) patch here, and that would let us turn on OCSP GET. If anyone's motivated to make that happen, go for it, and we'll happily review. Background: Having the OCSP POST fallback is a useful thing before we turn on OCSP GET, but it's unclear what the benefits are for doing the work to turn on OCSP GET. The motivation for using GET, better cache handling, seems to be eroding. I configured Let's Encrypt's CDN, for example, to cache responses to OCSP POSTs [1]. Other CDNs that I evaluated for Let's Encrypt also supported caching POSTs if you ticked the appropriate boxes [2], so other OCSP infrastructures likely do it or will do it soon. (IMO, let's spend our time getting OCSP Stapling working well everywhere instead of OCSP GET.) 1) No real reference, but I set the CDN up to do it, so take this as a primary source. 2) IIRC: Enabling HTTPbis support, followed by setting explicit cache headers

Bug 1456489 removed OCSP GET.