WIP: Initial digital signature support (v3) #334

gunnsth · 2019-01-25T17:08:44Z

Expands on #280 and addresses a few issues pointed out there.

…5i-pdf-sign

…res. Whitespace was causing ByteRange to not cover the entire document. Ideally should be everything outside the < > in the signature Contents field.

…ers.

Using updated pkcs7 package updated by mozilla

…riting to a buffer.

codecov · 2019-01-25T17:08:56Z

Codecov Report

Merging #334 into v3 will increase coverage by 0.64%.
The diff coverage is 71.2%.

@@            Coverage Diff            @@
##              v3     #334      +/-   ##
=========================================
+ Coverage   59.6%   60.24%   +0.64%     
=========================================
  Files        149      152       +3     
  Lines      26742    27169     +427     
=========================================
+ Hits       15940    16369     +429     
- Misses     10291    10408     +117     
+ Partials     511      392     -119

Impacted Files	Coverage Δ
pdf/model/form.go	`60.34% <100%> (-0.13%)`	⬇️
pdf/core/primitives.go	`78.85% <100%> (+0.72%)`	⬆️
pdf/model/writer.go	`82.92% <100%> (+0.36%)`	⬆️
pdf/model/sighandler/sighandler_rsa_sha1.go	`7.31% <5.4%> (ø)`
pdf/model/sighandler/sighandler_pkcs7.go	`66.66% <66.66%> (ø)`
pdf/model/signature_handler.go	`68.8% <68.8%> (ø)`
pdf/model/structures.go	`80% <80%> (ø)`	⬆️
pdf/model/fields.go	`54.94% <88%> (+1.33%)`	⬆️
pdf/model/signature.go	`87.7% <88.75%> (+32.14%)`	⬆️
pdf/model/appender.go	`86.63% <93.98%> (+7.68%)`	⬆️
... and 49 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65e8637...0fa9092. Read the comment docs.

adrg

The code and the logic look good to me. I left a couple of suggestions, minor changes.

pdf/model/appender.go

adrg · 2019-02-07T19:10:57Z

pdf/model/appender.go

 	}
+	a.written = true


Should the written field be set at the end of the method, after a successful write?

adrg · 2019-02-07T19:42:37Z

pdf/model/signature.go

