231230 - pkits test script missing CRLs for tests 4.5.2 4.5.5 and 4.5.7

Status: RESOLVED FIXED

(NSS :: Test, defect)

Description

[Nelson Bolyard (seldom reads bugmail)]

The pkits.sh test script does not load the CRLs requisite for some of the tests.
Consequently tests 2, 5 and 7 of section 4.5 fail.  The verifications done for
those test cases are supposed to detect that some of the relevant certs have 
been revoked.  But because the CRLs are not loaded, the tests do not detect
the revocation, and consequently the verification succeeds when it should fail.

Comment 1

[Bishakha Banerjee]

The script I am running in my workspace is the new revision with CRLs. I have
not yet checked it in, let me clean up the debug stuff in it, and I'll do so
later today.
The script now also implements your suggestion of deleting all the CRLs not
needed for a particular test.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Please attach your patch to this bug for review, and wait for review before
checking it in.  Thanks.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Bishakha, 
You created a patched pkits.sh that adds the CRLs.  
You attached that patched file to bug 231030 
(I think it should have been attached to this bug).

This is a review comment on that patched file.

This latest version of pkits.sh puts all the certs into the cert db,
and then only supplies the leaf (EE) cert on the vfychain command line.
That does not simulate the actual behavior as used in client programs.
Please put ONLY the trusted CA certs (the ones with trust flags), and
those few others that are needed to import CLRs, into the certdb, and 
supply all other CA certs on the vfychain command line.
That's important for valid test results.
Please correct that.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

One more comment about this pkits.sh:

Even if the intermediate CA certs are already in the cert DB, they must
also be specified on the vfychain command line because that simulates the
real world test behavior.

This script looks good, very readable, and is well designed, I think.
It just needs the cert names in more places.  :)

Comment 5

[Bishakha Banerjee]

Attachment: script with intermed certs

ok, I re-added back the intermediate certs (that are already in the database)
into the vfychain command again as well, to simulate real-life conditions.

the last patch had only the Root Cert and the certs associated with the CRLs in
the database, others if needed by the test, *had* been provided to the vfychain
command.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 139768 [details]
script with intermed certs

This revision of the script adds two new shell functions, crlImport and 
certImport, both of which have some problems (they both have the same 
problems), which are:

1. they do not put the command being run into the output log
2. They send the stdout and stderr output of the commands to a file
   (which is good), but they do not subsequently
   a) examine the output for errors, and/or
   b) copy the output to stdout, where it will show up in the test log.
3. They do not take note of the results of the command.  If the command
   fails, they do not report this fact in the output log.  

Please change this so that:
a) every time the script runs certutil, crlutil, or vfychain, it echos
   the exact command being run into the output log,
b) If any of those programs fail (return non-zero), that is reported in
   the log, and 
c) any and all output of those programs is included in the log.

Look at the function pkits or pkitsn to see examples of examining
the output of the program for errors, and copying the program's output
into the test's output log (using cat).

Comment 7

[Bishakha Banerjee]

Attachment: review comments addressed

this addresses the review comments:

a) every time the script runs certutil, crlutil, or vfychain, it echos
   the exact command being run into the output log,

now they put the exact comment in the output log, previously they were omitting
the trust flags for certutil -A

b) If any of those programs fail (return non-zero), that is reported in
   the log, and 

they do now, similar to the pkits and pkitsn commands

c) any and all output of those programs is included in the log.

they do now, similar to the pkits and pkitsn commands

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 140265 [details]
review comments addressed

Review comments:

>pkits()
>{
>  echo "$SCRIPTNAME: ${VFY_ACTION} --------------------------"
>  echo "vfychain -d PKITSdb -u 4 $*"
>  vfychain -d $PKITSdb -u 4 $* > ${PKITSDIR}/cmdout.txt 2>&1
>  RET=$(grep -c ERROR ${PKITSDIR}/cmdout.txt)
>  cat ${PKITSDIR}/cmdout.txt
> 
>  if [ "$RET" -ne 0 ]; then

RET contains the count of "ERROR" lines in the output of vfychain,
but does not consider the actual program return value.	

I suggest something like this:

   vfychain -d $PKITSdb -u 4 $* > ${PKITSDIR}/cmdout.txt 2>&1
   RET=$? 
   RET=$(expr $RET + $(grep -c ERROR ${PKITSDIR}/cmdout.txt))
   cat ${PKITSDIR}/cmdout.txt


>pkitsn()
>{
>  echo "$SCRIPTNAME: ${VFY_ACTION} --------------------------"
>  echo "vfychain -d PKITSdb -u 4 $*"
>  vfychain -d $PKITSdb -u 4 $* > ${PKITSDIR}/cmdout.txt 2>&1
>  RET=$(grep -c ERROR ${PKITSDIR}/cmdout.txt)
>  cat ${PKITSDIR}/cmdout.txt

same issue as in pkits, and same suggested fix.


>crlImport()
>{
>  echo "$SCRIPTNAME: crlutil -d PKITSdb -I -i $*"
>  crlutil -d ${PKITSdb} -I -i $* > ${PKITSDIR}/cmdout.txt 2>&1
>  RET=$(grep -c ERROR ${PKITSDIR}/cmdout.txt)
>  cat ${PKITSDIR}/cmdout.txt
>}

a) Unlike vfychain, crlutil doesn't output the word "ERROR" when errors 
occur, so counting occurrences of ERROR doesn't work for this program.
For crlutil and certutil, use
    RET=$?

b) if these operations fail, shouldn't that generate some RED HTML?

