Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CCM ciphersuite causes a lot of CPU load #199

Closed
cohosh opened this issue Feb 21, 2020 · 11 comments
Closed

CCM ciphersuite causes a lot of CPU load #199

cohosh opened this issue Feb 21, 2020 · 11 comments
Assignees

Comments

Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants
@cohosh
Copy link

@cohosh cohosh commented Feb 21, 2020

Your environment.

  • Version: v2.1.18
  • Browser: N/A (Snowflake)
  • Other Information: CPU profiling output given below

What did you do?

We've been running Snowflake with the pion/webrtc library for a while now. We noticed that our Snowflake proxies were using a lot more CPU than expected in sending and receiving WebRTC messages.

More info on snowflake: https://snowflake.torproject.org

What did you expect?

We expected the CPU usage to be around 10-20% for proxying a single client's traffic.

What happened?

Our CPU usage for a single client was over 60%.

After some investigations, we tracked a lot of CPU heavy operations coming from the CCM ciphersuite. Changing the ciphersuite to GCM resulted in the more expected usage of 10-20%.

See our ticket here: https://trac.torproject.org/projects/tor/ticket/33211#comment:14

I'd suggest

  1. allowing a way for application develoeprs to change the ciphersuite (as filed here: pion/webrtc#1043)

  2. taking a look at the CCM implementation to see if there are some optimizations available there, and

  3. using a different default ciphersuite and update the priority listing of available ciphersuites. GCM is supposed to be slightly more efficient, and is much more commonly seen in practice.

@Sean-Der
Copy link
Member

@Sean-Der Sean-Der commented Feb 21, 2020

My guess is this list is used backwards, and we are preferring the wrong CipherSuites.

I will look into that, and make sure we get a test in. Thanks for filing this @cohosh! I doubt we will ever get close to beating GCM Go, the stdlib use AES-NI and has a bunch of scrutiny :)

@cohosh
Copy link
Author

@cohosh cohosh commented Feb 21, 2020

Yep, that seems plausible. This is pion-to-pion traffic so the ciphersuite used should be the highest priority one listed there.

@cohosh
Copy link
Author

@cohosh cohosh commented Feb 21, 2020

It could also be that this list of all cipher is being used instead.

Sean-Der added a commit that referenced this issue Feb 21, 2020
Before encodeCipherSuites would improperly encode backwards, putting
our least preferred suite first. Fix this function so it properly encodes
and update TestHandshakeMessageClientHello so it has multiple CipherSuites
to make sure this doesn't regress.

Relates to #199
Sean-Der added a commit that referenced this issue Feb 21, 2020
Before encodeCipherSuites would improperly encode
backwards, putting our least preferred suite first. Fix
this function so it properly encodes and update
TestHandshakeMessageClientHello so it has multiple CipherSuites
to make sure this doesn't regress.

Relates to #199
@Sean-Der
Copy link
Member

@Sean-Der Sean-Der commented Feb 21, 2020

@cohosh Fixed! Mind reviewing that PR, I will merge + tag across.

Sean-Der added a commit that referenced this issue Feb 21, 2020
Match OpenSSL (and Chromium's) ordering of CipherSuites.
Go also has a hardware accelerated implementation, so this should
be a better experience in almost all cases.

Relates to #199
@Sean-Der
Copy link
Member

@Sean-Der Sean-Der commented Feb 21, 2020

Screen Shot 2020-02-21 at 2 26 26 PM

@Sean-Der
Copy link
Member

@Sean-Der Sean-Der commented Feb 21, 2020

@daenney @at-wat @igolaizola @cohosh I also have moved GCM above CBC. I wanted to add everyone, this feels like something we should have consensus on.

My motivation is

  • Matches Chromium/FireFox
  • In most cases will be faster for users

I have attached a screenshot above showing Chromium Nightly's current values.

@cohosh
Copy link
Author

@cohosh cohosh commented Feb 21, 2020

Nice, thanks @Sean-Der ! I'll take a look at this and get back to you later tonight.

Sean-Der added a commit that referenced this issue Feb 22, 2020
Match OpenSSL (and Chromium's) ordering of CipherSuites.
Go also has a hardware accelerated implementation, so this should
be a better experience in almost all cases.

Relates to #199
Sean-Der added a commit that referenced this issue Feb 22, 2020
Before encodeCipherSuites would improperly encode
backwards, putting our least preferred suite first. Fix
this function so it properly encodes and update
TestHandshakeMessageClientHello so it has multiple CipherSuites
to make sure this doesn't regress.

Relates to #199
Sean-Der added a commit that referenced this issue Feb 22, 2020
Match OpenSSL (and Chromium's) ordering of CipherSuites.
Go also has a hardware accelerated implementation, so this should
be a better experience in almost all cases.

Relates to #199
@daenney
Copy link
Contributor

@daenney daenney commented Feb 22, 2020

CCM should really not have been advertised or picked by default at all. That was only added in DTLS in order to support some IoT use-cases, b/c among other things the CoAP RFC requires it.

@Sean-Der I'd vote we add a second change, dropping any CCM suite from the defaults. Using CCM should be an explicit opt-in.

@daenney
Copy link
Contributor

@daenney daenney commented Feb 22, 2020

W.r.t optimising CCM, that's not really been undertaken yet b/c it would probably mean having to drop down to assembly. That's a maintenance burden and potential massive source of security issues I don't want to incur just yet.

@cohosh
Copy link
Author

@cohosh cohosh commented Feb 22, 2020

Thanks for taking care of this, the changes solve the issue we were having at our end :)

Sean-Der added a commit that referenced this issue Feb 22, 2020
If users want CCM they need to explicitly request it.

Resolves #199
@Sean-Der
Copy link
Member

@Sean-Der Sean-Der commented Feb 22, 2020

@daenney done! opened PR and added you as @jkralik as reviewer.

Sean-Der added a commit that referenced this issue Feb 23, 2020
If users want CCM they need to explicitly request it.

Resolves #199
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment