fix(cli): normalize validation exits, JSON schema, and export defaults#109
fix(cli): normalize validation exits, JSON schema, and export defaults#109danielewood merged 3 commits intodevelopfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aligns certkit’s CLI semantics and schemas across commands: it standardizes exit-code behavior for validation failures, normalizes JSON output keys/encoding, and hardens key-bearing export defaults (explicit passwords + sensitive file permissions), with accompanying docs/changelog updates.
Changes:
- Normalize validation exit-code behavior (exit code
2for validation failures; avoid misclassifying format limitations). - Standardize JSON schemas across commands (
diagnostics,subject/issuer, and shared payloaddata+encoding). - Require explicit PKCS#12/JKS export passwords by default (with explicit insecure override) and write key-bearing YAML bundle exports as sensitive (0600).
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/verify.go | Renames diagnoses → diagnostics in JSON and moves failing diagnosis status to error (while keeping formatting compatibility). |
| internal/verify_test.go | Updates tests to expect error instead of fail for failing diagnostics. |
| internal/exporter_test.go | Extends sensitive-file assertions to include .yaml bundle output. |
| internal/certstore/export.go | Requires explicit PKCS#12 password and marks bundle .yaml output as Sensitive: true. |
| internal/certstore/export_test.go | Updates sensitivity expectations and adds regression test for missing PKCS#12 password. |
| cmd/certkit/root.go | Adds --insecure-default-password global flag to explicitly opt into legacy changeit. |
| cmd/certkit/bundle.go | Normalizes exit semantics for chain validation failures; switches bundle/JSON payload schema to payloadJSON; requires explicit export password for p12/jks. |
| cmd/certkit/convert.go | Updates binary-format -o requirements; requires explicit p12/jks export password; switches JSON output to payloadJSON with explicit encoding. |
| cmd/certkit/convert_test.go | Adds regression test ensuring PKCS#12 multi-key limitation is a general error (not ValidationError). |
| cmd/certkit/scan.go | Wires explicit export-password selection into scan --bundle-path exports. |
| cmd/certkit/inspect.go | Refactors expired filtering into helper returning ValidationError (exit code 2). |
| cmd/certkit/verify.go | Renames Diagnoses → Diagnostics usage in CLI output paths. |
| cmd/certkit/ocsp.go | Normalizes verbose JSON keys to subject/issuer. |
| cmd/certkit/payload_json.go | Introduces shared JSON payload struct for bundle/convert. |
| cmd/certkit/cli_semantics_test.go | Adds tests around exit/error semantics and JSON shape expectations. |
| README.md | Updates supported formats, global flags, and export-password + YAML sensitivity documentation. |
| EXAMPLES.md | Updates examples to show explicit export passwords and describes insecure override. |
| CHANGELOG.md | Adds entries for JSON schema normalization, password hardening, and exit-semantics fixes. |
Comments suppressed due to low confidence (1)
EXAMPLES.md:254
- The PKCS#12 command is shown twice back-to-back (once above and again in this code block). Consider removing the duplicated PKCS#12 line here and keeping this snippet focused on the JKS example to avoid confusing/duplicative documentation.
PKCS#12/JKS exports require an explicit password via `-p`/`--password-file`:
```sh
certkit bundle cert.pem --key key.pem --format p12 -p "your-password" -o bundle.p12
certkit bundle cert.pem --key key.pem --format jks -p "your-password" -o keystore.jks
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.
This comment has been minimized.
This comment has been minimized.
|
Addressed the reviewer feedback in commit 3113397:\n\n- : wrapped export-password selection errors with context, converted to for CS-5 compliance, and changed to fail fast on errors (with for nil entries).\n- and : wrapped export-password selection errors per ERR-1.\n- + : removed duplicate internal coverage from CLI tests and moved verify-result JSON schema assertion to (diagnostics key + error status).\n- : marked JSON key rename entries as (CL-5) and corrected the PKCS#12/JKS password-default wording to match current behavior.\n- : removed the duplicated PKCS#12 command in the password section.\n\nValidation:\n- no commit to main........................................................Passed |
|
Correction to my previous note (shell escaped characters incorrectly). Addressed feedback in commit 3113397: bundle.go now uses an input struct for formatBundleOutput, wraps password-selection errors, and fails fast on KeyMatchesCert errors; convert.go and scan.go now wrap password-selection errors; duplicate internal assertions were removed from cmd/certkit/cli_semantics_test.go and moved to internal/verify_test.go; CHANGELOG.md breaking JSON changes are marked with Breaking prefixes and password-default wording was corrected; EXAMPLES.md duplicated PKCS#12 line was removed. pre-commit run --all-files passes. |
|
Follow-up: all review feedback has been addressed and fixes pushed. Please take another look when you can. |
|
Rechecked PR #109 feedback since the latest push (2026-03-01T23:42:19Z): no new actionable review or issue comments found. The only open review thread appears already addressed in code and has no additional follow-up. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Follow-up sweep complete: I found new actionable feedback from the latest review comment and pushed fixes in 1d4737f.\n\nWhat changed:\n- Removed direct tests of unexported CLI helpers from to keep coverage behavior-focused.\n- Replaced the direct test with , which verifies behavior through and still asserts this remains a general error (not ).\n- Added sentinel error in and switched test assertions to (no string-matching control flow).\n- Added sentinel error in and updated to assert with .\n- Updated changelog with this PR entry and added link reference.\n\nValidation:\n- no commit to main........................................................Passed |
|
Correction to prior comment (shell escaped markdown incorrectly): Follow-up sweep complete and new actionable feedback was addressed in commit 1d4737f. Updated files: cmd/certkit/cli_semantics_test.go, cmd/certkit/convert.go, cmd/certkit/convert_test.go, internal/certstore/export.go, internal/certstore/export_test.go, and CHANGELOG.md. This removes direct tests of unexported helpers, verifies the PKCS#12 multi-key case through runConvert behavior, replaces string-matching control flow checks with errors.Is-based assertions via sentinel errors, and records the change in changelog entries/links. Validation: pre-commit run --all-files passes. |
|
Feedback sweep complete: I reviewed review comments, review threads, reviews, and issue comments since commit 1d4737f and found no new actionable or unresolved feedback. |
This comment has been minimized.
This comment has been minimized.
9601c8e to
89d1f8c
Compare
89d1f8c to
8dc4206
Compare
Code review1 issue found. CL-3 / CL-4 (MUST): CHANGELOG.md references issue numbers as PR refsThe nine new changelog entries in this PR cite
The changes were made in this PR — #109. All nine entries should cite Additionally, the five link definitions added at the bottom of CHANGELOG.md (lines 984–988) incorrectly use Lines 982 to 990 in 8dc4206 |
* 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
diagnostics,subject/issuer, shared payloaddata+encoding)Testing
Closes #88
Closes #89
Closes #96
Closes #97
Closes #101