401001 - test clients in FIPS mode in ssl.sh

Status: RESOLVED DUPLICATE of bug 401303

(NSS :: Test, enhancement, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

In bug 400841 comment 5, Slavo wrote:
> We don't tests client in FIPS mode in ssl.sh, only server. Why ? 
> Is this not supported, or we just don't have tests for it ? 
> Should we write tests for testing client in FIPS mode ?

It is supported, and ssl.sh should test it.

Comment 1

[Slavomir Katuscak]

With increasing numbers of test combinations (and variables to enable/disable it in ssl.sh) I would like to use there one variable SSL_TESTS similar to TESTS variable in all.sh, where you can set which tests from ssl.sh you want to run.

Possible tests:
CRL (CRL tests)
BASIC (server normal, client normal)
BYPASS_SERVER (server bypass, client normal)
BYPASS_CLIENT (server normal, client bypass)
BYPASS_ONCE (server bypass, client bypass - used for PKIX tests)
FIPS_SERVER (server FIPS, client normal)
FIPS_CLIENT (server normaln, client FIPS)
FIPS_ONCE (server FIPS, client FIPS - do we need this ??)

Default settings:
SSL_TESTS="CRL BYPASS_SERVER BYPASS_CLIENT FIPS_SERVER FIPS_CLIENT"

If you want to run some other set of tests (disable some of them, run server + client both in bypass,..) you can set this variable manually.

Please send me some feedback how do you like this idea.

Comment 2

[Slavomir Katuscak]

Attachment: Patch.

Adding SSL tests for client in FIPS mode. 

This patch contains changes in ssl_cov(), ssl_auth() and ssl_stress() functions to skip unsupported tests and authenticate with password in tstclnt.

There were many variables used to enable/disable sets of ssl tests and I didn't wanted to add new, so I rewrited this part to use only one optional variable NSS_TESTS_SSL (changes also in all.sh and common/init.sh).

Also I added some small changes like using SSL_RUN_STRING instead of BYPASS_STRING and some improvements of ssl_set_fips() function.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 290705 [details] [diff] [review]
Patch.

Comments:
1) The following lines of code are repeated in all.sh and ssl.sh.
It seems that they are only necessary in ssl.sh, since no other
script uses the values of these variables.  So, why are they 
defined in both places?  

>+nss_tests_ssl="crl bypass_default default_bypass fips_default default_fips iopr"
>+NSS_TESTS_SSL=${NSS_TESTS_SSL:-$nss_tests_ssl}
>+ALL_TESTS_SSL=${NSS_TESTS_SSL}

It would be best if they were defined in one place, not two.
If they must be defined in both places, then each of the two 
places needs a comment reminding developers that any change 
to one place must also be made in the other.

2) Please move the crl test from the beginning of the test string to near the 
end, just before iopr.  

One of the principles of the tests in all.sh is that they should progress
from the simplest tests (the ones that have the fewest dependencies) first
to the more complex tests (with many dependencies) last, and any tests 
that are variants of the most basic fundamental tests should come AFTER 
those more basic fundamental tests.  This is why we test the freebl/blapi
tests first, because they have the fewest dependencies.  Other parts of NSS 
depend on them, but they do not depend on other parts of NSS.  

This principle causes failures in the lower-layer foundational elements to 
be detected before tests are done on the higher-layer functions that depend 
on the lower ones.  This helps us to pinpoint the causes of failures.  If 
we experience failures of both the simple and the complex tests, we fix the
simple ones first, and then retest to see if the failures in the complex
tests are also fixed.

The CRL tests have many more dependencies than the basic SSL tests, and 
are variants of the basic SSL tests, so they should not preceed the basic
SSL tests.

Comment 4

[Slavomir Katuscak]

(In reply to comment #3)
> (From update of attachment 290705 [details] [diff] [review])
> Comments:
> 1) The following lines of code are repeated in all.sh and ssl.sh.
> It seems that they are only necessary in ssl.sh, since no other
> script uses the values of these variables.  So, why are they 
> defined in both places?  

The reason why they are in all.sh is because NSS_TESTS_SSL can change (now for PKIX tests and in future maybe for shared DB tests) and we need to keep original value. And because ssl.sh can be run also directly without using all.sh, I defined those variables also there. I agree that it would be better to have it on one place, maybe common/init.sh would be better.
 
> 2) Please move the crl test from the beginning of the test string to near the 
> end, just before iopr.

I used the same order as there was before. No problem to change.

> One of the principles of the tests in all.sh is that they should progress
> from the simplest tests (the ones that have the fewest dependencies) first
> to the more complex tests (with many dependencies) last, and any tests 
> that are variants of the most basic fundamental tests should come AFTER 
> those more basic fundamental tests.  This is why we test the freebl/blapi
> tests first, because they have the fewest dependencies.  Other parts of NSS 
> depend on them, but they do not depend on other parts of NSS.  

Thanks for explanation.

Comment 5

[Slavomir Katuscak]

Attachment: Patch v2.

Previous patch with fixes suggested by Nelson.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

all.sh runs ssl.sh to do all the ssl testing.
As far as I know, the contents of the SSL variables are not needed until
ssl.sh runs, and only ssl.sh needs to know them.  When ssl.sh finishes,
no other script needs to know them.  If any script changes these variables
it should only be ssl.sh, and no others.  If ssl.sh needs to preserve these
variables, it can do so, and if it needs the original values after it has
modified them, it can do so.  All this can be done in ssl.sh.
So, why does all.sh need to define them?

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 290861 [details] [diff] [review]
Patch v2.

There are a bunch of variables that exist for only one purpose, to control the behavior of ssl.sh.  No other script besides ssl.sh has any need to look at them, to set them, 
to "back them up" or to "restore" them.  

ssl.sh runs only once in all.sh, so if it modifies its environment variables, does it need to restore them?  
For what other script's benefit would it do so?

I request that you rename the NSS_* variables that are only
for use by ssl.sh to SSL_*, and that you manipulate them 
only in ssl.sh.

Comment 8

[Slavomir Katuscak]

> ssl.sh runs only once in all.sh, so if it modifies its environment variables,
> does it need to restore them?  
> For what other script's benefit would it do so?

This is how it was before there were added PKIX tests and shared DB tests. Now all.sh runs all testing scripts from TESTS variable (including ssl.sh) 1-4 times (with default settings, with PKIX enabled, with DBs upgraded to shared DB and with native shared DB). We don't need to run all tests in all 4 cycles, for example for PKIX tests we need only tests with both server and client in bypass mode and that's the thing set in all.sh. 

Comment 9

[Slavomir Katuscak]

Patch from bug 401303 already fixed this bug.