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)

defect

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.3 --- fixed
b2g-v1.4 --- fixed

People

(Reporter: keeler, Assigned: briansmith)

References

Details

Attachments

(6 files, 2 obsolete files)

Basically, we need an automated test for bug 914034.
Attached patch patch (obsolete) — Splinter Review
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)
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 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-
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)
... and now I'm actually setting ni? (see comment 4)
Flags: needinfo?(brian)
I will clear the review flag.
Flags: needinfo?(brian)
Attachment #822552 - Flags: review?(brian)
Attachment #822552 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
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)
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?
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
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)
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     }
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)
Sounds good. I filed bug 950129.
Flags: needinfo?(dkeeler)
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.
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: dkeeler → brian
Attachment #8359137 - Flags: review?(dkeeler)
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+
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+
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)
https://hg.mozilla.org/mozilla-central/rev/0e14b3468b94
https://hg.mozilla.org/mozilla-central/rev/1b892043a386
Status: NEW → RESOLVED
Closed: 10 years ago
Priority: -- → P1
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla29 → ---
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+
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 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-
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)
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+
https://hg.mozilla.org/mozilla-central/rev/89be9d5a0209
https://hg.mozilla.org/mozilla-central/rev/7de70e5c5bb9
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
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+
I asked for
No longer depends on: mozilla::pkix
(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.
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)
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+
FWIW, 28 is being merged to beta today.
Attached patch fixup.patchSplinter Review
This seems to fix the issue. (diff -w).
Attachment #8369725 - Flags: review?(dkeeler)
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+
You need to log in before you can comment on or make changes to this bug.