🚑 fix(ci): route every installer download through a hardened fetch helper - #357
Merged
Conversation
CI-FETCHLIB-01. PR #351's REQUIRED kind-smoke check went red on a bash-only diff with `curl: (7) Failed to connect to get.helm.sh`. CI-HELMDL-01 fixed hack/install-helm.sh and kept its retry helper local by design, leaving seven sibling installers with the same bare `curl -fsSL`. hack/lib/fetch.sh is the single home for the flag list those call sites will share. fetch_to pairs curl's own --retry (a flaky origin that answers: 408/429/5xx) with an outer bash loop (the connect/DNS/TLS/dropped-connection class curl's --retry does NOT cover without --retry-connrefused). It pins the scheme on the request and the redirect chain, bounds the connect and the transfer, discards partial downloads, and fails at the fetch naming the host and curl's exit code rather than falling through. The gate proves each property against a real local HTTPS origin with a self-signed cert -- one that genuinely refuses connections, returns 503 before 200, drops a connection mid-request, redirects https->http and truncates a body -- because a stubbed curl can only prove that certain words appeared on a command line.
…fetch helper CI-FETCHLIB-01. All eight hack/install-*.sh scripts now source hack/lib/fetch.sh and fetch through fetch_to. Seven of them were still issuing the bare `curl -fsSL` that reddened the REQUIRED kind-smoke check on PR #351; install-helm.sh drops its local fetch_with_retry, whose own header named this refactor as the work it could not do. Semantics preserved at every call site. The four installers that resolved a digest with `EXPECTED_SHA256="$(curl ... | awk ...)"` now fetch the checksum body to a file and awk that: the pipe form propagated a dead network under set -e only because it was a BARE assignment, so any later refactor into a function would have silently left the `[[ -z ]]` guard as the only thing catching it. Every existing digest check, and the order that puts it before the install, is untouched. The repo-wide gate enumerates hack/install-*.sh from the filesystem rather than a list, so installer number nine is covered the day it lands, and it records in code that git-cliff and helm-docs verify no digest today -- a gap this lane does not close but refuses to leave undocumented. It is reached in CI through the already-wired ci_install_helm_hardening_test.sh, which now also follows the flag assertions into the shared helper.
CI-KINDLOOP-01. The three-attempt loop in .github/actions/kind-e2e-setup fell through after its last failed attempt onto `chmod +x` of a file that had never been written. It was never a false green -- set -euo pipefail still failed the job -- but an unreachable kind.sigs.k8s.io was reported as "No such file or directory", and the loop burned a pointless 10s sleep after its final attempt on a job already doomed. This is the loop that looks hardened, so it is the one that gets copied; CI-HELMDL-01 declined to imitate it for exactly this reason. Rather than repair it in place, the step now calls fetch_to from hack/lib/fetch.sh, the same helper the installers use, so the kind and helm downloads inside this one step cannot drift apart again -- the drift that left the helm side bare and reddened kind-smoke on PR #351. The helper is sourced via GITHUB_WORKSPACE rather than a relative path; the step already required the checkout, since it runs hack/install-helm.sh. The gate asserts the loop and the sleep are gone rather than merely bypassed: a leftover loop is what a later editor reattaches a body to.
Non-vacuity pass over all 27 mutations. Two assertions were passing for reasons weaker than they advertised. The digest scan counted `EXPECTED_SHA256` alongside `verify_sha256`/`sha256sum`, so an installer that kept the variable and deleted the comparison went green -- a digest variable with no digest check, which is the exact shape the assertion forbids. It now counts the comparison, not the name. Case G claimed to isolate --proto-redir. It does not: curl applies --proto to the redirect chain as well, so deleting --proto-redir alone leaves the downgrade blocked and the case green -- the static flag assertion in Part 2 is what caught that mutant. Only deleting BOTH pins turns the plaintext listener's counter non-zero. The comment now says what the case proves and where --proto-redir's coverage actually lives, rather than advertising coverage it does not have.
…he closing diagnosis Three review findings, all of them assertions that passed for less than they claimed. The direct-fetcher scan led with `(^|[^[:alnum:]_./-])`, which excludes / and - from the character allowed before the command name, so a PATH-QUALIFIED fetcher was invisible to it. A reviewer changed hack/install-polaris.sh's tarball fetch to `/usr/bin/curl -fsSL` and the gate stayed green: the positive assertions still passed because the CHECKSUM fetch in the same file still called fetch_to. That is the PR #351 shape exactly -- one hardened fetch and one bare one beside it -- surviving the assertion whose contract is 'no direct fetcher invocation at all'. The pattern is now defined once, shared by the installer and action scans, and admits an optional path prefix and the `$(command -v curl)` spelling. It is deliberately NOT used for the flag assertions on the helper: fetch_to's own diagnostic contains the words '(curl exit ${status})', which the wider pattern matches, so curl_command_lines stays narrow and gains only the path prefix. Case C counted sleep INVOCATIONS, so KOLLECT_FETCH_RETRY_DELAY=0 -- a backoff that does not back off -- survived the whole suite under a failure message that claimed to forbid it. It now asserts the delay. Deleting fetch_to's closing summary also survived: the per-attempt retry messages already name the host and carry curl's exit code, so nothing pinned the last-attempt diagnosis -- the one thing this lane advertises as the answer to CI-KINDLOOP-01's `chmod: No such file or directory`. Case D now reads the LAST line of stderr and requires it to name the host, carry the exit code, and not promise another attempt. fetch_to's header now records that it takes ownership of its destination path: on any failure it deletes the file, which is strictly more destructive than `curl -f -o`. No caller is exposed today, but that is a property of the callers, not of the helper. 38 mutants, 38 killed, each verified by message text.
…dropped CORRECTING THE RECORD FIRST. The previous commit said '38 mutants, 38 killed' and described its regex change as three additions 'each earning its place'. Both were true and both were misleading. Widening the pattern to catch /usr/bin/curl also NARROWED the leading class from 'any character except alnum/_/./-' to 'whitespace or one of four operators', which dropped backtick, single quote, double quote, = and backslash as opening characters. Five spellings the previous pattern demonstrably KILLED began to survive: \curl, BODY=`curl ...`, bash -c 'curl ...' in an installer, the same shape in the composite action, and bash -c 'wget ...' in install-helm.sh -- two of them on the surfaces that matter most, the CI-KINDLOOP-01 half and the gate already wired at ci.yaml:136. A mutation count is evidence about the mutants you wrote. It is not evidence that nothing regressed. The fix is strictly wider than either previous version and needs no path-prefix group at all: / is itself a non-word character, so restoring the permissive leading class reaches /usr/bin/curl on the slash before the name, while the trailing class rejects curl-config, curl.se and curl_x and accepts the quote, paren and space that really do end a command word. It also fixes a portability defect neither earlier version knew it had. GNU grep 3.12 and ugrep 7.8 disagree about (^|CLASS): under ugrep the anchor alternative swallows the group and the pattern matches NOTHING, not even a line beginning with curl. CI runs GNU grep; a developer machine may not, and there the gate failed GREEN. The input now carries a sentinel space so no ^ alternative is needed, and both engines agree on the whole corpus. The optional path-prefix group had the same split brain (GNU 10/10, ugrep 7/10). fetcher_selftest() runs all 14 bypass spellings and 6 lookalikes on every invocation, so the next person to touch this matcher gets both directions checked without having to remember to re-run an earlier round's mutants. Two review findings alongside: the justifying comment claimed $(dirname "$0")/curl matched when it did not -- it does now, and the quoted "/usr/bin/curl" form is covered too; and logical_lines() now strips trailing comments, conservatively, so a path in a trailing comment cannot read as a fetcher invocation. Case D reads its last stderr line with awk instead of grep|tail, which under pipefail aborted the gate with no diagnostic when stderr was empty -- unreachable precisely in the case the guard was written for. 44 mutants, 44 killed, under GNU grep and ugrep independently. No source file changed in this commit.
…the pattern CORRECTING TWO CLAIMS FROM THE LAST COMMIT FIRST. It said fetcher_selftest makes re-running prior mutants 'a property of the file rather than a claim in a commit message'. That was true of the regex and false of the pipeline that applies it: the harness re-implemented the sentinel with its own printf instead of calling fetcher_lines. Delete the sed from fetcher_lines and every self-test assertion still passed while a bare `curl -fsSL` at column 0 in an installer -- the literal PR #351 defect -- shipped green. Verified against the previous commit: that mutant exits 0 and prints 'All CI-FETCHLIB-01 tests passed'. It is the same failure mode as the round before it, reproduced one layer up, inside the mechanism added to prevent it. The corpus now writes to a real file and calls fetcher_lines, which covers logical_lines too, and its first entry sits at column 0 so the sentinel is load-bearing in the suite that guards it. It also said the grep portability problem 'cost real time'. Overstated. The divergence is real but was observed in this agent's own zsh, where grep is a harness shell function execing a ugrep-compatible binary. No ugrep is on PATH; /usr/bin/grep and /bin/grep are both GNU 3.12, and gate scripts run under bash, so no gate on this machine ever resolved grep to anything but GNU. Both of the previous commit's 'two engine' sweeps were GNU grep twice over. The construct is still worth dropping -- POSIX leaves ^ undefined outside the leading position -- but the honest argument for the sentinel form is that it matches 14/14 of the corpus where v1 managed 6/14 and v2 7/14, not that it fixed a live bug. The comment now says only that, notes that the divergence is specific to alternating ^ with a NEGATED bracket expression, and no longer condemns the (^|[[:space:]]) form this same file still uses safely. The helm gate's twin scan inlined the same sentinel with no self-test at all; it now has OTHER_FETCHER_RE, other_fetcher_lines and its own corpus, column-0 entry included. Also: the corpus sampled neither aria2c nor the optional 3 in python3?, so narrowing the alternation to drop either passed every assertion -- both are covered now, with env and eval spellings alongside. And the KNOWN PERMISSIVE note gains the second false positive it was missing: the trailing-comment strip skips quoted lines, so a quoted line with a trailing comment naming a path that ends in a fetcher reds this gate. 52 mutants, 52 killed, including the neutered-sentinel escape in isolation and combined with the column-0 curl. No source file changed.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
A bare
curl -fsSLinhack/install-helm.shtook the requiredkind-smokecheck red onPR #351:
A transient failure to reach a release host is not a defect in this repo, but with an unhardened
fetch it is indistinguishable from one: the required check goes red, the PR is blocked, and the
only remedy is a blind re-run.
Why a curl-level retry alone is not enough
curl --retry Ndoes not cover a failed connect. Exit 7 (Failed to connect) — and theexit-52 empty-reply class — are outside the set
--retryacts on unless--retry-connrefusedor
--retry-all-errorsis also passed. The exact failure that reddened #351 is therefore theone a naive
--retrysilently ignores.This lane keeps both layers on purpose:
--retryabsorbs transient HTTP 5xx responses without process churn, andclasses curl's retry declines to handle.
Each layer covers what the other does not; neither is redundant.
What
hack/lib/fetch.sh: https pinned end to end (anhttp://URL is refusedbefore a request is issued, and a redirect to plaintext is refused), a TLS floor, bounded
connect and transfer timeouts,
--retry, and an outer bounded-attempt loop that names thehost and curl's exit code on give-up. A failed transfer leaves nothing on disk — partial files
are discarded, so a truncated download can never be mistaken for a good one.
hack/install-*.sh. Every installer that verified a SHA256 beforestill does; the two unverified-by-design installers stay explicitly exempted.
.github/actions/kind-e2e-setup/action.yml(CI-KINDLOOP-01), whosehand-rolled 3-attempt loop had two defects:
chmod +xon a file that was neverwritten, so the download failure surfaced later as a confusing
chmoderror rather thanat the download; and
hack/test/ci_fetch_lib_hardening_test.sh) forbidding direct fetcherinvocations, so a future bare
curlcannot re-introduce the 👷 ci(release): derive the chart OCI coordinate once and publish it under charts/ #351 failure mode.Risk
This changes the
kinddownload path used by the requiredkind-smokecheck, which makeskind-smokethe most important signal on this PR. The shipped installer and action code wasvalidated by execution: it fetched the real 10,522,750-byte
kindbinary through the pinnedflag set and ran
kind version 0.32.0.Verification
Run on the rebased head:
hack/test/ci_fetch_lib_hardening_test.sh— pass (16 assertions; the lane's own gate)hack/test/ci_install_helm_hardening_test.sh— pass (CI-wired atci.yaml:136; it invokesthe sibling gate first)
hack/lint-shell.sh— passtask verify— passZero Go files are touched by this lane.
Review
Passed independent review with APPROVE, no P0 and no P1, after four rounds.