338208 - Need SSL tests using FIPS140-2 softoken

Status: RESOLVED FIXED

(NSS :: Test, enhancement, P2)

Description

[Julien Pierre]

Currently, ssl.sh only tests softoken in non-FIPS mode.
We need to make another pass in FIPS mode to make sure the higher-level code is OK.

SSL2 tests need to be disabled in this case, but all other tests are expected to work.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

assigning to Christophe, who I believe is already working on this.
Christophe, please attach your patch (or new script) to this bug 
so that others can start to test with it.

Comment 2

[Julien Pierre]

Attachment: Christophe's initial patch

This is what Christophe put together friday. It works fine against the tip. It does FIPS140-2 only, not non-FIPS and bypass modes.

This patch still tries to run the SSL2 tests, so you have to ignore these results.

Comment 3

[Julien Pierre]

Retargetting all P2s to 3.11.3 .

Comment 4

[Julien Pierre]

Comment on attachment 222265 [details] [diff] [review]
Christophe's initial patch

This patch needs to be changed to do the following before it can be checked in :
- skip all SSL2 cipher suites when the softoken is in FIPS140-2 mode
- skip all export cipher suites when the softoken is in FIPS140-2 mode

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #4)
> This patch needs to be changed to do the following before it can be checked in
> - skip all SSL2 cipher suites when the softoken is in FIPS140-2 mode
> - skip all export cipher suites when the softoken is in FIPS140-2 mode

Should these perhaps remain as negative tests?  
(That's not a rhetorical question.  I'm really not sure.)

Comment 6

[Julien Pierre]

Nelson,

No, they can't be negative tests because not all export cipher suites fail in FIPS mode, only a few of them, as reported in bug 338211 . So the best we can do is skip them.

Comment 7

[Julien Pierre]

*** Bug 338211 has been marked as a duplicate of this bug. ***

Comment 8

[Slavomir Katuscak]

Attachment: Patch.

New patch based on Christophe's patch.

Some changes:
-other tests are not disabled by default
-tests in FIPS mode can be disabled (using NSS_TEST_DISABLE_FIPS variable, similar to NSS_TEST_DISABLE_BYPASS)
-SSL2 tests and export tests are skipped in FIPS mode

Comment 9

[Slavomir Katuscak]

Attachment: Patch v2.

Small fix to previous patch (fixed message when skipping FIPS-mode tests).

Comment 10

[Julien Pierre]

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

Why do we need to make the FIPS tests optional ?

Assuming we do, this patch is lacking error checking on the new modutil commands.

Comment 11

[Slavomir Katuscak]

Attachment: Patch v3.

Testing in FIPS mode takes some time. There was a request to create a shorter version of all.sh (bug 335752), and we decided to have some test blocks optional. As bypass mode is optional, I think with FIPS mode it should be the same.

Modutil error checking added in this patch.

Comment 12

[Julien Pierre]

Comment on attachment 271823 [details] [diff] [review]
Patch v3.

You need to check the return code for each modutil. You call modutil twice in a row, but only check the result from the 2nd one.

Comment 13

[Slavomir Katuscak]

Attachment: Patch v4.

Added checking for return values.

In fact I was not checking return value of modutil before, but return value of grep piped to it (to check if fips module is really in db). Now there are 3 tests - return value of first modutil (setting mode), return value of second modutil (db list) and return value of grep (checking if fips module is in db).

Comment 14

[Slavomir Katuscak]

Attachment: Patch v5.

Fixed messages.

Comment 15

[Julien Pierre]

Slavo, there is no difference between patchv4 and patchv5 . What did you intend to change ?

Comment 16

[Julien Pierre]

Comment on attachment 272143 [details] [diff] [review]
Patch v5.

OK, I see it. I think you meant to change the words "on" to "off" in the second block (turning FIPS "off" rather than "on"). Please attach the correct patch.

Comment 17

[Slavomir Katuscak]

Comment on attachment 272143 [details] [diff] [review]
Patch v5.

New patch was patch v4, but after I sent it to bugzilla I fixed on to off and sent is as patch v5. So patch v5 should be the correct patch and changes between v3 and v5 are described in comment #13.

Comment 18

[Julien Pierre]

Comment on attachment 272143 [details] [diff] [review]
Patch v5.

Slavo,

You mistakenly attached the same patch for patchv4 and patchv5 . The diff diff feature in bugzilla shows no differences, as well as manual inspection. Please attach the patch with the correct strings.

Comment 19

[Slavomir Katuscak]

Attachment: Patch v6.

Sorry, I haven't noticed this. Sending new patch now.

Comment 20

[Slavomir Katuscak]

Trunk:
Checking in ssl.sh;
/cvsroot/mozilla/security/nss/tests/ssl/ssl.sh,v  <--  ssl.sh
new revision: 1.81; previous revision: 1.80
done

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 272942 [details] [diff] [review]
Patch v6.

r=nelson.  Note that

>+    if [ "${FIPSMODE}" = "true" ] ; then
>+        RET_EXP=0
>+    else
>+        RET_EXP=1
>+    fi

is equivalent to this:

      [ "${FIPSMODE}" = "true" ] ; RET_EXP=$?

Comment 22

[Slavomir Katuscak]

Checked in last patch + Nelson's suggested change from previous comment.

Trunk:
Checking in ssl.sh;
/cvsroot/mozilla/security/nss/tests/ssl/ssl.sh,v  <--  ssl.sh
new revision: 1.82; previous revision: 1.81
done

Branch:
Checking in ssl.sh;
/cvsroot/mozilla/security/nss/tests/ssl/ssl.sh,v  <--  ssl.sh
new revision: 1.61.2.13; previous revision: 1.61.2.12
done