Uh oh!
There was an error while loading. Please reload this page.
fix(installer): make the curl TLS floor structural, not per-call-site (backend#1252) - #400
Conversation
LukasWodka
commented
Jul 25, 2026
bugbot run |
Uh oh!
There was an error while loading. Please reload this page.
LukasWodka
commented
Jul 25, 2026
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit f337c15. Configure here.
LukasWodka
commented
Jul 25, 2026
Merge-order note:#401 is stacked on this branch (it fixes an unrelated No action needed from the reviewer beyond that — because |
… (backend#1252) `CURL_SECURE` was a bare constant every call site had to splice in by hand, so call sites kept losing it: seven live `curl` invocations ran with no minimum TLS version, including the POST in `verify_credentials()` that carries the client's password. These installs run on customer-managed hosts and behind TLS-inspecting proxies, which negotiate down to whatever the client permits — the reason this repo adopted an explicit floor instead of trusting curl's defaults. Add `curl_secure()` in `scripts/lib/common.sh` and route every fetch in `scripts/lib/*.sh` through it (18 call sites). The wrapper always passes `--tlsv1.2` and supplies default `--connect-timeout 30` / `--max-time 300`. Defaults are injected before `"$@"`, so a call site that wants a tighter bound still wins (curl honours the last occurrence), and a transfer that bounds itself with `--speed-limit`/`--speed-time` gets no injected `--max-time` — a hard deadline would fail a slow-but-healthy link on a large binary download. Every existing site therefore keeps its effective behaviour; seven gain the floor and nine previously unbounded ones gain a deadline. Also fixed while here: - `gpu-amd.sh` had the least-bounded curl usage in the repo — no TLS floor, no timeout, no retry. Both calls now go through the wrapper; the `.deb` download is retry-wrapped. The listing scrape deliberately is not: `retry()` reports attempts on stdout, which is that function's return value. - `install-k8s.ps1`'s WSL2 here-string had the same two nvidia-container-toolkit fetches bare. It cannot source `common.sh`, so it spells the flags out inline the way the bootstrap does. `scripts/install.sh` keeps its seven hardcoded literals: it is the trust root that fetches `common.sh`, so it cannot source the wrapper. `CURL_SECURE` stays defined and unchanged for out-of-tree callers, but nothing in the repo reads it now — the wrapper names the flag itself, so the constant can never silently reshape every fetch in the installer. Enforcement: an INTERIM third check in `scripts/check-style.sh` fails on a bare `curl`. tracebloc/.github#65 already implements this properly (a shell-aware lexer, not a grep) in a shared reusable workflow, but that workflow is not on `main` yet and cannot be referenced from here until it is. The check is marked for retirement the moment this repo adds that caller. Regenerated `scripts/manifest.sha256` (R8 supply-chain gate). Found in #399. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ne (Bugbot) `_fetch_kubectl` had no time bound at all, so routing it through `curl_secure` handed it the wrapper's default `--max-time 300`. kubectl is a ~50 MB binary, and this repo already documents (at `_fetch_k3d_release`, same file) that a fixed ceiling fails a slow-but-healthy link at that size — so the wrapper would have made every retry fail where the fetch previously completed. Give both fetches the same `--connect-timeout 15 --speed-limit 1024 --speed-time 60` as the k3d pair. That is also how `curl_secure` knows to skip its default deadline, and it is strictly better than before: the fetch was previously unbounded in both directions, so a mid-stream stall hung the step indefinitely. Audited the other 7 sites that now inherit the 300s default — get.docker.com, get-helm-3, the Homebrew script, stable.txt, the DMG checksum, the device-plugin manifest and the amdgpu-install package are all small text/script payloads. The only large downloads in the repo are kubectl, k3d and the Docker Desktop DMG; the latter two were already stall-bounded. Adds a bats test pinning it, since nothing covered `_fetch_kubectl` before. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
f337c15 to
0cefb9cCompareLukasWodka
commented
Jul 27, 2026
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 0cefb9c. Configure here.
Uh oh!
There was an error while loading. Please reload this page.
Resolves#400 (curl_secure) + #397 conflicts: - install_helm keeps #396's verified direct get.helm.sh download; drops develop's get-helm-3 hunk (that is what #396 removes). - Adopts #400's curl_secure() wrapper on #396's new curl calls (_fetch_helm_release + latest-version lookup); no per-call-site $CURL_SECURE remains. - Regenerated manifest.sha256 (idempotent) + copy-catalog golden. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Retargeting the PR base from #400's merged branch to develop doesn't fire a pull_request event, so standard-checks (Unit tests + Lint) never ran on this head. Empty commit fires synchronize so the required checks run against develop. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ler (#401) * fix(installer): make the curl TLS floor structural, not per-call-site (backend#1252) `CURL_SECURE` was a bare constant every call site had to splice in by hand, so call sites kept losing it: seven live `curl` invocations ran with no minimum TLS version, including the POST in `verify_credentials()` that carries the client's password. These installs run on customer-managed hosts and behind TLS-inspecting proxies, which negotiate down to whatever the client permits — the reason this repo adopted an explicit floor instead of trusting curl's defaults. Add `curl_secure()` in `scripts/lib/common.sh` and route every fetch in `scripts/lib/*.sh` through it (18 call sites). The wrapper always passes `--tlsv1.2` and supplies default `--connect-timeout 30` / `--max-time 300`. Defaults are injected before `"$@"`, so a call site that wants a tighter bound still wins (curl honours the last occurrence), and a transfer that bounds itself with `--speed-limit`/`--speed-time` gets no injected `--max-time` — a hard deadline would fail a slow-but-healthy link on a large binary download. Every existing site therefore keeps its effective behaviour; seven gain the floor and nine previously unbounded ones gain a deadline. Also fixed while here: - `gpu-amd.sh` had the least-bounded curl usage in the repo — no TLS floor, no timeout, no retry. Both calls now go through the wrapper; the `.deb` download is retry-wrapped. The listing scrape deliberately is not: `retry()` reports attempts on stdout, which is that function's return value. - `install-k8s.ps1`'s WSL2 here-string had the same two nvidia-container-toolkit fetches bare. It cannot source `common.sh`, so it spells the flags out inline the way the bootstrap does. `scripts/install.sh` keeps its seven hardcoded literals: it is the trust root that fetches `common.sh`, so it cannot source the wrapper. `CURL_SECURE` stays defined and unchanged for out-of-tree callers, but nothing in the repo reads it now — the wrapper names the flag itself, so the constant can never silently reshape every fetch in the installer. Enforcement: an INTERIM third check in `scripts/check-style.sh` fails on a bare `curl`. tracebloc/.github#65 already implements this properly (a shell-aware lexer, not a grep) in a shared reusable workflow, but that workflow is not on `main` yet and cannot be referenced from here until it is. The check is marked for retirement the moment this repo adds that caller. Regenerated `scripts/manifest.sha256` (R8 supply-chain gate). Found in #399. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(installer): stall-bound the kubectl fetch, don't give it a deadline (Bugbot) `_fetch_kubectl` had no time bound at all, so routing it through `curl_secure` handed it the wrapper's default `--max-time 300`. kubectl is a ~50 MB binary, and this repo already documents (at `_fetch_k3d_release`, same file) that a fixed ceiling fails a slow-but-healthy link at that size — so the wrapper would have made every retry fail where the fetch previously completed. Give both fetches the same `--connect-timeout 15 --speed-limit 1024 --speed-time 60` as the k3d pair. That is also how `curl_secure` knows to skip its default deadline, and it is strictly better than before: the fetch was previously unbounded in both directions, so a mid-stream stall hung the step indefinitely. Audited the other 7 sites that now inherit the 300s default — get.docker.com, get-helm-3, the Homebrew script, stable.txt, the DMG checksum, the device-plugin manifest and the amdgpu-install package are all small text/script payloads. The only large downloads in the repo are kubectl, k3d and the Docker Desktop DMG; the latter two were already stall-bounded. Adds a bats test pinning it, since nothing covered `_fetch_kubectl` before. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(installer): stop the ROCm package lookup from aborting the installer `_find_package_name` ran `curl … | grep … | head -1` as a single pipeline and returned its status. `install-k8s.sh` sources this lib under `set -euo pipefail`, so that pipeline could kill the installer two different ways: 1. A failed fetch (404, timeout, proxy block) made the command substitution non-zero, the caller's assignment inherited it, and `set -e` aborted BEFORE the friendly `[[ -z "$name" ]] && error "No amdgpu-install …"` on the next line could run. The user got a silent abort mid-GPU-step instead of an actionable message, and the RHEL major-version fallback was unreachable for the same reason. 2. `head -1` can close the pipe while grep is still writing, so grep takes SIGPIPE (141) and `pipefail` propagates that as a pipeline failure even though a filename WAS found. It only triggers when the directory index exceeds the pipe buffer, so it fails on large mirrors only. Capture the fetch and the match separately and let neither fail the function, then take the first match with `${var%%…}` so `head` leaves the pipeline entirely. The contract is unchanged — filename on stdout, nothing when not found — so emptiness remains the single signal all three callers already test. Adds scripts/tests/gpu-amd.bats (first coverage for this lib): the contract, both hazards, and a caller-shaped regression test under `set -euo pipefail`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: re-trigger standard-checks after base retarget to develop (#401) Retargeting the PR base from #400's merged branch to develop doesn't fire a pull_request event, so standard-checks (Unit tests + Lint) never ran on this head. Empty commit fires synchronize so the required checks run against develop. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
Summary
The installer's TLS floor was opt-in per call site:
CURL_SECUREwas a bare constant that everycurlinvocation had to splice in by hand, and seven live invocations had silently lost it — including the POST inverify_credentials()that carries the client's password. This replaces the constant with acurl_secure()wrapper inscripts/lib/common.shand routes every fetch inscripts/lib/*.shthrough it, so the floor is structural rather than remembered.Practical exploitability is low (modern curl/OpenSSL won't negotiate TLS 1.0/1.1 by default), but this installer targets customer-managed hosts, older distros, and TLS-inspecting corporate proxies that negotiate down to whatever the client permits — which is exactly why the repo adopted an explicit floor instead of trusting defaults. This was a deviation from our own stated control, on a path that carries a credential.
Related
Fixes tracebloc/backend#1252
Found in #399
Enforcement depends on tracebloc/.github#65 (see below)
Type of change
The wrapper
Four decisions worth reviewing:
Defaults are injected before
"$@". curl honours the last occurrence of a repeated option, so a call site that wants a tighter bound still wins (-m 15on the HEAD probe,-m 60on the credential POST,--max-time 120on the CLI download). No existing site loses its own tuning.A stall-bounded transfer gets no
--max-time.download_with_progress()and the k3d/kubectl binary fetches deliberately bound themselves with--speed-limit/--speed-timeinstead of a deadline, because a hard cap fails a slow-but-healthy link on a large download (the Docker Desktop DMG; and see the existing comment at_fetch_k3d_release). The wrapper detects those flags and adds a floor without ever adding a ceiling — so no already-correct call site changes its effective behaviour. Nine previously unbounded ones gain a deadline.Plain
curl, notcommand curl. The bats suite mocks transfers by defining acurlshell function;commandwould bypass every mock and dial the real network. Pinned by a test.CURL_SECUREstays defined, unchanged, for any out-of-tree caller — but nothing in the repo reads it now. The wrapper names--tlsv1.2itself, so growing the constant can never silently reshape every fetch in the installer. It also must stay a single flag: two call sites had quoted it ("$CURL_SECURE"), where a space-separated value collapses into one argv element that curl rejects. Both are now gone.Converted: 18 call sites in
scripts/lib/*.shSeven were the defect; eleven were already correct and were converted anyway, to leave one idiom (so copying a neighbouring line is always right) and to remove the two quoted-constant traps.
install-client-helm.sh:269-m 60common.sh:326,:347-m 15; download keeps stall detection)gpu-amd.sh:29,:52--max-time 30on the scrape;.debdownload nowretry 3 5gpu-nvidia.sh:92,:99--max-time 30gpu-nvidia.sh:80--max-time 30gpu-plugins.sh:26install-cli.sh:237--max-time 120preflight.sh:61--max-time 8setup-linux.sh:320,:322setup-linux.sh×4setup-linux.sh:363,:365,:409setup-macos.sh:11,:180Neither
gpu-amd.sh:29norgpu-nvidia.sh:92/99can be retry-wrapped:retry()reports its attempts on stdout, which in those three cases is the value being captured or the file content being written. Noted in comments at both sites.scripts/install.shkeeps its seven hardcoded literals — it is the trust root that fetchescommon.sh, so it cannot source the wrapper. Unchanged, as intended.Not on the ticket: the same defect on the Windows path
scripts/install-k8s.ps1:568,570runs a bash here-string inside WSL2 containing the same two nvidia-container-toolkit fetches, also with no TLS floor and no timeout. It can't sourcecommon.sh, so the flags are spelled out inline the way the bootstrap spells them out. That makes nine fixed sites, not seven.Enforcement — interim, and deliberately so
scripts/check-style.shgains a third check that fails on a barecurl. It is explicitly marked interim in the code, with the retirement condition named: tracebloc/.github#65 already implements these rules properly (curl-tls+curl-timeout, via a quote-aware, heredoc-aware lexer rather than a grep) in a shared reusable workflow, and reports 20 findings against this repo today. That workflow is not onmainyet, so no repo can reference it — and it shipssoft-fail: trueby default, so even once adopted it starts non-blocking. Waiting would leave the ticket's acceptance criterion unmet for an unknown number of weeks, so this repo gets a blocking grep now and deletes it when the shared caller lands.The grep is narrow:
\bcurl\bmatches the bare command word and notcurl_secure/curl_pid/nocurl, and it exempts lines naming--tlsv1.2(the two trust-root files), comments,has curl/command -v curlpresence tests, and thecurl … | shone-liner we print for users to copy. One user-facing copy line inpreflight.shcarries the existing# style-guard: allowopt-out. The timeout half of the rule needs no grep —curl_securesupplies bounds to everything that can source it.Verified both ways: clean on this branch, and exits 1 with the offending file:line when a bare
curlis planted in a lib.Bugbot round 1 — one real regression, fixed
_fetch_kubectlhad no time bound of its own, so routing it through the wrapper handed it the default--max-time 300. kubectl is a ~50 MB binary, and this repo already documents (at_fetch_k3d_release, same file) that a fixed ceiling fails a slow-but-healthy link at that size — the wrapper would have made every retry fail where the fetch previously completed. Caught by a learned rule; a fair catch that the "no already-correct site changes behaviour" claim above missed, because this site was not already correct.Fixed by giving both kubectl fetches the same
--connect-timeout 15 --speed-limit 1024 --speed-time 60as the k3d pair — which is also howcurl_secureknows to skip its default deadline. Net result is strictly better thandevelop: the fetch was previously unbounded in both directions, so a mid-stream stall hung the step indefinitely.Then audited the remaining seven sites that inherit the 300s default —
get.docker.com,get-helm-3, the Homebrew script,stable.txt, the DMG checksum, the GPU device-plugin manifest, and theamdgpu-installpackage. All small text/script payloads. The only large downloads in the repo are kubectl, k3d and the Docker Desktop DMG; the latter two were already stall-bounded. Added a bats test pinning the kubectl bound, since nothing covered_fetch_kubectlbefore.Test plan
Local, macOS (bash 3.2 + bash 5, shellcheck 0.11.0, bats 1.13.0):
bats scripts/tests/*.bats— 421/424, up from 413/416 (8 new tests, same 3 pre-existing failures)developand environmental —validate_configtmpdir shape, the un-stampedDEFAULT_REFplaceholder, and a BSD-vs-GNU quote-escape difference. None touch curl.shellcheck --severity=error— clean (the gating severity)shellcheck --severity=warning— identical finding set todevelop, only line numbers shifted. No new warnings.bash -non every*.sh— parsesbash scripts/check-style.sh— clean; and exits 1 on a planted barecurlscripts/gen-manifest.sh --check— up to date (manifest regenerated, R8)install-k8s.ps1parses clean; PSScriptAnalyzer 0 errors, 154 warnings — identical to baseline/bin/bash3.2 withset -euo pipefail(macOS is a supported target): argv shape correct for the plain / stall-bounded / tighter-deadline / no-args cases, exit status propagates200, stall-bounded download, HEAD probeNew tests (
scripts/tests/common.bats) pin the contract: floor always present, caller args preserved in order, defaults supplied, a caller's own deadline lands after the default, a stall-bounded call gets no--max-time, env overrides, dispatch through a mockablecurl,CURL_SECUREstill defined.CI on the first commit
Green across the board —
Static analysis,bats,Unit tests,Lint,Pester(ubuntu + windows), all 9Prereqsdistros, 3E2E clusterlegs andE2E auth-proxy. The one red,PATH persist — alpine:3, is an unrelated pre-existing flake: it exercises the cli repo's releasedinstall.sh(✖ bash install.sh (SHELL=bash) exited 1on Alpine v3.24, while zsh and fish passed on the same run), it has failed the same way on at least one other open branch, and nothing in this diff is on its path beyondcommon.sh's logging helpers — which demonstrably worked, since the job printed its results through them. Re-run requested.Deployment notes
None. No config, no env vars, no behaviour change on a healthy network. Two new optional knobs (
TB_CURL_CONNECT_TIMEOUT,TB_CURL_MAX_TIME) exist for hosts behind unusually slow proxies; both default to the values the bootstrap already hardcodes.Checklist
common.shand the interim check documents its own retirementbash scripts/check-style.shpasses (and gained a check)Note
Medium Risk
Touches credential verification and all outbound HTTPS in the installer; behavior is mostly additive (TLS floor, timeouts) with careful preservation of stall-bounded large downloads.
Overview
Introduces
curl_secure()incommon.shso installer fetches always use--tlsv1.2plus default connect/max timeouts, while call sites can still tighten deadlines or use stall detection without inheriting a hard--max-timeon large binaries.Routes essentially all
scripts/lib/*.shnetwork pulls through the wrapper (including the credential POST ininstall-client-helm.sh), adds retry on the AMDGPU.debdownload, gives_fetch_kubectlstall bounds so it does not pick up a 300s cap, and spells out the same TLS/timeout flags in the WSL2 NVIDIA toolkit snippet ininstall-k8s.ps1.Adds an interim
check-style.shrule against barecurl, bats coverage for the wrapper contract and kubectl fetch bounds, and updatesmanifest.sha256.Reviewed by Cursor Bugbot for commit 0cefb9c. Bugbot is set up for automated code reviews on this repo. Configure here.