-
Notifications
You must be signed in to change notification settings - Fork 280
add ecdsa handshake test #1146
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
add ecdsa handshake test #1146
Conversation
cc: @adamsitnik |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but please respond to my question of whether we might want to add more certificates in the future or not. If not, I am going to merge it as is.
src/benchmarks/micro/libraries/System.Net.Security/SslStreamTests.cs
Outdated
Show resolved
Hide resolved
src/benchmarks/micro/libraries/System.Net.Security/SslStreamTests.cs
Outdated
Show resolved
Hide resolved
I'm not sure if this is the best place but since we have all experts here looking at the SSL handshake test, let me dump it here. I was trying to figure out why there is so big difference for HandshakeAsync between Linux and Windows. 980 vs 45K seems quite a big difference when the rest of the crypto seem similar. After the initial handshake, there is 10s gap and then one would expect a new handshake with ClientHello as seen before. But that is not happening. Instead, clients send another encrypted message and probably resumes session with a ticket obtained at frame 21. (https://docs.microsoft.com/en-us/powershell/module/tls/disable-tlssessionticketkey?view=win10-ps) TLS13 is another issue as the handshake flow is quite different. I'm wondering if it would make sense to update the test and pin it to TLS12 and perhaps change the test so we measure same work by avoiding resumption. (or we can create a separate test aimed to compare apples to apples) On the OpenSSL side, tls tickets and resumption are possible but need to be explicitly requested and it is not used by default. If we believe there is value in this (mainly for microservices connecting to same endpoint) I can open a new issue in runtime repro to track it as feature work. |
What's the downside? |
Significantly changing the PAL to manage the session state manager, probably introducing global state and possibly introducing locking. Increased native memory usage, though probably not a whole lot. |
Thanks, Jeremy. Re: "Significantly changing the PAL to manage the session state manager"... presumably that's "just work" rather than some other ramification? re: "probably introducing global state and possibly introducing locking"... this is something to be wary of, but not a showstopper. re: "Increased native memory usage, though probably not a whole lot."... makes sense. Given all that, it sounds like something we should try. There is definitely value in connections to the same endpoint, whether it be a true production benefit for microservice workloads, a things-work-similarly-across-platforms consistency benefit, or a feel-good benchmark benefit. |
Yeah, none of it is showstopper. Just "downside" :) |
That sounds about right. Note that as I was going through TLS13 spec, they talk a lot about latency and roundtrips. |
To get closure on this, I decided to act on @bartonjs's recommendation and name tests by the algorithm. I also decided to leave old tests as it is and write a new version using Pipes. Te get everything more comparable, I locked test to TLS1.2 and I generated a set of certificates where only the key is different (e.g. all v3 extensions are identical) Since the original key was 4K RSA, one could expect similar performance. Here are the results (on somewhat comparable machines) Windows 10:
Linux Ubuntu 18.04 with OpenSSL 1.1.1
There are some interesting anomalies:
I'll keep digging into this but I feel that does not need to help back new tests. |
src/benchmarks/micro/libraries/System.Net.Security/SslStreamTests.cs
Outdated
Show resolved
Hide resolved
src/benchmarks/micro/libraries/System.Net.Security/SslStreamTests.cs
Outdated
Show resolved
Hide resolved
We do! Once we merge this PR (is it ready now?) our infra is going to run it daily and publish the results to our Reporting System I am going to forward you an email from @DrewScoggins who explains how to use it. |
yes, this should be good to go @adamsitnik. I may add more tests later but that should not hold this back IMHO. |
The final result from CI machines on identical hardware:
Rest of the tests is on a par between OSes. |
This is primarily to show perf impact on TLS handshake with different algorithms.
I was curious after seeing most CPU burned in BN_ functions in OpenSSL.
ECDSH shows ~25% difference on my Linux box.
For consistency, I did run on each platform and OSX is even more surprising
cc: @stephentoub @adamsitnik @bartonjs