mirror of
https://github.com/pgsty/minio.git
synced 2026-08-09 07:43:29 +03:00
fix: harden healthcheck and distroless lanes per adversarial review
Findings from an adversarial review (Codex, gpt-5.6-sol at max effort) of2ff594f4band4c34d2309, each independently verified before fixing: - SBOM generation: buildx attaches a provenance attestation, so every per-arch digest names an OCI index; Syft's platform default on an amd64 runner cannot resolve an arm64-only index and the step dies. Pass --platform explicitly on all four Syft calls (the two classic lanes had the same latent defect - the renamed workflow has not run yet, which is why it never fired). - Release ordering: the HEALTHCHECK survival check now runs against the pushed architecture image before the versioned and rolling multi-arch manifests are created, so a broken health config blocks their promotion; the comment now states honestly that the arch-suffixed tags are already public at that point. - Gate assertions: tar's member-argument mode exits non-zero on any missing name, which under pipefail masked a found forbidden file when exactly one of them existed; -tv prints symlinks as 'name -> target', defeating $-anchored greps; and the licenses check proved only one-of-three. Export the rootfs once and assert every required and forbidden entry individually (busybox/sh and usr/bin/mc[li] now covered), and match the image healthcheck as an exact array instead of a substring. - Probe target vs CLI-configured servers: a probe process cannot see PID 1's argv, so --url gains EnvVar MINIO_HEALTHCHECK_URL as the documented way to point the baked-in HEALTHCHECK at a server whose address/TLS comes from command-line arguments (verified end to end: server on --address :9010, env var alone turns the container healthy). Baseline regenerated for the new env token. - IPv6 zone identifiers: serialize probe URLs via url.URL.String() so [fe80::1%eth0]:9000 becomes a valid %25-escaped URL (tests added). - Boolean flags: read --json/--quiet via Bool() so --json=false is false, instead of IsSet() which treats any occurrence as true. - Docker's HEALTHCHECK timeout raised to 10s: an outer deadline equal to the probe's own 5s always SIGKILLed the probe before it could print its diagnostic line. - test-release path filter now also triggers on cmd/healthcheck-main.go and cmd/main.go, so subcommand regressions run the image gate. Not adopted: require_text's comment-insensitivity in verify-rebrand.sh (snapshot-tripwire by design, consistent with its other assertions - the semantic check lives in the CI gate now), and full staging-then-promote tag publishing (a workflow-wide redesign shared with the classic lanes, tracked as follow-up). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -220,6 +220,21 @@ jobs:
|
|||||||
org.opencontainers.image.created=${{ env.PUBLISHED_AT }}
|
org.opencontainers.image.created=${{ env.PUBLISHED_AT }}
|
||||||
org.opencontainers.image.revision=${{ env.RELEASE_REVISION }}
|
org.opencontainers.image.revision=${{ env.RELEASE_REVISION }}
|
||||||
|
|
||||||
|
- name: Verify HEALTHCHECK survived the distroless push
|
||||||
|
run: |
|
||||||
|
set -euo pipefail
|
||||||
|
# HEALTHCHECK is a Docker extension absent from the OCI image
|
||||||
|
# spec, and a publish path can drop it silently. Check the pushed
|
||||||
|
# architecture image now, before the versioned and rolling
|
||||||
|
# multi-arch manifests are created, so a broken health config
|
||||||
|
# stops their promotion. (The architecture-suffixed tags above
|
||||||
|
# are already public by this point - full staging-then-promote
|
||||||
|
# would be a workflow-wide redesign shared with the classic
|
||||||
|
# image lanes.)
|
||||||
|
docker pull "pgsty/silo:${RELEASE_TAG}-distroless-amd64" >/dev/null
|
||||||
|
test "$(docker inspect -f '{{json .Config.Healthcheck.Test}}' "pgsty/silo:${RELEASE_TAG}-distroless-amd64")" \
|
||||||
|
= '["CMD","/usr/bin/silo","healthcheck","ready"]'
|
||||||
|
|
||||||
- name: Publish multi-architecture manifests
|
- name: Publish multi-architecture manifests
|
||||||
run: |
|
run: |
|
||||||
set -euo pipefail
|
set -euo pipefail
|
||||||
@@ -274,17 +289,6 @@ jobs:
|
|||||||
fi
|
fi
|
||||||
echo "SILO_DISTROLESS_DIGEST=${DISTROLESS_RELEASE_DIGEST}" >> "${GITHUB_ENV}"
|
echo "SILO_DISTROLESS_DIGEST=${DISTROLESS_RELEASE_DIGEST}" >> "${GITHUB_ENV}"
|
||||||
|
|
||||||
- name: Verify HEALTHCHECK survived the distroless publish
|
|
||||||
run: |
|
|
||||||
set -euo pipefail
|
|
||||||
# HEALTHCHECK is a Docker extension absent from the OCI image
|
|
||||||
# spec, and OCI-media-type publishes can drop it silently. If the
|
|
||||||
# pushed image lost it, the pilot's zero-config compose health
|
|
||||||
# gating disappears - treat that as a release blocker.
|
|
||||||
docker pull "pgsty/silo:${RELEASE_TAG}-distroless" >/dev/null
|
|
||||||
docker inspect -f '{{json .Config.Healthcheck.Test}}' "pgsty/silo:${RELEASE_TAG}-distroless" \
|
|
||||||
| grep -F '"healthcheck"'
|
|
||||||
|
|
||||||
- name: Generate architecture image SBOMs
|
- name: Generate architecture image SBOMs
|
||||||
env:
|
env:
|
||||||
AMD64_DIGEST: ${{ steps.build-amd64.outputs.digest }}
|
AMD64_DIGEST: ${{ steps.build-amd64.outputs.digest }}
|
||||||
@@ -295,13 +299,21 @@ jobs:
|
|||||||
run: |
|
run: |
|
||||||
set -euo pipefail
|
set -euo pipefail
|
||||||
mkdir -p docker-release/sbom
|
mkdir -p docker-release/sbom
|
||||||
|
# Each per-architecture digest names an OCI index (image plus the
|
||||||
|
# provenance attestation buildx attaches), and Syft's platform
|
||||||
|
# default on an index follows the amd64 runner - an arm64-only
|
||||||
|
# index would fail outright. Select the platform explicitly.
|
||||||
syft "registry:index.docker.io/pgsty/silo@${AMD64_DIGEST}" \
|
syft "registry:index.docker.io/pgsty/silo@${AMD64_DIGEST}" \
|
||||||
|
--platform linux/amd64 \
|
||||||
--output "spdx-json=docker-release/sbom/linux-amd64.spdx.json"
|
--output "spdx-json=docker-release/sbom/linux-amd64.spdx.json"
|
||||||
syft "registry:index.docker.io/pgsty/silo@${ARM64_DIGEST}" \
|
syft "registry:index.docker.io/pgsty/silo@${ARM64_DIGEST}" \
|
||||||
|
--platform linux/arm64 \
|
||||||
--output "spdx-json=docker-release/sbom/linux-arm64.spdx.json"
|
--output "spdx-json=docker-release/sbom/linux-arm64.spdx.json"
|
||||||
syft "registry:index.docker.io/pgsty/silo@${DISTROLESS_AMD64_DIGEST}" \
|
syft "registry:index.docker.io/pgsty/silo@${DISTROLESS_AMD64_DIGEST}" \
|
||||||
|
--platform linux/amd64 \
|
||||||
--output "spdx-json=docker-release/sbom/linux-amd64-distroless.spdx.json"
|
--output "spdx-json=docker-release/sbom/linux-amd64-distroless.spdx.json"
|
||||||
syft "registry:index.docker.io/pgsty/silo@${DISTROLESS_ARM64_DIGEST}" \
|
syft "registry:index.docker.io/pgsty/silo@${DISTROLESS_ARM64_DIGEST}" \
|
||||||
|
--platform linux/arm64 \
|
||||||
--output "spdx-json=docker-release/sbom/linux-arm64-distroless.spdx.json"
|
--output "spdx-json=docker-release/sbom/linux-arm64-distroless.spdx.json"
|
||||||
|
|
||||||
- name: Attest amd64 image SBOM
|
- name: Attest amd64 image SBOM
|
||||||
|
|||||||
@@ -8,6 +8,8 @@ on:
|
|||||||
- ".github/nfpm.yml"
|
- ".github/nfpm.yml"
|
||||||
- "Dockerfile.goreleaser"
|
- "Dockerfile.goreleaser"
|
||||||
- "Dockerfile.distroless"
|
- "Dockerfile.distroless"
|
||||||
|
- "cmd/healthcheck-main.go"
|
||||||
|
- "cmd/main.go"
|
||||||
- "dockerscripts/download-static-curl.sh"
|
- "dockerscripts/download-static-curl.sh"
|
||||||
- "dockerscripts/docker-entrypoint.sh"
|
- "dockerscripts/docker-entrypoint.sh"
|
||||||
- "dockerscripts/docker-entrypoint_test.sh"
|
- "dockerscripts/docker-entrypoint_test.sh"
|
||||||
@@ -401,23 +403,33 @@ jobs:
|
|||||||
docker build -t silo-distroless-test:snapshot -f "${ctx}/Dockerfile.distroless" "${ctx}"
|
docker build -t silo-distroless-test:snapshot -f "${ctx}/Dockerfile.distroless" "${ctx}"
|
||||||
|
|
||||||
# HEALTHCHECK is a Docker extension absent from the OCI image
|
# HEALTHCHECK is a Docker extension absent from the OCI image
|
||||||
# spec; assert it survived into the built image config.
|
# spec; assert the exact probe command survived into the image
|
||||||
docker inspect -f '{{json .Config.Healthcheck.Test}}' silo-distroless-test:snapshot \
|
# config, not merely a substring of it.
|
||||||
| grep -F '"healthcheck"' >/dev/null
|
test "$(docker inspect -f '{{json .Config.Healthcheck.Test}}' silo-distroless-test:snapshot)" \
|
||||||
|
= '["CMD","/usr/bin/silo","healthcheck","ready"]'
|
||||||
|
|
||||||
# /data ships in the image layer world-writable (issue #55):
|
# /data ships in the image layer world-writable (issue #55):
|
||||||
# there is no entrypoint left to repair ownership at runtime. The
|
# there is no entrypoint left to repair ownership at runtime.
|
||||||
# same export proves the image carries the binary and the license
|
# Export the rootfs once, then assert each required and each
|
||||||
# set, and neither a shell nor a legacy /usr/bin/minio.
|
# forbidden entry individually: tar's member-argument mode exits
|
||||||
|
# non-zero on any missing name, which under pipefail masks a
|
||||||
|
# found forbidden file, and -tv prints symlinks as 'name ->
|
||||||
|
# target' which defeats $-anchored greps.
|
||||||
probe="$(docker create silo-distroless-test:snapshot server /data)"
|
probe="$(docker create silo-distroless-test:snapshot server /data)"
|
||||||
docker export "${probe}" | tar -tvf - data usr/bin/silo licenses > "${ctx}/listing.txt"
|
docker export "${probe}" -o "${ctx}/rootfs.tar"
|
||||||
grep -E '^drwxrwxrwx.* data/$' "${ctx}/listing.txt" >/dev/null
|
|
||||||
grep -E ' licenses/(LICENSE|NOTICE|CREDITS)$' "${ctx}/listing.txt" >/dev/null
|
|
||||||
if docker export "${probe}" | tar -tf - bin/sh usr/bin/minio 2>/dev/null | grep -q .; then
|
|
||||||
echo "distroless image unexpectedly contains a shell or /usr/bin/minio"
|
|
||||||
exit 1
|
|
||||||
fi
|
|
||||||
docker rm "${probe}" >/dev/null
|
docker rm "${probe}" >/dev/null
|
||||||
|
tar -tf "${ctx}/rootfs.tar" > "${ctx}/names.txt"
|
||||||
|
tar -tvf "${ctx}/rootfs.tar" > "${ctx}/verbose.txt"
|
||||||
|
grep -E '^drwxrwxrwx.* data/$' "${ctx}/verbose.txt" >/dev/null
|
||||||
|
for want in usr/bin/silo licenses/LICENSE licenses/NOTICE licenses/CREDITS; do
|
||||||
|
grep -Fxq "${want}" "${ctx}/names.txt" || { echo "missing ${want}"; exit 1; }
|
||||||
|
done
|
||||||
|
for forbid in bin/sh usr/bin/sh busybox/sh usr/bin/minio usr/bin/mc usr/bin/mcli; do
|
||||||
|
if grep -Fxq "${forbid}" "${ctx}/names.txt"; then
|
||||||
|
echo "distroless image unexpectedly contains ${forbid}"
|
||||||
|
exit 1
|
||||||
|
fi
|
||||||
|
done
|
||||||
|
|
||||||
# The baked-in healthcheck must drive Docker's health state on its
|
# The baked-in healthcheck must drive Docker's health state on its
|
||||||
# own, the probe binary must be directly exec-able without any
|
# own, the probe binary must be directly exec-able without any
|
||||||
|
|||||||
@@ -42,7 +42,9 @@ VOLUME ["/data"]
|
|||||||
# Exec form is mandatory: there is no /bin/sh in this image. `ready`
|
# Exec form is mandatory: there is no /bin/sh in this image. `ready`
|
||||||
# rather than `live` because Docker health feeds start-order gating
|
# rather than `live` because Docker health feeds start-order gating
|
||||||
# (readiness semantics); the two are identical unless KMS/etcd are used.
|
# (readiness semantics); the two are identical unless KMS/etcd are used.
|
||||||
HEALTHCHECK --interval=30s --timeout=5s --start-period=2m --start-interval=2s --retries=3 \
|
# The outer timeout stays above the probe's own 5s deadline so the
|
||||||
|
# probe can report its diagnostic line instead of being SIGKILLed.
|
||||||
|
HEALTHCHECK --interval=30s --timeout=10s --start-period=2m --start-interval=2s --retries=3 \
|
||||||
CMD ["/usr/bin/silo", "healthcheck", "ready"]
|
CMD ["/usr/bin/silo", "healthcheck", "ready"]
|
||||||
|
|
||||||
ENTRYPOINT ["/usr/bin/silo"]
|
ENTRYPOINT ["/usr/bin/silo"]
|
||||||
|
|||||||
@@ -252,6 +252,7 @@
|
|||||||
"MINIO_FS_OSYNC",
|
"MINIO_FS_OSYNC",
|
||||||
"MINIO_GID",
|
"MINIO_GID",
|
||||||
"MINIO_GROUPNAME",
|
"MINIO_GROUPNAME",
|
||||||
|
"MINIO_HEALTHCHECK_URL",
|
||||||
"MINIO_HEAL_BITROTSCAN",
|
"MINIO_HEAL_BITROTSCAN",
|
||||||
"MINIO_HEAL_DRIVE_WORKERS",
|
"MINIO_HEAL_DRIVE_WORKERS",
|
||||||
"MINIO_HEAL_MAX_IO",
|
"MINIO_HEAL_MAX_IO",
|
||||||
|
|||||||
@@ -61,8 +61,9 @@ var healthcheckFlags = []cli.Flag{
|
|||||||
EnvVar: "MINIO_ADDRESS",
|
EnvVar: "MINIO_ADDRESS",
|
||||||
},
|
},
|
||||||
cli.StringFlag{
|
cli.StringFlag{
|
||||||
Name: "url",
|
Name: "url",
|
||||||
Usage: "probe this base URL (http[s]://HOST:PORT) instead of deriving one from --address and the certs directory",
|
Usage: "probe this base URL (http[s]://HOST:PORT) instead of deriving one from --address and the certs directory",
|
||||||
|
EnvVar: "MINIO_HEALTHCHECK_URL",
|
||||||
},
|
},
|
||||||
cli.BoolFlag{
|
cli.BoolFlag{
|
||||||
Name: "maintenance",
|
Name: "maintenance",
|
||||||
@@ -152,7 +153,8 @@ func (r healthcheckResult) line() string {
|
|||||||
|
|
||||||
// healthcheckTarget derives the base URL to probe. An explicit rawURL wins;
|
// healthcheckTarget derives the base URL to probe. An explicit rawURL wins;
|
||||||
// otherwise the address' host:port is used, with the scheme decided by the
|
// otherwise the address' host:port is used, with the scheme decided by the
|
||||||
// same certificate presence check the server performs at startup.
|
// same certificate presence check the server performs at startup. URLs are
|
||||||
|
// serialized via url.URL so IPv6 zone identifiers survive as %25-escapes.
|
||||||
func healthcheckTarget(rawURL, address, certsDir string) (string, error) {
|
func healthcheckTarget(rawURL, address, certsDir string) (string, error) {
|
||||||
if rawURL != "" {
|
if rawURL != "" {
|
||||||
u, err := url.Parse(rawURL)
|
u, err := url.Parse(rawURL)
|
||||||
@@ -162,7 +164,7 @@ func healthcheckTarget(rawURL, address, certsDir string) (string, error) {
|
|||||||
if (u.Scheme != "http" && u.Scheme != "https") || u.Host == "" {
|
if (u.Scheme != "http" && u.Scheme != "https") || u.Host == "" {
|
||||||
return "", fmt.Errorf("invalid --url %q: expected http[s]://HOST:PORT", rawURL)
|
return "", fmt.Errorf("invalid --url %q: expected http[s]://HOST:PORT", rawURL)
|
||||||
}
|
}
|
||||||
return u.Scheme + "://" + u.Host, nil
|
return (&url.URL{Scheme: u.Scheme, Host: u.Host}).String(), nil
|
||||||
}
|
}
|
||||||
|
|
||||||
host, port, err := net.SplitHostPort(address)
|
host, port, err := net.SplitHostPort(address)
|
||||||
@@ -176,7 +178,7 @@ func healthcheckTarget(rawURL, address, certsDir string) (string, error) {
|
|||||||
if isFile(filepath.Join(certsDir, publicCertFile)) && isFile(filepath.Join(certsDir, privateKeyFile)) {
|
if isFile(filepath.Join(certsDir, publicCertFile)) && isFile(filepath.Join(certsDir, privateKeyFile)) {
|
||||||
scheme = "https"
|
scheme = "https"
|
||||||
}
|
}
|
||||||
return scheme + "://" + net.JoinHostPort(host, port), nil
|
return (&url.URL{Scheme: scheme, Host: net.JoinHostPort(host, port)}).String(), nil
|
||||||
}
|
}
|
||||||
|
|
||||||
// probeHealth performs one bounded, strictly anonymous GET against the
|
// probeHealth performs one bounded, strictly anonymous GET against the
|
||||||
@@ -282,8 +284,8 @@ func healthcheckMain(ctx *cli.Context) {
|
|||||||
|
|
||||||
res := probeHealth(baseURL, check, ctx.Bool("maintenance"), timeout)
|
res := probeHealth(baseURL, check, ctx.Bool("maintenance"), timeout)
|
||||||
|
|
||||||
quiet := ctx.IsSet("quiet") || ctx.GlobalIsSet("quiet")
|
quiet := ctx.Bool("quiet") || ctx.GlobalBool("quiet")
|
||||||
if ctx.IsSet("json") || ctx.GlobalIsSet("json") {
|
if ctx.Bool("json") || ctx.GlobalBool("json") {
|
||||||
buf, jerr := json.Marshal(res)
|
buf, jerr := json.Marshal(res)
|
||||||
if jerr != nil {
|
if jerr != nil {
|
||||||
fail("%v", jerr)
|
fail("%v", jerr)
|
||||||
|
|||||||
@@ -54,6 +54,9 @@ func TestHealthcheckTarget(t *testing.T) {
|
|||||||
}{
|
}{
|
||||||
{name: "default address", address: ":9000", certsDir: plainDir, want: "http://127.0.0.1:9000"},
|
{name: "default address", address: ":9000", certsDir: plainDir, want: "http://127.0.0.1:9000"},
|
||||||
{name: "explicit host", address: "10.0.0.7:9010", certsDir: plainDir, want: "http://10.0.0.7:9010"},
|
{name: "explicit host", address: "10.0.0.7:9010", certsDir: plainDir, want: "http://10.0.0.7:9010"},
|
||||||
|
{name: "ipv6 address", address: "[::1]:9000", certsDir: plainDir, want: "http://[::1]:9000"},
|
||||||
|
{name: "ipv6 zone is escaped", address: "[fe80::1%eth0]:9000", certsDir: plainDir, want: "http://[fe80::1%25eth0]:9000"},
|
||||||
|
{name: "ipv6 zone in url", rawURL: "http://[fe80::1%25eth0]:9000", certsDir: plainDir, want: "http://[fe80::1%25eth0]:9000"},
|
||||||
{name: "tls certs present", address: ":9000", certsDir: tlsDir, want: "https://127.0.0.1:9000"},
|
{name: "tls certs present", address: ":9000", certsDir: tlsDir, want: "https://127.0.0.1:9000"},
|
||||||
{name: "cert without key stays http", address: ":9000", certsDir: halfDir, want: "http://127.0.0.1:9000"},
|
{name: "cert without key stays http", address: ":9000", certsDir: halfDir, want: "http://127.0.0.1:9000"},
|
||||||
{name: "url override wins", rawURL: "https://silo.internal:9000", address: ":9000", certsDir: plainDir, want: "https://silo.internal:9000"},
|
{name: "url override wins", rawURL: "https://silo.internal:9000", address: ":9000", certsDir: plainDir, want: "https://silo.internal:9000"},
|
||||||
|
|||||||
@@ -10,12 +10,12 @@ The `silo` binary can probe those endpoints itself, which makes health checking
|
|||||||
silo healthcheck [FLAGS] [live|ready|cluster|cluster-read]
|
silo healthcheck [FLAGS] [live|ready|cluster|cluster-read]
|
||||||
```
|
```
|
||||||
|
|
||||||
The check name maps 1:1 onto `/minio/health/<path>`; `live` is the default. The exit code is `0` when healthy and `1` otherwise, and one diagnostic line (including the `x-minio-server-status` and quorum headers on failure) is printed for `docker inspect` to capture. The probe target is derived the same way the server derives its own listen address — `--address` / `MINIO_ADDRESS`, with HTTPS auto-detected from `public.crt` and `private.key` in `--certs-dir` — or overridden wholesale with `--url`. Certificate verification is skipped, matching the kubelet's behavior for HTTPS probes.
|
The check name maps 1:1 onto `/minio/health/<path>`; `live` is the default. The exit code is `0` when healthy and `1` otherwise, and one diagnostic line (including the `x-minio-server-status` and quorum headers on failure) is printed for `docker inspect` to capture. The probe target is derived the same way the server derives its own listen address — `--address` / `MINIO_ADDRESS`, with HTTPS auto-detected from `public.crt` and `private.key` in `--certs-dir` — or overridden wholesale with `--url` / `MINIO_HEALTHCHECK_URL`. The environment form exists for containers with a baked-in `HEALTHCHECK`: a probe process cannot see the server's command line, so when the server's address or TLS setup comes from CLI arguments rather than the environment, set `MINIO_HEALTHCHECK_URL` (e.g. `https://127.0.0.1:9010`) to point the built-in probe at it. Certificate verification is skipped, matching the kubelet's behavior for HTTPS probes.
|
||||||
|
|
||||||
Use it as an image `HEALTHCHECK` (exec form, since there may be no shell):
|
Use it as an image `HEALTHCHECK` (exec form, since there may be no shell; keep the outer timeout above the probe's own 5s deadline so its diagnostic line survives):
|
||||||
|
|
||||||
```
|
```
|
||||||
HEALTHCHECK --interval=30s --timeout=5s --start-period=2m --start-interval=2s --retries=3 \
|
HEALTHCHECK --interval=30s --timeout=10s --start-period=2m --start-interval=2s --retries=3 \
|
||||||
CMD ["/usr/bin/silo", "healthcheck", "ready"]
|
CMD ["/usr/bin/silo", "healthcheck", "ready"]
|
||||||
```
|
```
|
||||||
|
|
||||||
@@ -25,7 +25,7 @@ or as a Docker Compose healthcheck:
|
|||||||
healthcheck:
|
healthcheck:
|
||||||
test: ["CMD", "/usr/bin/silo", "healthcheck", "ready"]
|
test: ["CMD", "/usr/bin/silo", "healthcheck", "ready"]
|
||||||
interval: 5s
|
interval: 5s
|
||||||
timeout: 5s
|
timeout: 10s
|
||||||
retries: 5
|
retries: 5
|
||||||
```
|
```
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user