Closed
Bug 923304
Opened 11 years ago
Closed 10 years ago
test ocsp stapling with an intermediate certificate that itself has an ocsp aia
Categories
(Core :: Security: PSM, defect, P1)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: keeler, Assigned: briansmith)
References
Details
Attachments
(6 files, 2 obsolete files)
18.93 KB,
patch
|
briansmith
:
review+
|
Details | Diff | Splinter Review |
2.08 KB,
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
5.35 KB,
patch
|
keeler
:
review+
cviecco
:
feedback+
|
Details | Diff | Splinter Review |
3.86 KB,
patch
|
cviecco
:
review+
keeler
:
review+
|
Details | Diff | Splinter Review |
17.34 KB,
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
1.25 KB,
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
Basically, we need an automated test for bug 914034.
Reporter | ||
Comment 1•11 years ago
|
||
This patch uses our existing tlsserver functionality to add an OCSP responder that will aid in testing OCSP stapling (for example, bug 914034 and bug 929617).
Attachment #822552 -
Flags: review?(brian)
Reporter | ||
Comment 2•11 years ago
|
||
Comment on attachment 822552 [details] [diff] [review] patch cviecco - I think we need something like this for more thoroughly testing OCSP stapling. Let me know what you think.
Attachment #822552 -
Flags: review?(cviecco)
Comment 3•11 years ago
|
||
Comment on attachment 822552 [details] [diff] [review] patch Review of attachment 822552 [details] [diff] [review]: ----------------------------------------------------------------- While this seem to work (on POSIX) I dont like the idea of another ocsp responder. I prefer the ideas behind bug 927016, ie why create another sever for the http case given that we have httd.js? This code feels more complex than needed. Can you check if this compiles in win? But I found no nits :) ::: security/manager/ssl/tests/unit/tlsserver/lib/TLSServer.cpp @@ +4,5 @@ > > #include "TLSServer.h" > > #include <stdio.h> > +#include <unistd.h> This I think is POSIX only. @@ +58,5 @@ > > void > PrintPRError(const char *aPrefix) > { > + int32_t pid = (int32_t)getpid(); getpid is POSIX only.. does this compile on windows? ::: security/manager/ssl/tests/unit/tlsserver/lib/TLSServer.h @@ +54,2 @@ > int > +StartPlainServer(const char *nssCertDBDir, DataReadCallback dataReadCallback); This feels wrong: (start plain server inside TLSServer.*)
Attachment #822552 -
Flags: review?(cviecco) → review-
Reporter | ||
Comment 4•11 years ago
|
||
Brian: it looks like this change will be broken into at least two pieces, the first of which will be in bug 932519. This essentially makes this patch obsolete. So, I would cancel r?, but I don't want to clear any comments you have so far. (ni? just to make sure you get the heads-up)
Reporter | ||
Comment 5•11 years ago
|
||
... and now I'm actually setting ni? (see comment 4)
Flags: needinfo?(brian)
Assignee | ||
Updated•11 years ago
|
Attachment #822552 -
Flags: review?(brian)
Reporter | ||
Updated•11 years ago
|
Attachment #822552 -
Attachment is obsolete: true
Reporter | ||
Comment 7•10 years ago
|
||
Ok - let's try this again now that we have more testing infrastructure in place. Also, I know we have plans to not do OCSP fetching for intermediates, but as far as I can tell, that's a ways off, yet. If we add this test now, not only do we have better test coverage for what we're currently doing, we will already have a test for when we change behavior - we just have to change the expected number of OCSP requests in this test.
Attachment #8342052 -
Flags: review?(brian)
Attachment #8342052 -
Flags: feedback?(cviecco)
Reporter | ||
Comment 8•10 years ago
|
||
Comment on attachment 8342052 [details] [diff] [review] patch Review of attachment 8342052 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/tests/unit/tlsserver/cmd/OCSPStaplingServer.cpp @@ +55,5 @@ > > + const char *certNickname; > + if (strcmp(host->mHostName, > + "ocsp-stapling-with-intermediate.example.com") == 0) { > + certNickname = host->mAdditionalCertName; By the way, I'm not very happy with the solution I came up with here. The issue is that we need to differentiate which EE certificate the server uses to communicate with. Any ideas?
Reporter | ||
Comment 9•10 years ago
|
||
17:20 briansmith | Keeler: regarding https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=923304&attachment=8342052 17:21 briansmith | when you say "intermediate," you really mean "delegated OCSP response signing cert", right? 17:21 keeler | briansmith: no, actually - that's another type of test we need to add 17:22 briansmith | keeler: In the current code, why are we doing revocation checking of an intermediate at all? 17:22 briansmith | The clsasic NSS certificate verification code doesn't do so. 17:23 @dveditz | briansmith: but in theory, checking intermediate revocation is important 17:23 keeler | briansmith: hmm - let me see if I can reproduce what I was seeing 17:24 briansmith | dveditz: that's not being debated. I am just surprised that it is being done in the case that keeler is concerned about 17:24 briansmith | because, by my understanding, NSS won't do that, whether we think it should or not. 17:24 briansmith | keeler: is it possible that the problematic case was an EV cert? 17:25 keeler | could be, yes 17:25 briansmith | so, EV will do so. 17:26 briansmith | keeler: but, your test case isn't EV, right? 17:27 keeler | briansmith: correct. But there's still a request for the intermediate's OCSP status, so... 17:28 briansmith | keeler: maybe that is because, in the classic OCSP/certvfy.c code, there is a special check for the "delegated OCSP response signer" certUsage 17:29 briansmith | but, when the intermediate signs the OCSP response directly, then it gets re-validated for "AnyCA" usage or some such? 17:30 briansmith | And, maytbe this check only happens for the CERT_CacheCertFromSideChannel case? 17:30 briansmith | CERT_CacheOCSPResponseFromSideChannel, I mean. 17:31 briansmith | If so, that effectively means that OCSP stapling is not actually reducing the number of OCSP fetches being done, which is dumb. 17:32 keeler | looking... 17:42 keeler | briansmith: I'm having a hard time finding a good example, but I'll look into it and get back to you 17:42 briansmith | keeler: thanks. 17:43 briansmith | keeler: my point is that this test seems to be testing a case that we don't expect to happen. it isn't the case that we expect to change it to not happen in the future. 17:43 briansmith | it is already not supposed to be happening. 17:46 keeler | I see what you're saying
Reporter | ||
Comment 10•10 years ago
|
||
Comment on attachment 8342052 [details] [diff] [review] patch Cancelling reviews for now (see comment above).
Attachment #8342052 -
Flags: review?(brian)
Attachment #8342052 -
Flags: feedback?(cviecco)
Reporter | ||
Comment 11•10 years ago
|
||
What appears to be happening is if the certificate that signed the response is a CA, when verifying that certificate, CERT_VerifyCert will do an OCSP lookup for it (given that it has an OCSP AIA and no id-pkix-ocsp-nocheck extension). CERT_VerifyOCSPResponseSignature should have some way to specify to CERT_VerifyCert that no OCSP lookups should be performed. CERT_VerifyOCSPResponseSignature: ... 4156 if (CERT_IsCACert(signerCert, NULL)) { 4157 certUsage = certUsageAnyCA; 4158 } else { 4159 certUsage = certUsageStatusResponder; 4160 } 4161 rv = CERT_VerifyCert(handle, signerCert, PR_TRUE, 4162 certUsage, producedAt, pwArg, NULL); 4163 if (rv != SECSuccess) { 4164 PORT_SetError(SEC_ERROR_OCSP_INVALID_SIGNING_CERT); 4165 goto finish; 4166 } CERT_VerifyCert: ... 1332 statusConfig = CERT_GetStatusConfig(handle); 1333 if (certUsage != certUsageStatusResponder && statusConfig != NULL) { 1334 if (statusConfig->statusChecker != NULL) { 1335 rv = (* statusConfig->statusChecker)(handle, cert, 1336 t, wincx); 1337 if (rv != SECSuccess) { 1338 LOG_ERROR_OR_EXIT(log,cert,0,0); 1339 } 1340 } 1341 }
Assignee | ||
Comment 12•10 years ago
|
||
I believe that we agreed yesterday that we would punt on fixing this until we're using insanity::pkix, right keeler? I agree that is a reasonable thing to do, but we should still file a bug in product:nss component:libraries about the fact that NSS classic is doing this weird thing. Presumably, we agree that if NSS doesn't check a delegated OCSP response signing certificate for revocation (which it doesn't), then it shouldn't check the CA for revocation either, for consistency's sake. If we're in agreement, please file the new bug with the content of comment 11 and your expectation of how things should work. Thanks.
Flags: needinfo?(dkeeler)
Assignee | ||
Updated•10 years ago
|
Depends on: mozilla::pkix
Assignee | ||
Comment 14•10 years ago
|
||
Comment on attachment 8342052 [details] [diff] [review] patch Review of attachment 8342052 [details] [diff] [review]: ----------------------------------------------------------------- r+ except for test_ocsp_stapling_with_intermediate.js, with comments addressed. ::: security/manager/ssl/tests/unit/test_ocsp_stapling_with_intermediate.js @@ -6,5 @@ > > -// In which we connect to a number of domains (as faked by a server running > -// locally) with OCSP stapling enabled to determine that good things happen > -// and bad things don't, specifically with respect to various expired OCSP > -// responses (stapled and otherwise). I will skip this file, because I have written a replacement with the inverse logic. ::: security/manager/ssl/tests/unit/tlsserver/cmd/OCSPStaplingServer.cpp @@ +55,5 @@ > > + const char *certNickname; > + if (strcmp(host->mHostName, > + "ocsp-stapling-with-intermediate.example.com") == 0) { > + certNickname = host->mAdditionalCertName; I think it is fine for now. In the future we can generalize this so that every entry in the table can optionally provide a cert to override the default. ::: security/manager/ssl/tests/unit/tlsserver/generate_certs.sh @@ +83,5 @@ > + echo -e "$INT_RESPONSES" | $RUN_MOZILLA $CERTUTIL -d $OUTPUT_DIR -S \ > + -n $NICKNAME \ > + -s "$SUBJECT" \ > + -c $CA \ > + -t "CT,," \ It seems like this should be -t ",," for an intermediate, right? It will inherit trust from its root.
Assignee | ||
Comment 15•10 years ago
|
||
Comment on attachment 8342052 [details] [diff] [review] patch Review of attachment 8342052 [details] [diff] [review]: ----------------------------------------------------------------- r+ except for test_ocsp_stapling_with_intermediate.js, with comments addressed. ::: security/manager/ssl/tests/unit/test_ocsp_stapling_with_intermediate.js @@ -6,5 @@ > > -// In which we connect to a number of domains (as faked by a server running > -// locally) with OCSP stapling enabled to determine that good things happen > -// and bad things don't, specifically with respect to various expired OCSP > -// responses (stapled and otherwise). I will skip this file, because I have written a replacement with the inverse logic. ::: security/manager/ssl/tests/unit/tlsserver/cmd/OCSPStaplingServer.cpp @@ +55,5 @@ > > + const char *certNickname; > + if (strcmp(host->mHostName, > + "ocsp-stapling-with-intermediate.example.com") == 0) { > + certNickname = host->mAdditionalCertName; I think it is fine for now. In the future we can generalize this so that every entry in the table can optionally provide a cert to override the default. ::: security/manager/ssl/tests/unit/tlsserver/generate_certs.sh @@ +83,5 @@ > + echo -e "$INT_RESPONSES" | $RUN_MOZILLA $CERTUTIL -d $OUTPUT_DIR -S \ > + -n $NICKNAME \ > + -s "$SUBJECT" \ > + -c $CA \ > + -t "CT,," \ It seems like this should be -t ",," for an intermediate, right? It will inherit trust from its root.
Assignee | ||
Updated•10 years ago
|
Attachment #8342052 -
Flags: review+
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8342052 -
Attachment is obsolete: true
Attachment #8359136 -
Flags: review+
Assignee | ||
Comment 17•10 years ago
|
||
Assignee: dkeeler → brian
Attachment #8359137 -
Flags: review?(dkeeler)
Assignee | ||
Comment 18•10 years ago
|
||
Attachment #8359138 -
Flags: review?(dkeeler)
Reporter | ||
Comment 19•10 years ago
|
||
Comment on attachment 8359137 [details] [diff] [review] Part 1(b): The actual test Review of attachment 8359137 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. ::: security/manager/ssl/tests/unit/test_ocsp_stapling_with_intermediate.js @@ +9,5 @@ > +// that an OCSP request is not made for the intermediate. > + > +let gOCSPRequestCount = 0; > + > +function add_ocsp_test(aHost, aExpectedResult) { nit: we probably don't actually need this helper since there's only one connection test.
Attachment #8359137 -
Flags: review?(dkeeler) → review+
Reporter | ||
Comment 20•10 years ago
|
||
Comment on attachment 8359138 [details] [diff] [review] Part 2: Adjust EV tests Review of attachment 8359138 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. I would find out what's up with that last test. Also, I don't think all of the calls to clearOCSPCache are necessary, but I certainly could be wrong. ::: security/manager/ssl/tests/unit/test_ev_certs.js @@ +37,5 @@ > + let httpServer = new HttpServer(); > + httpServer.registerPrefixHandler("/", function(request, response) { > + do_check_true(false); > + }); > + httpServer.start(8888); SERVER_PORT, right? @@ +144,4 @@ > let evRootCA = certdb.findCertByNickname(null, evrootnick); > certdb.setCertTrust(evRootCA, nsIX509Cert.CA_CERT, 0); > + > + clearOCSPCache(); If the duplicate test is removed, it seems like this is the only call to clearOCSPCache that is necessary, unless I'm missing something? @@ +150,5 @@ > + ocspResponder.stop(run_next_test); > +}); > + > +// bug 917380: Chcek that a trusted EV root is trusted. > +// TODO: isn't this a duplicate of the above test? You mean the one on line 120? It does look like it... maybe ask Camilo?
Attachment #8359138 -
Flags: review?(dkeeler) → review+
Assignee | ||
Comment 21•10 years ago
|
||
Comment on attachment 8359138 [details] [diff] [review] Part 2: Adjust EV tests Review of attachment 8359138 [details] [diff] [review]: ----------------------------------------------------------------- cviecco: Can you look at the part of the patch with the question about the duplicate test, and let me know if it is really a duplicate. If it is a duplicate, should I just remove it, or was it intended to be some different test case? ::: security/manager/ssl/tests/unit/test_ev_certs.js @@ +144,4 @@ > let evRootCA = certdb.findCertByNickname(null, evrootnick); > certdb.setCertTrust(evRootCA, nsIX509Cert.CA_CERT, 0); > + > + clearOCSPCache(); I think resetting state as much as possible before each test case is a good practice, so I'm going to leave them all in. I agree that the repetition is unfortunate but it doesn't seem like a good use of time to try to factor the repetition out right now.
Attachment #8359138 -
Flags: feedback?(cviecco)
Assignee | ||
Comment 22•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0e14b3468b94 https://hg.mozilla.org/mozilla-central/rev/1b892043a386
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox29:
--- → fixed
Priority: -- → P1
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Comment 23•10 years ago
|
||
Backed out for xpcshell failures. https://hg.mozilla.org/mozilla-central/rev/f356b409d710 https://tbpl.mozilla.org/php/getParsedLog.php?id=32940386&tree=Mozilla-Central Please don't push directly to m-c if you a) aren't watching your push (not being on IRC to contact isn't saying you are anyway) and b) don't have an all-green Try run so you know you won't be burning the tree. https://wiki.mozilla.org/Tree_Rules
Updated•10 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla29 → ---
Comment 24•10 years ago
|
||
Another failure: https://tbpl.mozilla.org/php/getParsedLog.php?id=32941481&tree=Mozilla-Central
Comment 25•10 years ago
|
||
Comment on attachment 8359138 [details] [diff] [review] Part 2: Adjust EV tests Review of attachment 8359138 [details] [diff] [review]: ----------------------------------------------------------------- You can remove the test, but please put the resetting of the trust db in the same function that alters it. ::: security/manager/ssl/tests/unit/test_ev_certs.js @@ +150,5 @@ > + ocspResponder.stop(run_next_test); > +}); > + > +// bug 917380: Chcek that a trusted EV root is trusted. > +// TODO: isn't this a duplicate of the above test? This test was to ensure that the trust flags where reset to the previous state. I you want you can remove the second test, but I would strongly prefer to leave the reset of the state within the same test function.
Attachment #8359138 -
Flags: feedback?(cviecco) → feedback+
Assignee | ||
Comment 26•10 years ago
|
||
This is a patch on top of Part 2 to fix the xpcshell opt bustage.
Attachment #8359549 -
Flags: review?(dkeeler)
Attachment #8359549 -
Flags: review?(cviecco)
Comment 27•10 years ago
|
||
Comment on attachment 8359549 [details] [diff] [review] fix-bustage.patch Review of attachment 8359549 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/tests/unit/test_ev_certs.js @@ +119,5 @@ > add_test(function() { > clearOCSPCache(); > + let ocspResponder = start_ocsp_responder( > + isDebugBuild ? ["int-ev-valid", "ev-valid"] > + : ["ev-valid"]); I did not tought about this change when I gave feedback on the 'father' patch. I think keeping the single server running would be the 'best' solution to simplify the test code. I dont like the logic of is(or not) debug to be interwined in the code. Do you have any strong arguments on why this should be like the way you changed?
Attachment #8359549 -
Flags: review?(cviecco) → review-
Assignee | ||
Comment 28•10 years ago
|
||
Comment on attachment 8359549 [details] [diff] [review] fix-bustage.patch Review of attachment 8359549 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/tests/unit/test_ev_certs.js @@ +119,5 @@ > add_test(function() { > clearOCSPCache(); > + let ocspResponder = start_ocsp_responder( > + isDebugBuild ? ["int-ev-valid", "ev-valid"] > + : ["ev-valid"]); I am not quite sure what you mean by your first paragraph. I changed the structure of the test because the new structure is much more precise than the old structure. And, that precision is useful in ways that matter to changes we're making, like this one. It is also helpful, for example, in making sure we're doing revocation checking in the right order (from root to end-entity). There are lots of suboptimal things about our testing of this. I don't think that the isDebugBuild logic being mixed in here are the worst of it. I suggest we go with this and move on to other things. If you disagree, please give me a concrete suggestion for getting the r+. Thanks!
Attachment #8359549 -
Flags: review- → review?(cviecco)
Reporter | ||
Comment 29•10 years ago
|
||
Comment on attachment 8359549 [details] [diff] [review] fix-bustage.patch Review of attachment 8359549 [details] [diff] [review]: ----------------------------------------------------------------- Personally, I think this is a reasonable solution.
Attachment #8359549 -
Flags: review?(dkeeler) → review+
Assignee | ||
Comment 30•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/89be9d5a0209 https://hg.mozilla.org/integration/mozilla-inbound/rev/7de70e5c5bb9 I merged fix-bustage.patch into Part 2. Thanks for the reviews.
Comment 31•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/89be9d5a0209 https://hg.mozilla.org/mozilla-central/rev/7de70e5c5bb9
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Comment 32•10 years ago
|
||
Comment on attachment 8359549 [details] [diff] [review] fix-bustage.patch Review of attachment 8359549 [details] [diff] [review]: ----------------------------------------------------------------- Lets land.
Attachment #8359549 -
Flags: review?(cviecco) → review+
Assignee | ||
Comment 34•10 years ago
|
||
(In reply to Brian Smith (:briansmith, :bsmith; NEEDINFO? for response) from comment #33) > I asked for ... this bug 950129 to be uplifted to Aurora 28. If/when that happens, these tests should be uplifted too.
Assignee | ||
Comment 35•10 years ago
|
||
Because Mozilla-Aurora doesn't contain the fix for bug 962833, and because the test for bug 950240 was landed on Mozilla-Aurora before this one, I had to do a merge that was more complex than normal. Please review this carefully; in particular, make sure the differences with respect to bug 962833 make sense. Note that I copied *.db and *.der from Mozilla-Inbound's security/manager/ssl/tests/unit/tlsserver to resolve the merge conflicts in those files.
Attachment #8369715 -
Flags: review?(dkeeler)
Reporter | ||
Comment 36•10 years ago
|
||
Comment on attachment 8369715 [details] [diff] [review] Part 2 for Mozilla-Aurora Review of attachment 8369715 [details] [diff] [review]: ----------------------------------------------------------------- Just one issue. Otherwise looks correct. If the binary files don't work, you may have to regenerate them. ::: security/manager/ssl/tests/unit/test_ev_certs.js @@ +216,5 @@ > + let ocspResponder = start_ocsp_responder( > + isDebugBuild ? ["int-ev-valid", "ev-valid"] > + : ["ev-valid"]); > + check_ee_for_ev("ev-valid", isDebugBuild); > + ocspResponder.stop(run_next_test); stop() is asynchronous, so I think everything else in this function will complete before it does. I'm pretty sure you'll need to add something like add_test(function() { ... the rest of what's currently in this test... });
Attachment #8369715 -
Flags: review?(dkeeler) → review+
Comment 37•10 years ago
|
||
FWIW, 28 is being merged to beta today.
Assignee | ||
Comment 38•10 years ago
|
||
This seems to fix the issue. (diff -w).
Attachment #8369725 -
Flags: review?(dkeeler)
Reporter | ||
Comment 39•10 years ago
|
||
Comment on attachment 8369725 [details] [diff] [review] fixup.patch Review of attachment 8369725 [details] [diff] [review]: ----------------------------------------------------------------- LGTM assuming the body is indented (given this is a diff -w).
Attachment #8369725 -
Flags: review?(dkeeler) → review+
Assignee | ||
Comment 40•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/ccd34337f7d2 https://hg.mozilla.org/releases/mozilla-beta/rev/bce559074db1
status-firefox28:
--- → fixed
Flags: in-testsuite+
Comment 41•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/ccd34337f7d2 https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/bce559074db1
Updated•10 years ago
|
status-b2g-v1.3:
--- → fixed
status-b2g-v1.4:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•