@@ -90,8 +174,15 @@ func (sig *PdfSignature) ToPdfObject() core.PdfObject {
 	if sig.ContactInfo != nil {
 		dict.Set("ContactInfo", sig.ContactInfo)
 	}
-
-	// FIXME: ByteRange and Contents need to be updated dynamically.
+	if sig.ByteRange != nil {


Maybe use:

dict.SetIfNotNil("ByteRange", sig.ByteRange) dict.SetIfNotNil("Contents", sig.Contents)

adrg · 2019-02-07T19:43:46Z

pdf/model/signature.go

+	container := pss.container
+	dict := container.PdfObject.(*core.PdfObjectDictionary)
+
+	if pss.Ff != nil {


dict.SetIfNotNil can be used for all the fields that may be nil

adrg · 2019-02-07T19:44:16Z

pdf/model/signature.go

+func (pcs *PdfCertificateSeed) ToPdfObject() core.PdfObject {
+	container := pcs.container
+	dict := container.PdfObject.(*core.PdfObjectDictionary)
+	if pcs.Ff != nil {


dict.SetIfNotNil can be used for all the fields that may be nil.

adrg · 2019-02-07T19:50:49Z

pdf/model/signature_handler.go

+				if err != nil {
+					return nil, err
+				}
+				pairs = append(pairs, &sigFieldPair{sig: sig, field: f})


Finding the applicable handler could be done here in order to avoid extra iterations:

pair := &sigFieldPair{sig: sig, field: f} for _, handler := range handlers { if handler.IsApplicable(pair.sig) { pair.handler = handler break } } pairs = append(pairs, pair)

adrg · 2019-02-07T19:58:02Z

pdf/model/signature.go

@@ -201,6 +291,9 @@ func (sf *PdfSignatureField) ToPdfObject() core.PdfObject {
 	if sf.SV != nil {
 		dict.Set("SV", sf.SV)
 	}
+	if sf.Kids != nil {


dict.SetIfNotNil can be used for all the fields that may be nil

Signature support refactoring

gunnsth · 2019-02-12T23:53:56Z

pdf/model/appearance.go

+
+// PdfSignatureAppearance defines a signature with a specified form field and
+// annotation widget for appearance styling.
+type PdfSignatureAppearance struct {


@adrg Can you check whether you feel that this type is a duplicate of PdfFieldSignature in fields.go?

unidoc/pdf/model/fields.go

Line 434 in 2ef5b5a

type PdfFieldSignature struct {

Technically this one is more high level for creating signatures, so I suppose it's OK, although it could be confusing.

@gunnsth I do find it very similar to PdfFieldSignature. The only difference I can see is that PdfSignatureAppearance is also a widget. If that's the case, the PdfSignatureAppearance struct could be merged into PdfFieldSignature.

Can you please review my branch v3-a5i-pdf-sign? I merged the two structures there.
https://github.com/adrg/unidoc/tree/v3-a5i-pdf-sign

Digital signatures refactoring

gunnsth

A few minor changes needed.

gunnsth · 2019-02-18T22:05:57Z

pdf/model/appender.go

@@ -370,19 +379,72 @@ func (a *PdfAppender) ReplacePage(pageNum int, page *PdfPage) {
 	}
 }

-// ReplaceAcroForm replaces the acrobat form. It appends a new form to the Pdf which replaces the original acrobat form.
+// Sign signs a specific page with a digital signature using a specified signature handler.
+// Returns a PdfFieldSignature that can be used to customize the signature appearance.


Inaccurate "Returns a PdfFieldSignature..."

Should be:
The signature field specified must have a valid signature dictionary specified by its V field.

gunnsth · 2019-02-18T22:07:58Z

pdf/model/appender.go

 func (a *PdfAppender) ReplaceAcroForm(acroForm *PdfAcroForm) {
 	a.acroForm = acroForm
 }

 // Write writes the Appender output to io.Writer.
+// It can only be called once and further invokations will result in an error.


invocations

gunnsth · 2019-02-18T22:08:43Z

pdf/model/appender.go

@@ -132,7 +141,7 @@ func (a *PdfAppender) addNewObjects(obj core.PdfObject) {
 	switch v := obj.(type) {
 	case *core.PdfIndirectObject:
 		// Check the object is changing.
-		// If the indirect object has not the readonly parser then the object is changed.
+		// If the indirect object has not the read-only parser then the object is changed.


If the indirect object does not have the read-only parser, then the object has changed.

gunnsth · 2019-02-18T22:12:24Z

pdf/model/sighandler/sighandler_rsa_sha1.go

+	handler := *a
+	sig.Handler = &handler
+	sig.Filter = core.MakeName("Adobe.PPKLite")
+	//sig.Filter = core.MakeName("Adobe.PPKMS")


remove commented code

gunnsth · 2019-02-18T22:12:35Z

pdf/model/sighandler/sighandler_rsa_sha1.go

+	/*
+		switch sa {
+		case x509.SHA1WithRSA:
+			return crypto.SHA1, true


remove commented code

gunnsth · 2019-02-18T22:19:34Z

pdf/model/appender.go

+	if hasSigDict {
+		// For signatures, we need to write twice. First to find the byte offset of the Contents and then
+		// dynamically update file with the signature and ByteRange.
+		// Thus we create an empty buffer to write to and then at the e


Incomplete sentence

Minor code documentation improvements

a5i and others added 14 commits December 30, 2018 01:47

add digital sign API prototype

61e2022

add digital sign API prototype

7b66633

add digital sign API prototype

8efc55b

add digital sign API prototype

59c83c5

Merge branch 'v3-pdf-sign' of https://github.com/a5i/unidoc into v3-a…

27304fc

…5i-pdf-sign

Moved signature handler implementation logic to subpackage sighandlers.

11e11f4

Add doc.go to sighandlers

7bd43b1

Removed large whitespace in signature dictionary to get valid signatu…

649dbf0

…res. Whitespace was causing ByteRange to not cover the entire document. Ideally should be everything outside the < > in the signature Contents field.

Cleanup and adding comments. Added check to avoid panics on nil point…

37ea366

…ers.

Add function to convert PDF time to go time.Time

cad0c0e

Update pkcs7 dependency and use own fork

bfdab99

Using updated pkcs7 package updated by mozilla

Improve documentation in appender.go

54a3217

Return an error if try to invoke Appender Write twice. Fix bug when w…

7ad1184

…riting to a buffer.

Fix

094d203

gunnsth mentioned this pull request Jan 25, 2019

WIP: add digital sign API prototype #280

Closed

Merge branch 'v3' into v3-a5i-pdf-sign

60a7d67

gunnsth requested a review from adrg February 7, 2019 10:05

gunnsth added this to the v3.0.0-alpha.3 milestone Feb 7, 2019

adrg reviewed Feb 7, 2019

View reviewed changes

gunnsth and others added 6 commits February 12, 2019 14:19

Merge branch 'v3' into v3-a5i-pdf-sign

e7f5b37

Refactor signature code

1a92cec

Further refactoring

3233ce9

Rename PdfAppearance to PdfSignatureAppearance

cda8553

Merge pull request #343 from adrg/signature-support

4d25f7f

Signature support refactoring

Update comments

2ef5b5a

gunnsth commented Feb 12, 2019

View reviewed changes

adrg and others added 3 commits February 13, 2019 20:28

Combine PdfFieldSignature with PdfSignatureAppearance

c2d1efb

Change the prototype of the appender Sign method

28ad01f

Merge pull request #350 from adrg/v3-a5i-pdf-sign

473eb5f

Digital signatures refactoring

Merge branch 'v3' into v3-a5i-pdf-sign

e93ffb2

gunnsth commented Feb 18, 2019

View reviewed changes

adrg and others added 2 commits February 19, 2019 17:59

Minor code documentation improvements

38d71b4

Merge pull request #352 from adrg/v3-a5i-pdf-sign

0fa9092

Minor code documentation improvements

gunnsth merged commit c8b70ef into v3 Feb 19, 2019

gunnsth deleted the v3-a5i-pdf-sign branch February 19, 2019 17:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Initial digital signature support (v3) #334

WIP: Initial digital signature support (v3) #334

gunnsth commented Jan 25, 2019

codecov bot commented Jan 25, 2019 •

edited

adrg left a comment

adrg Feb 7, 2019

adrg Feb 7, 2019

adrg Feb 7, 2019

adrg Feb 7, 2019

adrg Feb 7, 2019

adrg Feb 7, 2019

gunnsth Feb 12, 2019

adrg Feb 13, 2019 •

edited

gunnsth left a comment

gunnsth Feb 18, 2019

gunnsth Feb 18, 2019

gunnsth Feb 18, 2019

gunnsth Feb 18, 2019

gunnsth Feb 18, 2019

gunnsth Feb 18, 2019

gunnsth Feb 18, 2019

WIP: Initial digital signature support (v3) #334

WIP: Initial digital signature support (v3) #334

Conversation

gunnsth commented Jan 25, 2019

codecov bot commented Jan 25, 2019 • edited

Codecov Report

adrg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrg Feb 13, 2019 • edited

Choose a reason for hiding this comment

gunnsth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 25, 2019 •

edited

adrg Feb 13, 2019 •

edited