Conversation
- Added appearance, document, execute, extract, fonts, forms, images, initials, resources, types, and verify core files. - Included internal implementations.
- Enhanced signing and verification logic (PDFxref, trailer, catalog, etc.). - Improved certificate and key usage handling in verify package.
- Added AWS KMS, Azure Key Vault, Google Cloud KMS, and CSC signer implementations. - Included PKCS#11 signer support.
- Added setup-dss.sh and verify_pdfs.sh utility scripts.
- Updated CLI source files (sign.go, verify.go) to integrate new core logic. - Updated go.mod/go.sum dependencies. - Updated CI workflow and README.
- Updated revocation package. - Deleted legacy sign.go. - Removed unused test resources.
- Added go.mod and go.sum for modular signer packages (AWS, Azure, GCP, CSC, PKCS#11).
- Added comprehensive unit tests for all packages (core, signers, verify). - Included test fixtures in testfiles/ directory. - Updated existing tests to align with refactoring.
- Fixed unchecked errors in renderer.go (errcheck). - Fixed unkeyed struct literals in appearance.go (govet). - Fixed empty branch in verify.go (staticcheck). - Verified with golangci-lint run (0 issues).
…ification - Update VerifySignature to correctly handle DocTimeStamp (ETSI.RFC3161) by parsing TSTInfo from raw bytes and verifying against the PDF byte range without overwriting content. - Enforce strict signature validation in sign_test.go (failure on invalid signatures). - Update sign tests to use SHA-512 explicitly to improve stability. - Exclude testfile_multi.pdf from sign tests as it contains legacy SHA-1 signatures that are no longer valid.
- Restore test coverage for testfile_multi.pdf by adding TestSignPDF_AppendToMultiSig. - Validates that signing works on files with existing signatures (some of which may use older algorithms), checking only the validity of the newly appended signature.
- Propagate AtTime logic to certificate verification. - Ensure VerifyResult.Valid is set to false if constraint checks fail. - Add tests for VerifyOptions (MinRSAKeySize, AllowedAlgorithms).
- Renamed VerifyOptions.CurrentTime to AtTime for consistency. - Resolved errcheck and staticcheck lint errors in tests.
- Updated VeraPDF download URL to 1.28.2 (official release) as GitHub latest asset is missing. - Allowed DSS Validation to fail without breaking the build (missing Maven artifact).
- Update URL to 1.28.2 (official release) via HTTP. - Implement headless installation using auto-install.xml. - Re-enable Corpus Security Test.
- Refactor VerifySignature to return structured errors - Introduce verify/errors.go for custom error types - Update Signer struct to hold ValidationErrors - Isolate legacy test files in testfiles/compatibility - Update Tests to use errors.As for robust error checking - Fix test file cleanup race condition
- Revert verifySignedFile to use os.Rename - Introduce verifyIntermediateFile for non-final test steps - Silence os.Remove errors in test cleanups - Remove copyFile helper function
- Add curl timeouts to setup-dss.sh health check - Check for container termination inside the wait loop - Optimize Dockerfile.dss to use java -jar instead of mvn spring-boot:run
- Tail container logs periodically during wait loop - Verify port mapping status during health check - Ensure container is still running throughout the loop
- Restore COUNT initialization in setup-dss.sh - Update go.yml to use correct test name TestValidateDSSValidation
This was referenced Apr 7, 2026
There was a problem hiding this comment.
Pull request overview
Major refactor of pdfsign to a fluent Document-centric API, alongside significant feature expansion (verification policies, rendering/appearances, forms, initials, signature extraction) and new external signer modules, with CI enhancements for PDF validation.
Changes:
- Introduces a fluent
DocumentAPI (Open/OpenFile, stagedSign()/Verify()/Write()) with examples/tests. - Expands functionality: advanced appearance rendering, fonts/images/resources, forms read/write, initials, signature extraction/benchmarks, stricter verification policies/errors.
- Adds external signer example modules (AWS/GCP/Azure/CSC/PKCS#11) and CI scripts/workflows for validation (pdfcpu + DSS + corpus runs).
Reviewed changes
Copilot reviewed 102 out of 122 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
verify/verify.go |
Deprecates legacy verify entrypoints; changes signature discovery to AcroForm traversal and uses VerifySignature. |
verify/verify_test.go |
Minor test adjustments for staticcheck/unreachable return patterns. |
verify/types.go |
Adds verification policy options and Signer error aggregation support (ValidationErrors) + NewSigner(). |
verify/signature_unit_test.go |
Updates unit tests to use NewSigner(). |
verify/revocation_timing_test.go |
Moves revocation timing checks onto Signer.IsRevokedBeforeSigning. |
verify/keyusage_test.go |
Updates tests to use NewSigner(); staticcheck-friendly control flow. |
verify/errors.go |
Adds typed verification error types (ValidationError, RevocationError, InvalidSignatureError, PolicyError). |
verify/certificate.go |
Refactors chain/revocation processing to return typed errors; adds AtTime support and revocation timing logic. |
verify_test.go |
Adds integration-ish test for fluent verification on an uninitialized Document. |
verify_example_test.go |
Adds examples for fluent verification and strict mode. |
testfiles/images/pdfsign-seal-transparent.png |
Adds image fixture for appearances. |
testfiles/dss/README.md |
Documents DSS validator test setup. |
testfiles/dss/Dockerfile.dss |
Adds Docker build for DSS demo webapp. |
testfiles/dss/docker-compose.yml |
Adds compose config + healthcheck for DSS validator. |
signers/pkcs11/pkcs11.go |
Adds PKCS#11-backed crypto.Signer implementation. |
signers/pkcs11/pkcs11_test.go |
Adds basic construction/public-key tests for PKCS#11 signer. |
signers/pkcs11/go.mod |
New nested module for PKCS#11 signer. |
signers/pkcs11/go.sum |
Dependency sums for PKCS#11 signer. |
signers/gcp/go.mod |
New nested module for GCP signer. |
signers/gcp/go.sum |
Dependency sums for GCP signer. |
signers/gcp/gcp.go |
Adds GCP KMS-backed crypto.Signer implementation. |
signers/gcp/gcp_test.go |
Adds mock-client tests for GCP signer. |
signers/csc/go.mod |
New nested module for CSC signer (with replace to root). |
signers/csc/go.sum |
Dependency sums for CSC signer. |
signers/csc/csc_test.go |
Adds constructor validation + helper function tests. |
signers/csc/csc_client_test.go |
Adds mock server tests for CSC client flows. |
signers/azure/go.mod |
New nested module for Azure signer. |
signers/azure/go.sum |
Dependency sums for Azure signer. |
signers/azure/azure.go |
Adds Azure Key Vault-backed crypto.Signer implementation. |
signers/azure/azure_test.go |
Adds mock-client tests for Azure signer. |
signers/aws/go.mod |
New nested module for AWS signer. |
signers/aws/go.sum |
Dependency sums for AWS signer. |
signers/aws/aws.go |
Adds AWS KMS-backed crypto.Signer implementation. |
signers/aws/aws_test.go |
Adds mock-client tests for AWS signer. |
sign/verify_options_test.go |
Adds integration tests for new verify constraints (min key sizes / allowed algos). |
sign/verify_dss_test.go |
Adds DSS-based validation test and detailed report walking. |
sign/types.go |
Extends signing data/context (object updates, presign callback, extra annots, compression). |
sign/test_utils_test.go |
Adds PEM fixtures + helper to load test key/cert. |
sign/revocation.go |
Adds revocation cache + configurable revocation embedding function. |
sign/revocation_embed_test.go |
Adds tests for revocation embedding paths. |
sign/pdfxref.go |
Exports object mutation APIs on SignContext (Add/Update/WriteObject). |
sign/pdfxref_test.go |
Updates tests for exported xref APIs. |
sign/pdfxref_stream.go |
Uses shared prev-xref helper; propagates /Encrypt into xref stream header. |
sign/pdfvisualsignature.go |
Supports extra annotations during incremental page updates. |
sign/pdftrailer.go |
Uses shared helper for /Prev offset. |
sign/pdfsignature.go |
Adjusts digest method placement; improves signing certificate attribute; improves signature replacement/padding & retry flow. |
sign/pdfcatalog.go |
Preserves existing AcroForm fields when writing updated catalog; improves indirect-ref serialization checks. |
sign/pdfbyterange.go |
Updates ByteRange placeholder search logic. |
sign/helpers.go |
Adds getPrevXrefOffset helper used by trailer/xref stream. |
sign/export_test.go |
Exports a test helper for external-package tests. |
sign/appearance.go |
Adds hook for custom appearance renderer; uses exported xref API. |
sign.go |
Removes old main entrypoint. |
scripts/verify_pdfs.sh |
Adds script to validate produced PDFs via pdfcpu (strict/relaxed). |
scripts/setup-dss.sh |
Adds script to build/run DSS validator container locally/CI. |
revocation/revocation.go |
Implements CRL-based revocation checks (replacing stub). |
revocation/revocation_test.go |
Adds basic coverage tests for revocation container methods. |
resources.go |
Adds document-level font/image resource registration + font scanning. |
resources_test.go |
Adds tests for resource registration/accessors. |
internal/render/types.go |
Adds internal render model types for advanced appearances. |
internal/render/template.go |
Adds template variable expansion + initials extraction. |
internal/render/render_test.go |
Adds render/image integration tests via fluent signing. |
internal/pdf/xobject.go |
Adds page extraction as XObject helper for embedding PDF content. |
internal/pdf/scanner.go |
Adds font scanning utilities for deduplication. |
internal/pdf/scanner_test.go |
Adds test for scanning existing fonts from fixtures. |
initials/initials.go |
Adds initials config/builder types. |
initials/initials_test.go |
Adds integration test for initials application via fluent API. |
initials.go |
Implements initials application via presign callback + annotation updates. |
images/images.go |
Adds images.Image resource type. |
go.mod |
Updates core dependencies and sets module Go version. |
go.sum |
Updates dependency sums. |
forms/forms.go |
Adds form field extraction + update generation + field mapping helpers. |
forms/forms_test.go |
Adds tests for form APIs and real-file integration. |
forms/forms_example_test.go |
Adds example for listing fields via fluent API. |
forms.go |
Adds Document form field APIs and staged updates integration. |
format_test.go |
Adds tests for signature format enforcement. |
fonts/fonts.go |
Adds font resource types + TTF metrics parsing and width calculations. |
extract/extract.go |
Adds signature iterator + byte range reader utilities. |
extract/extract_test.go |
Adds extraction integration test over a freshly signed PDF. |
extract/benchmark_extract_test.go |
Adds extraction benchmarks (iterator/contents/signed data). |
extract.go |
Exposes signature iteration on Document. |
execute.go |
Implements staged operation execution (Write) bridging fluent API to sign package. |
document.go |
Introduces new Document type and fluent entrypoints. |
document_test.go |
Adds fluent builder unit tests and basic Document method checks. |
document_example_test.go |
Adds usage examples for signing, compression, fonts, and verification. |
crypto_test.go |
Adds algorithm-profile matrix test for sign+verify+external validation. |
cli/verify.go |
Updates CLI verification to use fluent API and outputs new JSON shape. |
cli/verify_test.go |
Adds tests for CLI verify flows and error exits. |
cli/sign.go |
Updates CLI signing to use fluent API and improves error handling. |
cli/cli_test.go |
Updates CLI tests for new cert type parsing and subprocess usage test. |
benchmark_test.go |
Adds benchmarks for appearance creation, signing, and verification. |
appearance_test.go |
Adds basic coverage test for fluent appearance builder. |
appearance_example_test.go |
Adds appearance examples, including advanced and standard templates. |
.gitignore |
Updates ignores for binaries, OS/IDE files, Go work files, certs, and test artifacts. |
.github/workflows/go.yml |
Adds Java setup, pdfcpu validation, DSS validation, and corpus test job. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+113
to
+122
| // Verify CRL signature and content | ||
| crl, err := x509.ParseRevocationList(body) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to parse CRL: %v", err) | ||
| } | ||
|
|
||
| if err := crl.CheckSignatureFrom(issuer); err != nil { | ||
| // Just log or strictly fail? Strict fail is better for security. | ||
| return fmt.Errorf("CRL signature invalid: %v", err) | ||
| } |
There was a problem hiding this comment.
embedCRLRevocationStatus unconditionally verifies the CRL signature with crl.CheckSignatureFrom(issuer), but issuer can be nil (the function comment explicitly mentions a nil issuer). This will panic or always fail for unknown issuers. Add an issuer == nil guard (either skip signature verification, or return a clear error) before calling CheckSignatureFrom.
Comment on lines
+107
to
+115
| fmt.Fprintf(&buf, " /V /Off\n") | ||
| } | ||
| case string: | ||
| fmt.Fprintf(&buf, " /V (%s)\n", val) | ||
| case int, int64, float64: | ||
| fmt.Fprintf(&buf, " /V %v\n", val) | ||
| default: | ||
| fmt.Fprintf(&buf, " /V (%v)\n", val) | ||
| } |
There was a problem hiding this comment.
GenerateUpdate writes string values directly into a PDF literal string via (%s) without escaping parentheses, backslashes, or control characters. This can corrupt the PDF structure and enables content injection into the updated object. Use the same escaping logic as signing (e.g., a shared pdfString-escape helper) when formatting PDF strings.
execute.go
Outdated
Comment on lines
+123
to
+128
| // Need to get a ReadSeeker from our reader | ||
| if rs, ok := d.reader.(io.ReadSeeker); ok { | ||
| err := sign.SignWithData(rs, output, d.rdr, d.size, signData) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Document.Write() only runs signing if d.reader implements io.ReadSeeker; otherwise it silently skips signing and still returns a Result. Since Document.Open accepts io.ReaderAt, this will break for valid ReaderAt implementations that aren't also ReadSeeker. Consider wrapping d.reader with io.NewSectionReader(d.reader, 0, d.size) (or returning an explicit error) so Write always executes or fails deterministically.
Suggested change
| // Need to get a ReadSeeker from our reader | |
| if rs, ok := d.reader.(io.ReadSeeker); ok { | |
| err := sign.SignWithData(rs, output, d.rdr, d.size, signData) | |
| if err != nil { | |
| return nil, err | |
| } | |
| // Wrap the document reader so signing always receives a ReadSeeker. | |
| rs := io.NewSectionReader(d.reader, 0, d.size) | |
| err := sign.SignWithData(rs, output, d.rdr, d.size, signData) | |
| if err != nil { | |
| return nil, err |
Comment on lines
+60
to
+66
| for i := 0; i < fields.Len(); i++ { | ||
| if fieldsAdded > 0 { | ||
| catalog_buffer.WriteString(" ") | ||
| } | ||
| ptr := fields.Index(i).GetPtr() | ||
| catalog_buffer.WriteString(strconv.Itoa(int(ptr.GetID())) + " 0 R") | ||
| fieldsAdded++ |
There was a problem hiding this comment.
When copying existing AcroForm fields, this writes each entry as an indirect reference using ptr.GetID(). If a field entry is a direct object (ptr ID == 0), this will emit 0 0 R which is invalid PDF. Either skip entries with ptr.GetID()==0 or serialize direct objects (similar to serializeCatalogEntry’s ptr.GetID()>0 guard).
Comment on lines
+133
to
+146
| if field.Key("FT").Name() == "Sig" { | ||
| // Get the signature dictionary (the value of the field) | ||
| v := field.Key("V") | ||
|
|
||
| // Verify if it is a signature dictionary and has the correct filter | ||
| if !v.IsNull() && v.Key("Filter").Name() == "Adobe.PPKLite" { | ||
| foundSignature = true | ||
|
|
||
| // Use the new modular signature processing function | ||
| signer, err := VerifySignature(v, file, size, options) | ||
| if err != nil { | ||
| // Skip this signature if there's a critical error | ||
| return true // Continue to next | ||
| } |
There was a problem hiding this comment.
If VerifySignature returns a non-nil error, the code currently skips appending any signer result but still leaves foundSignature=true. This can produce a successful VerifyWithOptions response with zero signers and no error even though signatures existed but failed to process. Consider only setting foundSignature after a signature is successfully processed (or track a separate ‘fieldFound’ vs ‘processed’ state and surface an error when all signatures fail).
sign/types.go
Outdated
Comment on lines
+42
to
+46
| // PreSignCallback is called before the signature object is written. | ||
| // It allows adding additional objects (e.g., initials) using the SignContext. | ||
| // PreSignCallback is called before the signature object is written. | ||
| // It allows adding additional objects (e.g., initials) using the SignContext. | ||
| PreSignCallback func(context *SignContext) error |
There was a problem hiding this comment.
The PreSignCallback doc comment is duplicated verbatim. Please remove the duplicate to keep the exported API docs clean.
.github/workflows/go.yml
Outdated
| @@ -23,6 +23,12 @@ jobs: | |||
| with: | |||
| go-version: ^1.23 | |||
There was a problem hiding this comment.
The workflow sets up Go ^1.23, but the root go.mod declares go 1.24.0. This will fail in CI with a “go: go.mod requires go >= 1.24” style error. Update the workflow to use Go 1.24.x (or lower the module’s go directive if 1.24 features aren’t required).
Suggested change
| go-version: ^1.23 | |
| go-version: '1.24.x' |
.github/workflows/go.yml
Outdated
| go install github.com/pdfcpu/pdfcpu/cmd/pdfcpu@latest | ||
|
|
||
| # Install veraPDF greenfield | ||
| curl -L http://software.verapdf.org/rel/1.28/verapdf-greenfield-1.28.2-installer.zip -o verapdf.zip |
There was a problem hiding this comment.
veraPDF is downloaded over plain HTTP, which is vulnerable to MITM tampering in CI. Switch this to HTTPS (if available) or verify the downloaded artifact (checksum/signature) before executing/installing it.
Suggested change
| curl -L http://software.verapdf.org/rel/1.28/verapdf-greenfield-1.28.2-installer.zip -o verapdf.zip | |
| curl -L https://software.verapdf.org/rel/1.28/verapdf-greenfield-1.28.2-installer.zip -o verapdf.zip |
verify/certificate.go
Outdated
Comment on lines
14
to
16
| // buildCertificateChainsWithOptions builds certificate chains with custom verification options | ||
| func buildCertificateChainsWithOptions(p7 *pkcs7.PKCS7, signer *Signer, revInfo revocation.InfoArchival, options *VerifyOptions) (string, error) { | ||
| func buildCertificateChainsWithOptions(p7 *pkcs7.PKCS7, signer *Signer, revInfo revocation.InfoArchival, options *VerifyOptions) (error, error) { | ||
| // Directory of certificates, including OCSP |
There was a problem hiding this comment.
buildCertificateChainsWithOptions currently returns (error, error) but only ever returns a non-nil first error and always returns nil for the second value. This dual-error return is confusing for callers (it looks like a fatal+nonfatal split but isn’t implemented). Consider either returning a single error (and aggregate warnings into signer fields), or explicitly document/implement the two-tier error semantics (e.g., validationErr, fatalErr).
Member
Author
There was a problem hiding this comment.
Check the intention of the double error return value, I agree this is confusing.
Consider refactoring / splitting this function to reduce its length and complexity.
There was a problem hiding this comment.
Fixed in commit 445d607. buildCertificateChainsWithOptions now:
- Returns a single
(error)instead of the confusing(error, error)— the second value was alwaysnil - Is significantly shorter by extracting four helper functions:
resolveVerificationTime,parseEmbeddedRevocationData,applyRevocationStatus,applyRevocationImpact, andbuildRevocationWarning - The caller in
signature.gois updated accordingly
Comment on lines
63
to
+83
| @@ -34,18 +73,32 @@ func embedOCSPRevocationStatus(cert, issuer *x509.Certificate, i *revocation.Inf | |||
| } | |||
|
|
|||
| // check if we got a valid OCSP response | |||
| _, err = ocsp.ParseResponseForCert(body, cert, issuer) | |||
| ocspResp, err := ocsp.ParseResponseForCert(body, cert, issuer) | |||
| if err != nil { | |||
| return err | |||
| } | |||
| if ocspResp.Status != ocsp.Good { | |||
| return fmt.Errorf("OCSP status is not 'Good': %v", ocspResp.Status) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Revocation fetches use http.Get but don’t check resp.StatusCode. Non-200 responses will be treated as successful downloads and then fail later with parse errors that hide the real cause. Consider rejecting non-2xx responses with a clear error message before parsing/embedding.
Member
Author
|
@copilot apply changes based on the comments in this thread |
…fy, CI) Agent-Logs-Url: https://github.com/digitorus/pdfsign/sessions/b894ba1d-ae05-4ed4-b52f-6a188c31d188 Co-authored-by: vanbroup <[email protected]>
All changes from the review thread have been applied in commit 445d607:
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This Pull Request represents a comprehensive overhaul of the
pdfsignlibrary. It introduces a major API refactoring to a modern Fluent API model and significantly extends functionality across the board.We have moved away from the legacy functional options pattern to improve developer experience and added robust support for new capabilities like advanced appearance customization, form handling, and comprehensive external signer integrations.
Key Changes
1. Fluent API Model
We have replaced the legacy
SignWithOptionsand VerifyWithOptions patterns with a chainable builder interface.Before (Legacy):
After (New Fluent API):
2. Extended Functionality
We have significantly expanded the library's capabilities:
3. External Signer Integrations
Added distinct modules providing implementations for major remote signing services. These serve as production-ready examples of how to integrate external KMS providers without bloating the core library.
signers/awssigners/gcpsigners/azuresigners/cscsigners/pkcs114. Code Quality & Testing
*_test.gocoverage added for all packages.testfiles/.errcheck,govet, andstaticcheckissues; strict linting enforced.Breaking Changes
pdfsign.Sign()andpdfsign.Verify()top-level functions are deprecated and will be removed in a future release. Please migrate topdfsign.Open().Sign()andpdfsign.Open().Verify()immediately.types.gofor new definitions.Verification
go test ./...to execute the new test suite.scripts/verify_pdfs.shto validate generated signatures against DSS (if configured).