From 3b88a852f83527358e0e6f6c6c456b807df06636 Mon Sep 17 00:00:00 2001 From: lukasWuttke <54042461+LukasWodka@users.noreply.github.com> Date: Tue, 18 Aug 2026 10:32:13 +0200 Subject: [PATCH 1/6] sec(install): Windows verified the signature only if you already had cosign (#523) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit install.ps1 gated cosign verification on `if (Get-Command cosign)`. On a fresh Windows box cosign is never present, so the default path printed (cosign not installed; SHA256 verified, signature skipped) and installed. The SHA256 it kept is same-channel — SHA256SUMS ships from the same release as the binary, so whoever could swap one could swap the other. It proves the download finished, not who built it. That made README.md:75 ("Signature verification is mandatory ... fails closed", no platform qualifier) false on Windows, and only on Windows: install.sh has bootstrapped a pinned cosign and failed closed since backend#889. The README needs no edit — this makes the existing claim true. Windows now does what Linux and macOS do: bootstrap a pinned, checksum-verified cosign, and refuse to install when it can't. TRACEBLOC_ALLOW_UNVERIFIED=1 is the one escape, and it covers "cannot verify" only — never a verification that ran and FAILED, which is tampering evidence no env var should override. Two things this does that install.sh does not have to: - The bootstrap fetches cosign-windows-amd64.exe on BOTH architectures. Sigstore has never published a Windows arm64 build, so a per-arch name 404s and locks Windows-on-ARM out permanently (the same bug as tracebloc/client#734). Verification is over bytes, so the verifier's instruction set cannot change the verdict. - Test-CosignRuns separates "cosign won't start here" from "the signature is bad". They arrive through the same channel and warrant opposite messages — only one means the artifact may be tampered with. Absent x64 emulation on Windows-on-ARM is the case that makes this real, and "install cosign" would be useless advice there. Also sets a TLS 1.2 floor: PS 5.1 defaults to SSL3/TLS1.0 on older Windows, and every fetch here carries either the binary or the verifier that authenticates it. Tests — scripts/tests/install-ps1-verify.sh, install-verify.sh's sibling, wired into the same CI job. Against develop's installer: 0 passed, 5 failed. After: 5 passed, 0 failed. install.ps1 cannot be driven end-to-end on the Linux runner (it ends in Windows-registry PATH writes), so the behavioural tier extracts the helpers from the real file BY AST and executes them — a copy would pass while production broke. Six guards, each mutation-proven, each mutation asserted to have applied: [bool]$env:... instead of -eq '1' -> 4 fail checksum mismatch stops refusing -> 1 fail $LASTEXITCODE not armed before the probe -> 1 fail an opt-out on a FAILED verification -> 1 fail the no-cosign degrade restored -> 1 fail a \$ escape in a message -> 1 fail Three of those are bugs this change made and this tier caught before review: - $AllowUnverified was [bool]$env:TRACEBLOC_ALLOW_UNVERIFIED. Every non-empty string casts to $true in PowerShell, so setting it to 0 would have switched the bypass ON. - The bootstrap restated Get-Arch's logic minus its PROCESSOR_ARCHITEW6432 handling, so a 32-bit PowerShell host would have refused. Removed: the asset is arch-independent, there was nothing to branch on. - "Pin a signed \$env:RELEASE_VERSION" rendered as "Pin a signed ," — PowerShell escapes with a backtick, so \$ prints a backslash and expands the variable. Now a check of its own. One test in this tier was vacuous on its first writing: a "stale exit code" assertion built on an absent path, which throws and returns before ever reading $LASTEXITCODE. The mutation caught it — M3 applied and nothing reddened. Replaced with the input that is actually reachable: a PowerShell shim (scoop and chocolatey install cosign.ps1), which `&` dispatches in-process and which sets no $LASTEXITCODE at all. backend#2078 --- .github/workflows/build.yml | 7 + scripts/install.ps1 | 203 +++++++++++-- scripts/tests/install-ps1-functions.tests.ps1 | 287 ++++++++++++++++++ scripts/tests/install-ps1-verify.sh | 100 ++++++ 4 files changed, 578 insertions(+), 19 deletions(-) create mode 100644 scripts/tests/install-ps1-functions.tests.ps1 create mode 100755 scripts/tests/install-ps1-verify.sh 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/scripts/install.ps1 b/scripts/install.ps1 index cc63714..7d7266c 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,111 @@ $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. +try { + [Net.ServicePointManager]::SecurityProtocol = + [Net.ServicePointManager]::SecurityProtocol -bor [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 +245,102 @@ 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 ` + & $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..7d9bd3e --- /dev/null +++ b/scripts/tests/install-ps1-verify.sh @@ -0,0 +1,100 @@ +#!/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. 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 ] From 885c5c364e43ade8cf2ceb63af3c8b0dbe1f14da Mon Sep 17 00:00:00 2001 From: lukasWuttke <54042461+LukasWodka@users.noreply.github.com> Date: Tue, 18 Aug 2026 12:24:07 +0200 Subject: [PATCH 2/6] feat(telemetry): one outcome event per command, with no channel for a path (backend#1907) (#527) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The CLI emits nothing today, so the backend#736 class — the binary landing on a PATH the shell does not read, `cluster info` reading a kubeconfig context nobody meant — is only ever visible when a customer mentions it. This wires the #1897 helper to a single terminal event per invocation: command, duration, exit code, OS/arch, version, error class. The ticket's "no arguments, no paths, no data" is built as a structure rather than a rule, because a rule is a thing every future call site has to remember: * the command is a LOOKUP into the set of paths enumerated from the live cobra tree, so a value that is not a command the CLI dispatches cannot be emitted at all — it reports `unregistered`, which stays countable; * the error class is keyed on an INT, the CLI's own frozen exit-code contract. The classifier is never handed an error message, so there is nothing for a path or a cell value to travel down; * everything else is an int. There is no redaction regex anywhere in the change. A sanitiser has to anticipate what it strips; a closed set only admits what was enumerated. os.type / host.arch go in the RESOURCE layer under OpenTelemetry's own names (§1.1 forbids re-inventing them as tracebloc.os): they are compile-time constants of the binary, so they describe the process, not the occurrence, and adding them to resourceScope means a call site still cannot set them. The guard is derived, not restated. TestEveryEmittedStringComesFromAClosedSet 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 — so a free-text channel fails it whether or not anyone thought to forbid the thing travelling down it. Thirteen mutations were run against it; each reddened, and each anchor was asserted to have applied. One of them (smuggling the raw command into a second attribute) was caught only by the telemetry-side test and NOT by the cli-side one, which was inspecting a single key — that test now sweeps the whole payload. WHAT IS NOT CONNECTED. The transport. The ticket says "rides the gateway and token"; the 17 Aug decision (rfcs#28) replaced the Collector gateway with an ingest endpoint on the backend, which is backend#1905 and does not exist yet. pendingSink() returns nil, so every event is validated and dropped. Validation runs regardless, so a malformed event fails in CI wherever the binary was built, and connecting #1905 is one function body. Opt-out (default on) via TRACEBLOC_NO_TELEMETRY or DO_NOT_TRACK, documented in docs/troubleshooting.md — and the document's claim about which variables work is itself a test, because a user who exports a stale name believes they have opted out and nothing else would ever tell them. Co-authored-by: Claude Opus 5 --- cmd/tracebloc/main.go | 17 +- docs/troubleshooting.md | 28 ++ internal/cli/telemetry.go | 173 +++++++++++++ internal/cli/telemetry_test.go | 398 +++++++++++++++++++++++++++++ internal/telemetry/outcome.go | 199 +++++++++++++++ internal/telemetry/outcome_test.go | 365 ++++++++++++++++++++++++++ internal/telemetry/telemetry.go | 15 ++ scripts/coverage-floor.sh | 7 + 8 files changed, 1200 insertions(+), 2 deletions(-) create mode 100644 internal/cli/telemetry.go create mode 100644 internal/cli/telemetry_test.go create mode 100644 internal/telemetry/outcome.go create mode 100644 internal/telemetry/outcome_test.go 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..67c5964 --- /dev/null +++ b/internal/cli/telemetry.go @@ -0,0 +1,173 @@ +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. +// +// The signed-in environment wins because it is the backend these records are +// about; $CLIENT_ENV and the prod default are api.ResolveEnv's existing answer, +// reused rather than restated. An unrecognised value is not repaired here — the +// emitter refuses to export under a guessed environment (§3.2), and that +// refusal belongs in one place. +func telemetryEnv(signedInEnv string) string { + if api.IsKnownEnv(signedInEnv) { + return strings.ToLower(signedInEnv) + } + return api.ResolveEnv("") +} + +// 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..a376603 --- /dev/null +++ b/internal/cli/telemetry_test.go @@ -0,0 +1,398 @@ +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 TestTheEnvironmentIsNeverGuessed(t *testing.T) { + // §3.2 — an unrecognised environment must not export under a repaired or + // guessed value. `staging` is the classic near miss: it is the git branch + // name, and `stg` is the environment value. + for _, tc := range []struct { + signedIn string + want string + }{ + {api.EnvDev, api.EnvDev}, + {api.EnvStg, api.EnvStg}, + {"PROD", api.EnvProd}, + {"staging", api.EnvProd}, // not repaired to stg — falls back to the default + {"", api.EnvProd}, + } { + 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 TestAnUnknownEnvironmentDeliversNothing(t *testing.T) { + // The end-to-end consequence: the emitter refuses to export under a value no + // query filters on, and the wiring must not have talked it out of that. + isolateConfig(t) + t.Setenv("CLIENT_ENV", "staging") + root := NewRootCmd(testBuildInfo()) + if _, _, ok := captureOutcome(t, root, root, 0, nil); ok { + t.Fatal("delivered a record under an unrecognised environment") + } +} + +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) + } +} + +// --- 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 From df1daa9314e0f6dd5838f2f8fc0a640dc054f7f0 Mon Sep 17 00:00:00 2001 From: lukasWuttke <54042461+LukasWodka@users.noreply.github.com> Date: Tue, 18 Aug 2026 16:42:00 +0200 Subject: [PATCH 3/6] fix(install.ps1): verify-blob must not inherit a stale exit code (#529) * fix(install.ps1): verify-blob must not inherit a stale exit code `$LASTEXITCODE` persists from the previous command. `Test-CosignRuns` runs `cosign version` immediately before the verification, and presets 255 precisely so a binary that never starts cannot leave a stale 0 behind. The `verify-blob` call that actually GATES the install had no such preset. So a cosign shim that exits 0 on `version` and then no-ops on `verify-blob` leaves `$LASTEXITCODE` at 0, the `-ne 0` gate reads that as success, and the installer prints "cosign signature valid" and installs a binary nothing verified. That is RFC-0001 R8 defeated by a stale variable, one line from the guard that exists. Reproduced before fixing, driving the real block extracted from install.ps1 under pwsh with a no-op verifier: with fix -> REFUSED (LASTEXITCODE=255) without fix -> INSTALLED-UNVERIFIED (LASTEXITCODE=0) The regression assertion is a SOURCE check, and the limitation is stated rather than hidden: install.ps1 has NO behavioural coverage -- there is no pwsh or Pester anywhere in this repo's CI, which is why a signature gate that does not gate reached a promotion PR. Case 19 asserts the preset sits inside the `$sigDownloaded` block and before the invocation, by line number, so a preset elsewhere cannot satisfy it. It closes this hole; it does not make the Windows installer tested. Worth its own ticket. Mutation-proved: removing the preset reddens case 19 with the message naming the consequence. 34 passed / 0 failed; the mutation gives 33/1. One self-inflicted detail recorded because it is the repo's own failure class: the first version of the check grepped for `verify-blob` and matched the explanatory comment written directly above the call, so it failed on correct code. Anchored on the `& $cosign` invocation instead -- prose is not wiring. Found by Bugbot on release-train promotion PR cli#528 (High). Co-Authored-By: Claude Opus 5 * fix(install-verify): match the STATEMENT, not the text (shujaatTracebloc, #529) Both review findings were right, and both are the failure this PR is about -- left open on the side the PR did not anchor. A COMMENTED-OUT PRESET SATISFIED CASE 19. The check matched the substring `$global:LASTEXITCODE = 255` anywhere on a line, so `# $global:LASTEXITCODE = 255` passed it: the gate dead, the suite green. That is the likelier human mutation -- commenting the line out while debugging the installer -- and it was exactly the one not covered. The PR proved the DELETE mutation and missed this one. THE MIRROR IMAGE, ON THE SAME LINE. Hard-coded single spaces meant `$global:LASTEXITCODE=255` -- correct, equivalent PowerShell -- turned case 19 RED on a working gate, with a message asserting the installer would install unverified. A false alarm that names a supply-chain failure is worse than none. Anchoring the whole statement start-to-end, with flexible spacing, closes both. Suggestion taken as written from the review. AND THE `$` IS ESCAPED in the `if ($sigDownloaded)` grep, matching the two sibling patterns in the same 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 fails on correct code. Verified all three directions on this branch, reproducing the reviewer's results first: commented-out preset before 34/0 (escaped) -> after 33/1 (caught) no-space variant before 33/1 (false) -> after 34/0 (correct) preset deleted before 33/1 -> after 33/1 (still caught) Co-Authored-By: Claude Opus 5 --------- Co-authored-by: Claude Opus 5 --- scripts/install.ps1 | 14 +++++++++ scripts/tests/install-verify.sh | 51 +++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+) diff --git a/scripts/install.ps1 b/scripts/install.ps1 index 7d7266c..0ca0cf2 100644 --- a/scripts/install.ps1 +++ b/scripts/install.ps1 @@ -327,6 +327,20 @@ try { } if ($sigDownloaded) { + # 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' ` 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 ] From d18a520add7a418a8a809d80f704052b916c61b5 Mon Sep 17 00:00:00 2001 From: lukasWuttke <54042461+LukasWodka@users.noreply.github.com> Date: Tue, 18 Aug 2026 18:25:38 +0200 Subject: [PATCH 4/6] feat(2036): add-to-kanban authenticates as the App (backend#2036) (#525) The last board writer on PROJECTS_KANBAN_TOKEN. A per-repo COPY, so one PR per repo; the content stays byte-identical across the fleet because the guard compares it that way. `owner:` makes the installation token ORG-scoped -- 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; without that, Dependabot PRs would stop reaching the board with `Input required and not supplied` -- the exact failure PROJECTS_KANBAN_TOKEN already had to be dual-scoped to avoid. Refs backend#2036 Co-authored-by: Claude Opus 5 --- .github/workflows/add-to-kanban.yml | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) 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 }} + From 7f1fc286f3d6c563d4a1659af53f6dfd3fdb7e01 Mon Sep 17 00:00:00 2001 From: lukasWuttke <54042461+LukasWodka@users.noreply.github.com> Date: Wed, 19 Aug 2026 11:14:25 +0200 Subject: [PATCH 5/6] fix(telemetry): don't file a run signed into an unknown env under prod (#531) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(telemetry): do not file a run signed into an unknown env under prod telemetryEnv repaired a present-but-unrecognised signed-in environment through api.ResolveEnv(""), which returns prod when $CLIENT_ENV is unset. New() then saw a known env and exported — filing a run signed into an unknown backend under prod, the exact guess §3.2 forbids and this function's own doc disclaims. Distinguish the two cases: empty (not signed in) still resolves via $CLIENT_ENV then the prod default; a present-but-unknown value is passed through unchanged so New() disables export. Correct TestTheEnvironmentIsNeverGuessed, which asserted the buggy prod answer for a signed-in "staging", and add an end-to-end regression (TestASignedInUnknownEnvironmentDeliversNothing). Bugbot (Medium), cli#528 staging mirror. Co-Authored-By: Claude Opus 5 * fix(telemetry): label the record with the backend the client actually uses Reversing the direction of the first commit, per @saadqbal's review. The premise there — an unknown signed-in env is an unknown backend, so withhold — does not hold: sessionEnv (client.go) hands cfg.CurrentEnv to api.New verbatim and api.BaseURL routes every unrecognised value to prod. So a run signed into an unknown env genuinely hits prod, prod is the ACCURATE label, and withholding drops exactly the failed-install-on-prod runs this feature exists to see. telemetryEnv now mirrors api.BaseURL: resolve (CurrentEnv, else $CLIENT_ENV/prod), then known -> itself, unknown -> prod. The real bug it fixes is the old code reading $CLIENT_ENV for a signed-in env while the client ignores it — filing a run under 'dev' while every request went to prod. Rename the param env -> drop the shadow of signedInEnv(). Tests flip from 'delivers nothing' to 'labelled prod', plus TestASignedInUnknownEnvIgnoresClientEnv pinning the divergence (fails against the old code). Root cause — BaseURL silently routing unknown envs to prod — filed separately. Co-Authored-By: Claude Opus 5 --------- Co-authored-by: Claude Opus 5 --- internal/cli/telemetry.go | 38 +++++++++++++----- internal/cli/telemetry_test.go | 72 ++++++++++++++++++++++++++++------ 2 files changed, 90 insertions(+), 20 deletions(-) diff --git a/internal/cli/telemetry.go b/internal/cli/telemetry.go index 67c5964..df2ff2e 100644 --- a/internal/cli/telemetry.go +++ b/internal/cli/telemetry.go @@ -91,16 +91,36 @@ func commandPathOf(c *cobra.Command) string { // telemetryEnv picks deployment.environment for the records. // -// The signed-in environment wins because it is the backend these records are -// about; $CLIENT_ENV and the prod default are api.ResolveEnv's existing answer, -// reused rather than restated. An unrecognised value is not repaired here — the -// emitter refuses to export under a guessed environment (§3.2), and that -// refusal belongs in one place. -func telemetryEnv(signedInEnv string) string { - if api.IsKnownEnv(signedInEnv) { - return strings.ToLower(signedInEnv) +// 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) } - return api.ResolveEnv("") + // 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 diff --git a/internal/cli/telemetry_test.go b/internal/cli/telemetry_test.go index a376603..61de040 100644 --- a/internal/cli/telemetry_test.go +++ b/internal/cli/telemetry_test.go @@ -271,10 +271,12 @@ func TestTheOffSpellingsDoNotOptOut(t *testing.T) { // --- environment --------------------------------------------------------------- -func TestTheEnvironmentIsNeverGuessed(t *testing.T) { - // §3.2 — an unrecognised environment must not export under a repaired or - // guessed value. `staging` is the classic near miss: it is the git branch - // name, and `stg` is the environment value. +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 @@ -282,8 +284,8 @@ func TestTheEnvironmentIsNeverGuessed(t *testing.T) { {api.EnvDev, api.EnvDev}, {api.EnvStg, api.EnvStg}, {"PROD", api.EnvProd}, - {"staging", api.EnvProd}, // not repaired to stg — falls back to the default - {"", 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", "") @@ -294,14 +296,34 @@ func TestTheEnvironmentIsNeverGuessed(t *testing.T) { } } -func TestAnUnknownEnvironmentDeliversNothing(t *testing.T) { - // The end-to-end consequence: the emitter refuses to export under a value no - // query filters on, and the wiring must not have talked it out of that. +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()) - if _, _, ok := captureOutcome(t, root, root, 0, nil); ok { - t.Fatal("delivered a record under an unrecognised environment") + 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) } } @@ -332,6 +354,34 @@ func TestTheSignedInEnvironmentWins(t *testing.T) { } } +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) { From d44350951736418659a8baf4e8f98c2be51b3517 Mon Sep 17 00:00:00 2001 From: lukasWuttke <54042461+LukasWodka@users.noreply.github.com> Date: Wed, 19 Aug 2026 14:25:34 +0200 Subject: [PATCH 6/6] fix(install.ps1): SET the TLS 1.2 floor, don't OR it onto the default (#532) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(install.ps1): SET the TLS 1.2 floor, don't OR it onto the default The floor bitwise-OR-ed Tls12 onto [Net.ServicePointManager]::SecurityProtocol, which on PowerShell 5.1 already advertises SSL3/TLS1.0/1.1 — so those stay on and a fetch of the binary or the cosign verifier can still negotiate down, the exact downgrade the floor's own comment says it prevents (cli#528 Bugbot, Medium). Assign the protocol to Tls12 (dropping the weak ones), adding Tls13 only where the runtime defines the enum member (absent on older 5.1 hosts, where naming it throws). New install-ps1-verify assertion fails on the OR-onto-default form (mutation-proved); collapses newlines first since the old form spanned two lines. install-ps1-verify 6/6, behavioural tier 22/22. Co-Authored-By: Claude Opus 5 * fix(install.ps1): set only the Tls12 floor, drop the throwing Tls13 add Bugbot (Medium) on the first push: [Enum]::IsDefined([Net.SecurityProtocolType], 'Tls13') is true on .NET 4.8 even where Schannel cannot negotiate TLS 1.3 (Win10 21H1, Server 2019). Assigning Tls12 -bor Tls13 then THROWS, the empty catch swallows it, and SecurityProtocol is never set — so the Tls13 decoration could defeat the very Tls12 floor it was meant to extend. Assign Tls12 alone: it is the floor, always negotiable, and secure for these fetches. Verify assertion now pins the direct Tls12 assignment; still fails on the OR-onto-default form (mutation-proved). 6/6 verify, 22/22 behavioural. Co-Authored-By: Claude Opus 5 --------- Co-authored-by: Claude Opus 5 --- scripts/install.ps1 | 12 ++++++++++-- scripts/tests/install-ps1-verify.sh | 16 +++++++++++++++- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/scripts/install.ps1 b/scripts/install.ps1 index 0ca0cf2..3044df0 100644 --- a/scripts/install.ps1 +++ b/scripts/install.ps1 @@ -64,9 +64,17 @@ $AllowUnverified = ($env:TRACEBLOC_ALLOW_UNVERIFIED -eq '1') # 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.ServicePointManager]::SecurityProtocol -bor [Net.SecurityProtocolType]::Tls12 + [Net.ServicePointManager]::SecurityProtocol = [Net.SecurityProtocolType]::Tls12 } catch { } # --------------------------------------------------------------------- diff --git a/scripts/tests/install-ps1-verify.sh b/scripts/tests/install-ps1-verify.sh index 7d9bd3e..3bbb674 100755 --- a/scripts/tests/install-ps1-verify.sh +++ b/scripts/tests/install-ps1-verify.sh @@ -77,7 +77,21 @@ else bad 'the cosign bootstrap asks for an asset sigstore does not publish' fi -# ── 5. behavioural tier ───────────────────────────────────────────────────── +# ── 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