114964 - QA tests don't test multiple SSL handshakes on one connection

Status: RESOLVED FIXED

(NSS :: Test, enhancement, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

This "bug" requests additional SSL testing in the SSL QA tests to make
them more complete.

Bug 114787 reported a crash that occured whenever a second (or subsequent)
SSL/TLS handshake was done on a TCP connection.  That bug was a regression
introduced by a change to SSL in NSS 3.4 on November 4. This bug was not 
caught by the QA tests at the time the regression was introduced because 
the QA tests apparently do not test the case where multiple SSL handshakes
are done on a single TCP connection.  

The "selfserv" test server already has the ability to test multiple SSL
connections on a TCP connection in conjunction with SSL client authentication
(which is the usual cause of multiple handshakes, now that SSL Step-Up is 
mostly a thing of the past).  This is controlled by the number of "-r" 
arguments on the command line.  The number of -r argument is interpreted
as follows:

number   number of
of -r's  handshakes            Meaning
-------  -------  -----------------------------------------------
none        1     do not request or require client authentication

  1         1     request but do not require client authentication 

  2         1     request and require client authentication

  3         2     do not request or require client auth on first handshake,
                  request but do not require client auth on second handshake

  4         2     do not request or require client auth on first handshake,
                  request and require client auth on second handshake

I request that the present tests of client authentication, which all use
1 or 2 -r's, be supplemented with new tests that use 3 or 4 -r's, so 
that we test all 4 combinations.

Comment 1

[Sonja Mirtitsch]

I have lots of changes in the ssl script that wait for review and I would like
to check the first change in, before making the next addition

Comment 2

[Sonja Mirtitsch]

taking off dependency since other fix was checked in in the meantime (Ian reviewed)

Comment 3

[Sonja Mirtitsch]

Attachment: patch

Comment 4

[Sonja Mirtitsch]

Attachment: sslauth.txt

Comment 5

[Sonja Mirtitsch]

Attachment: output.log

Comment 6

[Sonja Mirtitsch]

Attachment: results

Comment 7

[Sonja Mirtitsch]

I'd like to check in the changes, please review

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

This looks OK to me, but with two comments:

1. In the first column, the old test cases used values of 0 and 254.   
   The new test cases use 0 and 1.  I'm surprised that the new test
   cases don't use the same values as the old ones there.

2. instead of using -r_-r_-r you could use -rrr

Comment 9

[Sonja Mirtitsch]

1 is what the tstclnt returns in this case - maybe someone should review
tstclnt.c to find out why if that is unexpected? Thanks for letting me know
about the simplified option, I'll keep it in mind for the future.

Comment 10

[Sonja Mirtitsch]

One of the failures that we could only recognize from the output.log has been
more than 1 cache miss - at the moment I can only test for more than 2 since the
script that evaluates the output.log is still a shell script (should be
re-written in perl)
line:
dewey.1/output.log:0 cache hits; 2 cache misses, 0 cache not reusable 

Comment 11

[Sonja Mirtitsch]

please Nelson or Wan-Teh, let me know if the scripts still need to do strict
testing regarding the cache misses. It would be very hard to implement this in
the shell script.
Thanks

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

2 cache misses is the expected and correct number when doing two full
SSL handshakes on the same connection.  

Testing the number of cache misses is important on stress tests, but 
IMO is not vital for these simple functionality tests.

Comment 13

[Sonja Mirtitsch]

Yes I realize this, but the script, being a dumb shell vs a smart perl script is
not context sensitive, and can't determine if it is evaluating a stress test
message or a auth test message

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

You could add another column to the table of test cases, which is the 
expected number of cache misses, and then compare the result with 
that number in the shell script.  It might require adding another 
positional parameter to some shell script subroutine.

Comment 15

[Sonja Mirtitsch]

I just noticed that the output line from the strsclnt looks different than from
the tstclnt - so this way the fix is fairly trivial.
It might still be a good idea to put the expected number of cache misses in the
.txt file as you suggested, and do the pass fail evaluation right from the
ssl.sh script where it belongs instead of putting it into the qa_stat script. I
will file a seperate QA bug on it.
thanks

Comment 16

[Sonja Mirtitsch]

workaround in QA stat, seperate bug files on adding # of expected cache misses
to .txt files