Closed Bug 297738 Opened 16 years ago Closed 13 years ago

root cert module has no test coverage

Categories

(NSS :: Test, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
3.11.8

People

(Reporter: julien.pierre, Assigned: slavomir.katuscak+mozilla)

Details

Attachments

(1 file, 2 obsolete files)

Currently, no tests in all.sh ever attempt to load the root cert module, nssckbi
.  Even if the module has a crasher bug or doesn't initialize, all.sh won't
catch it. This is a big hole in the test coverage, and needs to change.
Assignee: jason.m.reid → nobody
QA Contact: jason.m.reid → test
Priority: -- → P2
Simple tests like adding the module with modutil should be added. Most tests from all.sh should be run with secmod.db to which the root cert module has been added.
Assignee: nobody → slavomir.katuscak
I prepared the test, but when I was testing it on Windows, I found out that nssckbi is already in secmod.db. 

Linux:
$ certutil -N -d . -f pwd
$ modutil -dbdir . -list                                  
Using database directory ....                                                   
Listing of PKCS #11 Modules
-----------------------------------------------------------
  1. NSS Internal PKCS #11 Module
         slots: 2 slots attached
        status: loaded

         slot: NSS Internal Cryptographic Services                            
        token: NSS Generic Crypto Services

         slot: NSS User Private Key and Certificate Services                  
        token: NSS Certificate DB
-----------------------------------------------------------

Windows:
$ certutil -N -d . -f pwd
$ modutil -dbdir . -list
Using database directory ....

Listing of PKCS #11 Modules
-----------------------------------------------------------
  1. NSS Internal PKCS #11 Module
         slots: 2 slots attached
        status: loaded

         slot: NSS Internal Cryptographic Services                            
        token: NSS Generic Crypto Services

         slot: NSS User Private Key and Certificate Services                  
        token: NSS Certificate DB

  2. Root Certs
        library name: ./nssckbi.dll
         slots: There are no slots attached to this module
        status: Not loaded
-----------------------------------------------------------

Do we plan to add there exception for Windows or rather test if it's already loaded ?
Slavo, you've run into Bug 319658: "Problems loading shared libs on Windows when paths start with "." or use forward slashes".  The simple workaround is to 
not use "." as the DB directory.  You may use $PWD instead, e.g. 
certutil -d "$PWD"  or  modutil -dbdir "$PWD" or -d `pwd`   etc.
Attached patch Patch to load cert module. (obsolete) — Splinter Review
Julien, please check this patch if this is what you want.
Attachment #265806 - Flags: review?
Comment on attachment 265806 [details] [diff] [review]
Patch to load cert module.

Slavo,

Yes, that's the basic idea.
I'm giving this r- because of

1) error checking. There are no valid circumstances under which the root cert module does not exist. Yet you check for it in root_module
+    if [ "${CERTFILE}" ] ; then
Your function then returns an error (0) if it's missing.
Unfortunately, none of your callers appear to check the return code. So, this will be a silent failure.
I think all testing should abort if the root cert module is missing or fails to load.

2) completeness. There are several other places in cert.sh where a database is created . 
There are others in cert_iopr.sh too, and cert_gen.sh .

In the long run, it should be added to run_niscc.sh too, but right now since there are some small known memory leaks in the root cert modules, we should not do that, as it may cause the NISCC test suite to run out of memory and stop. But for the rest of the QA it should be OK - though it will show additional leaks on your leak report. This will be a good occasion to fix them.
Attachment #265806 - Flags: review? → review-
Attached patch New patch. (obsolete) — Splinter Review
1. Workaround for bug 391658 (`cd ${DIR_NAME}; pwd`) - only for DB creation and root certs loading.
2. Added modu() function to wrap modutil (reports html pass/fail status).
3. Load root certs for every DB created. 

For me it looks like basic part of cert.sh was originally written by Vipul Gupta and later modified by many other people. Some part is systematic more and some less, maybe some cleanup can be good. 

First half of script checks return values and breaks function when error occures, second half just ignores it, prints error message (part of certu() function) and continues. This patch uses for every modutil call (root certs load) the same way as certutil call (db create).

Completeness:
cert.sh - Done.
cert_iopr.sh - Done.
cert_gen.sh - This script is not called from NSS test suite, it doesn't share variables (paths) and doesn't know path to library binaries, so it's problem to detect root certs path. Who and when use this script ?
Attachment #265806 - Attachment is obsolete: true
Attachment #267302 - Flags: review?(julien.pierre.boogz)
Attached patch Correct patch.Splinter Review
Attachment #267302 - Attachment is obsolete: true
Attachment #267303 - Flags: review?(julien.pierre.boogz)
Attachment #267302 - Flags: review?(julien.pierre.boogz)
Attachment #267303 - Flags: review?(julien.pierre.boogz) → review+
Trunk:

Checking in cert/cert.sh;
/cvsroot/mozilla/security/nss/tests/cert/cert.sh,v  <--  cert.sh
new revision: 1.39; previous revision: 1.38
done
Checking in iopr/cert_iopr.sh;
/cvsroot/mozilla/security/nss/tests/iopr/cert_iopr.sh,v  <--  cert_iopr.sh
new revision: 1.3; previous revision: 1.2
done
Comment on attachment 267303 [details] [diff] [review]
Correct patch.

Patch already running in trunk, asking for superreview for branch.
Attachment #267303 - Flags: superreview?(nelson)
Comment on attachment 267303 [details] [diff] [review]
Correct patch.


>+    echo "$MODUTIL $*"
>+    echo | $MODUTIL $*

Is the purpose of this to cause modutil to 
be run with stdin at end of file?  
If so, why?  What problem does that solve?
   (Add a comment explaining)
If not, then please get rid of the "echo | "
Attachment #267303 - Flags: superreview?(nelson) → superreview+
Modutil expects press of Enter key. This "echo | " solves this problem.
Small change in trunk:
Checking in cert/cert.sh;
/cvsroot/mozilla/security/nss/tests/cert/cert.sh,v  <--  cert.sh
new revision: 1.42; previous revision: 1.41
done

Branch:
Checking in cert/cert.sh;
/cvsroot/mozilla/security/nss/tests/cert/cert.sh,v  <--  cert.sh
new revision: 1.28.2.10; previous revision: 1.28.2.9
done
Checking in iopr/cert_iopr.sh;
/cvsroot/mozilla/security/nss/tests/iopr/cert_iopr.sh,v  <--  cert_iopr.sh
new revision: 1.2.2.2; previous revision: 1.2.2.1
done
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.11.8
You need to log in before you can comment on or make changes to this bug.