Skip to content

Conversation

posita
Copy link
Contributor

@posita posita commented May 3, 2015

Fix #30. Replace ctypes with cryptography (cffi).

% python2.7 -m unittest discover && python3.4 -m unittest discover
..................................................................................................
----------------------------------------------------------------------
Ran 98 tests in 0.565s

OK
..................................................................................................
----------------------------------------------------------------------
Ran 98 tests in 0.502s

OK

There is currently one known issue: CECKey.set_privkey and CECKey.get_privkey require the FFI instance to expose the d2i_ECPrivateKey and i2d_ECPrivateKey functions, respectively, from the underlying OpenSSL library. Currently, cryptography does not expose these functions. This means that those functions will not work. I have not yet investigated or considered using a hybrid ctypes approach for that narrow set of functionality (if that is even possible).

This only affects the *_privkey getter and setter. I was able to provide alternatives to the o2i_ECPublicKey and i2o_ECPublicKey functions used in the corresponding *_pubkey functions (see this and this). I have not yet found (or am not clever enough to recognize) equivalents on the private key side.

The reason why unit tests still pass is because the *_privkey getter and setter are not called from any of the tests. This is unchanged from before (I did not add tests to cover those methods).

@posita posita mentioned this pull request May 3, 2015
@posita posita changed the title NOT READY YET - Fix #30. Replace ctypes with cffi/cryptography. Fix #30. Replace ctypes with cryptography (cffi). May 3, 2015
@posita posita changed the title Fix #30. Replace ctypes with cryptography (cffi). NEEDS REVIEW - Fix #30. Replace ctypes with cryptography (cffi). May 3, 2015
@posita posita changed the title NEEDS REVIEW - Fix #30. Replace ctypes with cryptography (cffi). NEEDS REVIEW - Fix #30. Replace ctypes with cryptography (cffi). May 3, 2015
@posita posita mentioned this pull request May 3, 2015
@posita
Copy link
Contributor Author

posita commented May 17, 2015

@petertodd, can you tell me if this is worth pursuing? Please close if not.

Other than this, I do not have any suggestions for addressing #30.

@petertodd
Copy link
Owner

Sorry, I'm off in the states travelling and haven't had a chance to look at this in detail. :( Give me another few days.

@posita
Copy link
Contributor Author

posita commented May 22, 2015

@petertodd, np. Sorry for nagging! 😊

I filed pyca/cryptography#1969 to request that d2i_ECPrivateKey and i2d_ECPrivateKey be added to cryptography. Apparently there's a workaround that I wasn't aware of, but it looks complicated. If this approach has legs, I'll investigate further. Otherwise, @HelloZeroNet has proposed an alternative, but it involves wrapping OpenSSL.

@petertodd
Copy link
Owner

Hmm, I'm pretty reluctant to move to https://github.com/pyca/cryptography, basically because it gets us further away from doing exactly what Bitcoin Core does by adding another layer/dependency. I'm inclined to wait for libsecp256k1 before doing anything wholesale. Equally, to fix #30 I'd rather see a more targeted patch.

What can I say, crypto code is dangerous and hard to review thoroughly, and I know people are using python-bitcoinlib in production, so I'm inclined to tread very carefully with regard to changing anything.

@posita
Copy link
Contributor Author

posita commented May 24, 2015

Totally understood. Closing as "won't merge". See also #77.

@posita posita closed this May 24, 2015
@posita posita deleted the use-pyca-cryptography-for-bignum-ops-30 branch May 24, 2015 03:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unit tests are segfaulting

2 participants