177398 - Add new certificate and CRL validation tests

Status: RESOLVED FIXED

(NSS :: Test, enhancement, P1)

Description

[Wan-Teh Chang]

We need to add new certificate and CRL validation tests.
Most of these can be based on the new vfychain test program.

The input to these tests should include the certs and CRLs
that broke NSS before.  We should also ask PSM QA for test
certs and CRLs or ask the CMS team to generate some for us.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

We must not limit our tests to only certs that we can create with our 
own tools.  Many of the certs and chains that we should test with are 
only produced by OpenSSL cert generation tools.  Some of these certs 
can be gleaned from Bugzilla bug reports.

Comment 2

[Wan-Teh Chang]

Moved to target milestone 3.8 because the original
NSS 3.7 release has been renamed 3.8.

Comment 3

[Julien Pierre]

The vfychain program is not necessarily the tool to use for these tests. It
requires that the certificate chain be broken down in files each containing
individual DER certs.

Certs are sometimes as available as a PKCS#7 chain, or with their private keys
in PKCS#12 format. It is the later format that I used in most of my tests.
Having the private key enabled me not to just verify the certificate, but to use
certs in SSL client auth against a server, which had revocation enabled (CRL in
the server database). I think this sort of test should be included with the
exisitng SSL tests.

I would suggest that we have to write scripts to use the proper tools to create
the validation tests.

One thing - some of the CRLs I have are very large. If we integrate them into
the test suite, it will mean that our test machines will require large amounts
of RAM, probably at least 384 MB, to avoid swapping.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

In NSS 3.4 and in NSS 3.6, we had numerous regressions in our ability to
verify ordinary cert chains from SSL servers and email correspondents.
It would be atypicaly for such chains of certs to be present in p12 files, IMO.

We need a set of tests that use existing cert chains that may be 
unusual (not what typically comes out of certutil) but which worked with
NSS 3.3, and we need to automatically make sure that they continue to 
verify in future releases.

If vfychain needs to be enhanced to know about different formats of cert
files, so be it.  NSS has other functions for takeing in certs, such as
functions to read a "package" of certs.  vfychain could be enhanced to 
optinoally use those other functions instead.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Remove target milestone of 3.8, since these bugs didn't get into that release.

Comment 6

[Wan-Teh Chang]

NIST is constructing a Public Key Interoperability Test
Suite (PKITS), which is perfect for this RFE.  I suggest
that we first implement the Name Constraint tests to test
Nelson's new name constraint checking code (bug 208047).

Comment 7

[Wan-Teh Chang]

The URL for NIST's PKITS is
http://csrc.nist.gov/pki/testing/x509paths.html.

Comment 8

[Bishakha Banerjee]

Attachment: script to run the NIST PKITS tests using vfychain

I'm submitting for review a script (pkits.sh) that will run the NIST PKITS test
suite on NSS.

NOTES based on NSS meeting discussions:
pkits.sh is an optional set of tests, hence will not be included in all.sh.  it
resides in mozilla/security/nss/tests/pkits (note the new directory "pkits")

