Skip to content

feat: add trust and expiry annotations to scan, inspect, and WASM - #57

Merged
danielewood merged 12 commits into
mainfrom
feat/trust-annotations
Feb 21, 2026
Merged

feat: add trust and expiry annotations to scan, inspect, and WASM#57
danielewood merged 12 commits into
mainfrom
feat/trust-annotations

Conversation

@danielewood

@danielewood danielewood commented Feb 21, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Add trust status (trusted/untrusted/self-signed) and expiry annotations to scan summary, inspect output, and WASM API
  • Harden AIA fetchers against SSRF and context cancellation with URL validation, timeouts, and redirect limits
  • Fix expired+untrusted double-counting in scan summary and return defensive copies from AllKeys
  • Strengthen trust/expiry assertions across all test suites
  • Fix error handling (ERR-1/ERR-5), WASM null-safety, and deadlock risks found by adversarial review
  • Breaking: refactor VerifyChainTrust to accept VerifyChainTrustInput struct (CS-5 compliance)
  • Document accepted risks for DNS rebinding SSRF and shared MozillaRootSubjects map

Commits

  • feat: add trust and expiry annotations to scan summary, inspect, and WASM
  • fix: strengthen trust verification, fix WASM race and error handling
  • fix: harden AIA fetchers against SSRF and context cancellation
  • fix: return copy from AllKeys, extract certAnnotation, add --out-file completion
  • test: strengthen trust/expiry assertions across all test suites
  • fix: prevent expired+untrusted double-counting in scan summary
  • chore: add missing PR refs to CHANGELOG entries
  • fix: close SSRF bypass, WASM panic, callback leak, and test gaps
  • fix: harden SSRF, WASM deadlock, memory leaks, and nil-roots fallback
  • fix: harden error handling, WASM safety, and test coverage gaps
  • docs: document accepted risks for DNS rebinding and shared map
  • fix: address review findings — CS-5, ERR-1, ERR-5 compliance

Test plan

  • go test -race ./... passes
  • go vet ./... passes
  • golangci-lint run passes
  • WASM build compiles (GOOS=js GOARCH=wasm go build ./cmd/wasm/)
  • certkit scan shows trust/expiry annotations in both text and JSON output
  • certkit inspect shows trust annotations
  • Expired and untrusted certs are not double-counted in scan summary

🤖 Generated with Claude Code

danielewood and others added 7 commits February 21, 2026 12:34
…WASM

Scan summary now shows expired and untrusted counts per cert type
(e.g., "Leaves: 6 (2 expired, 1 untrusted)"). Missing intermediates
are resolved via AIA before trust checking. Inspect output now includes
Expired and Trusted fields for each certificate.

Key changes:
- Add IsMozillaRoot() to detect cross-signed roots whose signing CA
  was removed from the trust store (e.g., GTS Root R1 × GlobalSign)
- Add HasUnresolvedIssuers() to conditionally trigger AIA resolution
- Add ScanSummaryInput with RootPool and AllowExpired parameters
- Add AnnotateInspectTrust() for per-certificate trust annotation
- Fix WASM getState() to use ExtKeyUsageAny and handle cross-signed
  roots and expired cert time-shifting consistently with CLI
- Replace fmt.Fprintf(os.Stderr) diagnostics with slog (OBS-1)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extract shared VerifyChainTrust function to eliminate triplicated chain
verification logic across scan summary, inspect, WASM, and --dump-certs.
Strengthen IsMozillaRoot with public key verification to prevent spoofed
trust anchors. Use NotBefore+1s time-shift for expired cert verification
(more robust when intermediates expired before the leaf).

Key fixes:
- WASM globalStore race condition (add sync.RWMutex)
- WASM MozillaRootPool error silently ignored
- --dump-certs missing ExtKeyUsageAny and IsMozillaRoot bypass
- Root certs always trust-checked regardless of --allow-expired
- Bare return err in inspect command (ERR-1)

