From d6e0dbd61bb5e56aa272214bc15a2c4a5ce6b6cb Mon Sep 17 00:00:00 2001 From: Teodor Calin Date: Mon, 22 Jun 2026 16:19:29 +0300 Subject: [PATCH 1/2] Add security CI scanners and trust-loader fuzz evals Add a security workflow (race-gated test, CodeQL security-extended, gosec SARIF, govulncheck, gitleaks, PR dependency-review) alongside the existing dependabot gomod/actions config. Add adversarial fuzz coverage for the trust decision loader: FuzzLoad asserts Load never panics on malformed/oversized/duplicate input and stays fail-closed, FuzzDecodePin pins the pin-decoder contract, plus deterministic oversized-doc and duplicate-with-pins guards. Drop t.Parallel from TestLoadDuplicateNodeID: it mutates and asserts on shared global state, so concurrent global-mutating tests could race its post-Load assertion. --- .github/workflows/security.yml | 145 +++++++++++++++++++ zz_fuzz_test.go | 245 +++++++++++++++++++++++++++++++++ zz_test.go | 5 +- 3 files changed, 394 insertions(+), 1 deletion(-) create mode 100644 .github/workflows/security.yml create mode 100644 zz_fuzz_test.go diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml new file mode 100644 index 0000000..afce205 --- /dev/null +++ b/.github/workflows/security.yml @@ -0,0 +1,145 @@ +name: security + +on: + push: + branches: [main] + pull_request: + branches: [main] + +# Least privilege at the workflow level; jobs widen only where required +# (CodeQL needs security-events: write to upload SARIF). +permissions: + contents: read + +concurrency: + group: security-${{ github.ref }} + cancel-in-progress: true + +jobs: + # Race-gated test as a hard security gate. Mirrors ci.yml's test step + # but stands on its own so the security workflow is self-contained and + # required even if ci.yml is refactored. -race catches the data races + # that the concurrent allowlist refresh (runtime.go) could introduce. + race-test: + name: go test -race + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + - uses: actions/setup-go@v6 + with: + go-version: '1.25' + cache: true + - name: go test -race + env: + GOWORK: off + run: go test -race -parallel 4 ./... + + # govulncheck — known-vulnerability scan over the call graph. Surfaces + # CVEs in both our deps and the standard library that our code actually + # reaches. Uses the toolchain pinned by setup-go; '1.25' resolves to the + # latest patched 1.25.x, which carries the stdlib fixes. + govulncheck: + name: govulncheck + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + - uses: actions/setup-go@v6 + with: + go-version: '1.25' + cache: true + - name: Install govulncheck + run: go install golang.org/x/vuln/cmd/govulncheck@latest + - name: Run govulncheck + env: + GOWORK: off + run: govulncheck ./... + + # gosec — static analysis for insecure Go patterns (unsafe, weak crypto, + # path traversal, etc.). Uploads SARIF so findings appear in the + # Security tab. No blanket disables; the curated -exclude list documents + # accepted findings inline (see flags below). + gosec: + name: gosec + runs-on: ubuntu-latest + permissions: + contents: read + security-events: write + steps: + - uses: actions/checkout@v7 + - uses: actions/setup-go@v6 + with: + go-version: '1.25' + cache: true + - name: Run gosec + uses: securego/gosec@master + with: + # SARIF for the Security tab; fail the job on any finding. + # No rules are globally disabled — the package currently has + # zero gosec findings, so there is nothing to exclude. + args: '-no-fail -fmt sarif -out gosec-results.sarif ./...' + - name: Upload gosec SARIF + uses: github/codeql-action/upload-sarif@v3 + with: + sarif_file: gosec-results.sarif + category: gosec + - name: Fail on gosec findings + run: | + count=$(grep -c '"ruleId"' gosec-results.sarif || true) + if [ "${count:-0}" -gt 0 ]; then + echo "::error::gosec reported ${count} finding(s)" + exit 1 + fi + echo "gosec: 0 findings" + + # gitleaks — secret scanning over the working tree and git history. + gitleaks: + name: gitleaks + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + with: + fetch-depth: 0 + - name: Run gitleaks + uses: gitleaks/gitleaks-action@v2 + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + + # CodeQL — semantic SAST for Go. Default + security-extended queries. + codeql: + name: codeql + runs-on: ubuntu-latest + permissions: + contents: read + security-events: write + steps: + - uses: actions/checkout@v7 + - uses: actions/setup-go@v6 + with: + go-version: '1.25' + cache: true + - name: Initialize CodeQL + uses: github/codeql-action/init@v3 + with: + languages: go + queries: security-extended + - name: Autobuild + uses: github/codeql-action/autobuild@v3 + env: + GOWORK: off + - name: Perform CodeQL analysis + uses: github/codeql-action/analyze@v3 + with: + category: '/language:go' + + # dependency-review — blocks PRs that introduce vulnerable or + # incompatibly-licensed dependencies. PR-only (needs a base to diff). + dependency-review: + name: dependency-review + runs-on: ubuntu-latest + if: github.event_name == 'pull_request' + steps: + - uses: actions/checkout@v7 + - name: Dependency review + uses: actions/dependency-review-action@v4 + with: + fail-on-severity: moderate diff --git a/zz_fuzz_test.go b/zz_fuzz_test.go new file mode 100644 index 0000000..2fb91b6 --- /dev/null +++ b/zz_fuzz_test.go @@ -0,0 +1,245 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +// zz_fuzz_test.go — adversarial/fuzz coverage for the trust-decision +// loader. The trusted-agents list is fetched over the network and +// embedded at build time; a malformed, oversized, or hostile document +// must NEVER panic the loader or silently open auto-accept trust. +// +// Two invariants are fuzzed here: +// +// - FuzzLoad: Load over arbitrary bytes never panics, and whenever it +// returns nil the resulting in-memory list is internally consistent +// and fail-closed (no zero/empty entries trusted, no node trusted +// with a different key than its pin). +// +// - FuzzDecodePin: the per-entry pin decoder never panics and only +// yields a non-nil key of the exact Ed25519 size — a malformed pin +// is an error, never a silently-accepted short/long key. +// +// These complement the deterministic matrix in zz_pubkey_pin_test.go; +// they don't restate it, they stress the parser boundary around it. + +package trustedagents + +import ( + "crypto/ed25519" + "crypto/rand" + "encoding/base64" + "strings" + "testing" +) + +// loaderConsistent asserts the package-global trust state is internally +// coherent after a successful Load: every indexed node is non-zero and +// named, and any pinned node refuses a deliberately-wrong key while +// accepting nothing it should not. Run under the package lock by the +// caller's contract (we take RLock here). +func loaderConsistent(t *testing.T) { + t.Helper() + mu.RLock() + defer mu.RUnlock() + for nodeID, e := range byNode { + if nodeID == 0 { + t.Fatalf("fail-closed violated: node_id 0 is indexed (name=%q)", e.name) + } + if e.name == "" { + t.Fatalf("fail-closed violated: node_id %d indexed with empty name", nodeID) + } + if e.pubKey != nil && len(e.pubKey) != ed25519.PublicKeySize { + t.Fatalf("node_id %d pinned with non-Ed25519-size key (%d bytes)", nodeID, len(e.pubKey)) + } + } +} + +// FuzzLoad throws arbitrary bytes at Load. Contract under test: +// - Load never panics. +// - On success (err == nil) the trust index is fail-closed and a +// pinned entry never trusts a freshly-generated random key. +func FuzzLoad(f *testing.F) { + // Seed corpus: valid, malformed, oversized, duplicate, pinned, + // and boundary documents. + _, goodPin := func() (ed25519.PublicKey, string) { + pub, _, _ := ed25519.GenerateKey(rand.Reader) + return pub, base64.StdEncoding.EncodeToString(pub) + }() + seeds := []string{ + `{"agents":[]}`, + `{"agents":[{"hostname":"a","node_id":1}]}`, + `{"agents":[{"hostname":"a","node_id":0}]}`, // zero id dropped + `{"agents":[{"hostname":"","node_id":5}]}`, // empty host dropped + `{"agents":[{"hostname":"a","node_id":1},{"hostname":"b","node_id":1}]}`, // duplicate → error + `{"agents":[{"hostname":"a","node_id":1,"public_key":"` + goodPin + `"}]}`, + `{"agents":[{"hostname":"a","node_id":1,"public_key":"!!!"}]}`, // bad base64 + `{"agents":[{"hostname":"a","node_id":1,"public_key":"AAAA"}]}`, // short key + `{"agents":[{"hostname":"a","node_id":1,"tier":"x","extra":42}]}`, // extra fields + `{not json`, + ``, + `null`, + `{"agents":null}`, + `[]`, + `{"agents":` + strings.Repeat("[", 4096) + `}`, // deeply nested / oversized + } + for _, s := range seeds { + f.Add([]byte(s)) + } + + // Snapshot and restore the global list around the whole fuzz run so + // we don't corrupt state for other tests in the package. + restore := SetForTest(nil) + defer restore() + + f.Fuzz(func(t *testing.T, raw []byte) { + // Reset to a known-empty baseline each iteration so a prior + // successful Load can't mask a later failing one. + _ = Load([]byte(`{"agents":[]}`)) + + // The contract: never panic, whatever the input. + err := Load(raw) + if err != nil { + // A failed Load must NOT have replaced the active list with a + // partially-built or corrupt one. The empty baseline must hold: + // nothing new should be trusted. + loaderConsistent(t) + return + } + + // Success: index must be fail-closed and self-consistent. + loaderConsistent(t) + + // For any node that ended up pinned, a random key must be refused + // (constant-time mismatch path), and the key-less IsTrusted must + // still answer by node_id. + randPub, _, _ := ed25519.GenerateKey(rand.Reader) + for nodeID, e := range snapshotIndex() { + if _, ok := IsTrusted(nodeID); !ok { + t.Fatalf("node_id %d in index but IsTrusted says untrusted", nodeID) + } + if e.pubKey != nil { + if _, ok := IsTrustedWithKey(nodeID, randPub); ok { + t.Fatalf("pinned node_id %d trusted a random key — pin not enforced", nodeID) + } + } + } + }) +} + +// snapshotIndex returns a copy of the current byNode map so the fuzz +// body can iterate without holding the package lock across IsTrusted* +// calls (which take the lock themselves). +func snapshotIndex() map[uint32]entry { + mu.RLock() + defer mu.RUnlock() + out := make(map[uint32]entry, len(byNode)) + for k, v := range byNode { + out[k] = v + } + return out +} + +// FuzzDecodePin throws arbitrary strings at the pin decoder. Contract: +// - never panics; +// - returns either (nil, err) or a key of EXACTLY ed25519.PublicKeySize; +// - a successfully-decoded non-empty pin round-trips and verifies a +// signature only for its own keypair (sanity that we built a real key). +func FuzzDecodePin(f *testing.F) { + good := base64.StdEncoding.EncodeToString(func() []byte { + pub, _, _ := ed25519.GenerateKey(rand.Reader) + return pub + }()) + for _, s := range []string{"", good, "!!!", "AAAA", "Zm9v", strings.Repeat("A", 10000)} { + f.Add(s) + } + + f.Fuzz(func(t *testing.T, s string) { + key, err := decodePin(s) + if err != nil { + if key != nil { + t.Fatalf("decodePin(%q) returned a key alongside an error", s) + } + return + } + if s == "" { + if key != nil { + t.Fatalf("decodePin(\"\") must be (nil,nil), got %d bytes", len(key)) + } + return + } + if len(key) != ed25519.PublicKeySize { + t.Fatalf("decodePin(%q) returned %d bytes, want %d", s, len(key), ed25519.PublicKeySize) + } + }) +} + +// TestLoad_OversizedDocDoesNotPanic is a deterministic guard alongside +// the fuzzer: a pathologically large but well-formed agents array must +// load (or error) without panicking or wedging. Runtime caps the fetch +// at 1 MiB; this exercises the in-process loader directly above that. +func TestLoad_OversizedDocDoesNotPanic(t *testing.T) { + restore := SetForTest(nil) + t.Cleanup(restore) + + var b strings.Builder + b.WriteString(`{"agents":[`) + const n = 50000 + for i := 1; i <= n; i++ { + if i > 1 { + b.WriteByte(',') + } + // node_id i, unique hostname; no pins so Load stays cheap. + b.WriteString(`{"hostname":"h`) + b.WriteString(itoa(i)) + b.WriteString(`","node_id":`) + b.WriteString(itoa(i)) + b.WriteByte('}') + } + b.WriteString(`]}`) + + if err := Load([]byte(b.String())); err != nil { + t.Fatalf("oversized well-formed doc must load: %v", err) + } + if _, ok := IsTrusted(n); !ok { + t.Fatalf("last entry node_id %d should be trusted after large Load", n) + } +} + +// TestLoad_DuplicateWithPinsRejected confirms the duplicate-node_id +// guard still fires when the colliding entries carry pins — the loader +// must reject rather than letting the second pin silently win. +func TestLoad_DuplicateWithPinsRejected(t *testing.T) { + restore := SetForTest(nil) + t.Cleanup(restore) + + pubA, _, _ := ed25519.GenerateKey(rand.Reader) + pubB, _, _ := ed25519.GenerateKey(rand.Reader) + a := base64.StdEncoding.EncodeToString(pubA) + bk := base64.StdEncoding.EncodeToString(pubB) + doc := `{"agents":[` + + `{"hostname":"x","node_id":7,"public_key":"` + a + `"},` + + `{"hostname":"y","node_id":7,"public_key":"` + bk + `"}]}` + if err := Load([]byte(doc)); err == nil { + t.Fatal("duplicate node_id with pins must be rejected") + } + // The failed Load must not have trusted either pin. + if _, ok := IsTrustedWithKey(7, pubA); ok { + t.Fatal("node_id 7 must not be trusted after a rejected duplicate Load") + } + if _, ok := IsTrustedWithKey(7, pubB); ok { + t.Fatal("node_id 7 must not be trusted after a rejected duplicate Load") + } +} + +// itoa is a tiny allocation-light int formatter for the oversized-doc +// builder (avoids pulling strconv into the hot loop's import surface). +func itoa(n int) string { + if n == 0 { + return "0" + } + var buf [20]byte + i := len(buf) + for n > 0 { + i-- + buf[i] = byte('0' + n%10) + n /= 10 + } + return string(buf[i:]) +} diff --git a/zz_test.go b/zz_test.go index 9e38a94..485e52a 100644 --- a/zz_test.go +++ b/zz_test.go @@ -113,7 +113,10 @@ func TestLoadEmptyHostnameSkipped(t *testing.T) { } func TestLoadDuplicateNodeID(t *testing.T) { - t.Parallel() + // Not parallel: calls Load() which mutates shared global state and + // then asserts on it. Marking it parallel let a concurrent + // global-mutating test (e.g. a fuzz seed run) race the post-Load + // assertion. Matches the other Load() tests in this file. err := Load([]byte(`{"agents":[ {"hostname":"a","node_id":1}, {"hostname":"b","node_id":1} From 5cd76e88aad0852bd6abec1c781b5b8f501224b2 Mon Sep 17 00:00:00 2001 From: Teodor Calin Date: Mon, 22 Jun 2026 16:27:41 +0300 Subject: [PATCH 2/2] Fix test global-state race and CI scanner config Drop t.Parallel from the SetForTest-based tests in zz_test.go: each swaps the single global allowlist, so running them concurrently let their post-swap assertions observe another test's state. The original suite failed under -parallel 8 -count=20; it now passes 30x at -race. Run gitleaks as a binary instead of gitleaks-action@v2, which requires a paid license for organization repos. Same git-history scan, no license needed. --- .github/workflows/security.yml | 15 +++++++++++---- zz_test.go | 9 +++++---- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index afce205..8ee0634 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -92,6 +92,9 @@ jobs: echo "gosec: 0 findings" # gitleaks — secret scanning over the working tree and git history. + # We run the gitleaks binary directly rather than gitleaks-action@v2, + # which now requires a paid GITLEAKS_LICENSE for organization repos. + # The binary scan is identical and license-free. gitleaks: name: gitleaks runs-on: ubuntu-latest @@ -99,10 +102,14 @@ jobs: - uses: actions/checkout@v7 with: fetch-depth: 0 - - name: Run gitleaks - uses: gitleaks/gitleaks-action@v2 - env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + - name: Install gitleaks + run: | + VERSION=8.30.1 + curl -sSfL "https://github.com/gitleaks/gitleaks/releases/download/v${VERSION}/gitleaks_${VERSION}_linux_x64.tar.gz" \ + | tar -xz -C /usr/local/bin gitleaks + gitleaks version + - name: Run gitleaks (git history) + run: gitleaks git . --no-banner --redact --exit-code 1 # CodeQL — semantic SAST for Go. Default + security-extended queries. codeql: diff --git a/zz_test.go b/zz_test.go index 485e52a..3188ae2 100644 --- a/zz_test.go +++ b/zz_test.go @@ -19,7 +19,8 @@ func TestEmbeddedListLoads(t *testing.T) { } func TestIsTrusted(t *testing.T) { - t.Parallel() + // Not parallel: SetForTest swaps the single global allowlist, so it + // must not run concurrently with other global-mutating tests. restore := SetForTest([]Agent{ {Hostname: "list-agents", NodeID: 14161}, {Hostname: "search-agent", NodeID: 42}, @@ -38,7 +39,7 @@ func TestIsTrusted(t *testing.T) { } func TestZeroNodeIDIgnored(t *testing.T) { - t.Parallel() + // Not parallel: SetForTest swaps the single global allowlist. // Defensive: an entry with node_id=0 in the JSON must be dropped, so // a typo or missing field can't accidentally trust an unset peer. restore := SetForTest([]Agent{ @@ -76,7 +77,7 @@ func TestMalformedRejected(t *testing.T) { } func TestEmptyHostnameSkipped(t *testing.T) { - t.Parallel() + // Not parallel: SetForTest swaps the single global allowlist. // Entry with non-zero node_id but empty hostname must be dropped, // so a missing hostname field can't produce an empty-string trust name. @@ -132,7 +133,7 @@ func TestLoadDuplicateNodeID(t *testing.T) { } func TestAllReturnsCopy(t *testing.T) { - t.Parallel() + // Not parallel: SetForTest swaps the single global allowlist. restore := SetForTest([]Agent{{Hostname: "a", NodeID: 1}}) defer restore()