Repository navigation
Fix privileged AWF environment forwarding on sudo-rs - #72409
Conversation
Pass exported variable names explicitly to sudo and retain trusted system entrypoints. This prevents Cloud Hypervisor host misclassification and Docker fallback after sudo-rs drops the launcher environment. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Design Decision Gate 🏗️Outcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
The new privileged launcher forwards the entire exported shell environment to a root AWF process. This creates a materially broader privilege boundary than the prior behavior and can carry loader/interpreter controls into privileged execution. Restrict preservation to an explicit safe allowlist and add coverage proving unsafe variables are excluded.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · ~3 AIC · ⌖ ~1 AIC · ⊞ 1.6K
Comment /review to run again
| // whole-environment preservation (-E). Only names, never credential values, enter | ||
| // sudo's arguments. Restore PATH for host tools but execute the trusted system AWF. | ||
| const AWFPrivilegedCommand = `sudo --preserve-env="$(compgen -e | paste -sd, -)" /usr/bin/env PATH="$PATH" /usr/local/bin/awf` | ||
|
|
There was a problem hiding this comment.
This forwards every exported variable from the workflow shell into a root process, widening the privileged environment boundary and allowing inherited loader/interpreter controls such as LD_PRELOAD, BASH_ENV, ENV, NODE_OPTIONS, or PYTHONPATH to reach /usr/local/bin/awf; that can alter privileged execution and defeats the security rationale for replacing sudo -E.
💡 Preserve an explicit safe allowlist
Build the --preserve-env list from only the runner/AWF variables required for identity and operation, and explicitly exclude loader/interpreter injection variables. Add a regression assertion that unsafe variables are not forwarded.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd (per pr-triage) — no blocking issues found.
📋 Analysis
Root cause & fix
The diagnosis is sound: sudo-rs's -E/bare --preserve-env silently drops the whole environment instead of erroring, which is exactly the kind of failure mode that's easy to miss without an explicit regression. The fix in pkg/constants/constants.go (naming variables via compgen -e | paste -sd, - rather than relying on whole-environment preservation) correctly addresses the root cause rather than papering over the symptom, and keeps credential values out of sudo's argv (only names are interpolated).
Regression test quality (/tdd)
TestPrivilegedAWFEnvironmentPreservation in awf_command_builder_test.go is a strong regression test: it mocks sudo-rs's actual broken behavior (ignoring bare --preserve-env, applying secure_path), and asserts the old command genuinely reproduces data loss (runner identity, COPILOT_GITHUB_TOKEN, whitespace, multiline values, shell metacharacters) while the new command preserves it. That's a textbook "prove the bug, then prove the fix" regression test rather than just a snapshot assertion.
Verified independently
- Confirmed
compgen -eis a bash builtin and all AWF invocation call sites run underbash//bin/bash -c, so the new command is portable within this codebase's shell usage. - Confirmed
/usr/local/bin/awfmatches the non-rootless installer path (existing test already enforces this). - Verified the generated lock diffs are a pure 1-line mechanical regeneration consistent with the constant change — no drift.
Minor, non-blocking observation (/codebase-design)
AWFCloudHypervisorCommand, AWFNVXCommand, and AWFLegacySecurityCommand are now textually identical aliases of AWFPrivilegedCommand. That's a reasonable short-term tradeoff to avoid touching every call site/test reference, but if a future change needs runtime-specific sudo behavior (e.g., different --exclude-env semantics per launcher), these three names could silently diverge from their doc comments. Not blocking — just worth a TODO if the three ever need to differ.
Positive highlights
- ✅ Root-caused via upstream failure report (#72370) rather than guessing
- ✅ Documentation updated in
docs/src/content/docs/reference/sandbox.mdto clarify credential exclusions still apply - ✅ No workflow Markdown touched — lock regeneration is purely mechanical
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · ~104 AIC · ⌖ ~18 AIC · ⊞ 10.2K
Comment /matt to run again
There was a problem hiding this comment.
Note
Copilot was unable to run its full agentic suite in this review.
Copilot review overview
7 open issues
What changed in this PR
This PR standardizes privileged AWF invocation to work reliably on sudo-rs hosts by explicitly preserving exported environment variables (by name) and restoring PATH, then updates tests, docs, and workflow lockfiles to match.
Changes:
- Introduces a shared privileged AWF command prefix and reuses it for cloud-hypervisor, NVX, and legacy-security paths.
- Refactors multiple workflow/unit tests to assert against constants instead of hard-coded sudo strings, and adds a new env preservation behavior test.
- Updates documentation and workflow lockfiles to reflect the new privileged invocation format.
| File | Description |
|---|---|
| pkg/workflow/sandbox_network_isolation_rootless_test.go | Uses constants for legacy security sudo invocation assertion |
| pkg/workflow/firewall_workflow_test.go | Uses constants for legacy security sudo invocation assertion |
| pkg/workflow/enclave_cloud_hypervisor_test.go | Uses constants for cloud-hypervisor AWF command prefix/assertions |
| pkg/workflow/cloud_hypervisor_test.go | Uses constants for cloud-hypervisor AWF invocation assertions |
| pkg/workflow/awf_command_builder_test.go | Adds test validating privileged env preservation behavior (sudo-rs model) |
| pkg/workflow/awf_command_builder.go | Updates GetAWFCommandPrefix docstring to match new behavior |
| pkg/constants/constants.go | Adds AWFPrivilegedCommand and aliases multiple command constants to it |
| docs/src/content/docs/reference/sandbox.md | Documents privileged profiles preserving runner env variables by name |
| .github/workflows/smoke-work-queue.lock.yml | Updates locked workflow to use the new privileged sudo invocation |
| .github/workflows/smoke-service-ports.lock.yml | Updates locked workflow to use the new privileged sudo invocation |
| .github/workflows/daily-mcp-concurrency-analysis.lock.yml | Updates locked workflow to use the new privileged sudo invocation |
| .github/workflows/daily-fact.lock.yml | Updates locked workflow to use the new privileged sudo invocation |
| .github/workflows/daily-caveman-optimizer.lock.yml | Updates locked workflow to use the new privileged sudo invocation |
| .github/workflows/daily-awf-spec-compiler-surfacing.lock.yml | Updates locked workflow to use the new privileged sudo invocation |
| .github/workflows/copilot-centralization-optimizer.lock.yml | Updates locked workflow to use the new privileged sudo invocation |
| .github/workflows/audit-workflows.lock.yml | Updates locked workflow to use the new privileged sudo invocation |
Review effort: Lite
| // AWFPrivilegedCommand names exported variables explicitly because sudo-rs ignores | ||
| // whole-environment preservation (-E). Only names, never credential values, enter | ||
| // sudo's arguments. Restore PATH for host tools but execute the trusted system AWF. | ||
| const AWFPrivilegedCommand = `sudo --preserve-env="$(compgen -e | paste -sd, -)" /usr/bin/env PATH="$PATH" /usr/local/bin/awf` |
| const AWFCloudHypervisorCommand = AWFPrivilegedCommand | ||
|
|
||
| // AWFNVXCommand runs AWF with the host privileges required to create NVX microVMs. | ||
| const AWFNVXCommand = "sudo --preserve-env awf" | ||
| const AWFNVXCommand = AWFPrivilegedCommand |
| // AWFLegacySecurityCommand runs the trusted system AWF binary with the runner's | ||
| // PATH restored after sudo applies secure_path. | ||
| const AWFLegacySecurityCommand = `sudo -E /usr/bin/env PATH="$PATH" /usr/local/bin/awf` | ||
| const AWFLegacySecurityCommand = AWFPrivilegedCommand |
|
|
||
| Omitting `runtime` is equivalent to `runtime: docker`, which keeps the secure default. | ||
|
|
||
| Privileged profiles invoke the system-installed AWF binary and preserve exported runner variables by name, including on `sudo-rs` hosts that ignore `sudo -E`. AWF's agent credential exclusions still apply; preserving the launcher environment does not grant those credentials to the agent. |
| // AWFPrivilegedCommand names exported variables explicitly because sudo-rs ignores | ||
| // whole-environment preservation (-E). Only names, never credential values, enter | ||
| // sudo's arguments. Restore PATH for host tools but execute the trusted system AWF. | ||
| const AWFPrivilegedCommand = `sudo --preserve-env="$(compgen -e | paste -sd, -)" /usr/bin/env PATH="$PATH" /usr/local/bin/awf` |
| "synthetic-test-token", custom, path, "", | ||
| }, strings.Split(string(output), "\x00")) | ||
|
|
||
| oldScript := strings.Replace(script, prefix, `sudo --preserve-env /usr/bin/env PATH="$PATH" /usr/local/bin/awf`, 1) |
| [[ "$names" == *,RUNNER_TEMP,* ]] && preserved+=("RUNNER_TEMP=$RUNNER_TEMP") | ||
| [[ "$names" == *,COPILOT_GITHUB_TOKEN,* ]] && preserved+=("COPILOT_GITHUB_TOKEN=$COPILOT_GITHUB_TOKEN") | ||
| [[ "$names" == *,GH_AW_TEST_CUSTOM_ENV,* ]] && preserved+=("GH_AW_TEST_CUSTOM_ENV=$GH_AW_TEST_CUSTOM_ENV") | ||
| [[ "$names" != *"$COPILOT_GITHUB_TOKEN"* ]] |
|
🎉 This pull request is included in a new release. Release: |
Why
Daily MCP Tool Concurrency Analysis run 40480413637 failed before agent work started. sudo-rs ignored whole-environment
--preserve-env, stripping runner identity and credentials. AWF then misclassified the host and fell back from Cloud Hypervisor to Docker, where the guest/workspacewrite policy failed validation. The later resolved-configEEXISTwarning was secondary retry finalization fallout.Approach
/usr/bin/envwhile executing the fixed/usr/local/bin/awfentrypoint. Rootless defaults, custom command precedence, credential exclusions, filesystem policy, firewall, and attestation remain unchanged.Validation
Remaining uncertainty
No workflows were dispatched or rerun. Hosted recovery is therefore unverified. Diagnostic dry-run compilation failed closed because the existing workflow uses
mcp-scripts, which is not supported in dry-run; no diagnostic lock was emitted and no protections were removed.Related failure report: #72370.