fix: harden core parsing and identity selection#107
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64ed695793
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR hardens certificate/key/container parsing and selection logic across the library and CLI to avoid silent data loss and incorrect identity/issuer selection, and adds targeted edge-case tests to lock in the behavior.
Changes:
- Make PEM parsing resilient to malformed blocks; expand PEM/DER private-key detection and scanning of PEM bundles.
- Preserve JKS private-key entry alias/chain pairing through decoding and container selection.
- Improve correctness checks for CA cert/key mismatch (CSR signing) and OCSP issuer auto-selection; add regression tests.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
sign.go |
Rejects CSR signing when CA certificate and CA key don’t match. |
sign_test.go |
Adds test covering CA cert/key mismatch fast-fail behavior. |
jks.go |
Adds DecodeJKSKeyEntries to preserve alias→key→chain pairing; keeps legacy DecodeJKS wrapper. |
jks_test.go |
Adds alias/chain pairing regression test for JKS key entries. |
internal/certstore/container.go |
Updates JKS selection to use paired key-entry chains; adds DER private-key parsing. |
internal/certstore/container_test.go |
Adds DER private-key parse test and JKS pairing selection test; adjusts unparseable inputs coverage. |
certkit.go |
Makes PEM cert parsing skip malformed blocks; makes PEM key/CSR parsers scan bundles; adds issuer auto-selection helper. |
certkit_test.go |
Adds tests for malformed+valid PEM preservation, key/CSR scanning, issuer selection, and updated error-path expectations. |
internal/inspect.go |
Inspects all PEM keys (not just first block); adds DER PKCS#1/SEC1 private key detection. |
internal/inspect_test.go |
Adds coverage for malformed cert blocks coexisting with valid certs; adds DER RSA key inspection test. |
internal/certstore/memstore.go |
Changes cert dedup identity to fall back to issuer+serial when AKI is missing. |
internal/certstore/memstore_test.go |
Adds test ensuring missing-AKI certs dedup by issuer+serial rather than serial alone. |
cmd/certkit/ocsp.go |
Improves issuer parsing (PEM or DER) and issuer auto-selection via signature-valid matching. |
CHANGELOG.md |
Records behavior changes/fixes and new edge-case test coverage under Unreleased. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Addressed reviewer feedback in fe88d6e:\n\n- : JKS selection now prefers the first key entry with a certificate chain, keeps key/cert pairing safe (no unrelated trusted cert fallback), and includes trusted JKS certs in when a key-entry leaf is used.\n- : removed error-string control flow in , added explicit skip logging for malformed/failed key and cert parse paths, and made continue scanning past malformed CSR blocks.\n- + : introduced sentinel and updated test assertions to .\n- : aligned SQLite certificate identity keying with missing-AKI issuer+serial dedup semantics to avoid dropped certs on persistence; added regression coverage in .\n- and : added/updated regression tests for malformed CSR scanning, table-driven issuer selection scenarios, JKS trusted-extra retention, and chained-entry selection in multi-entry JKS.\n\nAlso ran no commit to main........................................................Passed |
|
Correction to my prior note (shell escaped formatting issue). Addressed reviewer feedback in fe88d6e:\n\n- internal/certstore/container.go: JKS selection now prefers the first key entry with a certificate chain, keeps key/cert pairing safe (no unrelated trusted cert fallback), and includes trusted JKS certs in extra cert output when a key-entry leaf is used.\n- certkit.go: removed error-string control flow in ParsePEMPrivateKeyWithPasswords, added explicit slog.Debug skip logging for malformed/failed key and cert parse paths, and made ParsePEMCertificateRequest continue scanning past malformed CSR blocks.\n- sign.go + sign_test.go: introduced sentinel ErrCAKeyMismatch and updated test assertions to errors.Is.\n- internal/certstore/sqlite.go: aligned SQLite certificate identity keying with missing-AKI issuer+serial dedup semantics to avoid dropped certs on persistence; added regression coverage in internal/certstore/sqlite_test.go.\n- certkit_test.go and internal/certstore/container_test.go: added/updated regression tests for malformed CSR scanning, table-driven issuer selection scenarios, JKS trusted-extra retention, and chained-entry selection in multi-entry JKS.\n\nAlso ran pre-commit run --all-files successfully before commit. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe88d6ebdf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Try private key | ||
| if key, err := certkit.ParsePEMPrivateKeyWithPasswords(data, passwords); err == nil { | ||
| results = append(results, inspectKey(key)) | ||
| if keys, err := certkit.ParsePEMPrivateKeys(data, passwords); err == nil { |
There was a problem hiding this comment.
Preserve valid key results when one PEM key block fails
inspectPEMData now gates key inspection on ParsePEMPrivateKeys(data, passwords) succeeding, but ParsePEMPrivateKeys is fail-fast for any malformed/unsupported key block. In PEM bundles that contain at least one valid private key plus one bad key block (for example a corrupted or unsupported key later in the file), this drops all private_key results instead of reporting the valid key(s), which is a regression for inspect output robustness.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in c093758. inspectPEMData now scans PEM key blocks one-by-one and calls ParsePEMPrivateKeyWithPasswords per block, logging and skipping malformed key blocks instead of dropping all key results. Added regression coverage in TestInspectFile_PEMMalformedKeyAndValidKey to ensure valid keys are still reported when a malformed key block appears in the same bundle.
|
Follow-up: all review feedback has been addressed and fixes pushed. Please take another look when you can. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c093758328
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if err := cert.CheckSignatureFrom(candidate); err == nil { | ||
| return candidate |
There was a problem hiding this comment.
Keep issuer DN match when auto-selecting OCSP issuer
The fallback branch in SelectIssuerCertificate accepts any candidate where CheckSignatureFrom succeeds, even if cert.RawIssuer does not match the candidate subject; CheckSignatureFrom only validates signature/key usage and does not enforce issuer-name equality. In bundles containing two issuer certs that reuse the same signing key but have different subject encodings/names (common with cross-signing or reissued intermediates), this can select the wrong issuer cert, causing OCSP requests to be built with the wrong issuer-name hash and fail with unknown/no match despite having a valid issuer in ExtraCerts.
Useful? React with 👍 / 👎.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Completed another review-feedback sweep and addressed the new actionable items from comments after c093758.\n\nChanges pushed in 46c2132:\n- Added before all error-path branches in so skipped trusted/key-entry parse failures are observable (ERR-5).\n- Tightened to require issuer DN match + signature validity (removed fallback that could pick a DN-mismatched issuer with reused key material), and added debug logging for rejected candidates.\n- Consolidated duplicate CSR scan tests into a single table-driven test () to satisfy T-12 while preserving coverage.\n- Updated under Unreleased/Fixed for this behavior change set.\n\nValidation: no commit to main........................................................Passed |
|
Correction (previous comment had shell-escaped formatting glitches).\n\nCompleted another review-feedback sweep and addressed the new actionable items from comments after c093758.\n\nChanges pushed in 46c2132:\n- Added slog debug logging before all error-path continue branches in DecodeJKSKeyEntries so skipped trusted/key-entry parse failures are observable (ERR-5).\n- Tightened SelectIssuerCertificate to require issuer DN match plus signature validity, and removed the fallback that could pick a DN-mismatched issuer with reused key material.\n- Consolidated duplicate CSR scan tests into a single table-driven test: TestParsePEMCertificateRequest_SkipsBadBlocksBeforeValidCSR (T-12).\n- Updated CHANGELOG.md under Unreleased/Fixed for this follow-up behavior change.\n\nValidation: pre-commit run --all-files passed before commit. |
This comment has been minimized.
This comment has been minimized.
|
Completed another feedback sweep and found actionable new feedback after 46c2132. Addressed both ERR-5 items from the latest review note in 50c7a56:\n\n- Added before skipping non-private-key JKS aliases in \n- Added before breaking on malformed PKCS#8 key payloads in \n- Updated under Unreleased/Fixed for PR #107\n\nValidation: no commit to main........................................................Passed |
|
Correction to my prior comment (shell interpolation mangled formatting). Completed another feedback sweep and found actionable new feedback after 46c2132. Addressed both ERR-5 items from the latest review note in 50c7a56:
Validation: pre-commit run --all-files passed locally. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 920843de4c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for { | ||
| var block *pem.Block | ||
| block, rest = pem.Decode(rest) | ||
| if block == nil { | ||
| break |
There was a problem hiding this comment.
Stop scanning after first key block in password parser
ParsePEMPrivateKeyWithPasswords now keeps scanning all key PEM blocks after a failed parse/decrypt attempt, so a file with an encrypted first key and an unencrypted later key will return the later key when the provided password is wrong for the first block. This changes failure into silent key substitution, and callers that expect one key file to map to one signer (for example CSR generation via internal/csr.go) can proceed with an unintended key instead of surfacing a decryption error.
Useful? React with 👍 / 👎.
Code reviewTwo ERR-5 violations found in new code — missing
This PR already adds Issue 1 —
|
Code reviewOne category of ERR-5 (MUST) violations found in ERR-5 violationsRule: ERR-5 (MUST) — Use 1.
|
* fix(wasm): harden ingestion and require explicit unverified export (#105) * fix(wasm): harden ingest and make unverified export explicit * fix: address PR 105 reliability review feedback * fix(web): enforce upstream timeout through body reads * fix: address latest PR 105 review feedback * fix(crl): clarify read errors and harden size-limit coverage * fix: harden core parsing and identity selection (#107) * fix: harden parsing and issuer/key selection correctness * docs(changelog): reference PR for unreleased parsing fixes * fix: prevent parser fallback and JKS identity regressions * fix(inspect): preserve valid keys when PEM bundle has malformed blocks * fix: address remaining PR 107 review feedback * fix(jks): surface skipped-entry reasons in debug logs * fix(scan): harden scan boundaries and export text summary (#106) * fix(scan): keep traversal bounded and restore export summaries * fix(scan): fail fast on walker processing errors * fix(scan): use typed max-size errors in read paths * fix(scan): reject invalid export formats consistently * fix(scan): keep export destination off stdout * fix(network): harden SSRF fetch validation and apply default connect timeout (#108) * fix(network): harden revocation fetch SSRF checks and connect timeout defaults * fix(network): propagate SSRF validation deadlines and unblock inspect AIA opt-in * fix(bundle): restore private-network opt-in for AIA chain fetches * fix(network): address remaining PR feedback for inspect AIA handling * fix(wasm): keep AIA resolution working without DNS lookups * fix(cli): normalize validation exits, JSON schema, and export defaults (#109) * fix(cli): align validation exits and secure export defaults * fix(cli): address PR #109 review feedback * fix(tests): align PR #109 follow-up feedback * fix(security): remediate current code scanning alerts * build(hooks): consume shared develop branch-name exemption * fix: address PR 110 review feedback
Summary
Closes #92
Closes #93
Closes #94
Closes #102