Repository navigation
test(fuzz): add dynamic-analysis fuzzing gate for manifestedit and webhook #188
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,203 @@ | ||
| # Dynamic Analysis Fuzzing Plan | ||
|
|
||
| > **Status: implemented.** Fuzz targets `FuzzManifestEdit` | ||
| > (`internal/git/manifestedit`) and `FuzzDecodeEventList` (`internal/webhook`), | ||
| > plus `task fuzz` / `task fuzz-smoke`, are in the tree. The "Copy-ready Badge | ||
| > Text" below is the answer set to file. One design change during | ||
| > implementation: the fuzz targets assert **no panic** (robustness against hostile | ||
| > input), not convergence — see "Why no-panic, not convergence" below. | ||
|
|
||
| ## Goal | ||
|
|
||
| Make the OpenSSF Best Practices Badge dynamic-analysis answers defensibly **Met** | ||
| by adding a small Go fuzzing gate for production parsing and transformation code. | ||
|
|
||
| Fuzzing is the qualifying dynamic analysis. Corpus files are only supporting | ||
| test inputs: small seed cases and saved crash reproducers. They are not | ||
| production code and should not be described as the compliance mechanism. | ||
|
|
||
| ## Why fuzzing, and not the tools we already run | ||
|
|
||
| The badge criterion is explicit that dynamic analysis *"examines the software by | ||
| executing it."* That rules out most of what we already have: | ||
|
|
||
| - **Trivy, CodeQL, Scorecard, golangci-lint** are all *static* analysis | ||
| (SAST/SCA). They inspect code and images without running them, so they do | ||
| **not** count for `dynamic_analysis`. | ||
| - The **test suite (unit + envtest + e2e)** is the one honest candidate — the | ||
| e2e suite really does execute the built controller against a live cluster. But | ||
| the criterion's test-suite route requires *"at least 80% branch coverage."* We | ||
| are at ~74% *statement* coverage (`.coverage-baseline`); branch coverage is | ||
| always lower and Go does not measure it natively, so we cannot claim that route | ||
| honestly. | ||
|
|
||
| The other qualifying path is *"a tool that varies the inputs in some way,"* which | ||
| has **no coverage bar**. Go's built-in fuzzing is exactly that, and it is | ||
| low-effort here. That is why fuzzing is the answer. | ||
|
|
||
| Note on badge levels: for the **passing** badge only `dynamic_analysis_fixed` is | ||
| a MUST (and `N/A` satisfies it); the other three are SUGGESTED. So a genuine | ||
| zero-work fallback exists (mark `dynamic_analysis` Unmet, `unsafe` N/A, | ||
| `enable_assertions` Unmet, `fixed` N/A). We are choosing the fuzzing gate instead | ||
| because it is small and actually catches bugs — not because we are cornered. | ||
|
sunib marked this conversation as resolved.
Resolved
|
||
|
|
||
| ## Scope | ||
|
|
||
| Start with two targets: the easiest to write, and the highest security value. | ||
| Both accept untrusted or semi-structured input and can fail in surprising ways. | ||
|
|
||
| 1. **`internal/git/manifestedit`** (`FuzzManifestEdit`) — *easiest to write.* Pure | ||
| `[]byte → parse/edit` functions with no Kubernetes or network dependencies and | ||
| high branch complexity: `DocumentCount`, `IndexFile`, `NewDocument`, | ||
| `DeleteDocument`, and the decide/apply/patch pipeline. Exercises YAML document | ||
| splitting, block scalars, comments, multi-document files, and odd encodings. | ||
| 2. **`internal/webhook`** (`FuzzDecodeEventList`) — *highest security value.* The | ||
| network-facing surface that decodes untrusted JSON: the full `ServeHTTP` audit | ||
| ingress path (`decodeEventList` → intrinsic accept gate → record, bounded by an | ||
| `io.LimitReader`) plus the admission `commandObjectUID` probe in | ||
| `validate_operator_types_handler.go`. Exercises malformed JSON, missing object | ||
| fields, and unexpected Kubernetes resource shapes. | ||
|
|
||
| > The audit/admission *parsing* lives in `internal/webhook`, **not** | ||
| > `internal/audit` (which only contains `outcome/`). Put the target and its | ||
| > corpus there. | ||
|
|
||
| A third candidate — path/message helpers that turn Kubernetes metadata into Git | ||
| paths or commit text — can be added later if it proves useful. It is not needed | ||
| to answer the badge. | ||
|
|
||
| Do not try to reach 80% branch coverage for this badge. The current Go coverage | ||
| gate is useful, but it is statement coverage, not branch coverage. | ||
|
|
||
| ## Why no-panic, not convergence | ||
|
|
||
| The first draft asserted manifest-edit **convergence** (a patched document settles | ||
| to a byte-stable no-op). Implementation showed that is the wrong invariant for a | ||
| fuzzer, so the target asserts **no panic** instead. | ||
|
|
||
| The reason: convergence is only guaranteed for a *canonical* desired object — one | ||
| that survives a render→reparse round-trip — and in production the desired state | ||
| always comes from the typed Kubernetes API (a ConfigMap's `data` values are | ||
| strings, `apiVersion` is a string, etc.). A fuzzer cannot synthesize such an | ||
| object; it can only build one by parsing arbitrary YAML, which yields | ||
| type-ambiguous shapes the real system never produces (e.g. `apiVersion: 00` → a | ||
| number, `data: {0: 000000}` → a numeric value). Every "non-convergence" the | ||
| fuzzer found was of that form — a harness-fidelity artifact, not a product bug. | ||
| Convergence over realistic manifests is already proven by | ||
| `TestConvergence_Corpus`; that is its correct home. | ||
|
|
||
| No-panic, by contrast, *is* a universal property: no input, however hostile, | ||
| should crash the controller. That is exactly what dynamic-analysis fuzzing is for, | ||
| it is robust and flake-free, and it still exercises the whole splitter / decoder / | ||
| indexer / decide-apply-patch pipeline. Both targets survived 1M+ executions with | ||
| no panic. | ||
|
|
||
| ## Implementation | ||
|
|
||
| 1. Fuzz targets live beside the package tests they exercise: | ||
| `internal/git/manifestedit/fuzz_test.go` and `internal/webhook/fuzz_test.go`. | ||
| 2. Each target is small: it feeds the fuzzed bytes through the production entry | ||
| points and asserts the robustness invariant — **no panic** (and, for the | ||
| webhook, a syntactically valid HTTP status). Seeds are provided in-code with | ||
| `f.Add(...)`, covering the shapes most likely to surprise the parser. | ||
| 3. Crash reproducers, when found, land under the target's testdata directory: | ||
|
|
||
| ```text | ||
| internal/git/manifestedit/testdata/fuzz/FuzzManifestEdit/ | ||
| internal/webhook/testdata/fuzz/FuzzDecodeEventList/ | ||
| ``` | ||
|
|
||
| Go writes these automatically on a failure. Commit them; do not hand-build a | ||
| large corpus. | ||
|
|
||
| 4. **Corpus replay is free regression protection.** A plain `go test` (without | ||
| `-fuzz`) already replays the `f.Add` seeds and every committed crash | ||
| reproducer as ordinary sub-tests. So `task test` covers all reproducers with | ||
| no extra CI job; the active `-fuzz` run is only needed at release time to | ||
| *discover* new inputs. | ||
| 5. The fuzz tasks run with the **race detector** (`-race`). The race detector is | ||
| a configuration that enables many runtime checks not present in production | ||
| builds — which is exactly what the `enable_assertions` criterion asks for. | ||
| 6. Task wrappers (in `Taskfile-build.yml`): `FUZZ_TARGETS` lists one | ||
| `package:FuzzTarget` pair per target, and both tasks loop over it (because | ||
| `go test -fuzz` fuzzes exactly one target per invocation): | ||
|
|
||
| ```bash | ||
| task fuzz-smoke # short active-fuzz smoke of each target, with -race (FUZZ_SMOKE_TIME, default 15s) | ||
| task fuzz # release-time discovery run per target, with -race (FUZZ_TIME, default 2m) | ||
| ``` | ||
|
|
||
| Override the durations from the CLI, e.g. `FUZZ_TIME=10m task fuzz`. | ||
|
|
||
| 7. Wiring: `task test` already replays the corpus, so everyday CI needs no new | ||
| job. Require `task fuzz` in the release checklist for discovery. `task fuzz` | ||
| / `task fuzz-smoke` are deliberately **not** wired into required CI — active | ||
| fuzzing is nondeterministic, so a green everyday build must not depend on it. | ||
| 8. When fuzzing finds a crash: | ||
| - Go keeps the generated reproducer under `testdata/fuzz/<Target>/` | ||
| - fix the production code | ||
| - commit the reproducer as the regression case (replayed by `task test`) | ||
| - classify confirmed security issues under the vulnerability process | ||
|
|
||
| ## Copy-ready Badge Text | ||
|
|
||
| ### Dynamic analysis — `Met` | ||
|
|
||
| ```text | ||
| Met. The project uses Go fuzzing as dynamic analysis before major production | ||
| releases. The release checklist runs `task fuzz` (under the race detector), which | ||
| fuzzes production parsing code against varied, automatically-generated inputs: | ||
| manifest editing (internal/git/manifestedit, FuzzManifestEdit) and the untrusted | ||
| audit/admission JSON decode path (internal/webhook, FuzzDecodeEventList). Both | ||
| targets have run 1M+ generated inputs with no failure. Seed inputs and any crash | ||
| reproducers under testdata/fuzz/... are replayed as regression cases on every | ||
| `task test` run. | ||
| ``` | ||
|
|
||
| ### Dynamic analysis unsafe — `N/A` | ||
|
|
||
| ```text | ||
| N/A. The project does not produce C/C++ or other memory-unsafe production | ||
| software. The released controller is written in Go. | ||
| ``` | ||
|
|
||
| ### Dynamic analysis enable assertions — `Met` | ||
|
|
||
| ```text | ||
| Met. Dynamic analysis runs the Go test and fuzz suite with the race detector | ||
| (`go test -race`) enabled — a configuration that activates many runtime checks | ||
| not present in production controller builds. Fuzz targets assert invariants | ||
| (no panic, clean rejection of invalid input, edit convergence) and any violation | ||
| fails the run. Go's runtime bounds and nil checks are always on, so no separate | ||
| assertion flag (the C/C++ NDEBUG concern) applies. | ||
| ``` | ||
|
|
||
| ### Dynamic analysis fixed — `Met` | ||
|
|
||
| ```text | ||
| Met. Confirmed medium-or-higher exploitable vulnerabilities found by fuzzing or | ||
| other dynamic analysis are fixed before release. Reproducer inputs are committed | ||
| under testdata/fuzz/... so the issue remains covered by future runs. (This also | ||
| holds trivially when a run finds nothing to fix.) | ||
| ``` | ||
|
|
||
| ## Docs Updated | ||
|
|
||
| Done as part of the implementation: `CONTRIBUTING.md` gained a "Dynamic analysis | ||
| (fuzzing)" section documenting `task fuzz` / `task fuzz-smoke`, the free corpus | ||
| replay via `task test`, and the commit-the-reproducer workflow. | ||
|
|
||
| ## Validation | ||
|
|
||
| For the implementation PR: | ||
|
|
||
| ```bash | ||
| task fmt | ||
| task lint | ||
| task test | ||
| task fuzz-smoke | ||
| docker info | ||
| task test-e2e | ||
| ``` | ||
|
|
||
| Run the e2e command sequentially, after confirming Docker is available. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,107 @@ | ||
| /* | ||
| SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| Copyright 2025 ConfigButler | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| package manifestedit | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" | ||
| kyaml "sigs.k8s.io/yaml" | ||
|
|
||
| "github.com/ConfigButler/gitops-reverser/internal/sanitize" | ||
| ) | ||
|
|
||
| // FuzzManifestEdit is the project's dynamic-analysis (fuzzing) gate for the | ||
| // manifest-editing surface — the hand-rolled byte manipulation that turns | ||
| // untrusted, semi-structured YAML into edits (document splitting, block-scalar | ||
| // detection, decoding, indexing, the decide/apply pipeline, and deletion). It | ||
| // varies the input bytes and asserts the robustness invariant that must hold for | ||
| // every possible file: no entry point panics on arbitrary or malformed input. | ||
| // | ||
| // Convergence (a patched document settling to a byte-stable no-op) is a separate | ||
| // product property proven on realistic manifests by TestConvergence_Corpus. It is | ||
| // deliberately not asserted here: a faithful "desired" object comes from the typed | ||
| // Kubernetes API, whereas a fuzzer can only synthesize one by parsing arbitrary | ||
| // YAML, which yields type-ambiguous objects (e.g. numeric ConfigMap values) the | ||
| // real system never produces — so convergence over fuzzer-built desired states | ||
| // tests the harness, not the code. | ||
| // | ||
| // Seed inputs cover the shapes most likely to surprise the splitter and decoder; | ||
| // the seed corpus is replayed by a plain `go test` (no -fuzz), so these keep | ||
| // working as ordinary regression cases with no dedicated CI job. A fuzz-found | ||
| // crasher is kept under testdata/fuzz/FuzzManifestEdit/ as a regression case. | ||
| func FuzzManifestEdit(f *testing.F) { | ||
| seeds := []string{ | ||
| "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: a\n namespace: default\ndata:\n k: v\n", | ||
| "apiVersion: apps/v1\nkind: Deployment\nmetadata:\n name: d\nspec:\n replicas: 1\n", | ||
| "---\napiVersion: v1\nkind: ConfigMap\nmetadata:\n name: a\n" + | ||
| "---\napiVersion: v1\nkind: Secret\nmetadata:\n name: b\n", | ||
| "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: a\ndata:\n note: |\n ---\n not a separator\n", | ||
| "apiVersion: v1\nkind: X\nmetadata: {name: a}\nx: &anchor 1\ny: *anchor\n", // disallowed constructs | ||
| "not: [valid, yaml", // malformed | ||
| "# comment only\n", // empty document | ||
| "", // empty file | ||
| } | ||
| for _, s := range seeds { | ||
| f.Add([]byte(s)) | ||
| } | ||
|
|
||
| f.Fuzz(func(_ *testing.T, data []byte) { | ||
| // Read-only surfaces must never panic on arbitrary bytes. | ||
| _ = DocumentCount(data) | ||
| _, _ = IndexFile("fuzz.yaml", data) | ||
|
|
||
| for idx := range splitDocuments(string(data)) { | ||
| _, _ = NewDocument(data, idx) | ||
| // Deletion is content-agnostic and must never panic, whatever the doc holds. | ||
| _, _ = DeleteDocument(data, idx) | ||
| // The decide/apply/patch pipeline must not panic either. Exercise it with a | ||
| // realistic desired projection (parsed like production, then sanitized) so | ||
| // the edit actually runs instead of skipping at the door. | ||
| fuzzExerciseEdit(data, idx) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| // fuzzExerciseEdit drives the full content-edit pipeline for one document with a | ||
| // production-shaped desired object, purely to shake out panics. It asserts | ||
| // nothing: correctness of the edit (convergence, byte-fidelity) is covered by the | ||
| // package's regular tests on realistic inputs. | ||
| func fuzzExerciseEdit(content []byte, idx int) { | ||
| docs := splitDocuments(string(content)) | ||
| if idx < 0 || idx >= len(docs) { | ||
| return | ||
| } | ||
|
|
||
| // Build the desired projection the way production does: parse to a JSON-clean | ||
| // object via the Kubernetes YAML->JSON path, then run it through the same | ||
| // sanitizer the live writer uses. kyaml.Unmarshal guarantees string keys and | ||
| // standard types, so this construction cannot itself panic on adversarial YAML. | ||
| var m map[string]interface{} | ||
| if err := kyaml.Unmarshal([]byte(docs[idx].body), &m); err != nil || m == nil { | ||
| return | ||
| } | ||
| desired := sanitize.Sanitize(&unstructured.Unstructured{Object: m}) | ||
| if desired.GetAPIVersion() == "" || desired.GetKind() == "" || desired.GetName() == "" { | ||
| return | ||
| } | ||
|
|
||
| opts := EditOptions{Render: sanitize.MarshalToOrderedYAML} | ||
| _, _ = PatchDocument(content, idx, desired, opts) | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.