Repository navigation
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The downloaded repository key remains unauthenticated, and the security behavior lacks regression coverage.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Hardens Docker sbx installation by replacing remote root-shell execution with explicit apt repository configuration.
Changes:
- Adds Docker’s signing key and signed apt source directly.
- Documents the security hardening in a patch changeset.
| File | Description |
|---|---|
actions/setup/sh/sudo_docker_sbx_install.sh |
Replaces curl | sudo sh with apt repository setup. |
.changeset/harden-docker-sbx-install-apt-repo.md |
Records the installation hardening. |
| echo "Adding Docker apt repository for ${DISTRO_ID} ${DISTRO_CODENAME}..." | ||
| sudo install -m 0755 -d /etc/apt/keyrings | ||
| # runner-guard:ignore RGS-012 -- fetches Docker's GPG signing key to a file (never piped to a shell); the key is used only to verify the signed-by apt repository below. | ||
| curl -fsSL "https://download.docker.com/linux/${DISTRO_ID}/gpg" -o /tmp/docker.asc |
There was a problem hiding this comment.
Implemented in 820506d: the downloaded key is verified with gpg --show-keys against the pinned Docker fingerprint before installation. The inline rotation instruction requires verifying any replacement against Docker's published signing-key documentation.
| # runner-guard:ignore RGS-012 -- fetches Docker's GPG signing key to a file (never piped to a shell); the key is used only to verify the signed-by apt repository below. | ||
| curl -fsSL "https://download.docker.com/linux/${DISTRO_ID}/gpg" -o /tmp/docker.asc |
There was a problem hiding this comment.
Implemented in 820506d: TestDockerSbxShellScriptContent now requires the fingerprint verification, signed-by source, secure temporary file, and Docker download endpoint, while rejecting the legacy endpoint and pipe-to-shell form.
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "o205451.ingest.us.sentry.io"See Network Configuration for more information.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories.
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. PR #62991 only changed production files (.changeset/ and sudo_docker_sbx_install.sh). Test Quality Sentinel skipped. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "o205451.ingest.us.sentry.io"See Network Configuration for more information.
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "o205451.ingest.us.sentry.io"See Network Configuration for more information.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "o205451.ingest.us.sentry.io"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
This hardening is still incomplete: the repository trust root is still accepted from the network without any identity check, and the regression tests still would not catch a slide back to the original curl | sudo sh behavior.
Blocking themes
- Fetching the Docker signing key into a file is better than piping a script into root, but it is still a live, unpinned trust root. A compromised key endpoint or MITM still turns into a trusted apt repository.
- The shell-script test still only looks for broad substrings like
sudoandapt-get install, so it does not actually enforce the new security property this PR is claiming. That leaves the exact regression we care about invisible to CI. - This rewrite also assumes
VERSION_CODENAMEis present and published by Docker, with no explicit guard. When that assumption is false the job fails later as a genericapt-get updateerror instead of surfacing a supported-platform check.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
o205451.ingest.us.sentry.io
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "o205451.ingest.us.sentry.io"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 31.3 AIC · ⌖ 7.23 AIC · ⊞ 20.3K
Comment /review to run again
| sudo install -m 0644 /tmp/docker.asc "${KEYRING_PATH}" | ||
| rm -f /tmp/docker.asc | ||
|
|
||
| echo "deb [arch=$(dpkg --print-architecture) signed-by=${KEYRING_PATH}] https://download.docker.com/linux/${DISTRO_ID} ${DISTRO_CODENAME} stable" \ |
There was a problem hiding this comment.
This hard-codes ${DISTRO_CODENAME} into the apt source without validating that Docker publishes that suite, so on runners or derivatives where the field is empty or unsupported this step now dies at apt-get update and takes every docker-sbx workflow down.
💡 Why this matters and how to harden it
The old get.docker.com path handled distro detection internally; this replacement moved that responsibility into our script but never added the guardrails. At minimum, fail fast with a clear error when VERSION_CODENAME is empty, and preferably verify that the Docker repo exposes dists/${DISTRO_ID}/dists/${DISTRO_CODENAME} before writing the source line.
There was a problem hiding this comment.
Implemented in 820506d: the installer now validates non-empty ID and VERSION_CODENAME values before writing the apt source, failing with a clear error when either is unavailable.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs. Two other Copilot review comments already flag the key issues (unverified key fingerprint, weak test regression coverage) — I focused on a gap not yet raised.
📋 Key Themes & Highlights
Key Themes
- Predictable
/tmppath (line 28):/tmp/docker.ascis a fixed, guessable filename — a TOCTOU/symlink race on shared runners could poison the very trust root this PR hardens against. Suggestedmktemp+trapcleanup. - Two existing Copilot comments already correctly call out (1) the missing pinned-fingerprint verification of the downloaded GPG key, and (2) the test suite (
TestDockerSbxShellScriptContent) not asserting against the new signed-repo flow or regression tocurl | sudo sh. These remain valid and should be addressed together with the/tmpfix.
Positive Highlights
- ✅ Removes the unversioned
curl | sudo shroot install — clear net security improvement over the previous pattern. - ✅ Consistent with the existing hardened pattern in
install_gh_cli.sh(fetch-key-to-file +signed-bysource line). - ✅ Includes a changeset and an explicit
runner-guard:ignorejustification.
@copilot please address the review comments above.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
o205451.ingest.us.sentry.io
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "o205451.ingest.us.sentry.io"See Network Configuration for more information.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 40.6 AIC · ⌖ 14.5 AIC · ⊞ 10.1K
Comment /matt to run again
| echo "Adding Docker apt repository for ${DISTRO_ID} ${DISTRO_CODENAME}..." | ||
| sudo install -m 0755 -d /etc/apt/keyrings | ||
| # runner-guard:ignore RGS-012 -- fetches Docker's GPG signing key to a file (never piped to a shell); the key is used only to verify the signed-by apt repository below. | ||
| curl -fsSL "https://download.docker.com/linux/${DISTRO_ID}/gpg" -o /tmp/docker.asc |
There was a problem hiding this comment.
[/diagnosing-bugs] /tmp/docker.asc is a predictable, fixed filename in a world-writable shared directory — on a multi-tenant or reused runner this is a classic TOCTOU/symlink race: another process (or a prior failed run) could pre-create/replace that path before install -m 0644 reads it, silently poisoning the trust root this script is trying to harden.
💡 Suggested fix
Use mktemp for the scratch file instead of a fixed /tmp path:
KEY_TMP="$(mktemp)"
trap 'rm -f "$KEY_TMP"' EXIT
curl -fsSL "(download.docker.com/redacted) -o "$KEY_TMP"
sudo install -m 0644 "$KEY_TMP" "${KEYRING_PATH}"This closes the same class of predictable-path risk the PR is otherwise fixing (unverified root-trust material), and is consistent with treating /tmp as untrusted on shared runners.
@copilot please address this.
There was a problem hiding this comment.
Implemented in 820506d: the downloaded key now uses mktemp and an EXIT cleanup trap, eliminating the predictable shared /tmp/docker.asc path.
There was a problem hiding this comment.
Impeccable review — harden + audit
This PR's core goal (replacing curl | sudo sh with a deterministic apt repo setup) is a solid direction, but the new implementation has issues that undermine the hardening intent.
Blocking
- Predictable
/tmpintermediate file (sudo_docker_sbx_install.sh:28-29): the GPG key is fetched unprivileged to a fixed/tmp/docker.ascpath and then copied bysudo installinto the trust-root keyring. A pre-existing symlink at that path would let an unprivileged local actor redirect a privileged write.install_gh_cli.shavoids this by piping the download straight intosudo dd of=...with no intermediate world-writable file — recommend mirroring that, or usingmktempfor a private temp path. - Key is unauthenticated before becoming the trust root (existing Copilot comment, still open):
signed-byscopes trust to this key but nothing here verifies the downloaded key's identity/fingerprint, so a compromiseddownload.docker.comresponse (or MITM without TLS pinning beyond ambient CA trust) becomes silently trusted. Consider verifying against Docker's published fingerprint before installing it as a keyring.
Non-blocking
- Test coverage gap (existing Copilot comment, still open):
TestDockerSbxShellScriptContentforsudo_docker_sbx_install.shonly checks forsudo,apt-get install,docker-sbx,sbx version,/dev/kvm— none of which distinguish the new deterministic-apt-repo approach from the oldcurl | sudo shimplementation. Add assertions (e.g. forsigned-by=,KEYRING_PATH, absence ofget.docker.com) so a regression back to piping-into-root-shell would fail the test.
Good use of the existing install_gh_cli.sh pattern as a reference and the changeset docs the rationale well.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
o205451.ingest.us.sentry.io
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "o205451.ingest.us.sentry.io"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 38.8 AIC · ⌖ 13.3 AIC · ⊞ 8.1K
| echo "Adding Docker apt repository for ${DISTRO_ID} ${DISTRO_CODENAME}..." | ||
| sudo install -m 0755 -d /etc/apt/keyrings | ||
| # runner-guard:ignore RGS-012 -- fetches Docker's GPG signing key to a file (never piped to a shell); the key is used only to verify the signed-by apt repository below. | ||
| curl -fsSL "https://download.docker.com/linux/${DISTRO_ID}/gpg" -o /tmp/docker.asc |
There was a problem hiding this comment.
Symlink/TOCTOU risk with predictable /tmp/docker.asc path. This introduces an unprivileged curl write to a fixed, world-writable /tmp path, followed by a privileged sudo install copy into the apt trust-root keyring (/etc/apt/keyrings/docker.asc). A local attacker/process could pre-create /tmp/docker.asc as a symlink before this step runs, and the subsequent sudo install would then copy attacker-controlled content into the apt-trusted keyring.
The sibling install_gh_cli.sh avoids this class of bug entirely by piping straight into the privileged write (curl -fsSL ... | sudo dd of="${KEYRING_PATH}"), never touching an intermediate world-writable file. Recommend the same pattern here, or at minimum use mktemp for a private, non-predictable temp file instead of the fixed /tmp/docker.asc path.
@copilot please address this.
There was a problem hiding this comment.
Implemented in 820506d: the installer now uses a private mktemp file and cleans it on exit, preventing a predictable-path symlink or TOCTOU attack.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| KEY_FINGERPRINT="$(gpg --show-keys --with-colons "${KEY_TMP}" | awk -F: '$1 == "fpr" { print $10; exit }')" | ||
| if [[ "${KEY_FINGERPRINT}" != "${DOCKER_GPG_FINGERPRINT}" ]]; then | ||
| echo "Downloaded Docker GPG key fingerprint does not match the expected fingerprint." >&2 | ||
| exit 1 | ||
| fi | ||
| sudo install -m 0644 "${KEY_TMP}" "${KEYRING_PATH}" |
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
PR-finisher pass completed in 3c476df. The installer now rejects empty keyrings and validates every primary Docker key fingerprint, with regression coverage; local formatting, lint, unit/integration tests, shellcheck, recompilation, secret scan, and final security validation passed. CI and mergeability are pending on the new head. |
|
@copilot Please move this PR forward. Open review feedback remains from: github-advanced-security and prior copilot review passes. Please refresh the branch, verify all review findings are addressed on the latest head, and run the Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
|
@copilot Open review feedback remains from copilot review passes and github-advanced-security. Please refresh the branch if needed, make sure each open review concern is addressed on the current head, and run the Current blockers still called out in review:
Please summarize the focused validation you ran after the next push.
|
sudo_docker_sbx_install.shpiped an unversioned, unverified remote script (get.docker.com) directly into a root shell to configure the Docker apt repository — a classic supply-chain risk with no pinning or checksum verification, inconsistent with the hardened pattern already used by sibling installers.Changes
curl -fsSL https://get.docker.com | sudo REPO_ONLY=1 shwith deterministic apt-repo configuration:/etc/apt/keyrings/docker.ascdeb [signed-by=...]apt source lineapt-get update && apt-get install -y docker-sbxinstall_gh_cli.sh(fetch-key-to-file + signed-by source line) and the pinned/verified download style insudo_gvisor_install.sh.runner-guard:ignore RGS-012annotation with justification, since the key is only downloaded to disk and used for apt signature verification, not executed.curl | sudo shroot install in sudo_docker_sbx_install.sh (supply-chain risk) #62521