Disable ChaCha in transport tests that check cipher suite

RESOLVED FIXED in Firefox 48

Status

()

defect
P1
normal
Rank:
19
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mt, Assigned: mt)

Tracking

(Blocks 1 bug)

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

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

Comment 3

3 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.
backlog: --- → webrtc/webaudio+
Rank: 19
Priority: -- → P1
Assignee: nobody → martin.thomson
Martin -- Is this ready to land?
Flags: needinfo?(martin.thomson)
Assignee

Updated

3 years ago
Attachment #8724612 - Attachment is obsolete: true
Assignee

Comment 6

3 years ago
I want to run try again.  There was inexplicable bustage there.
Assignee

Comment 7

3 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")
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
Assignee

Comment 9

3 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

3 years ago
Flags: needinfo?(martin.thomson)

Comment 11

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b569aa5415ca
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.