feat: add downloadBaseURL input to support custom download mirrors#260
feat: add downloadBaseURL input to support custom download mirrors#260acouvreur wants to merge 1 commit into
Conversation
|
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
❤️ 💡 Thank you so much for this @acouvreur, also for the contribution for #261 , I really appreciate this, gentle Fyi to regular contributors like @davidgamero or @bosesuneha
Here are some key notes and worth to get thoughts and eye collectively here, but worth a clear share in details:
TL;DR
This PR is a definitely small, reasonable feature with an outsized security surface. The diff itself is clean, but it ships an arbitrary-URL fetch-and-exec primitive with no integrity verification, no scheme validation, no SSRF guardrails, and an incomplete realization of its own use case (latest still hits dl.k8s.io). The PR is a much stronger merge candidate with input validation + a checksum story baked in, even as a follow-up tracked in the PR description.
Behavior changes vs. current
-
New optional input
downloadBaseURL(defaulthttps://dl.k8s.io) — backward compatible when omitted. Trailing slash is stripped viabaseURL.replace(/\/$/, ''). -
getkubectlDownloadURL(version, arch, baseURL)— third param added with default. Default-arg keeps callers happy; no breaking signature change at runtime, but TypeScript consumers importing the helper get a wider signature. -
downloadKubectl(version, downloadBaseURL)— same pattern. Default'https://dl.k8s.io'is duplicated in three places (action.yml,getkubectlDownloadURL,downloadKubectl). Drift risk — should be one exported constant. -
run()readsdownloadBaseURLand forwards it. Note:core.getInputreturns''(empty string) when not set, notundefined. The current code passes that empty string straight intodownloadKubectl(version, ''), which then passes''intogetkubectlDownloadURL(version, arch, ''). Because the default-parameter only kicks in forundefined, the action.yml defaulthttps://dl.k8s.iois what actually saves it — the function defaults are dead code in the GH Actions path. If a non-Actions caller forgets the param they get the helper default; if it's the Actions path the action.yml default wins. Inconsistent but not broken. Worth either: (a) coalescing|| 'https://dl.k8s.io'inrun.ts, or (b) removing the action.yml default and relying on the function defaults. -
getStableKubectlVersion()still hardcodeshttps://dl.k8s.io/release/stable.txt. So in an air-gapped scenario whereversion: latestis set, the mirror is bypassed and the call todl.k8s.iofails. The feature is incomplete for its stated air-gapped use case —stableVersionUrlshould also derive fromdownloadBaseURL. -
No checksum/signature verification anywhere in the action (this is pre-existing, but #260 makes the gap much worse — see security).
Security aspects: this is where it gets interesting
1. SSRF / arbitrary binary execution (the big one)
The whole point of this input is "fetch a binary from a URL of the caller's choosing, mark it executable, and add it to PATH". That is a introduced unrestricted-URL fetch + exec sink, and it lives in a GitHub Action that runs in workflows.
Threat model matters a lot here:
- Repo-controlled workflow with hardcoded
downloadBaseURL: low risk. Workflow author already controls what runs. - Reusable workflow / composite action that forwards inputs: medium risk. A downstream consumer can supply a malicious mirror.
pull_request_targetor matrix from untrusted source / Dependabot-like flows that pass values through: high risk. An attacker who can influence the input value gets RCE on the runner with whatever secrets the job has.- Workflows that interpolate
${{ github.event.* }}into action inputs (anti-pattern but common): direct path to attacker-controlled URL.
Mitigations the PR could/should adopt:
- Scheme allow-list: reject anything but
https://(and maybehttp://only when explicitly allowed). The current code accepts ,ftp://,data:,javascript:(toolCache may filter some, but defense in depth). - URL parsing via
new URL(baseURL)to validate structure, then reconstruct — don't just string-concatenate. - Optional allow-list input (
downloadBaseURLAllowlistor doc guidance) so org admins can pin via org-level required workflows. - Warn loudly (
core.warningorcore.notice) when a non-default base URL is used — gives audit visibility in logs.
2. No integrity verification
With the official https://dl.k8s.io host you at least get TLS + a host the k8s community controls. With a custom mirror you've delegated trust to whoever runs that mirror, and the action still:
- Doesn't verify a SHA256 against
https://dl.k8s.io/release/<v>/bin/<os>/<arch>/kubectl.sha256(which exists upstream). - Doesn't verify cosign / sigstore signatures (kubectl is signed since v1.26).
Recommendation worth raising on the PR: when downloadBaseURL != default, require or at least strongly encourage a checksum input. The combination "custom mirror + no checksum" is the actually-dangerous configuration. The PR would be much more defensible if it shipped with checksum verification in the same change (or as an immediate follow-up tracked in the PR description).
3. URL construction — string concat with user input
const base = baseURL.replace(/\/$/, '')
return `${base}/release/${version}/bin/linux/${arch}/kubectl`Issues:
versionhere is also user-controlled (same sink as #261's concern). Combined with custom base URL, an attacker controlling both could craftbaseURL=https://evil.tld+version=../../whateverto bend the path. Tool cache key (toolCache.find('kubectl', version)) also picks this up as a directory name. Validateversionagainst a regex (the suggestion from #261 applies here too — even more so now).- No validation that
baseURLdoesn't already include/release/.... User putsdownloadBaseURL: https://mirror/k8s/releaseand you get…/release/release/v…— silent 404. Cosmetic, but a sharp edge. - Userinfo in URL (
https://user:pass@host) is accepted. That can leak credentials into logs viacore.infoof the download URL (if anything logs it). - IDN / unicode hosts:
new URL('https://раураl.com')parses fine. Probably out of scope, but doc the assumption.
4. Open redirect / 302 to another origin
toolCache.downloadTool follows redirects. A mirror that 302s to passwd or http://169.254.169.254/... (cloud metadata) is a classic SSRF pivot. Mitigation: pass { allowRedirects: false } or validate the final resolved URL stays on the same origin as baseURL. At minimum: when not the default base URL, disable HTTP fallback (only allow HTTPS end-to-end, including redirects).
5. Cloud metadata / SSRF to link-local
http://169.254.169.254/ (AWS/Azure IMDS), http://[fd00:ec2::254], http://localhost:..., http://10.x.x.x. On hosted runners these may be partly mitigated (IMDSv2 etc.), but on self-hosted runners this is real. Worth blocking RFC1918 / link-local / loopback by default and gating behind an explicit "allow private" flag.
6. Test coverage gaps
The PR's 7 tests cover happy-path + trailing-slash. They do not assert:
- Empty-string input is handled.
- Invalid URLs are rejected.
- The
getStableKubectlVersionpath is unaffected (it still hitsdl.k8s.io, which contradicts the air-gap goal — should be a test or an acknowledged limitation). - Behavior when
downloadBaseURLincludes a path (https://host/k8s) — does it concatenate correctly? Looks like yes, but no test.
7. Supply-chain framing
GitHub's own actions/setup-* actions have moved toward signature/checksum verification specifically because "let users point at a mirror" is a known footgun (actions/setup-node, setup-go discussions). Worth aligning with that direction before merging.
A plan for moving forward PR as a concrete review asks for #260
- Validate the input:
new URL()parse, requirehttps:, reject userinfo, optionally reject private/link-local hosts unless an opt-in flag is set. - Single source of truth for the default: export
const DEFAULT_KUBECTL_BASE_URL = 'https://dl.k8s.io'fromhelpers.tsand import it in action.yml docs andrun.ts. Remove duplication. - Fix the
getStableKubectlVersiongap sostableVersionUrlis built fromdownloadBaseURL— otherwiseversion: latest+ custom mirror is broken (the headline use case is air-gapped envs). - Validate
versionwith a regex (same fix that #261 needs). - Add checksum verification, or at least an optional
checksum/checksumURLinput, and document that custom mirrors should always set it. Without this the feature meaningfully degrades the action's security posture. - Disable or constrain redirects in
toolCache.downloadTool; or post-validate that the final URL's origin matchesbaseURL's origin. core.noticewhen a non-default base URL is in use — visible in the Actions summary, helps audit.- Tests: empty string, invalid URL, redirect to disallowed scheme, IMDS address, base URL containing a path, base URL with userinfo.
- Docs: warn loudly in README that the input bypasses upstream and the user is on the hook for integrity; recommend pairing with checksum.
There was a problem hiding this comment.
Pull request overview
Adds an optional downloadBaseURL action input so users in air-gapped/enterprise environments can fetch kubectl from a custom mirror instead of https://dl.k8s.io. The new value is threaded from run() through downloadKubectl() into getkubectlDownloadURL(), with trailing-slash normalization, and is covered by new unit tests.
Changes:
- New optional
downloadBaseURLinput inaction.ymldefaulting tohttps://dl.k8s.io. getkubectlDownloadURLanddownloadKubectlaccept a base URL (default preserved, trailing/stripped).- New tests cover custom base URL, trailing-slash normalization, the default URL, and that
core.getInputis called for the new input.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| action.yml | Declares the new optional downloadBaseURL input with default https://dl.k8s.io. |
| src/helpers.ts | getkubectlDownloadURL gains a baseURL parameter and strips a trailing slash before building per-OS URLs. |
| src/run.ts | run() reads downloadBaseURL via core.getInput and forwards it; downloadKubectl accepts and propagates it with a default. |
| src/run.test.ts | Adds tests for custom base URL, trailing-slash stripping, default-URL assertion, and new getInput call. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export async function downloadKubectl( | ||
| version: string, | ||
| downloadBaseURL: string = 'https://dl.k8s.io' | ||
| ): Promise<string> { |
| version = await resolveKubectlVersion(version) | ||
| } | ||
| const cachedPath = await downloadKubectl(version) | ||
| const downloadBaseURL = core.getInput('downloadBaseURL', {required: false}) |
| const downloadBaseURL = core.getInput('downloadBaseURL', {required: false}) | ||
| const cachedPath = await downloadKubectl(version, downloadBaseURL) |
| required: true | ||
| default: 'latest' | ||
| downloadBaseURL: | ||
| description: 'Set the download base URL' |
Summary
Closes #206
Adds a new optional
downloadBaseURLaction input that allows users to specify a custom or private mirror for kubectl downloads, useful in air-gapped or enterprise environments.Changes
action.yml— new optional inputdownloadBaseURL(default:https://dl.k8s.io)src/helpers.ts—getkubectlDownloadURL()accepts abaseURLparameter (trailing slashes are stripped)src/run.ts—downloadKubectl()accepts and forwardsdownloadBaseURL;run()reads the input and passes it throughsrc/run.test.ts— 7 new tests covering custom base URL and trailing-slash normalizationUsage
Tests
All 37 tests pass.