3031 replace pycryptopp #616
Conversation
|
Hi @heartsucker just got back from the Summit and haven't taken an in-depth look yet. I think cryptography is a great choice if it looks to cover the primitives we need! I do, however, feel like we've been lately moving towards more "factory functions" instead of "do complex things in constructors" -- so this would point to keeping something like |
Codecov Report
@@ Coverage Diff @@
## master #616 +/- ##
=========================================
Coverage ? 84.97%
=========================================
Files ? 162
Lines ? 28852
Branches ? 4107
=========================================
Hits ? 24516
Misses ? 3629
Partials ? 707
Continue to review full report at Codecov.
|
Yeah, we could do this too. Part of the reason I wrapped them was to keep existing APIs and provide helpers to all the crypto code was in one place. I really don't like having crypto code scattered all around the codebase because it makes it harder to say "no really don't do this" with something like key formatting. But I can make changes around that after I can even get this PR to work in the first place. 🙃 |
There was a problem hiding this comment.
Overall, looks pretty straightforward to use cryptography -- yay!
A lot of newer Tahoe code has been preferring to move to "factory functions" instead of doing complex work in object constructors. This would point to keeping the make_keypair (or similar) function that returns some objects (or, if the API was kept the same, returns some encoded keys and different helper functions create required objects .. like parse_public_key() / parse_private_key() or similar).
I see that the main "wrapper" classes are mostly to keep the API similar to pycryptopp..? If we do keep them, I'd at least prefer to get rid of @classmethods (and use bare helpers instead) and they all need to be new-style classes and have docstrings for everything.
However, consider: if I'm reading code that's using the AES class, it gives me .process() and that's it. I then have to figure out what self._encryptor is, find out it's a Cipher, and look up that doc instead. Consider, instead, an approach like:
def create_aes_encryptor(key, iv=None):
"""
returns an object capable of doing AES encryption for Tahoe-LAFS. This
is CTR mode with an optional IV (16 bytes of zeros by default).
""""
# do all the checks that are in AES's constructor here, instead
def encrypt_aes_bytes(aes_encryptor, plaintext):
"""
Using an encryption object created by `create_aes_encryptor` this will
return bytes that are an encrypted version of the `plaintext` bytes
"""
# confirm aes_encryptor is a correct object
return aes_encryptor.update(plaintext)
This way, it doesn't matter what kind of object create_aes_encryptor returns: users of
the code will always be doing things via function calls from this encryption helper module. The object
returned from the factory-function could be a straight Cryptography object or it could be an instance of class AES(object): which just promises to have a .process(). Or it could be an internal-only wrapper
and the "only" way to get work done is via encrypt_aes_bytes().
I don't necessarily see a benefit to sticking with the pycryptopp interface -- mostly because process() doesn't tell me anything when reading the code. It's used in relatively few places, and dropping in encrypt_aes_bytes(enc, chunk) vs. .process() is pretty easy to review.
|
Looking at this. Takes a while. |
|
re @heartsucker
I think in this case there's approximately the same amount of "crypto code scattered" because there are wrapper-objects that get used in this PR as it stands -- and so the only change would be "use the cryptography-provided classes" versus "use our own custom wrappers that just call through anyway". The motivation to use the cryptography-provided ones is that they're documented and better-known than a custom wrapper. |
|
I have switched the RSA interfaces over to a functional-style one; the ed25519 stuff needs a similar treatment and some of the other fixups in the review have not been accomplished yet. |
|
Can someone kick the circle-ci/integration builder to see if that was a transient error? (Running |
|
@heartsucker can you resolve the merge conflicts? |
|
@tpltnt I don't think heartsucker is working on this any longer (but, I will rebase it) |
|
Okay, I re-based this ... but then I thought, "what is GitHub going to do if I force-push this?" so I have started a new PR in case github freaks out and "does bad stuff" to all the comments and discussion in here ... |
|
Okay, on this branch I did " |
|
Okay, I believe I have addressed all comments. When CI goes green, should be good for merge -- if anyone else would like to review, that would be awesome! Please leave a comment if you are so I don't merge it ... |
Fixes: ticket #3031
Ticket: https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3031
This change is