fix: default readonly trust to mozilla and fix export csr subjects - #195
Conversation
Use explicit Mozilla-first trust-store selection for scan, verify, inspect, and connect, stop exporting unmatched bundle directories, preserve CSR common names in PEM and JSON exports, and include the dependency updates from the repository pre-commit hooks. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR shifts the CLI’s default trust behavior to Mozilla roots (unless --trust-store system is selected), tightens bundle export so only configured bundles are emitted, and fixes CSR exports to preserve the certificate Common Name in PEM/JSON outputs. It also includes routine dependency lockfile/module updates.
Changes:
- Default
scan,verify,inspect, andconnecttrust evaluation to Mozilla roots and plumb--trust-storethrough relevant code paths. - Prevent exporting bundle directories for certificates that don’t match any configured bundle entry.
- Preserve CSR
subject.common_namein generated CSR artifacts (PEM/JSON) and add/adjust tests accordingly.
Reviewed changes
Copilot reviewed 20 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
web/package-lock.json |
Updates frontend dependency lockfile entries. |
internal/verify.go |
Adjusts trust-source selection and passes selected trust store into chain display/trust anchor reporting. |
internal/verify_test.go |
Updates expectations to match Mozilla-only default trust attempts. |
internal/inspect.go |
Adds trust-store parameter to trust annotation and forwards it into trust anchor checks. |
internal/inspect_test.go |
Updates tests for new AnnotateInspectTrust signature. |
internal/exporter.go |
Stops assigning fallback bundle names for unmatched certs; adds (optional) system fallback retry logic for unknown-authority failures. |
internal/exporter_test.go |
Expands coverage for system fallback retry behavior and unmatched bundle name behavior. |
internal/certstore/export.go |
Preserves CSR Common Name in CSR subject and includes it in generated CSR JSON. |
internal/certstore/export_test.go |
Adds assertions ensuring CSR CN is preserved in CSR and CSR JSON. |
cmd/wasm/inspect.go |
Updates WASM inspect trust annotation call to pass Mozilla trust store explicitly. |
cmd/certkit/truststore.go |
Adds helper to load the selected trust pool (Mozilla vs system). |
cmd/certkit/scan.go |
Adds --trust-store flag; uses selected trust pool for summaries/exports. |
cmd/certkit/readonly_commands_test.go |
Adds regression test ensuring scan JSON summary uses Mozilla trust only by default. |
cmd/certkit/inspect.go |
Adds --trust-store flag and forwards it to trust annotation. |
cmd/certkit/verify.go |
Adds --trust-store flag and forwards it into verification inputs/docs. |
cmd/certkit/connect.go |
Adds --trust-store flag; loads selected roots and uses them for TLS verification and trust anchor reporting. |
cmd/certkit/connect_test.go |
Updates trust status helper invocation to include trust store. |
bundle.go |
Extends trust anchor checking to honor a selected trust store and defaults to Mozilla when unspecified. |
README.md |
Documents new --trust-store flags for relevant commands. |
CHANGELOG.md |
Notes default trust-store change and related fixes; requires consistency updates with older Unreleased bullets. |
go.mod / go.sum |
Bumps Go module dependencies. |
Files not reviewed (1)
- web/package-lock.json: Language not supported
Comments suppressed due to low confidence (1)
cmd/certkit/scan.go:407
- The PR/CHANGELOG text mentions that system-trust fallback during
scan --bundle-pathis only used when explicitly enabled, butrunScanalways callsinternal.ExportBundleswithout settingAllowSystemFallbackand there is no CLI flag to turn it on. Either wire an explicit flag/config through toExportBundlesInput.AllowSystemFallback(and document it), or update the release notes to reflect that the fallback is now unavailable from the CLI.
if err := internal.ExportBundles(cmd.Context(), internal.ExportBundlesInput{
Configs: bundleConfigs,
OutDir: scanBundlePath,
Store: store,
TrustStore: scanTrustStore,
ForceBundle: scanForceExport,
Duplicates: scanDuplicates,
P12Password: p12Password,
EncryptKey: len(passwordSets.Export) > 0,
}); err != nil {
|
|
||
| - **Breaking:** Default `TrustStore` in `DefaultOptions()` changed from `"system"` to `"mozilla"` — pure-Go Mozilla root verification is used by default instead of macOS `SecTrustEvaluateWithError` syscalls, eliminating multi-minute hangs on large certificate stores | ||
| - Default `scan`, `verify`, `inspect`, and `connect` trust-store selection to Mozilla, and require an explicit `--trust-store system` to consult host trust roots | ||
| - Parallelize trust verification in scan summary, dump-certs, and AIA resolution — mozilla checks run concurrently, system checks only run for certs mozilla didn't trust |
There was a problem hiding this comment.
This bullet about running both Mozilla and system trust verification ("system checks only run for certs mozilla didn't trust") no longer matches the current scan/verify behavior after introducing --trust-store selection (loadScanTrustPools now returns only one pool). Please update/remove this entry (and any related Unreleased bullets) so the changelog reflects the new single-selected trust-store flow.
There was a problem hiding this comment.
Updated the Unreleased changelog entries to match the current selected-trust-store flow. The scan/export note now points users to --trust-store system when they want host-root validation, and the stale dual-store wording was removed in c3b60f3.
| func exportMatchedBundleWithSystemFallback( | ||
| ctx context.Context, | ||
| commonName string, | ||
| exportInput certstore.ExportMatchedBundleInput, | ||
| opts certkit.BundleOptions, | ||
| allowSystemFallback bool, | ||
| exportFn func(context.Context, certstore.ExportMatchedBundleInput) error, | ||
| ) (bool, error) { |
There was a problem hiding this comment.
CS-5: exportMatchedBundleWithSystemFallback now takes 6 parameters (ctx + 5). Per project guidelines, functions with more than 2 args should take an input struct (with context kept separate). Consider introducing an input struct (e.g., commonName/exportInput/opts/allowSystemFallback/exportFn) to keep the call sites readable and avoid further parameter creep.
There was a problem hiding this comment.
Addressed in c3b60f3. exportMatchedBundleWithSystemFallback now takes a dedicated input struct so it stays within CS-5 and the call site is easier to read.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0db6a5ee2
ℹ️ 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".
| } | ||
| } | ||
| return strings.ReplaceAll(cn, "*", "_") | ||
| return "" |
There was a problem hiding this comment.
Preserve matched bundle names when unmatched cert shares SKI
Returning "" for unmatched CNs interacts badly with AssignBundleNames: that loop calls store.SetBundleName(rec.SKI, name), which updates all certs for the same SKI. If one cert for a reused key matches config and another does not, whichever record is iterated last wins; with this change, an unmatched record can clear the matched record's bundle name and cause exports to be skipped nondeterministically (map iteration order from AllCertsFlat() is not stable).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. I changed AssignBundleNames to assign bundle names per certificate record instead of calling SetBundleName by SKI. That keeps an unmatched renewal on the same key out of the configured bundle while still exporting the latest certificate for the exact matched bundle. Added regression coverage in TestAssignBundleNames_IgnoresUnmatchedRenewalWithSameSKI in c3b60f3.
Refactor exporter fallback to use an input struct, assign bundle names per certificate record so same-SKI renewals do not override configured bundles, and align changelog text with the selected trust-store behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3b60f39c6
ℹ️ 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 := internal.AnnotateInspectTrust(results); err != nil { | ||
| if err := internal.AnnotateInspectTrust(results, inspectTrustStore); err != nil { |
There was a problem hiding this comment.
Validate inspect trust-store before running trust checks
The new --trust-store flag is passed straight into AnnotateInspectTrust without command-level validation, so invalid values (for example, a typo like --trust-store mozillaa) do not fail the command; instead, each cert is reported as untrusted with warnings and inspect still exits successfully. This can silently produce incorrect trust results in scripts that rely on exit status and JSON output.
Useful? React with 👍 / 👎.
Carry the selected cert record through bundle export so same-SKI renewals do not substitute a newer certificate from a different bundle. This keeps exact-bundle export aligned with expiry filtering and prevents expired bundles like dnsstg from borrowing a valid zimperium leaf. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| for _, rec := range store.AllCertsFlat() { | ||
| name := determineBundleName(rec.Cert.Subject.CommonName, configs) | ||
| store.SetBundleName(rec.SKI, name) | ||
| rec.BundleName = determineBundleName(rec.Cert.Subject.CommonName, configs) |
There was a problem hiding this comment.
AssignBundleNames mutates CertRecord.BundleName directly on pointers returned by MemStore.AllCertsFlat() without holding the MemStore mutex. This introduces a data race risk and violates CC-3 (shared state must be protected), especially since other code can read from the store concurrently. Consider adding a MemStore method that updates a specific CertRecord's BundleName (or performs the whole assignment) under lock, and use that here so bundle-name assignment remains per-record without bypassing synchronization.
| rec.BundleName = determineBundleName(rec.Cert.Subject.CommonName, configs) | |
| bundleName := determineBundleName(rec.Cert.Subject.CommonName, configs) | |
| store.UpdateBundleName(rec, bundleName) |
| case "", "mozilla": | ||
| pool, err := certkit.MozillaRootPool() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("loading Mozilla root pool: %w", err) |
There was a problem hiding this comment.
ERR-4: error strings should be lowercase. "loading Mozilla root pool" starts with a capitalized proper noun; consider using "loading mozilla root pool" (or otherwise aligning with the project’s lowercase error-string convention).
| return nil, fmt.Errorf("loading Mozilla root pool: %w", err) | |
| return nil, fmt.Errorf("loading mozilla root pool: %w", err) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1653a80725
ℹ️ 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".
| trustPools, err := scanValidationTrustPoolLoader(scanTrustStore) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Skip trust loading when force mode disables trust filtering
runScan loads the selected trust pool unconditionally for --dump-certs, but the subsequent trust checks are gated by !scanForceExport. This means certkit scan --dump-certs ... --force can still fail early on trust-store initialization (for example, --trust-store system when loading system roots errors) even though force mode is explicitly bypassing trust validation. Deferring trust-pool loading until the non-force path avoids a false failure mode.
Useful? React with 👍 / 👎.
| break | ||
| } | ||
| checkRoots("system", systemPool) | ||
| case "custom", "file": |
There was a problem hiding this comment.
CheckTrustAnchors treats TrustStore values "custom" and "file" as a no-op (no selected-roots check and no warning), even though the function comment says results are returned for the selected trust store. This can produce empty trust_anchors even when a certificate is trusted by custom roots. Consider either (a) adding a way to pass custom roots/pool into CheckTrustAnchorsInput and checking them here, or (b) returning a warning explaining that custom/file cannot be evaluated without an explicit roots pool.
| case "custom", "file": | |
| case "custom": | |
| result.Warnings = append(result.Warnings, "custom trust store cannot be evaluated without an explicit roots pool") | |
| case "file": | |
| if input.FileRoots == nil { | |
| result.Warnings = append(result.Warnings, "file trust store cannot be evaluated without file roots") | |
| } |
| // AnnotateInspectTrust sets the Expired, Trusted, and TrustAnchors fields on | ||
| // certificate results. Intermediate certificates found in the results are used | ||
| // to build chains. |
There was a problem hiding this comment.
The doc comment for AnnotateInspectTrust doesn’t mention the new trustStore parameter. Since trust evaluation now depends on the selected trust store, the comment should describe how trustStore affects the Trusted/TrustAnchors fields (and what happens for empty/unknown values).
| // AnnotateInspectTrust sets the Expired, Trusted, and TrustAnchors fields on | |
| // certificate results. Intermediate certificates found in the results are used | |
| // to build chains. | |
| // AnnotateInspectTrust sets the Expired, Trusted, TrustAnchors, and | |
| // TrustWarnings fields on certificate results. Intermediate certificates found | |
| // in the results are used to build chains. The trustStore parameter selects the | |
| // trust store used for trust evaluation, so Trusted and TrustAnchors reflect | |
| // anchors available from that store. If trustStore is empty, the default trust | |
| // store behavior of certkit.CheckTrustAnchors is used. If trustStore is | |
| // unknown or unsupported, no trust anchors are returned, Trusted is set to | |
| // false, and any related warnings are recorded in TrustWarnings. |
| // buildCSRSubject creates a pkix.Name for CSR generation from either the | ||
| // override or the existing certificate's subject fields. | ||
| func buildCSRSubject(cert *x509.Certificate, override *CSRSubjectOverride) pkix.Name { |
There was a problem hiding this comment.
buildCSRSubject now always preserves CommonName from the certificate (even when an override is provided), but the comment still implies the override fully replaces the certificate subject. Please update the comment to reflect the CN-preserving behavior so future changes don’t accidentally regress it.
| // ExportMatchedBundleInput holds parameters for ExportMatchedBundles. | ||
| type ExportMatchedBundleInput struct { | ||
| Store *MemStore | ||
| SKIs []string // matched-pair SKIs to export | ||
| Certs []*CertRecord | ||
| BundleOpts certkit.BundleOptions |
There was a problem hiding this comment.
ExportMatchedBundleInput now has both SKIs and Certs, but the struct comment doesn’t document how callers should choose between them (or what happens if both are set). Consider documenting the precedence (currently Certs wins) and/or validating that exactly one selector is provided to avoid ambiguous call sites.
| case "", "mozilla": | ||
| return scanTrustPools{Mozilla: pool}, nil | ||
| default: | ||
| return scanTrustPools{}, fmt.Errorf("%w: %q", errUnsupportedTrustStore, trustStore) | ||
| } |
There was a problem hiding this comment.
In loadScanTrustPools, the default branch in the switch trustStore is effectively unreachable because loadSelectedTrustPool(trustStore) already returns an unsupported trust store error for unknown values. Consider simplifying by switching/validating before loading the pool (or removing the redundant default/error path) to keep the control flow clearer.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57e434253e
ℹ️ 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".
| Configs: bundleConfigs, | ||
| OutDir: scanBundlePath, | ||
| Store: store, | ||
| TrustStore: scanTrustStore, |
There was a problem hiding this comment.
Enable unknown-authority fallback for scan bundle exports
exportMatchedBundleWithSystemFallback now retries with system roots only when AllowSystemFallback is true, but the scan --bundle-path call site never sets that field. As a result, the new retry path is unreachable in CLI exports and certificates that are trusted only by host-installed roots (common in enterprise PKI) are always skipped when --trust-store mozilla is used. This is a regression from the previous behavior where those bundles were retried against system trust instead of being dropped.
Useful? React with 👍 / 👎.
| if _, err := loadSelectedTrustPool(verifyTrustStore); err != nil { | ||
| return fmt.Errorf("loading trust store: %w", err) |
There was a problem hiding this comment.
Avoid failing verify before file-root trust can run
This eager loadSelectedTrustPool check aborts verify as soon as the selected built-in trust source fails to load, which prevents the later verification flow from trying --roots anchors. In environments where x509.SystemCertPool fails, certkit verify --trust-store system --roots <file> now exits immediately even though verifyTrustAnchors can otherwise proceed with file-backed trust and emit warnings for unavailable built-in sources.
Useful? React with 👍 / 👎.
|
@codex review |
| } | ||
| default: | ||
| result.Warnings = append(result.Warnings, fmt.Sprintf("unsupported trust store %q", trustStore)) | ||
| return result |
There was a problem hiding this comment.
CheckTrustAnchors returns early for unsupported TrustStore values, which prevents FileRoots from being evaluated even when input.FileRoots is provided. Consider recording the warning but continuing so file-backed roots can still produce a "file" anchor (or explicitly document that unsupported TrustStore disables all trust evaluation).
| return result |
| origSystemLoader := loadSystemCertPool | ||
| t.Cleanup(func() { | ||
| loadSystemCertPool = origSystemLoader | ||
| }) | ||
| loadSystemCertPool = func() (*x509.CertPool, error) { | ||
| return nil, errInjectedSystemTrustLoadFailure | ||
| } |
There was a problem hiding this comment.
This test stubs loadSystemCertPool, but runVerify/internal.VerifyCert do not use that loader (they call certkit.SystemCertPoolCached directly). As written, the stubbed failure is never exercised, so the test can pass even if system trust loading failures would still break verification. Consider either removing the stub and renaming the test to what it actually verifies (file roots work alongside --trust-store system), or refactoring the verify path to use an injectable system-pool loader so the failure mode can be tested.
| if err := os.MkdirAll(scanBundlePath, 0o755); err != nil { | ||
| return fmt.Errorf("creating output directory %s: %w", scanBundlePath, err) | ||
| } | ||
| if err := internal.ExportBundles(cmd.Context(), internal.ExportBundlesInput{ | ||
| Configs: bundleConfigs, | ||
| OutDir: scanBundlePath, | ||
| Store: store, | ||
| ForceBundle: scanForceExport, | ||
| Duplicates: scanDuplicates, | ||
| P12Password: p12Password, | ||
| EncryptKey: len(passwordSets.Export) > 0, | ||
| if err := scanExportBundles(cmd.Context(), internal.ExportBundlesInput{ | ||
| Configs: bundleConfigs, | ||
| OutDir: scanBundlePath, | ||
| Store: store, | ||
| TrustStore: scanTrustStore, | ||
| ForceBundle: scanForceExport, |
There was a problem hiding this comment.
scan now accepts --trust-store, but runScan does not validate scanTrustStore before creating the bundle output directory and attempting export. For invalid values this can leave partially-created output directories and defer failure until later code paths. Consider calling validateSelectedTrustStore(scanTrustStore) early (before os.MkdirAll / ExportBundles), consistent with verify/inspect.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a582e21545
ℹ️ 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".
| } | ||
| checkRoots("system", systemPool) | ||
| case "custom": | ||
| result.Warnings = append(result.Warnings, "custom trust store cannot be evaluated without an explicit roots pool") |
There was a problem hiding this comment.
Suppress custom-store warning when file roots are present
When CheckTrustAnchors is called with TrustStore: "custom" and a non-nil FileRoots pool (for example via VerifyCert with custom roots), this branch always adds the warning that no explicit roots pool was provided. The same call can still append "file" to trust_anchors, so output becomes self-contradictory (trust_anchors shows success while trust_warnings claims roots are missing), which can trigger false failures in warning-sensitive tooling.
Useful? React with 👍 / 👎.
Summary
--trust-store systemis setValidation