Skip to content

fix: prevent scan stalls and restore all bundle formats - #233

Merged
danielewood merged 3 commits into
mainfrom
fix/scan-stalls-and-default-formats
Oct 14, 2026
Merged

danielewood merged 3 commits into
mainfrom
fix/scan-stalls-and-default-formats

Conversation

@danielewood

@danielewood danielewood commented Oct 14, 2026 •

Copy link
Copy Markdown
Collaborator

Description

Directory scans could stall when unrelated JSON or malformed PKCS#7 input reached the recursive BER decoder. Validate the complete BER structure, enforce child boundaries, and limit nesting to 32 constructed values. Normalize BER lengths before upstream parsing. Valid definite- and indefinite-length BER bundles remain supported.

Terminal progress now shows the active file or phase and elapsed time through scanning, managed planning, writes, and trust checks. Warnings stay separate from progress. Cleanup failures retain the original scan error.

Managed exports select all 12 formats by default, including CSR, CSR JSON, YAML, and Kubernetes YAML. Explicit --formats selections still limit output. CN-derived bundle directories keep their names; invalid derived Secret names get a valid label with a stable hash suffix, reported in the plan. Explicit bundle names remain unchanged. Update the help and documentation.

Test Plan

  • An accepted-OID overlapping BER payload times out with the previous parser path and is rejected by the fix. Regressions cover all three accepted content types, definite and indefinite envelopes, excessive nesting, invalid lengths, and truncated values.
  • DER and BER round trips preserve the certificate chain, including nested indefinite lengths and nonminimal definite lengths.
  • Parser fuzzing completed over 500,000 inputs without a crash or stall.
  • Managed export tests cover all 12 defaults, exact explicit subsets, CN-derived Secret names, stable refresh names, and name collisions.
  • CLI tests verify progress during managed planning and trust loading, preservation of cleanup errors, and failure before writing if the password warning cannot be printed.
  • The reported input directory completes scanning all 2,206 files.
  • Full pre-commit checks include Go race tests, vet, lint, vulnerability checks, WASM, and web tests. Run with uv-managed Python and an isolated golangci-lint v2.14.0. Exclude the npm dependency updater to keep unrelated dependency changes out of this PR.
  • Go editor diagnostics pass for all changed Go files.

Checklist

  • Commit messages follow Conventional Commits
  • CHANGELOG.md updated (if user-facing change)
  • Exported functions have doc comments

Reject unrelated inputs before the PKCS#7 BER parser can consume excessive
CPU. Keep the current scan file or phase visible during slow work and clear
progress before warnings.

Restore all 12 managed export formats by default while preserving explicit
subsets, so CSR and YAML artifacts are no longer omitted. Add regression
coverage and update the CLI help and documentation.
Copilot AI balanced review requested due to automatic review settings October 14, 2026 16:32
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-14T17:34:51.711710Z adfc5c7 New commits
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The PKCS#7 guard still permits attacker-controlled recursive BER parsing, alongside several mandatory convention violations.

4 open findings
What changed in this PR

Prevents scan stalls, improves terminal progress, and restores all managed bundle formats.

Changes:

  • Prevalidates PKCS#7 envelopes and adds regressions.
  • Adds live scan activity and elapsed-time progress.
  • Defaults managed exports to all 12 formats.
File Description
README.md Documents default formats.
EXAMPLES.md Updates export examples.
CHANGELOG.md Records user-facing fixes.
.claude/​docs/​architecture.md Updates architecture notes.
pkcs.go Adds PKCS#7 envelope precheck.
pkcs_test.go Tests DER, BER, and malformed inputs.
internal/​certstore/​export_formats.go Restores all default formats.
cmd/​certkit/​scan.go Integrates scan progress.
cmd/​certkit/​scan_progress.go Implements terminal progress.
cmd/​certkit/​scan_progress_test.go Tests progress behavior.
cmd/​certkit/​scan_feedback_test.go Tests JSON scan regression.
cmd/​certkit/​scan_export.go Updates formats help.
cmd/​certkit/​scan_export_test.go Verifies format artifacts.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread pkcs.go
Comment thread cmd/certkit/scan.go
Comment thread CHANGELOG.md Outdated
Comment thread pkcs.go Outdated
Copilot AI balanced review requested due to automatic review settings October 14, 2026 16:36

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: afad16e30a

ℹ️ 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".

Comment thread internal/certstore/export_formats.go
Comment thread pkcs.go Outdated
Comment thread pkcs.go
Comment thread cmd/certkit/scan.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Two mandatory error-handling and error-message conventions remain violated.

3 open findings
1 resolved since last review

🧠 Review effort: Balanced

Accepted PKCS7 identifiers still allowed malformed nested BER values into
an unbounded recursive decoder. Validate and normalize the full structure
iteratively, bound nesting, and retain definite and indefinite BER support.

Keep default exports usable with CN-derived names. Preserve progress
cleanup errors and refresh progress through managed bundle operations.
Copilot AI balanced review requested due to automatic review settings October 14, 2026 17:31

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The implementation is bounded, consistently integrated, documented, and covered by focused regression tests.

0 open findings

3 resolved since last review

🧠 Review effort: Balanced

@danielewood
danielewood merged commit 8e0137e into main Oct 14, 2026
12 checks passed
@danielewood
danielewood deleted the fix/scan-stalls-and-default-formats branch October 14, 2026 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants