324887 - merge ECC and non-ECC QA test scripts

Status: RESOLVED FIXED

(NSS :: Test, enhancement, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

In the NSS QA test script source directory .../mozilla/security/nss/tests 
where the all.sh script lives, several of the subdirectories contain two
versions of the test script, an ECC version and a non-ECC version.  
One directory also contains other files (not scripts) that have both ECC
and non-ECC versions.  Here is a list.

Directory   Original Non ECC script   New Non ECC script    ECC script
----------- -----------------------  --------------------- --------------
cert        cert.sh                  noeccert.sh           eccert.sh
tools       tools.sh                 noectools.sh          ectools.sh
smime       smime.sh                 noecsmime.sh          ecsmime.sh
ssl         ssl.sh                   noecssl.sh            ecssl.sh
            sslauth.txt              noecsslauth.txt       ecsslauth.txt
            sslcov.txt               noecsslcov.txt        ecsslcov.txt
            sslstress.txt            noecsslstress.txt     ecsslstress.txt

The files in the first and third columns are checked in.  
The files in the second column are created by a script (see below).  

In the mozilla/security/nss/tools directory lives a script named fixtests.sh.
This script is run with one of two argument values: enable_ecc or disable_ecc.
This script does the following things:
  1. if the noec* files (in the second columns above) do not exist, 
     create them as copies of the respective files in the first column.
  2. if the argument is enable_ecc, copy each of the ec* files (third column)
     over on top of the corresponding file in the first column, else
  3. if the argument is disable_ecc, copy each of the noec files (second 
     column) over the top of the corresponding file in the first column.

Thus the script changes the files in the first column to be either the ECC
or non-ECC versions.  The idea is that one runs fixtests to select a mode of
operation, and then thereafter runs all.sh, which runs the selected scripts.

There are several problems with this approach.
1. It is not possible to do both the ECC and non-ECC tests in the same run 
   of all.sh

2. The two scripts in each directory tend to get out of sync.  Changes are
   often made to the non-ECC script, and not to the corresponding ECC script.

3. The nss/tests directory is part of the source tree, and should be treated
   as read only by the QA tests.  The QA tests should not modify or add to 
   the contents of the source directories in any way.  Multiple systems
   should be able to share a common read-only version of this directory 
   for simultaneous testing.  But the act of switching between ECC and 
   non-ECC scripts modifies the source tree, affecting all systems.  

4. if one attempts to run one set of tests, and then run the other set of 
tests in the same tests_results directory, the second set fails, usually
due to assumptions that both scripts make about starting with clean 
directories.  

So, the challenge is to merge the ECC and non-ECC scripts and configuration
files (for SSL) into a single set that chooses whether to run ECC or non-ECC
or both sets of tests according to an environment variable or some other 
means, other than modifying source files.  

We need this very soon, (in the next week or two).

Comment 1

[Vipul Gupta]

(In reply to comment #0)

I'm glad to see this effort. Just a point of clarification. Originally,
the ECC scripts were created as supersets of the non-ECC scripts
(to avoid the first problem mentioned in Nelson's list) but over time, the
ECC scripts haven't kept up with all of the changes in their
non-ECC counterparts.

The reason we couldn't initially replace the non ECC scripts
with the ECC scripts (even though the latter were a superset)
was that ECC support wasn't always enabled. How are we planning
to tackle this issue? Could one write a test program that returns
true/false depending on whether NSS was compiled with 
NSS_ENABLE_ECC set to 1?

> There are several problems with this approach.
> 1. It is not possible to do both the ECC and non-ECC tests in the same run 
>    of all.sh

Comment 2

[Wan-Teh Chang]

Vipul, the easiest solution is for the test scripts to
check the same environment variable NSS_ENABLE_ECC that
is checked by the build system.

Comment 3

[Christophe Ravel]

Attachment: Patch for cert.sh

Use NSS_ENABLE_ECC to run the ECC tests in addition of the regular tests.
We test is NSS_ENABLE_ECC is set as it is the case in the source code (not only =1).

I have just added the additional ECC parts from eccert.sh to cert.sh.
One exception is for html_head: added "with ECC"

Comment 4

[Vipul Gupta]

I've looked at the patch briefly and have one high
level comment. I suggest folding:

1) cert_create_certs into cert_create_cert
2) cert_add_certs into cert_add_cert

The additional ECC actions in the merged
functions would be only performed if ECC is
enabled, i.e. the ECC compilation test should
be moved lower down within these functions.

IINM, both cert_create_cert and cert_create_certs
can get called resulting in a reinitialization of cert
and key databases!

vipul

Comment 5

[Christophe Ravel]

I was tempted to merge cert_create_certs with cert_create_cert, but the original eccert.sh uses the 2:

cert_smime_client()
{
  CERTFAILED=0
  echo "$SCRIPTNAME: Creating Client CA Issued Certificates =============="

  cert_create_certs ${ALICEDIR} "Alice" 30 ${D_ALICE}
  cert_create_certs ${BOBDIR} "Bob" 40  ${D_BOB}

  echo "$SCRIPTNAME: Creating Dave's Certificate -------------------------"
  cert_create_cert "${DAVEDIR}" Dave 50 ${D_DAVE}

  echo "$SCRIPTNAME: Creating multiEmail's Certificate --------------------"
  cert_create_cert "${EVEDIR}" "Eve" 60 ${D_EVE} "-7 [e-mail],[e-mail],[e-mail]"

------

Also, could you clarify what you meant by "the ECC compilation test should
be moved lower down within these functions" ?

Comment 6

[Christophe Ravel]

Attachment: Patch for tools.sh

Using same method as for cert.sh.

Added test for ECC: Listing Alice's pk12 EC file (pk12util -l)
This test was existing for the non-ECC part.

Comment 7

[Vipul Gupta]

(In reply to comment #5)

> I was tempted to merge cert_create_certs with cert_create_cert, but the
> original eccert.sh uses the 2:

  This is true but my sense is that these remaining calls to cert_create_cert
  could be replaced by calls to cert_create_certs -- it is just that the EC certs
  for Dave and Eve won't be used by any tests (since we do not yet support
  ECC based encrypted S/MIME messages ... we only support ECC based
  signed S/MIME messages).

> cert_smime_client()
> {
>   CERTFAILED=0
>   echo "$SCRIPTNAME: Creating Client CA Issued Certificates =============="
> 
>   cert_create_certs ${ALICEDIR} "Alice" 30 ${D_ALICE}
>   cert_create_certs ${BOBDIR} "Bob" 40  ${D_BOB}
> 
>   echo "$SCRIPTNAME: Creating Dave's Certificate -------------------------"
>   cert_create_cert "${DAVEDIR}" Dave 50 ${D_DAVE}
> 
>   echo "$SCRIPTNAME: Creating multiEmail's Certificate --------------------"
>   cert_create_cert "${EVEDIR}" "Eve" 60 ${D_EVE} "-7
> [e-mail],[e-mail],[e-mail]"
> 
> ------
> 
> Also, could you clarify what you meant by "the ECC compilation test should
> be moved lower down within these functions" ?

   I can see how I wasn't very clear :-). All I meant was that the additonal
   ECC related code in cert_create_certs and cert_add_certs should be preceded
   by the check for NSS_ENABLE_ECC.

   thanks,

   vipul

Comment 8

[Vipul Gupta]

One more thing ...

I'd recommend setting CA_CURVE to secp521r1 and CURVE to secp256r1.
There are two motivations for this. 

a) Unlike secp160r1 and secp160r2, these two curves are part of 
NIST Suite B and implemented by other vendors
so testing them is more important.

b) By including some large curves (secp521r1 is our largest
supported GF(p) curve) in our tests, we can potentially uncover
problems like those in bug 319619 sooner rather than later.
(see https://bugzilla.mozilla.org/show_bug.cgi?id=319619#c6)

vipul

Comment 9

[Christophe Ravel]

Attachment: Patch for smime.sh

Same idea here too.

Comment 10

[Christophe Ravel]

(In reply to comment #8)
> One more thing ...
> 
> I'd recommend setting CA_CURVE to secp521r1 and CURVE to secp256r1.
> There are two motivations for this. 
> 
> a) Unlike secp160r1 and secp160r2, these two curves are part of 
> NIST Suite B and implemented by other vendors
> so testing them is more important.

Vipul, we currently have:
  cert_all_CA: CA_CURVE="secp160r1"
  cert_extended_ssl: EC_CURVE="sect163r1"
It is sligtly difference from your comment above.

I feel comfortable to merge the tests, but I have no knowledge of ECC.
Shouldn't this curve change be part of a specific bug ?

Comment 11

[Christophe Ravel]

Attachment: Patch v2 for cert.sh

Folds cert_create_certs into cert_create_cert as suggested by Vipul.
There was already only 1 version of cert_add_cert.

Comment 12

[Vipul Gupta]

(In reply to comment #10)
> Vipul, we currently have:
>   cert_all_CA: CA_CURVE="secp160r1"
>   cert_extended_ssl: EC_CURVE="sect163r1"
> It is sligtly difference from your comment above.

   Ah ... I had overlooked EC_CURVE and it seems that
   the new patch forgot to carry over CURVE (from the 
   existing eccert.sh -- first line of code in cert_add_certs)

   So there are really three curves that are used in the
   current version of eccert.sh -- secp160r2 (CURVE),
   secp160r1 (CA_CURVE), sect163r1 (EC_CURVE).

   I'd recommend replacing these with the three Suite B
   curves that are implemented by other large vendors.
   CA_CURVE = secp521r1, CURVE=secp384r1,
   EC_CURVE=secp256r1

   Wan-Teh, are you ok with this change? One drawback
   of this suggestion is that we no longer test GF(2^m), i.e.
   the sect* curves. Perhaps we can change EC_CURVE to
   sect571r1 as a way to test our largest GF(2^m) curve.


> I feel comfortable to merge the tests, but I have no knowledge of ECC.
> Shouldn't this curve change be part of a specific bug ?

    I don't think we need to create a new bug for this change
    but I'll defer that decision to Wan-Teh.

    vipul

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Vipul, please review these patches in my place.  Thanks.

Comment 14

[Christophe Ravel]

Attachment: Patch v3 for cert.sh

Change curves as suggested by Vipul:
   CA_CURVE = secp521r1
   CURVE=secp384r1
   EC_CURVE=secp256r1

Add CRL generation for ECC in cert_crl_ssl and cert_ec_CA functions.

Comment 15

[Christophe Ravel]

Attachment: Patch for ssl.sh (and sslcov.txt, sslauth.txt, sslstress.txt)

In the files sslcov.txt, sslauth.txt, sslstress.txt:
- Add a column to tell if this is for ECC (similar to TLS/noTLS)
- ECC means usable for ECC enabled only
- noECC means usable anytime

In ssl.sh, if ECC is NOT enabled, we skip the cases when the previous flag is "ECC"
Added support for CRL test with ECC.

Some ECC SSL cipher tests still don't pass (C001 to C00F). ECC SSL ciphers C010 to C014 pass the test.

Comment 16

[Christophe Ravel]

Comment on attachment 209882 [details] [diff] [review]
Patch for tools.sh

Change reviewer to Vipul

Comment 17

[Christophe Ravel]

Comment on attachment 209895 [details] [diff] [review]
Patch for smime.sh

Change reviewer to Vipul.

Comment 18

[Christophe Ravel]

Attachment: Test results for ssl.sh

Tests on Solaris 9 SPARC

Comment 19

[Christophe Ravel]

Attachment: Test output for ssl.sh

Same as test results for ssl.sh

Comment 20

[Vipul Gupta]

Hi Cristophe,

   In the log file, I noticed that the ECC tests fail with the error:
"Cannot communicate securely with peer: no common encryption algorithm(s)."
This can happen if selfserv hasn't been configured with an ECC 
certificate. If you look inside the start_selfsrv() function in ecssl.sh,
you'll notice that selfserv needs to be given an additional argument
(search for "-e" in this function).

   From what I can tell, this change has not been carried over to the
new test script. Note that the -e argument should not be passed
when ECC support has been compiled in.

vipul

Comment 21

[Vipul Gupta]

(In reply to comment #20)

> Note that the -e argument should not be passed
> when ECC support has been compiled in.

  That's a typo. I meant to say

   the -e argument should be passed only when ECC 
   support has been compiled in

Comment 22

[Christophe Ravel]

(In reply to comment #21)
>    the -e argument should be passed only when ECC 
>    support has been compiled in
> 
You're absolutely right. I made this change and all the SSL tests are passed.

I'll submit a new patch for ssl.h and also include results and output for review.

Comment 23

[Christophe Ravel]

Attachment: Patch v2 for ssl.sh

Start selfserv with -e option when ECC is enabled.

All SSL tests are now green.

Comment 24

[Christophe Ravel]

Attachment: Results for ssl.sh (with patch v2)

Comment 25

[Christophe Ravel]

Attachment: Output for ssl.sh (with patch v2)

Comment 26

[Vipul Gupta]

Attachment: Patch v2 for smime

Moved ECDSA tests inside smime_sign

Comment 27

[Vipul Gupta]

In the smime patch, I moved the ECDSA tests inside smime_sign
and added a note to expand the ECDSA tests to 
cover hash functions other than SHA1 after
bug 320583 is resolved. (see Patch v2 for smime).

vipul

Comment 28

[Vipul Gupta]

When running the merged scripts on my Mac OS X laptop, I
repeatedly saw the following messages in output.log

./all.sh: line 333: [: =: unary operator expected

and 

./all.sh: line 346: [: =: unary operator expected

Here's an example (the complaint seems to originate in ssl.sh)

ssl.sh: Stress SSL2 RC4 128 with MD5 ----
selfserv -D -p 8443 -d ../server -n dummy.domain  \
         -e dummy.domain-ec -w nss   -i ../tests_pid.3820  &
selfserv started at Sat Feb  4 12:44:53 PST 2006
tstclnt -p 8443 -h dummy.domain -B -s -q \
        -d ../client < /Users/vgupta/Projects/ECC/NSS/nss-20060201/mozilla/security/nss/tests/ssl/sslreq.dat
./all.sh: line 333: [: =: unary operator expected
Strsclnt -q -p 8443 -d ../client -B -s -w nss -c 1000 -C A \
          dummy.domain
strsclnt started at Sat Feb  4 12:44:54 PST 2006
strsclnt: -- SSL: Server Certificate Validated.
strsclnt: 0 cache hits; 0 cache misses, 0 cache not reusable
strsclnt: 1 server certificates tested.
strsclnt completed at Sat Feb  4 12:44:59 PST 2006
./all.sh: line 346: [: =: unary operator expected

Comment 29

[Vipul Gupta]

Hi Christophe,

In the results.html file, there are three consecutive lines that say:
"Import kyril.red.iplanet.com's EC Cert"
but the equivalent test for the non-EC cert happens only once.
I haven't figured out why but I thought I should point this out.

vipul

Comment 30

[Vipul Gupta]

Attachment: Patch v4 for cert.sh

Fixed type (certu -> crlu); Added check for NSS_ENABLE_ECC before removing ecroot.cert files;
Added commands to import EC certs also in cert_smime_client even though we don't yet
support S/MIME encryption using ECC; Added chmod 600 ${CRL_FILE_GRP_3}

Comment 31

[Wan-Teh Chang]

Vipul,

That error message is most likely because some of the
variables in a shell script are not quoted.  An example
is this one:

http://lxr.mozilla.org/security/source/security/nss/tests/ssl/ssl.sh#309

309       elif [ $value != "#" ]; then

Here $value is not quoted, so if the variable 'value'
has an empty value, that != test will be

       != "#"

and will generate an error about unary operator.

This particular problem with the variable 'value' can
be avoided by making sure the sslstress.txt file doesn't
have any blank line.

Comment 32

[Wan-Teh Chang]

Comment on attachment 210200 [details] [diff] [review]
Patch v2 for ssl.sh

In sslstress.txt, we have:

>+   ECC      0      -c_:C013  -c_100_-C_:C013     Stress TLS ECDHE-RSA AES 128 CBC with SHA
>+
> #
> # add client auth versions here...
> #

This is a blank line that would cause the
unary operator error.

Comment 33

[Christophe Ravel]

(In reply to comment #29)
> In the results.html file, there are three consecutive lines that say:
> "Import kyril.red.iplanet.com's EC Cert"

The issue is with some calls to "certu". "certu" uses the variable CU_ACTION to describe what it is doing and print it in the test report.
If CU_ACTION is not set for a call to "certu", it is reusing the previous one.

In this case, the missing CU_ACTION is in the "cert_ssl" function (cert/cert.sh).

cert_ssl()
{
...
  certu -M -n "TestCA" -t "TC,TC,TC" -d ${PROFILEDIR}
if [ ! -z "$NSS_ENABLE_ECC" ] ; then
  certu -M -n "TestCA-ec" -t "TC,TC,TC" -d ${PROFILEDIR}
fi
...
}
"certutil -M" does "Modify trust attributes of certificate".

I would suggest that we set CU_ACTION to "Modify trust attributes of [TestCA|TestCA-ec] -t TC,TC,TC".

  CU_ACTION="Modify trust attributes of TestCA -t TC,TC,TC"
  certu -M -n "TestCA" -t "TC,TC,TC" -d ${PROFILEDIR}
if [ ! -z "$NSS_ENABLE_ECC" ] ; then
  CU_ACTION="Modify trust attributes of TestCA-ec -t TC,TC,TC"
  certu -M -n "TestCA-ec" -t "TC,TC,TC" -d ${PROFILEDIR}
fi

What do you think ?

Comment 34

[Christophe Ravel]

Attachment: Patch v3 for ssl

Fix a couple of issues reported by Wan-Teh and Vipul:
- Remove blank line in sslstress.txt
- Add quotes for test of $tls (if [ "$tls" = "TLS" ]).

Comment 35

[Vipul Gupta]

(In reply to comment #33)

Your suggested change looks good. I recommend using the
phrase "Root CA" (and "EC Root CA") instead of "Test CA"
(and "TestCA-ec") for consistency with other CU_ACTIONs.
 
> I would suggest that we set CU_ACTION to "Modify trust attributes of
> [TestCA|TestCA-ec] -t TC,TC,TC".
 
> What do you think ?

Comment 36

[Vipul Gupta]

Attachment: Patch v5 for cert.sh

Made the CU_ACTION change as per comment 35 and
fixed a typo in a comment (the function cert_extended_ssl
was mislabeled as cert_ssl in the comments).

Comment 37

[Vipul Gupta]

Hi Christophe,

   On my Mac OS X, `uname -n` returns an empty string
which causes the "unary operator" errors inside ssl.sh so
it might make sense to replace the two occurrrences with
"`uname -a`".

   I noticed a couple of things about load_group_crl
(a) It is never called with ECC as its second argument
(b) Inside its definition, in the part that says echo ... Updating DB ...
  I don't see any difference between how crlu is invoked for
  the ECC and non-ECC cases. Unless I'm missing something,
  it should be possible to simple have the CU_ACTION string be
  different, as shown below.

I already have a patch with these modifications and if you
agree with these changes, I'll upload that patch as "Patch v4 for ssl"

What do you think?

vipul

....

        echo "=== Updating DB for group $grpBegin - $grpEnd and restarting selfserv ====="

        kill_selfserv
        if [ "$ectype" = "noECC" ] ; then
            CU_ACTION="Importing CRL for groups $grpBegin - $grpEnd"
        else
            CU_ACTION="Importing ECC CRL for groups $grpBegin - $grpEnd"
        fi
        crlu -d ${R_SERVERDIR} -I -i ${SERVERDIR}/root.crl_${grpBegin}-${grpEnd} \
             -p ../tests.pw.928
        ret=$?
        if [ "$ret" -eq 0 ]; then
            return 1
        fi
        start_selfserv

...

Comment 38

[Christophe Ravel]

(In reply to comment #37)
Looks good to me.

Comment 39

[Vipul Gupta]

(In reply to comment #38)
> (In reply to comment #37)
> Looks good to me.

I looked at load_group_crl() again and noticed that my earlier
suggestion in comment #37 is incorrect, crlu (in patch v3 for ssl)
uses different values for the -i flag (there is a -ec suffix in the file
used for the ECC case). I didn't see the -ec part when
I did an sdiff (it cuts off characters at the end if a line is too long).

I'm unclear about the crl tests and still don't understand why
load_group_crl is never called with ECC as its second 
argument? 


Some other things I'm confused about:

a) In cert.sh (after patch v5), shouldn't a new CU_ACTION  be set 
just before line 989 where we have:

if [ ! -z "$NSS_ENABLE_ECC" ] ; then
  crlu -d $CADIR -G -n "TestCA-ec" -f ${R_PWFILE} -o ${CRL_FILE_GRP_1}_or-ec <<EOF_CRLINI
update=$CRLUPDATE
addcert ${CRL_GRP_1_BEGIN}-${CRL_GRP_END_} $CRL_GRP_DATE
addext reasonCode 0 4
addext issuerAltNames 0 "rfc822Name:[e-mail]|dnsName:ca.com|x400Address:x400Address|directoryName:CN=NSS Test CA,O=BOGUS NSS,L=Mountain View,ST=California,C=US|URI:http://ca.com|ipAddress:192.168.0.1|registerID=reg CA"
EOF_CRLINI
  CRL_GEN_RES=`expr $? + $CRL_GEN_RES`
  chmod 600 ${CRL_FILE_GRP_1}_or-ec

b) This code creates  ${CRL_FILE_GRP_1}_or-ec which maps to
${R_SERVERDIR}/root.crl_${CRL_GRP_1_BEGIN}-${CRL_GRP_END}_or-ec
but the ssl.sh (after patch v3) script makes references to 
a file ending in -ec not _or-ec.

vipul

Comment 40

[Vipul Gupta]

(In reply to comment #36)
> Created an attachment (id=210911) [edit]
> Patch v5 for cert.sh
> 
> Made the CU_ACTION change as per comment 35 and
> fixed a typo in a comment (the function cert_extended_ssl
> was mislabeled as cert_ssl in the comments).
> 

Looks like we need to insert new EC-specific CU_ACTIONS
at several other places. Here are some other lines from
the results.html (https://bugzilla.mozilla.org/attachment.cgi?id=210201)

Testing Certificate Key Usage Extension	Passed
Testing Certificate Key Usage Extension	Passed
Testing Certificate Basic Constraints Extension	Passed
Testing Certificate Basic Constraints Extension	Passed
Testing Certificate Authority Key Identifier Extension	Passed
Testing Certificate Authority Key Identifier Extension	Passed
Testing CRL Distribution Points Extension	Passed
Testing CRL Distribution Points Extension	Passed

....

Generating CRL for range 40-42 TestCA authority	Passed
Generating CRL for range 40-42 TestCA authority	Passed
Modification CRL by adding one more cert	Passed
Modification CRL by adding one more cert	Passed
Modification CRL by removing one cert	Passed
Modification CRL by removing one cert	Passed
Creating CRL for groups 1 and 2	Passed
Creating CRL for groups 1 and 2	Passed
Creating CRL for groups 1, 2 and 3	Passed
Creating CRL for groups 1, 2 and 3	Passed
Importing CRL for groups 1	Passed
Importing CRL for groups 1	Passed

Christophe, can you take a look at this. I'm really not sure
about the CRL utility. Back when we first contributed the ECC
code, I only updated certutil to be ECC aware ... not sure if 
crlutil was even around then.

vipul

Comment 41

[Vipul Gupta]

I noticed that cert.sh and ssl.sh started dealing with CRLs
only in April 2005 (cert.sh v1.26, ssl.sh v1.59).
Hence, the ECC tests that were modelled on earlier versions
of these scripts make no references to CRLs. In fact, unlike
certutil, crlutil was never made ECC aware (I don't know yet
what this entails).

Therefore, I suggest that the merged scripts should 
not attempt to use CRLs with the ECC code. 
The task of making crlutil ECC aware and enhancing
ECC tests to work with CRLs should be undertaken as
a separate bug.

If folks are ok with this suggestion, I can quickly create
new patches for cert.sh and ssl.sh. We won't be losing
anything because the current ECC tests do not deal with
CRLs even now. 

BTW, the latest patches for tools.sh and smime.sh 
look fine to me. 

vipul

Comment 42

[Julien Pierre]

Vipul,

There shouldn't be anything in the CRL code that's specific to RSA for signatures. As far as library code, nss uses CERT_VerifySignedData . If that can work on an ECDSA signature and cert, then there shouldn't be any code change needed.
I recommend that you should attempt to sign CRLs with the ECC code and update the test scripts to be current.

Comment 43

[Christophe Ravel]

(In reply to comment #42)
Julien,

Could you review the CRL part in cert.sh and ssl.sh ?
This is a part that didn't exist in eccert.sh and ecssl.sh. I tried to make it similar for the ECC part to what was existing for the no-ECC part.

Comment 44

[Julien Pierre]

Comment on attachment 210899 [details] [diff] [review]
Patch v3 for ssl

Christophe,

This patch looks like it does the job, but at the expense of duplicating a lot of blocks of code. For example in load_group_crl you do :

+      if [ "$ectype" = "noECC" ] ; then
         echo "================= Reloading CRL for group $grpBegin - $grpEnd ============="
 
         echo "tstclnt -p ${PORT} -h ${HOSTADDR} -f -d ${R_CLIENTDIR} \\"
@@ -481,10 +506,33 @@
             ret=1
             return 1
         fi
+      else
+        echo "================= Reloading ECC CRL for group $grpBegin - $grpEnd ============="
+
+ whole bunch of stuff duplicated from above

There should be no need for the dpulication. I believe the only thing that changes is the nickname of the signing cert and the filename for the CRL. How about only putting those only in the if statements by using intermediate variables, and having the rest of the logic only once ? This will make it much easier to maintain and enhance the scripts going forward.

Comment 45

[Christophe Ravel]

Attachment: Patch v4 for ssl

Take into account Julien's comment about avoiding duplicate code for the CRL tests.

Comment 46

[Vipul Gupta]

Attachment: Patch v6 for cert.sh

Patch v6 for cert.sh fixes S/MIME test failures we were seeing
with the v5 patch. The issue is described in more detail within
comments added in v6 (quoted below):

+## XXX With this new script merging ECC and non-ECC tests, the
+## call to cert_create_cert ends up creating two separate certs
+## one for Eve and another for Eve-ec but they both end up with
+## the same Subject Alt Name Extension, i.e., both the cert for
+## [e-mail] and the cert for [e-mail] end up 
+## listing [e-mail] in the Certificate Subject Alt Name extension. 
+## This can cause a problem later when cmsutil attempts to create
+## enveloped data and accidently picks up the ECC cert (NSS currently
+## does not support ECC for enveloped data creation). This script
+## avoids the problem by ensuring that these conflicting certs are
+## never added to the same cert database (see comment marked XXXX).

vipul

Comment 47

[Christophe Ravel]

Vipul,

In comment #7, you said:
>  This is true but my sense is that these remaining calls to cert_create_cert
>  could be replaced by calls to cert_create_certs -- it is just that the EC certs
>  for Dave and Eve won't be used by any tests (since we do not yet support
>  ECC based encrypted S/MIME messages ... we only support ECC based
>  signed S/MIME messages).

We just found out that it actually matters: it makes cmsutil crash.

Do you think this crash is in cmsutil or in the NSS libraries? In the 2nd case, I guess this is a serious bug and it should be fixed.

Comment 48

[Vipul Gupta]

(In reply to comment #47)
> Vipul,
> 
> In comment #7, you said:
> >  This is true but my sense is that these remaining calls to cert_create_cert
> >  could be replaced by calls to cert_create_certs -- it is just that the EC certs
> >  for Dave and Eve won't be used by any tests (since we do not yet support
> >  ECC based encrypted S/MIME messages ... we only support ECC based
> >  signed S/MIME messages).
> 
> We just found out that it actually matters: it makes cmsutil crash.
> 
> Do you think this crash is in cmsutil or in the NSS libraries? In the 2nd case,
> I guess this is a serious bug and it should be fixed.

    Hi Christophe,

     I haven't seen cmsutil crash when it tries to create enveloped
    data using ECC (a feature not currently supported). Could you 
    please try to reproduce what I did (outlined below) and let me
    know if you see a different result:

    1) I created an ECC-enabled build of NSS and ran the tests
    with Patch v5 of cert.sh

    2) The test scripts create a directory called smime and I did:

      % cd smime
      % cmsutil -E -i alice.txt -d ../alicedir -o temp.env -r [e-mail]

    3) This resulted in an error but I didn't see a crash:

      % cmsutil -E -i alice.txt -d ../alicedir -o temp.env -r [e-mail]
cmsutil: cannot find certificate for "[e-mail]": security library: bad database.
cmsutil: problem enveloping: security library: bad database.
      %

    4) However, I do see an assertion failure when the test is run as part of 
      smime.sh:

      Assertion failure: secmod_PrivateModuleCount == 0, at pk11util.c:120

     I plan to take a closer look at it today.

    vipul
     

Comment 49

[Christophe Ravel]

(In reply to comment #48)
>     4) However, I do see an assertion failure when the test is run as part of 
>       smime.sh:
> 
>       Assertion failure: secmod_PrivateModuleCount == 0, at pk11util.c:120

With an optimized build, we have the following output:
smime.sh: Testing multiple email addrs ------------------------------
cmsutil -E -i alicecc.txt -d ../alicedir -o aliceve.env \
        -r [e-mail]
ERROR: cannot create CMS recipientInfo object.
cmsutil: problem enveloping: security library: invalid algorithm.
cmsutil: NSS_Shutdown failed: NSS could not shutdown. Objects are still in use.
smime.sh: Encrypt to a Multiple Email cert . FAILED

With a debug build, we have the following output:
smime.sh: Testing multiple email addrs ------------------------------
cmsutil -E -i alicecc.txt -d ../alicedir -o aliceve.env \
        -r [e-mail]
ERROR: cannot create CMS recipientInfo object.
cmsutil: problem enveloping: security library: invalid algorithm.
Assertion failure: secmod_PrivateModuleCount == 0, at pk11util.c:120
Abort - core dumped
smime.sh: Encrypt to a Multiple Email cert . FAILED

I would suggest that we open a different bug to track this issue.

I have tested with patch v6 for cert.sh and all tests are passing. However, I believe that the goal of running QA is not to have passed tests but to find bugs before they hit customers.
- If this is an issue within the NSS libraries, I would prefer seeing the test fail until the bug is fixed.
- If this is just a case not handled gracefully by cmsutil (a QA tool), then we can avoid this case until we resolve the problem with cmsutil.

Any other expert view on this subject ?

Comment 50

[Nelson Bolyard (seldom reads bugmail)]

When the environment variable NSS_STRICT_SHUTDOWN is defined, SECMOD_Shutdown 
(part of NSS_Shutdown) asserts that all the PKCS#11 modules have succesfully 
unloaded. Assertions cause core dumps in debug builds only, and do nothing in
optimizzed builds.  

NSS's Softoken PKCS#11 module does not unload if there are any object reference leaks; that is, if any of its objects' reference counts have not gone to zero
in the course of shutting down.  This is a way for our QA tests to detect 
object reference leaks.  The results Christophe reported indicate an object
reference leak has occurred.  I think it is likely that this leak is is (er, 
may be) related to the error path encountered with the particular failure occuring in this test.

Unfortunately, it is nearly impossible to find the cause of the object
reference leak from the core file produced by that assertion.  So these 
core files primarily exist to prompt NSS developers to go digging for the
leak, but don't point to the leak.  Sometimes a memory leak detector helps,
but not always.  There's no point in saving the core files.

I agree that a separate bug should be filed about this assertion failure.
The QA test is apparently succeeding at doing what it is supposed to do,
detecting bugs in the NSS product's code.  

Comment 51

[Vipul Gupta]

Comment on attachment 209882 [details] [diff] [review]
Patch for tools.sh

Looks fine.

Comment 52

[Julien Pierre]

Comment on attachment 211092 [details] [diff] [review]
Patch v4 for ssl

I talked to Vipul yesterday and there seems to be several problems with this patch. I don't recall all of them at the moment. One of them is that if NSS_ENABLE_ECC is set, we should run both the regular non-ECC tests and the ECC tests. It's not clear if this patch accomplishes that, or how. It appears it does one or the other. I didn't review the patch verify carefully so perhaps I missed the logic . I think the change could be made at the top-level of the script to just invoke the functions twice with EC/no-EC arguments; first without EC unconditionally, and second with EC, conditionally.

There is a call to load_group_crl with a hardcoded value of noECC for the second argument which is probably wrong.

Comment 53

[Christophe Ravel]

(In reply to comment #52)
> (From update of attachment 211092 [details] [diff] [review] [edit])
> I talked to Vipul yesterday and there seems to be several problems with this
> patch. I don't recall all of them at the moment. One of them is that if
> NSS_ENABLE_ECC is set, we should run both the regular non-ECC tests and the ECC
> tests. It's not clear if this patch accomplishes that, or how. It appears it
> does one or the other. I didn't review the patch verify carefully so perhaps I
> missed the logic .

The SSL test does not work that way. We have a list of cipher suites to test (see .txt files). The test is run for each cipher in the list.
We always run the test on the non-EC ciphers.
We run the test on the EC ciphers only if NSS_ENABLE_ECC is set. 

  while read ectype value sparam cparam testname
  do
    if [ "$ectype" = "ECC" -a  -z "$NSS_ENABLE_ECC" ] ; then
        echo "$SCRIPTNAME: skipping $testname (ECC only)"
    elif [ "$ectype" != "#" ]; then
        run the test here
    fi
  done < ${SSLAUTH}

> I think the change could be made at the top-level of the
> script to just invoke the functions twice with EC/no-EC arguments; first
> without EC unconditionally, and second with EC, conditionally.

I don't think this would make sense here (see above).

> 
> There is a call to load_group_crl with a hardcoded value of noECC for the
> second argument which is probably wrong.

There was already a call to load_group_crl with a hardcoded value of 1 for the group in your version of this test. It reset the CRL settings to default group 1.
Now we have 2 sets of CRL for each group (non-EC and EC).
The first case in sslauth.txt is non-EC so we have to reload the non-EC group 1 CRL.

Comment 54

[Christophe Ravel]

Attachment: Patch v3 for smime.sh

Remove this misleading typo about "-i alicecc.txt" in the echo command when the real command uses "-i alice.txt".

Comment 55

[Christophe Ravel]

Changes

cert.sh: Sign Eve's EC Request --------------------------
certutil -C -c TestCA-ec -m 60 -v 60 -d ../CA -i req -o Eve-ec.cert -f
../tests.pw.19704 -7 [e-mail],[e-mail],[e-mail]

INTO:

cert.sh: Sign Eve's EC Request --------------------------
certutil -C -c TestCA-ec -m 60 -v 60 -d ../CA -i req -o Eve-ec.cert -f
../tests.pw.19704 -7 [e-mail],[e-mail],[e-mail]

The test "smime.sh: Testing multiple email addrs" passes with this change.

See also discussion in bug https://bugzilla.mozilla.org/show_bug.cgi?id=327684

Comment 56

[Nelson Bolyard (seldom reads bugmail)]

While reviewing the most recent test script runs, I observed several 
disturbing things, including:

a) All stress tests appear to use the "bypass" feature.  
   We have no stress tests that use pure PKCS11 (disabling the bypass).
   We MUST restore stress testing of the default non-bypass configurations. 
   We will not detect regressions otherwise.

b) It appears that we do NO stress testing with client authentication.
   None with RSA certs.  None with EC certs.  That's a big problem.
   We must get such testing into the script ASAP.  

If I'm not mistaken, the 3.10 test scripts did stress test client auth.
Perhaps that was removed for 3.11, or perhaps for the ECC merge.
(Or perhaps I'm hallucinating that we ever stress tested client auth.)

Christophe, please fix these two issues ASAP.

Comment 57

[Vipul Gupta]

Comment on attachment 211231 [details] [diff] [review]
Patch v6 for cert.sh

Let me know if someone else should be reviewing this patch.
I'd like to make progress on resolving this bug. I've reviewed and approved the patches to the tools and smime merged scripts.

Comment 58

[Wan-Teh Chang]

Comment on attachment 211231 [details] [diff] [review]
Patch v6 for cert.sh

Julien, could you review this patch?  Thanks.

Comment 59

[Nelson Bolyard (seldom reads bugmail)]

In reply to comment 56, 

Christophe reminded me that these tests test the bypass client against the non-bypass server, and the non-bypass client against the bypass server, so both bypass and non-bypass code get tested each time.  That's right.  My mistake.
Sorry about that.

However, I confirmed that there's no stress testing being done on client 
authentication at all.  That does need to be fixed.  Maybe that should go 
into a separate bug?

Comment 60

[Julien Pierre]

Comment on attachment 211231 [details] [diff] [review]
Patch v6 for cert.sh

nit:
In cert_add_cert(), the CURVE variable definition should be inside of the if NSS_ENABLE_ECC block for better readability.

Same problem with CA_CURVE in cert_all_CA .

Comment 61

[Vipul Gupta]

Comment on attachment 212700 [details] [diff] [review]
Patch v3 for smime.sh

Julien, please review. This should
be fairly quick. Thanks.

Comment 62

[Vipul Gupta]

Attachment: Patch v7 for cert.sh

Julien, this version addresses your review comments.

Comment 63

[Vipul Gupta]

Comment on attachment 213204 [details] [diff] [review]
Patch v7 for cert.sh

Please review Patch v8 for cert.sh
instead.

Comment 64

[Vipul Gupta]

Attachment: Patch v8 for cert.sh

There was one more place besides the two you mentioned in comment 60
where your feedback applied. Please review.

Comment 65

[Julien Pierre]

Comment on attachment 213205 [details] [diff] [review]
Patch v8 for cert.sh

Looks fine. I'm afraid I found one more place where my comment applied that you didn't fix - at the end of cert_all_CA, in the removal of ecroot.cert . It can also be grouped with the earlier if block.

Comment 66

[Vipul Gupta]

Attachment: Patch v9 for cert.sh

Good catch Julien. This patch addresses comment 65.

Comment 67

[Christophe Ravel]

Comment on attachment 211092 [details] [diff] [review]
Patch v4 for ssl

Do you think the explanation on the test sequence answer your questions ?

Comment 68

[Wan-Teh Chang]

Comment on attachment 209882 [details] [diff] [review]
Patch for tools.sh

r=wtc.  Please make the following changes when you
check this in.

1. Indent the new code properly.

2. Change the two instances of

   ! -z "$NSS_ENABLE_ECC"

to

   -n "$NSS_ENABLE_ECC"

Comment 69

[Vipul Gupta]

(In reply to comment #67)
> (From update of attachment 211092 [details] [diff] [review] [edit])
> Do you think the explanation on the test sequence answer your questions ?

Hi Christophe,

  I now have a much better sense of what the merged ssl.sh script is
doing but still have a few questions (see below). I've uploaded a 
new version (v5) of the ssl patch with the following changes:

- In ssl.sh,
   * Enclosed `uname -n` in double quotes (on my Mac, it produces a null string
      and results in error messages ... see comment #28, comment #31)
   * I noticed CU_ACTION was being set but not used so I added a line
     html_passed "<TR><TD> ${CU_ACTION}"
     But, for some reason, I don't see the corresponding output in results.html
     (perhaps you can take a look).
   * Included ${eccomment} in 
         html_msg $ret 0 "Load group $LOADED_GRP ${eccomment}crl"
   * Added a similar message where group 1 is loaded
   * fixed calling ssl_cleanup to eliminate multiple instances of "END_OF_TEST" in results.html

- In sslcov.txt, moved ECC cipher suite tests after all the non-ECC tests plus cosmetic change
   to align various columns

- In sslstress.txt, cosmetic change to align the last column

Please make sure that these changes make sense. Also, 
I'm still confused why we never see the Group 1 ECC CRL
being used by this script.

vipul

Comment 70

[Vipul Gupta]

Attachment: Patch v5 for ssl.sh

Here is Patch v5 for ssl as described in comment #69.

Comment 71

[Vipul Gupta]

Comment on attachment 213349 [details] [diff] [review]
Patch v9 for cert.sh

Redirecting review request to Nelson.

Comment 72

[Vipul Gupta]

Attachment: Patch v4 for smime.sh

Now that ECDSA signatures work for hashes other than SHA1
(bug 320583), I've expanded the ECDSA tests to cover those
hash algorithms.

A prior version of this patch had already seen two reviews
so I hope this one can be reviewed quickly. It simply removes
the check for a specific HASH algorithm (SHA1) in the ECDSA
tests.

vipul

Comment 73

[Alexei Volkov]

Attachment: ssl.sh v6 (entire script, as revised by Alexei)

Remove unnecessary step of reloading CRL after selfserv has been restarted.
New selfserv process will read TestCA and TestCA-ec crls from db.

Comment 74

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Patch v6 ssl.sh (patch made from Alexei's file)

Made Alexei's script into a patch so it can be compared to previous patch.

Comment 75

[Nelson Bolyard (seldom reads bugmail)]

Attachment: ssl.sh patch v6, merged Alexei's with Vipul's

Vipul's patch changed 4 files.  Alexei evidently replaced one of them.
This patch (should) represent Alexei's one file, plus Vipul's other 3.
Hopefully this patch will compare properly with Vipul's patch 5 for ssl.sh

Comment 76

[Nelson Bolyard (seldom reads bugmail)]

Attachment: vipul's patch v5 for ssl.sh converted to a unified diff

Comment 77

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 213528 [details] [diff] [review]
ssl.sh patch v6, merged Alexei's with Vipul's

Julien, please review this merged patch.

Comment 78

[Julien Pierre]

Comment on attachment 213528 [details] [diff] [review]
ssl.sh patch v6, merged Alexei's with Vipul's

This is fine.
Alexei and I determined today that the "load_group_crl 1" at the end of the loop was not necessary.

This function was restarting the server with a DB that already contained the CRL to be loaded in RAM subsequently. That was a duplicate since the CRL was already in the DB, so there was no need to do the loading part. Restarting the server is still necessary to flush the RAM CRL cache within selfserv. It could be flushed programmatically in selfserv, but no flush function was implemented in selfserv for that, so restarting the server is easier to do to accomplish this goal.

Comment 79

[Julien Pierre]

Comment on attachment 213516 [details] [diff] [review]
Patch v4 for smime.sh

This is fine. I think the comment about bug 320583 can be removed before checkin, though.

Comment 80

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 213349 [details] [diff] [review]
Patch v9 for cert.sh

sr+ = nelson, provided that you fix a number of nits, listed below.

>+if [ ! -z "$NSS_ENABLE_ECC" ] ; then

Please change all the lines (new and old) that test for ! -z 
to test for -n instead, e.g. 
  if [ -n "$NSS_ENABLE_ECC" ] ; then

>-################################ certu #################################
>+################################ cerlu #################################
                                    ^  should be crlu

>+  CU_ACTION="Modification CRL (ECC) by adding one more cert"
>+  CU_ACTION="Modification CRL (ECC) by removing one cert"

There are numerous lines in the script, both new and old, that use the 
word "Modification" as if it is a verb.  Please change them all to use
"Modify" instead.  e.g. 
    CU_ACTION="Modify CRL (ECC) by removing one cert"

Please let me know if you need someone to commit your patch(es) for you.

Comment 81

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 213531 [details] [diff] [review]
vipul's patch v5 for ssl.sh converted to a unified diff

Just a reminder: All patches should be unified diffs, e.g. cvs diff -u ...

Comment 82

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 213516 [details] [diff] [review]
Patch v4 for smime.sh

sr+=nelson
Please change all ! -z tests to -n tests before checkin.  Thanks.
I'm ignoring indentation issues.
Please let me know if you need someone to commit these changes.

Comment 83

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 213528 [details] [diff] [review]
ssl.sh patch v6, merged Alexei's with Vipul's

sr-  See below.

>+if [ ! -z "$NSS_ENABLE_ECC" ] ; then
First, my usual nit:  change ! -z   to  -n

Now the real issue

> ########################### is_selfserv_alive ##########################
> # local shell function to exit with a fatal error if selfserver is not
>@@ -145,11 +152,11 @@
>       html_failed "<TR><TD> Wait for Server "
>       echo "RETRY: tstclnt -p ${PORT} -h ${HOSTADDR} ${CLIENT_OPTIONS} -q \\"
>       echo "               -d ${P_R_CLIENTDIR} < ${REQUEST_FILE}"
>       tstclnt -p ${PORT} -h ${HOSTADDR} ${CLIENT_OPTIONS} -q \
>               -d ${P_R_CLIENTDIR} < ${REQUEST_FILE}
>-  elif [ sparam = "-c ABCDEFcdefgijklmnvyz" ] ; then # "$1" = "cov" ] ; then
>+  elif [ sparam = "-c ABCDEFcdefgijklmnvyz" -o sparam = "-c ABCDEF:C001:C002:C003:C004:C005:C006:C007:C008:C009:C00A:C00B:C00C:C00D:C00E:C00F:C010:C011:C012:C013:C014cdefgijklmnvyz" ] ; then # "$1" = "cov" ] ; then

That elif expression (both the old one and the new one) is always false.
As written, it is comparing "sparam" as a literal string, not as a variable.
Instead of 
    sparam = "something"
you want
    "$sparam" = "something"

While I'm at it, Please remove the trailing comment from that line.
That is, remove the "# "$1" = "cov" ] ; then"
Also, instead of duplicating those big strings in several places, I suggest
you define some shell variables to hold them in main, and then refer to them 
by their shell variable names.   E.g.  at the beginning of main have

CSHORT="-c ABCDEFcdefgijklmnvyz"
CLONG="-c ABCDEF:C001:C002:C003:C004:C005:C006:C007:C008:C009:C00A:C00B:C00C:C00D:C00E:C00F:C010:C011:C012:C013:C014cdefgijklmnvyz"

Then the above elif can be coded as

  elif [ "$sparam" = "$CSHORT" -o "$sparam" = "$CLONG" ]; then 

which is much more readable.  Likewise, 

>+if [ ! -z "$NSS_ENABLE_ECC" ] ; then
>+  sparam="-c ABCDEF:C001:C002:C003:C004:C005:C006:C007:C008:C009:C00A:C00B:C00C:C00D:C00E:C00F:C010:C011:C012:C013:C014cdefgijklmnvyz"
>+else
>   sparam="-c ABCDEFcdefgijklmnvyz"
>+fi

Can become 
  if [ -n "$NSS_ENABLE_ECC" ] ; then
    sparam="$CLONG"
  else
    sparam="$CSHORT"
  fi

>-          if [ `uname -n` = "sjsu" ] ; then
>+          if [ "`uname -n`" = "sjsu" ] ; then

Let me suggest that in new/modified code, instead of using back-quotes,
use the newer POSIX form $(cmd).  E.g. instead of "`uname -n`" 
use "$(uname -n)".  This is just a suggestion.


>         echo "RELOAD time $i"
>         tstclnt -p ${PORT} -h ${HOSTADDR} -f  \
>-            -d ${R_CLIENTDIR} -w nss -n TestUser${UNREVOKED_CERT_GRP_1} \
>+            -d ${R_CLIENTDIR} -w nss -n TestUser${UNREVOKED_CERT_GRP_1}${ecsuffix} \
>             <<_EOF_REQUEST_ >${OUTFILE_TMP}  2>&1

Does that work as expected?  
I would have thought it must be written as
              >${OUTFILE_TMP}  2>&1  <<_EOF_REQUEST_
>-GET crl://${SERVERDIR}/root.crl_${grpBegin}-${grpEnd}
>+GET crl://${SERVERDIR}/root.crl_${grpBegin}-${grpEnd}${ecsuffix}
> 
> _EOF_REQUEST_



>+# ECC ciphers
>+# XXX Session reuse does not seem to work for ECDH-ECDSA, ECDHE-ECDSA ciphers
>+# but works ok for ECDHE-RSA ciphers. With session reuse turned off
>+# setting up 1000 connections would take too long so use only 10 connections

Surely this is due to bug 238051, no??

Comment 84

[Vipul Gupta]

Attachment: Patch v2 for tools.sh

Addresses comment# 68 from Wan-Teh. Should be ready for check-in.

Comment 85

[Vipul Gupta]

Attachment: Patch v5 for smime.sh

Addresses comment# 79 and comment# 82 (also fixes indentation).
Should be ready to check-in.

Comment 86

[Wan-Teh Chang]

Comment on attachment 213607 [details] [diff] [review]
Patch v2 for tools.sh

r=wtc.  Vipul, the only thing you didn't carry over
to tools.sh is your name in the Contributor(s) section.

Comment 87

[Vipul Gupta]

Attachment: Patch v3 for tools.sh

Addresses comment# 86 :-)
[I hope I don't have to look at this file another time :)]

Comment 88

[Wan-Teh Chang]

Comment on attachment 213622 [details] [diff] [review]
Patch v3 for tools.sh

r=wtc.  Neither do I.  Thanks a lot, Vipul.

Comment 89

[Vipul Gupta]

Attachment: Patch v10 for cert.sh

Addresses comment# 80 and fixes indentation and wraps some long lines.

Comment 90

[Wan-Teh Chang]

Comment on attachment 213624 [details] [diff] [review]
Patch v10 for cert.sh

r=wtc.  Good work, Vipul.  Just two nits: at the end of
the cert_all_CA function, we have

>+	rm $CLIENT_CADIR/ecroot.cert $SERVER_CADIR/ecroot.cert
>+fi

The "fi" needs to be indented.

I'm wondering if we should add an EC version of the following
comment after the 'rm' statement:

     # root.cert in $CLIENT_CADIR and in $SERVER_CADIR is the one of the last 
     # in the chain

Comment 91

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 213624 [details] [diff] [review]
Patch v10 for cert.sh

I found one more item that I didn't see in the previous review.
Our certutil & crlutil tools don't properly encode x400Addresses.
So in the long string below, the substring "|x400Address:x400Address"
will cause an invalid x400 Address to be encoded.
This is the subject of bug 292285. 

Until that bug is fixed, the workaround is to 
1. remove that substring from the issuerAltNames string, AND 
2. add -q to the crlutil command line options below

>      crlu -d $CADIR -G -n "TestCA-ec" -f ${R_PWFILE} \
>	   -o ${CRL_FILE_GRP_1}_or-ec <<EOF_CRLINI
>+update=$CRLUPDATE
>+addcert ${CRL_GRP_1_BEGIN}-${CRL_GRP_END_} $CRL_GRP_DATE
>+addext reasonCode 0 4
>+addext issuerAltNames 0 "rfc822Name:[e-mail]|dnsName:ca-ec.com|x400Address:x400Address|directoryName:CN=NSS Test CA (ECC),O=BOGUS NSS,L=Mountain View,ST=California,C=US|URI:http://ca-ec.com|ipAddress:192.168.0.1|registerID=reg CA (ECC)"
>+EOF_CRLINI

Please make those changes, then (IMO) this can be committed without
further review.   r=nelson with the above changes made.

Comment 92

[Vipul Gupta]

Attachment: Patch v11 for cert.sh

Addresses comment#90 and comment# 91. 

Nelson, please add a superreview request
if you think we need one.

vipul

Comment 93

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 213692 [details] [diff] [review]
Patch v11 for cert.sh

r=nelson.  

Do all these patches need to be checked in together when they've all passed review?

Comment 94

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 213610 [details] [diff] [review]
Patch v5 for smime.sh

r=nelson for this patch.

Comment 95

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 213622 [details] [diff] [review]
Patch v3 for tools.sh

r=nelson

Comment 96

[Wan-Teh Chang]

Comment on attachment 213692 [details] [diff] [review]
Patch v11 for cert.sh

r=wtc.  Just curious, what does "x400Address:x400Address" mean?
Why are the name and value the same in this name value pair?

Comment 97

[Nelson Bolyard (seldom reads bugmail)]

All the patches except the one for ssl.sh seem ready to go now.  Yes?

Comment 98

[Vipul Gupta]

Attachment: Patch v7 for ssl.sh (and related files)

Addresses comment# 83. Nelson, Please add a super review request
if needed.

Ideally, all of these patches should be checked in together and the
following files should be removed:
tests/fixtests.sh
tests/cert/eccert.sh
tests/smime/ecsmime.sh
tests/ssl/ecssl.sh
tests/ssl/ecsslauth.txt
tests/ssl/ecsslcov.txt
tests/ssl/ecsslstress.txt
tests/tools/ectools.sh

vipul

Comment 99

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 213695 [details] [diff] [review]
Patch v7 for ssl.sh (and related files)

r=nelson

Christophe, can you do a build & test the incorporates the latest versions of all these patches?

Comment 100

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 213695 [details] [diff] [review]
Patch v7 for ssl.sh (and related files)

Wan-Teh, I want your opinion on the use of $(command) instead of `command`, among any other review comments you may have.

Comment 101

[Wan-Teh Chang]

Comment on attachment 213695 [details] [diff] [review]
Patch v7 for ssl.sh (and related files)

Nelson, I didn't know about the newer POSIX form $(command) of
command substitution.  In this patch we should continue to use
the back-quote form `command`.

Comment 102

[Vipul Gupta]

(In reply to comment #101)
> (From update of attachment 213695 [details] [diff] [review] [edit])
> Nelson, I didn't know about the newer POSIX form $(command) of
> command substitution.  In this patch we should continue to use
> the back-quote form `command`.

Wan-Teh, Nelson,

Is this something that can be edited at check-in time
(search for uname in ssl.sh) or do you want me to 
submit a new patch?

vipul

Comment 103

[Vipul Gupta]

Comment on attachment 213695 [details] [diff] [review]
Patch v7 for ssl.sh (and related files)

This patch is not ready for check-in. I noticed that the v6 of the patch replaced the
"load_group_crl 1 noECC"
with
kill_selfserv
start_selfserv

But it left in the line
html_msg $ret 0 "Load group 1 ${eccomment}crl " \
             "produced a returncode of $ret, expected is 0"

In tests I've conducted today, this line prints a failure message (produced 254, expected 0).

I think the html_msg line should
also be removed.

Is that correct? Julien? Alexei?

vipul

Comment 104

[Julien Pierre]

Vipul, the message could be removed or replaced by something like "restarting selfserv".

Comment 105

[Vipul Gupta]

(In reply to comment #104)
> Vipul, the message could be removed or replaced by something like "restarting
> selfserv".
> 

In the process of making this change, I noticed that 
load_group_crl already does kill_selfserv, start_selfserv, is_selfserv_alive
when the group is set to 1. So, do we still want to do this in
ssl_crl_cache or let it call load_group_call with group 1
(as it used to before patch v6)? Julien/Alexei, any thoughts?

vipul

Comment 106

[Julien Pierre]

Vipul,

I know load_group_crl already kills/restarts the server, but we really don't want to call this function because it appears we are trying to load a specific CRL when we are not - the purpose of the server restart is actually to flush the CRL cache. This change is not functionally necessary, but it should be made so that the script is easier to understand.

Comment 107

[Vipul Gupta]

(In reply to comment #106)
> Vipul,
> 
> I know load_group_crl already kills/restarts the server, but we really don't
> want to call this function because it appears we are trying to load a specific
> CRL when we are not - the purpose of the server restart is actually to flush
> the CRL cache. This change is not functionally necessary, but it should be made
> so that the script is easier to understand.

load_group_crl was doing more than just killing and restarting selfserv
when called with group 1. Now, apparently that code is never called for 
group 1. I just want to make sure this is as intended.

vipul

Comment 108

[Julien Pierre]

Vipul,

Yes, it is. Please see comment 78 . The group 1 CRL is already in the permanent database, so it doesn't need to be explicitly loaded on-the-fly by selfserv.

Comment 109

[Vipul Gupta]

Attachment: Patch v8 for ssl.sh (and related files)

Addresses comment# 101 and comment# 104.
All tests pass.

Comment 110

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 213867 [details] [diff] [review]
Patch v8 for ssl.sh (and related files)

r=nelson

Comment 111

[Wan-Teh Chang]

Comment on attachment 213867 [details] [diff] [review]
Patch v8 for ssl.sh (and related files)

r=wtc.  I didn't review the patch itself.  Instead,
I reviewed the diffs between ec<file> and the patched
<file> to verify that this patch doesn't lose any of
the current ECC SSL tests.  (<file> is one of ssl.sh,
sslauth.txt, sslcov.txt, and sslstress.txt.)

I suggest some comment and indentation changes below,
which you may disregard.  If you make the changes,
don't submit a new patch for review.  Just create a
new patch right before you check it in and attach it
here.

1. In ssl.sh, we have:

>+if [ -n "$NSS_ENABLE_ECC" ] ; then
>+  ECC_STRING=" - with ECC"
>+else
>+  ECC_STRING=""
>+fi

Please fix the indentation.

>+  if [ -n "$NSS_ENABLE_ECC" ] ; then
>+    ECC_OPTIONS="-e ${HOSTADDR}-ec"
>+  else
>+    ECC_OPTIONS=""
>+  fi

Please fix the indentation of the two
ECC_OPTIONS= lines.

>+if [ -n "$NSS_ENABLE_ECC" ] ; then
>+  sparam="$CLONG"
>+else
>+  sparam="$CSHORT"
>+fi

Please fix the indentation.

Also, please search for "round off" in this file
and change the "off" to "of".

2. In sslstress.txt, we have:

> # This file defines the tests for client auth.

This should read "This file defines the stress tests."

>-#     0       -r  -w_bogus_-n_"Test_User"     TLS Request don't require client auth (bad password)
>+# noECC     0       -r  -w_bogus_-n_"Test_User"   TLS Request don't require client auth (bad password)

We should add a copy of this line to the non-ECC ciphers
section, and then change "noECC" to "ECC" in this line
(in the ECC ciphers section).

Vipul, thank you very much for working on this bug.

Comment 112

[Vipul Gupta]

Attachment: Patch v9 for ssl.sh (and related files)

Addresses comment 111.

Wan-Teh, please take a look and check it in if
you are happy with this version (you should
also remove some other files as described in
comment 98.

[Just wondering ... did this bug set some sort of
a record for the most comments/patches associated 
with it :-)]

vipul

Comment 113

[Wan-Teh Chang]

Comment on attachment 213909 [details] [diff] [review]
Patch v9 for ssl.sh (and related files)

r=wtc.  Thanks, Vipul.  I checked in the patches and
cvs removed the files listed in comment 98 on the NSS
trunk (NSS 3.12).

Christophe, since this bug's target milestone is 3.11.1,
I'll let you do the checkin on the NSS_3_11_BRANCH.
Please ask Nelson, Julien, and Alexei how this should
be done.  The NSS trunk has some new tests that
NSS_3_11_BRANCH doesn't have, so the patches in this
bug won't apply to the branch.

Here is the NSS trunk CVS checkin output.

Removing fixtests.sh;
/cvsroot/mozilla/security/nss/tests/fixtests.sh,v  <--  fixtests.sh
new revision: delete; previous revision: 1.3
done
Checking in cert/cert.sh;
/cvsroot/mozilla/security/nss/tests/cert/cert.sh,v  <--  cert.sh
new revision: 1.32; previous revision: 1.31
done
Removing cert/eccert.sh;
/cvsroot/mozilla/security/nss/tests/cert/eccert.sh,v  <--  eccert.sh
new revision: delete; previous revision: 1.4
done
Removing smime/ecsmime.sh;
/cvsroot/mozilla/security/nss/tests/smime/ecsmime.sh,v  <--  ecsmime.sh
new revision: delete; previous revision: 1.1
done
Checking in smime/smime.sh;
/cvsroot/mozilla/security/nss/tests/smime/smime.sh,v  <--  smime.sh
new revision: 1.22; previous revision: 1.21
done
Removing ssl/ecssl.sh;
/cvsroot/mozilla/security/nss/tests/ssl/ecssl.sh,v  <--  ecssl.sh
new revision: delete; previous revision: 1.4
done
Removing ssl/ecsslauth.txt;
/cvsroot/mozilla/security/nss/tests/ssl/ecsslauth.txt,v  <--  ecsslauth.txt
new revision: delete; previous revision: 1.2
done
Removing ssl/ecsslcov.txt;
/cvsroot/mozilla/security/nss/tests/ssl/ecsslcov.txt,v  <--  ecsslcov.txt
new revision: delete; previous revision: 1.2
done
Removing ssl/ecsslstress.txt;
/cvsroot/mozilla/security/nss/tests/ssl/ecsslstress.txt,v  <--  ecsslstress.txt
new revision: delete; previous revision: 1.3
done
Checking in ssl/ssl.sh;
/cvsroot/mozilla/security/nss/tests/ssl/ssl.sh,v  <--  ssl.sh
new revision: 1.63; previous revision: 1.62
done
Checking in ssl/sslauth.txt;
/cvsroot/mozilla/security/nss/tests/ssl/sslauth.txt,v  <--  sslauth.txt
new revision: 1.7; previous revision: 1.6
done
Checking in ssl/sslcov.txt;
/cvsroot/mozilla/security/nss/tests/ssl/sslcov.txt,v  <--  sslcov.txt
new revision: 1.6; previous revision: 1.5
done
Checking in ssl/sslstress.txt;
/cvsroot/mozilla/security/nss/tests/ssl/sslstress.txt,v  <--  sslstress.txt
new revision: 1.6; previous revision: 1.5
done
Removing tools/ectools.sh;
/cvsroot/mozilla/security/nss/tests/tools/ectools.sh,v  <--  ectools.sh
new revision: delete; previous revision: 1.2
done
Checking in tools/tools.sh;
/cvsroot/mozilla/security/nss/tests/tools/tools.sh,v  <--  tools.sh
new revision: 1.14; previous revision: 1.13
done

Comment 114

[Wan-Teh Chang]

Attachment: Patch for sslstress.txt to fix a comment error

This patch fixes a comment bug introduced in
"Patch v9 for ssl.sh (and related files)".

In the ssl*.txt files, a comment line must begin with
exactly one '#' character followed by white space.
This is because we test for a comment line like
this:

  while read ectype value sparam cparam testname
  do
      ...
      elif [ "$ectype" != "#" ]; then

So the first token $ectype of a comment line must
be exactly "#".

I've checked this patch in.

Checking in sslstress.txt;
/cvsroot/mozilla/security/nss/tests/ssl/sslstress.txt,v  <--  sslstress.txt
new revision: 1.7; previous revision: 1.6
done

Comment 115

[Christophe Ravel]

Latest patches tested and committed on NSS_3_11_BRANCH.

Removing fixtests.sh;
/cvsroot/mozilla/security/nss/tests/Attic/fixtests.sh,v  <--  fixtests.sh
new revision: delete; previous revision: 1.3
done
Checking in cert/cert.sh;
/cvsroot/mozilla/security/nss/tests/cert/cert.sh,v  <--  cert.sh
new revision: 1.28.2.2; previous revision: 1.28.2.1
done
Removing cert/eccert.sh;
/cvsroot/mozilla/security/nss/tests/cert/Attic/eccert.sh,v  <--  eccert.sh
new revision: delete; previous revision: 1.4
done
Removing smime/ecsmime.sh;
/cvsroot/mozilla/security/nss/tests/smime/Attic/ecsmime.sh,v  <--  ecsmime.sh
new revision: delete; previous revision: 1.1
done
Checking in smime/smime.sh;
/cvsroot/mozilla/security/nss/tests/smime/smime.sh,v  <--  smime.sh
new revision: 1.21.2.1; previous revision: 1.21
done
Removing ssl/ecssl.sh;
/cvsroot/mozilla/security/nss/tests/ssl/Attic/ecssl.sh,v  <--  ecssl.sh
new revision: delete; previous revision: 1.3.28.1
done
Removing ssl/ecsslauth.txt;
/cvsroot/mozilla/security/nss/tests/ssl/Attic/ecsslauth.txt,v  <--  ecsslauth.txt
new revision: delete; previous revision: 1.1.82.1
done
Removing ssl/ecsslcov.txt;
/cvsroot/mozilla/security/nss/tests/ssl/Attic/ecsslcov.txt,v  <--  ecsslcov.txt
new revision: delete; previous revision: 1.1.82.1
done
Removing ssl/ecsslstress.txt;
/cvsroot/mozilla/security/nss/tests/ssl/Attic/ecsslstress.txt,v  <--  ecsslstress.txt
new revision: delete; previous revision: 1.1.82.1
done
Checking in ssl/ssl.sh;
/cvsroot/mozilla/security/nss/tests/ssl/ssl.sh,v  <--  ssl.sh
new revision: 1.61.2.2; previous revision: 1.61.2.1
done
Checking in ssl/sslauth.txt;
/cvsroot/mozilla/security/nss/tests/ssl/sslauth.txt,v  <--  sslauth.txt
new revision: 1.6.80.1; previous revision: 1.6
done
Checking in ssl/sslcov.txt;
/cvsroot/mozilla/security/nss/tests/ssl/sslcov.txt,v  <--  sslcov.txt
new revision: 1.5.144.1; previous revision: 1.5
done
Checking in ssl/sslstress.txt;
/cvsroot/mozilla/security/nss/tests/ssl/sslstress.txt,v  <--  sslstress.txt
new revision: 1.5.80.1; previous revision: 1.5
done
Removing tools/ectools.sh;
/cvsroot/mozilla/security/nss/tests/tools/Attic/ectools.sh,v  <--  ectools.sh
new revision: delete; previous revision: 1.2
done
Checking in tools/tools.sh;
/cvsroot/mozilla/security/nss/tests/tools/tools.sh,v  <--  tools.sh
new revision: 1.13.28.1; previous revision: 1.13
done

Comment 116

[Christophe Ravel]

Attachment: Fix for ssl_stress: no error message written in output.log

As reported by Nelson, there is no error message when ssl_stress fails.
This line was dropped during the previous review process. We still need to display the error code from strsclnt.

Comment 117

[Christophe Ravel]

Attachment: Fix for ssl_stress: no error message written in output.log (with more context)

Same as previous patch but with more context to make it understandable.

Comment 118

[Vipul Gupta]

Comment on attachment 214332 [details] [diff] [review]
Fix for ssl_stress: no error message written in output.log (with more context)


The patch looks fine. Do we need a similar fix for ssl_cov (search for html_msg)?

vipul

Comment 119

[Christophe Ravel]

Attachment: Fix for ssl_stress and ssl_cov: no error message written in output.log

Added return information for ssl_cov as mentioned by Vipul.

Comment 120

[Christophe Ravel]

Comment on attachment 214370 [details] [diff] [review]
Fix for ssl_stress and ssl_cov: no error message written in output.log

Patch tested on Solaris 10 SPARC.

Comment 121

[Christophe Ravel]

Comment on attachment 214370 [details] [diff] [review]
Fix for ssl_stress and ssl_cov: no error message written in output.log

r=nelson (while logged in as Christophe :)

Comment 122

[Christophe Ravel]

patch 214370 committed
- on the tip:
Checking in ssl.sh:
/cvsroot/mozilla/security/nss/tests/ssl/ssl.sh,v  <--  ssl.sh
new revision: 1.64; previous revision: 1.63
done
- on NSS_3_11_BRANCH:
Checking in ssl.sh;
/cvsroot/mozilla/security/nss/tests/ssl/ssl.sh,v  <--  ssl.sh
new revision: 1.61.2.3; previous revision: 1.61.2.2
done

Comment 123

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 214370 [details] [diff] [review]
Fix for ssl_stress and ssl_cov: no error message written in output.log

sr=nelson (while logged in as nelson this time :)

Comment 124

[Nelson Bolyard (seldom reads bugmail)]

Is any more work remaining to be done for this bug?
Are all the patches now checked in?
Are any more changes planned for this?

If not, Christophe, please mark this resolved/fixed.
Many thanks to all who helped get it this far!

Comment 125

[Vipul Gupta]

(In reply to comment #124)
> Is any more work remaining to be done for this bug?
> Are all the patches now checked in?
> Are any more changes planned for this?

AFAIK, this bug can be marked resolved.
However, we should perhaps open up a 
new bug to expand all.sh to cover 
parts of NSS that are not being tested. 
All new tests, including stress tests involving
client auth, could be worked on as part of that bug.

vipul