search

cocagne / pysrp

Python implementation of the Secure Remote Password protocol (SRP)
MIT License
113 stars 41 forks source link

SRP updates #18

Open lsmag opened 8 years ago

lsmag commented 8 years ago

Hi,

I'm working on some updates on your library. These are the things I'm working on right now:

Also, why is the ctypes code optional? It is available in both py2 and py3 (at least in my Arch linux), is there some version or platform which doesn't come with this module? If ctypes is in fact optional, there's another option: compile _pysrp.py with Cython, if available, like mistune does, for example.

I'm yet to benchmark the Cython version against all others, but the advantage of Cython is that you can use just regular Python and still have a performance improvement, so maybe you can get rid of _ctsrp.py and maintain just _srp.c and _pysrp.py; less code duplication, less things to maintain. I'll update this thread later with the benchmarks.

cocagne commented 8 years ago

The reason for the ctypes version was for use on systems where compiling the c-module is not possible but the openssl library exists. The primary environment for which that is useful today is Pypy. If I recall correctly, there's only a minimal performance difference between the native C module and the ctypes version so if I were going to drop one, it'd probably be the C module. It's worth discussing though. With respect to Cython, I thought about that a couple years ago but ruled it out as it's not available everywhere. I haven't looked at it again since though so if things have changed, it might be worth reconsidering that stance.

Tom

On Fri, Jun 10, 2016 at 7:58 AM, lsmag notifications@github.com wrote:

Hi,

I'm working on some updates on your library. These are the things I'm working on right now:

  • making the C extension optional (I talked about it on a PR)
  • making the code py2/py3 compatible (I'll probably add tox to manage tests in each python version)

Also, why is the ctypes code optional? It is available in both py2 and py3 (at least in my Arch linux), is there some version or platform which doesn't come with this module? If ctypes is in fact optional, there's another option: compile _pysrp.py with Cython, if available, like mistune does https://github.com/lepture/mistune/blob/master/setup.py#L92, for example.

I'm yet to benchmark the Cython version against all others, but the advantage of Cython is that you can use just regular Python and still have a performance improvement, so maybe you can get rid of _ctsrp.py and maintain just _srp.c and _pysrp.py; less code duplication, less things to maintain. I'll update this thread later with the benchmarks.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/cocagne/pysrp/issues/18, or mute the thread https://github.com/notifications/unsubscribe/ABrLfGSylRy17gSz-RPp0NDJgC3vI78Vks5qKXuBgaJpZM4IzCI9 .

lsmag commented 8 years ago

Mistune actually only uses Cython if it's available. The documentation recommends installing it before if performance is desired, like pip install cython mistune. But anyway, forget about it: I benchmarked it against the pure python code and the performance wasn't good enough to justify the addition. Here's the Cython analysis of the pure python code: cython-analysis.zip

About pypy: you're right, I forgot about it. Maybe it's worth checking cffi out and see if it solves the problem? I'll add tox first and include pypy too, and then maybe I'll look into it.

lsmag commented 8 years ago

Just a quick update: cryptography.io supports PyPy and Py3 and has a handy OpenSSL cffi binding:

>>> from cryptography.hazmat.bindings.openssl.binding import Binding
>>> Binding.lib.BN_new()
<cdata 'BIGNUM *' 0x11d78d0>
cocagne commented 8 years ago

Thanks Ismag. After rerunning the performance tests, I'm starting to think you have the right idea. The C-implementation is not providing a significant performance benefit over the ctypes module so it's probably best to just remove it entirely. With respect to cryptography.io, thanks for pointing that out, I had no idea. The transparent Py3 support is awfully attractive and looks like it would dramatically simplify simultaneous Py2/Py3 support. If you get this working, please send me a pull request. I'll gladly accept it. If not, I'll put it on my queue for investigating when I can once again find time to work through the backlog.

lsmag commented 8 years ago

I'll send a PR with the first patches I mentioned in this thread, then I'll look into porting the ctypes code to cryptography.io. I'll let you know when I start working on it, as there will be some structural refactoring and the code isn't mine, of course :)

lsmag commented 8 years ago

So, a heads up of what I've done last night on this branch:

The test case right now is full of skipIf's, so the majority of the tests simply won't run in a Python version that won't support them, like the C implementation on Py3. They're probably going to disappear as I keep working in the source code, though.

There's a single bug I didn't solve yet on _ctsrp.py, which breaks py3's tests. I don't think it's worth it to fix that specific bug since I'm now moving to a new implementation using cryptography.io, which aims to replace the ctypes implementation completely.

Now, what I'm planning to do from now on:

Finally, about multiple versions: in my experience, installing cryptography.io works on almost all platofrms. It's written and tested to work on all major Python implementations, but I've seen the build fail in some CentOS bases, mostly because of an obsolete OpenSSL. Just letting you know that.

So, in the end, what do you think? Should I proceed?