From 50238698b0817725df988f0e7651d2bc7cf623ba Mon Sep 17 00:00:00 2001 From: shujaat hasan Date: Fri, 24 Jul 2026 15:50:00 +0200 Subject: [PATCH 1/2] fix: resolve Bugbot findings on promotion PR #397 (TLS floor + delete cache) - installer: pin the TLS 1.2 floor (--tlsv1.2) on the shared installerRunScript download, matching scripts/install.sh, so `tracebloc upgrade` and `prepare-host` can never negotiate a weaker protocol - update-check: skip the post-command nudge after `tracebloc delete` (reuse SkipUpdateNudge) AND make writeUpdateCache refuse to recreate a missing ~/.tracebloc, so a clean offboard is never resurrected by the throttle cache Co-Authored-By: Claude Opus 4.8 --- internal/cli/delete.go | 16 +++++++++++++++- internal/cli/prepare_host.go | 9 ++++++--- internal/cli/prepare_host_test.go | 16 ++++++++++++++++ internal/cli/update_check.go | 12 ++++++++++-- internal/cli/update_check_test.go | 28 ++++++++++++++++++++++++++++ internal/cli/upgrade.go | 15 ++++++++++++--- internal/cli/upgrade_test.go | 9 +++++++-- 7 files changed, 94 insertions(+), 11 deletions(-) diff --git a/internal/cli/delete.go b/internal/cli/delete.go index cf27d6a..9efcf9d 100644 --- a/internal/cli/delete.go +++ b/internal/cli/delete.go @@ -44,6 +44,20 @@ type deleteOpts struct { namespace string } +// deleteCmdName is the top-level offboard command. main skips the post-command +// update nudge after it (see isDeleteCommand / SkipUpdateNudge): the CLI has just +// removed itself and, on the default path, wiped ~/.tracebloc. +const deleteCmdName = "delete" + +// isDeleteCommand reports whether the executed command is `tracebloc delete`, so +// main can skip the post-command update nudge. Offboarding removes the running +// CLI and (unless --keep-data) wipes ~/.tracebloc, so nudging to upgrade a +// just-removed binary is nonsensical — and the nudge's cache write would recreate +// the just-offboarded host data dir (Bugbot #397). +func isDeleteCommand(cmd *cobra.Command) bool { + return cmd != nil && cmd.Name() == deleteCmdName +} + // newDeleteCmd wires the TOP-LEVEL `tracebloc delete` — offboarding this machine // (RFC-0001 §7.10). It is deliberately NOT under `client` and NOT // `client delete --uninstall`: on the single-machine CLI the host owns exactly @@ -64,7 +78,7 @@ type deleteOpts struct { func newDeleteCmd() *cobra.Command { var o deleteOpts cmd := &cobra.Command{ - Use: "delete", + Use: deleteCmdName, Short: "Offboard this machine from tracebloc (revoke, uninstall, reclaim disk)", Long: `Removes tracebloc from this machine: revokes the machine credential, uninstalls the Helm release, deletes the local cluster, reclaims the tracebloc diff --git a/internal/cli/prepare_host.go b/internal/cli/prepare_host.go index 2a00d77..d946aeb 100644 --- a/internal/cli/prepare_host.go +++ b/internal/cli/prepare_host.go @@ -35,8 +35,11 @@ var prepareHostUserRe = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9._-]{0,31}$`) // error) abort with a non-zero status instead of silently running nothing. // (`curl | bash` swallowed this — bash read empty stdin and exited 0.) // -// The temp file is removed on exit. The URL comes from installerURL (doctor.go) -// so every installer path shares one source and can't drift (Bugbot #394/#397). +// The download uses `--tlsv1.2` — the TLS 1.2 floor scripts/install.sh enforces +// on every security-sensitive fetch — so this privileged installer download can +// never negotiate a weaker protocol (Bugbot #397). The temp file is removed on +// exit. The URL comes from installerURL (doctor.go) so every installer path +// shares one source and can't drift (Bugbot #394/#397). func installerRunScript(subcommand string) string { run := `bash "$tmp"` if subcommand != "" { @@ -45,7 +48,7 @@ func installerRunScript(subcommand string) string { return `set -e tmp="$(mktemp)" trap 'rm -f "$tmp"' EXIT -curl -fsSL ` + installerURL + ` -o "$tmp" +curl -fsSL --tlsv1.2 ` + installerURL + ` -o "$tmp" ` + run } diff --git a/internal/cli/prepare_host_test.go b/internal/cli/prepare_host_test.go index 799995a..1da7d94 100644 --- a/internal/cli/prepare_host_test.go +++ b/internal/cli/prepare_host_test.go @@ -25,6 +25,22 @@ func TestPrepareHostCmdFailsClosedOnDownloadError(t *testing.T) { } } +// Every curl in the shared installer script must pin the TLS 1.2 floor +// (--tlsv1.2), matching scripts/install.sh, so this privileged download can never +// negotiate a weaker protocol. Guards both the upgrade (no subcommand) and +// prepare-host paths since they share installerRunScript (Bugbot #397). +func TestInstallerRunScriptPinsTLSFloor(t *testing.T) { + for _, sub := range []string{"", "prepare-host"} { + script := installerRunScript(sub) + if !strings.Contains(script, "curl") { + t.Fatalf("installerRunScript(%q) must curl the installer; got: %q", sub, script) + } + if !strings.Contains(script, "--tlsv1.2") { + t.Errorf("installerRunScript(%q) must pin --tlsv1.2 on the download (matches install.sh); got: %q", sub, script) + } + } +} + // The installer must NOT be fed to bash over a pipe: `curl | bash -s` makes the // inner bash read its program from the pipe, stealing the installer's stdin so // any interactive prompt in prepare-host gets EOF. We download and run a file diff --git a/internal/cli/update_check.go b/internal/cli/update_check.go index b694347..e4d319e 100644 --- a/internal/cli/update_check.go +++ b/internal/cli/update_check.go @@ -149,12 +149,20 @@ func readUpdateCache(path string) (updateCache, bool) { return c, true } +// writeUpdateCache persists the throttle state next to the config. Best-effort +// (every caller ignores the error) and, critically, it must NOT create the +// tracebloc data directory: after `tracebloc delete` wipes ~/.tracebloc, the +// post-command update check still runs, and an MkdirAll here would resurrect the +// just-offboarded host data dir (Bugbot #397; RFC-0003 offboard hygiene). So we +// only write when the dir already exists — its lifecycle is owned by +// login/client-create and delete, never by a throttle cache. A missing dir is a +// silent no-op (the throttle simply isn't persisted until the dir exists again). func writeUpdateCache(path string, c updateCache) error { if path == "" { return nil } - if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { - return err + if _, err := os.Stat(filepath.Dir(path)); err != nil { + return nil // dir gone (fresh machine, or just-offboarded) — don't recreate it } raw, err := json.Marshal(c) if err != nil { diff --git a/internal/cli/update_check_test.go b/internal/cli/update_check_test.go index f26a2c2..3a5bb3f 100644 --- a/internal/cli/update_check_test.go +++ b/internal/cli/update_check_test.go @@ -54,6 +54,34 @@ func TestUpdateCacheRoundTrip(t *testing.T) { } } +// writeUpdateCache must never create the config dir: after `tracebloc delete` +// wipes ~/.tracebloc, the post-command update check still runs, and recreating +// the dir to write the throttle cache would resurrect the just-offboarded host +// data dir. A missing dir is a silent no-op — no file, no dir (Bugbot #397). +func TestWriteUpdateCache_DoesNotResurrectMissingDir(t *testing.T) { + base := t.TempDir() + gone := filepath.Join(base, "wiped") // deliberately never created + path := filepath.Join(gone, updateCacheFile) + + if err := writeUpdateCache(path, updateCache{CheckedAt: time.Now(), Latest: "0.9.9"}); err != nil { + t.Fatalf("writeUpdateCache to a missing dir must be a silent no-op, got err: %v", err) + } + if _, err := os.Stat(gone); !os.IsNotExist(err) { + t.Errorf("writeUpdateCache must not create the missing dir %s (err=%v)", gone, err) + } + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Errorf("writeUpdateCache must not write a cache file into a missing dir: %v", err) + } + + // Sanity: with the dir present it still writes (round-trips). + if err := writeUpdateCache(filepath.Join(base, updateCacheFile), updateCache{CheckedAt: time.Now(), Latest: "1.0.0"}); err != nil { + t.Fatalf("writeUpdateCache into an existing dir must succeed: %v", err) + } + if _, ok := readUpdateCache(filepath.Join(base, updateCacheFile)); !ok { + t.Error("cache should be present after writing into an existing dir") + } +} + func TestFetchLatestRelease(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { _, _ = w.Write([]byte(`{"tag_name":"v0.9.7","name":"ignored"}`)) diff --git a/internal/cli/upgrade.go b/internal/cli/upgrade.go index d0e27b8..a2522bb 100644 --- a/internal/cli/upgrade.go +++ b/internal/cli/upgrade.go @@ -68,9 +68,18 @@ func isUpgradeCommand(cmd *cobra.Command) bool { return cmd != nil && cmd.Name() == upgradeCmdName } -// SkipUpdateNudge reports whether the update nudge should be suppressed for the -// command that just ran. Exported for main, which owns the nudge call. -func SkipUpdateNudge(cmd *cobra.Command) bool { return isUpgradeCommand(cmd) } +// SkipUpdateNudge reports whether the post-command update nudge (and the cache +// write it triggers) must be suppressed for the command that just ran. Exported +// for main, which owns the nudge call. Suppressed after: +// - upgrade: the running process swapped its own binary and now carries the +// stale compile-time version — it would nudge about the release it just +// installed. +// - delete: offboarding removed the CLI and (on the default path) wiped +// ~/.tracebloc, so the nudge is nonsensical and its cache write would +// resurrect the just-offboarded host data dir (Bugbot #397). +func SkipUpdateNudge(cmd *cobra.Command) bool { + return isUpgradeCommand(cmd) || isDeleteCommand(cmd) +} // newUpgradeCmd implements `tracebloc upgrade` — the apply step the update // nudge (update_check.go) and the 426 "too old" error both point at. diff --git a/internal/cli/upgrade_test.go b/internal/cli/upgrade_test.go index 61f41d0..4c66426 100644 --- a/internal/cli/upgrade_test.go +++ b/internal/cli/upgrade_test.go @@ -92,13 +92,18 @@ func TestUpgradePlanFor_PerOS(t *testing.T) { // TestSkipUpdateNudge: the nudge must be suppressed right after `tracebloc // upgrade` (the running process is stale-by-design once it swaps its own binary) +// AND after `tracebloc delete` (offboarding removed the CLI and wiped +// ~/.tracebloc — the nudge's cache write would resurrect the dir; Bugbot #397), // but fire for any other command. func TestSkipUpdateNudge(t *testing.T) { if !SkipUpdateNudge(newUpgradeCmd()) { t.Error("upgrade command must skip the update nudge") } - if SkipUpdateNudge(newDeleteCmd()) { - t.Error("non-upgrade command must not skip the nudge") + if !SkipUpdateNudge(newDeleteCmd()) { + t.Error("delete command must skip the update nudge (its cache write would resurrect the wiped ~/.tracebloc)") + } + if SkipUpdateNudge(newDoctorCmd(false)) { + t.Error("an ordinary command (doctor) must not skip the nudge") } if SkipUpdateNudge(nil) { t.Error("nil command must not skip the nudge") From db9c2abe526356361ae6f3fdf43f40548cdf0308 Mon Sep 17 00:00:00 2001 From: shujaat hasan Date: Fri, 24 Jul 2026 15:57:49 +0200 Subject: [PATCH 2/2] fix: drive update-nudge skip by annotation, not command name (Bugbot #404) isDeleteCommand matched any leaf named "delete", so the nudge was wrongly skipped after `data delete` too (which neither offboards nor wipes ~/.tracebloc). Replace the name-based upgrade/delete predicates with a self-declared cobra annotation (skipUpdateNudgeAnnotation) set on the top-level upgrade and delete commands, so `data delete` is unaffected. Add a table-guard test. Co-Authored-By: Claude Opus 4.8 --- internal/cli/delete.go | 24 ++++++++++-------------- internal/cli/upgrade.go | 27 ++++++++++++++++----------- internal/cli/upgrade_test.go | 6 ++++++ 3 files changed, 32 insertions(+), 25 deletions(-) diff --git a/internal/cli/delete.go b/internal/cli/delete.go index 9efcf9d..e601f4a 100644 --- a/internal/cli/delete.go +++ b/internal/cli/delete.go @@ -44,20 +44,9 @@ type deleteOpts struct { namespace string } -// deleteCmdName is the top-level offboard command. main skips the post-command -// update nudge after it (see isDeleteCommand / SkipUpdateNudge): the CLI has just -// removed itself and, on the default path, wiped ~/.tracebloc. +// deleteCmdName is the top-level offboard command's name. const deleteCmdName = "delete" -// isDeleteCommand reports whether the executed command is `tracebloc delete`, so -// main can skip the post-command update nudge. Offboarding removes the running -// CLI and (unless --keep-data) wipes ~/.tracebloc, so nudging to upgrade a -// just-removed binary is nonsensical — and the nudge's cache write would recreate -// the just-offboarded host data dir (Bugbot #397). -func isDeleteCommand(cmd *cobra.Command) bool { - return cmd != nil && cmd.Name() == deleteCmdName -} - // newDeleteCmd wires the TOP-LEVEL `tracebloc delete` — offboarding this machine // (RFC-0001 §7.10). It is deliberately NOT under `client` and NOT // `client delete --uninstall`: on the single-machine CLI the host owns exactly @@ -78,8 +67,15 @@ func isDeleteCommand(cmd *cobra.Command) bool { func newDeleteCmd() *cobra.Command { var o deleteOpts cmd := &cobra.Command{ - Use: deleteCmdName, - Short: "Offboard this machine from tracebloc (revoke, uninstall, reclaim disk)", + Use: deleteCmdName, + // Suppress the post-command update nudge/cache-write: offboarding removes + // the CLI and (on the default path) wipes ~/.tracebloc, so nudging to + // upgrade a just-removed binary is nonsensical and the cache write would + // resurrect the just-wiped data dir. The annotation (not a name match) means + // `data delete` is unaffected — only this top-level command opts in + // (Bugbot #397, #404). See SkipUpdateNudge. + Annotations: map[string]string{skipUpdateNudgeAnnotation: "true"}, + Short: "Offboard this machine from tracebloc (revoke, uninstall, reclaim disk)", Long: `Removes tracebloc from this machine: revokes the machine credential, uninstalls the Helm release, deletes the local cluster, reclaims the tracebloc container images, and clears local state — then removes the CLI itself. diff --git a/internal/cli/upgrade.go b/internal/cli/upgrade.go index a2522bb..9c0d8b6 100644 --- a/internal/cli/upgrade.go +++ b/internal/cli/upgrade.go @@ -60,17 +60,19 @@ func upgradePlanFor(goos string) upgradePlan { } } -// isUpgradeCommand reports whether the executed command is `tracebloc upgrade`, -// so main can skip the post-command update nudge (the still-running process -// carries its old compile-time version after swapping its own binary, and would -// otherwise nudge about the very release it just installed). -func isUpgradeCommand(cmd *cobra.Command) bool { - return cmd != nil && cmd.Name() == upgradeCmdName -} +// skipUpdateNudgeAnnotation marks a command after which main must NOT run the +// post-command update nudge (nor the cache write it triggers). Commands +// self-declare it in their cobra Annotations (see newUpgradeCmd, newDeleteCmd), +// so SkipUpdateNudge needs no central name list and — critically — can't be +// fooled by a same-named subcommand: the top-level offboard `delete` carries the +// annotation while `data delete` does not, even though both leaves are named +// "delete" (Bugbot #404). +const skipUpdateNudgeAnnotation = "tracebloc.io/skip-update-nudge" // SkipUpdateNudge reports whether the post-command update nudge (and the cache // write it triggers) must be suppressed for the command that just ran. Exported -// for main, which owns the nudge call. Suppressed after: +// for main, which owns the nudge call. A command opts in via +// skipUpdateNudgeAnnotation; today that's: // - upgrade: the running process swapped its own binary and now carries the // stale compile-time version — it would nudge about the release it just // installed. @@ -78,15 +80,18 @@ func isUpgradeCommand(cmd *cobra.Command) bool { // ~/.tracebloc, so the nudge is nonsensical and its cache write would // resurrect the just-offboarded host data dir (Bugbot #397). func SkipUpdateNudge(cmd *cobra.Command) bool { - return isUpgradeCommand(cmd) || isDeleteCommand(cmd) + return cmd != nil && cmd.Annotations[skipUpdateNudgeAnnotation] == "true" } // newUpgradeCmd implements `tracebloc upgrade` — the apply step the update // nudge (update_check.go) and the 426 "too old" error both point at. func newUpgradeCmd() *cobra.Command { return &cobra.Command{ - Use: upgradeCmdName, - Short: "Update tracebloc to the latest release", + Use: upgradeCmdName, + // Suppress the post-command update nudge/cache-write: this process just + // swapped its own binary and is stale by design (see SkipUpdateNudge). + Annotations: map[string]string{skipUpdateNudgeAnnotation: "true"}, + Short: "Update tracebloc to the latest release", Long: `Updates tracebloc to the latest release by re-running the official installer. It verifies signatures (cosign) and replaces the CLI; on Linux/macOS it also upgrades your secure environment's services to match, so the CLI and the diff --git a/internal/cli/upgrade_test.go b/internal/cli/upgrade_test.go index 4c66426..5984389 100644 --- a/internal/cli/upgrade_test.go +++ b/internal/cli/upgrade_test.go @@ -102,6 +102,12 @@ func TestSkipUpdateNudge(t *testing.T) { if !SkipUpdateNudge(newDeleteCmd()) { t.Error("delete command must skip the update nudge (its cache write would resurrect the wiped ~/.tracebloc)") } + // `data delete` shares the leaf name "delete" but does NOT offboard/wipe — it + // must still get the nudge. Guards the name-collision fix (Bugbot #404): the + // skip is driven by an annotation, not by cmd.Name(). + if SkipUpdateNudge(newDataDeleteCmd()) { + t.Error("`data delete` must NOT skip the nudge — it doesn't wipe ~/.tracebloc or remove the CLI") + } if SkipUpdateNudge(newDoctorCmd(false)) { t.Error("an ordinary command (doctor) must not skip the nudge") }