feat: add trust and expiry annotations to scan, inspect, and WASM - #57
Conversation
…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>
There was a problem hiding this comment.
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
ExpiredandTrustedfields toInspectResultand extendScanSummarywith expired/untrusted counts for roots, intermediates, and leaves - Implement
VerifyChainTrustfor consistent chain verification andIsMozillaRootwith public key verification to prevent spoofed trust anchors - Add
ValidateAIAURLto block SSRF via non-HTTP schemes and literal private/loopback IP addresses, with redirect validation and context cancellation support in WASMjsFetchURL - Fix
AllKeys()to return a defensive copy usingmaps.Copyand addsync.RWMutexto protect WASMglobalStorefrom 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, andTestValidateAIAURL
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 |
- 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>
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.
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>
This comment has been minimized.
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>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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>
- 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>
|
test |
Code reviewFound 4 issues: 1 CLAUDE.md compliance violation and 3 bugs. API-1 (MUST) —
|
|
test |
Code reviewFound 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):
|
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>
#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>
Summary
AllKeysVerifyChainTrustto acceptVerifyChainTrustInputstruct (CS-5 compliance)MozillaRootSubjectsmapCommits
feat: add trust and expiry annotations to scan summary, inspect, and WASMfix: strengthen trust verification, fix WASM race and error handlingfix: harden AIA fetchers against SSRF and context cancellationfix: return copy from AllKeys, extract certAnnotation, add --out-file completiontest: strengthen trust/expiry assertions across all test suitesfix: prevent expired+untrusted double-counting in scan summarychore: add missing PR refs to CHANGELOG entriesfix: close SSRF bypass, WASM panic, callback leak, and test gapsfix: harden SSRF, WASM deadlock, memory leaks, and nil-roots fallbackfix: harden error handling, WASM safety, and test coverage gapsdocs: document accepted risks for DNS rebinding and shared mapfix: address review findings — CS-5, ERR-1, ERR-5 complianceTest plan
go test -race ./...passesgo vet ./...passesgolangci-lint runpassesGOOS=js GOARCH=wasm go build ./cmd/wasm/)certkit scanshows trust/expiry annotations in both text and JSON outputcertkit inspectshows trust annotations🤖 Generated with Claude Code