Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add support for verifying S/MIME messages #12267

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Add support for verifying S/MIME messages #12267

wants to merge 12 commits into from
+980 −8

Conversation

nitneuqr
Copy link
Contributor

As promised in #11555, I'm opening this PR with an initial implementation of S/MIME verification, in order to better discuss the API design, and to start the reviews while I finish some other features.

Namely, the new pkcs7_verify functions do not handle the certificate verification feature as of now. It as similar to a openssl smime -verify with the -noverify flag, to verify the signature but not the certificates (similar to what #12116 needs). Can you point me towards some existing code verifying X.509 certificates, if some exists?

Also, I have one question about the certificate parameter in the functions: should we verify against one certificates? Multiple ones? All the ones that are stored in the signature (if any)?

My essential thoughts for testing were to do the round-trip: signature using the PKCS7SignatureBuilder and verifying using the pkcs_decrypt functions. For now, I've not replaced the test_support.pkcs7_verify function, but I'm planning to do so as soon as the certificate verification feature is developed.

I'm still new to rust, so please let me know if you see some issues in variable lifetime, or some unnecessary copying between Python & Rust.

cc @alex

@nitneuqr nitneuqr changed the title Add support for decrypting S/MIME messages Add support for verifying S/MIME messages Jan 10, 2025
@alex
Copy link
Member

alex commented Jan 10, 2025 via email

nitneuqr
nitneuqr commented Jan 10, 2025
View reviewed changes
src/cryptography/hazmat/primitives/serialization/pkcs7.py Outdated
raise ValueError(
"Malformed multipart/signed message: must be multipart"
)
if not isinstance(payload[0], email.message.Message):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, I'm not managing to create any test case (specific MIME message) that would raise this error. However, according to mypy, the case is still possible. Should I go with an assert in order to please mypy or have you got any idea on how the parts could be some text and not an email.message.Message?

@nitneuqr
Copy link
Contributor Author

@alex, for now I'm trying the following code:

def _verify_pkcs7_certificates(certificates: list[x509.Certificate]) -> None:
    builder = PolicyBuilder().store(Store(certificates))
    verifier = builder.build_client_verifier()
    verifier.verify(certificates[0], certificates[1:])

However, I'm getting the following error:

E       cryptography.hazmat.bindings._rust.x509.VerificationError: validation failed: basicConstraints.cA must not be asserted in an EE certificate (encountered processing <Certificate(subject=<Name(C=US,CN=cryptography CA)>, ...)>)

To be honest, I'm not clear as to what we are supposed to do in certificate verification. I've not found anything specific in the RFC, but I'll keep looking. If you can guide me through this, I'd love some help 🛩️

Tarnum-tst
Tarnum-tst reviewed Jan 11, 2025
View reviewed changes
src/rust/src/pkcs7.rs Outdated Show resolved Hide resolved
@alex
Copy link
Member

alex commented Jan 13, 2025

Basically certificates used as end entities (i.e., the cert used to sign a PKCS#7/SMIME message) should not hvae ca=true in their basic constraints extension. I can't remember which spec that comes from, but its a general best practice in any event /cc @woodruffw

@woodruffw
Copy link
Contributor

Yep, that comes from CABF -- we have a test and cite for it here: https://x509-limbo.com/testcases/webpki/#webpkica-as-leaf.

(RFC 5280 doesn't impose the requirement that the EE not be a CA, but firmly agreed about it being best practice.)

@nitneuqr
Copy link
Contributor Author

Basically certificates used as end entities (i.e., the cert used to sign a PKCS#7/SMIME message) should not hvae ca=true in their basic constraints extension. I can't remember which spec that comes from, but its a general best practice in any event /cc @woodruffw

OK, that raises two things for me:

  • That means we will have to create a specific certificate for verifying (and signing ?) PKCS#7 files, either statically or dynamically. I'll look into it.
  • This raises the question of test_support.pkcs7_verify function. It does not check this, as we can see in test_support.rs:93, by just passing an empty stack of certificates.
let certs = openssl::stack::Stack::new()?;

p7.verify(
    &certs,
    &store,
    msg.as_ref().map(|m| m.as_bytes()),
    None,
    flags,
)?;

@nitneuqr
feat: functions now have optional keyword arguments
1e92f20 
certificate is now optional

feat: handling RSA case

feat: No signature parameter

adapted tests accordingly
@nitneuqr
fix: adapted docmentation
db95cf5 
fix: passed into Cow again

coverage: added one test case
@nitneuqr
one more test case
01ab390
@nitneuqr
changing the error message for clarity
ecd6da3
@nitneuqr
two more test cases handled
3649c38
@nitneuqr
loading the load_der func for all backends
fb78777
@nitneuqr
Copy link
Contributor Author

@alex this should be ready for review, the tests are passing. A few considerations to keep in mind:

  • What should the priority be for user-input parameters vs. what's found in the PKCS7 structure (content, certificate)?
  • Handling PKCS#7 message with multiple signers: trying to verify the first signer? All of them?
  • Missing the documentation for the new pkcs#7 test vectors; still WIP.

Keep in mind that for now, I've used a certificate chain from endesive to make the tests pass, but I'll work on creating new cryptography certificates in the following days.

Some more things about the certs:

  • the certificate used for testing PKCS#7 signing cannot be verified properly (due to some EE error above)
  • Verifying the signed messages, when using openssl, only works when some other certificates are passed in the -CAfile parameter. Otherwise we are getting a unable to get local issuer certificate error. This is not necessary (for some reason that I do not understand) in the new functions.

CC @reaperhulk @facutuesca if you miraculously have time for a review 😄

@nitneuqr
Copy link
Contributor Author

Stumbled on #12104 and realized we might have the same issue with the certificate verification code!

@nitneuqr nitneuqr requested a review from alex January 20, 2025 15:12
@alex
Copy link
Member

alex commented Jan 20, 2025 via email

@nitneuqr
Copy link
Contributor Author

No worries! Just checking in in case you forgot about it 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tarnum-tst Tarnum-tst Tarnum-tst approved these changes

@alex alex Awaiting requested review from alex

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nitneuqr @alex @woodruffw @Tarnum-tst