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

PKCS7SignatureBuilder now supports new option NoCerts when signing #5500

Merged
merged 1 commit into from Oct 25, 2020

Conversation

frennkie
Copy link
Contributor

This adds the possibility to add the PKCS7_NOCERTS flag/option (as per PKCS7_sign_add_signer) in order to exclude the signer's certificate.

If PKCS7_NOCERTS is set the signer's certificate will not be included in the PKCS7 structure, the signer's 
certificate must still be supplied in the signcert parameter though. This can reduce the size of the signature 
if the signers certificate can be obtained by other means: for example a previously signed message.

@frennkie
Copy link
Contributor Author

frennkie commented Oct 25, 2020

@reaperhulk I only now saw #5498 and the way you are checking for the presence/amount of the certs:

        assert (
            sig.count(rsa_cert.public_bytes(serialization.Encoding.DER)) == 1
        )

Should I change this PR to use the same method or is my approach acceptable too?

UPDATE: I have changed it.

@frennkie frennkie force-pushed the add-pkcs7-opt-nocerts branch 2 times, most recently from b4276fa to d457764 Compare October 25, 2020 10:30
Copy link
Member

@alex alex left a comment

Choose a reason for hiding this comment

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

Strictly speaking, this is something that can vary on a per-signer basis. Should this flag be passed to add_signer?

@frennkie
Copy link
Contributor Author

Strictly speaking, this is something that can vary on a per-signer basis.

Correct.

Should this flag be passed to add_signer?

I would leave it up to you to decide this.

It's the same approach with PKCS7_NOSMIMECAP and PKCS7_NOATTR for which it appears a deliberate decision was made to allow this setting only once for all signers.

As #5498 is now merged it would still be possible to e.g. set the PKCS7_NOCERTS for all signers, but then selectively adding the certificate of one particular signer.

@reaperhulk
Copy link
Member

@frennkie we're definitely interested in your opinion on Alex's question. As you no doubt know, since you followed the current implementation exactly, we treat a few per-signer flags as globals right now. We can continue to do this or we can expose it per-signer instead.

My personal inclination is to leave this global and add a per-signer form later if we get requests for it. This form would then be the shorthand for "add the flag to all signers".

@reaperhulk
Copy link
Member

Haha, I see we replied simultaneously. Definitely curious if you see a use case right now for it to be per-signer though.

@frennkie
Copy link
Contributor Author

frennkie commented Oct 25, 2020

I absolutely see no use case for per-signer! The reason for this is that my (currently only) use for this is the sending of S/MIME e-mails. And in this scenario I don't see how/why two different signers would sign the same message.

This would be different for e.g. subsequently signing a PDF document by (several) different people.

As outlined above.. the current API covers the "normal use case" by default - and still leaves room for special/edge cases.

@reaperhulk
Copy link
Member

Good enough for me! Thanks for working on this @frennkie

@reaperhulk reaperhulk merged commit 611c4a3 into pyca:master Oct 25, 2020
@frennkie frennkie deleted the add-pkcs7-opt-nocerts branch October 25, 2020 15:07
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants