From 3e2826ce85fc9c084bb2c94a7dbb648365946bf3 Mon Sep 17 00:00:00 2001 From: Teodor Calin Date: Mon, 22 Jun 2026 16:24:07 +0300 Subject: [PATCH 1/2] Add badgeverify fuzz/redteam evals and security CI Harden the badgeverify badge/enrollment/recovery crypto and the common-module CI: - Native Go fuzz tests for all three parsers (round-trip/malleability guard) and all three verifiers (fail-closed, no-panic), with deterministic seed corpora. - Strengthen the red-team suite: cross-type confusion in both directions, wrong-keyring/other-key rejection, non-canonical integer encodings for all three statement types, all-zero placeholder key rejection for every type, signed-but-wrong-node binding, and the never-expiring badge-allowed vs recovery-disallowed asymmetry. - Make all three parsers canonical-strict: reject leading zeros, signs, and whitespace in node_id/timestamps/exp so a logical value has one signable wire spelling; reject recovery exp<=0 at parse. Closes the enrollment/recovery malleability gap left by the badge-only verify check (uncovered by the new fuzzers). - Add security workflow: race-test gate, CodeQL (Go), gosec (scoped to badgeverify), govulncheck, gitleaks, and a bounded badgeverify fuzz job. Add dependency-review on PR; extend dependabot to github-actions. - Bump go directive to 1.25.11 to clear GO-2026-5037 (stdlib x509). --- .github/dependabot.yml | 12 ++ .github/workflows/dependency-review.yml | 19 +++ .github/workflows/security.yml | 110 ++++++++++++ badgeverify/badgeverify.go | 68 +++++++- badgeverify/credential.go | 27 +-- badgeverify/fuzz_test.go | 198 ++++++++++++++++++++++ badgeverify/redteam_extra_test.go | 216 ++++++++++++++++++++++++ go.mod | 2 +- 8 files changed, 633 insertions(+), 19 deletions(-) create mode 100644 .github/dependabot.yml create mode 100644 .github/workflows/dependency-review.yml create mode 100644 .github/workflows/security.yml create mode 100644 badgeverify/fuzz_test.go create mode 100644 badgeverify/redteam_extra_test.go diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 0000000..3918641 --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,12 @@ +version: 2 +updates: + - package-ecosystem: "gomod" + directory: "/" + schedule: + interval: "weekly" + open-pull-requests-limit: 10 + - package-ecosystem: "github-actions" + directory: "/" + schedule: + interval: "weekly" + open-pull-requests-limit: 10 diff --git a/.github/workflows/dependency-review.yml b/.github/workflows/dependency-review.yml new file mode 100644 index 0000000..30a88cf --- /dev/null +++ b/.github/workflows/dependency-review.yml @@ -0,0 +1,19 @@ +name: dependency-review + +on: + pull_request: + branches: [main] + +permissions: + contents: read + +jobs: + dependency-review: + name: dependency-review + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - name: Dependency Review + uses: actions/dependency-review-action@v4 + with: + fail-on-severity: high diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml new file mode 100644 index 0000000..df89c4c --- /dev/null +++ b/.github/workflows/security.yml @@ -0,0 +1,110 @@ +name: security + +on: + push: + branches: [main] + pull_request: + branches: [main] + +permissions: + contents: read + +jobs: + # Gating race-test job, dedicated to the security workflow so it can be a + # required status check independent of the coverage job in ci.yml. + race-test: + name: go test -race + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-go@v5 + with: + go-version: '1.25' + cache: true + - name: go test -race ./... + run: go test -race ./... + + codeql: + name: CodeQL + runs-on: ubuntu-latest + permissions: + contents: read + security-events: write + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-go@v5 + with: + go-version: '1.25' + cache: true + - name: Initialize CodeQL + uses: github/codeql-action/init@v3 + with: + languages: go + - name: Autobuild + uses: github/codeql-action/autobuild@v3 + - name: Perform CodeQL Analysis + uses: github/codeql-action/analyze@v3 + with: + category: "/language:go" + + gosec: + name: gosec (badgeverify) + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-go@v5 + with: + go-version: '1.25' + cache: true + # Scoped to the crown-jewel badge crypto. The rest of the module + # carries pre-existing G104 findings (unhandled Close()/Set() errors) + # tracked as follow-up; scoping keeps this gate meaningful and green + # without rewriting unrelated packages in this PR. + - name: gosec + run: go run github.com/securego/gosec/v2/cmd/gosec@v2.22.5 -fmt=text ./badgeverify/... + + govulncheck: + name: govulncheck + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-go@v5 + with: + go-version: '1.25' + cache: true + - name: govulncheck + run: go run golang.org/x/vuln/cmd/govulncheck@v1.1.4 ./... + + gitleaks: + name: gitleaks + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 + - name: gitleaks + uses: gitleaks/gitleaks-action@v2 + env: + GITLEAKS_ENABLE_UPLOAD_ARTIFACT: "false" + + fuzz: + name: badgeverify fuzz + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-go@v5 + with: + go-version: '1.25' + cache: true + # Bounded fuzz window over every badgeverify target. Seed corpora are + # deterministic; this re-runs the seeds and explores new inputs for a + # short, CI-friendly slice per target. + - name: Fuzz parsers and verifiers + run: | + set -euo pipefail + targets="FuzzParseBadge FuzzParseEnrollment FuzzParseRecovery FuzzVerify FuzzVerifyEnrollment FuzzVerifyRecovery" + for t in $targets; do + echo "::group::$t" + go test -run='^$' -fuzz="^${t}$" -fuzztime=30s ./badgeverify/ + echo "::endgroup::" + done diff --git a/badgeverify/badgeverify.go b/badgeverify/badgeverify.go index 5bdd2bd..25a7290 100644 --- a/badgeverify/badgeverify.go +++ b/badgeverify/badgeverify.go @@ -112,6 +112,60 @@ func noColon(field, v string) error { return nil } +// isCanonicalDecimal reports whether s is the UNIQUE base-10 spelling of a +// non-negative integer: no sign, no leading zeros (except the literal "0"), +// no whitespace, no "+1"/"01"/" 1" malleability. strconv.ParseUint/ParseInt +// accept several spellings of the same number; because the signed bytes ARE +// the wire string, two spellings of one logical value would be two distinct +// signable strings (a malleability hazard). Requiring the canonical spelling +// makes the parsers themselves reject the alternates — defense in depth under +// the verify-layer round-trip check — and closes the same gap for the +// enrollment and recovery parsers, which have no verify-layer guard. +func isCanonicalDecimal(s string) bool { + if s == "" { + return false + } + if s == "0" { + return true + } + if s[0] == '0' { // leading zero on a multi-digit number + return false + } + for i := 0; i < len(s); i++ { + if s[i] < '0' || s[i] > '9' { // rejects '+', '-', whitespace, any non-digit + return false + } + } + return true +} + +// parseCanonicalUint32 parses a node_id, rejecting any non-canonical +// spelling before delegating to strconv for the range check. +func parseCanonicalUint32(field, s string) (uint32, error) { + if !isCanonicalDecimal(s) { + return 0, fmt.Errorf("%w: %s is not canonical decimal: %q", ErrMalformed, field, s) + } + v, err := strconv.ParseUint(s, 10, 32) + if err != nil { + return 0, fmt.Errorf("%w: %s: %v", ErrMalformed, field, err) + } + return uint32(v), nil +} + +// parseCanonicalInt64 parses a non-negative timestamp/exp. All numeric +// fields on the wire are non-negative (unix seconds, 0 = none), so a +// leading '-' is itself non-canonical and rejected. +func parseCanonicalInt64(field, s string) (int64, error) { + if !isCanonicalDecimal(s) { + return 0, fmt.Errorf("%w: %s is not canonical decimal: %q", ErrMalformed, field, s) + } + v, err := strconv.ParseInt(s, 10, 64) + if err != nil { + return 0, fmt.Errorf("%w: %s: %v", ErrMalformed, field, err) + } + return v, nil +} + // Canonical returns the exact byte string that is signed and that travels // on the wire as the "badge" field. The issuer signs these bytes; the // verifier checks the signature over the bytes it received and only then @@ -149,17 +203,17 @@ func Parse(s string) (Badge, error) { if parts[1] != Version { return Badge{}, fmt.Errorf("%w: unsupported version %q", ErrMalformed, parts[1]) } - nodeID, err := strconv.ParseUint(parts[2], 10, 32) + nodeID, err := parseCanonicalUint32("node_id", parts[2]) if err != nil { - return Badge{}, fmt.Errorf("%w: node_id: %v", ErrMalformed, err) + return Badge{}, err } - verifiedAt, err := strconv.ParseInt(parts[4], 10, 64) + verifiedAt, err := parseCanonicalInt64("verified_at", parts[4]) if err != nil { - return Badge{}, fmt.Errorf("%w: verified_at: %v", ErrMalformed, err) + return Badge{}, err } - exp, err := strconv.ParseInt(parts[5], 10, 64) + exp, err := parseCanonicalInt64("exp", parts[5]) if err != nil { - return Badge{}, fmt.Errorf("%w: exp: %v", ErrMalformed, err) + return Badge{}, err } if parts[3] == "" { return Badge{}, fmt.Errorf("%w: provider is required", ErrMalformed) @@ -168,7 +222,7 @@ func Parse(s string) (Badge, error) { return Badge{}, fmt.Errorf("%w: kid is required", ErrMalformed) } return Badge{ - NodeID: uint32(nodeID), + NodeID: nodeID, Provider: parts[3], VerifiedAt: verifiedAt, Exp: exp, diff --git a/badgeverify/credential.go b/badgeverify/credential.go index 3c20c41..5b92ccc 100644 --- a/badgeverify/credential.go +++ b/badgeverify/credential.go @@ -4,7 +4,6 @@ package badgeverify import ( "fmt" - "strconv" "strings" "time" ) @@ -99,19 +98,19 @@ func ParseEnrollment(s string) (Enrollment, error) { if parts[1] != Version { return Enrollment{}, fmt.Errorf("%w: unsupported version %q", ErrMalformed, parts[1]) } - nodeID, err := strconv.ParseUint(parts[2], 10, 32) + nodeID, err := parseCanonicalUint32("node_id", parts[2]) if err != nil { - return Enrollment{}, fmt.Errorf("%w: node_id: %v", ErrMalformed, err) + return Enrollment{}, err } - issuedAt, err := strconv.ParseInt(parts[5], 10, 64) + issuedAt, err := parseCanonicalInt64("issued_at", parts[5]) if err != nil { - return Enrollment{}, fmt.Errorf("%w: issued_at: %v", ErrMalformed, err) + return Enrollment{}, err } if parts[3] == "" || parts[4] == "" || parts[6] == "" { return Enrollment{}, fmt.Errorf("%w: enrollment has empty required field", ErrMalformed) } return Enrollment{ - NodeID: uint32(nodeID), Provider: parts[3], Commitment: parts[4], + NodeID: nodeID, Provider: parts[3], Commitment: parts[4], IssuedAt: issuedAt, Kid: parts[6], }, nil } @@ -129,19 +128,25 @@ func ParseRecovery(s string) (Recovery, error) { if parts[1] != Version { return Recovery{}, fmt.Errorf("%w: unsupported version %q", ErrMalformed, parts[1]) } - nodeID, err := strconv.ParseUint(parts[2], 10, 32) + nodeID, err := parseCanonicalUint32("node_id", parts[2]) if err != nil { - return Recovery{}, fmt.Errorf("%w: node_id: %v", ErrMalformed, err) + return Recovery{}, err } - exp, err := strconv.ParseInt(parts[5], 10, 64) + exp, err := parseCanonicalInt64("exp", parts[5]) if err != nil { - return Recovery{}, fmt.Errorf("%w: exp: %v", ErrMalformed, err) + return Recovery{}, err + } + // A recovery with exp<=0 is never valid (CanonicalRecovery refuses to + // emit one); reject it at the parse boundary so the round-trip is exact + // and an exp=0 takeover token cannot even be represented. + if exp <= 0 { + return Recovery{}, fmt.Errorf("%w: recovery exp must be set (non-zero)", ErrMalformed) } if parts[3] == "" || parts[4] == "" || parts[6] == "" || parts[7] == "" { return Recovery{}, fmt.Errorf("%w: recovery has empty required field", ErrMalformed) } return Recovery{ - NodeID: uint32(nodeID), NewPubKey: parts[3], Commitment: parts[4], + NodeID: nodeID, NewPubKey: parts[3], Commitment: parts[4], Exp: exp, Nonce: parts[6], Kid: parts[7], }, nil } diff --git a/badgeverify/fuzz_test.go b/badgeverify/fuzz_test.go new file mode 100644 index 0000000..a82eb5e --- /dev/null +++ b/badgeverify/fuzz_test.go @@ -0,0 +1,198 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package badgeverify + +import ( + "crypto/ed25519" + "crypto/rand" + "encoding/base64" + "testing" +) + +// The fuzz suite drives the THREE parsers and the THREE verifiers with +// adversarial input. Two invariants are asserted everywhere: +// +// 1. No panic. A parser/verifier handed hostile bytes must return an +// error, never crash — these run on untrusted, attacker-controlled +// wire data. +// 2. Fail-closed + canonical round-trip. Anything a parser ACCEPTS must +// re-encode to the exact bytes it was handed (no malleability: a second +// encoding of the same logical value must not exist), and no verifier +// ever returns success for input that was not signed by a pinned key. +// +// Seeds are deterministic and small so the corpus stays reproducible and +// the CI fuzz window finds the interesting branches fast. + +// seedBadges returns canonical + a few near-miss badge strings. +func seedBadges() []string { + return []string{ + "pilotbadge:v1:1:github:0:0:v1:", + "pilotbadge:v1:109517:github:1700000000:0:v1:acme-corp", + "pilotbadge:v1:4294967295:google:1700000000:1800000000:v1:label", + "pilotbadge:v1:0:workos:0:0:v1:", + // near-misses / malleability probes + "pilotbadge:v1:01:github:0:0:v1:", // leading-zero node_id + "pilotbadge:v1:+1:github:0:0:v1:", // signed integer form + "pilotbadge:v1:1:github:00:0:v1:", // leading-zero verified_at + "pilotbadge:v1:1:github:0:-0:v1:", // negative-zero exp + "pilotbadge:v1:1:github: 1:0:v1:", // whitespace integer + "pilotbadge:v0:1:github:0:0:v1:", // wrong version + "notpilot:v1:1:github:0:0:v1:", // wrong prefix + "pilotbadge:v1:1:github:0:0:v1", // too few fields + "pilotbadge:v1:1:github:0:0:v1::x", // too many fields + "", + "::::::::", + } +} + +func seedEnrollments() []string { + return []string{ + "pilotenroll:v1:1:github:Y29tbWl0:1700000000:v1", + "pilotenroll:v1:4294967295:google:abc:0:v1", + "pilotenroll:v1:01:github:c:0:v1", // leading-zero node_id + "pilotenroll:v1:1:github:c:+5:v1", // signed issued_at + "pilotbadge:v1:1:github:0:0:v1:", // wrong type (cross-confusion) + "pilotenroll:v1:1::c:0:v1", // empty provider + "pilotenroll:v1:1:github:c:0:v1:x", // too many fields + "", + } +} + +func seedRecoveries() []string { + return []string{ + "pilotrecover:v1:1:bmV3a2V5:Y29tbWl0:1800000000:nonce1:rec", + "pilotrecover:v1:4294967295:np:C:1:n:rec", + "pilotrecover:v1:01:np:C:1:n:rec", // leading-zero node_id + "pilotrecover:v1:1:np:C:01:n:rec", // leading-zero exp + "pilotrecover:v1:1:np:C:0:n:rec", // exp=0 (forbidden) + "pilotrecover:v1:1:np:C:-1:n:rec", // negative exp + "pilotbadge:v1:1:github:0:0:v1:", // wrong type (cross-confusion) + "pilotrecover:v1:1:np::1:n:rec", // empty commitment + "pilotrecover:v1:1:np:C:1:n:rec:extra", // too many fields + "", + } +} + +// FuzzParseBadge asserts Parse never panics and that any accepted badge +// re-encodes to the EXACT input (canonical, non-malleable). +func FuzzParseBadge(f *testing.F) { + for _, s := range seedBadges() { + f.Add(s) + } + f.Fuzz(func(t *testing.T, s string) { + b, err := Parse(s) + if err != nil { + return + } + got, cErr := Canonical(b) + if cErr != nil { + t.Fatalf("Parse accepted %q but Canonical rejected the parsed value: %v", s, cErr) + } + if got != s { + t.Fatalf("malleability: Parse(%q) re-encodes to %q (a non-canonical form was accepted)", s, got) + } + }) +} + +// FuzzParseEnrollment: same round-trip guard for enrollments. +func FuzzParseEnrollment(f *testing.F) { + for _, s := range seedEnrollments() { + f.Add(s) + } + f.Fuzz(func(t *testing.T, s string) { + e, err := ParseEnrollment(s) + if err != nil { + return + } + got, cErr := CanonicalEnrollment(e) + if cErr != nil { + t.Fatalf("ParseEnrollment accepted %q but CanonicalEnrollment rejected it: %v", s, cErr) + } + if got != s { + t.Fatalf("malleability: ParseEnrollment(%q) re-encodes to %q", s, got) + } + }) +} + +// FuzzParseRecovery: same round-trip guard for recovery authorizations. +func FuzzParseRecovery(f *testing.F) { + for _, s := range seedRecoveries() { + f.Add(s) + } + f.Fuzz(func(t *testing.T, s string) { + r, err := ParseRecovery(s) + if err != nil { + return + } + got, cErr := CanonicalRecovery(r) + if cErr != nil { + t.Fatalf("ParseRecovery accepted %q but CanonicalRecovery rejected it: %v", s, cErr) + } + if got != s { + t.Fatalf("malleability: ParseRecovery(%q) re-encodes to %q", s, got) + } + }) +} + +// fuzzKeyring installs a real keyring (online "v1", cold "rec") for the +// verify fuzzers so they exercise the full signature path, not just the +// no-key short-circuit. Reset after the run. +func fuzzKeyring(f *testing.F) { + f.Helper() + oPub, _, _ := ed25519.GenerateKey(rand.Reader) + rPub, _, _ := ed25519.GenerateKey(rand.Reader) + origK, origR := keyringB64, recoveryKeyringB64 + keyringB64 = "v1=" + base64.StdEncoding.EncodeToString(oPub) + recoveryKeyringB64 = "rec=" + base64.StdEncoding.EncodeToString(rPub) + f.Cleanup(func() { keyringB64, recoveryKeyringB64 = origK, origR }) +} + +// FuzzVerify drives Verify with arbitrary (badge, sig) pairs. A pinned key +// exists but the adversary does not hold its private half, so Verify must +// NEVER succeed (fail-closed) and must never panic. +func FuzzVerify(f *testing.F) { + fuzzKeyring(f) + for _, s := range seedBadges() { + f.Add(s, "") + f.Add(s, "!!notbase64") + f.Add(s, base64.StdEncoding.EncodeToString(make([]byte, ed25519.SignatureSize))) + } + f.Fuzz(func(t *testing.T, badge, sig string) { + if _, err := Verify(badge, sig); err == nil { + t.Fatalf("Verify returned success for unsigned input: badge=%q sig=%q", badge, sig) + } + // VerifyForNode is Verify plus a binding check; it must also stay closed. + if _, err := VerifyForNode(badge, sig, 1); err == nil { + t.Fatalf("VerifyForNode returned success for unsigned input: badge=%q sig=%q", badge, sig) + } + }) +} + +// FuzzVerifyEnrollment: enrollment verifier must fail-closed for garbage. +func FuzzVerifyEnrollment(f *testing.F) { + fuzzKeyring(f) + for _, s := range seedEnrollments() { + f.Add(s, "") + f.Add(s, base64.StdEncoding.EncodeToString(make([]byte, ed25519.SignatureSize))) + } + f.Fuzz(func(t *testing.T, s, sig string) { + if _, err := VerifyEnrollment(s, sig); err == nil { + t.Fatalf("VerifyEnrollment returned success for unsigned input: s=%q sig=%q", s, sig) + } + }) +} + +// FuzzVerifyRecovery: recovery verifier must fail-closed for garbage, AND a +// badge-shaped string must never sneak through (cross-statement defense). +func FuzzVerifyRecovery(f *testing.F) { + fuzzKeyring(f) + for _, s := range append(seedRecoveries(), seedBadges()...) { + f.Add(s, "") + f.Add(s, base64.StdEncoding.EncodeToString(make([]byte, ed25519.SignatureSize))) + } + f.Fuzz(func(t *testing.T, s, sig string) { + if _, err := VerifyRecovery(s, sig); err == nil { + t.Fatalf("VerifyRecovery returned success for unsigned input: s=%q sig=%q", s, sig) + } + }) +} diff --git a/badgeverify/redteam_extra_test.go b/badgeverify/redteam_extra_test.go new file mode 100644 index 0000000..b43caee --- /dev/null +++ b/badgeverify/redteam_extra_test.go @@ -0,0 +1,216 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package badgeverify + +import ( + "crypto/ed25519" + "crypto/rand" + "encoding/base64" + "errors" + "testing" + "time" +) + +// This file supplements redteam_test.go with the cross-type and +// non-canonical-encoding attacks that the original suite did not cover +// explicitly: +// +// - confusion in BOTH directions across all three statement types +// - a key that is valid in the OTHER keyring (online vs cold) must not +// verify the wrong statement type +// - non-canonical integer encodings (leading zeros, signs, whitespace) +// rejected at the VERIFY boundary for all three types +// - the all-zero placeholder key fails closed for recovery + enrollment +// too, not just badges + +// signWith returns a base64 detached signature over s. +func signWith(p ed25519.PrivateKey, s string) string { + return base64.StdEncoding.EncodeToString(ed25519.Sign(p, []byte(s))) +} + +// ATTACK: a legitimately-signed badge must not be accepted by the +// enrollment verifier, and a legitimately-signed enrollment must not be +// accepted by the badge or recovery verifier. (Completes the confusion +// matrix that redteam_test.go covered only for recovery<->badge.) +func TestRedteamExtra_EnrollmentConfusionBothWays(t *testing.T) { + signBadge, _, badgePriv, _ := redteam(t) + _ = signBadge + + // A valid enrollment under the online badge key. + enr, err := CanonicalEnrollment(Enrollment{NodeID: 1, Provider: "github", Commitment: "C", IssuedAt: 100, Kid: "bdg"}) + if err != nil { + t.Fatalf("canonical enrollment: %v", err) + } + enrSig := signWith(badgePriv, enr) + if _, err := VerifyEnrollment(enr, enrSig); err != nil { + t.Fatalf("control: legit enrollment should verify: %v", err) + } + + // Enrollment must NOT verify as a badge (wrong prefix → ErrMalformed). + if _, err := Verify(enr, enrSig); !errors.Is(err, ErrMalformed) { + t.Errorf("enrollment accepted as badge: %v", err) + } + // Enrollment must NOT verify as a recovery. + if _, err := VerifyRecovery(enr, enrSig); !errors.Is(err, ErrMalformed) { + t.Errorf("enrollment accepted as recovery: %v", err) + } + + // A valid badge must NOT verify as an enrollment. + b, _ := Canonical(Badge{NodeID: 1, Provider: "github", Kid: "bdg"}) + bSig := signWith(badgePriv, b) + if _, err := VerifyEnrollment(b, bSig); !errors.Is(err, ErrMalformed) { + t.Errorf("badge accepted as enrollment: %v", err) + } +} + +// ATTACK: a key that is valid in ONE keyring must not verify a statement +// routed to the OTHER keyring. The cold recovery key signs an enrollment +// naming the recovery kid; enrollment verifies against the ONLINE keyring, +// which has no "rec" entry → fail closed. +func TestRedteamExtra_WrongKeyringRejected(t *testing.T) { + _, _, _, recPriv := redteam(t) + + // Enrollment naming the cold kid, signed by the cold key. + enr, _ := CanonicalEnrollment(Enrollment{NodeID: 1, Provider: "github", Commitment: "C", IssuedAt: 100, Kid: "rec"}) + sig := signWith(recPriv, enr) + // Online keyring has no "rec" → ErrNoKey, never an accidental accept. + if _, err := VerifyEnrollment(enr, sig); !errors.Is(err, ErrNoKey) { + t.Fatalf("enrollment under cold kid must fail closed, got %v", err) + } + + // And a badge naming the cold kid likewise fails closed. + b, _ := Canonical(Badge{NodeID: 1, Provider: "github", Kid: "rec"}) + if _, err := Verify(b, signWith(recPriv, b)); !errors.Is(err, ErrNoKey) { + t.Fatalf("badge under cold kid must fail closed, got %v", err) + } +} + +// ATTACK: a key from a DIFFERENT, unrelated keyring (attacker's own pinned +// world) must not verify our statements. Generate a fresh keypair, pin it +// under the same kid name in a throwaway keyring, and confirm a signature +// from the REAL issuer no longer verifies once the pinned key is swapped. +func TestRedteamExtra_KeyFromOtherKeyringRejected(t *testing.T) { + signBadge, _, _, _ := redteam(t) + b, _ := Canonical(Badge{NodeID: 1, Provider: "github", Kid: "bdg"}) + goodSig := signBadge(b) + if _, err := Verify(b, goodSig); err != nil { + t.Fatalf("control: should verify under real key: %v", err) + } + + // Swap the pinned "bdg" key for an unrelated one; the real signature + // must now be rejected (proves verify is bound to the pinned key, not + // merely to any key named "bdg"). + otherPub, _, _ := ed25519.GenerateKey(rand.Reader) + orig := keyringB64 + keyringB64 = "bdg=" + base64.StdEncoding.EncodeToString(otherPub) + t.Cleanup(func() { keyringB64 = orig }) + if _, err := Verify(b, goodSig); !errors.Is(err, ErrBadSignature) { + t.Fatalf("signature must not verify under a different pinned key, got %v", err) + } +} + +// ATTACK: non-canonical integer encodings must be rejected at the VERIFY +// boundary for ALL THREE statement types — even when correctly signed — +// because the signed bytes are the non-canonical form and the parser must +// refuse the alternate encoding (no malleable second representation). +func TestRedteamExtra_NonCanonicalIntegersRejected(t *testing.T) { + signBadge, signRecover, badgePriv, _ := redteam(t) + _ = signBadge + + // Each entry is a hand-built wire string with a non-canonical integer. + // We sign the exact bytes (hostile issuer) and confirm parse/verify + // still rejects them as malformed. + type tc struct { + name string + s string + verA func(s, sig string) error + } + verBadge := func(s, sig string) error { _, e := Verify(s, sig); return e } + verEnr := func(s, sig string) error { _, e := VerifyEnrollment(s, sig); return e } + verRec := func(s, sig string) error { _, e := VerifyRecovery(s, sig); return e } + + cases := []tc{ + // Badge: leading-zero / signed / whitespace integers. + {"badge leading-zero node", "pilotbadge:" + Version + ":01:github:0:0:bdg:", verBadge}, + {"badge signed node", "pilotbadge:" + Version + ":+1:github:0:0:bdg:", verBadge}, + {"badge leading-zero verifiedat", "pilotbadge:" + Version + ":1:github:00:0:bdg:", verBadge}, + {"badge whitespace exp", "pilotbadge:" + Version + ":1:github:0: 1:bdg:", verBadge}, + // Enrollment. + {"enroll leading-zero node", "pilotenroll:" + Version + ":01:github:C:0:bdg", verEnr}, + {"enroll signed issuedat", "pilotenroll:" + Version + ":1:github:C:+5:bdg", verEnr}, + // Recovery. + {"recover leading-zero node", "pilotrecover:" + Version + ":01:np:C:1:n:rec", verRec}, + {"recover leading-zero exp", "pilotrecover:" + Version + ":1:np:C:01:n:rec", verRec}, + } + + signFor := func(s string) string { + // recovery strings are signed by the cold key, everything else by online. + if len(s) >= len(prefixRecover) && s[:len(prefixRecover)] == prefixRecover { + return signRecover(s) + } + return signWith(badgePriv, s) + } + + for _, c := range cases { + if err := c.verA(c.s, signFor(c.s)); !errors.Is(err, ErrMalformed) { + t.Errorf("%s: non-canonical integer must be rejected as malformed, got %v", c.name, err) + } + } +} + +// ATTACK: the all-zero placeholder key (forgot the -ldflags override) must +// fail closed for recovery and enrollment too, not just badges. +func TestRedteamExtra_PlaceholderKeyFailsClosedAllTypes(t *testing.T) { + // Restore the compiled-in all-zero placeholders for the duration. + origK, origR := keyringB64, recoveryKeyringB64 + keyringB64 = "v1=AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=" + recoveryKeyringB64 = "rec-v1=AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=" + t.Cleanup(func() { keyringB64, recoveryKeyringB64 = origK, origR }) + + _, priv, _ := ed25519.GenerateKey(rand.Reader) + + enr, _ := CanonicalEnrollment(Enrollment{NodeID: 1, Provider: "github", Commitment: "C", IssuedAt: 1, Kid: "v1"}) + if _, err := VerifyEnrollment(enr, signWith(priv, enr)); !errors.Is(err, ErrNoKey) { + t.Errorf("enrollment under placeholder key must fail closed, got %v", err) + } + + rec, _ := CanonicalRecovery(Recovery{NodeID: 1, NewPubKey: "np", Commitment: "C", Exp: time.Now().Add(time.Minute).Unix(), Nonce: "n", Kid: "rec-v1"}) + if _, err := VerifyRecovery(rec, signWith(priv, rec)); !errors.Is(err, ErrNoKey) { + t.Errorf("recovery under placeholder key must fail closed, got %v", err) + } +} + +// ATTACK: a never-expiring badge is ALLOWED (Exp==0 means no expiry) and +// must verify; a never-expiring recovery is DISALLOWED and must be rejected +// even when correctly signed. Pins the asymmetry the spec requires. +func TestRedteamExtra_NeverExpiringPolicyAsymmetry(t *testing.T) { + signBadge, signRecover, _, _ := redteam(t) + + // Badge with Exp==0 verifies (no-expiry is a legitimate badge). + b, _ := Canonical(Badge{NodeID: 1, Provider: "github", Exp: 0, Kid: "bdg"}) + if _, err := Verify(b, signBadge(b)); err != nil { + t.Fatalf("no-expiry badge must verify, got %v", err) + } + + // Recovery with exp=0, signed correctly by the cold key, must be rejected. + rec := "pilotrecover:" + Version + ":1:np:C:0:n:rec" + if _, err := VerifyRecovery(rec, signRecover(rec)); !errors.Is(err, ErrMalformed) { + t.Fatalf("no-expiry recovery must be rejected even when signed, got %v", err) + } +} + +// ATTACK: node-binding mismatch with a CORRECTLY signed badge — the +// signature is valid but the badge is bound to a different node than the +// authenticated peer. (redteam_test.go covers the unsigned-replay path; +// this pins the signed-but-wrong-node path through VerifyForNode.) +func TestRedteamExtra_SignedBadgeWrongNodeRejected(t *testing.T) { + signBadge, _, _, _ := redteam(t) + b, _ := Canonical(Badge{NodeID: 0x1234, Provider: "github", Kid: "bdg"}) + sig := signBadge(b) + if _, err := VerifyForNode(b, sig, 0x1234); err != nil { + t.Fatalf("matching node must verify, got %v", err) + } + if _, err := VerifyForNode(b, sig, 0x5678); !errors.Is(err, ErrNodeMismatch) { + t.Fatalf("signed badge bound to wrong node must fail, got %v", err) + } +} diff --git a/go.mod b/go.mod index 933a51d..35bc3d5 100644 --- a/go.mod +++ b/go.mod @@ -1,3 +1,3 @@ module github.com/pilot-protocol/common -go 1.25.10 +go 1.25.11 From b6ec4a42b294795837385bddcb2ffe06945def24 Mon Sep 17 00:00:00 2001 From: Teodor Calin Date: Mon, 22 Jun 2026 16:30:49 +0300 Subject: [PATCH 2/2] Run gitleaks CLI directly and soft-gate dependency-review The gitleaks Action requires a paid GITLEAKS_LICENSE on org repos; run the OSS gitleaks binary instead (no license, fails on any finding). dependency-review hard-errors until the repo Dependency Graph feature is enabled, so mark it continue-on-error until an admin enables it. --- .github/workflows/dependency-review.yml | 5 +++++ .github/workflows/security.yml | 17 +++++++++++++---- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/.github/workflows/dependency-review.yml b/.github/workflows/dependency-review.yml index 30a88cf..e4fbb3e 100644 --- a/.github/workflows/dependency-review.yml +++ b/.github/workflows/dependency-review.yml @@ -13,7 +13,12 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 + # The action hard-errors until the repo's Dependency Graph feature is + # enabled (Settings > Code security). continue-on-error keeps the job + # from blocking PRs before an admin flips that switch; once enabled the + # step reports real findings. Flip this to a hard gate after enabling. - name: Dependency Review + continue-on-error: true uses: actions/dependency-review-action@v4 with: fail-on-severity: high diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index df89c4c..7fcdca5 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -82,10 +82,19 @@ jobs: - uses: actions/checkout@v4 with: fetch-depth: 0 - - name: gitleaks - uses: gitleaks/gitleaks-action@v2 - env: - GITLEAKS_ENABLE_UPLOAD_ARTIFACT: "false" + # The gitleaks GitHub Action requires a paid GITLEAKS_LICENSE secret on + # organization repos. The gitleaks CLI itself is OSS and unrestricted, + # so we run the pinned binary directly — full-history secret scan, no + # license gate, fails the job on any finding. + - name: Install gitleaks + run: | + set -euo pipefail + VERSION=8.21.2 + curl -sSL "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: Scan repository history + run: gitleaks detect --source . --redact --verbose --exit-code 1 fuzz: name: badgeverify fuzz