220380 - Add SSL client auth stress tests.

Status: RESOLVED FIXED

(NSS :: Test, enhancement, P1)

Description

[Wan-Teh Chang]

This enhancement request was suggested by Nelson in
bug 163123 comment 3.

The current SSL/TLS stress tests do not test client
authentication at all.  There is one line that would
do one such test, but it is commented out.  Client
auth should be tested with SSL3 and TLS, and should
probably be tested both with and without restarts.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Christophe, this RFE should only require some additional scripting work.
Please have a look at it.  Thanks.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

There are several different sets of client auth stress tests needed.
They may or may not necessitate changes to strsclnt.

1. A stress test in which the client sets the SSL_NO_CACHE option on
each and every SSL socket, but the server operates normally (that is,
selfserv does NOT disable the cache with the -N option).  The client
uses a single SSL client auth cert for all the tests on all the threads.
I don't know if the strsclnt program has a -N command line option that 
sets the SSL_NO_CACHE socket option (like selfserv has), but if not,
that will need to be added.

One of the objectives of this test is to ensure that the client ALWAYS
succesfully authenticates on EVERY handshake where it is requested.  
So, we need some way to detect a failure to authenticate, that is, to 
detect that the client has not sent a certificate when it was requested.
One way to do this is for the server to *require* client auth, and then
have the client detect the error alert that the server sends when no
client auth is performed, and treat that error as a fatal error (for 
testing purposes).

2. A new client stress test, which may be implemented as an option to 
strsclnt or might be a new test client program.  This test will use
multiple client certificates.  Each thread will use a different client
certificate.  Each thread will represent a different virtual user.  

The client cache will be partitioned into multiple virtual client caches, 
one for each virtual user.  This partitioning is accomplished by 
calling SSL_SetSockPeerID on each client socket when that socket is being
initialized/configured.  Each socket is given a different "PeerID" string.
I suggest using the nickname string as the peer ID string for each socket.

In this second test, each client thread's session needs to be flushed from 
its respective virtual cache every few handshakes.  This is done by calling
SSL_InvalidateSession on the socket before closing it, to invalidate the
session for that socket.  

The second test is of lower priority that the first one above.  The first
test should be implemented first, and checked in and added to nightly testing,
before the work on implementing the second test is begun.

Both tests need to be added to ssl.sh, so that they are performed during 
nightly QA and also during tinderbox testing.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Raising to P1.  We MUST eliminate the CERT library races in 3.11.1.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Alexei, please make this your top priority.  It's mostly adding to ssl.sh script.
Based on the findings in bug 329072 comment 2 , I'd say we want to do lots of 
runs of strsclnt, with a few threads and a few connections per thread, rather 
than with hundreds or thousands of tests in a single process run.  

Comment 5

[Alexei Volkov]

Attachment: strsclnt and selfserv param modifications

Modification to selfserv arguments to make selfserv authenticate client by requesting a client cert.
Use -N(no session reuse) on client side.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 216778 [details] [diff] [review]
strsclnt and selfserv param modifications

looks good to me

Comment 7

[Alexei Volkov]

this bug depends on modifications reqired to fix 332279

Comment 8

[Alexei Volkov]

Attachment: strsclnt auth patch

sets the same options for selfserv and strsclnt as the first patch, except
one new option on the client side "-n TestUser", which sets what user cert to
use.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 217889 [details] [diff] [review]
strsclnt auth patch

>Index: ssl.sh

>-          cparam=`echo $cparam | sed -e 's;_; ;g'`
>+          cparam=`echo $cparam | sed -e 's;_; ;g' -e "s/TestUser/$USER_NICKNAME/g" `

