In order to enable OCSP Get in gecko we first need to allow the mozilla::pkix branch to do OCSP get requests.

Dont like the econding part, but it does implement http get without fallback.

Comment on attachment 8419051 [details] [diff] [review] nss-trustdomain-get Review of attachment 8419051 [details] [diff] [review]: ----------------------------------------------------------------- Good start. I think it's important to include fallback functionality. ::: security/certverifier/NSSCertDBTrustDomain.h @@ +58,5 @@ > }; > NSSCertDBTrustDomain(SECTrustType certDBTrustType, OCSPFetching ocspFetching, > OCSPCache& ocspCache, void* pinArg, > + CERTChainVerifyCallback* checkChainCallback = nullptr, > + CertVerifier::ocsp_get_config requestPolicy = CertVerifier::ocsp_get_disabled); I'm concerned "requestPolicy" will make it seem like we're referring to certificate policies here... Maybe "tryGETConfig"? ::: security/certverifier/OCSPRequestor.cpp @@ +100,5 @@ > + nsCString encodedBase64Request; > + nsresult srv; > + char* cur; > + char* end; > + switch (requestPolicy){ This needs to be refactored. Maybe something like this: DoOCSPRequset(...) { if (tryGET) { rv = attemptRequestWithMethod("GET"); } if (failed(rv)) { rv = attemptRequestWithMethod("POST"); } return rv; } Where attemptRequestWithMethod is basically what DoOCSPRequest is now with a few "if (usingGET) { ... }" / "if (usingPOST) { ... }". Also, find or refactor the base64/url-encoding into a separate function (I'm fairly sure one already exists that does what we want here). ::: security/certverifier/OCSPRequestor.h @@ +6,5 @@ > > #ifndef mozilla_psm_OCSPRequestor_h > #define mozilla_psm_OCSPRequestor_h > > +#include "CertVerifier.h" nit: extra space after include

Comment on attachment 8419691 [details] [diff] [review] nss-trustdomain-get (v2) Review of attachment 8419691 [details] [diff] [review]: ----------------------------------------------------------------- Still missing tests on GET on by default

Comment on attachment 8419691 [details] [diff] [review] nss-trustdomain-get (v2) Review of attachment 8419691 [details] [diff] [review]: ----------------------------------------------------------------- This is looking good. One thing that's making it difficult for me to review this is that the diff wasn't very kind to the changes in OCSPRequestor.cpp. I think it would help if tryOCSPRequest were defined after DoOCSPRequest. ::: security/certverifier/CertVerifier.cpp @@ +405,5 @@ > #ifndef MOZ_NO_EV_CERTS > // Try to validate for EV first. > SECOidTag evPolicy = SEC_OID_UNKNOWN; > rv = GetFirstEVPolicy(cert, evPolicy); > + ocsp_get_config requestPolicy = mOCSPGETEnabled ? ocsp_get_enabled : tryGETConfig or OCSPGetConfig In fact, It's not clear why we basically go from bool to enum to bool again, but maybe we'll have some different OCSP GET modes in the future (I doubt it, though). ::: security/certverifier/NSSCertDBTrustDomain.cpp @@ +44,5 @@ > OCSPFetching ocspFetching, > OCSPCache& ocspCache, > void* pinArg, > + CERTChainVerifyCallback* checkChainCallback, > + CertVerifier::ocsp_get_config requestPolicy) tryGETConfig or OCSPGetConfig (just be consistent either way) ::: security/certverifier/NSSCertDBTrustDomain.h @@ +58,5 @@ > }; > NSSCertDBTrustDomain(SECTrustType certDBTrustType, OCSPFetching ocspFetching, > OCSPCache& ocspCache, void* pinArg, > + CERTChainVerifyCallback* checkChainCallback = nullptr, > + CertVerifier::ocsp_get_config requestPolicy = CertVerifier::ocsp_get_disabled); tryGETConfig @@ +93,5 @@ > const SECItem* encodedResponse, EncodedResponseSource responseSource); > > const SECTrustType mCertDBTrustType; > const OCSPFetching mOCSPFetching; > + const CertVerifier::ocsp_get_config mOCSPGetConfig; tryGETConfig or use OCSPGetConfig everywhere - it's good to be consistent ::: security/certverifier/OCSPRequestor.cpp @@ +55,5 @@ > + PR_LOG(gCertVerifierLog, PR_LOG_DEBUG, > + ("Setting up OCSP GET path, pre path =%s\n", PromiseFlatCString(path).get())); > + > + // Apply OCSP url encoding (from nss' ocsp_UrlEncodeBase64Buf) > + // XXX I am sure we have somthing that does this, but we should Will this work? https://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/Helpers.cpp#206 @@ +60,5 @@ > + // delay this the right way until we can get some compatibility tests. > + char* cur = base64Request.BeginWriting(); > + char* end = base64Request.EndWriting(); > + for (; cur<end; ++cur) { > + switch (*cur) { nit: this section needs re-indenting @@ +112,5 @@ > + } > + > + SEC_HTTP_REQUEST_SESSION requestSessionPtr = nullptr; > + if (nsNSSHttpInterface::createFcn(serverSession.get(), "http", > + finalPath.BeginReading(), // XXX What's the issue on this line? @@ +212,5 @@ > + encodedRequest, timeout, true); > + if (result) { > + return result; > + } > + // XXX update timeout based on the time spent on the get response. good call ::: security/manager/ssl/tests/unit/test_ev_certs.js @@ +54,5 @@ > httpServer.registerPrefixHandler("/", > function handleServerCallback(aRequest, aResponse) { > do_check_neq(aRequest.host, "crl.example.com"); // No CRL checks > + let cert_nick = aRequest.path.slice(1, aRequest.path.length - 1). > + split("/", 2)[0]; nit: indent this line two spaces and include the "." from the previous line

