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

@frennkie frennkie commented Oct 25, 2020

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 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 frennkie:add-pkcs7-opt-nocerts branch 2 times, most recently from b4276fa to d457764 Oct 25, 2020
@frennkie frennkie force-pushed the frennkie:add-pkcs7-opt-nocerts branch from d457764 to d47add4 Oct 25, 2020
Copy link
Member

@alex alex left a comment

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

@frennkie frennkie commented Oct 25, 2020

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

@reaperhulk reaperhulk commented Oct 25, 2020

@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

@reaperhulk reaperhulk commented Oct 25, 2020

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 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

@reaperhulk reaperhulk commented Oct 25, 2020

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

@reaperhulk reaperhulk merged commit 611c4a3 into pyca:master Oct 25, 2020
20 checks passed
20 checks passed
Python 2.7 on macOS
Details
Python 3.5 on macOS
Details
Python 3.9 on macOS
Details
Python 2.7 on win32
Details
Python 3.5 on win32
Details
Python 3.6 on win32
Details
Python 3.7 on win32
Details
Python 3.8 on win32
Details
Python 3.9 on win32 Python 3.9 on win32
Details
Python 2.7 on win64
Details
Python 3.5 on win64
Details
Python 3.6 on win64
Details
Python 3.7 on win64
Details
Python 3.8 on win64
Details
Python 3.9 on win64 Python 3.9 on win64
Details
Travis CI - Pull Request Build Passed
Details
codecov/patch 100.00% of diff hit (target 100.00%)
Details
codecov/project 100.00% (target 100.00%)
Details
docs/readthedocs.org:cryptography Read the Docs build succeeded!
Details
pyca/check Summary
Details
@frennkie frennkie deleted the frennkie:add-pkcs7-opt-nocerts branch Oct 25, 2020
@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.
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants