Closed
Bug 1251822
Opened 8 years ago
Closed 8 years ago
Disable ChaCha in transport tests that check cipher suite
Categories
(Core :: WebRTC: Networking, defect, P1)
Core
WebRTC: Networking
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
backlog | webrtc/webaudio+ |
People
(Reporter: mt, Assigned: mt)
References
Details
Attachments
(1 file, 1 obsolete file)
This will allow us to land a cipher suite tweak that prioritizes ChaCha over AES for Android (ARM) builds.
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/37095/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37095/
Attachment #8724612 -
Flags: review?(ekr)
Updated•8 years ago
|
Attachment #8724612 -
Flags: review?(ekr) → review+
Comment 2•8 years ago
|
||
Comment on attachment 8724612 [details] MozReview Request: Bug 1251822 - Disable ChaCha20Poly1305 for unit tests that check cipher suite, r?ekr https://reviewboard.mozilla.org/r/37095/#review33639 ::: media/mtransport/test/transport_unittests.cpp:962 (Diff revision 1) > > +static void DisableChaCha(TransportTestPeer* peer) { > + // On ARM, ChaCha20Poly1305 might be preferred; disable it for the tests that > + // want to check the cipher suite. It doesn't matter which peer disables the > + // suite, disabling on either side has the same effect. > + std::vector<uint16_t> chachaSuites; > + chachaSuites.push_back(TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256); > + chachaSuites.push_back(TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256); > + peer->SetCipherSuiteChanges(std::vector<uint16_t>(), chachaSuites); > +} > + I'm OK with this, but wouldn't it be better to have an ARM #ifdef?
Assignee | ||
Comment 3•8 years ago
|
||
Absent more direct control over the suite order, this is best I think. If we wanted to predict the chosen suite, it would require careful coordination of the patch landings. This should allow us (Tim) to land the change in order for ARM. Then we can consider better testing of that change. BTW, I am of the opinion that platform-specific tests or variations are a bad idea because then you can't guarantee that everyone can run every variation.
Updated•8 years ago
|
backlog: --- → webrtc/webaudio+
Rank: 19
Priority: -- → P1
Updated•8 years ago
|
Assignee: nobody → martin.thomson
Assignee | ||
Comment 5•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40353/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40353/
Attachment #8731082 -
Flags: review?(ekr)
Assignee | ||
Updated•8 years ago
|
Attachment #8724612 -
Attachment is obsolete: true
Assignee | ||
Comment 6•8 years ago
|
||
I want to run try again. There was inexplicable bustage there.
Assignee | ||
Comment 7•8 years ago
|
||
(Of course, nothing in this game is inexplicable, it was probably the wrong version of m-c, one without the right version of NSS. Assume inexplicable means "couldn't be bothered working out what is wrong")
Updated•8 years ago
|
Attachment #8731082 -
Flags: review?(ekr) → review+
Comment 8•8 years ago
|
||
Comment on attachment 8731082 [details] MozReview Request: Bug 1251822 - Disable ChaCha20Poly1305 for unit tests that check cipher suite, r=ekr https://reviewboard.mozilla.org/r/40353/#review36921 ::: media/mtransport/test/transport_unittests.cpp:969 (Diff revision 1) > + std::vector<uint16_t> chachaSuites; > + chachaSuites.push_back(TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256); > + chachaSuites.push_back(TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256); > + peer->SetCipherSuiteChanges(std::vector<uint16_t>(), chachaSuites); This is not going to scale well, but I guess we'll find failure if we add another ChaCha suite (TLS_ECDHE_RLWE_RSA_WITH_..) or something ::: media/mtransport/test/transport_unittests.cpp:987 (Diff revision 1) > TEST_F(TransportTest, TestConnectSrtp) { > SetupSrtp(); > SetDtlsPeer(); > + DisableChaCha(p2_); > ConnectSocket(); Why are you disabling it in p1_ above and p2_ below? I know it doesn't matter, but consistency is good
Assignee | ||
Comment 9•8 years ago
|
||
https://reviewboard.mozilla.org/r/40353/#review36921 > Why are you disabling it in p1_ above and p2_ below? I know it doesn't matter, but consistency is good I know, but I wanted to disable from either end.
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(martin.thomson)
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b569aa5415ca
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•