Conversation
39e070e to
53879fc
Compare
Code ReviewFound 4 issues that need to be addressed: 1. Password Logging Violation (SEC-2) - pkcs.go:52Critical Security Issue The debug log statement logs the PKCS#12 password in plaintext: slog.Debug("decoding PKCS#12 bundle", "size", len(pfxData), "password", password)This violates SEC-2 (MUST): "Never log secrets (private keys, passwords)". PKCS#12 passwords protect private keys and must never be logged, even at debug level. Debug logs are routinely enabled in development, staging, and production troubleshooting, creating a credential leak vector. Fix: Remove the "password", password field: slog.Debug("decoding PKCS#12 bundle", "size", len(pfxData))2. Missing CHANGELOG.md Entry (CL-1)This PR modifies production code in pkcs.go (adds debug logging and nil-check logic) but does not update CHANGELOG.md. Per CL-1 (MUST): "Every commit that changes behavior, fixes a bug, or adds a feature must add a line to the ## [Unreleased] section of CHANGELOG.md... a commit that touches production code (not just tests) always needs one." Fix: Add an entry to the ## [Unreleased] section under ### Fixed: - Fix nil pointer in PKCS#7 encoding when certificates have nil Raw fields ([75fdea1])And add the link reference at the bottom: [75fdea1]: https://github.com/sensiblebit/certkit/commit/75fdea16d969f4e541fe34f82efc35c6341e83303. Silent Continue Without Debug Logging (ERR-5) - pkcs.go:68-70The nil check uses continue without debug logging: if cert.Raw == nil {
continue
}This violates ERR-5 (MUST): "Never silently ignore errors. Use continue in loops only with a slog.Debug explaining why." Fix: Add debug logging before the continue: if cert.Raw == nil {
slog.Debug("skipping certificate with nil Raw field in PKCS#7 encoding")
continue
}4. Empty DER Bytes Logic Issue - pkcs.go:72After the nil-check loop, derBytes could be empty if all certificates have nil Raw fields. This bypasses the len(certs) == 0 guard at line 62 and could pass empty data to pkcs7.DegenerateCertificate, producing a semantically empty PKCS#7 container. Since EncodePKCS7 is an exported public API function, external callers could pass manually-constructed certificates with nil Raw fields. Fix: Add a check after the loop (after line 71): if len(derBytes) == 0 {
return nil, errors.New("no valid certificates to encode")
}This ensures the function never passes empty data to pkcs7.DegenerateCertificate. |
Description
Add structured debug logging to PKCS#12 decode path and skip nil raw certificates in PKCS#7 encoding.
Test Plan
-l debug