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

RESOLVED FIXED in 3.4

Status

NSS
Test
P2
enhancement
RESOLVED FIXED
17 years ago
17 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Sonja Mirtitsch)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

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.
(Reporter)

Updated

17 years ago
Priority: -- → P2
Target Milestone: --- → 3.4
(Assignee)

Comment 1

17 years ago
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
Depends on: 73098
(Assignee)

Comment 2

17 years ago
taking off dependency since other fix was checked in in the meantime (Ian reviewed)
Status: NEW → ASSIGNED
No longer depends on: 73098
(Assignee)

Comment 3

17 years ago
Created attachment 62458 [details] [diff] [review]
patch
(Assignee)

Comment 4

17 years ago
Created attachment 62459 [details]
sslauth.txt
(Assignee)

Comment 5

17 years ago
Created attachment 62460 [details]
output.log
(Assignee)

Comment 6

17 years ago
Created attachment 62461 [details]
results
(Assignee)

Comment 7

17 years ago
I'd like to check in the changes, please review
Whiteboard: review needed
(Reporter)

Comment 8

17 years ago
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

(Assignee)

Comment 9

17 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
(Assignee)

Comment 10

17 years ago
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 
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 11

17 years ago
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
Whiteboard: review needed → one more review needed
(Reporter)

Comment 12

17 years ago
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.
(Assignee)

Comment 13

17 years ago
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
(Reporter)

Comment 14

17 years ago
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.
(Assignee)

Comment 15

17 years ago
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
Whiteboard: one more review needed
(Assignee)

Comment 16

17 years ago
workaround in QA stat, seperate bug files on adding # of expected cache misses
to .txt files
Status: REOPENED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.