Closed Bug 1266633 Opened 8 years ago Closed 8 years ago

Create a simple gtest for all cipher suites

Categories

(NSS :: Test, defect)

defect
Not set
normal

Tracking

(firefox48 affected)

RESOLVED FIXED
Future
Tracking Status
firefox48 --- affected

People

(Reporter: mt, Assigned: mt)

Details

Attachments

(1 file, 5 obsolete files)

This should help with adding new tests, and reduce the dependency on the script-based tests.  I have a first cut at this and it takes less than 3 seconds to run on my machine in debug mode.
Attached patch bug1266633.patch (obsolete) — Splinter Review
I noticed that TLS 1.3 allows a DHE suite, but it doesn't support DHE, so I moved it.

n-i on ekr so that he notices this.
Flags: needinfo?(ekr)
Attachment #8744153 - Flags: review?(emaldona)
Assignee: nobody → martin.thomson
Comment on attachment 8744153 [details] [diff] [review]
bug1266633.patch

Review of attachment 8744153 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you Martin. The problem is that the patch didn't apply cleanly after the recent changes in the code. Other changes LGTM. I'm probably not the best qualified reviewer when it comes to gtest which I'm just learning.

::: external_tests/ssl_gtest/ssl_ciphersuite_unittest.cc
@@ +46,5 @@
> +
> +      case ssl_auth_ecdsa:
> +      case ssl_auth_ecdh_rsa:
> +      case ssl_auth_ecdh_ecdsa:
> +        ResetEcdsa();

The linker couldn't resolve this one.
Attachment #8744153 - Flags: review?(emaldona) → review-
Attached patch bug1266633_v2.patch (obsolete) — Splinter Review
The modified version of the previous patch and what that I'm currently using. This is a subset of the one one I posted at https://bug923089.bmoattachments.org/attachment.cgi?id=8744456
Comment on attachment 8744462 [details] [diff] [review]
bug1266633_v2.patch

Yep, those look like the same changes that I needed to make.  Sorry for missing the rebase.
Attachment #8744462 - Flags: feedback+
Attached patch bug1266633.patch (obsolete) — Splinter Review
Here's an updated patch.  Elio, you can use this as a basis, just add your ciphersuites in the appropriate places.
Attachment #8744153 - Attachment is obsolete: true
Attachment #8744462 - Attachment is obsolete: true
Attachment #8744744 - Flags: review?(ttaubert)
Comment on attachment 8744744 [details] [diff] [review]
bug1266633.patch

Review of attachment 8744744 [details] [diff] [review]:
-----------------------------------------------------------------

::: external_tests/ssl_gtest/ssl_ciphersuite_unittest.cc
@@ +49,5 @@
> +        Reset(TlsAgent::kServerEcdsa);
> +        break;
> +      case ssl_auth_ecdh_rsa:
> +        Reset(TlsAgent::kServerEcdhRsa);
> +        break;

Should we fail with an error message that explicitly says this is (currently) unsupported? Might be more helpful than any of the error messages that might pop up if we actually try to set up the server.

@@ +88,5 @@
> +// This awful macro makes the test instantiations easier to read.
> +#define INSTANTIATE_CIPHER_TEST_P(name, modes, versions, ...)           \
> +  static const uint16_t k##name##CiphersArr[] = { __VA_ARGS__ };        \
> +  static const ::testing::internal::ParamGenerator<uint16_t>            \
> +  k##name##Ciphers = ::testing::ValuesIn(k##name##CiphersArr);          \

Maybe indent this so it's clear it's a continuation of the previous line.

