Repository navigation
Add check-for-go-fips-test pre-commit hook - #18
AmberArcadia wants to merge 6 commits into
Conversation
dannf
left a comment
dannf
left a comment
There was a problem hiding this comment.
The test looks good to me, a few comments about optimizations inside. Would you mind raising this in #eng-os? I'd like to give other people a chance to chime in, since this is establishing a de facto policy (and I'm not really a go-fips expert). @murraybd might be interested in it as well, as a test proliferater himself :)
One note: this test is looking at the YAML file in a pre-compiled state, so it will miss things such as packages with names like ${{vars.expands-to-go-fips}}-foo. If we implemented it as a melange linter or melange compiled it, it wouldn't have that problem. I wonder if there's a way to have one hook do the compilation such that all follow-on tests could consume it w/o recompiling?
| [^/]+\.ya?ml # matches .yaml or .yml files at the top level only | ||
| )$ | ||
| - id: check-for-go-fips-test | ||
| files: '^[^.][^/]*\.yaml$' # matches non-hidden .yaml files at the top level only |
There was a problem hiding this comment.
It might be more efficient to use this to identify the go-fips packages that need to be parsed. If we can filter out packages at this level, we needn't spawn another python interpreter to run the check. Of course, if it might be the case that only a subpackage will match, then that won't work.
| # Check if main package uses go-fips | ||
| main_uses_fips = False | ||
| main_has_test = False | ||
|
|
There was a problem hiding this comment.
I feel like we shouldn't need to duplicate the code between main package parsing and subpkg parsing. See the shellcheck test, where it puts the main package and any sub package in a list, and just iterates through them all the same way. We should probably consider creating a library class that just does stuff like this for us so we can share it.
Implements GitHub issue requirements: - Detects packages using go-fips in environment or pipeline - Verifies they have corresponding go-fips test (uses: test/go-fips-check) - Reports failures for packages missing the required test
- Detect any go-fips variant (go-fips, go-fips-md5, etc.) using startswith() - Check each package/subpackage independently for compliance - Require matching tests for each go-fips usage (not just any test) - Provide detailed error messages specifying which component lacks tests - Handle subpackage test sections properly Fixes comprehensive validation of go-fips test requirements.
Running the previous version over the 10,047 package files on stereo
main flagged 25 files, of which 2 were real gaps. The rest were main
packages whose environment lists go-fips only for the benefit of
subpackages, range subpackages covered by a go-fips-check in the main
test, one package using test/tw/emptypackage, and five files that
crashed the hook on a null `pipeline:`.
The check now asks, per package and subpackage, whether that scope
compiles Go with a FIPS toolchain and whether a go-fips-check covers it:
- go/build and go/install count when their go-package input starts with
go-fips. go/build/v2 and hand-written `go build` / `go install` lines
use the environment's go, so they count only when the environment
provides a go-fips toolchain. A main environment entry alone no longer
implicates a main package that builds nothing.
- Coverage is the scope's own test, test/emptypackage or
test/tw/emptypackage, or for subpackages a main test that runs
go-fips-check and installs the subpackage (range names match on the
prefix before ${{range.key}}).
- split/bin in a subpackage moves the main binaries there, so that
subpackage inherits the obligation and the main package is released.
- Nested `pipeline:` lists are searched; null pipelines no longer crash.
- The main and subpackage code paths share one set of helpers.
With this version stereo main reports exactly the 2 real gaps:
crossplane-provider-aws-cloudsearch-fips and two harvester-fips
subpackages. A stereo PR adds the missing tests.
Known limits, left for follow-ups: FIPS_GO_PREFIXES is only go-fips (not
go-msft or go-geomys), templated go-package values such as
${{vars.go-version}} are invisible without a melange compile, and Go
built through make or a script is not detected.
Fixtures under test-data/ show the three flagged shapes and the covered
ones.
946abf3 to
783c691
Compare
|
Rebased onto main and pushed 783c691, a rework driven by running the hook over every package file on stereo main: the previous version flagged 25 files of which 2 were real. It now attributes builds per scope (go/build and go/install via |
2 participants
What
Adds
check-for-go-fips-test, for chainguard-dev/internal-dev#9313: a package or subpackage that compiles Go with a FIPS toolchain must have atest/go-fips-checkcovering it.How it decides
Per package and per subpackage:
go/buildorgo/installstep hasgo-packagestarting withgo-fips, or when the environment provides ago-fips*toolchain and the scope runsgo/build/v2or aruns:block containinggo build/go install. Ago-fipsentry in the main environment on its own no longer implicates a main package that builds nothing.test/go-fips-check; bytest/emptypackageortest/tw/emptypackage; or, for a subpackage, by a main test that runsgo-fips-checkand installs the subpackage. Range subpackages match on the prefix before${{range.key}}, so${{package.name}}-controllerin the main test environment covers${{package.name}}-${{range.key}}.split/binin a subpackage moves the main binaries there: that subpackage inherits the obligation and the main package is released.pipeline:lists are searched. A nullpipeline:no longer crashes the hook.Results on stereo main (10,047 package files)
The 23 dropped were 17 main packages with
go-fipsin the environment only for their subpackages (every Go subpackage already tested), 5cluster-api-fipsrange subpackages covered by the main test, andzabbix-fips-7.0, which usestest/tw/emptypackage. The two that remain,crossplane-provider-aws-cloudsearch-fipsand theharvester-fips-webhookand-upgrade-helpersubpackages, are fixed in chainguard-dev/stereo#362436.Review feedback
iter_steps; once both land I will move the helpers into a shared module, kept local for now to avoid a cross-PR conflict.${{vars.*}}expansions. Still true, listed under limits. The shellcheck hook already compiles with melange in docker, so a shared compile step is the natural follow-up.files:level. pre-commit cannot select on content, so the hook has to parse every YAML it is given; the per-file cost is one safe-load.Known limits
FIPS_GO_PREFIXESisgo-fipsonly.go-msft-*andgo-geomys-*toolchains are not included; whether they should be held to the same rule is a policy call, and somegitlab-rails-*-fipspackages built withgo-msfthave nogo-fips-checktoday, so adding the prefix would need fixes there first.go-package: ${{vars.go-version}}) are invisible without a melange compile.makeor a script is not detected.Test
test-data/go-fips-test-bad.yamlproduces three findings (main viago/build, a subpackage viago/build/v2, a subpackage viaruns: go build).test-data/go-fips-test-good.yamlpasses and exercises main-test coverage of range subpackages, an own-test subpackage, a non-Go subpackage and a null pipeline.pre-commit run --all-filesis clean.