From 829ca659b664efc380394d34c20a595217bde5a7 Mon Sep 17 00:00:00 2001 From: Teodor Calin Date: Mon, 22 Jun 2026 14:17:42 +0300 Subject: [PATCH] Fail closed when gh is absent during attestation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The SLSA provenance gate for checksums.txt called gh attestation verify, but verifyChecksumsAttestationFn returned nil (pass) whenever gh was not on PATH. Headless production hosts never install gh (install.sh omits it), so the provenance check was a silent no-op there — collapsing auto-update integrity back to "anyone with GitHub repo-write access can ship a malicious release." Make attestation fail closed: when gh is missing and SkipAttestation is false, verification now returns an error and the update is refused. The only bypass is the explicit Config.SkipAttestation opt-out (intended for test repos without real attestations). The mandatory SHA256 checksums.txt match is unchanged and still enforced regardless of SkipAttestation, and both RunOnce and the gated checkLoop share applyUpdate so neither can skip the gate. Replace the test that codified "gh absent => pass" with one asserting fail-closed, and add coverage for SkipAttestation=true still requiring a SHA256 match (pass on match, refuse on mismatch). --- updater.go | 46 +++++++++------ zz_checksums_required_bug_test.go | 93 +++++++++++++++++++++++++++++++ zz_coverage_test.go | 47 +++++++++++++--- zz_test.go | 10 ++-- 4 files changed, 168 insertions(+), 28 deletions(-) diff --git a/updater.go b/updater.go index 9ead82a..532c35d 100644 --- a/updater.go +++ b/updater.go @@ -53,9 +53,13 @@ type Config struct { PinnedVersion string // SkipAttestation disables SLSA attestation verification of - // checksums.txt. Intended for test environments where the test - // repos do not have real attestations. Default (false) enables - // verification in production. + // checksums.txt. This is the ONLY way to bypass attestation: when + // false (the default) and the gh CLI is absent, verification fails + // closed and the update is refused. Intended for test environments + // where the test repos do not have real attestations. Leaving it + // false in production keeps provenance verification mandatory. + // The SHA256 checksums.txt match is always enforced regardless of + // this flag. SkipAttestation bool // StatePath, when non-empty, points to a JSON control file @@ -403,8 +407,8 @@ func (u *Updater) applyUpdate(release *GitHubRelease) error { // actions/attest-build-provenance@v2 (PILOT-120, PR #166). // This closes the "attacker publishes matched fake binary + // fake checksums.txt" gap — the attestation ties checksums.txt - // to the trusted CI workflow. Graceful skip when gh CLI is - // unavailable (operator directive: not every environment has it). + // to the trusted CI workflow. Fails closed when the gh CLI is + // unavailable unless SkipAttestation is explicitly set. if err := u.verifyChecksumsAttestation(checksumsPath); err != nil { return fmt.Errorf("checksums attestation verification failed: %w", err) } @@ -756,27 +760,35 @@ func (u *Updater) signalDaemonRestartLinux() { // binary + fake checksums.txt" gap — the attestation ties checksums.txt to // the trusted CI workflow identity. // -// If gh is not on PATH, we log a warning and return nil (graceful skip per -// operator directive: not every environment has gh installed). If gh is -// present but verification fails, we return an error — the checksums file -// cannot be trusted. +// This gate FAILS CLOSED: if the gh CLI is not on PATH, verification returns +// an error and the update does not proceed. The only way to skip attestation +// is to set Config.SkipAttestation explicitly — there is no implicit skip. +// This is deliberate: the prior "gh absent => pass" behaviour silently +// disabled the entire provenance gate on headless production hosts (install.sh +// never installs gh), collapsing auto-update integrity back to "anyone with +// GitHub repo-write access can ship a malicious release." func (u *Updater) verifyChecksumsAttestation(checksumsPath string) error { if u.config.SkipAttestation { - slog.Debug("skipping attestation verification (SkipAttestation=true)") + slog.Warn("SLSA attestation verification disabled (SkipAttestation=true) — checksums provenance is NOT verified") return nil } return verifyChecksumsAttestationFn(u.config.Repo, checksumsPath) } -// verifyChecksumsAttestationFn is the default attestation verification -// implementation. Tests may replace it to avoid requiring a real GitHub -// repo with SLSA attestations. -var verifyChecksumsAttestationFn = func(repo, checksumsPath string) error { +// verifyChecksumsAttestationFn is the attestation verification implementation +// used by the updater. Tests may replace it to avoid requiring a real GitHub +// repo with SLSA attestations; they restore realVerifyChecksumsAttestationFn +// to exercise the production path. +var verifyChecksumsAttestationFn = realVerifyChecksumsAttestationFn + +// realVerifyChecksumsAttestationFn is the production attestation check. It +// fails closed when gh is absent: callers only reach this function when +// SkipAttestation is false, so a missing gh means provenance cannot be +// established and the update must be refused. +func realVerifyChecksumsAttestationFn(repo, checksumsPath string) error { ghPath, err := exec.LookPath("gh") if err != nil { - slog.Warn("gh CLI not found — skipping attestation verification; install gh for full provenance guarantee", - "install_url", "https://cli.github.com/") - return nil + return fmt.Errorf("gh CLI required for SLSA attestation verification; install gh (https://cli.github.com/) or set SkipAttestation to bypass: %w", err) } cmd := exec.Command(ghPath, "attestation", "verify", checksumsPath, "--repo", repo) diff --git a/zz_checksums_required_bug_test.go b/zz_checksums_required_bug_test.go index 7f69651..712a762 100644 --- a/zz_checksums_required_bug_test.go +++ b/zz_checksums_required_bug_test.go @@ -196,3 +196,96 @@ func TestApplyUpdate_PassesWithValidChecksums(t *testing.T) { t.Fatalf("green-path install failed: %v", err) } } + +// TestApplyUpdate_SkipAttestationStillRequiresChecksum verifies that +// SkipAttestation=true bypasses ONLY the SLSA attestation check — the +// mandatory SHA256 checksums.txt match is still enforced. A matching +// archive installs; a tampered archive is refused. The real attestation +// function is restored so the test proves SkipAttestation is the live +// bypass, not the TestMain stub. +func TestApplyUpdate_SkipAttestationStillRequiresChecksum(t *testing.T) { + // Not parallel: restores the package-level verifyChecksumsAttestationFn. + origFn := verifyChecksumsAttestationFn + verifyChecksumsAttestationFn = realVerifyChecksumsAttestationFn + defer func() { verifyChecksumsAttestationFn = origFn }() + + archiveName := fmt.Sprintf("pilot-%s-%s.tar.gz", runtime.GOOS, runtime.GOARCH) + + newServer := func(t *testing.T, archiveContent []byte, checksumsContent string) *httptest.Server { + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/download/" + archiveName: + w.Write(archiveContent) + case "/download/checksums.txt": + w.Write([]byte(checksumsContent)) + default: + http.NotFound(w, r) + } + })) + } + + newRelease := func(srvURL string) *GitHubRelease { + return &GitHubRelease{ + TagName: "v1.1.0", + Assets: []GitHubAsset{ + {Name: archiveName, BrowserDownloadURL: srvURL + "/download/" + archiveName}, + {Name: "checksums.txt", BrowserDownloadURL: srvURL + "/download/checksums.txt"}, + }, + } + } + + t.Run("matching checksum installs", func(t *testing.T) { + installDir := filepath.Join(t.TempDir(), "bin") + os.MkdirAll(installDir, 0755) + os.WriteFile(filepath.Join(installDir, ".pilot-version"), []byte("v1.0.0\n"), 0644) + + archivePath := filepath.Join(t.TempDir(), archiveName) + createTestTarGz(t, archivePath, map[string]string{"daemon": "good-content"}) + archiveContent, _ := os.ReadFile(archivePath) + hash := sha256.Sum256(archiveContent) + checksumsContent := fmt.Sprintf("%x %s\n", hash, archiveName) + + srv := newServer(t, archiveContent, checksumsContent) + defer srv.Close() + + u := &Updater{ + config: Config{InstallDir: installDir, SkipAttestation: true}, + client: srv.Client(), + stopCh: make(chan struct{}), + exitFn: func(int) {}, + } + if err := u.applyUpdate(newRelease(srv.URL)); err != nil { + t.Fatalf("SkipAttestation=true with matching checksum should install: %v", err) + } + }) + + t.Run("checksum mismatch is refused", func(t *testing.T) { + installDir := filepath.Join(t.TempDir(), "bin") + os.MkdirAll(installDir, 0755) + os.WriteFile(filepath.Join(installDir, ".pilot-version"), []byte("v1.0.0\n"), 0644) + + archivePath := filepath.Join(t.TempDir(), archiveName) + createTestTarGz(t, archivePath, map[string]string{"daemon": "good-content"}) + archiveContent, _ := os.ReadFile(archivePath) + // Checksum for DIFFERENT content — simulates a tampered archive. + wrongHash := sha256.Sum256([]byte("attacker-controlled")) + checksumsContent := fmt.Sprintf("%x %s\n", wrongHash, archiveName) + + srv := newServer(t, archiveContent, checksumsContent) + defer srv.Close() + + u := &Updater{ + config: Config{InstallDir: installDir, SkipAttestation: true}, + client: srv.Client(), + stopCh: make(chan struct{}), + exitFn: func(int) {}, + } + err := u.applyUpdate(newRelease(srv.URL)) + if err == nil { + t.Fatal("SkipAttestation=true must still enforce SHA256: mismatch should fail") + } + if !strings.Contains(err.Error(), "checksum") { + t.Errorf("expected checksum verification error, got: %v", err) + } + }) +} diff --git a/zz_coverage_test.go b/zz_coverage_test.go index d7e1f3c..d42219e 100644 --- a/zz_coverage_test.go +++ b/zz_coverage_test.go @@ -787,23 +787,56 @@ func TestArchiveToInstallMapping(t *testing.T) { } } -// TestVerifyChecksumsAttestation_GhNotInstalled verifies graceful -// skip when gh CLI is not on PATH. +// TestVerifyChecksumsAttestation_GhNotInstalled verifies the gate FAILS +// CLOSED when the gh CLI is not on PATH. The previous behaviour (silent +// pass) turned the SLSA provenance gate into a no-op on every headless +// production host, since install.sh never installs gh. func TestVerifyChecksumsAttestation_GhNotInstalled(t *testing.T) { - t.Parallel() + // Not parallel: mutates the process-wide PATH. // Temporarily unset PATH so LookPath fails. origPath := os.Getenv("PATH") os.Setenv("PATH", "") defer os.Setenv("PATH", origPath) - err := verifyChecksumsAttestationFn("test/repo", "/nonexistent/checksums.txt") - if err != nil { - t.Errorf("expected graceful skip when gh not installed, got error: %v", err) + // Call the real implementation directly — TestMain stubs the package + // variable to a nil-returning func for the rest of the suite. + err := realVerifyChecksumsAttestationFn("test/repo", "/nonexistent/checksums.txt") + if err == nil { + t.Fatal("expected error when gh is absent (fail closed), got nil") + } + if !strings.Contains(err.Error(), "gh CLI required") { + t.Errorf("error should explain gh is required, got: %v", err) + } +} + +// TestVerifyChecksumsAttestation_FailsClosedWithoutSkip verifies that the +// wrapper refuses the update when gh is absent and SkipAttestation is false. +func TestVerifyChecksumsAttestation_FailsClosedWithoutSkip(t *testing.T) { + // Not parallel: mutates the process-wide PATH and the package-level + // verifyChecksumsAttestationFn. + origFn := verifyChecksumsAttestationFn + verifyChecksumsAttestationFn = realVerifyChecksumsAttestationFn + defer func() { verifyChecksumsAttestationFn = origFn }() + + origPath := os.Getenv("PATH") + os.Setenv("PATH", "") + defer os.Setenv("PATH", origPath) + + u := New(Config{ + InstallDir: t.TempDir(), + Repo: "test/repo", + SkipAttestation: false, + }) + + err := u.verifyChecksumsAttestation("/nonexistent/checksums.txt") + if err == nil { + t.Fatal("expected fail-closed error when gh absent and SkipAttestation=false, got nil") } } -// TestVerifyChecksumsAttestation_SkipConfig verifies the config-driven skip. +// TestVerifyChecksumsAttestation_SkipConfig verifies the config-driven skip +// is the only way to bypass attestation. func TestVerifyChecksumsAttestation_SkipConfig(t *testing.T) { t.Parallel() diff --git a/zz_test.go b/zz_test.go index 007dafd..dae630d 100644 --- a/zz_test.go +++ b/zz_test.go @@ -18,10 +18,12 @@ import ( "time" ) -// TestMain disables attestation verification for all tests — test repos -// don't have real GitHub SLSA attestations, and the test environment may -// not have the gh CLI installed. A dedicated TestVerifyChecksumsAttestation -// test exercises the real function in isolation. +// TestMain stubs attestation verification for the bulk of the suite — +// test repos don't have real GitHub SLSA attestations, and the test +// environment may not have the gh CLI installed. Tests that need the +// production fail-closed behaviour restore realVerifyChecksumsAttestationFn +// explicitly (see TestVerifyChecksumsAttestation_* and +// TestApplyUpdate_SkipAttestationStillRequiresChecksum). func TestMain(m *testing.M) { verifyChecksumsAttestationFn = func(repo, checksumsPath string) error { return nil