@@ +97,5 @@
> +                               k##name##Ciphers))
> +
> +// The cipher suites of type ECDH_RSA are disabled because we don't have
> +// a compatible certificate in the test fixture.
> +INSTANTIATE_CIPHER_TEST_P(RC4, Stream, V10ToV12,

We have this left in ssl_loopback_uinittest.cc:

> TEST_P(TlsConnectStreamPre13, ConnectRC4) {
>   ConnectWithCipherSuite(TLS_RSA_WITH_RC4_128_SHA);
> }

Should this be moved here as well? It's also the last remaining use of TlsConnectTestBase::ConnectWithCipherSuite().

::: lib/ssl/ssl3con.c
@@ +756,1 @@
>              return vrange->max == SSL_LIBRARY_VERSION_TLS_1_2;

Possibly stupid question, has this moved here because in practice SChannel doesn't allow anything other than 1024-bit DHE and as those are unsupported in TLS 1.3 there's no way to make this work? I thought that in general DHE_RSA could work with 2048+-bit groups.

Should we move this to a separate bug to document this change?
Attachment #8744744 - Flags: review?(ttaubert) → review+
I tried this patch by itself (none on my sh284 related patches applied) and got
SUMMARY:
========
NSS variables:
--------------
HOST=localhost
DOMSUF=localdomain
BUILD_OPT=
USE_X32=
USE_64=1
NSS_CYCLES="standard"
NSS_TESTS="ssl_gtests"
NSS_SSL_TESTS=" "
NSS_SSL_RUN=" "
NSS_AIA_PATH=
NSS_AIA_HTTP=
NSS_AIA_OCSP=
IOPR_HOSTADDR_LIST=
PKITS_DATA=

Tests summary:
--------------
Passed:             726
Failed:             1
Failed with core:   0
Unknown status:     0

~/testMTPatch
selfserv_9915: no process found
error: test suite had 5 test failure(s)
Makefile:32: recipe for target 'some' failed
make: *** [some] Error 1
[emaldona@dhcp-16-216 testMTPatch]$ grep FAILED tests_results/security/localhost.1/output.log 
[  FAILED  ] SSLv2ClientHelloTestF.Connect13 (5023 ms)
[  FAILED  ] 1 test, listed below:
[  FAILED  ] SSLv2ClientHelloTestF.Connect13
 1 FAILED TEST
ssl_gtest.sh: #9: ssl_gtest run successfully  - FAILED

Here is an excerpt of output.log
...................
[ RUN      ] CipherSuiteCBCDatagram/TlsCipherSuiteTest.ConnectWithCipherSuite/17
Version: DTLS 1.2
Cipher suite: TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA
rsa_sign: Changing state from INIT to CONNECTING
client: Changing state from INIT to CONNECTING
client: Handshake
client: Would have blocked
rsa_sign: Handshake
rsa_sign: Would have blocked
Poll() waiters = 2 timers = 348
client: Readable
client: Handshake
client: Would have blocked
Poll() waiters = 2 timers = 347
rsa_sign: Readable
rsa_sign: Handshake
rsa_sign: Handshake success
rsa_sign: Changing state from CONNECTING to CONNECTED
client: Readable
client: Handshake
client: Handshake success
client: Changing state from CONNECTING to CONNECTED
Connected with version 771 cipher suite TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA
client: Writing 50 bytes
rsa_sign: Writing 50 bytes
Poll() waiters = 2 timers = 346
rsa_sign: Readable
rsa_sign: ReadBytes 50
client: Readable
client: ReadBytes 50
[       OK ] CipherSuiteCBCDatagram/TlsCipherSuiteTest.ConnectWithCipherSuite/17 (14 ms)
[----------] 18 tests from CipherSuiteCBCDatagram/TlsCipherSuiteTest (320 ms total)

[----------] Global test environment tear-down
[==========] 718 tests from 31 test cases ran. (29035 ms total)
[  PASSED  ] 717 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] SSLv2ClientHelloTestF.Connect13

 1 FAILED TEST
  YOU HAVE 5 DISABLED TESTS

ssl_gtest.sh: #9: ssl_gtest run successfully  - FAILED
.......................
Elio, I'm not getting that error.  Would you mind running the test again?  Like so:

LD_LIBRARY_PATH=<.../dist/Linux.../lib> \
  <.../dist/Linux.../lib/ssl_gtest -d tests_results/security/localhost.1/ssl_gtests --gtest_filter=SSLv2ClientHelloTestF.Connect13

It looks like you delete things, so you might need to run ./ssl_gtests.sh again to build the directory.
(In reply to Martin Thomson [:mt:] from comment #8)
> Elio, I'm not getting that error.  Would you mind running the test again? 
> Like so:
> 
> LD_LIBRARY_PATH=<.../dist/Linux.../lib> \
>   <.../dist/Linux.../lib/ssl_gtest -d
> tests_results/security/localhost.1/ssl_gtests
> --gtest_filter=SSLv2ClientHelloTestF.Connect13
> 
> It looks like you delete things, so you might need to run ./ssl_gtests.sh
> again to build the directory.

For some reason I cannot run the tests the normal way so let me attach a tar ball with the supprting Makefile and a scripts that I use.
Attached file only Way Elio Can Run Tests (obsolete) —
I get many failures when I run the test the way it's documented no matter what system. To work around this problem I use the setup that you'll see when you extract the files in this archive. Building is not a problem but testing is.

In the directory above nss is where I place the Makefile and the two shell scripts. I use the target 'make test' tun run the entire test suite. The 'make some' target I use to run only the gtests.
(In reply to Tim Taubert [:ttaubert] from comment #6)
> Should we fail with an error message that explicitly says this is (currently) unsupported? Might be more helpful than any of the error messages that might pop up if we actually try to set up the server.

I've removed the kServerEcdhRsa value (well, moved it behind comments).  That way, it fails to compile if used outside of this file; and then it will hit the default statement of the switch.

> Possibly stupid question, has this moved here because in practice SChannel
> doesn't allow anything other than 1024-bit DHE and as those are unsupported
> in TLS 1.3 there's no way to make this work? I thought that in general
> DHE_RSA could work with 2048+-bit groups.

I should have made it a separate change, but it's because we don't support DHE for 1.3.  See https://bugzilla.mozilla.org/show_bug.cgi?id=1266237
 
> Should we move this to a separate bug to document this change?

Yes, that would be best.  I'll remove it here and add it to bug 1267504.
Attached patch bug1266633.patch (obsolete) — Splinter Review
Tree is closed, so I'll leave this here for the next week or so.
Attachment #8744744 - Attachment is obsolete: true
Attachment #8745148 - Flags: review+
Attached patch bug1266633.patchSplinter Review
Because I suck at history editing.
Attachment #8745148 - Attachment is obsolete: true
Attachment #8745153 - Flags: review+
Flags: needinfo?(ekr)
Attachment #8745138 - Attachment is obsolete: true
https://hg.mozilla.org/projects/nss/rev/ae2655b46969
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Future
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: