Create a simple gtest for all cipher suites

RESOLVED FIXED in Future

Status

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mt, Assigned: mt)

Tracking

trunk
Future

Firefox Tracking Flags

(firefox48 affected)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

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

Comment 1

3 years ago
Created attachment 8744153 [details] [diff] [review]
bug1266633.patch

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 2

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

Comment 3

3 years ago
Created attachment 8744462 [details] [diff] [review]
bug1266633_v2.patch

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
(Assignee)

Comment 4

3 years ago
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+
(Assignee)

Comment 5

3 years ago
Created attachment 8744744 [details] [diff] [review]
bug1266633.patch

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+

Comment 7

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

Comment 8

3 years ago
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.

Comment 9

3 years ago
(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.

Comment 10

3 years ago
Created attachment 8745138 [details]
only Way Elio Can Run Tests

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

Comment 11

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

Comment 12

3 years ago
Created attachment 8745148 [details] [diff] [review]
bug1266633.patch

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+
(Assignee)

Comment 13

3 years ago
Created attachment 8745153 [details] [diff] [review]
bug1266633.patch

Because I suck at history editing.
Attachment #8745148 - Attachment is obsolete: true
Attachment #8745153 - Flags: review+

Updated

3 years ago
Flags: needinfo?(ekr)

Updated

3 years ago
Attachment #8745138 - Attachment is obsolete: true
(Assignee)

Comment 14

3 years ago
https://hg.mozilla.org/projects/nss/rev/ae2655b46969
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Future
You need to log in before you can comment on or make changes to this bug.