feat(testing/fipsscan): extend the fipsscan package to improve FIPS auto-audit - #438
feat(testing/fipsscan): extend the fipsscan package to improve FIPS auto-audit#438macdewee wants to merge 16 commits into
Conversation
…ault
Add named constants for eight additional non-FIPS crypto libraries
(jcmturner/{aescts,gofork,gokrb5}, xdg-go/pbkdf2, ProtonMail/go-crypto,
cloudflare/circl, Azure/go-ntlmssp, youmark/pkcs8, filippo.io/) alongside
the existing XCrypto constant.
Introduce CommonForbiddenPkgs as the canonical baseline list (including
XCrypto) and wire Scan to use it automatically, so callers scanning for
all known violations no longer need to pass the full list manually.
extraForbiddenPkgs remains for project-specific additions.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace the flat map[string]string knownViolations with map[string][]KnownViolation keyed by binary entry point. Each entry documents an accepted (Importer, Imported) pair; use "" as the key for library modules or cross-binary violations. Drop the Reason field in favour of a Go comment next to each entry. Also removes CheckViolations (superseded by CheckModule), extracts findViolations helper to deduplicate scan logic, and adds ViolationKey -> Violation.Binary field so violations carry their binary attribution as a plain string. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tection, and two correctness fixes
- Add Reason string to KnownViolation so it appears in test log output when
a known violation fires, not just as a source comment invisible in CI.
- Change pattern string to patterns []string in CheckModule and ScanBinaries
so callers can target specific binaries without scanning CGO-heavy packages.
- Scope stale detection to binaries found in the current scan, fixing false
stale errors when CheckModule is called per-binary in subtests.
- Fix findKnown double-scan: when v.Binary=="", []string{binary, ""} iterated
knownViolations[""] twice; now collapses to a single pass.
- Fix displayChain: replace append(chain, v.Imported) with an explicit make+copy
to avoid writing into chain's backing array if ShortestChain ever pre-allocates.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ies from scan Adds skipBinaries []string to CheckModule, ScanBinaries, and the internal scanBinaries. Binaries whose import path is in the skip list are excluded before BFS traversal, so they produce no violations and are not considered in stale detection. Useful for dev tools, scripts, and assets that are not part of the shipped FIPS-compliant binary set. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Change CheckModule signature: knownViolations map[string][]KnownViolation → map[string]map[string][]KnownViolation Outer key is the binary entry-point path (or ""); inner key is a component prefix — any package path or module root that must appear in the BFS chain from the binary to the importer. Using a module root (e.g. "github.com/twmb/franz-go") groups all subpackage violations under one readable entry, making it clear which library introduces each violation. Stale detection: an entry is stale when the binary was scanned, the component prefix is still reachable from it, but the (Importer, Imported) pair is no longer a violation. If the component is no longer reachable (dependency removed), its entries are silently skipped rather than reported as stale. Adds chainContains and isReachableFromAny helpers. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rt chain Show a concrete binary→importer→imported chain in the CheckModule doc comment and README bootstrapping section, with a table of all valid component key values at each level. Clarifies that the component key must match the importer chain, not the forbidden (Imported) package. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… the shortest chain Replace the BFS-chain-based component matching with a reachability-based approach. A violation's component key is now matched if the component can reach the violating importer anywhere in the import graph, not only when the key appears in the single BFS shortest path from binary to importer. This allows listing the same (Importer, Imported) pair under multiple component keys when several components transitively import the same non-FIPS library. All matched entries are suppressed; none are flagged stale as long as the violation persists. Stale detection is updated to use the same reachability check: an entry is silently skipped when the component can no longer reach the importer (path broken or dependency removed), and flagged stale only when the component still reaches the importer but the violation is gone. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
KnownViolation.Importer now supports three forms: - "" (empty): wildcard, matches any intermediate importer reachable from the component key — use when only the forbidden package matters. - exact path: matches only that specific package (existing behaviour). - module-root prefix: "github.com/foo/bar" matches any subpackage "github.com/foo/bar/baz", collapsing many per-subpackage entries into one. Update stale detection to use componentCanReachImporter which handles prefix and empty importer values correctly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Imported now supports the same three forms as Importer: exact path, module-root prefix, or "" wildcard. A prefix like "github.com/jcmturner/gokrb5/v8" matches any gokrb5 subpackage as the forbidden import, collapsing per-subpackage entries. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Binary key prefix matching: binKeysForBinary now accepts module-root
prefixes (e.g. "github.com/elastic/elastic-agent") in addition to exact
binary import paths, so real-world usage no longer silently ignores all
documented violations.
- go list cwd: extract goListPackages helper that sets cmd.Dir to the
module root (found by walking up to go.mod), fixing silent fallback to
library mode when the test file lives in a subdirectory and ./... would
otherwise miss binaries elsewhere in the module. This also deduplicates
the go list pipeline between scanBinaries and Scan.
- Trailing slash in Imported: importedMatches now calls TrimRight before
building the prefix check, matching the existing isForbidden logic, so
KnownViolation{Imported: fipsscan.XCrypto} works correctly.
- Library mode stale detection: when binsToCheck is nil (no binaries
found), fall back to len(compPkgs[compKey]) > 0 instead of
isReachableFromAny, so non-wildcard component stale entries are checked
rather than silently skipped.
Also extract matchesBinaryKey as a package-level function and add unit
tests for it, isForbidden, and moduleRoot.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
kruskall
left a comment
There was a problem hiding this comment.
FIPS compliance needs to be enforced across multiple Elastic repositories (elastic-agent, otel-collector-components, beats, …). Without a shared helper, each repo would reimplement violation detection independently and inconsistently.
I'm not convinced this is enough to justify this. Downstream components have different dependencies and they need to constantly audit their dependency graph (ideally even without fips). It would be more surprising if they all share the same forbidden packages.
elastic-agent-libs is a library, we shouldn't leak (more) implementation details of downstream consumers in here (despite how bad it already is). Trying to maintain a centralized list will add noise a maintenance burden to everyone.
| // FilippioIO covers filippo.io/ packages (edwards25519, age, mlkem768, | ||
| // etc.). None are part of a FIPS 140-3 certified module boundary, even | ||
| // when they implement a FIPS-standardized algorithm (e.g. FIPS 203 ML-KEM). | ||
| FilippioIO = "filippo.io/" |
There was a problem hiding this comment.
This is excluding a whole namespace which looks too broad :(
| // and decodes the output. Shared by scanBinaries and Scan. | ||
| func goListPackages(t testing.TB, patterns []string) []goListPackage { | ||
| t.Helper() | ||
| args := append([]string{"list", "-json", "-deps", "-tags", "requirefips"}, patterns...) |
There was a problem hiding this comment.
consumers of this library use different tags so this needs to be configurable to work reliably
belimawr
left a comment
There was a problem hiding this comment.
So far I mostly vibe-reviewed it, my clanker found one issue:
• High: CheckModule can suppress a violation for a component that is not reachable from the scanned binary. markKnown only checks that the component can reach the importer (componentCanReach), but never verifies that
the component is reachable from v.Binary:
• testing/fipsscan/fipsscan.go:462-468
• testing/fipsscan/fipsscan.go:540-552
Because component reachability is computed across the entire merged import graph, an unrelated component elsewhere in the module can document/suppress a violation from another binary whenever both eventually reach the
same importer. This silently weakens the FIPS audit and contradicts the API documentation requiring both directions of reachability.
markKnown should additionally verify that the component key matches a package reachable from the violation’s binary (except library-mode scans where Binary == ""). Add a regression test covering two binaries sharing an
importer, with the known component reachable only from the other binary.
The tagged package tests and PR CI pass, but they do not exercise this cross-binary attribution case.
Could you take a look at it? It seems legit. After that I'll do a more in-depth review.
| // and decodes the output. Shared by scanBinaries and Scan. | ||
| func goListPackages(t testing.TB, patterns []string) []goListPackage { | ||
| t.Helper() | ||
| args := append([]string{"list", "-json", "-deps", "-tags", "requirefips"}, patterns...) |
… markKnown
markKnown only verified that a component could reach the violating importer
(componentCanReach) but never checked that the component is reachable from
the violation's own binary. This allowed a component from binary B to
silently suppress a violation attributed to binary A whenever both reach the
same importer via the merged import graph.
Fix: add isReachableFromAny(compKey, []string{binary}, reachableFrom) in
markKnown so a component entry is only consulted when it is actually part of
the binary being audited.
Extract matching logic into checkViolations() so the bug can be exercised by
a unit test without spawning go list.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tants The package no longer ships a default list of non-FIPS libraries or exported constants. Callers own the full forbiddenPkgs slice — this removes the sync burden from elastic-agent-libs and avoids coupling the library to downstream consumers' dependency choices. extraForbiddenPkgs renamed to forbiddenPkgs everywhere to reflect that the caller now supplies the complete list, not an extension of a default. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Remove //go:build requirefips from the package and tests — the package is now policy-agnostic and carries no build constraints. It is up to each consumer's test file to add whatever build tag their CI uses. Update README to reflect the agnostic design: drop the stale ForbiddenPkgs/constants/extraForbiddenPkgs references, replace the nil placeholder with an explicit forbiddenPkgs variable in the quick-start example, and rewrite the "What is checked" section as caller-owned list. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Remove comments that reference internal function names, caller relationships, algorithm internals, deleted constants, and development-process rationale. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
cef9e07 to
2d9e637
Compare
Callers now supply the build tags passed to go list. Passing nil or an empty slice runs go list with no -tags flag. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
💚 Build Succeeded
History
|
TL;DR
Unifying the way we test binaries/modules for FIPS compliance. Aim was to just import this module, prepare list of known violations and Voilà. Example usages:
vibe-coded. This PR is bigger than I expected and mostly resonate about resolving the dependencies between binary, component and library which violates the FIPS
Iterated few times to give satisfying results and minimal usage in fleet-server, elastic-agent and beats repo
Summary
Extend
testing/fipsscanpackage — a shared Go testing helper for auditing FIPS 140-3 compliance of Go modules. It scans dependency trees for imports of known non-FIPS crypto libraries and reports violations via the standardtesting.Tinterface.The package is guarded by
//go:build requirefipsso it compiles and incurs zero overhead outside FIPS-focused test runs (go test -tags requirefips).Motivation
FIPS compliance needs to be enforced across multiple Elastic repositories (elastic-agent, otel-collector-components, beats, …). Without a shared helper, each repo would reimplement violation detection independently and inconsistently. This package provides a single place to maintain the list of known non-FIPS libraries and a stable API for documenting accepted violations per component.
API
CheckModule— the main entry pointCheckModulereports undocumented violations viat.Errorfand flags stale entries (violations that no longer exist) so the map stays current.Key design points
binary → component → violations) lets you document which part of the binary owns each violation. The component key is any package path or module-root prefix appearing in the import chain from the binary down to the direct importer — use a broad prefix like"github.com/twmb/franz-go"to cover all violations from that library in one entry, or an exact package for precision.KnownViolation.Importedsupport module-root prefixes.{Imported: "github.com/jcmturner/gokrb5/v8"}matches anygokrb5/v8/…sub-package. Binary key"github.com/elastic/elastic-agent"matches any binary under that module root.package mainentries.skipBinaries. Dev tools, test helpers, and non-shipped binaries can be excluded from the scan and from stale detection.go listpass. All patterns are resolved in one subprocess invocation regardless of how many binaries match.Lower-level functions
Baseline forbidden-package list
Ten third-party crypto libraries are checked by default (all exported as constants):
XCryptogolang.org/x/crypto/JcmturnerGokrb5github.com/jcmturner/gokrb5/JcmturnerAesctsgithub.com/jcmturner/aescts/JcmturnerGoforkgithub.com/jcmturner/gofork/XdgGoPbkdf2github.com/xdg-go/pbkdf2ProtonMailGoCryptogithub.com/ProtonMail/go-crypto/CloudflareCirclgithub.com/cloudflare/circl/AzureGoNtlmsspgithub.com/Azure/go-ntlmsspYoumarkPkcs8github.com/youmark/pkcs8FilippioIOfilippo.io/Usage convention
The recommended filename is
fips_compliance_test.goand function nameTestFIPSCompliancein every adopting module. This makes ecosystem-wide auditing trivial:Test plan
go build -tags requirefips ./testing/fipsscan/passesgo test -tags requirefips ./testing/fipsscan/passes (unit tests forShortestChain,FormatChain,matchesBinaryKey,isForbidden,moduleRoot)drosiek-fipsbranch in elastic-agent uses this package forelastic-agentandedotbinaries