feat(installer): self-serve Helm prerequisites — verified direct download, no get-helm-3 - #396
Conversation
…load, no get-helm-3 (#395) The installer now takes care of its own requirements instead of erroring out and telling the user to install tools: - install_helm fetches the pinned Helm release directly from get.helm.sh and verifies it against the published .sha256sum (fail-closed), exactly like the k3d direct download (#382). helm's get-helm-3 script is gone — it floats on the mutable helm/helm@main, performs its checksum step with openssl (absent on minimal cloud images, Bugbot #383), and its fetches are unbounded. openssl is no longer needed anywhere in the flow. - HELM_VERSION pin in common.sh (v4.2.3; 'latest' resolves at install time via get.helm.sh/helm-latest-version, same verified path; malformed tags fail closed before any fetch). - _ensure_unpack_tools: when tar/gzip are genuinely missing on the Tier 0 fast path (which skips install_system_deps), install them via the package manager — quietly as root/passwordless sudo, with an honest one-line reason when a password is needed — rather than aborting with 'go install tar'. install_system_deps drops openssl, adds gzip. Supersedes the fatal-error approach in #395 (product call: never tell the user to install tools we can install ourselves). Windows already fetches Helm directly from get.helm.sh, so the ps1 path is untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…Bugbot r1) The A2 sudo shadow runs '-n true' as a command when root, and 'has sudo' is always true because the shadow function exists — use the #372 primitives like preflight_sudo/_probe_privilege do. The root-without- sudo-binary strip is dropped entirely: the shadow already handles root by executing the command directly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…it + one combined install (Bugbot r2) _ensure_unpack_tools ran package installs on the Tier 0 path without the full flow's guards: apt could sit on the dpkg lock invisibly behind the spinner, and a long wait could outlast the just-primed sudo timestamp so the next sudo re-prompts behind the spinner and hangs. Prime, then keep the ticket warm (preflight_sudo's pattern; killed right after the install — the zero-privilege tier shouldn't hold a warm admin ticket), wait out the dpkg lock (bounded + visible), and install everything in ONE package- manager call so there's a single sudo consumer right after priming. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- _ensure_unpack_tools kills only the keepalive IT started: on the Tier
1/2 recovery path SUDO_KEEPALIVE_PID belongs to preflight_sudo, and
killing it would let later privileged steps re-prompt behind a spinner.
The global is only claimed when empty (install_cleanup coverage) and
only cleared when it is ours.
- HELM_VERSION=latest: isolate the endpoint body with tail -1 — retry's
attempt notices go to stdout and a failed-then-successful fetch would
concatenate them into the captured tag, failing the anchored regex with
a false 'couldn't resolve'. (install_k3d's resolver is immune: its
${var##*/} strip discards anything before the redirect URL.)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@cursor review |
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 922c288. Configure here.
divyasinghds
left a comment
divyasinghds
left a comment
There was a problem hiding this comment.
Code LGTM — holding formal approval until CI is green.
The diff itself is sound (reviewed in full):
- Fail-closed on checksum mismatch, double-guarded; tag regex
^v[0-9][A-Za-z0-9._-]*$blocks path traversal before any fetch;latestresolves then re-validates. _ensure_unpack_toolssudo/keepalive handling is well covered (passwordless, prompt, no-rights, no-sudo-binary, never-kill-a-preflight-keepalive).- openssl removal audited: nothing in the install runtime needs it; the remaining use is
scripts/tests/e2e-proxy.sh(a test tool), correctly left in place. - Pinning Helm to v4.2.3 matches what floating
get-helm-3already resolved to, so it's more deterministic, not a behavior change.
Why I'm not clicking Approve yet: the suite is red — PATH persist on all 7 distros, Unit tests, and bats (mocked) fail. But those failures are in the CLI PATH-persistence path and a sudo modprobe overlay (exit 127) case — subsystems this diff doesn't touch — and the same jobs pass green on the sibling client PRs #398 and #381. This branch is diverged from develop (behind by 1 commit) and last ran CI 3 days ago, so this looks like staleness, not a defect here.
Please rebase on develop and push for a fresh run; once it's green I'll approve. (Happy to approve now if you'd rather, but a green run first matches the "don't merge on red" bar.)
Reviewed with Claude Code.
… knob The PR adds a HELM_VERSION env var (common.sh), which surfaces one line in install-k8s.sh --help. emit_install reads --help live, so the golden drifted by exactly that line. Regenerated via TB_UPDATE_GOLDEN=1; verified locally (bats copy-catalog now green).
|
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 24b6f1f. Configure here.
divyasinghds
left a comment
divyasinghds
left a comment
There was a problem hiding this comment.
Approve — CI is now green (29 pass / 0 fail / 0 pending) on the rebased head 24b6f1f0, and the earlier red suite is confirmed to have been staleness: the previously-failing PATH persist / Unit tests / bats jobs all pass now, and none of them touch this diff.
Re-reviewed the rebased diff against my earlier pass: the code in common.sh, setup-linux.sh, and setup-linux.bats is unchanged. The only addition is scripts/testdata/golden/00-install.golden gaining the HELM_VERSION --help line — a benign, welcome fixture update that matches the new help text. The original review stands:
- Fail-closed checksum verify, double-guarded; tag regex blocks path traversal before any fetch;
latestresolves then re-validates. _ensure_unpack_toolssudo/keepalive paths well covered.- openssl removal audited — nothing in the install runtime needs it.
CONFLICTING / behind develop by 2 commits. Please rebase once more to clear the conflict before merging — the green run above was against the pre-conflict head. This approval covers the code; it stands after a clean conflict resolution.
Reviewed with Claude Code.
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>
|
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 8d8deaf. Configure here.
divyasinghds
left a comment
divyasinghds
left a comment
There was a problem hiding this comment.
Re-approve. Prior approval was auto-dismissed by the conflict-resolving push (head 24b6f1f0 → 8d8deafb).
Re-verified the resolved diff against the version I approved: the only code change is curl -fsSL $CURL_SECURE → curl_secure -fsSL in _fetch_helm_release and the latest-version resolution — adopting the curl_secure wrapper that landed on develop (#400), with the manifest hashes updated to match. Same behavior; the security/logic review is unchanged. CI is green (29 pass, 1 skipping) and the branch is now MERGEABLE.
Reviewed with Claude Code.
aptracebloc
left a comment
aptracebloc
left a comment
There was a problem hiding this comment.
Approving — verified the core supply-chain property.
install_helm → _fetch_helm_release replaces the unpinned get-helm-3 (mutable helm@main code run on the host, sometimes with sudo) with a pinned, checksum-verified direct download from get.helm.sh, mirroring the k3d pattern:
- Tag validated before any fetch —
HELM_VERSIONmust match^v[0-9][A-Za-z0-9._-]*$or belatest, else it errors before the tag reaches a URL (no URL injection); resolved-latestis re-validated against the same regex. - Checksum is fail-closed — downloads the tarball +
.sha256sumovercurl_secure -fsSL, thensha256sum --check --quiet; on any mismatch it cleans up anderrors before unpacking or installing. Only thehelmbinary is extracted, and Tier 0 keeps it under$HOMEwith no sudo. - Manifest integrity checks out (both changed scripts match). Bugbot's 4 findings resolved, CI green.
A clear improvement over running unpinned upstream code, and it fixes the minimal-image reliability bug (openssl/tar) by self-serving the prereqs. (Inherent, not a defect: the checksum is same-host, so it guards transit/MITM but not a fully-compromised get.helm.sh — standard for direct-download installers and the same model as k3d.)
LGTM.
— drafted with Claude (Opus 4.8), sent by @aptracebloc
Regenerate manifest.sha256 for the merged installer set (idempotent). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
65ab4ee
… blocks on openssl (Bugbot #396) get-helm-3 is gone (replaced by a sha256-verified direct download), so Helm no longer needs openssl. The Tier-0 preflight still demanded openssl+tar and failed fast, aborting minimal-image installs for a dependency Helm doesn't use — and pre-empting _ensure_unpack_tools, which installs tar/gzip Tier-0-aware. Remove the function and its Tier-0 call; drop the tests that encoded the old openssl preflight (tar/gzip stays covered by _ensure_unpack_tools' own tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
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 5dd5873. Configure here.
Regenerate manifest.sha256 for the merged installer set (idempotent). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
4 participants
What
The Tier 0 fast path skips
install_system_deps, butinstall_helmshelled out to helm'sget-helm-3— which needsopenssl(checksum step) andtar(unpack). On minimal cloud images (Amazon Linux 2023, minimal RHEL) the Helm install died cryptically after we promised a zero-privilege install (Bugbot High on #383, found by @shujaatTracebloc — thank you, the finding is fully real).#395 proposed failing fast with "install openssl and tar, then re-run". Product direction (Lukas): the installer takes care of its own requirements — never tell the user to go install tools. This PR implements that:
opensslis no longer needed at all.install_helmnow downloads the pinned Helm release directly from get.helm.sh and verifies it against the release's published.sha256sumwithsha256sum(coreutils — present everywhere we run), exactly mirroring the k3d direct download from fix(installer): pin the k3d release (K3D_VERSION) — deterministic, rate-limit-proof installs #382. This also removes a real supply-chain wart:get-helm-3floats on the mutablehelm/helm@main(unpinned code executed on the host, sometimes with sudo), and its internal fetches are unbounded.HELM_VERSIONpin in common.sh (defaultv4.2.3— the same releaseget-helm-3resolves today viahelm-latest-version, so no version change in practice; CI's 9-distro matrix exercises it).HELM_VERSION=latestresolves at install time and takes the same verified path; malformed tags fail closed before any fetch.tar/gzipget installed, not demanded. New_ensure_unpack_tools: on the rare Tier 0 host genuinely missing them, install via the package manager — quietly when root / passwordless sudo; with an honest one-liner ("missing tar, needed once to unpack Helm — administrator password required") when a password is needed; an error only as the true last resort (no rights at all), and even that error now says exactly why.install_system_depsdropsopenssl, keepstar, addsgzip(audit: nothing else in the flow uses openssl;taris also used bydiagnose.sh).Windows is untouched —
install-k8s.ps1already fetches Helm directly from get.helm.sh.Verification
setup-linux.bats): 11 new — direct-download happy path / checksum fail-closed /latestresolution / malformed-tag fail-fast / sudo-mv system path / already-present skip, plus 4_ensure_unpack_toolscases (no-op, passwordless install, honest no-rights error, failed-install fatal).helm-v4.2.3-linux-amd64.tar.gz→ 200;.sha256sumbody is<sha256> helm-v4.2.3-linux-amd64.tar.gz, the exactsha256sum --checkformat the code consumes.bash -n,shellcheck --severity=error,scripts/check-style.sh, manifest regenerated.Supersedes #395.
🤖 Generated with Claude Code
Note
Medium Risk
Changes the Linux install path and download/supply-chain behavior for a core tool (Helm), including Tier 0 sudo/package installs, but follows the existing k3d pattern with fail-closed checksums and extensive bats coverage.
Overview
Replaces Linux Helm installation via
get-helm-3with a pinned, checksum-verified tarball fetch fromget.helm.sh(same pattern as k3d in #382). AddsHELM_VERSION(defaultv4.2.3;latestresolves at install time; bad tags fail before download).opensslis dropped as an installer dependency; verification usessha256sum. Full installs still pulltar+gzipviainstall_system_deps; Tier 0 gets_ensure_unpack_toolsto install missing unpack tools through the package manager (with sudo priming / apt lock wait) instead of the removed_ensure_helm_prereqsfail-fast.Help text, golden fixtures,
manifest.sha256, andsetup-linux.batsare updated with broad coverage for direct download, checksum failure, and unpack-tool paths.Reviewed by Cursor Bugbot for commit 5dd5873. Bugbot is set up for automated code reviews on this repo. Configure here.