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 multiple signatures #5

Open
wants to merge 3 commits into
base: signature-feature
Choose a base branch
from

Conversation

packdat
Copy link

@packdat packdat commented Jul 20, 2024

PR as requested.

Adds support for incremental updates and multiple signatures.

Please consider these changes as highly experimental !


private int GetContentLength()
{
return signer.GetSignedCms(new MemoryStream(new byte[] { 0 }), document).Length + 10;
Copy link

Choose a reason for hiding this comment

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

This method causes the encryption to be called twice, I think that is not good solution, especially if the encryption requires external actions then those are called twice...

Copy link

Choose a reason for hiding this comment

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

in lopdf this was handled by creating placeholder values which would be changed later: https://github.com/ralpha/pdf_signing/blob/master/src/signature_info.rs#L117-L124

Copy link
Author

Choose a reason for hiding this comment

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

The content length is used to reserve space in the PDF, where the real signature is stored in a later step.
The current approach was used to get a fairly safe estimate of the length, without the risk of reserving too few or too much space.
You could certainly skip this step, but then you have to reserve space for the largest possible content length of any signer that may be used.
Another approach may be to let the user provide a value (e.g. an EstimatedContentLength - property in the signature-options)

Copy link

Choose a reason for hiding this comment

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

I at least recommend putting it behind an interface so it can be overriden in the client application. In my use case the digital signature is entered by external card reader and the user needs to enter PIN to the card reader software, asking the PIN twice would be real bad, much worse than causing the pdf size to grow a bit more than necessary

Copy link
Author

Choose a reason for hiding this comment

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

@Havunen Yeah, that's one use case i didn't had in mind when creating the PR, my bad.
Would be nice if we would know more about how users might apply signatures; when knowing the use cases, we could design better interface to handle these...

inputStream = documentStream;
// apply signature as an incremental update
document = PdfReader.Open(documentStream, PdfDocumentOpenMode.Append);
signer = new DefaultSigner(signatureOptions);
Copy link

Choose a reason for hiding this comment

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

Is there some particular reason why ISigner cannot be given in constructor? Now it always uses DefaultSIgner?

Copy link
Author

Choose a reason for hiding this comment

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

As stated in the PR, these changes are experimental.
There's no real reason other than that it was the fastest way to come up with a working PoC.
Feel free to adapt to your needs.

var content = Enumerable.Repeat<byte>(0, contentLength).ToArray();
signatureDict.Contents = content;
signatureDict.Filter = "/Adobe.PPKLite";
signatureDict.SubFilter = "/adbe.pkcs7.detached";
Copy link

Choose a reason for hiding this comment

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

For my implementation I had to move these filter types public, because I had to change the subfilter to "ETSI.CAdES.detached"

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

Successfully merging this pull request may close these issues.

2 participants