fix: legacy-security mode installs awf non-rootless, uses plain sudo for log parsing#48098
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…do for log parsing When `legacy-security: enable` is set, the AWF execution step correctly uses `sudo -E awf` (which requires the binary to be in sudo's secure_path, i.e. /usr/local/bin). However, two other steps incorrectly added `--rootless`: 1. The `install_awf_binary.sh` step passed `--rootless` (installing to ~/.local/bin), making `sudo awf` fail with "command not found". 2. The `print_firewall_logs.sh` step passed `--rootless` (using non-interactive sudo with a non-sudo fallback), even though AWF had full sudo access. Fix: In both `generateAWFInstallationStep` and `generateFirewallLogParsingStep`, skip `--rootless` when `LegacySecurity` is true, even if `NetworkIsolation` is also true (which is the default). This also fixes the compiled `.github/workflows/smoke-service-ports.lock.yml` which already used `legacy-security: enable`. Closes #47804 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix is correct and well-scoped. Both generateAWFInstallationStep and generateFirewallLogParsingStep now correctly skip --rootless when LegacySecurity is true, matching the documented behavior that sudo -E awf requires the binary in sudos secure_path. The reuse of getAgentConfig in engine_firewall_support.go is consistent with the existing pattern. Tests cover the unit cases and an end-to-end compilation check.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 17.4 AIC · ⌖ 5.14 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — one minor observation on the new code; no blocking issues.
📋 Key Themes & Highlights
Findings
- Dead nil guard (
engine_firewall_support.go:139): theagentCfg == nilbranch is unreachable — see inline comment.
Positive Highlights
- ✅ Root cause correctly identified and fixed in both affected callsites
- ✅ Regression tests added for the
LegacySecurity=true+NetworkIsolation=truecombination in three different test files - ✅ End-to-end compilation test (
TestLegacySecurityInstallNonRootless) verifies the lock file output end-to-end - ✅
smoke-service-ports.lock.ymlregenerated — confirming the previously silently broken workflow is now correct - ✅ Comment in
copilot_engine_installation.goclearly documents thesudo secure_pathconstraint
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 26.1 AIC · ⌖ 4.67 AIC · ⊞ 6.7K
Comment /matt to run again
| scriptArg := "" | ||
| if isAWFNetworkIsolationEnabled(workflowData) { | ||
| agentCfg := getAgentConfig(workflowData) | ||
| if isAWFNetworkIsolationEnabled(workflowData) && (agentCfg == nil || !agentCfg.LegacySecurity) { |
There was a problem hiding this comment.
[/diagnosing-bugs] The agentCfg == nil guard is dead code — isAWFNetworkIsolationEnabled already returns false when agentCfg is nil (see firewall.go lines 154-157), so the outer condition is already false before the nil check is evaluated.
💡 Suggested simplification
// agentCfg is guaranteed non-nil when isAWFNetworkIsolationEnabled returns true
agentCfg := getAgentConfig(workflowData)
if isAWFNetworkIsolationEnabled(workflowData) && !agentCfg.LegacySecurity {
scriptArg = " --rootless"
}Removing the unreachable branch makes the invariant explicit and prevents future readers from thinking nil is a valid outcome here.
@copilot please address this.
Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (>100 new lines in Draft ADR committed:
What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel Report
📊 Metrics (6 tests)
|
There was a problem hiding this comment.
Review: legacy-security non-rootless install fix
The core fix is correct — LegacySecurity mode must install AWF to /usr/local/bin (non-rootless) so sudo -E awf can find it, and the firewall log parsing step must match. Two issues flagged; neither is a blocker but one warrants attention.
Findings
Medium — Dead nil guard in engine_firewall_support.go:139
The agentCfg == nil branch in (agentCfg == nil || !agentCfg.LegacySecurity) is unreachable. isAWFNetworkIsolationEnabled already returns false when agentConfig is nil, so the outer && short-circuits before the inner || is evaluated. This is not a correctness bug today, but it misleads readers and could silently regress if isAWFNetworkIsolationEnabled is refactored. See inline comment.
Low — TestLegacySecurityInstallNonRootless relies on implicit network-isolation defaults
The test frontmatter does not declare network-isolation: true explicitly, meaning the test only exercises the LegacySecurity exception if network isolation happens to be the default. See inline comment.
Out-of-diff observation (no comment posted)
Other callers of isAWFNetworkIsolationEnabled — notably difc-proxy host selection in awf_helpers.go:738 and compiler_difc_proxy.go:266/585 — do not have a LegacySecurity exception. For legacy-security+network-isolation, AWF runs on the host network (non-rootless), so the difc-proxy should route via host.docker.internal rather than awmg-cli-proxy. This PR does not touch those files; worth a follow-up issue.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 63.7 AIC · ⌖ 4.87 AIC · ⊞ 5.7K
Comment /review to run again
| scriptArg := "" | ||
| if isAWFNetworkIsolationEnabled(workflowData) { | ||
| agentCfg := getAgentConfig(workflowData) | ||
| if isAWFNetworkIsolationEnabled(workflowData) && (agentCfg == nil || !agentCfg.LegacySecurity) { |
There was a problem hiding this comment.
Dead nil guard creates misleading logic: the agentCfg == nil branch in the new condition is unreachable, since isAWFNetworkIsolationEnabled already returns false when agentConfig == nil. This is not a crash, but it obscures intent and could mask a future regression if isAWFNetworkIsolationEnabled is refactored.
💡 Explanation and suggested fix
isAWFNetworkIsolationEnabled (firewall.go) checks agentConfig == nil first and returns false immediately, so the outer && already guarantees agentCfg is non-nil when the inner condition is reached. Simplify to:
agentCfg := getAgentConfig(workflowData)
if isAWFNetworkIsolationEnabled(workflowData) && !agentCfg.LegacySecurity {
scriptArg = " --rootless"
}If you prefer to keep the nil guard for defensive style, add a comment explaining the invariant so future readers don’t get confused.
| strict: false | ||
| network: | ||
| allowed: | ||
| - github.com |
There was a problem hiding this comment.
TestLegacySecurityInstallNonRootless relies on implicit network-isolation defaults: the test frontmatter omits network-isolation: true and id: awf with an explicit NetworkIsolation field, so the --rootless suppression is exercised only if network isolation happens to be enabled by default. If the default changes, the test silently stops covering the LegacySecurity exception.
💡 Suggested improvement
Make the network-isolation dependency explicit in the test frontmatter:
sandbox:
agent:
id: awf
network-isolation: true # explicit — required for --rootless suppression test
legacy-security: enableThis documents the invariant under test and prevents silent test staleness if the default changes.
github-actions
Bot
added
pr-agent:copilot
pr-priority:medium
pr-risk:medium
pr-type:bug
labels
🤖 PR Triage — §30192186368
Why: Fixes #47804 — Next: Verify CI passes, then fast-track merge.
|
|
@copilot please run the Unresolved review feedback / follow-up:
Please reply with the specific remaining blocker, if any, after checking the current branch state.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
🤖 PR Triage
Legacy-security mode agent startup fix (sudo path issue). CI mostly passing. ADR included.
|
|
@copilot please run the Recent context:
|
|
@copilot please run the
|
🤖 PR Triage — §30214847023
Rationale: Legacy-security mode: AWF installed non-rootless, plain sudo for log parsing. 164 additions in 7 files including Go engine/firewall code, tests, ADR, and lock.yml. CI passing (2 runs). AI reviewers approved. Ready for expedited human review.
|
|
@copilot please run the skill, review the current branch state, address any still-open reviewer feedback and failed checks, and reply with whether this PR is ready for maintainer review. Run: https://github.com/github/gh-aw/actions/runs/30215321924
|
legacy-security: enableopts the agent job intosudo -E awf, but the install step unconditionally passed--rootlesswhenNetworkIsolation=true(the default), placing the binary in~/.local/bin— outside sudo'ssecure_path. Result: agent startup fails withsudo: awf: command not found. Theprint_firewall_logs.shstep had the same logic flaw.Changes
pkg/workflow/copilot_engine_installation.go—generateAWFInstallationStep: skip--rootlesswhenagentConfig.LegacySecurityis true, so the binary lands in/usr/local/binpkg/workflow/engine_firewall_support.go—generateFirewallLogParsingStep: skip--rootlesswhenLegacySecurityis true; AWF already has full sudo access in this mode.github/workflows/smoke-service-ports.lock.yml— regenerated (this workflow was already usinglegacy-security: enableand was silently broken)LegacySecurity=true+NetworkIsolation=truecombination, plus an end-to-end compilation testThe condition in both places went from:
to:
sudo -E awfbut installs awf with --rootless, so the agent fails at startup withsudo: awf: command not found#47804