Tests: IsMozillaRoot (genuine root, spoofed subject, unrelated cert),
VerifyChainTrust (trusted chain, expired time-shift, untrusted, Mozilla
root bypass), HasUnresolvedIssuers (all skip conditions), positive trust
case in AnnotateInspectTrust, strengthened ScanSummaryTrust.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add ValidateAIAURL to reject non-HTTP schemes and literal private/loopback
IP addresses. Apply to library FetchAIACertificates, CLI httpAIAFetcher,
and redirect handlers (max 3 redirects). Fix WASM jsFetchURL to honor
context cancellation via select on ctx.Done().

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… completion

AllKeys() now returns a copy of the internal map (like AllCerts()) so
callers cannot corrupt store state. Extract certAnnotation to
internal.CertAnnotation for testability. Add fileCompletion for
--out-file flag on the bundle command.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Expand test coverage for trust and expiry annotations across packages:

- ScanSummaryTrust: assert all 6 fields, add expired root case
- AnnotateInspectTrust: add chain case exercising intermediate pool
- HasUnresolvedIssuers: add Mozilla-root-issued leaf case
- VerifyChainTrust: add not-yet-valid leaf case, document expired
  intermediate limitation in doc comment
- EmptyStore: add trust/expiry zero-checks

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Expired certificates were counted as both expired and untrusted,
misleading users about the number of actual trust issues. Now expired
certs are only counted as expired — trust checking is skipped for them
regardless of the AllowExpired setting.

