Fail closed when gh is absent during attestation

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)

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)
+		}
+	})
+}

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()
 

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