fix for issue 124 (Signature size for RSA 3072 and 4096 bits is too small)#125
fix for issue 124 (Signature size for RSA 3072 and 4096 bits is too small)#125mitresthen wants to merge 2 commits intodigitorus:mainfrom
Conversation
…g signing to avoid corruption.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #125 +/- ##
==========================================
+ Coverage 60.42% 62.03% +1.60%
==========================================
Files 25 26 +1
Lines 1908 1973 +65
==========================================
+ Hits 1153 1224 +71
+ Misses 590 579 -11
- Partials 165 170 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Fixes signature placeholder sizing for RSA 3072/4096 (and other key types) by basing the estimate on the certificate’s public key size, adds bounded retry logic to prevent infinite loops, and resets signing state between retries to avoid PDF corruption.
Changes:
- Compute signature sizing from the certificate public key (with optional
SignatureSizeOverride) and validate signer/certificate key match. - Add bounded retry logic for “signature too long” and reset accumulated signing state on retry.
- Add new key-size utilities and multiple regression tests covering RSA-3072/4096 and override/retry behavior.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| verify/utils_test.go | Whitespace cleanup in tests. |
| sign/types.go | Adds SignatureSizeOverride and tracks retry count in context. |
| sign/sign.go | Resets signing state for retries; uses public-key-based signature sizing; skips double-write after retry recursion. |
| sign/pdfsignature.go | Adds capped retry logic with padding strategy and clearer error on retry exhaustion. |
| sign/keysize.go | Introduces signature-size estimation per public key type + signer/cert match validation helpers. |
| sign/keysize_test.go | Adds unit tests and benchmarks for key sizing and signer/cert matching. |
| sign/keysizes_test.go | Adds a broad key-matrix signing/verification test harness and reporting helpers. |
| sign/sign_test.go | Adds regression tests for retry validity, override behavior, and RSA-3072 signing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Capture log output to verify retry happens | ||
| var logBuf bytes.Buffer | ||
| log.SetOutput(&logBuf) | ||
| defer log.SetOutput(os.Stderr) | ||
|
|
There was a problem hiding this comment.
This test mutates the global logger output. In this package there are subtests that call t.Parallel() (e.g., earlier in this file), so this can introduce data races/flaky assertions by capturing logs from other concurrent tests. Prefer avoiding global log.SetOutput in tests (or guard it with a package-level mutex) and restore the previous writer (save old := log.Writer() then defer log.SetOutput(old)) rather than forcing os.Stderr.
| // Generate RSA-4096 key - produces 512-byte signatures | ||
| privateKey, err := rsa.GenerateKey(rand.Reader, 4096) | ||
| if err != nil { | ||
| t.Fatalf("failed to generate RSA-4096 key: %v", err) | ||
| } | ||
|
|
||
| template := &x509.Certificate{ | ||
| SerialNumber: big.NewInt(1), | ||
| Subject: pkix.Name{CommonName: "Retry Test"}, | ||
| NotBefore: time.Now(), | ||
| NotAfter: time.Now().Add(24 * time.Hour), | ||
| KeyUsage: x509.KeyUsageDigitalSignature, | ||
| BasicConstraintsValid: true, | ||
| } | ||
| certDER, err := x509.CreateCertificate(rand.Reader, template, template, &privateKey.PublicKey, privateKey) | ||
| if err != nil { | ||
| t.Fatalf("failed to create certificate: %v", err) | ||
| } | ||
| cert, err := x509.ParseCertificate(certDER) | ||
| if err != nil { | ||
| t.Fatalf("failed to parse certificate: %v", err) |
There was a problem hiding this comment.
Generating an RSA-4096 key inside a unit test is relatively expensive and can noticeably slow down/flake CI (especially on constrained runners). Consider using a static test key fixture (PEM in the test like other tests here), or moving this into a //go:build slow/integration test suite.
| // Generate RSA-4096 key - produces 512-byte signatures | |
| privateKey, err := rsa.GenerateKey(rand.Reader, 4096) | |
| if err != nil { | |
| t.Fatalf("failed to generate RSA-4096 key: %v", err) | |
| } | |
| template := &x509.Certificate{ | |
| SerialNumber: big.NewInt(1), | |
| Subject: pkix.Name{CommonName: "Retry Test"}, | |
| NotBefore: time.Now(), | |
| NotAfter: time.Now().Add(24 * time.Hour), | |
| KeyUsage: x509.KeyUsageDigitalSignature, | |
| BasicConstraintsValid: true, | |
| } | |
| certDER, err := x509.CreateCertificate(rand.Reader, template, template, &privateKey.PublicKey, privateKey) | |
| if err != nil { | |
| t.Fatalf("failed to create certificate: %v", err) | |
| } | |
| cert, err := x509.ParseCertificate(certDER) | |
| if err != nil { | |
| t.Fatalf("failed to parse certificate: %v", err) | |
| // Use static RSA test fixtures instead of generating a new RSA-4096 key | |
| // during the unit test. This preserves the large-signature behavior while | |
| // avoiding expensive and potentially flaky CI work. | |
| privateKeyBlock, _ := pem.Decode([]byte(signKeyPem)) | |
| if privateKeyBlock == nil { | |
| t.Fatal("failed to decode test private key PEM") | |
| } | |
| var privateKey *rsa.PrivateKey | |
| if pkcs1Key, err := x509.ParsePKCS1PrivateKey(privateKeyBlock.Bytes); err == nil { | |
| privateKey = pkcs1Key | |
| } else { | |
| privateKeyAny, err := x509.ParsePKCS8PrivateKey(privateKeyBlock.Bytes) | |
| if err != nil { | |
| t.Fatalf("failed to parse test private key: %v", err) | |
| } | |
| var ok bool | |
| privateKey, ok = privateKeyAny.(*rsa.PrivateKey) | |
| if !ok { | |
| t.Fatal("test private key is not an RSA private key") | |
| } | |
| } | |
| certBlock, _ := pem.Decode([]byte(signCertPem)) | |
| if certBlock == nil { | |
| t.Fatal("failed to decode test certificate PEM") | |
| } | |
| cert, err := x509.ParseCertificate(certBlock.Bytes) | |
| if err != nil { | |
| t.Fatalf("failed to parse test certificate: %v", err) |
| var err error | ||
| sigSize, err = PublicKeySignatureSize(context.SignData.Certificate.PublicKey) | ||
| if err != nil { | ||
| sigSize = DefaultSignatureSize |
There was a problem hiding this comment.
PublicKeySignatureSize errors are silently swallowed by falling back to DefaultSignatureSize. This can hide misconfigured/unsupported certificates (e.g., nil/unsupported public key) and makes sizing behavior hard to reason about now that SignatureSizeOverride exists. Consider returning an explicit error when auto-detection fails and SignatureSizeOverride is not set (or at least logging the sizing error and guiding users to SignatureSizeOverride).
| sigSize = DefaultSignatureSize | |
| return fmt.Errorf("failed to determine signature size from certificate public key: %w; set SignatureSizeOverride to specify the placeholder size explicitly", err) |
| if signErr != nil { | ||
| result.SignOK = false | ||
| result.Error = signErr.Error() | ||
| } else { | ||
| result.SignOK = true |
There was a problem hiding this comment.
This test records signing/verification failures in result but does not fail the subtest (so the suite can pass even when signing breaks). If this is intended to be a regression test matrix, consider asserting expected outcomes (e.g., t.Fatalf when SignFile/verification fails) or explicitly t.Skip/move this to an example/tooling test so it doesn’t mask regressions.
| // Generate key and certificate | ||
| privateKey, cert, err := generateTestCertificate(tc.KeyType, tc.KeySize, tc.CASignatureAlgo) | ||
| if err != nil { |
There was a problem hiding this comment.
This generates a fresh keypair/certificate for every matrix case (including RSA-4096), which can make the standard go test suite slow. Consider caching generated keys per size, using static fixtures, or gating the full matrix behind a build tag (e.g., //go:build slow) so CI/default runs stay fast.
| rsaKey, _ := rsa.GenerateKey(rand.Reader, 4096) | ||
| ecKey, _ := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) | ||
| _, edKey, _ := ed25519.GenerateKey(rand.Reader) |
There was a problem hiding this comment.
These benchmarks ignore key-generation errors. If rsa.GenerateKey / ecdsa.GenerateKey / ed25519.GenerateKey fails (e.g., entropy/read failure), the benchmark may panic later when using a nil key. Handle the errors (fail the benchmark) to keep behavior deterministic.
| certDER, _ := x509.CreateCertificate(rand.Reader, template, template, rsaKey.Public(), rsaKey) | ||
| cert, _ := x509.ParseCertificate(certDER) | ||
|
|
There was a problem hiding this comment.
These certificate creation/parsing calls ignore errors. If either fails, cert can be nil and ValidateSignerCertificateMatch may panic or skew benchmark results. Consider checking and failing the benchmark on error before b.ResetTimer().
| // The Bug Under Investigation: | ||
| // ============================ | ||
| // The library uses Certificate.SignatureAlgorithm to estimate signature buffer size. | ||
| // This is WRONG because it confuses the CA's signing algorithm with your key size. |
There was a problem hiding this comment.
The header comment describes the bug as if it still exists ("The library uses Certificate.SignatureAlgorithm to estimate signature buffer size"). Since this PR changes the sizing logic, this comment is now misleading; consider updating it to describe the historical bug/regression being tested (or removing the outdated explanation).
| // The Bug Under Investigation: | |
| // ============================ | |
| // The library uses Certificate.SignatureAlgorithm to estimate signature buffer size. | |
| // This is WRONG because it confuses the CA's signing algorithm with your key size. | |
| // Historical Bug / Regression Coverage: | |
| // ===================================== | |
| // This test suite covers a previous bug where signature buffer sizing was | |
| // estimated from Certificate.SignatureAlgorithm instead of the actual signing | |
| // key. That behavior was incorrect because it confused the CA's signing | |
| // algorithm with the signer's key size or curve characteristics. |
There was a problem hiding this comment.
| // The Bug Under Investigation: | |
| // ============================ | |
| // The library uses Certificate.SignatureAlgorithm to estimate signature buffer size. | |
| // This is WRONG because it confuses the CA's signing algorithm with your key size. |
|
Please note that #126 is going te replace the current main branch when ready. |
Add a hopefully more correct way of calculating signature size, based on how it's done in other pdf signing libraries. Set an upper limit on retries to avoid a loop. Reset state inbetween retries to avoid corrupt pdf's.