391721 - GetBestCRL does not set error code when CRL is not found or invalid

Status: RESOLVED FIXED

(NSS :: Test, defect, P2)

Description

[Christophe Ravel]

There is an error in the output below (crlutil security library: bad database), but the result of the test is PASSED:

cert.sh: Creating CA CRL =====================================
cert.sh: Generating CRL for range 40-42 TestCA authority --------------------------
crlutil -q -d /Users/chravel/Documents/Sun/ws/mozilla/tests_results/security/Christophe-iMac.1/CA -G -n TestCA -f ../tests.pw.13730 -o ../server/root.crl_40-42_or
cert.sh: Modifying CA CRL by adding one more cert ============
cert.sh: Modify CRL by adding one more cert --------------------------
crlutil -q -d /Users/chravel/Documents/Sun/ws/mozilla/tests_results/security/Christophe-iMac.1/CA -M -n TestCA -f ../tests.pw.13730 -o ../server/root.crl_40-42_or1 -i ../server/root.crl_40-42_or
cert.sh: Modifying CA CRL by removing one cert ===============
cert.sh: Modify CRL by removing one cert --------------------------
crlutil -q -d /Users/chravel/Documents/Sun/ws/mozilla/tests_results/security/Christophe-iMac.1/CA -M -n TestCA -f ../tests.pw.13730 -o ../server/root.crl_40-42 -i ../server/root.crl_40-42_or1
cert.sh: Creating CA CRL for groups 1 and 2  ===============
cert.sh: Creating CRL for groups 1 and 2 --------------------------
crlutil -q -d /Users/chravel/Documents/Sun/ws/mozilla/tests_results/security/Christophe-iMac.1/CA -M -n TestCA -f ../tests.pw.13730 -o ../server/root.crl_43-48 -i ../server/root.crl_40-42
cert.sh: Creating CA CRL for groups 1, 2 and 3  ===============
cert.sh: Creating CRL for groups 1, 2 and 3 --------------------------
crlutil -q -d /Users/chravel/Documents/Sun/ws/mozilla/tests_results/security/Christophe-iMac.1/CA -M -n TestCA -f ../tests.pw.13730 -o ../server/root.crl_49-52 -i ../server/root.crl_43-48
cert.sh: Importing Server CA Issued CRL for certs  trough 52
cert.sh: Importing CRL for groups 1 --------------------------
crlutil -q -D -n TestCA -f ../tests.pw.13730 -d ../server
crlutil: could not find TestCA's CRL: security library: bad database.
crlutil: could not find the issuer TestCA's CRL: security library: bad database.
cert.sh: Importing CRL for groups 1 --------------------------
crlutil -q -I -i ../server/root.crl_40-42 -n TestCA -f ../tests.pw.13730 -d ../server
cert.sh SUCCESS: SSL CRL prep passed
cert.sh cert.sh: finished cert.sh
TIMESTAMP cert END: Fri Aug 10 11:58:02 PDT 2007

Comment 1

[Christophe Ravel]

We have this "bad database" error on all platforms that we test. We seem to happily ignore it: it doesn't have any effect on the results of the CRL tests.

The cert.sh script doesn't test the output of the first crlutil (-D), but it tests the result of the second crlutil (-I).

  crlu -D -n TestCA  -f "${R_PWFILE}" -d "${R_SERVERDIR}"
  crlu -I -i ${CRL_FILE} -n "TestCA" -f "${R_PWFILE}" -d "${R_SERVERDIR}"
  CRL_GEN_RES=`expr $? + $CRL_GEN_RES`

Maybe crlutil doesn't report an error for "bad database".

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Is this a "negative" test?  (A test that is supposed to fail?) 

Comment 3

[Christophe Ravel]

No. This is in the part where we create the CRLs.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Are there no negatve CRL creation tests?  
That would be a deficiency in our CRL testing.

Comment 5

[Slavomir Katuscak]