Comment on attachment 8419691 [details] [diff] [review] nss-trustdomain-get (v2) Review of attachment 8419691 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/certverifier/OCSPRequestor.cpp @@ +55,5 @@ > + PR_LOG(gCertVerifierLog, PR_LOG_DEBUG, > + ("Setting up OCSP GET path, pre path =%s\n", PromiseFlatCString(path).get())); > + > + // Apply OCSP url encoding (from nss' ocsp_UrlEncodeBase64Buf) > + // XXX I am sure we have somthing that does this, but we should It is similar encoding but different. This should be a follow up bug (I am also concerned about compatibility) I think a follow up bu to clean this up should be ok.

Comment on attachment 8421403 [details] [diff] [review] nss-trustdomain-get (v3) Review of attachment 8421403 [details] [diff] [review]: ----------------------------------------------------------------- This is looking good. There are still a couple of issues. Most of them are style nits (and please be sure to address these), but the one thing I wanted to be sure to mention was it's better to use library functions that encapsulate functionality rather than using raw pointers to do what we want. ::: security/certverifier/NSSCertDBTrustDomain.h @@ +58,5 @@ > }; > NSSCertDBTrustDomain(SECTrustType certDBTrustType, OCSPFetching ocspFetching, > OCSPCache& ocspCache, void* pinArg, > + CERTChainVerifyCallback* checkChainCallback = nullptr, > + CertVerifier::ocsp_get_config ocspGETConfig = CertVerifier::ocsp_get_disabled); This argument doesn't need to have a default value (which I guess means it would go before checkChainCallback). ::: security/certverifier/OCSPRequestor.cpp @@ +38,5 @@ > typedef ScopedPtr<nsNSSHttpRequestSession, ReleaseHttpRequestSession> > ScopedHTTPRequestSession; > > +static > +nsresult AppendEscapedBase64Item(const SECItem* encodedRequest, nit: static nsresult on its own line, AppendEscapedBase64Item on its own line @@ +39,5 @@ > ScopedHTTPRequestSession; > > +static > +nsresult AppendEscapedBase64Item(const SECItem* encodedRequest, > + nsACString &path) { nit: nsACString& @@ +42,5 @@ > +nsresult AppendEscapedBase64Item(const SECItem* encodedRequest, > + nsACString &path) { > + nsresult rv; > + nsCString base64Request; > + rv = Base64Encode(nsDependentCSubstring( Maybe the encodedRequest to CString conversion should go on its own line so this one isn't overly long. @@ +51,5 @@ > + return rv; > + } > + > + PR_LOG(gCertVerifierLog, PR_LOG_DEBUG, > + ("Setting up OCSP GET path, pre path =%s\n", PromiseFlatCString(path).get())); Is this line <=80 characters? @@ +54,5 @@ > + PR_LOG(gCertVerifierLog, PR_LOG_DEBUG, > + ("Setting up OCSP GET path, pre path =%s\n", PromiseFlatCString(path).get())); > + > + // Apply OCSP url encoding (from nss' ocsp_UrlEncodeBase64Buf) > + // XXX I am sure we have somthing that does this, but we should Looks like you're right: http://tools.ietf.org/html/rfc5019#section-5 Let's document the transformation here (i.e. '+' -> "%2B, etc.) @@ +58,5 @@ > + // XXX I am sure we have somthing that does this, but we should > + // delay this the right way until we can get some compatibility tests. > + char* cur = base64Request.BeginWriting(); > + char* end = base64Request.EndWriting(); > + for (; cur<end; ++cur) { Instead of iterating with raw pointers, let's use base64Request.ReplaceSubstring(...); for each replacement (yes, it's slower, but it's much easier to maintain). @@ +77,5 @@ > + return NS_OK; > +} > + > +static > +SECItem* tryOCSPRequest(PLArenaPool* arena, const nsACString &hostname, nit: static SECItem* on its own line, tryOCSPRequest on the next line @@ +78,5 @@ > +} > + > +static > +SECItem* tryOCSPRequest(PLArenaPool* arena, const nsACString &hostname, > + const int32_t port, const nsACString &path, nit: & next to type, not identifier @@ +82,5 @@ > + const int32_t port, const nsACString &path, > + const SECItem* encodedRequest, PRIntervalTime timeout, > + const bool useGET); > + > + nit: unnecessary blank line @@ +87,1 @@ > SECItem* DoOCSPRequest(PLArenaPool* arena, const char* url, Whoops - this should have had the return type on its own line. Feel free to change it while we're modifying nearby lines. @@ +127,5 @@ > + PR_LOG(gCertVerifierLog, PR_LOG_DEBUG, > + ("Setting up OCSP request: pre all path =%s pathlen=%d\n", path.get(), > + pathLen)); > + if (pathLen <= 0) { > + path.Assign("/"); Is this an issue? If we don't need to handle this case, let's not do extra/confusing work. If it is necessary, comment why. @@ +131,5 @@ > + path.Assign("/"); > + } > + > + if (tryGETConfig == CertVerifier::ocsp_get_enabled) { > + SECItem *result = tryOCSPRequest(arena, hostname, port, path, nit: SECItem* @@ +136,5 @@ > + encodedRequest, timeout, true); > + if (result) { > + return result; > + } > + // XXX update timeout based on the time spent on the get response. use PR_Now, etc. (tryOCSPRequest blocks, so this will work as expected) @@ +140,5 @@ > + // XXX update timeout based on the time spent on the get response. > + } > + return tryOCSPRequest(arena, hostname, port, path, encodedRequest, timeout, > + false); > + nit: unnecessary blank line @@ +145,5 @@ > +} > + > + > + > + nit: we only need one empty line between functions @@ +152,5 @@ > + const int32_t port, const nsACString &path, > + const SECItem* encodedRequest, PRIntervalTime timeout, > + const bool useGET) { > + > + nsresult rv; declare this closer to where it's used @@ +166,5 @@ > + nsAutoCString finalPath(path); > + nsAutoCString method("POST"); > + if (useGET) { > + method.Assign("GET"); > + if (path.Length() > 0) { It seems like both this and the other length check (back in DoOCSPRequest) aren't necessary - maybe only keep this one? Either way, I think this can be cleaned up a little. Documentation for sure would help (e.g. "tryOCSPRequest is expecting a non-empty path ending in '/'" or something). @@ +169,5 @@ > + method.Assign("GET"); > + if (path.Length() > 0) { > + const char* cur = path.EndReading(); > + cur--; > + if ((*cur) != '/') { How about "if (!StringEndsWith(finalPath, NS_LITERAL_CSTRING('/')))"? @@ +184,2 @@ > if (nsNSSHttpInterface::createFcn(serverSession.get(), "http", > + finalPath.get(), method.get(), Let's ask a gecko peer if .get() or .BeginReading() is the appropriate function to use. ::: security/certverifier/OCSPRequestor.h @@ +14,5 @@ > > // The memory returned is owned by the given arena. > SECItem* DoOCSPRequest(PLArenaPool* arena, const char* url, > + const SECItem* encodedRequest, PRIntervalTime timeout, > + const CertVerifier::ocsp_get_config tryGETConfig); Let's be consistent about what we call this parameter - it's called ocspGETConfig elsewhere. ::: security/manager/ssl/tests/unit/test_ev_certs.js @@ +54,5 @@ > httpServer.registerPrefixHandler("/", > function handleServerCallback(aRequest, aResponse) { > do_check_neq(aRequest.host, "crl.example.com"); // No CRL checks > + let cert_nick = aRequest.path.slice(1, aRequest.path.length - 1). > + split("/", 2)[0]; See previous review. Let me know if it's not clear what the issue is.

Comment on attachment 8421403 [details] [diff] [review] nss-trustdomain-get (v3) Review of attachment 8421403 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/certverifier/NSSCertDBTrustDomain.h @@ +58,5 @@ > }; > NSSCertDBTrustDomain(SECTrustType certDBTrustType, OCSPFetching ocspFetching, > OCSPCache& ocspCache, void* pinArg, > + CERTChainVerifyCallback* checkChainCallback = nullptr, > + CertVerifier::ocsp_get_config ocspGETConfig = CertVerifier::ocsp_get_disabled); I dont understand this comment, I made is as a parameter with default value to simplify callers but if you this agree with this, please clarify: 1. Do you want this go go before the callback(y/n) 2. Do you want this NOT to have a default value ::: security/certverifier/OCSPRequestor.cpp @@ +184,2 @@ > if (nsNSSHttpInterface::createFcn(serverSession.get(), "http", > + finalPath.get(), method.get(), I asked dveditz he said use get ::: security/manager/ssl/tests/unit/test_ev_certs.js @@ +54,5 @@ > httpServer.registerPrefixHandler("/", > function handleServerCallback(aRequest, aResponse) { > do_check_neq(aRequest.host, "crl.example.com"); // No CRL checks > + let cert_nick = aRequest.path.slice(1, aRequest.path.length - 1). > + split("/", 2)[0]; My bad I missed that.

Can you comment on the default value part of comment 10?

(In reply to Camilo Viecco (:cviecco) from comment #10) > Comment on attachment 8421403 [details] [diff] [review] > nss-trustdomain-get (v3) > > Review of attachment 8421403 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: security/certverifier/NSSCertDBTrustDomain.h > @@ +58,5 @@ > > }; > > NSSCertDBTrustDomain(SECTrustType certDBTrustType, OCSPFetching ocspFetching, > > OCSPCache& ocspCache, void* pinArg, > > + CERTChainVerifyCallback* checkChainCallback = nullptr, > > + CertVerifier::ocsp_get_config ocspGETConfig = CertVerifier::ocsp_get_disabled); > > I dont understand this comment, I made is as a parameter with default value > to simplify callers but if you this agree with this, please clarify: > 1. Do you want this go go before the callback(y/n) > 2. Do you want this NOT to have a default value In the same way that ocspFetching doesn't have a default value, I don't think that ocspGETConfig should have a default value. As a result, my understanding is it would need to go before checkChainCallback. I guess the answer is yes to both 1 and 2.

Comment on attachment 8422830 [details] [diff] [review] nss-trustdomain-get (v4) Review of attachment 8422830 [details] [diff] [review]: ----------------------------------------------------------------- Are you planning on adding tests to this bug or doing that in a separate bug? Otherwise, looks good. r=me with comments addressed, but let's not land this until we fix the bug Brian brought up. I would also feel better about waiting to land this until we have tests. Also, I would definitely appreciate an explanation of why test_ev_certs.js needs to change as a result of this patch. ::: security/certverifier/CertVerifier.cpp @@ +373,5 @@ > : NSSCertDBTrustDomain::FetchOCSPForDVHardFail; > > SECStatus rv; > + ocsp_get_config ocspGETConfig = mOCSPGETEnabled ? ocsp_get_enabled : > + ocsp_get_disabled; nit: ":" on this line ::: security/certverifier/NSSCertDBTrustDomain.cpp @@ +50,5 @@ > , mOCSPFetching(ocspFetching) > , mOCSPCache(ocspCache) > , mPinArg(pinArg) > , mCheckChainCallback(checkChainCallback) > + , mOCSPGetConfig(ocspGETConfig) I think compilers will complain about this being initialized after mCheckChainCallback (basically, these should be in the same order as they're declared in NSSCertDBTrustDomain.h) ::: security/certverifier/OCSPRequestor.cpp @@ +42,5 @@ > +AppendEscapedBase64Item(const SECItem* encodedRequest, nsACString& path) > +{ > + nsresult rv; > + nsDependentCSubstring requestAsSubstring( > + reinterpret_cast<const char*>(encodedRequest->data), I guess this didn't help that much with long lines :( Maybe just have this line indented only four spaces total? @@ +54,5 @@ > + PR_LOG(gCertVerifierLog, PR_LOG_DEBUG, > + ("Setting up OCSP GET path, pre path =%s\n", > + PromiseFlatCString(path).get())); > + > + // Apply OCSP url encoding (from nss' ocsp_UrlEncodeBase64Buf) We don't need to reference nss anymore here. @@ +114,5 @@ > nsAutoCString hostname(url + authorityPos + hostnamePos, hostnameLen); > + nsAutoCString path(url + pathPos, pathLen); > + PR_LOG(gCertVerifierLog, PR_LOG_DEBUG, > + ("Setting up OCSP request: pre all path =%s pathlen=%d\n", path.get(), > + pathLen)); nit: indent one less space ::: security/certverifier/OCSPRequestor.h @@ +14,5 @@ > > // The memory returned is owned by the given arena. > SECItem* DoOCSPRequest(PLArenaPool* arena, const char* url, > + const SECItem* encodedRequest, PRIntervalTime timeout, > + const CertVerifier::ocsp_get_config ocspGETConfig); This doesn't need to be const. ::: security/manager/ssl/tests/unit/test_ev_certs.js @@ +54,5 @@ > httpServer.registerPrefixHandler("/", > function handleServerCallback(aRequest, aResponse) { > do_check_neq(aRequest.host, "crl.example.com"); // No CRL checks > + let cert_nick = aRequest.path.slice(1, aRequest.path.length - 1) > + .split("/", 2)[0]; Do we need the "2" argument to split? In fact, why does this need to change at all? Does this patch affect the OCSP requests we're doing now? We haven't yet enabled OCSP GET, right?

After writting the tests the fallback mechanism suggested on https://bugzilla.mozilla.org/show_bug.cgi?id=1005142#c2 is unfortunatelly a regression from the NSS classic and libpkix as it only fallbacks if there is no http response, not if the response is somehow invalid (not a response, a response for something else or a bad (cached) response). I think a remake is necesary: 1. Patch to add ocsp get (no fallback). 2. Patch for adding POST fallback to NSSCertDBTrustDomain when in GET mode 3. tests. #2 could be a diferent patch. Keeler what do you think?

I'm not sure I understand the issue. OCSPRequestor doesn't validate the response - it just returns it if it gets one. How is the suggestion in comment 2 different from what NSS does?

nss does fallback after checking that the response is invalid (or no response), we do fall only if there is no http response, so we will fail in the case of corrupt caches where nss will not.

We talked on IRC and decided the fallback needs to happen in NSSCertDBTrustDomain because (for example) we don't want an invalid/old entry in the cache to prevent retrying with POST. It seems like a good approach to do this in separate patches (and maybe even separate bugs).

Comment on attachment 8429624 [details] [diff] [review] test-ocsp-get Review of attachment 8429624 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/tests/unit/test_ocsp_fetch_method.js @@ +71,5 @@ > + //GET does fallback on bad entry > + add_test(function() { > + clearOCSPCache(); > + Services.prefs.setBoolPref("security.OCSP.GET.enabled", true); > + if (!useMozillaPKIX) { Forgot to add // XXX Bug 1016681

Comment on attachment 8429621 [details] [diff] [review] nss-trustdomain-get-1 Review of attachment 8429621 [details] [diff] [review]: ----------------------------------------------------------------- You already reviewed a previous version of this (with ocsp get). So this is more of a extra review on the changes to OCSPRequestor.cpp

Comment on attachment 8429621 [details] [diff] [review] nss-trustdomain-get-1 Review of attachment 8429621 [details] [diff] [review]: ----------------------------------------------------------------- Great! r=me with nits addressed. ::: security/certverifier/NSSCertDBTrustDomain.h @@ +93,5 @@ > const SECItem* encodedResponse, EncodedResponseSource responseSource); > > const SECTrustType mCertDBTrustType; > const OCSPFetching mOCSPFetching; > + const CertVerifier::ocsp_get_config mOCSPGetConfig; I guess my previous comment wasn't clear. I think it's best if these fields are in the same order as the constructor takes them. So, this would be: const SECTrustType mCertDBTrustType; const OCSPFetching mOCSPFetching; OCSPCache& mOCSPCache; // non-owning! void* mPinArg; // non-owning! const CertVerifier::ocsp_get_config mOCSPGetConfig; CERTChainVerifyCallback* mCheckChainCallback; // non-owning! ::: security/certverifier/OCSPRequestor.cpp @@ +33,5 @@ > } > typedef ScopedPtr<nsNSSHttpRequestSession, ReleaseHttpRequestSession> > ScopedHTTPRequestSession; > > + nit: unnecessary blank line @@ +116,5 @@ > if (port == -1) { > port = 80; > } > + nsAutoCString hostname(url + authorityPos + hostnamePos, hostnameLen); > + nsAutoCString path; nit: hostname and path should be declared/initialized closer to where they're used

Comment on attachment 8429624 [details] [diff] [review] test-ocsp-get Review of attachment 8429624 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. r=me with comments addressed. ::: security/manager/ssl/tests/unit/head_psm.js @@ +452,5 @@ > if (expectedBasePaths.length >= 1) { > do_check_eq(basePath, expectedBasePaths.shift()); > } > do_check_true(expectedCertNames.length >= 1); > + if (expectedMethods && expectedMethods.length >= 1) { hmmm - shouldn't we also check that expectedMethods is empty (or null) when the responder gets shut down? (same for expectedBasePaths - I think expectedCertNames already does it) ::: security/manager/ssl/tests/unit/test_ocsp_fetch_method.js @@ +4,5 @@ > +// file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +"use strict"; > + > +// In which we try to validate several ocsp responses, checking in particular This comment needs updating. @@ +12,5 @@ > +do_get_profile(); // must be called before getting nsIX509CertDB > +const certdb = Cc["@mozilla.org/security/x509certdb;1"] > + .getService(Ci.nsIX509CertDB); > + > +const SERVER_PORT = 8989; Why port 8989? @@ +66,5 @@ > + check_cert_err("a", 0); > + ocspResponder.stop(run_next_test); > + }); > + > + nit: unnecessary blank line @@ +67,5 @@ > + ocspResponder.stop(run_next_test); > + }); > + > + > + //GET does fallback on bad entry nit: space after "//" Also, maybe explain a little more about how this test is supposed to work (looks like the responder is serving up a response for the wrong certificate, so the client tries again with POST). @@ +71,5 @@ > + //GET does fallback on bad entry > + add_test(function() { > + clearOCSPCache(); > + Services.prefs.setBoolPref("security.OCSP.GET.enabled", true); > + if (!useMozillaPKIX) { Also add a comment that says mozilla::pkix doesn't implement this yet. ::: security/manager/ssl/tests/unit/test_ocsp_fetch_method/generate.py @@ +6,5 @@ > +sys.path.append(libpath) > +import CertUtils > + > +srcdir = os.getcwd() > +db = tempfile.mkdtemp() Doesn't look like db (and hence tempfile) is used anymore. @@ +23,5 @@ > + generate_ca_cert(srcdir, srcdir, noise_file, 'ca') > + generate_child_cert(srcdir, srcdir, noise_file, 'int', 'ca', False, '') > + ocsp_url = "http://www.example.com:8989/" > + generate_child_cert(srcdir, srcdir, noise_file, "a", 'int', True, ocsp_url) > + generate_child_cert(srcdir, srcdir, noise_file, "b", 'int', True, ocsp_url) nit: let's be consistent about single vs double quotes Also, "a" and "b" aren't very descriptive. How about "end-entity-1" and "end-entity-2" or something? ::: security/manager/ssl/tests/unit/xpcshell.ini @@ +85,5 @@ > +[test_ocsp_fetch_method.js] > +run-sequentially = hardcoded ports > +# Bug 1009158: this test times out on Android > +skip-if = os == "android" > + nit: unnecessary trailing blank line

Comment on attachment 8429624 [details] [diff] [review] test-ocsp-get Review of attachment 8429624 [details] [diff] [review]: ----------------------------------------------------------------- thanks for the fast review. I addressed all your comments (except for the port issue) ::: security/manager/ssl/tests/unit/test_ocsp_fetch_method.js @@ +12,5 @@ > +do_get_profile(); // must be called before getting nsIX509CertDB > +const certdb = Cc["@mozilla.org/security/x509certdb;1"] > + .getService(Ci.nsIX509CertDB); > + > +const SERVER_PORT = 8989; In hope that we can parallelize this someday, as long as generate.py uses the same port as here it should not be an issue.

keeping r+ from keeler

keeping r+ from keeler

I understand your reasoning, and I agree it would be great to parallelize these tests someday, but I'm concerned that having inconsistent port numbers will make it more difficult to maintain the code right now and to make that transition in the future. Right now we can ask "what port should the OCSP responder be running on?" or "what port do I look for in wireshark?" and the answer is consistently "8080". By using port 8989, we have to remember that some ports are 8080 but others are 8989 (and what happens when we add more tests?). Basically, I think it needlessly complicates the code now for little theoretical gain in the future.

OK lets unify the ports.

Awesome - thanks.

keeping r+ from keeler, moving ocsp port to 8080

https://tbpl.mozilla.org/?tree=Try&rev=55d3a0ac6a23

Comment on attachment 8430152 [details] [diff] [review] test-ocsp-get (v3) Review of attachment 8430152 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/tests/unit/test_ocsp_fetch_method.js @@ +6,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. 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.

Thank you brian, I am going to copy this to Bug 1016681 where the fallback is going to be implemented.

keeping r+ from keeler. Fixed compilation issues on b2g/win32 opt.

Pushed to inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/c288e2c355ab https://hg.mozilla.org/integration/mozilla-inbound/rev/21574b16ad1e

https://hg.mozilla.org/mozilla-central/rev/c288e2c355ab https://hg.mozilla.org/mozilla-central/rev/21574b16ad1e