The data (certs) for testing need to be downloaded from the NIST website 
(http://csrc.nist.gov/pki/testing/x509paths.html under a link that says "Test
Data")and unzipped in to the folder "mozilla/PKITS_data"

The cert database will be created under "mozilla/PKITSdb"

At this point, the return codes for vfychain need to be changed (bug filed).
Once that is done, the html.results file will report correctly pass or fail of
the tests. Since a large number of these test cases are negative testcases, the
output of vfychain will need to be parsed to determine if the right NSS error
code is being output. On Wan-Teh's suggestion, another category in the
html.results file will be added for negative test cases, that of 
Partial Fail", i.e., the test fails, but with an incorrect error code.

This is a large script, will take time to review, submitting work done so far.
You can copy this script to mozilla/security/nss/tests/pkits, give execute
perms and run it.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Here's some feedback after a cursory review.

1. Please shorten all lines of asterisks to have fewer than 80 asterisks.
   72 would be a good number.

2. Please fold all lines that are longer than 80 columns using trailing 
   backslashes.  No line should exceed 80 columns in length.

3. Files created by this script (including DBs) should, IMO, go into 
   mozilla/tests_results/security/<hostname>.<n> just like the other test
   scripts do, so that multiple hosts can run the tests from the same 
   source directory and have their results not clobber each other.  

4. I suggest that if the user has an environment variable named PKITS_CERTS
that you use the content of that variable for $certs, rather than forcing
$certs to be under mozilla.  That way, we can run the tests without copying
all the pkits files into every source tree.  

Comment 10

[Julien Pierre]

Does PKITS include any CRLs ?

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Here are some more review comments.

1. Most (if not all) of the tests involve CRLs as well as certs.  The script
needs to load the CRLs into the DB during (or immediately after) the init 
step (which creates the DBs).

2. Rather than specifying "-d $PKITSdb" on every line that invokes the pkits
shell function, move the "-d $PKITSdb" into the pkits shell function itself.

3. The pkits shell function reports test failures for all negative tests, even
though they're expected to fail, if I'm not mistaken.  I suggest creating
another shell function, perhaps named pkitsn (for negative) that is just like
pkits except that it expects $RET to be non-zero.  Use that function for all
the negative tests.  There are other ways to solve this problem, that's just one
suggestion.

Comment 12

[Bishakha Banerjee]

Attachment: new script

Incorporated all of Nelson's comments but one.

Discussed remaining one comment regarding loading CRLs with Nelson on Friday. 
Issues to consider: There are a fair number of CRLs provided in the test data,
at last count 169.
vfychain does not explicitly require CRLs to be included, it however checks the
database for the CRLs. During my testing, I tested with not loading required
CRLS for some tests, and saw that even without the CRLs, vfychain was testing
correctly. This was only done for some tests, not all.
However, as Nelson pointed out, to be truly accurate to the tests, I should
include all the CRLS asked for. However, since most of the tests require their
own CRLs, this will be fairly time and space consuming to load these into the
db. 
So Nelson has suggested that I send him  teh output of pp on the crls, so he
can determine the optimal set of CRLS needed to be loaded.  I will also attach
that to this bug as well.

Comment 13

[Bishakha Banerjee]

Forgot to mention in last comment, environment variable for data (that you need
to set before executing this script) is PKITS_DATA, not PKITS_CERTS as Nelson
suggested.

Comment 14

[Julien Pierre]

Bishakha,

Note that the certificate database can currently only store one full CRL for
each CA subject. If you have multiple CRLs for the same CA, you won't be able to
load them at the same time in a single database. When you use crlutil it will
replace any old CRL with a newer one. If you need to backlevel a CRL, you need
to delete the existing one first before adding an older one.

Does the PKI test suite include any non-full CRLs, ie. delta CRLs or CRLs using
distribution points ? These are not currently supported by NSS.

Comment 15

[Bishakha Banerjee]

Julien,

Yes, there are (both delta and distrib pt crls):
deltaCRLCA1CRL.crl
deltaCRLCA1deltaCRL.crl
deltaCRLCA2CRL.crl
deltaCRLCA2deltaCRL.crl
deltaCRLCA3CRL.crl
deltaCRLCA3deltaCRL.crl
deltaCRLIndicatorNoBaseCACRL.crl
distributionPoint1CACRL.crl
distributionPoint2CACRL.crl
I haven't yet implemented the Test CRLs section, I'll keep your comments in
mind, and work around.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Two initial comments on this latest script:
1. Many lines still are longer than 80 columns.  For example, all tests in 
   section 4.13 exceed 80 columns.  There are other cases also.
   To fix this, I suggest:
   a) don't use stair-case indentation.  Just indent all continuations lines
      by the same amount as the first continuation line.
   b) reduce the number of asterisks that get echoed.
   b) fold ALL lines with multiple cert file names in them.

2. The new version requires the user to define $PKITS_DATA.  This should be
optional.  If the user doesn't set PKITS_DATA, the script should fall back
on using the directory you previously used, I think.  I suggest using the 
shell's conditional assignment operators to accomplish this.  E.g.
VARIABLE  =${VARIABLE:-word}

Comment 17

[Bishakha Banerjee]

Attachment: stylistic changes

indentation/stylistic changes. Also if PKITS_DATA not set in environment, look
for it under the mozilla source tree.

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 133448 [details]
stylistic changes

r=MisterSSL for first checkin.

Comment 19

[Bishakha Banerjee]

Attachment: PKITS test with the remainder sections

here are the other sections of the suite. The tests follow comments (stylistic
and command) in comments in bug 231230 for the remainder sections as well.

Some of these results (from the newer sections) have been verbally reviewed
with Nelson, but no bugs filed about them yet. Will do so on Monday.

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 139769 [details]
PKITS test with the remainder sections

The crlImport, certImport and delete functions all have the problem that
any errors they experience are completely hidden, not revealed in the output
log.  There is more info about this in bug 231230.

Please change the certImport function so that it takes a single argument
that is used as both the nickname and the cert file name.  The function
itself can add the $certs/ prefix and .crt suffix to the nickname to 
produce the file name.

Comment 21

[Bishakha Banerjee]

Attachment: review comments addressed

this has the comments from bug 232479 and 231230 (same comments in both bugs)
addressed, as well as the stylistic comments of:
changing the certImport function so that it takes a single argument
to be used as both the nickname and the cert file name.

Comment 22

[Bishakha Banerjee]

Attachment: newest script

this new script implements more suggestions from bug 231230 and fixes a typo.

Comment 23

[Bishakha Banerjee]

Marking FIXED.

All tests from the NIST PKITS document (for certificate and CRL validation tests
and other tests) have been written, reviewed and checked in.

The only tests not implemented from the NIST PKITS section are the delta CRL and
distribution point CRL tests (regular CRL tests have been implemented). The
tests that check for Certificate Policies have also been written and checked in.

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 132968 [details]
script to run the NIST PKITS tests using vfychain

This attachment was not a patch

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 133210 [details]
new script

This attachment was not a patch

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 133448 [details]
stylistic changes

This attachment was not a patch

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 140273 [details]
review comments addressed

This attachment was not a patch

Comment 28

[Nelson Bolyard (seldom reads bugmail)]

RFE was originally filed against NSS 3.7

Comment 29

[Nelson Bolyard (seldom reads bugmail)]

Bug was filed before 3.7 released, so marking it against 3.6