Delete command (crlutil -D) is called also in case when there is no CRL in DB, if there is no CRL the result is:

crlutil: could not find TestCA's CRL: security library: bad database.
crlutil: could not find the issuer TestCA's CRL: security library: bad database.

We don't check if there is CRL in DB or not, we just delete it (this is why we don't check return value). We don't test delete command, we test only import in next command (crlutil -I).

If we want to test also delete command then suggested solution is to check first if there is CRL in DB (crlutil -L) and call delete command (crlutil -D) only if CRL is found. Is this what we want ? 

Comment 6

[Julien Pierre]

Nelson,

What do you mean about "negative CRL creation test" ? Invalid input syntax to the CRL creation tool (crlutil -G) ?

Slavo,

We shouldn't be trying to delete the CRL if there isn't one present, unless we are specifically testing for a failure case of crlutil -D . But I don't think this was the case here, it was just a script error.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Julien,  The weakest area of NSS's testing is testing of behavior in error 
paths.  We just don't test enough of them.  
That's how we came to have the embarrassing bug 343231, where certutil -C 
generates a bogus cert even though the input CSR is invalid.  
We need to test all our commands with inputs & arguments that are intended
to provoke failure, to ensure that it fails as expected.  

When I asked (comment 2) if the failure was a negative test case, the 
answer (comment 3) seemed to implicitly be saying "No, this section of the 
script is testing cert creation, and we don't do negative testing in this 
section of the script."  

In this case, it appears that we're deleting one CRL in preparation for 
importing another one.  Do we test to see that CRL deletion fails in the 
expected manner when (say) the named CRL doesn't exist?  Do we test to see
what happens if we import a second CRL without deleting the previous one?
(I thought that was supposed to succeed, actually, with the second 
replacing the first, so I don't understand why the -D is necessary.) 
Do we test that CRL creation fails, and does not create a bogus CRL 
(unlike bug 343231) when the input is invalid?

Comment 8

[Slavomir Katuscak]

(In reply to comment #7)
> In this case, it appears that we're deleting one CRL in preparation for 
> importing another one.  Do we test to see that CRL deletion fails in the 
> expected manner when (say) the named CRL doesn't exist?  Do we test to see
> what happens if we import a second CRL without deleting the previous one?
> (I thought that was supposed to succeed, actually, with the second 
> replacing the first, so I don't understand why the -D is necessary.) 
> Do we test that CRL creation fails, and does not create a bogus CRL 
> (unlike bug 343231) when the input is invalid?
> 

Nelson, in this case we really do just cleanup before other tests (that's why return value is not checked - it can pass or fail, depends of what was there before). But answer to your questions is no, we don't have tests like this yet. 

Please let me know, if there is something needed to do with this bug or if I can close it. If there is need to write new tests related to crlutil, please report this as new bug.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Slavo, why would you close this bug? 
There is no patch.
No "fix" seems to have been made to any problem.
Please plan to fix the problem reported in this bug.

Comment 10

[Slavomir Katuscak]

Nelson, that's why I asked if there is something needed to do with this bug.
I explained in comment 5 that error messages doesn't mean that there is a problem if it's expected, so in this case it's not a bug.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

In comment 2, I asked if this was a negative test.
A negative test is a test that is expected and intended to fail.
The response I received was an unequivocal NO.

The only alternative to a negative test is a positive test, one that is 
expected to pass.  When a positive test fails, that is a bug that must be
fixed.

Now, this bug report shows that there is a test that fails.  Either:
a) failure is the expected and desired outcome, in which case this is a 
negative test, or 
b) failure is an unexpected and undesired outcome, in which case this is a 
bug that needs to be fixed.

Given that Christophe reported this bug on the grounds that the test failed,
and that he denied that this is a negative test, I therefore gather that
this is a positive test that is failing, and so there is a bug that needs 
to be fixed.  This bug report cannot be closed until the bug is fixed.

Now, it may be that Christophe has changed his mind, and now thinks this is
a negative test after all, and that failure is a desired and expected outcome.
In that case, Christophe can resolve this bug INVALID.  That will be fine.
But either
a) is is invalid, because it is a negative test that should fail, or 
b) is is a valid bug, because this is a positive test that is failing, 
in which case the assignee of this bug must not close it until it has been
fixed. 

