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.

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

support for PKCS7_NO_VERIFY in test_support.pkcs7_verify #12116

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

Conversation

zworkb
Copy link

@zworkb zworkb commented Dec 6, 2024

This PR adds support for Pkcs7Flags::NOVERIFY so that we can verify pkcs7 signatures without verifying the certificates.

M2Crypto also exposes this option but I would prefer to use cryptography ;)

It is needed when the verifying a signed apple wallet pass. Now I can verify it with the call

    test_support.pkcs7_verify(
        serialization.Encoding.DER,
        signature,
        manifest_json.encode("utf-8"),
        [],
        [pkcs7.PKCS7Options.NoVerify],
    )

without the NoVerify option I get the error:

cryptography.exceptions.InternalError: Unknown OpenSSL error. This error is commonly encountered
...
... (error:10800075:PKCS7 routines:PKCS7_verify:certificate verify error:../crypto/pkcs7/pk7_smime.c:295:Verify error: unable to get local issuer certificate)

The unittests as in

test_support.pkcs7_verify(

use ECPrivateKey but when using RSAPrivateKey I get the above error.

@alex
Copy link
Member

alex commented Dec 6, 2024

test_support.pkcs7_verify is not a public API, so we're not going to take PRs to expand its functionality.

If you're interested in helping us design a public API for PKCS#7 verification, that's something we'd be interested in!

@zworkb
Copy link
Author

zworkb commented Dec 7, 2024

Currently I am migrating a project from M2Crypto to cryptography. There is an API for verification of pkcs7: https://github.com/eventbrite/m2crypto/blob/0f61819722c07ecffd7cf47c25095b516bf2d85c/M2Crypto/SMIME.py#L207, which is quite similar to test_support.pkcs7_verify.

I am willing to help, but since I am new to the cryptography project I would need some hints what you are expecting from such a public API that exceeds the complexity of M2Crypto's variant

@alex
Copy link
Member

alex commented Dec 7, 2024

That's great to hear re:migration. It's definitely a use case we want to support.

The big thing we'd be looking for in a public API is: Something that fits in place with the rest of our PKCS#7 APIs, something that is general enough to support many different verification use cases, and something that pushes users towards a secure result.

Internal APIs like test_support are really just the bare minimum to write tests, so they're not a good starting point for figuring out what our public APIs should look like.

#11555 is a recent example of working through the API design to add PKCS#7 decrpytion APIs.

@zworkb
Copy link
Author

zworkb commented Dec 7, 2024

Do you have in mind encapsuling the api in configurable objects as in x509 like
PyServerVerifier/rust and ServerVerifyer/python ?

Additionally I would appreciate some pointers which PKCS#7 verification usecases we should cover for starter.

@nitneuqr
Copy link
Contributor

nitneuqr commented Dec 16, 2024

Do you have in mind encapsuling the api in configurable objects as in x509 like PyServerVerifier/rust and ServerVerifyer/python ?

Additionally I would appreciate some pointers which PKCS#7 verification usecases we should cover for starter.

Hey, I've worked on the new pkcs7_decrypt features the past months.

On my side, my approach was simple: develop something that mimics test_support.pkcs7_decrypt as our own public API, replace the test_support function in our tests, and delete it eventually.

Since decryption is to encryption what verifying is to signing, we should probably adopt the same approach, then improve the function with new features afterwards (maybe PKCS7_NO_VERIFY ?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants