-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for decrypting S/MIME messages #11555
base: main
Are you sure you want to change the base?
Conversation
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. |
cb082ce
to
e1c4620
Compare
Rebased to main after integration of #11556. Should I split it again in smaller PRs? Maybe around |
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. |
@@ -263,6 +263,116 @@ def encrypt( | |||
return rust_pkcs7.encrypt_and_serialize(self, encoding, options) | |||
|
|||
|
|||
class PKCS7EnvelopeDecryptor: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I wonder if we need a builder for this case, is pkcs7_decrypt(data, recipient_cert, recipient_key, options)
sufficient, or might we want other things?
cc: @reaperhulk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH on my side I was more focused on developing code that worked, so any API design is fine to me. I'd probably go for the unique function as it's the simplest one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two ideas I'm thinking about right now:
- Should we allow multiple certs / pkeys as arguments and let
pkcs7_decrypt
choose the (or a) certificate that works? Outlook does that for instance (I've got multiple certificates associated with different mailboxes). - In your case, you're missing the
encoding
parameter (DER, PEM, SMIME). Are we expectingpkcs7_decrypt
to figure it out by itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd bias to the function as well. I would not want the function to guess though -- we should either have a param or 3 separate functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, everywhere else in our API we have seperate functions for DER vs. PEM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just pushed first implementation of pkcs7_decrypt_{encoding}
😄
@facutuesca FYI, if you're interested |
first round-trip tests feat: made asn1 structures readable refacto: adapted existing functions accordingly feat/pkcs12: added symmetric_decrypt feat: deserialize 3 possible encodings feat: handling AES-128 & AES-256 CBC feat: raise error when no recipient is found feat/pkcs7: added decanonicalize function feat/asn1: added decode_der_data feat/pkcs7: added smime_enveloped_decode tests are the round-trip (encrypt & decrypt)
d4e7741
to
69d2e23
Compare
added algorithm to pkcs7_encrypt signature refacto: decrypt function is clearer flow is more natural refacto: added all rust error tests refacto: added another CA chain for checking
refacto: removed SMIME_ENVELOPED_DECODE from rust code refacto: removed decode_der_data adapted tests accordingly
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 thePKCS7EnvelopeDecryptor
(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