Closed
Bug 1266633
Opened 8 years ago
Closed 8 years ago
Create a simple gtest for all cipher suites
Categories
(NSS :: Test, defect)
NSS
Test
Tracking
(firefox48 affected)
RESOLVED
FIXED
Future
Tracking | Status | |
---|---|---|
firefox48 | --- | affected |
People
(Reporter: mt, Assigned: mt)
Details
Attachments
(1 file, 5 obsolete files)
19.69 KB,
patch
|
mt
:
review+
|
Details | Diff | Splinter Review |
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•8 years ago
|
||
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)
Updated•8 years ago
|
Assignee: nobody → martin.thomson
Comment 2•8 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•8 years ago
|
||
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•8 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•8 years ago
|
||
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 6•8 years ago
|
||
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•8 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•8 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•8 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•8 years ago
|
||
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•8 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•8 years ago
|
||
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•8 years ago
|
||
Because I suck at history editing.
Attachment #8745148 -
Attachment is obsolete: true
Attachment #8745153 -
Flags: review+
Updated•8 years ago
|
Flags: needinfo?(ekr)
Updated•8 years ago
|
Attachment #8745138 -
Attachment is obsolete: true
Assignee | ||
Comment 14•8 years ago
|
||
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.
Description
•