Closed Bug 1251822 Opened 5 years ago Closed 5 years ago

Disable ChaCha in transport tests that check cipher suite

Categories

(Core :: WebRTC: Networking, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed
Blocking Flags:

People

(Reporter: mt, Assigned: mt)

References

(Blocks 1 open bug)

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.
Attachment #8724612 - Flags: review?(ekr) → review+
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?
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.
backlog: --- → webrtc/webaudio+
Rank: 19
Priority: -- → P1
Assignee: nobody → martin.thomson
Martin -- Is this ready to land?
Flags: needinfo?(martin.thomson)
Attachment #8724612 - Attachment is obsolete: true
I want to run try again.  There was inexplicable bustage there.
(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")
Attachment #8731082 - Flags: review?(ekr) → review+
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
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.
Flags: needinfo?(martin.thomson)
https://hg.mozilla.org/mozilla-central/rev/b569aa5415ca
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.