Opened 5 years ago

Closed 3 years ago

#7413 enhancement closed fixed (fixed)

twisted.conch should use PyCA cryptography instead of PyCrypto

Reported by: radix Owned by: Tristan Seligmann
Priority: normal Milestone:
Component: core Keywords:
Cc: Alex Gaynor, Adi Roiban Branch: branches/modern-cryptography-7413-3
branch-diff, diff-cov, branch-cov, buildbot
Author: adiroiban, mithrandi, glyph

Description

This would allow it to run on PyPy, and probably has other benefits.

Change History (30)

comment:1 Changed 5 years ago by radix

comment:2 Changed 5 years ago by Alex Gaynor

Cc: Alex Gaynor added

comment:3 Changed 5 years ago by zooko

related: #4633

comment:4 Changed 4 years ago by Alex Gaynor

Just for the record, the branch of mine that radix linked is now basically complete, however it is blocked by #7693. I'm unlikely to have the time to see this through, but anyone is welcome to run with my code, and I can try to answer questions.

comment:5 Changed 4 years ago by zooko

Ticket describing how this bug impacts Tahoe-LAFS: https://tahoe-lafs.org/trac/tahoe-lafs/ticket/2330

comment:6 Changed 4 years ago by Adi Roiban

Cc: Adi Roiban added

comment:7 Changed 3 years ago by Glyph

Author: glyph
Branch: branches/modern-cryptography-7413

(In [46025]) Branching to modern-cryptography-7413.

comment:8 Changed 3 years ago by Glyph

The branch does replace most of the actual crypto, but it's not really complete. test_keys still uses it in a few places, the buildbots need to be updated to a newer version of cryptography that includes DSA (tests are failing), numerous docstrings need to be updated, and there are compatibility concerns to be resolved. Nevertheless the blocker has been resolved so anyone interested in this can feel free to start contributing to the branch.

comment:9 Changed 3 years ago by Adi Roiban

Hi, I would love to see the python cryptography used ... since we already depend on if if a newer pyOpenSSL is used.

Maybe we can create a milestone for the new python cryptography dependency... I would also like to see gmpy gone and replaced it with cryptography.

Do you want people to contribute to the new branch as a "feature branch", ie apply/review patches based on it, rather than trunk?


Later maybe we can deprecate the whole twisted.conch.ssh.key module and start with a fresh one?

Maybe we can split the loading/serialization code from the crypto related (sign/verify) code.... and have an implementation based on zope.interfaces

One thing that I would like to have in Twisted is HSM/Smartcard integration... so that I can brink my own ssh.Key implementation which will know how to forward sign/verify requests to the HSM/smartcard

...but I think that this should be done in cooperation with the python cryptography developers.

comment:10 Changed 3 years ago by Adi Roiban

On a second thought the setter/getter for the keyObject ivar will not work... so maybe we can just create a new Key object and deprecate the old one.

comment:11 in reply to:  9 ; Changed 3 years ago by Glyph

Replying to adiroiban:

Hi, I would love to see the python cryptography used ... since we already depend on if if a newer pyOpenSSL is used.

Do we even still work with the older pyOpenSSL? So many features are only available in the new one...

Maybe we can create a milestone for the new python cryptography dependency... I would also like to see gmpy gone and replaced it with cryptography.

I think we might actually just try to eliminate the gmpy dependency. Do we have hard evidence that it's faster than Python's built-in arbitrary-precision integers?

Do you want people to contribute to the new branch as a "feature branch", ie apply/review patches based on it, rather than trunk?

No, I don't think this is going to get much bigger. Most of the remaining changes are docs, there might be a bit of compatibility glue.

Later maybe we can deprecate the whole twisted.conch.ssh.key module and start with a fresh one?

It seems like most of the module is fine.

Maybe we can split the loading/serialization code from the crypto related (sign/verify) code.... and have an implementation based on zope.interfaces

You can never completely separate these, a fact that the Cryptography library acknowledges with its "backend" interfaces.

One thing that I would like to have in Twisted is HSM/Smartcard integration... so that I can brink my own ssh.Key implementation which will know how to forward sign/verify requests to the HSM/smartcard

This is way, way out of scope for this ticket :).

...but I think that this should be done in cooperation with the python cryptography developers.

comment:12 in reply to:  10 Changed 3 years ago by Glyph

Replying to adiroiban:

On a second thought the setter/getter for the keyObject ivar will not work... so maybe we can just create a new Key object and deprecate the old one.

