perf: parallelize trust verification and default to mozilla root store - #183
Conversation
On macOS, x509.Certificate.Verify with the system cert pool calls SecTrustEvaluateWithError — a blocking syscall that can take seconds per certificate for OCSP/CRL checks. With large certificate stores (2500+ certs), scan+export operations hung indefinitely because every certificate was verified sequentially against the system trust store. Three changes fix this: 1. Default TrustStore from "system" to "mozilla". The embedded Mozilla root pool uses pure-Go verification — no syscalls, no network I/O. This alone eliminates the hang for the common case. 2. Parallelize trust verification in ScanSummary, dump-certs, and countAIAUnresolvedIssuers. All mozilla checks fire concurrently via goroutines, then only certs that mozilla didn't trust fall through to the (slower) system trust check. 3. Add TrustStore label to VerifyChainTrustInput and debug-log every trust verification call with subject, store name, and result. This makes future performance diagnosis trivial with -l debug. Before: scan of ~2500 certs hung indefinitely (>10 minutes, killed) After: same scan completes in ~45 seconds Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR improves certificate trust verification performance by defaulting library verification to the embedded Mozilla root store (avoiding slow macOS system trust calls) and parallelizing trust checks in key scan/export paths, while adding a TrustStore label for per-check debug observability.
Changes:
- Change
DefaultOptions().TrustStoredefault from"system"to"mozilla"(breaking) and update docs/tests/changelog accordingly. - Add
TrustStorelabel toVerifyChainTrustInputand emit debug logs for each trust check. - Parallelize trust verification in scan summary and
--dump-certspaths (mozilla-first, system fallback).
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
bundle.go |
Adds TrustStore label + debug logs to trust verification; defaults trust store to Mozilla; concurrent trust checks for AIA unresolved issuer counting. |
bundle_test.go |
Updates DefaultOptions() test to expect "mozilla". |
internal/certstore/memstore.go |
Parallelizes trust verification in ScanSummary (mozilla-first, then system fallback). |
cmd/certkit/scan.go |
Parallelizes trust checks for --dump-certs filtering (mozilla-first, then system fallback). |
internal/verify.go |
Threads TrustStore label through trust checks for AIA-related verification. |
cmd/wasm/main.go |
Labels WASM trust checks as "mozilla". |
README.md |
Updates example text to reflect the new default trust store. |
CHANGELOG.md |
Documents breaking default change and performance improvements under Unreleased. |
web/package-lock.json |
Updates various web dev dependencies/lockfile entries. |
Files not reviewed (1)
- web/package-lock.json: Language not supported
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 690400c1f4
ℹ️ 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".
- Add semaphore (runtime.NumCPU) for system trust goroutines to avoid overwhelming macOS SecTrust with unbounded concurrent syscalls - Guard VerifyChainTrust against nil cert before debug logging - Retry bundle export with system trust store when mozilla fails, so certs trusted only by the host OS (corporate roots) export without requiring --force - Fix golangci-lint modernize: use atomic.Int32 in verify_test.go Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 483bb63003
ℹ️ 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 retryErr := certstore.ExportMatchedBundles(ctx, exportInput); retryErr == nil { | ||
| continue | ||
| } | ||
| } | ||
| if input.Opts.Verify && isBundleVerificationError(err) { |
There was a problem hiding this comment.
When the initial Mozilla export fails verification and the system-store retry hits a different error, retryErr is discarded and execution falls through to the original err, which is still classified as a verification failure. In that case scan --bundle-path silently skips a system-trusted bundle as “untrusted” instead of surfacing the real failure (for example, if loading the system pool or writing the bundle files fails after the retry gets past verification).
Useful? React with 👍 / 👎.
| func exportMatchedBundleWithSystemFallback( | ||
| ctx context.Context, | ||
| commonName string, | ||
| exportInput certstore.ExportMatchedBundleInput, | ||
| opts certkit.BundleOptions, | ||
| exportFn func(context.Context, certstore.ExportMatchedBundleInput) error, | ||
| ) (bool, error) { | ||
| err := exportFn(ctx, exportInput) | ||
| if err == nil { | ||
| return false, nil | ||
| } | ||
|
|
||
| // If mozilla verification failed, retry with system trust store so | ||
| // certificates trusted only by the host OS (e.g. corporate keychain | ||
| // roots) still export without requiring --force. | ||
| if opts.Verify && isBundleVerificationError(err) && opts.TrustStore != "system" { | ||
| slog.Debug("mozilla trust failed, retrying with system trust store", "cn", commonName) | ||
| systemOpts := opts | ||
| systemOpts.TrustStore = "system" | ||
| exportInput.BundleOpts = systemOpts | ||
| retryErr := exportFn(ctx, exportInput) | ||
| if retryErr == nil { | ||
| return false, nil | ||
| } | ||
| if !isBundleVerificationError(retryErr) { | ||
| return false, fmt.Errorf("exporting bundle for %q: %w", commonName, retryErr) | ||
| } | ||
| } | ||
|
|
||
| if opts.Verify && isBundleVerificationError(err) { | ||
| slog.Debug("skipping untrusted bundle candidate", "cn", commonName, "error", err) | ||
| return true, nil | ||
| } | ||
|
|
||
| return false, fmt.Errorf("exporting bundle for %q: %w", commonName, err) |
There was a problem hiding this comment.
This helper takes 4 non-context parameters (commonName, exportInput, opts, exportFn), which violates CS-5 and makes the call sites harder to keep consistent (exportInput.BundleOpts vs opts can diverge). Consider introducing a small input struct (ctx stays separate) and/or deriving opts from exportInput.BundleOpts so there is a single source of truth.
| func exportMatchedBundleWithSystemFallback( | |
| ctx context.Context, | |
| commonName string, | |
| exportInput certstore.ExportMatchedBundleInput, | |
| opts certkit.BundleOptions, | |
| exportFn func(context.Context, certstore.ExportMatchedBundleInput) error, | |
| ) (bool, error) { | |
| err := exportFn(ctx, exportInput) | |
| if err == nil { | |
| return false, nil | |
| } | |
| // If mozilla verification failed, retry with system trust store so | |
| // certificates trusted only by the host OS (e.g. corporate keychain | |
| // roots) still export without requiring --force. | |
| if opts.Verify && isBundleVerificationError(err) && opts.TrustStore != "system" { | |
| slog.Debug("mozilla trust failed, retrying with system trust store", "cn", commonName) | |
| systemOpts := opts | |
| systemOpts.TrustStore = "system" | |
| exportInput.BundleOpts = systemOpts | |
| retryErr := exportFn(ctx, exportInput) | |
| if retryErr == nil { | |
| return false, nil | |
| } | |
| if !isBundleVerificationError(retryErr) { | |
| return false, fmt.Errorf("exporting bundle for %q: %w", commonName, retryErr) | |
| } | |
| } | |
| if opts.Verify && isBundleVerificationError(err) { | |
| slog.Debug("skipping untrusted bundle candidate", "cn", commonName, "error", err) | |
| return true, nil | |
| } | |
| return false, fmt.Errorf("exporting bundle for %q: %w", commonName, err) | |
| type exportMatchedBundleWithSystemFallbackInput struct { | |
| CommonName string | |
| ExportInput certstore.ExportMatchedBundleInput | |
| ExportFn func(context.Context, certstore.ExportMatchedBundleInput) error | |
| } | |
| func exportMatchedBundleWithSystemFallback( | |
| ctx context.Context, | |
| input exportMatchedBundleWithSystemFallbackInput, | |
| ) (bool, error) { | |
| err := input.ExportFn(ctx, input.ExportInput) | |
| if err == nil { | |
| return false, nil | |
| } | |
| opts := input.ExportInput.BundleOpts | |
| // If mozilla verification failed, retry with system trust store so | |
| // certificates trusted only by the host OS (e.g. corporate keychain | |
| // roots) still export without requiring --force. | |
| if opts.Verify && isBundleVerificationError(err) && opts.TrustStore != "system" { | |
| slog.Debug("mozilla trust failed, retrying with system trust store", "cn", input.CommonName) | |
| systemOpts := opts | |
| systemOpts.TrustStore = "system" | |
| input.ExportInput.BundleOpts = systemOpts | |
| retryErr := input.ExportFn(ctx, input.ExportInput) | |
| if retryErr == nil { | |
| return false, nil | |
| } | |
| if !isBundleVerificationError(retryErr) { | |
| return false, fmt.Errorf("exporting bundle for %q: %w", input.CommonName, retryErr) | |
| } | |
| } | |
| if opts.Verify && isBundleVerificationError(err) { | |
| slog.Debug("skipping untrusted bundle candidate", "cn", input.CommonName, "error", err) | |
| return true, nil | |
| } | |
| return false, fmt.Errorf("exporting bundle for %q: %w", input.CommonName, err) |
| // If mozilla verification failed, retry with system trust store so | ||
| // certificates trusted only by the host OS (e.g. corporate keychain | ||
| // roots) still export without requiring --force. | ||
| if opts.Verify && isBundleVerificationError(err) && opts.TrustStore != "system" { | ||
| slog.Debug("mozilla trust failed, retrying with system trust store", "cn", commonName) | ||
| systemOpts := opts | ||
| systemOpts.TrustStore = "system" | ||
| exportInput.BundleOpts = systemOpts |
There was a problem hiding this comment.
The comment says this retry is for when mozilla verification failed, but the condition only checks opts.TrustStore != "system". If this helper is ever called with an explicit non-mozilla trust store (e.g., "custom" in future), it would silently fall back to system roots and potentially export bundles that the caller did not intend to trust. Consider gating the retry on opts.TrustStore == "mozilla" (or otherwise making the intended behavior explicit).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7aee93f7c
ℹ️ 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".
| wg.Add(1) | ||
| go func(idx int, cert *x509.Certificate) { | ||
| defer wg.Done() | ||
| trustResults[idx].mozilla = certkit.VerifyChainTrust(certkit.VerifyChainTrustInput{ | ||
| Cert: cert, | ||
| Roots: input.MozillaPool, | ||
| Intermediates: intermediatePool, | ||
| TrustStore: "mozilla", | ||
| }) |
There was a problem hiding this comment.
In ScanSummary this new Mozilla phase starts one goroutine per non-expired certificate with no semaphore, unlike the bounded system phase immediately below. On the large stores this change is targeting (for example concatenated PEM dumps with tens of thousands of certs), that means tens of thousands of live goroutines before any verification completes, which can add hundreds of MB of stack/scheduler overhead and turn the “speedup” into an OOM/GC-thrash regression. The same unbounded pattern also appears in cmd/certkit/scan.go's --dump-certs precompute path.
Useful? React with 👍 / 👎.
Summary
TrustStoreinDefaultOptions()changed from"system"to"mozilla"— eliminates multi-minute hangs on macOS when scanning large certificate stores--dump-certs, and AIA resolution using goroutinesTrustStorelabel toVerifyChainTrustInputwith debug logging for every trust checkThe Bug
On macOS,
x509.Certificate.Verifywith the system cert pool calls into the Security framework'sSecTrustEvaluateWithError— a blocking syscall that performs OCSP/CRL network checks and can take seconds per certificate. The scan command was verifying every certificate sequentially against the system trust store:With ~2500 certificates and ~134,000 AIA resolution trust checks, the scan would hang indefinitely (killed after 10+ minutes with zero output).
Root Cause
Two compounding issues:
DefaultOptions()usedTrustStore: "system"— everyBundle()call during export verified certs against macOS SecTrust, which does network I/O per certScanSummary,countAIAUnresolvedIssuers, or--dump-certsThe Fix
1. Default to Mozilla (pure Go, no syscalls)
The embedded Mozilla root pool uses Go's
x509.Verifywith an in-memory cert pool — no system calls, no network I/O. This alone eliminates the hang.2. Mozilla-first short-circuit
System trust checks now only run for certs that Mozilla didn't trust. Since most valid certs are Mozilla-trusted, this skips the expensive syscall for the majority:
3. Parallel verification
All trust checks within a phase fire concurrently via goroutines:
4. Debug observability
Every
VerifyChainTrustcall now logs subject, store name, and result at debug level:Results
Breaking Change
DefaultOptions().TrustStoreis now"mozilla"instead of"system". Library callers that relied on the system trust store as default should explicitly setopts.TrustStore = "system".Test plan
pre-commit run --all-files)DefaultOptions()test updated for new defaultcertkit scan <dir> --bundle-path <out> -l debugcompletes in ~45s with correct output🤖 Generated with Claude Code