Remove the now-unused AllowExpired field from ScanSummaryInput.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add ([#56]) refs to 7 unreleased entries that were missing them.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings February 21, 2026 17:34

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.

Pull request overview

This PR adds trust status (trusted/untrusted/self-signed) and expiry annotations to scan summary, inspect output, and the WASM API. It also hardens AIA fetchers against SSRF attacks and context cancellation, fixes a race condition in WASM, and prevents double-counting of expired certificates in scan summaries.

Changes:

  • Add Expired and Trusted fields to InspectResult and extend ScanSummary with expired/untrusted counts for roots, intermediates, and leaves
  • Implement VerifyChainTrust for consistent chain verification and IsMozillaRoot with public key verification to prevent spoofed trust anchors
  • Add ValidateAIAURL to block SSRF via non-HTTP schemes and literal private/loopback IP addresses, with redirect validation and context cancellation support in WASM jsFetchURL
  • Fix AllKeys() to return a defensive copy using maps.Copy and add sync.RWMutex to protect WASM globalStore from concurrent access
  • Integrate AIA resolution into scan command before trust checking and fix expired certificate double-counting logic in ScanSummary
  • Add comprehensive test coverage including TestAnnotateInspectTrust, TestMemStore_ScanSummaryTrust, TestHasUnresolvedIssuers, TestIsMozillaRoot, TestVerifyChainTrust, and TestValidateAIAURL

Reviewed changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
internal/inspect.go Add Expired and Trusted pointer fields to InspectResult, add unexported cert field for trust annotation, implement AnnotateInspectTrust function
internal/inspect_test.go Add TestAnnotateInspectTrust covering self-signed, expired, Mozilla roots, and intermediate chains
internal/format.go New file with CertAnnotation helper for formatting trust/expiry counts in scan summary
internal/format_test.go New file with TestCertAnnotation covering all four code paths
internal/certstore/summary.go Add ScanSummaryInput struct and extend ScanSummary with 6 new count fields for expired/untrusted certificates
internal/certstore/memstore.go Update ScanSummary to accept ScanSummaryInput, add trust/expiry checking logic, fix AllKeys to return defensive copy
internal/certstore/memstore_test.go Add TestMemStore_ScanSummaryTrust and TestMemStore_AllKeys_ReturnsCopy
internal/certstore/testhelpers_test.go Add newExpiredRoot test helper function
internal/certstore/sqlite_test.go Update ScanSummary call to pass ScanSummaryInput{}
internal/certstore/aia.go Add HasUnresolvedIssuers function to gate AIA resolution
internal/certstore/aia_test.go Add TestHasUnresolvedIssuers with comprehensive coverage
cmd/certkit/scan.go Integrate AIA resolution before trust checking, add httpAIAFetcher and aiaHTTPClient with SSRF protection, update summary output formatting
cmd/certkit/inspect.go Call AnnotateInspectTrust before filtering, simplify expired check using r.Expired field
cmd/certkit/completions.go Add fileCompletion helper
cmd/certkit/bundle.go Update warning output to use slog.Warn, add --out-file completion
cmd/wasm/store.go Add storeMu sync.RWMutex to protect globalStore
cmd/wasm/main.go Add mutex protection to addFiles, getState, exportBundlesJS, resetStore, fix error handling in getState
cmd/wasm/aia.go Update jsFetchURL to respect context cancellation
bundle.go Add IsMozillaRoot, VerifyChainTrust, ValidateAIAURL functions with SSRF hardening
bundle_test.go Update test to use localhost instead of 127.0.0.1 to pass SSRF validation
certkit_test.go Add TestIsMozillaRoot, TestVerifyChainTrust, TestValidateAIAURL
CHANGELOG.md Document all changes with PR references

Comment thread cmd/certkit/scan.go
Comment thread cmd/certkit/scan.go
- Block unspecified addresses (0.0.0.0, ::) in ValidateAIAURL SSRF check
- Avoid releasing js.Func callbacks on context cancellation in jsFetchURL
  to prevent panic when JS promise settles after Go cancels
- Release setTimeout callback in addFiles to prevent per-call leak
- Add test cases for IPv6 ULA, unspecified addresses, untrusted
  intermediate counting in ScanSummary
- Fix silent error suppression in TestAnnotateInspectTrust by reusing
  newIntermediateCA helper
- Use atomic.Int32 for fetchCount in AIA tests (consistency with
  bundle_test.go)
- Use parsed certs instead of templates as x509.CreateCertificate parents
  in AIA max depth test

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@claude

This comment has been minimized.

@claude

This comment has been minimized.

@claude

This comment has been minimized.

@claude

This comment has been minimized.

Adversarial review found seven issues in the trust-annotations PR:

- ValidateAIAURL re-parsed CIDR ranges on every call and missed
  100.64.0.0/10 (CGN/shared address space). Move to package init.
- ResolveAIA passed raw AIA URLs to the fetcher without SSRF
  validation — the WASM jsFetchURL path had no URL checks at all.
- VerifyChainTrust silently fell back to system roots when roots
  was nil (x509.VerifyOptions behavior). Now returns false.
- WASM getState/resetStore blocked the JS event loop when AIA held
  the store lock, causing a deadlock. Use TryRLock/TryLock instead.
- WASM jsFetchURL accepted unbounded response data. Add 1MB limit.
- WASM addFiles/exportBundlesJS/jsError leaked js.FuncOf promise
  executors. Release immediately after Promise.New (called sync).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings February 21, 2026 17:59

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.

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.

@claude

This comment has been minimized.

Adversarial review found ERR-1/ERR-5 violations, a WASM catch callback
null-safety issue, a missing defer on RUnlock, and several untested code
paths. This fixes all production code issues and adds targeted tests.

Production fixes:
- Wrap bare return err in scan.go MozillaRootPool and httpAIAFetcher (ERR-1)
- Add slog.Debug for silent continues in mozillaRootPublicKeys and ResolveAIA (ERR-5)
- Log json.Marshal errors in WASM instead of silently discarding (ERR-5)
- Guard WASM catchCb against null/non-Error JS rejections
- Use defer for RUnlock in exportBundlesJS to prevent deadlock on panic
- Add context prefix to WASM AIA fetch error messages

Test additions:
- ResolveAIA SSRF URL rejection (loopback, private, file scheme)
- VerifyChainTrust with nil intermediates pool and expired intermediate
- ScanSummary nil-pool with expired certs
- Strengthen ValidateAIAURL empty-scheme assertion

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@claude

This comment has been minimized.

@claude

This comment has been minimized.

Explain why ValidateAIAURL intentionally allows hostnames that resolve
to private IPs (short-lived CLI, internal CA support, Control callback
has same TOCTOU) and why MozillaRootSubjects returns the backing map
directly (all callers read-only, defensive copy wastes allocations).

Both were flagged in a prior adversarial review and accepted as known
limitations.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings February 21, 2026 18:29
- Refactor VerifyChainTrust to accept VerifyChainTrustInput struct
  (CS-5: more than 2 arguments requires input struct)
- Wrap bare io.ReadAll return in httpAIAFetcher with context (ERR-1)
- Reject WASM addFiles promise on json.Marshal failure instead of
  resolving with empty string (ERR-5)
- Add slog.Debug for silent continue in MozillaRootSubjects (ERR-5)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

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.

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.

Comment thread cmd/certkit/scan.go
@danielewood
danielewood merged commit 47d7811 into main Feb 21, 2026
16 checks passed
@danielewood
danielewood deleted the feat/trust-annotations branch February 21, 2026 18:45
@claude

claude Bot commented Feb 21, 2026

Copy link
Copy Markdown

test

@claude

claude Bot commented Feb 21, 2026

Copy link
Copy Markdown

Code review

Found 4 issues: 1 CLAUDE.md compliance violation and 3 bugs.


API-1 (MUST) — VerifyChainTrust missing godoc comment

File: bundle.go:230

The comment block at lines 211–223 is a single contiguous // block. The final line, // VerifyChainTrustInput holds parameters for VerifyChainTrust., has no blank line between it and type VerifyChainTrustInput struct — so the entire block attaches to the struct, not to func VerifyChainTrust. The function has no godoc comment.

Violates CLAUDE.md API-1: "Exported functions: doc comment required (godoc style). No exceptions."

Fix: move VerifyChainTrustInput before the function's doc comment (satisfying CS-6 as well), giving each declaration its own godoc block:

// VerifyChainTrustInput holds parameters for VerifyChainTrust.
type VerifyChainTrustInput struct {
	Cert          *x509.Certificate
	Roots         *x509.CertPool
	Intermediates *x509.CertPool
}

// VerifyChainTrust reports whether the given certificate chains to a trusted
// root. Cross-signed roots (same Subject and public key as a Mozilla root)
// are trusted directly. For expired certificates, verification is performed
// at a time just after the certificate's NotBefore to determine if the chain
// was ever valid — this is more robust than checking just before NotAfter,
// because intermediates that expired between issuance and the leaf's expiry
// will still be valid at NotBefore time.
//
// Known limitation: if an intermediate expired before the leaf's NotBefore,
// the time-shifted verification will still fail because the intermediate is
// invalid at the leaf's issuance time. This is an uncommon edge case in
// practice (intermediates outlive the leaves they sign).
func VerifyChainTrust(input VerifyChainTrustInput) bool {

Bug — Non-deferred storeMu.Unlock() in addFiles (permanent deadlock on panic)

File: cmd/wasm/main.go:65

storeMu.Lock() at line 65 is released with a plain storeMu.Unlock() at line 92 — no defer. The intervening loop accesses JS values (filesArg.Index(i), file.Get("name"), file.Get("data"), js.CopyBytesToGo) and calls certstore.ProcessData on untrusted input. A panic in any of these permanently deadlocks the store — all future calls to getState, exportBundlesJS, resetStore, and addFiles will block forever.

This PR fixed the same class of bug in exportBundlesJS (defer storeMu.RUnlock() at line 272), but addFiles was not updated consistently.

Fix: replace the plain Unlock() with defer storeMu.Unlock() immediately after the lock acquisition and remove the explicit call at line 92.


Bug — Non-deferred storeMu.Unlock() in AIA goroutine (permanent deadlock on panic)

File: cmd/wasm/main.go:110

Same pattern: storeMu.Lock() at line 110, plain Unlock() at line 112. resolveAIA fetches and parses untrusted certificate data from the network; a panic inside it permanently deadlocks the store.

Fix: use defer storeMu.Unlock() immediately after the lock acquisition and remove the explicit Unlock() at line 112.


ERR-6 (MUST) — Missing return after json.Marshal error

File: cmd/wasm/main.go:118-131

When json.Marshal(warnings) fails, warnJSON is nil and the error is logged. Execution then falls through: the setTimeout callback is still registered and onComplete.Invoke(string(warnJSON)) is called with string(nil) == "", delivering an empty string instead of valid JSON to the JS caller.

Violates CLAUDE.md ERR-6: "Fail fast — return errors immediately, don't accumulate them."

Fix:

warnJSON, err := json.Marshal(warnings)
if err != nil {
	slog.Error("marshaling AIA warnings", "error", err)
	return
}

@claude

claude Bot commented Feb 21, 2026

Copy link
Copy Markdown

test

@claude

claude Bot commented Feb 21, 2026

Copy link
Copy Markdown

Code review

Found 2 issues (both CLAUDE.md MUST violations).

Checked for: bugs, CLAUDE.md compliance (API-1, CS-5, CS-6, ERR-1, ERR-5, ERR-6, CC-3, T-2, T-9, T-11, T-12, CL-1–CL-6, CLI-1–CLI-7).


Issue 1 — API-1 (MUST): VerifyChainTrust has no godoc comment

File: bundle.go

Rule: API-1 (MUST)"Exported functions: doc comment required (godoc style). No exceptions."

The intended doc comment for VerifyChainTrust (the long block starting with // VerifyChainTrust reports whether…) is not separated by a blank line from // VerifyChainTrustInput holds parameters for VerifyChainTrust.. In Go, a contiguous block of // comment lines immediately preceding a declaration becomes that declaration's godoc comment — so the entire block (including the VerifyChainTrust description) becomes the godoc for type VerifyChainTrustInput. The function itself ends up with no godoc.

Current (broken):

// VerifyChainTrust reports whether the given certificate chains to a trusted
// root. ...
//
// Known limitation: ...
// practice (intermediates outlive the leaves they sign).
// VerifyChainTrustInput holds parameters for VerifyChainTrust.  ← no blank line before this
type VerifyChainTrustInput struct { ... }

func VerifyChainTrust(input VerifyChainTrustInput) bool {  ← no godoc

Fix: place the short struct comment above the type declaration, add a blank line after it, then put the long VerifyChainTrust description immediately before the function:

// VerifyChainTrustInput holds parameters for VerifyChainTrust.
type VerifyChainTrustInput struct {
	Cert          *x509.Certificate
	Roots         *x509.CertPool
	Intermediates *x509.CertPool
}

// VerifyChainTrust reports whether the given certificate chains to a trusted
// root. Cross-signed roots (same Subject and public key as a Mozilla root)
// are trusted directly. ...
func VerifyChainTrust(input VerifyChainTrustInput) bool {

Issue 2 — CL-3/CL-4 (MUST): commit ref b5969b0 does not exist in this PR

File: CHANGELOG.md

Rules: CL-3/CL-4 (MUST)"Each entry ends with a commit ref. Use the short (7-char) commit SHA."

Three changelog entries and the link definition at the bottom reference [b5969b0], but none of the 12 commits in this PR have a SHA starting with b5969b0. The PR's commits are: 92f1da0, 070ee62, 8491ddc, 15363ef, 129177f, ffe3170, d8702ae, e1a9b0f, 734e4f9, eb244b0, dd090b5, bb66119.

Fix: replace b5969b0 (in the three entry lines and the link definition at the bottom of CHANGELOG.md) with the correct short SHA of the commit that introduced the trust/expiry annotation feature.

danielewood added a commit that referenced this pull request Feb 23, 2026
Fix three panic-deadlock bugs in WASM (non-deferred mutex unlocks in
addFiles and AIA goroutine), an ERR-6 fall-through after json.Marshal
failure, a godoc mis-attachment on VerifyChainTrust, and three changelog
entries referencing a non-existent commit SHA.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
danielewood added a commit that referenced this pull request Feb 23, 2026
#58)

Fix three panic-deadlock bugs in WASM (non-deferred mutex unlocks in
addFiles and AIA goroutine), an ERR-6 fall-through after json.Marshal
failure, a godoc mis-attachment on VerifyChainTrust, and three changelog
entries referencing a non-existent commit SHA.

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
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