unidoc / unipdf

Golang PDF library for creating and processing PDF files (pure go)
https://unidoc.io
Other
2.48k stars 250 forks source link

[BUG] model.SignatureHandler is called twice, on initialize and on write #417

Closed Kunamatata closed 3 years ago

Kunamatata commented 3 years ago

Description

The model.SignatureHandler returned from sighandler.NewAdobeX509RSASHA1Custom is called twice when applying a digital signature to a PDF.

It is called the first time on signature.Initialize() signature being of type *model.PdfSignature It is called the second time on appender.Write(...)appenderbeing of type*model.PdfAppender`

Expected Behavior

The handler shouldn't be called twice. This results in calling the SaaS (e.g: KMS in our case) twice for 1 signature

Actual Behavior

Steps to reproduce the behavior:

  1. Use https://github.com/unidoc/unipdf-examples/blob/5f8b246aa4aa4b5ee6feec2124ab4351dccd2fac/signatures/pdf_sign_hsm_pkcs11.go from the unipdf-examples
  2. The handler returned from this line: https://github.com/unidoc/unipdf-examples/blob/5f8b246aa4aa4b5ee6feec2124ab4351dccd2fac/signatures/pdf_sign_hsm_pkcs11.go#L226 will be called twice.

Attachments

Trace Attachment from pprof:

image

Zoomed in version of pprof:

image
gunnsth commented 3 years ago

@Kunamatata The extra call is made to get an initial estimate for the signature size. However, as you point out, this is not desirable when working with an external API.

In UniPDF v3.12.0 we added an option for sighandler.NewDocTimeStampWithOpts to skip this extra call by specifying a size parameter for the signature. Typically fine to make this quite a bit bigger than needed. Few extra bytes dont matter. See the release notes for an example of how to use this: https://github.com/unidoc/unipdf/releases/tag/v3.12.0

Basically specifying it like this will avoid the extra call:

sigLen := 6000 // Reserve 6000 bytes for signature.
handler, err := sighandler.NewDocTimeStampWithOpts(
        testTimestampServerURL,
        crypto.SHA512,
        &sighandler.DocTimeStampOpts{SignatureSize: sigLen},
    )

It would make sense to create this kind of option for other signature handlers, such as the one you are using here (sighandler.NewAdobeX509RSASHA1Custom).

I recommend that you log into our Customer Service Desk and create a ticket there and we will get on this.

gunnsth commented 3 years ago

@Kunamatata We have added a comparable feature in v3.14.0 for NewAdobeX509RSASHA1Custom which enables estimating the signature size and avoiding the extra call.

// AdobeX509RSASHA1Opts defines options for configuring the adbe.x509.rsa_sha1
// signature handler.
type AdobeX509RSASHA1Opts struct {
    // EstimateSize specifies whether the size of the signature contents
    // should be estimated based on the modulus size of the public key
    // extracted from the signing certificate. If set to false, a mock Sign
    // call is made in order to estimate the size of the signature contents.
    EstimateSize bool
}

So to use it, set EstimateSize to true, and pass your options to NewAdobeX509RSASHA1CustomWithOpts

Kunamatata commented 3 years ago

@gunnsth Thank you so much for this enhancement. I'm attaching some pprof trace images for good measure.

Before the enhancement, call to KMS in AWS: image

After the enhancement: image

gunnsth commented 3 years ago

Thanks