Opened 3 years ago

Closed 2 years ago

#8079 enhancement closed fixed (fixed)

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

Reported by: Adi Roiban Owned by: hawkowl
Priority: normal Milestone:
Component: conch Keywords:
Cc: z3p Branch: 8079-no-gmpy
branch-diff, diff-cov, branch-cov, buildbot
Author:

Description

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.

Attachments (1)

timing.py (1.5 KB) - added by mark williams 3 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 3 years ago by DefaultCC Plugin

Cc: z3p added

comment:2 Changed 3 years ago by 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

comment:3 Changed 3 years ago by 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.

comment:4 Changed 3 years ago by hawkowl

Branch: 8079-no-gmpy

comment:5 Changed 3 years ago by hawkowl

Keywords: review added

comment:6 Changed 3 years ago by hawkowl

Owner: Adi Roiban deleted

Changed 3 years ago by mark williams

Attachment: timing.py added

comment:7 Changed 3 years ago by 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?

comment:8 Changed 3 years ago by 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

comment:9 Changed 3 years ago by Craig Rodrigues

Keywords: review removed
Owner: set to hawkowl

comment:10 Changed 3 years ago by 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.

comment:11 in reply to:  10 Changed 3 years ago by 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.

comment:12 Changed 2 years ago by Craig Rodrigues <rodrigc@…>

Resolution: fixed
Status: newclosed

In 01611cba:

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

Author: hawkowl
Reviewer: rodrigc
Fixes: #8079

Remove gmpy support

Note: See TracTickets for help on using tickets.