Closed
Bug 177398
Opened 22 years ago
Closed 20 years ago
Add new certificate and CRL validation tests
Categories
(NSS :: Test, enhancement, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
3.10
People
(Reporter: wtc, Assigned: bishakhabanerjee)
Details
Attachments
(2 files, 4 obsolete files)
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.
Reporter | ||
Updated•22 years ago
|
Priority: -- → P1
Target Milestone: --- → 3.7
Comment 1•22 years ago
|
||
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.
Reporter | ||
Comment 2•22 years ago
|
||
Moved to target milestone 3.8 because the original NSS 3.7 release has been renamed 3.8.
Target Milestone: 3.7 → 3.8
Comment 3•22 years ago
|
||
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•22 years ago
|
||
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•21 years ago
|
||
Remove target milestone of 3.8, since these bugs didn't get into that release.
Target Milestone: 3.8 → ---
Assignee | ||
Updated•21 years ago
|
Target Milestone: --- → 3.9
Reporter | ||
Comment 6•21 years ago
|
||
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).
Reporter | ||
Comment 7•21 years ago
|
||
The URL for NIST's PKITS is http://csrc.nist.gov/pki/testing/x509paths.html.
Assignee | ||
Comment 8•21 years ago
|
||
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•21 years ago
|
||
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•21 years ago
|
||
Does PKITS include any CRLs ?
Comment 11•21 years ago
|
||
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.
Assignee | ||
Comment 12•21 years ago
|
||
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.
Attachment #132968 -
Attachment is obsolete: true
Assignee | ||
Comment 13•21 years ago
|
||
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•21 years ago
|
||
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.
Assignee | ||
Comment 15•21 years ago
|
||
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•21 years ago
|
||
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}
Assignee | ||
Comment 17•21 years ago
|
||
indentation/stylistic changes. Also if PKITS_DATA not set in environment, look for it under the mozilla source tree.
Attachment #133210 -
Attachment is obsolete: true
Comment 18•21 years ago
|
||
Comment on attachment 133448 [details]
stylistic changes
r=MisterSSL for first checkin.
Attachment #133448 -
Flags: review+
Assignee | ||
Comment 19•21 years ago
|
||
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•21 years ago
|
||
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.
Attachment #139769 -
Flags: review-
Assignee | ||
Comment 21•21 years ago
|
||
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.
Assignee | ||
Updated•21 years ago
|
Attachment #133448 -
Attachment is obsolete: true
Attachment #139769 -
Attachment is obsolete: true
Assignee | ||
Comment 22•21 years ago
|
||
this new script implements more suggestions from bug 231230 and fixes a typo.
Updated•21 years ago
|
Target Milestone: 3.9 → 3.10
Assignee | ||
Comment 23•20 years ago
|
||
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.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Comment 24•19 years ago
|
||
Comment on attachment 132968 [details]
script to run the NIST PKITS tests using vfychain
This attachment was not a patch
Attachment #132968 -
Attachment is patch: false
Comment 25•19 years ago
|
||
Comment on attachment 133210 [details]
new script
This attachment was not a patch
Attachment #133210 -
Attachment is patch: false
Comment 26•19 years ago
|
||
Comment on attachment 133448 [details]
stylistic changes
This attachment was not a patch
Attachment #133448 -
Attachment is patch: false
Comment 27•19 years ago
|
||
Comment on attachment 140273 [details]
review comments addressed
This attachment was not a patch
Attachment #140273 -
Attachment is patch: false
Comment 29•19 years ago
|
||
Bug was filed before 3.7 released, so marking it against 3.6
Version: 3.7 → 3.6
You need to log in
before you can comment on or make changes to this bug.
Description
•