key exchange performance degradation

[jtougas]

We've noticed a substantial degradation of performance in the key exchange portion of a ssh connection setup using asyncssh after upgrading cryptography from 40.0.2 to 41.0.0.

Full reproduction is available here here.

Here are some numbers that stood out from some profiling the same test case with version 41 vs 40:

crypto-41.prof

         1516341 function calls (1403297 primitive calls) in 2.502 seconds

   Ordered by: cumulative time
   List reduced from 6481 to 6416 due to restriction <0.99>
   List reduced from 6416 to 390 due to restriction <'cryptography'>

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
        1    0.000    0.000    0.379    0.379 /workspaces/project/.venv/lib/python3.10/site-packages/cryptography/hazmat/primitives/asymmetric/dh.py:50(parameters)
        1    0.000    0.000    0.379    0.379 /workspaces/project/.venv/lib/python3.10/site-packages/cryptography/hazmat/backends/openssl/backend.py:1479(load_dh_parameter_numbers)
        1    0.000    0.000    0.199    0.199 /workspaces/project/.venv/lib/python3.10/site-packages/cryptography/hazmat/primitives/asymmetric/dh.py:92(public_key)

crypto-40.prof

         1529613 function calls (1416401 primitive calls) in 2.011 seconds

   Ordered by: cumulative time
   List reduced from 6539 to 6474 due to restriction <0.99>
   List reduced from 6474 to 431 due to restriction <'cryptography'>

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
        1    0.000    0.000    0.037    0.037 /workspaces/project/.venv/lib/python3.10/site-packages/cryptography/hazmat/primitives/ciphers/aead.py:1(<module>)
        1    0.000    0.000    0.037    0.037 /workspaces/project/.venv/lib/python3.10/site-packages/cryptography/hazmat/backends/openssl/__init__.py:1(<module>)
        1    0.000    0.000    0.036    0.036 /workspaces/project/.venv/lib/python3.10/site-packages/cryptography/hazmat/backends/openssl/backend.py:1(<module>)

[johnfzc]

One important note about this issue, it doesn't occur with all key exchange algorithms. We see this degradation of performance with diffie-hellman-group-exchange-sha256 which is the best algorithm supported by some of the systems we connect to.

[alex]

Thanks for the clear report. Right now the minimal reproducer still has all of asyncssh, the next step for us is to minimize down to just the parts of pyca/cryptography that regressed.

It looks like https://github.com/ronf/asyncssh/blob/develop/asyncssh/crypto/dh.py is the relevant part of asyncssh.

Which part of it is in the hot-path?

[jtougas]

Nice. I removed asyncssh from the reproduction thanks to that and updated the README accordingly.
There seem to be two lines with noticeable degradation.

https://github.com/jtougas/ssh-perf-degraded/blob/053381ddcada12b7405171a248f5416f9ce764f2/main.py#L22

priv_key = pn.parameters().generate_private_key()

and

https://github.com/jtougas/ssh-perf-degraded/blob/053381ddcada12b7405171a248f5416f9ce764f2/main.py#L28

dh.DHPublicNumbers(peer_public, pn).public_key()

[alex]

Thanks! I suspect the issue might be that we are more aggressive in validating that DH params are valid now, but I'll dig and really measure this evening.

[alex]

@jtougas can you take a look and confirm if #9071 fixes this issue for you?

[jtougas]

Confirmed.

[2023-06-14 14:20:13,301] Using cryptography 42.0.0.dev1
[2023-06-14 14:20:13,301] 1
[2023-06-14 14:20:13,301] 2
[2023-06-14 14:20:13,369] 3
[2023-06-14 14:20:13,369] 4
[2023-06-14 14:20:13,369] 5
[2023-06-14 14:20:13,369] 6
[2023-06-14 14:20:13,373] 7
[2023-06-14 14:20:13,373] 8

[alex]

Great. We'll have to do some security analysis to make sure removing these checks isn't a problem, but it seems likely we'll be good to merge that.

[johnfzc]

Thanks very much for the quick fix on this issue. When do you expect to release a version that will include this fix?

[alex]

We don't currently have any releases planned, so I don't have a better answer than "TBD".

…

On Mon, Jun 19, 2023 at 8:36 AM John Marrett ***@***.***> wrote: Thanks very much for the quick fix on this issue. When do you expect to release a version that will include this fix? — Reply to this email directly, view it on GitHub <#9063 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAAGBE7ZTCOHPCEAR4PNBTXMBIXVANCNFSM6AAAAAAZEEXN64> . You are receiving this because you commented.Message ID: ***@***.***>

-- All that is necessary for evil to succeed is for good people to do nothing.