Is that clear now?

Don't tell me this bug is blocked on me any more.

So, Slavo and 

Comment 12

[Christophe Ravel]

I haven't changed my mind on this bug.
This is a positive test case: it is expected to pass.

As far as I understand, the unit test is made up of several steps. One of these steps is to delete a CRL. This step fails even though to unit test (the series of steps) passes.
Having a message like "security library: bad database." in a unit test that passes (and is supposed to pass) is very misleading.

Removing or avoiding the "security library: bad database." message when the database is good is the purpose of this bug.

Comment 13

[Slavomir Katuscak]

I had discussion with Christophe about this bug. The problem there is not about positive/negative tests - it's not test, it's just DB cleanup which can pass or fail (depends on content of DB), it's preparation for tests.

What is wrong is error message "security library: bad database.". This message is printed when CRL is not found in DB, and that's confusing, because DB is not bad. I'm changing summary now, to have it clear.

My solution for this bug (after discussion with Christophe) is to change error message. Nelson, please let me know if this is OK with you.

Comment 14

[Julien Pierre]

I agree that the wrong error message was printed by crlutil. This was caused by a bug in the NSS CRL code. Patch forthcoming.

Comment 15

[Julien Pierre]

Attachment: Set missing error code in a couple of cases

Comment 16

[Julien Pierre]

Comment on attachment 310141 [details] [diff] [review]
Set missing error code in a couple of cases

This patch contained more than expected. Sorry.

Comment 17

[Julien Pierre]

Attachment: Set missing error code in a couple of cases

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 310142 [details] [diff] [review]
Set missing error code in a couple of cases

I have an issue with the choice of one of these error codes.

>@@ -2345,6 +2346,7 @@ static CERTSignedCrl* GetBestCRL(CRLDPCa

>+    PORT_SetError(SEC_ERROR_CRL_INVALID);
>     return NULL;
> }

The error string for that error is "New CRL has an invalid format."
That error is clearly appropriate only in the context of importing
a CRL, not in the context of using a CRL already in the CRL cache.

One might ask, "why do we have a CRL with an invalid format in the 
CRL cache?"  And I think we should not.

So maybe SEC_ERROR_CRL_NOT_FOUND is the appropriate choice in 
both cases, unless some other CRL error code seems appropriate.

Comment 19

[Julien Pierre]

Nelson,

The CRL cache tries to cache all CRLs. One of its benefits is to save on token transfer and decoding time. If there is an improperly formed CRL in the database, we won't keep trying to decode it over and over again. It will just be decoded once and cached as bad. I can return SEC_ERROR_CRL_NOT_FOUND for this case too, though.

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

> If there is an improperly formed CRL in the database, we won't keep 
> trying to decode it over and over again.

Why do we allow improperly formed CRLs to get in to the database?
Seems like a bad idea.  Can we fix that?

Comment 21

[Julien Pierre]

Nelson,

I believe we do a partial decode of CRLs on import to speed things up. Errors can occur in the entries which aren't decoded on import. Also, disk corruption can occur that can make decoding fail.

Comment 22

[Julien Pierre]

Another possible legit reason for decode failure when reading the CRL from the DB is that the CRL got imported to the DB in a release that supported certain CRL formats (eg. distribution points) but the DB is being read in another release that doesn't.

Comment 23

[Julien Pierre]

Attachment: as discussed in person

Comment 24

[Julien Pierre]

Thanks for the review, Nelson. I checked this in to the tip.

Checking in crl.c;
/cvsroot/mozilla/security/nss/lib/certdb/crl.c,v  <--  crl.c
new revision: 1.59; previous revision: 1.58
done