c) maybe you need crlImport and crlImportn, so that negative test cases, 
where the CRL import is SUPPOSED to fail, will be handled as successes.

>delete()
>{
>  echo "$SCRIPTNAME: crlutil -d PKITSdb -D -n $*"
>  crlutil -d ${PKITSdb} -D -n $* >  ${PKITSDIR}/cmdout.txt 2>&1
>  RET=$(grep -c ERROR ${PKITSDIR}/cmdout.txt)
>  cat ${PKITSDIR}/cmdout.txt
>
>  echo "$SCRIPTNAME: certutil -d PKITSdb -D -n $*"
>  certutil -d $PKITSdb -D -n $* >  ${PKITSDIR}/cmdout.txt 2>&1
>  RET=$(grep -c ERROR ${PKITSDIR}/cmdout.txt)
>  cat ${PKITSDIR}/cmdout.txt
>}

Neither crlutil nor certutil output "ERROR", so don't count them.

>certImport()
>{
>  echo "$SCRIPTNAME: certutil -d PKITSdb -A -t \",,\" $*"
>  certutil -d $PKITSdb -A -t ",," $* >  ${PKITSDIR}/cmdout.txt 2>&1
>  RET=$(grep -c ERROR ${PKITSDIR}/cmdout.txt)
>  cat ${PKITSDIR}/cmdout.txt
>}

a) certutil doesn't output ERROR, so don't count it.

b) do you need certImport and certImportN?  (not sure)

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch to correct cert and CRL names

This patch corrects the names of several cert and crl files in the 
pkits.sh script shown above (labelled "review comments addressed").

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch to fix the tests for program failure

This patch changes pkits.sh to notice and report relevant failures.

Comment 11

[Bishakha Banerjee]

Nelson,

I'm going to discuss this extensively with you offline.

There are several issues raised here, that I need clarification on:

(1) This patch reports multiple instances of failure for one NIST testcase on
the results.html page (this is an undesirable feature)

(2) I have implemented the crlImportn (incorrect CRL import) function, however, 
    (a) for a particular NIST testcase, if the import of a CRL fails, then are 
 we to automatically conclude that NSS is doing things right, even though
vfychain (which looks at the CRLs) reports an incorrect chain to be correct?
That is, what would happen in a real world scenario?

    (b) what if the CRL is imported nonetheless (using the -B option, bypassing
the security check)? I have a script that does this, and I'd like to discucc teh
eresults with you.

    (c) in the negative testcases, if a CRL import fails, correctly, you have
said that vfychain is behaving correctly if it claims the chain is good.
Shouldn't vfychain say that the appropriate CRL was incorrect?

    (d) in the negative testcases with an incorrect CRL, the NIST PKITS testcase
(in many instances) lists a different reason for the chain to not invalidate.
I'll discuss a few of these cases with you with the PKITS document.

(3) Is there a reason to fail the NIST chaining test if an associated CRL fails
to be deleted from the database? In other words, shouldn't this be a more
"benign" error? I've noticed crlutil returning an error only when a CRL could
not be found in the database (when it was not imported due to valid reasons). 
FOr the NIST test cases, this is perfectly fine, since there is no CRL left
behid in teh database to affect the outcome of following tests.

Comment 12

[Bishakha Banerjee]

Corrections on my last set of questions/comments..

    (c) in the negative testcases, if a CRL import fails, correctly, you have
said that vfychain is behaving correctly if it claims the chain is good.
Shouldn't vfychain say that the appropriate CRL was incorrect?
should have said:
in the negative testcases, if a CRL import fails, correctly, you have
said that vfychain is behaving correctly if it claims the chain is good.
Shouldn't vfychain say that the appropriate CRL is not present, hence chain is
incorrect, or unable to be validated?

    (d) in the negative testcases with an incorrect CRL, the NIST PKITS testcase
(in many instances) lists a different reason for the chain to not invalidate.
I'll discuss a few of these cases with you with the PKITS document.

should have said:
    (d) in the negative testcases with an incorrect CRL, the NIST PKITS testcase
(in many instances) lists a different reason for the chain to invalidate.
I'll discuss a few of these cases with you with the PKITS document.

Comment 13

[Bishakha Banerjee]

The problem for which this bug was originally filed (pkits.sh test script does
not load the CRLs requisite for tests 2, 5 and 7 of section 4.5 causing them to
fail) has been corrected. Marking this bug FIXED.