v2 hardening effort part 1#578
Conversation
Signed-off-by: Adam Martin <adam.martin@ranchergovernment.com>
Signed-off-by: Adam Martin <adam.martin@ranchergovernment.com>
…ardening (H3/H4) Signed-off-by: Adam Martin <adam.martin@ranchergovernment.com>
Signed-off-by: Adam Martin <adam.martin@ranchergovernment.com>
amartin120
added
size/M
Signed-off-by: Adam Martin <adam.martin@ranchergovernment.com>
amartin120
force-pushed
the
hardening-part-1
branch
from
ba42bb4 to
c2a559a
Compare
…H4 follow-up) Signed-off-by: Adam Martin <adam.martin@ranchergovernment.com>
There was a problem hiding this comment.
Pull request overview
This PR is the first part of a v2 hardening effort, focusing on making previously parsed security-related flags take effect, adding bounded I/O limits for downloads/manifests/archive extraction, adding SSRF protections to HTTP fetching, and making stdout/stderr capture safe under concurrency.
Changes:
- Propagates Helm chart verify/auth/TLS-related
action.ChartPathOptionsinto the Helm client and switches Helm log suppression to a goroutine-safe capture helper. - Adds centralized hardening limits and enforces bounded reads/copies for HTTP downloads, OCI manifest/index reads, and archive extraction/joining.
- Introduces SSRF protections (scheme allowlist + internal IP blocking with an opt-out flag) and threads
--allow-internal-targetsthrough store commands.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/content/chart/chart.go |
Propagates chart verify/auth/TLS options; uses goroutine-safe output capture during chart location. |
pkg/content/chart/chart_test.go |
Adds regression tests covering --verify behavior and option propagation. |
pkg/log/logcapture.go |
Serializes stdout/stderr capture and adds panic safety for capture windows. |
pkg/log/logcapture_test.go |
Adds concurrency/panic/FD-restore coverage for output capture. |
pkg/consts/limits.go |
Adds centralized caps for downloads, manifests, archive extraction, and HTTP timeout. |
pkg/getter/https.go |
Implements bounded HTTP reads, timeouts, redirect validation, and dial-time SSRF IP checks. |
pkg/getter/getter.go |
Plumbs AllowInternalTargets through the default HTTP getter via ClientOptions. |
pkg/getter/https_security_test.go |
Adds tests for scheme rejection, SSRF blocking, redirects, download caps, and timeout behavior. |
internal/flags/store.go |
Adds --allow-internal-targets flag to store root options. |
cmd/hauler/cli/store/add.go |
Threads allowInternalTargets into storeFile() and the getter client options. |
cmd/hauler/cli/store/load.go |
Threads SSRF opt-out into remote archive fetching; adds bounded remote download and bounded index.json read. |
cmd/hauler/cli/store/load_test.go |
Updates tests for unarchiveLayoutTo() signature and loopback HTTP fixtures. |
cmd/hauler/cli/store/sync.go |
Threads SSRF opt-out into HTTP getter; adds bounded remote downloads and bounded manifest reads. |
cmd/hauler/cli/store/sync_test.go |
Updates tests to opt out of SSRF protection for loopback test servers. |
pkg/store/store.go |
Adds bounded manifest/index reads during descriptor-graph copy. |
pkg/archives/unarchiver.go |
Adds extraction caps (per-file/aggregate/file-count/decompression ratio) and bounds chunk joining size. |
pkg/archives/archiver.go |
Exposes compression/archive format vars for tests to build compatible archives. |
pkg/archives/limits_test.go |
Adds tests validating extraction caps and successful extraction within limits. |
pkg/artifacts/file/file_test.go |
Updates tests to allow loopback HTTP targets. |
install.sh |
Tightens permissions under ~/.hauler from world-writable to owner-only. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| transport := &http.Transport{ | ||
| DialContext: func(ctx context.Context, network, address string) (net.Conn, error) { | ||
| return h.safeDial(ctx, baseDialer, network, address) | ||
| }, | ||
| TLSHandshakeTimeout: 10 * time.Second, | ||
| ResponseHeaderTimeout: 30 * time.Second, | ||
| ExpectContinueTimeout: 1 * time.Second, | ||
| MaxIdleConns: 10, | ||
| IdleConnTimeout: 90 * time.Second, | ||
| // Do NOT set TLSClientConfig: Go derives tls.Config.ServerName from | ||
| // the request URL hostname, so TLS cert verification continues to use | ||
| // the hostname even though we dial by IP literal. | ||
| } |
There was a problem hiding this comment.
NewHttpWithOptions builds a custom http.Transport but doesn’t set Proxy: http.ProxyFromEnvironment. Compared to the previous use of http.DefaultClient/http.Get, this disables standard HTTP(S) proxy env var support (HTTP_PROXY/HTTPS_PROXY/NO_PROXY), which can break users in proxied environments. If proxies must remain disabled to preserve SSRF protections, please make that an explicit/intentional choice (e.g., comment/docs/flag); otherwise set ProxyFromEnvironment and add appropriate target-host SSRF validation (proxying otherwise bypasses the dial-time IP checks).
| # ensure hauler directory is writable (by user or root privileges) | ||
| chmod -R 777 "${HAULER_DIR}" || fatal "Failed to Update Permissions of Hauler Directory: ${HAULER_DIR}" | ||
| # ensure hauler directory is only accessible by the owner | ||
| chmod -R 0700 "${HAULER_DIR}" || fatal "Failed to Update Permissions of Hauler Directory: ${HAULER_DIR}" |
There was a problem hiding this comment.
chmod -R 0700 applies execute bits to all files under ~/.hauler (not just directories), which can unintentionally make regular files executable and also removes group read access even for non-sensitive artifacts. If the goal is “owner-only access”, prefer a mode that applies x only to directories (e.g., chmod -R go-rwx,u+rwX ...) or chmod the directory itself and set file modes more selectively.
| chmod -R 0700 "${HAULER_DIR}" || fatal "Failed to Update Permissions of Hauler Directory: ${HAULER_DIR}" | |
| chmod -R go-rwx,u+rwX "${HAULER_DIR}" || fatal "Failed to Update Permissions of Hauler Directory: ${HAULER_DIR}" |
| remaining := consts.MaxArchiveBytes - joinedTotal | ||
| if remaining <= 0 { | ||
| cf.Close() | ||
| return "", fmt.Errorf("failed to copy chunk [%s]: %w", chunk, err) | ||
| return "", fmt.Errorf("joined chunks exceed maximum allowed size (%d bytes)", consts.MaxArchiveBytes) | ||
| } | ||
| n, err := io.Copy(outf, io.LimitReader(cf, remaining+1)) | ||
| cf.Close() | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to copy chunk [%s]: %w", chunk, err) | ||
| } | ||
| joinedTotal += n | ||
| if joinedTotal > consts.MaxArchiveBytes { | ||
| return "", fmt.Errorf("joined chunks exceed maximum allowed size (%d bytes)", consts.MaxArchiveBytes) | ||
| } |
There was a problem hiding this comment.
JoinChunks uses io.LimitReader(cf, remaining+1) and then checks joinedTotal > MaxArchiveBytes after the copy. This can write up to 1 byte beyond the configured cap into the joined output before returning an error. Enforce the cap without writing past it (e.g., copy only remaining bytes, then attempt to read one extra byte to detect overflow, and/or truncate/remove the partially written joined file on overflow).
| n, err := io.Copy(out, io.LimitReader(rc, consts.MaxDownloadBytes+1)) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if n > consts.MaxDownloadBytes { |
There was a problem hiding this comment.
This download path bounds the copy with io.LimitReader(rc, MaxDownloadBytes+1) and then checks n > MaxDownloadBytes after writing. That can leave an oversized temp file (up to +1 byte) on disk even though the operation fails. Consider copying only up to MaxDownloadBytes and probing for one extra byte (or truncating/removing the temp file) to enforce a strict on-disk cap; also ensure the reported URL in any error message isn’t using the rewritten local haulPath value.
| return err | ||
| } | ||
| if n > consts.MaxDownloadBytes { | ||
| return fmt.Errorf("remote manifest at %s exceeds maximum allowed size (%d bytes)", haulPath, consts.MaxDownloadBytes) | ||
| } |
There was a problem hiding this comment.
The remote-manifest download uses io.LimitReader(rc, MaxDownloadBytes+1) and only errors after the copy completes. This can write past the intended cap (up to +1 byte) into the temp file before failing. Consider enforcing the cap without writing beyond it (copy MaxDownloadBytes then check for an extra byte, and/or truncate/remove the temp file), and make sure the error message reports the original remote URL rather than the rewritten local haulPath temp path.
| return err | ||
| } | ||
| if n > consts.MaxDownloadBytes { | ||
| return fmt.Errorf("remote image.txt at %s exceeds maximum allowed size (%d bytes)", haulPath, consts.MaxDownloadBytes) | ||
| } |
There was a problem hiding this comment.
Same issue as the manifest download: the image.txt download can write beyond the intended MaxDownloadBytes cap (up to +1 byte) before returning an error, leaving an oversized temp file behind. Enforce the cap without writing past it (copy MaxDownloadBytes then check for an extra byte, and/or truncate/remove the temp file) and ensure the error message reports the original URL rather than the local haulPath temp file path.
|
|
||
| // Wait for logging goroutines to finish | ||
| wg.Wait() | ||
|
|
There was a problem hiding this comment.
CaptureOutput starts pipes for stdout/stderr but never closes stdoutReader/stderrReader after the logStream goroutines finish. Even though the writers are closed, the read ends are still FDs and should be closed to avoid leaking file descriptors across repeated captures (especially in long-running processes/tests).
| // Close the read ends now that the logStream goroutines are done. | |
| stdoutReader.Close() | |
| stderrReader.Close() |
Please check below, if the PR fulfills these requirements:
Associated Links:
Types of Changes:
Proposed Changes:
A1 — Helm chart --verify/auth/TLS option propagation (H1)
pkg/content/chart/chart.go— populates full action.ChartPathOptions (Verify, Username, Password, CertFile, KeyFile, CaFile, InsecureSkipTLSverify, PlainHTTP) before constructing the registry client.pkg/content/chart/chart_test.go(new tests) — regression test that --verify fails on an unsigned chart.A2 — install.sh permissions (H2)
install.sh— chmod -R 777 → chmod -R 0700.A3 — Bounded HTTP downloads / archive extraction / registry reads (H3)
pkg/consts/limits.go(NEW) — centralized caps: 16 MiB manifests, 10 GiB downloads, 100 GiB total archive, 50 GiB per-file, 100k files, 100× decompression ratio, 30 min HTTP timeout.pkg/getter/https.go— http.Client{Timeout, CheckRedirect}, Content-Length validation, limitedReadCloser with cap+1 detection.pkg/archives/unarchiver.go— extractionLimits, extractionState, per-file/aggregate/file-count caps in copyBounded, decompression-ratio check, JoinChunks bounded.pkg/archives/archiver.go— exposed compressionZstd/archivalTar so tests can build real archives.pkg/store/store.go— bounded manifest/index reads (read cap+1, reject if > cap) at the two copyDescriptorGraph sites.cmd/hauler/cli/store/load.go— bounded remote-archive download and index.json read.cmd/hauler/cli/store/sync.go— bounded product manifest read, remote-manifest download, image.txt download.pkg/archives/limits_test.go(new tests) — per-file cap, aggregate cap, file-count cap, decompression-ratio cap, within-limits success.A4 — SSRF hardening (H4)
pkg/getter/https.go— scheme allowlist (http/https), validateRequest enforces private/loopback/link-local/ULA rejection on initial request and every redirect.pkg/getter/getter.go— ClientOptions.AllowInternalTargets plumbed into the default HTTP getter.internal/flags/store.go— --allow-internal-targets opt-out flag on StoreRootOpts.cmd/hauler/cli/store/add.go— storeFile() gains allowInternalTargets parameter.cmd/hauler/cli/store/sync.go— passes the flag through to NewHttpWithOptions at three call sites and to storeFile.cmd/hauler/cli/store/load.go— unarchiveLayoutTo() gains allowInternalTargets, threads it to the HTTP getter.pkg/getter/https_security_test.go(new tests) — scheme rejection, private-IP rejection, redirect re-validation, opt-out behavior.A5 — Goroutine-safe stdout/stderr capture (H5)
pkg/log/logcapture.go— captureOutputMu mutex serializes capture windows, defer recover() restores FDs on panic, fn panics propagate via err channel.pkg/content/chart/chart.go— NewChart no longer mutates os.Stdout/os.Stderr directly; routes through log.CaptureOutput.pkg/log/logcapture_test.go(new tests) — concurrent stdout writes with -race, FD restore after normal return, FD restore after panic.A4 ripple — test adaptations only (signature changes)
These files only changed call sites to pass the new allowInternalTargets argument (or unarchiveLayoutTo arg). No new logic.
cmd/hauler/cli/store/add_test.gocmd/hauler/cli/store/copy_test.gocmd/hauler/cli/store/add.go— storeFile() gains allowInternalTargets parameter.cmd/hauler/cli/store/sync.go— passes the flag through to NewHttpWithOptions at three call sites and to storeFile.cmd/hauler/cli/store/load.go— unarchiveLayoutTo() gains allowInternalTargets, threads it to the HTTP getter.cmd/hauler/cli/store/load.go— unarchiveLayoutTo() gains allowInternalTargets, threads it to the HTTP getter.pkg/getter/https_security_test.go(new tests) — scheme rejection, private-IP rejection, redirect re-validation, opt-out behavior.cmd/hauler/cli/store/add.go— storeFile() gains allowInternalTargets parameter.cmd/hauler/cli/store/sync.go— passes the flag through to NewHttpWithOptions at three call sites and to storeFile.cmd/hauler/cli/store/load.go— unarchiveLayoutTo() gains allowInternalTargets, threads it to the HTTP getter.pkg/getter/https_security_test.go(new tests) — scheme rejection, private-IP rejection, redirect re-validation, opt-out behavior.A5 — Goroutine-safe stdout/stderr capture (H5)
pkg/log/logcapture.go— captureOutputMu mutex serializes capture windows, defer recover() restores FDs on panic, fn panics propagate via err channel.pkg/content/chart/chart.go— NewChart no longer mutates os.Stdout/os.Stderr directly; routes through log.CaptureOutput.pkg/log/logcapture_test.go(new tests) — concurrent stdout writes with -race, FD restore after normal return, FD restore after panic.A4 ripple — test adaptations only (signature changes)
These files only changed call sites to pass the new allowInternalTargets argument (or unarchiveLayoutTo arg). No new logic.
cmd/hauler/cli/store/add_test.gocmd/hauler/cli/store/copy_test.gocmd/hauler/cli/store/extract_test.gocmd/hauler/cli/store/info_test.gocmd/hauler/cli/store/lifecycle_test.gocmd/hauler/cli/store/load_test.gocmd/hauler/cli/store/remove_test.gocmd/hauler/cli/store/save_test.gocmd/hauler/cli/store/sync_test.gopkg/artifacts/file/file_test.goVerification/Testing of Changes:
Additional Context:
User-visible default-behavior changes by fix:
A1 — Chart flags actually take effect now (chart.go, what you're looking at)
Before: --verify, --username, --password, --cert-file, --key-file, --ca-file, --insecure-skip-tls-verify, --plain-http were parsed but silently ignored when running hauler store add chart.
After: those flags now flow into the Helm registry client. Practical impact:
A2 — install.sh leaves ~/.hauler mode 0700 instead of 0777.
A3 — HTTP and archive caps
A4 — SSRF block (the most impactful change)
This is the one most likely to surprise existing users.