pyca
Add support for decrypting S/MIME messages
As discussed on #6263, I'm opening this PR with an initial implementation of S/MIME decryption, in order to better discuss the API design, the algorithms we want to support, and how we want to approach testing.
My essential thoughts for testing were to do the round-trip: encryption using the PKCS7EnvelopeBuilder and decryption using the PKCS7EnvelopeDecryptor (or whatever its name will be). No tests were made against openssl, even though it might interesting.
For the Python API, I've tried to stick as much as possible with the current API design (PKCS7SignatureBuilder, PKCS7EnvelopeBuilder).
I'm new to rust, so please let me know if you see some issues in variable lifetime, or some unnecessary copying between Python & Rust.
During development, I've realized that we could migrate PKCS7 unpadding to rust instead of using types, so I did migrate it. I'll probably open another smaller PR for that matter (if that makes sense?). I still have some code using PKCS7 Unpadding on Python if needed.
cc @alex
Thanks for working on this! I haven't had a chance to look in depth, but re: PKCS7Unpadder, it'd be great to have that in a stand-alone PR.
Thanks for working on this! I haven't had a chance to look in depth, but re: PKCS7Unpadder, it'd be great to have that in a stand-alone PR.
Sure! Opened #11556 for that matter.
Rebased to main after integration of #11556. Should I split it again in smaller PRs? Maybe around symmetric_decrypt or other subfunctions in the code.
We're always happy to have smaller PRs split out (though it can sometimes be complicated due to coverage). I'm hoping to have time to review this today, though it might be tomorrow.
@alex @reaperhulk took into account the first comments and adapted the tests / coverage accordingly.
Do you have any time for another review? Do you need anything else from me before reviewing? 😊
(Thanks for updating, I'm going to pause on reviewing until https://github.com/pyca/cryptography/pull/11843 is merged, hopefully that'll be quick.)
#11843 is merged, I've rebased the branch onto main to take the new changes into account. It should b ready to review again!
Great, will try to get to it this evening. Thanks!
On Wed, Nov 6, 2024 at 3:37 AM Quentin Retourne @.***> wrote:
#11843 is merged, I've rebased the branch onto main to take the new changes into account. It should b ready to review again!
— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>
-- All that is necessary for evil to succeed is for good people to do nothing.
Hey, we're getting closer now, but this conversation just raised new issue:
https://github.com/pyca/cryptography/pull/11555#discussion_r1845593139
Using the openssl executable, a round-trip (encryption + decryption) seems to transform Hello, world!\n into Hello, world!\r\r\n. This issue did not seem to happen in the old test_support.pkcs7_decrypt function (using openssl on Rust): the round-trip seemed to work fine. Is it correct that we should stick to this behavior instead of the other one?
In any cases, I'm going to add more tests cases when some data is encrypted with some options, and decrypted with some others.
Are you sure the old version round tripped in that way? If you look at the tests, it looks to me like this PR changes them -- they used to have things like:
# New lines are canonicalized to '\r\n' when not using Binary
expected_data = (
data
if pkcs7.PKCS7Options.Binary in options
else data.replace(b"\n", b"\r\n")
)
assert decrypted_bytes == expected_data
and now they have
assert decrypted_bytes == data
Are you sure the old version round tripped in that way? If you look at the tests, it looks to me like this PR changes them -- they used to have things like:
# New lines are canonicalized to '\r\n' when not using Binary expected_data = ( data if pkcs7.PKCS7Options.Binary in options else data.replace(b"\n", b"\r\n") ) assert decrypted_bytes == expected_data
That's correct. I'll check this out. It might be yet another way, as it seems only one "\r" is added.
That's correct. I'll check this out. It might be yet another way, as it seems only one "\r" is added.
Should be good now. decanonicalize function was way too complicated, -binary option does not seem to be handled by openssl smime -decrypt. I should have compared with test_support function way sooner, and should not have changed the previous tests assertions 😞 I'll move what's possible to pkcs7.rs and let you know when you can do a (hopefully!) final review.
@alex @reaperhulk took most comments into account, still have some minor questions but would love a (hopefully!) final review.
Besides these unwrap comments and @reaperhulk's on text, I think this is ready. Thanks so much for your continued work on it.
Thanks for your reviews! Should be good now, header removal is now tested in test_pkcs7_decrypt_der_text_handmade_header, and unwraps have been removed.
One final nit + Paul's comments and we can merge!
Should be done now! Merci beaucoup! Also added some documentation 🛩️
Yes, thank you for sticking with this and being so responsive. It's a pleasure having contributors like you!
My pleasure! I might come back in a few days / weeks to tackle some more problems (namely, pkcs verify and encrypt/decrypt with other content encryption algorithms)