297738 - root cert module has no test coverage

Status: RESOLVED FIXED

(NSS :: Test, defect, P2)

Description

[Julien Pierre]

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.

Comment 1

[Julien Pierre]

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.

Comment 2

[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 ?

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 4

[Slavomir Katuscak]

Attachment: Patch to load cert module.

Julien, please check this patch if this is what you want.

Comment 5

[Julien Pierre]

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.

Comment 6

[Slavomir Katuscak]

Attachment: New patch.

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 ?

Comment 7

[Slavomir Katuscak]

Attachment: Correct patch.

Comment 8

[Slavomir Katuscak]

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 9

[Slavomir Katuscak]

Comment on attachment 267303 [details] [diff] [review]
Correct patch.

Patch already running in trunk, asking for superreview for branch.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

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 | "

Comment 11

[Slavomir Katuscak]

Modutil expects press of Enter key. This "echo | " solves this problem.

Comment 12

[Slavomir Katuscak]

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