Actually... upon further consideration, I think it would work :).

Here's how it would go: internally we'd have a new _cryptographyKeyObject and base all operations on that.

Then we have a new, deprecated keyObject attribute, which only works if you have PyCrypto installed, and has a local import of all necessary dependencies. In its getter it loads a PyCrypto key from a Cryptography one, and in its setter it does the reverse. Key.__init__ is not part of an interface (no constructors are), so it can deprecate taking a PyCrypto key normally, and just use the aforementioned setter to synthesize the necessary Cryptography key.

twisted.conch.ssh.keys.objectType can similarly perform a local import - or simply look at class names - and emit a deprecation warning as well. Again, not a part of an interface so its behavior can change as part of a normal deprecation cycle.

comment:13 Changed 3 years ago by Adi Roiban

gmpy is very fast. Without gmpy you get 1 or 2 seconds of CPU blocking work in #7672.

Once cryptography is merged I will try to look at gmpy... #8079


pycrypto keyObject has internal private _RSA / _DSA objects but they are exposed as public members :(

I am worried about this scenario... but I hope people are not abusing pycrypto in this way.

keyObject = Crypto.PublicKey.construct(keyData)
key = conch.ssh.Key(keyObject)
keyObject.key.e = 3

On master, the above code will stay in sync... but with a getter/setter based on _cryptographyKeyObject the keyObject will get out of sync with the original keyObject


I will try to deprecate twisted.conch.ssh.keys.objectType (with a local import) in a dedicated ticket #8080


Just to make it clear.

Glyph, you were saying that you job in this branch is done and that you invite others to continue the work started in this branch?

comment:14 Changed 3 years ago by Adi Roiban

Owner: set to Adi Roiban

I start working on the setter/getter part.


I think that changes from twisted/conch/scripts/ckeygen.py can be done in a separate ticket... I have extended the _saveKey tests in #8080 and I can continue to work on this in a separate ticekt

comment:15 Changed 3 years ago by Adi Roiban

I have pushed the setter/getter and I continue to work on the DSA part.

I see that pkcs1Pad, pkcs1Digest, lenSig and ID_SHA1 were removed wihout a deprecation. I s that OK?

comment:16 Changed 3 years ago by Adi Roiban

I see that twisted/persisted/sob.py also depends on PyCrypto for AES, maybe we can start to replace PyCryto in twisted/persisted/sob.py as a way to push PyCA cryptography to our builders.

comment:17 in reply to:  16 Changed 3 years ago by Glyph

Replying to adiroiban:

I see that twisted/persisted/sob.py also depends on PyCrypto for AES, maybe we can start to replace PyCryto in twisted/persisted/sob.py as a way to push PyCA cryptography to our builders.

We need to just remove that functionality. It's not a good way to encrypt stuff. It's using MD5 as a KDF, it's doing its own wacky custom padding with spaces, it's using ECB mode... it basically isn't even encryption at all. And I hope nobody is actually using pickling of application state any more - we should deprecate persistence entirely.

comment:18 Changed 3 years ago by Adi Roiban

Thanks for the feedback. I have create #8081 to handle the sob AES deprecation.

I have also pushed a new commit which fix all errors from tests... but some tests are still failing.

comment:19 in reply to:  18 Changed 3 years ago by Glyph

Replying to adiroiban:

Thanks for the feedback. I have create #8081 to handle the sob AES deprecation.

I have also pushed a new commit which fix all errors from tests... but some tests are still failing.

Well... wait, which is it? Are they failing or aren't they? :)

comment:20 Changed 3 years ago by Adi Roiban

I have pushed a few more changes to this branch.

Some tests should pass once #8080 is merged and after that I can create a separate ticket to migrate the ckeygen.py ...

#8080 should also fix the openssh_compat part.

I think that the keydata.privateRSA_lsh key is invalid and this is why the test fail.

I think that the DSA signature works... but with cryptography we are no longer in control of the k random parameter so the test will always produce a new signature...but that signature can be validated using our own code.

Validation code is run against a pre-computed value... so we should be safe.

I have also split a few multi assert tests in separate test to make it easier to follow what is broken.

These are the test I was focused:

./bin/trial twisted.conch.test.test_keys.KeyTests

bytes_to_int will need padding support to match the PyCrypto function... and bytes_to_int and int_to_bytes will need tests.

Maybe we can add bytes_to_int and int_to_bytes in a separate ticket to reduce the size of this ticket.


There are still test which are failing at RSA signature part

twisted.conch.test.test_userauth.SSHUserAuthServerTests.test_successfulPrivateKeyAuthentication

twisted.conch.test.test_agent.AgentIdentityRequestsTests.test_signDataRSA

twisted.conch.test.test_checkers.SSHPublicKeyCheckerTests.test_usernameReturnedOnSuccess

so even if RSA sign/verify test pass, I think that something is broke there


If you have time, please give it a try.

comment:21 Changed 3 years ago by Adi Roiban

I have pushed the changes which fixes twisted/conch/test/test_ckeygen.py

comment:22 in reply to:  11 Changed 3 years ago by Colin Watson

Replying to glyph:

I think we might actually just try to eliminate the gmpy dependency. Do we have hard evidence that it's faster than Python's built-in arbitrary-precision integers?

The speedup from gmpy is very noticeable indeed in some situations with conch. See for example comments 2 and 7 in #7672.

comment:23 Changed 3 years ago by Adi Roiban

I can confirm the big performance issue which can be observed without gmpy.

It should not be complicated to write some tests to compare int(py3)/long(py2) vs gmpy vs cryptography bindings to BN

I was expecting that since we now depend on the 'cryptography' project we can use its src/_cffi_src/openssl/bignum.py binding instead of stdlib.

We can still keep gmpy as soft dependency.


But I think that first we need dedicated performance tests for SSH. I hope I can find some time to look at writing a few benchmark tests.

As a side note, I have just done a quick and dirty test for transferring 500MB file over SFTP vs HTTPS and SFTP is 20 times slower with AES128.

comment:24 Changed 3 years ago by Adi Roiban

Author: glyphadiroiban, glyph
Branch: branches/modern-cryptography-7413branches/modern-cryptography-7413-2

(In [46468]) Branching to modern-cryptography-7413-2.

comment:25 Changed 3 years ago by Adi Roiban

I have created a new branch and merged the latest ssh key port to py3.

Feel free to continue if you have time :)

