Add support for decrypting S/MIME messages #11555
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. |
nitneuqr
force-pushed
the
smime-decryption
branch
2 times, most recently
from
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.
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.
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.
Two ideas I'm thinking about right now:
- Should we allow multiple certs / pkeys as arguments and let
pkcs7_decryptchoose 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
encodingparameter (DER, PEM, SMIME). Are we expectingpkcs7_decryptto figure it out by itself?
There was a problem hiding this comment.
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.
Ah yes, everywhere else in our API we have seperate functions for DER vs. PEM.
There was a problem hiding this comment.
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)
nitneuqr
force-pushed
the
smime-decryption
branch
from
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
PKCS7EnvelopeBuilderand 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