diff --git a/.github/workflows/add-to-kanban.yml b/.github/workflows/add-to-kanban.yml index 603751a..07283e3 100644 --- a/.github/workflows/add-to-kanban.yml +++ b/.github/workflows/add-to-kanban.yml @@ -10,7 +10,24 @@ jobs: add-to-project: runs-on: ubuntu-latest steps: + # Board writes authenticate as the tracebloc-release-train App (backend#2036), + # not a human's PAT. `owner:` yields an ORG-scoped installation token; a + # repo-scoped one cannot write the org project. No fallback to the PAT: a + # fallback would let a broken App path keep working silently. + # + # This workflow also fires on DEPENDABOT PRs, which GitHub gates on a separate + # secret scope -- both app secrets are set there too, or Dependabot PRs would + # stop reaching the board with `Input required and not supplied`. + - name: Mint an installation token + id: app-token + uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3.2.0 + with: + app-id: ${{ secrets.RELEASE_TRAIN_APP_ID }} + private-key: ${{ secrets.RELEASE_TRAIN_APP_PRIVATE_KEY }} + owner: ${{ github.repository_owner }} + - uses: actions/add-to-project@5afcf98fcd03f1c2f92c3c83f58ae24323cc57fd # v2.0.0 with: project-url: https://github.com/orgs/tracebloc/projects/2 - github-token: ${{ secrets.PROJECTS_KANBAN_TOKEN }} + github-token: ${{ steps.app-token.outputs.token }} + diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 1b3381f..ed2b875 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -55,8 +55,15 @@ jobs: shellcheck --shell=bash --severity=error scripts/check-tool-pins.sh dash -n scripts/install.sh bash -n scripts/tests/install-verify.sh + shellcheck --shell=bash --severity=error scripts/tests/install-ps1-verify.sh + bash -n scripts/tests/install-ps1-verify.sh - name: Verification harness (mandatory cosign / fail-closed) run: bash scripts/tests/install-verify.sh + # Same property on Windows (backend#2078). pwsh is preinstalled on the + # ubuntu runner image; the harness FAILS rather than skips if it isn't, + # since "cannot tell" is not evidence that verification is mandatory. + - name: Verification harness — Windows (mandatory cosign / fail-closed) + run: bash scripts/tests/install-ps1-verify.sh test: timeout-minutes: 15 diff --git a/cmd/tracebloc/main.go b/cmd/tracebloc/main.go index 29ef9f0..7300d82 100644 --- a/cmd/tracebloc/main.go +++ b/cmd/tracebloc/main.go @@ -23,6 +23,7 @@ import ( "os" "os/signal" "syscall" + "time" "github.com/tracebloc/cli/internal/cli" ) @@ -59,11 +60,23 @@ func main() { syscall.SIGINT, syscall.SIGTERM) defer stop() - executed, err := cli.NewRootCmd(cli.BuildInfo{ + info := cli.BuildInfo{ Version: version, GitSHA: gitSHA, BuildDate: buildDate, - }).ExecuteContextC(ctx) + } + root := cli.NewRootCmd(info) + + started := time.Now() + executed, err := root.ExecuteContextC(ctx) + + // backend#1907: one command-outcome event per invocation, emitted from the + // single point every command path converges on — command name, duration, + // exit code, OS/arch, version, error class. No arguments, no paths (see + // internal/cli/telemetry.go for why that is structural rather than a rule). + // Opt-out via TRACEBLOC_NO_TELEMETRY / DO_NOT_TRACK; best-effort and silent, + // so nothing here can change what the customer sees or what we exit with. + cli.RecordCommandOutcome(root, executed, info, cli.ExitCodeFromError(err), time.Since(started)) // F1: after the command runs, a quiet once-a-day nudge if a newer release // exists (best-effort; silent on dev builds, off a terminal, in CI, or with diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 9943e9e..5f852c9 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -131,6 +131,34 @@ produces that code. | `9` | The ingestion Job exited non-zero, completed with row-level failures the summary panel reports, or its outcome couldn't be determined / followed | `data ingest` | `exitIngestFailed` | | `130` | You hit Ctrl-C while something was already running — the sign-in wait, `client status --wait`, the seal check, or an installer re-run (128+SIGINT). Ctrl-C at a *question* is `0` instead: nothing had started | `login`, `client status --wait`, `client status --seal`, `upgrade`, `prepare-host` | `exitInterrupted` | +## Usage reporting + +The CLI records one outcome event per command so we can see failures like the +ones on this page without waiting for someone to report them. It is on by +default and it is a fixed, closed set of fields — there is no free-text field +in the record at all: + +| Field | Example | Where it comes from | +|---|---|---| +| command | `data ingest` | the command you ran, looked up in the CLI's own command list. A value that isn't one of those commands is reported as `unregistered` | +| exit code | `4` | the table above | +| error class | `no_secure_environment` | derived from that exit code, nothing else | +| duration | `1520` ms | wall clock | +| OS / architecture | `darwin` / `arm64` | compiled into the binary | +| version | `0.10.9` | the release you're running | + +**What is never sent:** your arguments, any file or directory path, any dataset +or file contents, your username, your hostname, your kubeconfig, your tokens. +Not "filtered out" — the record has nowhere to put them. Each run gets a fresh +random id, so runs are not linked to each other or to you. + +Turn it off with either of: + +```bash +export TRACEBLOC_NO_TELEMETRY=1 +export DO_NOT_TRACK=1 +``` + ## Still stuck? Open an issue at [github.com/tracebloc/cli/issues](https://github.com/tracebloc/cli/issues) diff --git a/internal/cli/telemetry.go b/internal/cli/telemetry.go new file mode 100644 index 0000000..df2ff2e --- /dev/null +++ b/internal/cli/telemetry.go @@ -0,0 +1,193 @@ +package cli + +// Command-outcome telemetry wiring — backend#1907. +// +// One event per invocation, emitted from the single place every command path +// converges on (main.go, after ExecuteContextC returns). Hooking each handler +// instead would mean N call sites that each have to remember, and §6.5's +// "terminal event on every path" would then be true only for the handlers +// somebody remembered. +// +// WHERE THIS STOPS TODAY. The transport is a seam. RFC-BACKEND-1872's Collector +// gateway was replaced on 17 Aug by an ingest endpoint on the backend +// (rfcs#28), which is backend#1905 and does not exist yet — so pendingSink +// returns nil and every event is validated and dropped. That is deliberate: +// validation runs on every build regardless, so a malformed event fails in CI +// wherever the binary was built, and connecting #1905 is one function. + +import ( + "crypto/rand" + "encoding/hex" + "os" + "strings" + "time" + + "github.com/spf13/cobra" + + "github.com/tracebloc/cli/internal/api" + "github.com/tracebloc/cli/internal/config" + "github.com/tracebloc/cli/internal/telemetry" +) + +// telemetryOptOutVars disable emission when set. Opt-OUT, per the ticket: +// telemetry that only the already-convinced enable measures the wrong +// population, and the population this exists for is people whose install just +// failed. DO_NOT_TRACK is the cross-vendor spelling; supporting it means a user +// who has already expressed the preference once does not have to learn ours. +var telemetryOptOutVars = []string{"TRACEBLOC_NO_TELEMETRY", "DO_NOT_TRACK"} + +// telemetryEnabled reports whether this invocation may emit. +// +// Anything other than the explicit "off" spellings counts as opting out. The +// asymmetry is on purpose: a user who typed TRACEBLOC_NO_TELEMETRY=please +// meant it, and guessing wrong in the other direction sends a record they +// declined. +func telemetryEnabled(getenv func(string) string) bool { + for _, name := range telemetryOptOutVars { + switch strings.ToLower(strings.TrimSpace(getenv(name))) { + case "", "0", "false": + continue + default: + return false + } + } + return true +} + +// commandPaths enumerates every path the tree can dispatch, DERIVED from the +// live tree rather than listed here. That is what makes the closed set in +// telemetry.NewOutcomeRecorder maintain itself: a command added to NewRootCmd is +// reportable the day it lands, and a value that is not a command in the tree can +// never be emitted — including one assembled out of user input. +func commandPaths(root *cobra.Command) []string { + var out []string + var walk func(c *cobra.Command) + walk = func(c *cobra.Command) { + out = append(out, commandPathOf(c)) + for _, sub := range c.Commands() { + walk(sub) + } + } + walk(root) + return out +} + +// commandPathOf renders one command as the contract's tracebloc.cli.command +// value: the invocation minus the binary name, "data ingest" (§7.1). The bare +// root reports its own name rather than an empty string, which normalise would +// drop as absent — leaving the one invocation shape a first-time user is most +// likely to produce as the only one with no command on the record. +func commandPathOf(c *cobra.Command) string { + if c == nil { + return "" + } + path := strings.TrimSpace(c.CommandPath()) + root := c.Root().Name() + if path == root || path == "" { + return root + } + return strings.TrimSpace(strings.TrimPrefix(path, root)) +} + +// telemetryEnv picks deployment.environment for the records. +// +// It labels each record with the backend the client is ACTUALLY talking to, +// resolved exactly the way api.BaseURL resolves it — because that is the host +// these records are about. The mapping mirrors BaseURL: a known env is itself; a +// present-but-unrecognised value is prod, because api.BaseURL routes every +// unknown value to https://api.tracebloc.io (sessionEnv hands cfg.CurrentEnv to +// api.New verbatim). So prod is the accurate label for that population, not a +// guess — and NOT withheld: a misconfigured install that hits prod and fails is +// exactly the run this feature exists to see. +// +// $CLIENT_ENV is consulted only when there is no signed-in env, matching +// sessionEnv: once cfg.CurrentEnv is set the client ignores $CLIENT_ENV, so +// resolving a signed-in unknown through $CLIENT_ENV would label the record for a +// backend the client never contacts (the bug this replaces). +// +// NOTE: that api.BaseURL silently routes an unknown env to prod — so an install +// believing it is on another backend sends its token there — is a real defect, +// but in client.go, not here; tracked separately. This function must match that +// behaviour until it changes, not diverge from it. +func telemetryEnv(env string) string { + resolved := env + if resolved == "" { + // Not signed in: $CLIENT_ENV, then the prod default (as sessionEnv does). + resolved = api.ResolveEnv("") + } + if api.IsKnownEnv(resolved) { + return strings.ToLower(resolved) + } + // Unrecognised: api.BaseURL sends it to prod, so prod is where these records + // belong. + return api.EnvProd +} + +// signedInEnv reads the environment the config points at, best-effort. A +// missing or unreadable config is simply "not signed in". +func signedInEnv() string { + cfg, err := config.Load() + if err != nil || cfg == nil { + return "" + } + return cfg.CurrentEnv +} + +// processInstanceID is the per-PROCESS id §2 asks for off-cluster. +// +// Not the hostname, and not a persisted machine id. Hostnames in this product's +// field data are overwhelmingly "-macbook", which §7.3 forbids +// outright; a persisted id would be a durable identifier we would then have to +// answer erasure requests about. A fresh random value per run still separates +// concurrent runs, which is all service.instance.id is for here. +func processInstanceID() string { + b := make([]byte, 8) + if _, err := rand.Read(b); err != nil { + // Omitted rather than faked. New() drops an empty instance id, and a + // constant stand-in would silently fuse every affected run into one. + return "" + } + return hex.EncodeToString(b) +} + +// pendingSink is the transport seam for backend#1905. +// +// nil means validate-and-drop (telemetry.SetSink's documented contract). When +// the ingest endpoint lands this returns the client that posts to it, and +// nothing else in this file changes. +func pendingSink() telemetry.Sink { return nil } + +// RecordCommandOutcome emits the single terminal event for this invocation. +// main.go calls it once, after the command tree has returned and before exit. +// +// It never returns an error and never panics: a CLI that died because telemetry +// was unhappy would be a strictly worse CLI. A malformed event is caught by the +// tests below, where it is free. +func RecordCommandOutcome(root, executed *cobra.Command, info BuildInfo, exitCode int, elapsed time.Duration) { + _ = recordCommandOutcome(root, executed, info, exitCode, elapsed, os.Getenv, pendingSink()) +} + +// recordCommandOutcome is RecordCommandOutcome with its two ambient +// dependencies passed in, so the tests drive the real thing. +func recordCommandOutcome( + root, executed *cobra.Command, + info BuildInfo, + exitCode int, + elapsed time.Duration, + getenv func(string) string, + sink telemetry.Sink, +) error { + if !telemetryEnabled(getenv) { + return nil + } + emitter := telemetry.New(telemetryEnv(signedInEnv()), info.Version, processInstanceID()) + if sink != nil { + emitter.SetSink(sink) + } + recorder := telemetry.NewOutcomeRecorder(emitter, commandPaths(root)) + return recorder.Record(telemetry.Outcome{ + Command: commandPathOf(executed), + ExitCode: exitCode, + Elapsed: elapsed, + }) +} diff --git a/internal/cli/telemetry_test.go b/internal/cli/telemetry_test.go new file mode 100644 index 0000000..61de040 --- /dev/null +++ b/internal/cli/telemetry_test.go @@ -0,0 +1,448 @@ +package cli + +import ( + "encoding/json" + "fmt" + "os" + "path/filepath" + "regexp" + "strings" + "testing" + "time" + + "github.com/spf13/cobra" + "github.com/spf13/pflag" + + "github.com/tracebloc/cli/internal/api" + "github.com/tracebloc/cli/internal/telemetry" +) + +// telemetryCanary is a value that could only have come off a user's command +// line: a path segment with a patient identifier in it. Written down here, and +// never derived from anything the code under test produces — a needle iterated +// out of the haystack finds itself and nothing else. +const telemetryCanary = "CANARY-PATIENT-7" + +func testBuildInfo() BuildInfo { + return BuildInfo{Version: "0.10.9", GitSHA: "abc1234", BuildDate: "2026-08-18"} +} + +// isolateConfig points config.Load at an empty directory so these tests never +// read (or report on) the developer's real signed-in environment. +func isolateConfig(t *testing.T) { + t.Helper() + t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) + t.Setenv("CLIENT_ENV", api.EnvProd) +} + +// captureOutcome runs the real recorder over the real tree and returns what +// reached the sink. +func captureOutcome( + t *testing.T, root, executed *cobra.Command, exitCode int, env map[string]string, +) (map[string]string, map[string]any, bool) { + t.Helper() + var res map[string]string + var rec map[string]any + delivered := 0 + sink := telemetry.Sink(func(r map[string]string, d map[string]any) { + res, rec = r, d + delivered++ + }) + getenv := func(k string) string { return env[k] } + if err := recordCommandOutcome( + root, executed, testBuildInfo(), exitCode, 1500*time.Millisecond, getenv, sink, + ); err != nil { + t.Fatalf("recordCommandOutcome: %v", err) + } + if delivered > 1 { + t.Fatalf("one invocation delivered %d events; the contract is exactly one", delivered) + } + return res, rec, delivered == 1 +} + +// walkTree yields every command in the tree, so the assertions below are +// derived from what actually dispatches rather than from a list somebody has to +// remember to extend. +func walkTree(root *cobra.Command) []*cobra.Command { + var out []*cobra.Command + var walk func(c *cobra.Command) + walk = func(c *cobra.Command) { + out = append(out, c) + for _, sub := range c.Commands() { + walk(sub) + } + } + walk(root) + return out +} + +// --- the closed set is the live tree ------------------------------------------ + +func TestEveryCommandInTheLiveTreeReportsItsOwnPath(t *testing.T) { + // The point of deriving the set from the tree: a command added to + // NewRootCmd is reportable the day it lands. If this ever fails for a new + // command, the enumeration and the dispatcher have drifted — which would + // mean that command's failures were being filed under "unregistered". + isolateConfig(t) + root := NewRootCmd(testBuildInfo()) + + commands := walkTree(root) + if len(commands) < 10 { + t.Fatalf("walked only %d commands — the tree was not built", len(commands)) + } + for _, cmd := range commands { + t.Run(cmd.CommandPath(), func(t *testing.T) { + _, rec, ok := captureOutcome(t, root, cmd, 0, nil) + if !ok { + t.Fatal("nothing was delivered") + } + want := commandPathOf(cmd) + if rec[telemetry.AttrCommand] != want { + t.Fatalf("%s = %v, want %q — this command is not in the enumerated set", + telemetry.AttrCommand, rec[telemetry.AttrCommand], want) + } + if rec[telemetry.AttrCommand] == telemetry.CommandUnregistered { + t.Fatalf("%q dispatches but is not enumerated", cmd.CommandPath()) + } + }) + } +} + +func TestTheBareRootIsNamedNotBlank(t *testing.T) { + // commandPathOf's root case: an empty string is dropped by the emitter's + // omit-when-absent rule, which would leave the invocation a first-time user + // is most likely to produce as the only one with no command on the record. + isolateConfig(t) + root := NewRootCmd(testBuildInfo()) + _, rec, ok := captureOutcome(t, root, root, 0, nil) + if !ok { + t.Fatal("nothing was delivered") + } + if rec[telemetry.AttrCommand] != root.Name() { + t.Fatalf("bare root reported %v, want %q", rec[telemetry.AttrCommand], root.Name()) + } +} + +// --- the privacy boundary, over the real tree --------------------------------- + +// TestNoFlagValueOrArgumentCanReachTheRecord is the ticket's hard boundary, +// checked against what the code emits rather than against a list of keys we +// hope nobody adds. +// +// Every command in the live tree has every one of its flags set to the canary, +// canary positional args attached, and a canary in the environment. Then the +// whole delivered payload — keys and values, resource and record — is searched. +func TestNoFlagValueOrArgumentCanReachTheRecord(t *testing.T) { + isolateConfig(t) + root := NewRootCmd(testBuildInfo()) + + inspected := 0 + for _, cmd := range walkTree(root) { + // Load the command up with everything a user could have typed. + cmd.Flags().VisitAll(func(f *pflag.Flag) { + _ = f.Value.Set(telemetryCanary) + f.Changed = true + }) + cmd.SetArgs([]string{"/Users/" + telemetryCanary + "/oncology.csv", telemetryCanary}) + + env := map[string]string{ + "CLIENT_ENV": api.EnvProd, + "TRACEBLOC_CONFIG_DIR": "/home/" + telemetryCanary + "/.tracebloc", + } + res, rec, ok := captureOutcome(t, root, cmd, 9, env) + if !ok { + t.Fatalf("%s delivered nothing", cmd.CommandPath()) + } + for k, v := range res { + assertNoTelemetryCanary(t, cmd.CommandPath(), "resource", k, v) + inspected++ + } + for k, v := range rec { + assertNoTelemetryCanary(t, cmd.CommandPath(), "record", k, fmt.Sprint(v)) + inspected++ + } + } + // The anchor: an inert loop over an empty payload reads exactly like a clean + // sweep in the log. + if inspected < 100 { + t.Fatalf("only %d attributes were searched — the sweep ran over nothing", inspected) + } +} + +func TestACommandPathCarryingAnArgumentIsRefusedNotCleaned(t *testing.T) { + // The failure mode this guards: some future caller passing os.Args, or a + // cobra change that starts including args in CommandPath(). The lookup makes + // that a countable "unregistered", never a partially-scrubbed string. + isolateConfig(t) + // The impostor's NAME is the canary, so commandPathOf hands the recorder a + // path carrying it. (Note what cobra itself does not do: Name() takes the + // first word of Use, so `Use: "ingest "` yields "ingest" — CommandPath + // structurally cannot contain an argument today. This test is the guard for + // the day that stops being true, or for a caller that builds the path itself.) + impostor := &cobra.Command{Use: telemetryCanary} + tree := NewRootCmd(testBuildInfo()) + tree.AddCommand(impostor) + // Enumerate from a tree that never had it — the set is what main.go builds + // from the dispatcher, and this command was never meant to be in it. + clean := NewRootCmd(testBuildInfo()) + + if got := commandPathOf(impostor); !strings.Contains(got, telemetryCanary) { + t.Fatalf("the fixture is inert: commandPathOf gave %q, which carries no canary "+ + "for the lookup to refuse", got) + } + + res, rec, ok := captureOutcome(t, clean, impostor, 1, nil) + if !ok { + t.Fatal("nothing was delivered") + } + if rec[telemetry.AttrCommand] != telemetry.CommandUnregistered { + t.Fatalf("%s = %v, want %q", telemetry.AttrCommand, + rec[telemetry.AttrCommand], telemetry.CommandUnregistered) + } + // Sweep the WHOLE payload, not just the one key. Checking only + // tracebloc.cli.command leaves the canary free to arrive under any other + // attribute — which is exactly what happened under the "smuggle the raw + // command into a second attribute" mutation: this test stayed green while + // the record carried the path verbatim. + for k, v := range res { + assertNoTelemetryCanary(t, "impostor", "resource", k, v) + } + for k, v := range rec { + assertNoTelemetryCanary(t, "impostor", "record", k, fmt.Sprint(v)) + } +} + +func TestTheWholePayloadIsSerialisableAndSmall(t *testing.T) { + // The record is what a transport will put on the wire. Anything that will + // not round-trip as JSON primitives is the retired extraData defect arriving + // by another door, and an outcome event that needs more than a kilobyte is + // carrying something it should not. + isolateConfig(t) + root := NewRootCmd(testBuildInfo()) + res, rec, ok := captureOutcome(t, root, root, 9, nil) + if !ok { + t.Fatal("nothing was delivered") + } + blob, err := json.Marshal(map[string]any{"resource": res, "attributes": rec}) + if err != nil { + t.Fatalf("the payload does not serialise: %v", err) + } + if len(blob) > 1024 { + t.Fatalf("an outcome event serialised to %d bytes: %s", len(blob), blob) + } +} + +// --- opt-out ------------------------------------------------------------------ + +func TestOptOutStopsEmissionEntirely(t *testing.T) { + isolateConfig(t) + root := NewRootCmd(testBuildInfo()) + + // DERIVED: the variables the production code declares, not a list restated + // here. Adding a spelling to telemetryOptOutVars covers it automatically. + for _, name := range telemetryOptOutVars { + for _, value := range []string{"1", "true", "yes", "please", " 1 "} { + t.Run(name+"="+strings.TrimSpace(value), func(t *testing.T) { + _, _, ok := captureOutcome(t, root, root, 0, map[string]string{name: value}) + if ok { + t.Fatalf("%s=%q still emitted", name, value) + } + }) + } + } +} + +func TestTheOffSpellingsDoNotOptOut(t *testing.T) { + // The mutation anchor for the test above: if telemetryEnabled returned false + // unconditionally, every opt-out case would pass and the feature would be + // dead. These are the values that must NOT disable it. + isolateConfig(t) + root := NewRootCmd(testBuildInfo()) + for _, value := range []string{"", "0", "false", "FALSE"} { + t.Run("value_"+value, func(t *testing.T) { + _, _, ok := captureOutcome(t, root, root, + 0, map[string]string{"TRACEBLOC_NO_TELEMETRY": value}) + if !ok { + t.Fatalf("%q disabled telemetry; only an explicit opt-out should", value) + } + }) + } +} + +// --- environment --------------------------------------------------------------- + +func TestTheEnvironmentLabelMatchesTheBackend(t *testing.T) { + // The label is the backend api.BaseURL actually targets: a known env is + // itself; anything unrecognised is prod, because BaseURL routes it there. + // `staging` is the classic near miss — the git branch name, not the `stg` + // environment value — and it resolves to prod (where a client signed into + // "staging" really goes), NOT to stg. + for _, tc := range []struct { + signedIn string + want string + }{ + {api.EnvDev, api.EnvDev}, + {api.EnvStg, api.EnvStg}, + {"PROD", api.EnvProd}, + {"staging", api.EnvProd}, // unknown -> prod, matching api.BaseURL + {"", api.EnvProd}, // not signed in, CLIENT_ENV empty -> prod + } { + t.Run("signed_in_"+tc.signedIn, func(t *testing.T) { + t.Setenv("CLIENT_ENV", "") + if got := telemetryEnv(tc.signedIn); got != tc.want { + t.Fatalf("telemetryEnv(%q) = %q, want %q", tc.signedIn, got, tc.want) + } + }) + } +} + +func TestASignedInUnknownEnvIgnoresClientEnv(t *testing.T) { + // The bug this pins (Asad, cli#528 review): the client resolves a signed-in + // env via sessionEnv, which returns cfg.CurrentEnv VERBATIM and never consults + // $CLIENT_ENV — so a config on "banana" talks to prod (api.BaseURL default) + // regardless of $CLIENT_ENV. The old code resolved the label through + // ResolveEnv, which DOES read $CLIENT_ENV, so it filed the run under "dev" + // while every request went to prod. The label must be prod, not dev. + t.Setenv("CLIENT_ENV", "dev") + if got := telemetryEnv("banana"); got != api.EnvProd { + t.Fatalf("telemetryEnv(%q) with CLIENT_ENV=dev = %q, want %q — the label "+ + "must match the backend the client actually contacts (prod)", "banana", got, api.EnvProd) + } +} + +func TestAnUnknownClientEnvIsLabelledProd(t *testing.T) { + // The end-to-end consequence: not signed in, CLIENT_ENV=staging. sessionEnv + // resolves that through ResolveEnv -> "staging", and api.BaseURL routes it to + // prod — so the run genuinely hits prod and its record must be filed under + // prod, the population this feature exists for, not withheld. + isolateConfig(t) + t.Setenv("CLIENT_ENV", "staging") + root := NewRootCmd(testBuildInfo()) + res, _, ok := captureOutcome(t, root, root, 0, nil) + if !ok { + t.Fatal("withheld a record for a run that hits prod under an unknown CLIENT_ENV") + } + if res["deployment.environment"] != api.EnvProd { + t.Fatalf("deployment.environment = %q, want %q", res["deployment.environment"], api.EnvProd) + } +} + +func TestTheSignedInEnvironmentWins(t *testing.T) { + // A developer signed into dev must not have their runs filed under prod. + dir := t.TempDir() + t.Setenv("TRACEBLOC_CONFIG_DIR", dir) + t.Setenv("CLIENT_ENV", "") + body := `{"version":2,"current_env":"dev","profiles":{"dev":{"token":"x"}}}` + if err := os.WriteFile(filepath.Join(dir, "config.json"), []byte(body), 0o600); err != nil { + t.Fatal(err) + } + // Read it back before asserting on the record: a skip here would be a + // fail-open, and a config layout this fixture no longer matches must be a + // finding, not a quiet pass. + if got := signedInEnv(); got != api.EnvDev { + t.Fatalf("signedInEnv() = %q, want %q — the on-disk config layout changed "+ + "and this fixture (and possibly the reader) is stale", got, api.EnvDev) + } + root := NewRootCmd(testBuildInfo()) + res, _, ok := captureOutcome(t, root, root, 0, nil) + if !ok { + t.Fatal("nothing was delivered") + } + if res["deployment.environment"] != api.EnvDev { + t.Fatalf("deployment.environment = %q, want %q", + res["deployment.environment"], api.EnvDev) + } +} + +func TestASignedInUnknownEnvironmentIsLabelledProd(t *testing.T) { + // A run signed into an environment the CLI does not recognise talks to prod + // (sessionEnv hands cfg.CurrentEnv to api.New verbatim, api.BaseURL routes the + // unknown value to prod), so its record must be filed under prod — that + // failed-install-on-prod run is exactly what this feature exists to capture. + dir := t.TempDir() + t.Setenv("TRACEBLOC_CONFIG_DIR", dir) + t.Setenv("CLIENT_ENV", "") // so the label comes from the config, not the env + body := `{"version":2,"current_env":"banana","profiles":{"banana":{"token":"x"}}}` + if err := os.WriteFile(filepath.Join(dir, "config.json"), []byte(body), 0o600); err != nil { + t.Fatal(err) + } + // Read it back before asserting: a config layout this fixture no longer + // matches must be a finding, not a quiet pass that exercises the empty path. + if got := signedInEnv(); got != "banana" { + t.Fatalf("signedInEnv() = %q, want %q — the on-disk config layout changed "+ + "and this fixture (and possibly the reader) is stale", got, "banana") + } + root := NewRootCmd(testBuildInfo()) + res, _, ok := captureOutcome(t, root, root, 0, nil) + if !ok { + t.Fatal("withheld a record for a run signed into an unknown env that hits prod") + } + if res["deployment.environment"] != api.EnvProd { + t.Fatalf("deployment.environment = %q, want %q", res["deployment.environment"], api.EnvProd) + } +} + +// --- instance id --------------------------------------------------------------- + +func TestTheInstanceIDIsPerProcessAndNotTheHostname(t *testing.T) { + // §2 asks for a stable per-process uuid off-cluster. Not the hostname: field + // hostnames are overwhelmingly "-macbook", which §7.3 forbids + // outright. Two calls must differ, and neither may look like a host. + host, _ := os.Hostname() + a, b := processInstanceID(), processInstanceID() + if a == b { + t.Fatal("two invocations shared an instance id — that is a durable identifier") + } + if len(a) != 16 { + t.Fatalf("instance id %q is not the expected 16 hex chars", a) + } + if host != "" && strings.Contains(a, host) { + t.Fatalf("the instance id embeds the hostname: %q", a) + } +} + +func assertNoTelemetryCanary(t *testing.T, where, layer, key, value string) { + t.Helper() + if strings.Contains(key, telemetryCanary) { + t.Fatalf("%s: %s key %q carries the canary", where, layer, key) + } + if strings.Contains(value, telemetryCanary) { + t.Fatalf("%s: %s %q = %q carries the canary", where, layer, key, value) + } +} + +// TestTheDocumentedOptOutVariablesAreTheRealOnes closes the gap that makes a +// stale doc worse than no doc: a user who exports the variable +// docs/troubleshooting.md names believes they have opted out. If the name there +// has drifted from telemetryOptOutVars, they have not, and nothing else would +// ever tell them. +// +// DERIVED both ways — it parses the variable names out of the document and +// compares the SET against the production slice, so neither a rename in the +// code nor an edit to the doc can pass on its own. +func TestTheDocumentedOptOutVariablesAreTheRealOnes(t *testing.T) { + body, err := os.ReadFile(filepath.Join("..", "..", "docs", "troubleshooting.md")) + if err != nil { + // Fail closed: an unreadable document is not evidence of agreement. + t.Fatalf("cannot read the document this guard checks: %v", err) + } + found := map[string]bool{} + for _, m := range regexp.MustCompile(`export ([A-Z_]+)=1`).FindAllStringSubmatch(string(body), -1) { + found[m[1]] = true + } + if len(found) == 0 { + t.Fatal("the document names no opt-out variable — either the section was " + + "removed (then remove this guard) or its shape changed and this parse is inert") + } + for _, name := range telemetryOptOutVars { + if !found[name] { + t.Errorf("%s disables telemetry but docs/troubleshooting.md does not tell "+ + "anyone so", name) + } + delete(found, name) + } + for name := range found { + t.Errorf("docs/troubleshooting.md tells users to export %s, which disables "+ + "nothing — telemetryOptOutVars is %v", name, telemetryOptOutVars) + } +} diff --git a/internal/telemetry/outcome.go b/internal/telemetry/outcome.go new file mode 100644 index 0000000..3dedc22 --- /dev/null +++ b/internal/telemetry/outcome.go @@ -0,0 +1,199 @@ +package telemetry + +import "time" + +// Command outcomes — backend#1907, RFC-BACKEND-1872 D12's host-process path. +// +// WHAT THIS IS FOR. The CLI is the least-observed surface in the product and +// the one that runs on the most different machines. Every failure in the +// backend#736 class — the binary landing on a PATH the shell does not read, a +// cluster command reading a kubeconfig context nobody meant, a package manager +// blocked on a lock held by something else — was invisible until a customer +// happened to mention it. One outcome event per invocation is what turns that +// class into a number. +// +// THE PRIVACY BOUNDARY IS STRUCTURAL, NOT EDITORIAL. The ticket's rule is "no +// arguments, no paths, no data", and a rule phrased that way is a convention +// that asks. What is built here instead is a record with no free-text channel +// at all: +// +// - the command is a LOOKUP into the set of paths enumerated from the live +// cobra tree at startup; anything else reports CommandUnregistered; +// - the error class is a lookup keyed on an INT — the CLI's own frozen +// exit-code contract — so the classifier cannot see an error message, a +// path or an argument, because it is never given one; +// - everything else is an int. +// +// A sanitiser would have to anticipate what it strips. A closed set only ever +// admits what was enumerated. That difference is why there is no redaction +// regex anywhere in this file. + +// The three terminal event names. Compile-time constants per contract §6.2 — +// no segment is ever computed. There is deliberately no cli.command.started: +// §6.5 requires a terminal event on every path where a `started` is emitted, +// and a process that is killed outright cannot honour that. +const ( + EventCommandSucceeded = "cli.command.succeeded" + EventCommandFailed = "cli.command.failed" + EventCommandCancelled = "cli.command.cancelled" +) + +// Record-scope attribute keys. tracebloc.cli.command is the contract's own name +// for this field (§7.1: "CLI command path — e.g. `data ingest`, never the +// arguments"). +const ( + AttrCommand = "tracebloc.cli.command" + AttrExitCode = "tracebloc.cli.exit_code" + AttrDurationMS = "tracebloc.cli.duration_ms" +) + +// CommandUnregistered is what a path outside the registered set reports. +// +// It is the fail-closed answer, and it is reported as a VALUE rather than by +// dropping the attribute so that "we saw a command we could not name" stays +// countable. A recorder built with no registered commands at all therefore +// reports this for everything — forgetting to register cannot silently turn the +// lookup into a pass-through. +const CommandUnregistered = "unregistered" + +// ExitCancelled is 128+SIGINT — the user pressed Ctrl-C. It is not a failure and +// must not inflate one: a cancel that counted as a failure would move the rate +// D9's alerts are written against every time someone changed their mind. +const ExitCancelled = 130 + +// The closed error.type vocabulary for the `cli` domain (contract §8.4; the +// spec's open question 1 says each emitter ticket proposes its own). +// +// DERIVED FROM THE EXIT CODES, NOT FROM THE ERROR TEXT. internal/cli/exitcodes.go +// already carries a reviewed, documented, FROZEN classification of every way the +// CLI can fail — it is the scripting contract customers branch on. Classifying +// from anywhere else would mean inventing a second taxonomy that drifts from the +// first, and would mean handing the classifier an error string, which is exactly +// the channel a path or a cell value travels down. +const ( + ClassUnspecifiedFailure = "unspecified_failure" + ClassInvalidInput = "invalid_input" + ClassLocalEnvironment = "local_environment" + ClassNoSecureEnvironment = "no_secure_environment" + ClassAuth = "auth" + ClassConflict = "conflict" + ClassClusterOperation = "cluster_operation" + ClassSubmitRejected = "submit_rejected" + ClassIngestFailed = "ingest_failed" + ClassUnclassified = "unclassified" +) + +// exitClasses maps the CLI's exit codes to that vocabulary. Several codes carry +// more than one per-command meaning (exitChecksFailed shares 2 with +// exitBadInput, exitNoSuchDataset shares 5 with exitAuth, and 7 is three +// meanings) — the class names the shared bucket, because the code is what a +// customer's script sees and grouping finer than the contract would be a +// distinction nothing downstream can act on. tracebloc.cli.command separates +// them when it matters. +var exitClasses = map[int]string{ + 1: ClassUnspecifiedFailure, + 2: ClassInvalidInput, + 3: ClassLocalEnvironment, + 4: ClassNoSecureEnvironment, + 5: ClassAuth, + 6: ClassConflict, + 7: ClassClusterOperation, + 8: ClassSubmitRejected, + 9: ClassIngestFailed, +} + +// ClassifyExit maps an exit code to a member of the closed vocabulary. +// +// Total by construction: every int has an answer, and an unmapped one is +// ClassUnclassified rather than the code rendered as a string. That is the +// fail-closed half — a code this table has not seen is a finding you can alert +// on, not a new namespace that appears on its own. +func ClassifyExit(code int) string { + if class, ok := exitClasses[code]; ok { + return class + } + return ClassUnclassified +} + +// Sink is the delivery seam. It is a named type so the wiring in internal/cli +// can say what it is handing over; Emitter.SetSink takes the same shape. +type Sink func(resource map[string]string, record map[string]any) + +// Outcome is one invocation, as measured by the caller. +type Outcome struct { + // Command is the cobra command path minus the arguments — "data ingest". + // It is not trusted: Record looks it up in the registered set and reports + // CommandUnregistered if it is not there. + Command string + // ExitCode is what the process is about to exit with. + ExitCode int + // Elapsed is wall-clock time for the invocation. + Elapsed time.Duration +} + +// OutcomeRecorder turns an Outcome into exactly one contract-conformant event. +type OutcomeRecorder struct { + emitter *Emitter + commands map[string]bool +} + +// NewOutcomeRecorder closes the tracebloc.cli.command value set over commands. +// +// The caller passes the paths enumerated from the live command tree, so the set +// is derived from the thing that actually dispatches rather than restated here. +// A command added to the tree is covered without touching this file; a value +// that is not a command in the tree cannot be emitted at all. +func NewOutcomeRecorder(e *Emitter, commands []string) *OutcomeRecorder { + set := make(map[string]bool, len(commands)) + for _, c := range commands { + if c != "" { + set[c] = true + } + } + return &OutcomeRecorder{emitter: e, commands: set} +} + +// Record emits the invocation's single terminal event. +func (r *OutcomeRecorder) Record(o Outcome) error { + name := EventCommandSucceeded + switch { + case o.ExitCode == 0: + case o.ExitCode == ExitCancelled: + name = EventCommandCancelled + default: + name = EventCommandFailed + } + + attrs := Attrs{ + AttrCommand: r.commandValue(o.Command), + AttrExitCode: o.ExitCode, + AttrDurationMS: durationMS(o.Elapsed), + } + // §8.4 — only the failure outcomes oblige the error set. Emit enforces this + // too; setting it here for a cancel would be a second, disagreeing opinion + // about what counts as a failure. + if name == EventCommandFailed { + attrs["error.type"] = ClassifyExit(o.ExitCode) + } + return r.emitter.Emit(name, attrs) +} + +// commandValue is the privacy boundary: a set membership test, not a cleanup +// pass. Whatever the caller hands over either IS one of the paths the tree can +// dispatch, or it does not reach the record in any form. +func (r *OutcomeRecorder) commandValue(path string) string { + if r.commands[path] { + return path + } + return CommandUnregistered +} + +// durationMS clamps below zero. A wall-clock delta can go negative across a +// clock step, and a negative duration is not a measurement — it is a number +// that would quietly skew every percentile computed over the column. +func durationMS(d time.Duration) int64 { + if ms := Duration(d); ms > 0 { + return ms + } + return 0 +} diff --git a/internal/telemetry/outcome_test.go b/internal/telemetry/outcome_test.go new file mode 100644 index 0000000..fe18e8c --- /dev/null +++ b/internal/telemetry/outcome_test.go @@ -0,0 +1,365 @@ +package telemetry + +import ( + "fmt" + "regexp" + "runtime" + "strings" + "testing" + "time" + + "github.com/tracebloc/cli/internal/api" +) + +// registered is the stand-in for what internal/cli enumerates from the live +// cobra tree. It is deliberately NOT the real tree here: this package must not +// know about cobra, and the tree-derived version of the same assertion lives in +// internal/cli/telemetry_test.go. +var registered = []string{"tracebloc", "data ingest", "cluster info", "login"} + +func recorderWithSink(t *testing.T) (*OutcomeRecorder, func() (map[string]string, map[string]any)) { + t.Helper() + e := New(api.EnvProd, "0.10.9", "abcdef0123456789") + var res map[string]string + var rec map[string]any + e.SetSink(func(r map[string]string, d map[string]any) { res, rec = r, d }) + return NewOutcomeRecorder(e, registered), func() (map[string]string, map[string]any) { return res, rec } +} + +// --- the event name is the outcome ------------------------------------------ + +func TestTheEventNameFollowsTheExitCode(t *testing.T) { + for _, tc := range []struct { + name string + code int + want string + }{ + {"success", 0, EventCommandSucceeded}, + {"generic failure", 1, EventCommandFailed}, + {"bad input", 2, EventCommandFailed}, + {"ingest failed", 9, EventCommandFailed}, + {"interrupted", ExitCancelled, EventCommandCancelled}, + {"a code no table knows", 77, EventCommandFailed}, + } { + t.Run(tc.name, func(t *testing.T) { + r, read := recorderWithSink(t) + if err := r.Record(Outcome{Command: "data ingest", ExitCode: tc.code}); err != nil { + t.Fatalf("Record: %v", err) + } + _, rec := read() + if rec["event.name"] != tc.want { + t.Fatalf("exit %d emitted %q, want %q", tc.code, rec["event.name"], tc.want) + } + }) + } +} + +func TestACancelIsNotAFailure(t *testing.T) { + // 130 is the user pressing Ctrl-C. Counting it as a failure moves the rate + // D9's alerts are written against every time somebody changes their mind, + // so the record must carry no error.type at all — not even an "ok" one. + r, read := recorderWithSink(t) + if err := r.Record(Outcome{Command: "login", ExitCode: ExitCancelled}); err != nil { + t.Fatalf("Record: %v", err) + } + _, rec := read() + if _, ok := rec["error.type"]; ok { + t.Fatalf("a cancel carried error.type=%v", rec["error.type"]) + } + if rec[AttrExitCode] != ExitCancelled { + t.Fatalf("the exit code was lost: %v", rec[AttrExitCode]) + } +} + +func TestEveryFailureCarriesAClassFromTheClosedVocabulary(t *testing.T) { + // DERIVED input domain: every key the production table declares, plus codes + // outside it. Mutation coverage cannot see a vocabulary gap (workspace + // CLAUDE.md rule 6), so the domain comes from the producer's own surface. + codes := []int{} + for code := range exitClasses { + codes = append(codes, code) + } + codes = append(codes, 42, 77, 255, -1) + + allowed := map[string]bool{ClassUnclassified: true} + for _, class := range exitClasses { + allowed[class] = true + } + + for _, code := range codes { + t.Run(fmt.Sprintf("exit_%d", code), func(t *testing.T) { + r, read := recorderWithSink(t) + if err := r.Record(Outcome{Command: "data ingest", ExitCode: code}); err != nil { + t.Fatalf("Record: %v", err) + } + _, rec := read() + class, ok := rec["error.type"].(string) + if !ok { + t.Fatalf("exit %d produced no error.type: %v", code, rec) + } + if !allowed[class] { + t.Fatalf("exit %d produced error.type %q, which is outside the "+ + "closed vocabulary %v", code, class, allowed) + } + }) + } +} + +func TestAnUnmappedExitCodeIsUnclassifiedNotStringified(t *testing.T) { + // Fail closed: a code the table has not seen must be a countable "we cannot + // name this", never the number rendered into a new value that appears on its + // own. Asserting WHICH answer, because "some member of the vocabulary" is + // also satisfied by returning ClassUnspecifiedFailure for everything. + if got := ClassifyExit(77); got != ClassUnclassified { + t.Fatalf("ClassifyExit(77) = %q, want %q", got, ClassUnclassified) + } + if got := ClassifyExit(2); got != ClassInvalidInput { + t.Fatalf("ClassifyExit(2) = %q, want %q — the mapped codes must still map", + got, ClassInvalidInput) + } +} + +// --- the privacy boundary ---------------------------------------------------- + +// canary is a value that could only have arrived from a user's command line. It +// is written down here, independently of anything the matcher checks — never +// iterated out of the thing under test (workspace CLAUDE.md rule 9). +const canary = "CANARY-PATIENT-7" + +func TestAnUnregisteredCommandIsReplacedNotSanitised(t *testing.T) { + // The whole ticket, in one assertion: an argument-bearing path is not + // cleaned up, it is refused. The two checks are separate on purpose — the + // "canary absent" half alone would pass if the attribute were dropped + // entirely, and the "value is unregistered" half is the mutation anchor: it + // reddens the moment commandValue stops looking the path up. + for _, path := range []string{ + "data ingest /Users/" + canary + "/oncology.csv", + "data ingest --name " + canary, + "login --token " + canary, + canary, + "", + } { + t.Run(path, func(t *testing.T) { + r, read := recorderWithSink(t) + if err := r.Record(Outcome{Command: path, ExitCode: 0}); err != nil { + t.Fatalf("Record: %v", err) + } + res, rec := read() + if rec[AttrCommand] != CommandUnregistered { + t.Fatalf("%s = %v, want %q", AttrCommand, rec[AttrCommand], CommandUnregistered) + } + assertNoCanary(t, res, rec) + }) + } +} + +func TestARegisteredCommandSurvivesIntact(t *testing.T) { + // The other half: the lookup must not become a blanket refusal, or the + // column is CommandUnregistered forever and nothing above is doing any work. + for _, path := range registered { + t.Run(path, func(t *testing.T) { + r, read := recorderWithSink(t) + if err := r.Record(Outcome{Command: path, ExitCode: 0}); err != nil { + t.Fatalf("Record: %v", err) + } + _, rec := read() + if rec[AttrCommand] != path { + t.Fatalf("%s = %v, want %q", AttrCommand, rec[AttrCommand], path) + } + }) + } +} + +func TestARecorderWithNoRegisteredCommandsFailsClosed(t *testing.T) { + // Forgetting to register must not turn the lookup into a pass-through. + e := New(api.EnvProd, "0.10.9", "abcdef0123456789") + var rec map[string]any + e.SetSink(func(_ map[string]string, d map[string]any) { rec = d }) + r := NewOutcomeRecorder(e, nil) + if err := r.Record(Outcome{Command: "data ingest", ExitCode: 0}); err != nil { + t.Fatalf("Record: %v", err) + } + if rec[AttrCommand] != CommandUnregistered { + t.Fatalf("an empty registry reported %v, want %q", rec[AttrCommand], CommandUnregistered) + } +} + +// TestEveryEmittedStringComesFromAClosedSet is the derived form of "no +// arguments, no paths, no data". +// +// It does not hold a list of forbidden keys — a list like that agrees with +// itself and says nothing about the twentieth attribute somebody adds. It walks +// what the code ACTUALLY emits and requires every value to be an int, or a +// member of a set assembled from the producer's own declarations. A free-text +// channel of any kind fails it, whether or not anyone thought to forbid the +// thing travelling down it. +func TestEveryEmittedStringComesFromAClosedSet(t *testing.T) { + allowed := map[string]bool{ + EventCommandSucceeded: true, + EventCommandFailed: true, + EventCommandCancelled: true, + CommandUnregistered: true, + ClassUnclassified: true, + } + for _, c := range registered { + allowed[c] = true + } + for _, class := range exitClasses { + allowed[class] = true + } + + // Resource values are process identity, not occurrence data: fixed strings, + // or shapes with no room for a payload. + shaped := map[string]*regexp.Regexp{ + "service.instance.id": regexp.MustCompile(`^[0-9a-f]{16}$`), + "service.version": regexp.MustCompile(`^[0-9A-Za-z.\-+]{1,32}$`), + } + fixed := map[string]string{ + "service.name": Service, + "tracebloc.component": Component, + "os.type": runtime.GOOS, + "host.arch": runtime.GOARCH, + "deployment.environment": api.EnvProd, + } + + paths := append([]string{}, registered...) + paths = append(paths, "data ingest /var/"+canary+"/rows.csv", canary, "") + codes := []int{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 42, ExitCancelled} + + seen := 0 + for _, path := range paths { + for _, code := range codes { + r, read := recorderWithSink(t) + if err := r.Record(Outcome{ + Command: path, ExitCode: code, Elapsed: 1234 * time.Millisecond, + }); err != nil { + t.Fatalf("Record(%q, %d): %v", path, code, err) + } + res, rec := read() + + for k, v := range rec { + seen++ + switch value := v.(type) { + case int, int64: + case string: + if !allowed[value] { + t.Fatalf("record key %q carried %q, which is not in any "+ + "declared vocabulary — that is a free-text channel", k, value) + } + default: + t.Fatalf("record key %q carried %T; the record must be ints and "+ + "closed-set strings only", k, v) + } + } + for k, v := range res { + seen++ + if want, ok := fixed[k]; ok { + if v != want { + t.Fatalf("resource %q = %q, want %q", k, v, want) + } + continue + } + re, ok := shaped[k] + if !ok { + t.Fatalf("resource carries %q, which this guard has never been "+ + "taught to constrain — classify it before shipping it", k) + } + if !re.MatchString(v) { + t.Fatalf("resource %q = %q, outside %s", k, v, re) + } + } + } + } + // An inert run and full coverage look identical in a log. This is the anchor + // that says the loop above actually inspected something. + if want := len(paths) * len(codes) * 10; seen < want { + t.Fatalf("only %d attributes were inspected (expected at least %d) — the "+ + "guard ran over an empty record", seen, want) + } +} + +// --- measurements ------------------------------------------------------------- + +func TestDurationIsMillisecondsAndNeverNegative(t *testing.T) { + r, read := recorderWithSink(t) + if err := r.Record(Outcome{ + Command: "data ingest", ExitCode: 0, Elapsed: 2500 * time.Millisecond, + }); err != nil { + t.Fatalf("Record: %v", err) + } + _, rec := read() + if rec[AttrDurationMS] != int64(2500) { + t.Fatalf("%s = %v, want 2500", AttrDurationMS, rec[AttrDurationMS]) + } + + // A clock step can hand us a negative delta. A negative duration is not a + // measurement — it skews every percentile computed over the column. + r2, read2 := recorderWithSink(t) + if err := r2.Record(Outcome{ + Command: "data ingest", ExitCode: 0, Elapsed: -5 * time.Second, + }); err != nil { + t.Fatalf("Record: %v", err) + } + _, rec2 := read2() + if rec2[AttrDurationMS] != int64(0) { + t.Fatalf("a negative elapsed became %v, want 0", rec2[AttrDurationMS]) + } +} + +func TestAZeroDurationIsStillReported(t *testing.T) { + // A sub-millisecond command rounds to 0, and 0 is a measurement. §1.2's + // omit-when-absent rule must not swallow it — a command that always returns + // instantly would otherwise have no duration column at all. + r, read := recorderWithSink(t) + if err := r.Record(Outcome{Command: "tracebloc", ExitCode: 0, Elapsed: 0}); err != nil { + t.Fatalf("Record: %v", err) + } + _, rec := read() + if _, ok := rec[AttrDurationMS]; !ok { + t.Fatalf("a 0 ms duration was dropped as absent: %v", rec) + } +} + +// --- the resource layer ------------------------------------------------------- + +func TestOSAndArchAreOTelNamesInTheResourceLayer(t *testing.T) { + // §1.1 forbids re-inventing an attribute OTel already names, so these are + // os.type / host.arch and not tracebloc.os / tracebloc.arch. They are + // compile-time constants of the binary, so they belong to the process, not + // to the occurrence. + res := New(api.EnvProd, "0.10.9", "h").Resource() + if res["os.type"] != runtime.GOOS { + t.Fatalf("os.type = %q, want %q", res["os.type"], runtime.GOOS) + } + if res["host.arch"] != runtime.GOARCH { + t.Fatalf("host.arch = %q, want %q", res["host.arch"], runtime.GOARCH) + } + // …and being resource scope, a call site may not send them. The generic + // version of this assertion iterates resourceScope, so it covers these two + // automatically; this names the rule that must fire. + e := New(api.EnvProd, "0.10.9", "h") + for _, k := range []string{"os.type", "host.arch"} { + err := e.Emit(EventCommandSucceeded, Attrs{k: "impostor"}) + if err == nil { + t.Fatalf("a call site set %q", k) + } + if !strings.Contains(err.Error(), "RESOURCE scope") { + t.Fatalf("%q was refused by another rule, so the layer check is doing "+ + "no work for it: %v", k, err) + } + } +} + +func assertNoCanary(t *testing.T, res map[string]string, rec map[string]any) { + t.Helper() + for k, v := range res { + if strings.Contains(k, canary) || strings.Contains(v, canary) { + t.Fatalf("resource leaked the canary: %q = %q", k, v) + } + } + for k, v := range rec { + if strings.Contains(k, canary) || strings.Contains(fmt.Sprint(v), canary) { + t.Fatalf("record leaked the canary: %q = %v", k, v) + } + } +} diff --git a/internal/telemetry/telemetry.go b/internal/telemetry/telemetry.go index 1795053..df3689d 100644 --- a/internal/telemetry/telemetry.go +++ b/internal/telemetry/telemetry.go @@ -22,6 +22,7 @@ import ( "fmt" "reflect" "regexp" + "runtime" "sort" "strings" "time" @@ -85,10 +86,17 @@ var retired = map[string]bool{ // resourceScope is set once per process by New. A call site may never send one. // tracebloc.component is listed even though it is correctly tracebloc.-prefixed: // the namespace rule alone would wave it past. +// +// os.type and host.arch are here rather than in recordScope because they +// describe the PROCESS, not the occurrence: they are compile-time constants of +// this binary and cannot differ between two events from one run. §1.1 also +// forbids re-inventing them as tracebloc.os / tracebloc.arch — OpenTelemetry +// already names them, so the contract requires OTel's spelling. var resourceScope = map[string]bool{ "service.name": true, "service.version": true, "service.instance.id": true, "deployment.environment": true, "tracebloc.component": true, "tracebloc.tenant.id": true, + "os.type": true, "host.arch": true, } // recordScope is the set of OTel names a call site MAY send. event.name is @@ -127,6 +135,13 @@ func New(env, version, instanceID string) *Emitter { "service.name": Service, "tracebloc.component": Component, "service.version": normaliseVersion(version), + // backend#1907 asks for OS/arch on every command outcome. They are + // read from the Go build's own constants, never from `uname`, a + // hostname or an env var: a compile-time constant cannot carry a + // customer identifier, and runtime.GOOS/GOARCH are closed sets, so + // there is no value here a query cannot filter on. + "os.type": runtime.GOOS, + "host.arch": runtime.GOARCH, }} // §1.2 — omitted rather than stamped empty. os.Hostname() returns "" on // error, and an empty service.instance.id is the "sent as empty rather than diff --git a/scripts/coverage-floor.sh b/scripts/coverage-floor.sh index 723cfd3..01c9301 100755 --- a/scripts/coverage-floor.sh +++ b/scripts/coverage-floor.sh @@ -13,6 +13,12 @@ # tests. Current (develop, 2026-07-14, ubuntu CI runner): internal/cli 82.9%, # internal/submit 80.4%, internal/push 89.0%, internal/cluster 74.6%. # +# internal/telemetry joined the list with backend#1907, when it stopped being +# an unwired helper and became the thing that decides what leaves a customer's +# machine. It is the privacy boundary for the CLI, so a rotting test there is +# not a coverage regression like the others — it is the guard going quiet. It +# measures 100.0% today; the floor is set at 95 for the usual ratchet headroom. +# # NOTE: internal/cluster measures higher on a dev machine with a real # ~/.kube/config (78.5% on macOS) than on the bare CI runner (74.6%) — the # kubeconfig-resolution paths only execute where one exists. Floors must be @@ -30,6 +36,7 @@ internal/cli:80 internal/submit:78 internal/push:87 internal/cluster:73 +internal/telemetry:95 " status=0 diff --git a/scripts/install.ps1 b/scripts/install.ps1 index cc63714..3044df0 100644 --- a/scripts/install.ps1 +++ b/scripts/install.ps1 @@ -10,7 +10,10 @@ # 2. Resolves the latest release tag (or honors $env:RELEASE_VERSION) # 3. Downloads tracebloc--windows-amd64.exe + SHA256SUMS # 4. Verifies SHA256 -# 5. (Optional) Verifies cosign signature if cosign.exe is on PATH +# 5. Verifies the cosign signature — MANDATORY (RFC-0001 R8). If cosign +# isn't on PATH it bootstraps a pinned, checksum-verified copy; if it +# can't, the install FAILS CLOSED rather than trusting the same-channel +# SHA256 alone. TRACEBLOC_ALLOW_UNVERIFIED=1 is the one (loud) escape. # 6. Installs to $env:USERPROFILE\AppData\Local\Programs\tracebloc\tracebloc.exe # and PATH-adds it via user-scope env var # @@ -45,6 +48,119 @@ $InstallPrefix = if ($env:INSTALL_PREFIX) { $env:INSTALL_PREFIX } ` $GitHubRepo = 'tracebloc/cli' $BinaryName = 'tracebloc.exe' +# Pinned verifier. Keep in lockstep with tracebloc/client's install.sh / +# install.ps1 COSIGN_VERSION and release.yml's cosign-installer pin. +$CosignVersion = 'v2.4.1' + +# The ONE escape from mandatory verification, for a genuinely constrained +# environment. Loud, and never the default (RFC-0001 R8). +# +# Compare against '1' explicitly. NOT [bool]$env:... — PowerShell casts any +# non-empty string to $true, so TRACEBLOC_ALLOW_UNVERIFIED=0 would have +# switched the bypass ON. Matches install.sh's `[ "$ALLOW_UNVERIFIED" = "1" ]`. +$AllowUnverified = ($env:TRACEBLOC_ALLOW_UNVERIFIED -eq '1') + +# TLS 1.2 floor. PowerShell 5.1 defaults to SSL3/TLS1.0 on older Windows, and +# every fetch below carries either the binary we are about to run or the +# verifier that authenticates it — neither may negotiate down. PS7+ already +# defaults higher; setting it is harmless there. +# +# ASSIGN Tls12, do NOT -bor onto the default: OR-ing Tls12 on LEAVES SSL3/TLS1.0/1.1 +# advertised, so a downgrade stays on the table — the exact thing this floor exists +# to remove. Tls12 alone is the floor and is always negotiable on any host that can +# reach us. We deliberately do NOT also add Tls13: [Enum]::IsDefined is true on +# .NET 4.8 even where Schannel cannot negotiate 1.3 (Win10 21H1, Server 2019), and +# assigning it then THROWS — the empty catch would swallow that and leave the floor +# unset, so a `Tls12 -bor Tls13` attempt can defeat the very floor it decorates +# (cli#528 Bugbot). 1.2 is secure for these fetches; 1.3 is not worth that risk. +try { + [Net.ServicePointManager]::SecurityProtocol = [Net.SecurityProtocolType]::Tls12 +} catch { } + +# --------------------------------------------------------------------- +# cosign bootstrap (RFC-0001 R8). +# --------------------------------------------------------------------- + +function Get-Sha256([string]$Path) { + return (Get-FileHash -Algorithm SHA256 -Path $Path).Hash.ToLower() +} + +# Resolve a cosign we can vouch for: one already on PATH, else a pinned build +# fetched and checked against sigstore's own published checksums. Returns the +# path, or $null when it cannot be obtained — the caller decides what that means. +# +# A cosign we cannot vouch for is no better than no cosign, so a checksum +# mismatch returns $null rather than a usable path. +function Resolve-Cosign([string]$TmpDir) { + $onPath = Get-Command cosign -ErrorAction SilentlyContinue + if ($onPath) { return $onPath.Source } + + # BOTH architectures fetch the amd64 build, deliberately. + # + # Sigstore has never published a Windows arm64 cosign — not at $CosignVersion, + # not at any release. `cosign-windows-amd64.exe` is the only Windows asset + # there is, so asking for a per-arch name 404s and blocks Windows-on-ARM + # permanently (tracebloc/client#734, fixed there the same way). + # + # Running it under Windows-on-ARM's x64 emulation costs nothing that matters: + # cosign verifies a signature over BYTES, so the instruction set it was + # compiled for cannot change the verdict, and the artifact we hand it is + # still the native arm64 binary. It is checksum-verified below exactly as on + # amd64, so the trust chain is identical. + # + # Do not "fix" this to $arch. There is nothing on the other end — and + # since the asset is arch-independent there is nothing to branch on + # either; whether it RUNS here is Test-CosignRuns' question, not ours. + $base = "https://github.com/sigstore/cosign/releases/download/$CosignVersion" + $asset = 'cosign-windows-amd64.exe' + $bin = Join-Path $TmpDir 'cosign.exe' + $sums = Join-Path $TmpDir 'cosign_checksums.txt' + + Write-Host " cosign not found — downloading pinned cosign $CosignVersion (~17 MB) to verify the signature..." + try { + Invoke-WebRequest -Uri "$base/$asset" -OutFile $bin -UseBasicParsing + Invoke-WebRequest -Uri "$base/cosign_checksums.txt" -OutFile $sums -UseBasicParsing + } catch { + Write-Host " ⚠ couldn't download cosign: $($_.Exception.Message)" + return $null + } + + # cosign_checksums.txt lines: " ". + $want = $null + foreach ($line in Get-Content -LiteralPath $sums) { + $parts = @($line -split '\s+' | Where-Object { $_ -ne '' }) + if ($parts.Count -ge 2 -and $parts[-1] -eq $asset) { $want = $parts[0].ToLower(); break } + } + if (-not $want) { return $null } + if ((Get-Sha256 $bin) -ne $want) { + Write-Host " Bootstrapped cosign failed its own checksum — not using it." -ForegroundColor Red + return $null + } + Write-Host " ✓ cosign $CosignVersion downloaded and checksum-verified" + return $bin +} + +# Can this cosign actually EXECUTE here? A trivial `cosign version`. +# +# A binary that will not start reports through the same channel as a signature +# that did not verify, and those warrant opposite reactions — only one of them +# means the artifact may be tampered with. Windows-on-ARM makes it real: the +# amd64 build needs x64 emulation, and where that is absent cosign never runs. +# The 255 preset means a binary that never starts cannot leave a stale 0 behind. +function Test-CosignRuns([string]$Cosign) { + $global:LASTEXITCODE = 255 + $prev = $ErrorActionPreference + try { + $ErrorActionPreference = 'Continue' + & $Cosign version 2>&1 | Out-Null + } catch { + return $false + } finally { + $ErrorActionPreference = $prev + } + return ($LASTEXITCODE -eq 0) +} + # --------------------------------------------------------------------- # Detect arch. # --------------------------------------------------------------------- @@ -137,45 +253,116 @@ try { Write-Host " ✓ checksum matches" # ------------------------------------------------------------- - # Cosign signature verification (optional). + # Cosign signature verification — MANDATORY (RFC-0001 R8). + # + # The SHA256 above is same-channel: it comes from the same GitHub + # release as the binary, so whoever could swap the binary could swap + # SHA256SUMS with it. It proves the download completed, not who built + # it. The cosign signature is the independent, Sigstore-rooted proof + # that tracebloc's release workflow produced these bytes. + # + # So this no longer skips when cosign is absent — it bootstraps a + # pinned, checksum-verified cosign, and FAILS CLOSED when it cannot. + # This mirrors install.sh exactly; Windows was the one platform still + # installing on the checksum alone (backend#2078). + # + # TRACEBLOC_ALLOW_UNVERIFIED=1 covers "cannot verify" — no cosign, no + # .sig/.cert. It deliberately does NOT cover a verification that ran + # and FAILED: that is evidence of tampering, and no env var overrides + # it. # ------------------------------------------------------------- - if (Get-Command cosign -ErrorAction SilentlyContinue) { + $cosign = Resolve-Cosign $tmpDir + + if ($cosign -and -not (Test-CosignRuns $cosign)) { + # Distinct from "no cosign": we have one, it just won't start here. + # On Windows-on-ARM that means x64 emulation is missing or blocked; + # it can also be SmartScreen/AV quarantine or a policy block. Saying + # "install cosign" here would be useless advice — one is installed. + if ($AllowUnverified) { + Write-Host " WARNING: cosign is present but won't run here — signature NOT" -ForegroundColor Yellow + Write-Host " verified (TRACEBLOC_ALLOW_UNVERIFIED=1)." -ForegroundColor Yellow + $cosign = $null + } else { + Write-Host "Error: cosign was found but won't execute on this machine, so the" -ForegroundColor Red + Write-Host " signature can't be verified (RFC-0001 R8)." -ForegroundColor Red + Write-Host " On Windows-on-ARM this usually means x64 emulation is" -ForegroundColor Red + Write-Host " unavailable; it can also be a quarantine or policy block." -ForegroundColor Red + Write-Host " Fix that, or for a constrained environment re-run with" -ForegroundColor Red + Write-Host " TRACEBLOC_ALLOW_UNVERIFIED=1." -ForegroundColor Red + exit 1 + } + } + + if (-not $cosign) { + if (-not $AllowUnverified) { + Write-Host "Error: cosign is required to verify the binary's signature and" -ForegroundColor Red + Write-Host " could not be found or bootstrapped — refusing to install on" -ForegroundColor Red + Write-Host " an unauthenticated, same-channel checksum alone (RFC-0001 R8)." -ForegroundColor Red + Write-Host " Fix: install cosign and re-run —" -ForegroundColor Red + Write-Host " https://docs.sigstore.dev/cosign/system_config/installation/" -ForegroundColor Red + Write-Host " or for a constrained environment re-run with" -ForegroundColor Red + Write-Host " TRACEBLOC_ALLOW_UNVERIFIED=1." -ForegroundColor Red + exit 1 + } + Write-Host " WARNING: cosign unavailable and couldn't be bootstrapped —" -ForegroundColor Yellow + Write-Host " signature NOT verified (TRACEBLOC_ALLOW_UNVERIFIED=1). The SHA256" -ForegroundColor Yellow + Write-Host " above is same-channel only; do not use this path in production." -ForegroundColor Yellow + } else { Write-Host "Verifying cosign signature..." - # Separate "download .sig/.cert" (recoverable if absent — old - # releases predate signing) from "verify the downloaded sig" - # (NOT recoverable — a failed verification means the binary - # is potentially tampered, refuse to install). Bugbot PR #11 - # caught the prior structure: with $ErrorActionPreference = - # 'Stop', Write-Error inside the try-block was thrown and - # caught by the same catch that handled missing-sig, so a - # failed verify silently downgraded to "skip + continue." + + # "download .sig/.cert" and "verify the downloaded sig" must stay + # separate. With $ErrorActionPreference = 'Stop', a Write-Error + # inside the try-block is thrown and caught by the same catch that + # handles a missing sig — so a FAILED verify silently downgrades to + # "skip + continue" (Bugbot, PR #11). The verify below therefore + # runs OUTSIDE any try/catch: & invokes cosign as an external + # process, whose non-zero $LASTEXITCODE cannot be caught anyway. $sigDownloaded = $false try { Invoke-WebRequest -Uri "$baseUrl/$binaryFile.sig" -OutFile (Join-Path $tmpDir "$binaryFile.sig") -UseBasicParsing Invoke-WebRequest -Uri "$baseUrl/$binaryFile.cert" -OutFile (Join-Path $tmpDir "$binaryFile.cert") -UseBasicParsing $sigDownloaded = $true } catch { - Write-Host " ⚠ couldn't download .sig/.cert — release may pre-date signing." + if (-not $AllowUnverified) { + Write-Host "Error: couldn't download $binaryFile.sig / .cert for $tag — the" -ForegroundColor Red + Write-Host " release is unsigned or incomplete. Every supported release" -ForegroundColor Red + Write-Host " is cosign-signed; refusing to install unverified (RFC-0001 R8)." -ForegroundColor Red + Write-Host ' Pin a signed $env:RELEASE_VERSION, or re-run with TRACEBLOC_ALLOW_UNVERIFIED=1.' -ForegroundColor Red + exit 1 + } + Write-Host " WARNING: .sig/.cert not published for $tag — signature NOT" -ForegroundColor Yellow + Write-Host " verified (TRACEBLOC_ALLOW_UNVERIFIED=1)." -ForegroundColor Yellow } + if ($sigDownloaded) { - # Verify OUTSIDE the try/catch: a non-zero $LASTEXITCODE - # from cosign is a hard refusal, not a swallowed - # exception. & invokes cosign as an external process, - # which doesn't interact with $ErrorActionPreference. - & cosign verify-blob ` + # PRESET 255, exactly as Test-CosignRuns does — and for the same reason, + # which this call site was missing (Bugbot, HIGH, cli#528). + # + # $LASTEXITCODE persists from the PREVIOUS command. Test-CosignRuns runs + # `cosign version` immediately before this, so a shim that exits 0 there + # and then never sets an exit code on verify-blob leaves $LASTEXITCODE at + # 0 — and the `-ne 0` check below reads that stale success as a valid + # signature. The installer then prints "cosign signature valid" and + # installs a binary nothing verified. + # + # 255 means "no verdict yet": a verifier that never runs cannot inherit a + # pass. Only cosign actually completing can bring it back to 0. That is + # the whole guarantee behind RFC-0001 R8, and it was one line away. + $global:LASTEXITCODE = 255 + & $cosign verify-blob ` --certificate-identity-regexp "https://github.com/$GitHubRepo/.github/workflows/release.yml@refs/tags/v.*" ` --certificate-oidc-issuer 'https://token.actions.githubusercontent.com' ` --certificate (Join-Path $tmpDir "$binaryFile.cert") ` --signature (Join-Path $tmpDir "$binaryFile.sig") ` (Join-Path $tmpDir $binaryFile) 2>$null if ($LASTEXITCODE -ne 0) { + # No TRACEBLOC_ALLOW_UNVERIFIED branch here, deliberately. + # Verification RAN and said no. Write-Host "Error: cosign signature verification FAILED — refusing to install." -ForegroundColor Red exit 1 } Write-Host " ✓ cosign signature valid" } - } else { - Write-Host " (cosign not installed; SHA256 verified, signature skipped)" } # ------------------------------------------------------------- diff --git a/scripts/tests/install-ps1-functions.tests.ps1 b/scripts/tests/install-ps1-functions.tests.ps1 new file mode 100644 index 0000000..d696c96 --- /dev/null +++ b/scripts/tests/install-ps1-functions.tests.ps1 @@ -0,0 +1,287 @@ +# ============================================================================= +# install-ps1-functions.tests.ps1 — behavioural tests for install.ps1's +# verification helpers (RFC-0001 R8, backend#2078). +# +# install.ps1 is a `irm | iex` entrypoint that ends in Windows-registry PATH +# writes, so it cannot be driven end-to-end on the Linux runner the way +# install-verify.sh drives install.sh. What CAN be driven — and is where the +# security decisions actually live — are its pure helpers. +# +# They are EXTRACTED FROM THE REAL FILE by AST and evaluated here. Nothing +# below re-implements a rule from install.ps1: if a helper changes, this test +# runs the changed helper. A copy would pass while production broke, which is +# the failure mode CLAUDE.md rule 9 names. +# +# No Pester: this repo has no Pester tier, and standing one up to assert four +# things costs more than it returns. Exit code is the contract. +# ============================================================================= +Set-StrictMode -Version Latest +$ErrorActionPreference = 'Stop' + +$script:Pass = 0 +$script:Fail = 0 +function ok ([string]$m) { Write-Host " ok $m"; $script:Pass++ } +function bad ([string]$m) { Write-Host " FAIL $m"; $script:Fail++ } +function is ([string]$m, $got, $want) { + if ($got -eq $want) { ok $m } else { bad "$m (got '$got', want '$want')" } +} + +$installer = Join-Path $PSScriptRoot '..' 'install.ps1' | Resolve-Path + +# ── extract, never restate ────────────────────────────────────────────────── +$errs = $null +$ast = [System.Management.Automation.Language.Parser]::ParseFile( + "$installer", [ref]$null, [ref]$errs) +if ($errs) { + # A parse error is a finding, not a skip: we cannot tell whether the + # helpers are correct, and "cannot tell" never passes (CLAUDE.md rule 3). + $errs | ForEach-Object { Write-Host " FAIL parse: $($_.Message)" } + exit 1 +} +ok 'install.ps1 parses' + +function Get-Fn([string]$Name) { + $hit = $ast.FindAll({ + param($n) + $n -is [System.Management.Automation.Language.FunctionDefinitionAst] -and + $n.Name -eq $Name + }, $true) + if ($hit.Count -ne 1) { + bad "expected exactly one definition of $Name, found $($hit.Count)" + return $null + } + return $hit[0].Extent.Text +} + +foreach ($n in 'Get-Sha256', 'Resolve-Cosign', 'Test-CosignRuns') { + $src = Get-Fn $n + if (-not $src) { Write-Host "install-ps1-functions: $script:Pass passed, $script:Fail failed"; exit 1 } + Invoke-Expression $src +} +ok 'Get-Sha256 / Resolve-Cosign / Test-CosignRuns extracted' + +# ── 1. $AllowUnverified: only the literal '1' opts out ────────────────────── +# The bypass is the single thing standing between a user and an unverified +# binary, so its parsing gets a truth table rather than a spot check. +# +# '0' is the case that matters. [bool]'0' is $true in PowerShell — any +# non-empty string is — so the natural-looking `[bool]$env:...` turns +# TRACEBLOC_ALLOW_UNVERIFIED=0 into "verification off". Caught here before it +# shipped; this test is why it stays caught. +$assign = $ast.FindAll({ + param($n) + $n -is [System.Management.Automation.Language.AssignmentStatementAst] -and + $n.Left -is [System.Management.Automation.Language.VariableExpressionAst] -and + $n.Left.VariablePath.UserPath -eq 'AllowUnverified' +}, $true) +if ($assign.Count -ne 1) { + bad "expected one \$AllowUnverified assignment, found $($assign.Count)" +} else { + $expr = $assign[0].Right.Extent.Text + # Written down independently of the expression under test — the values come + # from what a user might plausibly set, not from reading the matcher + # (CLAUDE.md rule 9's "never test a list against itself"). + $cases = @( + @{ v = $null; want = $false; why = 'unset' }, + @{ v = ''; want = $false; why = 'empty' }, + @{ v = '0'; want = $false; why = "the string 0" }, + @{ v = 'false'; want = $false; why = "'false'" }, + @{ v = 'no'; want = $false; why = "'no'" }, + @{ v = 'true'; want = $false; why = "'true' is not the documented opt-in" }, + @{ v = '1'; want = $true; why = 'the documented opt-in' } + ) + foreach ($c in $cases) { + if ($null -eq $c.v) { Remove-Item Env:TRACEBLOC_ALLOW_UNVERIFIED -ErrorAction SilentlyContinue } + else { $env:TRACEBLOC_ALLOW_UNVERIFIED = $c.v } + is "AllowUnverified is $($c.want) for $($c.why)" (Invoke-Expression $expr) $c.want + } + Remove-Item Env:TRACEBLOC_ALLOW_UNVERIFIED -ErrorAction SilentlyContinue +} + +# ── 2. Test-CosignRuns distinguishes "won't start" from "ran and said no" ─── +# This is the Windows-on-ARM case in miniature: a cosign that cannot execute +# reports through the same channel as a signature that failed to verify, and +# those two warrant opposite messages. /bin/true and /bin/false stand in for a +# cosign that starts and one that doesn't. +$tmp = Join-Path ([IO.Path]::GetTempPath()) ("tb-ps1-" + [Guid]::NewGuid()) +New-Item -ItemType Directory -Path $tmp -Force | Out-Null +try { + # Resolved, not hardcoded: /bin/true on Linux, /usr/bin/true on macOS. + $trueBin = (Get-Command true -CommandType Application).Source | Select-Object -First 1 + $falseBin = (Get-Command false -CommandType Application).Source | Select-Object -First 1 + is 'Test-CosignRuns true for a binary that exits 0' (Test-CosignRuns $trueBin) $true + is 'Test-CosignRuns false for a binary that exits 1' (Test-CosignRuns $falseBin) $false + + # The one that actually reproduces Windows-on-ARM without emulation: the + # file is there, it just cannot be executed. Must be $false, not a throw — + # a throw would escape into the installer's `Stop` preference and surface + # as a stack trace instead of the actionable message. + $noexec = Join-Path $tmp 'not-executable' + Set-Content -LiteralPath $noexec -Value 'not a binary' + is 'Test-CosignRuns false for a non-executable file' (Test-CosignRuns $noexec) $false + is 'Test-CosignRuns false for a path that is absent' (Test-CosignRuns (Join-Path $tmp 'nope')) $false + + # A stale success must not leak through, and this is the input that proves + # it. An absent path throws and returns early, so it never reads + # $LASTEXITCODE — a "stale 0" assertion built on one is vacuous (it was, and + # the mutation caught it). The reachable case is a PowerShell SHIM: scoop and + # chocolatey install tools as cosign.ps1, Get-Command finds .ps1 on PATH + # whatever PATHEXT says, and `&` dispatches it IN-PROCESS — setting no + # $LASTEXITCODE at all. Without the 255 preset the helper then reads the 0 + # left by the last successful command and reports that a shim which did + # nothing is a working cosign. + $shim = Join-Path $tmp 'cosign.ps1' + Set-Content -LiteralPath $shim -Value 'Write-Output "shim: nothing to do"' + & $trueBin # arm a stale 0, exactly as the happy path does + is 'Test-CosignRuns false for a PowerShell shim that sets no exit code' ` + (Test-CosignRuns $shim) $false + + # ── 3. Resolve-Cosign returns the one on PATH without a download ───────── + $onpath = Join-Path $tmp 'pathbin' + New-Item -ItemType Directory -Path $onpath -Force | Out-Null + $fake = Join-Path $onpath 'cosign' + Set-Content -LiteralPath $fake -Value "#!/bin/sh`nexit 0" + & chmod +x $fake + $savedPath = $env:PATH + try { + $env:PATH = "${onpath}:${savedPath}" + $got = Resolve-Cosign $tmp + if ($got -and (Resolve-Path $got).Path -eq (Resolve-Path $fake).Path) { + ok 'Resolve-Cosign short-circuits to the cosign already on PATH' + } else { + bad "Resolve-Cosign short-circuits to the cosign already on PATH (got '$got')" + } + } finally { $env:PATH = $savedPath } + + # ── 4. a bootstrapped cosign that fails its own checksum is refused ────── + # The security-critical branch. A verifier we cannot vouch for is worth no + # more than no verifier, so Resolve-Cosign must return $null — NOT a usable + # path — and let the caller's fail-closed logic run. + # + # Shadowing the cmdlet: a function defined here wins over Invoke-WebRequest + # for the duration, so the extracted Resolve-Cosign hits this instead of + # the network. $CosignVersion is what the real file pins. + $CosignVersion = 'v0.0.0-test' + function Invoke-WebRequest { + param($Uri, $OutFile) + if ($Uri -match 'checksums') { + # A well-formed checksums file naming the right asset — with the + # WRONG digest. Nothing about the transfer failed; the bytes are + # simply not the bytes sigstore published. + Set-Content -LiteralPath $OutFile -Value ("{0} cosign-windows-amd64.exe" -f ('0' * 64)) + } else { + Set-Content -LiteralPath $OutFile -Value 'pretend-cosign-bytes' + } + } + $bootstrapDir = Join-Path $tmp 'boot' + New-Item -ItemType Directory -Path $bootstrapDir -Force | Out-Null + is 'Resolve-Cosign returns $null when the bootstrap fails its checksum' ` + (Resolve-Cosign $bootstrapDir) $null + + # …and returns a path when the digest DOES match, so the check above is + # failing for the reason it claims and not because the mock never worked. + # Without this pair, a Resolve-Cosign that always returned $null would look + # perfectly healthy (CLAUDE.md rule 5: assert the anchor applied). + $good = Join-Path $tmp 'good' + New-Item -ItemType Directory -Path $good -Force | Out-Null + $payload = 'pretend-cosign-bytes' + function Invoke-WebRequest { + param($Uri, $OutFile) + if ($Uri -match 'checksums') { + $probe = Join-Path ([IO.Path]::GetTempPath()) ([Guid]::NewGuid()) + Set-Content -LiteralPath $probe -Value 'pretend-cosign-bytes' -NoNewline + # Match how Set-Content writes the payload below, byte for byte. + $h = (Get-FileHash -Algorithm SHA256 -Path $probe).Hash.ToLower() + Remove-Item $probe -Force + Set-Content -LiteralPath $OutFile -Value "$h cosign-windows-amd64.exe" + } else { + Set-Content -LiteralPath $OutFile -Value 'pretend-cosign-bytes' -NoNewline + } + } + $gotGood = Resolve-Cosign $good + if ($gotGood -and (Test-Path $gotGood)) { + ok 'Resolve-Cosign returns the bootstrapped path when the checksum matches' + } else { + bad "Resolve-Cosign returns the bootstrapped path when the checksum matches (got '$gotGood')" + } +} finally { + Remove-Item -Recurse -Force $tmp -ErrorAction SilentlyContinue +} + +# ── 5. the verification that RAN and failed has no escape hatch ───────────── +# TRACEBLOC_ALLOW_UNVERIFIED covers "cannot verify" — no cosign, no .sig/.cert. +# It must NOT cover "verified and FAILED": that is evidence of tampering, and no +# env var overrides it. Asserted against the AST rather than by reading the +# file, so it holds however the branch is reformatted. +$verifyCall = $ast.FindAll({ + param($n) + $n -is [System.Management.Automation.Language.CommandAst] -and + $n.Extent.Text -match 'verify-blob' +}, $true) +if ($verifyCall.Count -ne 1) { + bad "expected one cosign verify-blob call, found $($verifyCall.Count)" +} else { + ok 'exactly one cosign verify-blob call' + # The refusal is the `if ($LASTEXITCODE -ne 0)` that follows it. + $guard = $ast.FindAll({ + param($n) + $n -is [System.Management.Automation.Language.IfStatementAst] -and + $n.Extent.StartOffset -gt $verifyCall[0].Extent.EndOffset -and + $n.Clauses[0].Item1.Extent.Text -match 'LASTEXITCODE' + }, $true) | Sort-Object { $_.Extent.StartOffset } | Select-Object -First 1 + + if (-not $guard) { + bad 'no $LASTEXITCODE guard follows the verify-blob call' + } else { + $body = $guard.Clauses[0].Item2.Extent.Text + if ($body -match '\bexit\b') { ok 'a failed verification exits' } + else { bad 'a failed verification does not exit' } + if ($body -match 'AllowUnverified') { + bad 'the failed-verification branch has a TRACEBLOC_ALLOW_UNVERIFIED escape' + } else { + ok 'no TRACEBLOC_ALLOW_UNVERIFIED escape from a FAILED verification' + } + } +} + +# ── 6. every "cannot verify" path is a refusal by default ─────────────────── +# Each branch that gives up on verifying must sit under an $AllowUnverified +# test AND exit when that test is false. Counted from the AST: three such +# branches exist (no cosign, cosign won't run, no .sig/.cert). A fourth added +# later without a refusal shows up here as a count change rather than passing +# silently. +$escapes = $ast.FindAll({ + param($n) + $n -is [System.Management.Automation.Language.IfStatementAst] -and + $n.Clauses[0].Item1.Extent.Text -match 'AllowUnverified' +}, $true) +$withExit = @($escapes | Where-Object { $_.Extent.Text -match '(?m)^\s*exit 1\s*$' }) +if ($escapes.Count -ge 3 -and $withExit.Count -eq $escapes.Count) { + ok "all $($escapes.Count) cannot-verify branches refuse unless opted out" +} else { + bad ("cannot-verify branches: $($escapes.Count) found, " + + "$($withExit.Count) refuse by default (want >=3, all refusing)") +} + +# ── 7. no user-facing message loses text to a bash-shaped escape ──────────── +# `\$` is not an escape in PowerShell — the escape character is a backtick. In a +# double-quoted string it renders as a literal backslash followed by the +# EXPANDED variable, so "Pin a signed \$env:RELEASE_VERSION" prints +# "Pin a signed ," silently dropping the one thing the user needed. Written +# during this change and caught by rendering the messages; asserted here so the +# next person reaching for bash muscle memory gets told. +$backslashDollar = $ast.FindAll({ + param($n) + $n -is [System.Management.Automation.Language.ExpandableStringExpressionAst] -and + $n.Value -match '\\\$' +}, $true) +if ($backslashDollar.Count -eq 0) { + ok 'no double-quoted message uses a bash-style \$ escape' +} else { + $backslashDollar | ForEach-Object { bad "bash-style \$ escape in: $($_.Extent.Text)" } +} + +Write-Host '' +Write-Host "install-ps1-functions: $script:Pass passed, $script:Fail failed" +if ($script:Fail -gt 0) { exit 1 } +exit 0 diff --git a/scripts/tests/install-ps1-verify.sh b/scripts/tests/install-ps1-verify.sh new file mode 100755 index 0000000..3bbb674 --- /dev/null +++ b/scripts/tests/install-ps1-verify.sh @@ -0,0 +1,114 @@ +#!/usr/bin/env bash +# ============================================================================= +# install-ps1-verify.sh — assert the MANDATORY cosign verification in +# install.ps1 (RFC-0001 R8, backend#2078). +# +# install-verify.sh's sibling. Same property, other platform: the Windows +# installer must NOT install on the same-channel SHA256 alone when cosign is +# absent. Until backend#2078 it did exactly that, printing +# "(cosign not installed; SHA256 verified, signature skipped)" — so Windows +# was the one platform where the README's "verification is mandatory … fails +# closed" was false. +# +# Two tiers: +# 1. string-level, here — the old degrade path is gone and stays gone. +# 2. behavioural, in install-ps1-functions.tests.ps1 — the helpers are +# extracted from install.ps1 by AST and actually executed. +# +# Why not drive install.ps1 end-to-end the way install-verify.sh drives +# install.sh: it finishes with Windows-registry PATH writes that throw on a +# Linux runner, so a full run would fail for reasons unrelated to what is +# under test. Tier 2 runs the parts that hold the security decisions. +# ============================================================================= +# Deliberately NO -e: this harness counts pass/fail itself and must keep going +# after a failed assertion (same shape as install-verify.sh). +set -uo pipefail + +SELF_DIR="$(cd "$(dirname "$0")" && pwd)" +INSTALLER="$SELF_DIR/../install.ps1" + +PASS=0 +FAIL=0 +ok() { printf ' ok %s\n' "$1"; PASS=$((PASS+1)); } +bad() { printf ' FAIL %s\n' "$1"; FAIL=$((FAIL+1)); } + +if [ ! -f "$INSTALLER" ]; then + # Fail closed: an installer we cannot read is not an installer that verifies. + printf ' FAIL install.ps1 not found at %s\n' "$INSTALLER" + exit 1 +fi + +# ── 1. the exact old-behaviour string must never come back ────────────────── +# Named in the ticket's acceptance criteria. install-verify.sh asserts this +# against install.sh; this is the Windows equivalent. +if grep -q 'signature skipped' "$INSTALLER"; then + bad "found the old 'signature skipped' degrade path" +else + ok "no 'signature skipped' degrade path" +fi + +# ── 2. verification is not gated on cosign happening to be installed ──────── +# The old shape was `if (Get-Command cosign …) { verify } else { skip }`, which +# makes the default — a fresh Windows box, where cosign is never present — +# the unverified one. +if grep -Eq 'if *\( *Get-Command +cosign' "$INSTALLER"; then + bad 'verification is still gated on cosign being pre-installed' +else + ok 'verification is not gated on cosign being pre-installed' +fi + +# ── 3. the header no longer advertises verification as optional ───────────── +# A stale header is how the next reader concludes the skip is intended. +if grep -Eq '^# *[0-9]+\. *\(Optional\).*cosign' "$INSTALLER"; then + bad 'the header still describes cosign verification as (Optional)' +else + ok 'the header describes verification as mandatory' +fi + +# ── 4. the bootstrap fetches the only Windows asset sigstore publishes ────── +# There has never been a cosign-windows-arm64.exe. Asking for one 404s and +# blocks Windows-on-ARM permanently — the bug this repo's sibling hit in +# tracebloc/client#734. The amd64 build under emulation is correct: cosign +# verifies a signature over bytes. +if grep -q 'cosign-windows-amd64.exe' "$INSTALLER" \ + && ! grep -q 'cosign-windows-arm64' "$INSTALLER"; then + ok 'the cosign bootstrap asks for the amd64 asset on both architectures' +else + bad 'the cosign bootstrap asks for an asset sigstore does not publish' +fi + +# ── 5. the TLS floor is ASSIGNED, not OR-ed onto the default ──────────────── +# `-bor Tls12` onto [Net.ServicePointManager]::SecurityProtocol leaves SSL3/ +# TLS1.0/1.1 advertised, so a fetch can still negotiate down — the exact thing +# the floor exists to remove (cli#528 Bugbot). The floor must ASSIGN the value. +# Collapse newlines first: the old form spanned two lines (`= \n -bor`). +_ps1_flat=$(tr '\n' ' ' < "$INSTALLER") +if printf '%s' "$_ps1_flat" | grep -Eq 'SecurityProtocol[[:space:]]*=[[:space:]]*\[Net\.ServicePointManager\]::SecurityProtocol[[:space:]]*-bor'; then + bad 'the TLS floor OR-s onto the default SecurityProtocol (SSL3/TLS1.0/1.1 stay advertised)' +elif printf '%s' "$_ps1_flat" | grep -Eq 'ServicePointManager\]::SecurityProtocol[[:space:]]*=[[:space:]]*\[Net\.SecurityProtocolType\]::Tls12'; then + ok 'the TLS floor assigns Tls12 directly (weak protocols dropped)' +else + bad 'no assigned TLS 1.2 floor found in the installer' +fi + +# ── 6. behavioural tier ───────────────────────────────────────────────────── +# pwsh is preinstalled on GitHub-hosted ubuntu runners. If it is missing we +# cannot tell whether the helpers behave, and "cannot tell" is a finding, not a +# pass (CLAUDE.md rule 3). Set ALLOW_NO_PWSH=1 to downgrade it on a dev box +# that genuinely has no pwsh — CI never sets it. +if command -v pwsh >/dev/null 2>&1; then + echo + if pwsh -NoProfile -File "$SELF_DIR/install-ps1-functions.tests.ps1"; then + ok 'behavioural tier (install-ps1-functions.tests.ps1)' + else + bad 'behavioural tier (install-ps1-functions.tests.ps1)' + fi + echo +elif [ "${ALLOW_NO_PWSH:-0}" = "1" ]; then + printf ' SKIP behavioural tier — no pwsh, ALLOW_NO_PWSH=1\n' +else + bad 'pwsh not found, so the behavioural tier could not run (ALLOW_NO_PWSH=1 to allow)' +fi + +echo "install-ps1-verify: $PASS passed, $FAIL failed" +[ "$FAIL" -eq 0 ] diff --git a/scripts/tests/install-verify.sh b/scripts/tests/install-verify.sh index 58d30a1..018612a 100755 --- a/scripts/tests/install-verify.sh +++ b/scripts/tests/install-verify.sh @@ -529,6 +529,57 @@ else fi drop_sandbox +# -- 19. install.ps1 presets LASTEXITCODE before verify-blob ----------------- +# THE WINDOWS HALF OF R8, and until now nothing checked it. $LASTEXITCODE persists +# from the previous command, so a cosign shim that exits 0 on `version` (which +# Test-CosignRuns runs immediately before) and then no-ops on verify-blob leaves a +# stale 0 behind — and the `-ne 0` gate reads that as a valid signature. The +# installer prints "cosign signature valid" and installs a binary nothing verified +# (Bugbot, HIGH, cli#528). +# +# Test-CosignRuns already presets 255 for exactly this reason. This asserts the +# same preset guards the call that actually gates the install. +# +# A SOURCE ASSERTION, and the limitation is worth stating: install.ps1 has no +# behavioural coverage at all — there is no pwsh or Pester anywhere in this repo's +# CI, which is why the gap reached a promotion PR. This closes the specific hole; +# it does not make the Windows installer tested. Reproduced behaviourally with +# pwsh before writing it: without the preset the block accepts an unverified +# binary, with it it refuses. +PS1="$SELF_DIR/../install.ps1" +if [ ! -f "$PS1" ]; then + bad "install.ps1 not found — cannot assert the verify-blob exit preset" +else + # The preset must appear INSIDE the $sigDownloaded block and BEFORE the + # verify-blob invocation. Line numbers, so a preset elsewhere in the file + # cannot satisfy it. + # The INVOCATION, not the word: the first version grepped for `verify-blob` + # and matched the explanatory comment written directly above the call, so the + # preset looked out of order and the check failed on correct code. Prose is not + # wiring -- anchor on the `& $cosign` invocation itself. + vb_line=$(grep -n '& \$cosign verify-blob' "$PS1" | head -1 | cut -d: -f1) + # A WHOLE STATEMENT, not a substring (shujaatTracebloc, #529). Two failures in + # opposite directions came from matching text rather than code: + # * a COMMENTED-OUT preset satisfied the check -- the gate dead, case 19 green. + # That is the likelier human mutation (commenting it out while debugging the + # installer) and it was the one not covered. + # * hard-coded single spaces meant `$global:LASTEXITCODE=255` -- correct, + # equivalent PowerShell -- turned case 19 RED on a working gate, asserting the + # installer would install unverified. + # Anchoring start-to-end with flexible spacing closes both. + pre_line=$(awk '/^[[:space:]]*\$global:LASTEXITCODE[[:space:]]*=[[:space:]]*255[[:space:]]*$/ {print NR}' "$PS1" | awk -v v="${vb_line:-0}" '$1 < v {last=$1} END {print last+0}') + # `\$` ESCAPED, like the two sibling patterns in this block. In a POSIX BRE a `$` + # that is not at the end is undefined; an implementation treating it as an anchor + # matches nothing, `blk_line` comes back empty, and case 19 goes red on correct + # code with the "would install unverified" message (shujaatTracebloc, #529). + blk_line=$(grep -n 'if (\$sigDownloaded)' "$PS1" | head -1 | cut -d: -f1) + if [ -n "$vb_line" ] && [ -n "$blk_line" ] && [ "$pre_line" -gt "$blk_line" ] && [ "$pre_line" -lt "$vb_line" ]; then + ok "install.ps1: LASTEXITCODE preset guards verify-blob (line $pre_line, before $vb_line)" + else + bad "install.ps1: no LASTEXITCODE preset between the \$sigDownloaded block (line ${blk_line:-?}) and verify-blob (line ${vb_line:-?}) — a no-op verifier would inherit a stale 0 and install unverified" + fi +fi + echo echo "install-verify: $PASS passed, $FAIL failed" [ "$FAIL" -eq 0 ]