comment:26 Changed 3 years ago by Tristan Seligmann

Author: adiroiban, glyphadiroiban, mithrandi, glyph
Branch: branches/modern-cryptography-7413-2branches/modern-cryptography-7413-3

(In [46703]) Branching to modern-cryptography-7413-3.

comment:27 Changed 3 years ago by Tristan Seligmann

Keywords: review added
Owner: Adi Roiban deleted

Putting this up for review; the OS X builder is still red (due to a problem in cryptography 0.9), but https://github.com/twisted-infra/braid/issues/184 should fix this.

comment:28 Changed 3 years ago by Tristan Seligmann

The OS X builder is *still* red, but that is due to #8189; the conch stuff all passes now.

comment:29 Changed 3 years ago by Glyph

Keywords: review removed
Owner: set to Tristan Seligmann

This looks pretty good. I have spotted some minor issues:

  1. Some of the test docstrings sound like they were written by ... non-native english speakers, shall we say. The grammar on "It can serializes RSA key" is mystifying. In addition to the agreement issues, use action verbs, not being verbs: "it serializes", not "it can..." (I'm not sure I like the "It ..." idiom for test docstrings.)
  2. In the Conch README, the project's name is "PyCA's 'Cryptography'", the word "extensions" should not appear. Please capitalize "Cryptography" as it is a proper noun in this context.
  3. The keyObject attribute should actually be deprecated (i.e.: raise a warning, as per ). If adding tests for deprecation is a hassle please feel free to file a separate ticket; no reason that needs to happen in the same change.
  4. There are a number of new twistedchecker errors: please appease it :).
  5. Two occurrences of the typo "PyCryto" in docstrings.
  6. One occurrence of "PyCrytpo" in an error message.
  7. "It provides the compatibility layer for PyCryto during the transition." - transition from what to what?
  8. "An C{Crypto.PublicKey}" - in English, "C" is a consonant, not a vowel :).

These are mostly docstring typos and style issues, so please fix them to your satisfaction and then land.

comment:30 Changed 3 years ago by Tristan Seligmann

Resolution: fixed
Status: newclosed

(In [46746]) Merge modern-cryptography-7413-3: Port Conch from PyCrypto to Cryptography.

Author: adiroiban, mithrandi Reviewer: glyph Fixes: #7413

This should make it much easier and faster to use Conch on PyPy, as well as reducing Twisted's cryptography-related dependency count by one.

Note: See TracTickets for help on using tickets.