Please use $(echo...g") instead of `echo...g"`
Please let me know if that causes any problems on any platforms.

>Index: sslstress.txt

> #        expected
> # Enable  return  server     client                         Test Case name
> #  ECC    value   params     params
> # ------- ------  ------     ------                         ---------------
>-  noECC     0      _     -c_1000_-C_A             Stress SSL2 RC4 128 with MD5
>-  noECC     0      _     -c_1000_-C_c_-T          Stress SSL3 RC4 128 with MD5
>-  noECC     0      _     -c_1000_-C_c             Stress TLS  RC4 128 with MD5
>+  noECC     0      _         -c_1000_-C_A                   Stress SSL2 RC4 128 with MD5
>+  noECC     0      _         -c_1000_-C_c_-T                Stress SSL3 RC4 128 with MD5
>+  noECC     0      _         -c_1000_-C_c                   Stress TLS  RC4 128 with MD5
>+  noECC     0      -r_-r     -c_1000_-C_A_-N_-n_TestUser    Stress SSL2 RC4 128 with MD5
>+  noECC     0      -r_-r     -c_1000_-C_c_-T_-N_-n_TestUser Stress SSL3 RC4 128 with MD5
>+  noECC     0      -r_-r     -c_1000_-C_c_-N_-n_TestUser    Stress TLS  RC4 128 with MD5

The -n option should only be needed for SSL2, not for SSL3 or TLS.
We also need to do client auth stress test for ECC.

Comment 10

[Alexei Volkov]

$(cmd ...) does not work on sh from /usr/bin on solaris. sh on HP, cygwin and mkstk are fine.

-n TestUser cert is used for SSL3 and TLS because it has problem as well that reported in bug 332426. Using "-n cert" would be a temporary solution until second part of bug 220308 comment 2 is implemented.

Comment 11

[Alexei Volkov]

Wrong bug number in comment #10. The correct one is bug 220380 comment 2 or just simply comment 2.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 217889 [details] [diff] [review]
strsclnt auth patch

ok, we'll live with Solaris limitations.  :-(

Comment 13

[Alexei Volkov]

Attachment: do not compare ssl stat with # of connects for SSL2

Fix for the following problem:
We have code in http://lxr.mozilla.org/security/source/security/nss/cmd/strsclnt/strsclnt.c#1490 that compares a number of connections with ssl3 statistics in case when -N (NoReuse) flag is specified in command line of strsclnt.
Some of the changes to sslstress.txt involves adding -N flag to strsclnt including the case when SSL2 cipher suite is tested. 
This combination make test to fail, because of absents of statistics for SSL2.

Comment 14

[Alexei Volkov]

Comment on attachment 218206 [details] [diff] [review]
do not compare ssl stat with # of connects for SSL2

Just realized that I should have use PR_AtomicInrement();
instead of Lock and Unlock.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 218206 [details] [diff] [review]
do not compare ssl stat with # of connects for SSL2

r-
I don't think we need yet another ssl3 connections counter.
And if we do, it should be incremented with PR_AtomicIncrement,
not with a lock/unlock.  

What we need is a test (at the end, where we're defining exitVal)
that is correct for SSL3 and also for SSL2, that doesn't ignore
the connection counts in either case.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Please try to get this done for 3.11 this week.

Comment 17

[Alexei Volkov]

Attachment: skip comparison with ssl3 stat for ssl2 tests

Nelson,

I didn't find a variable responsible to track ssl3 connections.

The only possible way to verify number of connections made with ssl2 proto is to compare number of "connections" with "certTested" variable value.

Comment 18

[Alexei Volkov]

Attachment: auth patch

Added stress tests with authentication for ecc suite.

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 222740 [details] [diff] [review]
auth patch

Alexei, some questions about this patch.

>Index: ssl.sh

>   while read ectype value sparam cparam testname
>   do
>+      if [ -z "$ectype" ]; then

Please add a comment here, saying "# silently ignore blank lines"

> #
> # add client auth versions here...
> #
>-#  ECC      0      -r        -w_bogus_-n_"Test_User-ec"  TLS Request don't require client auth (bad password)
>+   ECC      0      -r_-r_-c_:C009  -c_10_-C_:C009_-N_-T_-n_TestUser-ec Stress SSL3 ECDHE-ECDSA AES 128 CBC with SHA (no reuse, client auth)
>+   ECC      0      -r_-r_-c_:C013  -c_100_-C_:C013_-T_-n_TestUser-ec Stress SSL3 ECDHE-RSA AES 128 CBC with SHA (client auth)
>+   ECC      0      -r_-r_-c_:C004  -c_10_-C_:C004_-N_-n_TestUser-ec Stress TLS ECDH-ECDSA AES 128 CBC with SHA (no reuse, client auth)
>+#   ECC      0      -r_-r_-c_:C00E  -c_10_-C_:C00E_-N_-n_TestUser-ec Stress TLS ECDH-RSA AES 128 CBC with SHA (no reuse, client auth)

The above one of these newly defined tests is commented out.
Is that intentional?  
Why is it added, but commented out?  

>+   ECC      0      -r_-r_-c_:C013  -c_100_-C_:C013_-n_TestUser-ec Stress TLS ECDHE-RSA AES 128 CBC with SHA(client auth)

Will all these new ECC client auth tests use the same cert, with the same curve?
Or will they use different certs with different curves?

Comment 20

[Alexei Volkov]

> >+      if [ -z "$ectype" ]; then
> 
> Please add a comment here, saying "# silently ignore blank lines"

The comment is added.

> 
> >+   ECC      0      -r_-r_-c_:C004  -c_10_-C_:C004_-N_-n_TestUser-ec Stress TLS ECDH-ECDSA AES 128 CBC with SHA (no reuse, client auth)
> >+#   ECC      0      -r_-r_-c_:C00E  -c_10_-C_:C00E_-N_-n_TestUser-ec Stress TLS ECDH-RSA AES 128 CBC with SHA (no reuse, client auth)
> 
> The above one of these newly defined tests is commented out.
> Is that intentional?  
> Why is it added, but commented out?  

This was the workaround for bug 332222. Please check the link:
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/security/nss/tests/ssl/sslstress.txt&rev=1.9

> 
> >+   ECC      0      -r_-r_-c_:C013  -c_100_-C_:C013_-n_TestUser-ec Stress TLS ECDHE-RSA AES 128 CBC with SHA(client auth)
> 
> Will all these new ECC client auth tests use the same cert, with the same
> curve?
> Or will they use different certs with different curves?

Right now one cert has been used for all curves.

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 222740 [details] [diff] [review]
auth patch

(In reply to comment #20)

> > >+#   ECC      0      -r_-r_-c_:C00E  -c_10_-C_:C00E_-N_-n_TestUser-ec Stress TLS ECDH-RSA AES 128 CBC with SHA (no reuse, client auth)
> > 
> > The above one of these newly defined tests is commented out.
> > Is that intentional?  
> > Why is it added, but commented out?  
> 
> This was the workaround for bug 332222. 

OK, please add a comment right above that commented out line, saying
# following line commented to woraround bug 332222

> > Will all these new ECC client auth tests use the same cert, with 
> > the same curve?
> > Or will they use different certs with different curves?
> 
> Right now one cert has been used for all curves.

Well, each cert can have at most one curve.  So, if one cert is being
used for all the testing, then only one curve is being tested for all 
the cipher suites.  

But I guess we should walk before we run.  So, let's get the client auth
tests going with one curve, and we can figure out about more curves later.

Comment 22

[Alexei Volkov]

Checking in ssl.sh;
/cvsroot/mozilla/security/nss/tests/ssl/ssl.sh,v  <--  ssl.sh
new revision: 1.67; previous revision: 1.66

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

Checking in strsclnt.c;
/cvsroot/mozilla/security/nss/cmd/strsclnt/strsclnt.c  <--  strsclnt.c
new revision: 1.51; previous revision: 1.50

Comment 23

[Alexei Volkov]

Checking in ssl.sh;
/cvsroot/mozilla/security/nss/tests/ssl/ssl.sh,v  <--  ssl.sh
new revision: 1.67; previous revision: 1.66

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

Checking in strsclnt.c;
/cvsroot/mozilla/security/nss/cmd/strsclnt/strsclnt.c  <--  strsclnt.c
new revision: 1.51; previous revision: 1.50

Comment 24

[Julien Pierre]

This bug is marked fixed with a target milestone of 3.11.1 . However, the fix was never checked in to the NSS_3_11_BRANCH . Alexei, you will need to request a second review on each patch for the branch.

Comment 25

[Alexei Volkov]

To make test work both patches (attachment 219947 [details] [diff] [review] and attachment 222740 [details] [diff] [review]) need to get second review for 3.11.x.

Comment 26

[Alexei Volkov]

Comment on attachment 222740 [details] [diff] [review]
auth patch

this patch fails to apply to 3.11. I will resubmit the patch shortly

Comment 27

[Alexei Volkov]

Attachment: auth patch for 3.11

Comment 28

[Julien Pierre]

Comment on attachment 236259 [details] [diff] [review]
auth patch for 3.11

This patch backs out some changes that were made in bug 332222.

Comment 29

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 219947 [details] [diff] [review]
skip comparison with ssl3 stat for ssl2 tests

r=nelson

Comment 30

[Alexei Volkov]

Attachment: auth tests with C00E commented out

Comment 31

[Alexei Volkov]

Checked in 3.11 branch:

/cvsroot/mozilla/security/nss/tests/ssl/ssl.sh,v  <--  ssl.sh
new revision: 1.61.2.6; previous revision: 1.61.2.5
/cvsroot/mozilla/security/nss/tests/ssl/sslstress.txt,v  <--  sslstress.txt
new revision: 1.5.80.6; previous revision: 1.5.80.5
/cvsroot/mozilla/security/nss/cmd/strsclnt/strsclnt.c,v  <--  strsclnt.c
new revision: 1.43.2.7; previous revision: 1.43.2.6