Once conch.ssh depend on PyCA cryptography look at replacing gmpy

[Adi Roiban]

Once cryptography is merged I will try to look at gmpy... and see if it can be replaced with python cryptography.

Check that we still get a good performance... or maybe still use gmpy if it is available but also try python cryptography, if it has a better performance than the build it version.

[Daira Hopwood]

A reason to avoid gmpy is that it's unmaintained (the replacement is gmpy2) and does not have binary wheels on PyPI -- see ​https://tahoe-lafs.org/trac/tahoe-lafs/ticket/2094#comment:3

[hawkowl]

It's not installed on any of our builders, and if you want speed, use PyPy -- the Cryptography port means that conch now works fine on it.

We should remove it.

[hawkowl]

PR: ​https://github.com/twisted/twisted/pull/540

[mark williams]

I'm worried that using pow for modular exponentiation during Diffie Hellman key exchange opens conch up for timing attacks. I'm going off of ​Kocher's 1996 "Timing Attacks" paper. While I'm sure it no longer represents the state of the art, it does say that reduction in modular multiplication results in exploitable timing variations. I'm pretty sure that pow uses modular reduction for bignums (FIVEARY_CUTOFF is 8):

​https://github.com/python/cpython/blob/2.7/Objects/longobject.c#L3502-L3560 ​https://github.com/python/cpython/blob/3.5/Objects/longobject.c#L3915-L3973

Obviously I'm not great at math, so I wrote a program that measures the time taken to do DH exponentiation via cryptography's bindings to OpenSSL's bignum code or Python's pow. I've attached it as timing.py.

Here are the results (python means pow and openssl means OpenSSL's bignums):

$ ministat -n python openssl
x python
+ openssl
    N           Min           Max        Median           Avg        Stddev
x 10000  0.0058999062   0.018929005  0.0061118603  0.0063149349 0.00057601654
+ 10000 0.00078105927  0.0015981197 0.00080513954 0.00082731056 8.3011552e-05

OpenSSL's bignums are both faster (lower median) and more consistent (smaller standard deviation.) I think the former is nice and the latter is pretty important.

The downside is that cryptography doesn't expose easy-to-use bindings for OpenSSL's bignum code. If what I'm saying sounds reasonable, I can look into those bindings.

Thoughts?

[Adi Roiban]

gmpy, at least on Ubuntu works... and is the fastest option available

I can do pip install gmpy and I get a big performance improvement.

I am using OpeSSH client as ssh -oKexAlgorithms=diffie-hellman-group-exchange-sha256 -p 5022 user@localhost and run a simple test using the simple ssh server example (​https://gist.github.com/adiroiban/c0e309b6d58494b8ebea).

Some raw measurements on my laptop i7-4500U CPU @ 1.80GHz with Ubuntu 16.04:

• py27 without gmpy 3.5s
• py27 with gmpy 0.6s
• pypy 5.4.1 1.8s

My intention with this ticket was to check if we already have the equivalent methods in cryptography and try to use them instead of python code .

Since this is a soft dependency I think that we should remove it only if the openssl version is faster.

Installing gmpy on the Travis-ci should not be a complicated task.

Since pypy is still slower than py2.7 + gmpy, I am -1 on completely removing the gmpy support, as of now.

Leaving this in review to get feedback from more devs

[Craig Rodrigues]

​https://github.com/twisted/twisted/pull/540#pullrequestreview-479322

[hawkowl]

So, markrwilliams: that sounds good, although I don't think it should block this ticket.

Adi: there is a bit more perf using gmpy for the initial connection; yes, but Twisted also shouldn't monkeypatch globals. People that want to accept the risk of gmpy possibly crashing and causing a DoS (which it's been known to do if you pass it the wrong things), in potentially random parts of their python, can copy out the old code and monkeypatch the globals themselves. Randomly changing behaviour in a potentially insecure and surprising way, to code that *happens* to import something from conch -- not even using it! -- I don't think is acceptable.

If we still want gmpy in Conch, I think we should do it by integrating gmpy2, which publishes binary wheels, and implement it in a way that doesn't involve monkeypatching globals, instead explicitly passing in the ones they should use, or at least monkeypatching the module like the rest of the old code did.

[mark williams]

Replying to hawkowl:

So, markrwilliams: that sounds good, although I don't think it should block this ticket.

Fair enough. gmpy has enough problems that it should go.

If we still want gmpy in Conch, I think we should do it by integrating gmpy2

I don't think gmpy2 is worth the effort, because ​OpenSSL actually has Diffie-Hellman calculation primitives. (I was totally wrong about using BN_mod_exp before -- crypto is hard.)

I expect DH_compute_key to be comparable in speed and security to OpenSSH because OpenSSH itself uses it. Unfortunately, cryptography doesn't expose a public API for this or DH_generate_key yet. Once it does, we can revisit this in #8831.

[Craig Rodrigues <rodrigc@…>]

In 01611cba:

Merge pull request #540 from twisted/8079-no-gmpy

Author: hawkowl
Reviewer: rodrigc
Fixes: #8079

Remove gmpy support