feat(workstation): Remove strict sudo requirement on workstation#2668
feat(workstation): Remove strict sudo requirement on workstation#2668rmvangun wants to merge 22 commits into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…orms - Added RevertGuest method to ColimaNetworkManager to remove in-VM iptables FORWARD rules. - Implemented RevertHostRoute and RevertDNS methods in BaseNetworkManager for macOS, Linux, and Windows to undo host route and DNS configurations. - Enhanced NetworkManager interface to include Revert* methods for host route, guest, and DNS. - Updated Workstation to call Revert methods during network reversion, ensuring idempotency and proper error handling. - Added comprehensive unit tests for all new Revert methods across different platforms to ensure expected behavior and error handling.
…n and add hints for user guidance
…ry-run and --revert options
…t onApply hooks returning (bool, error)
…t for elevation requirements
…work for network configuration
…tion and add tests
…r sudo on macOS/Linux and require Administrator PowerShell on Windows
…installation and suppress success message
…rate them into routing configurations
…rrors and persist DNS address
…e optional outcomes
…n state and update configure command precondition
rmvangun
force-pushed
the
feat/sudo-out-of-up
branch
from
a7d6c62 to
1676ab3
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } | ||
| contextName := n.configHandler.GetContext() | ||
| profileName := fmt.Sprintf("windsor-%s", contextName) | ||
| deleteCommand := fmt.Sprintf("sudo iptables -t filter -D FORWARD -i col0 -o %s -s %s -d %s -j ACCEPT 2>/dev/null </dev/null", outputInterface, hostIP, cidr) |
There was a problem hiding this comment.
Interface name from VM interpolated into sh -c without format validation
Low Severity · pkg/workstation/network/colima_network.go:187
This PR adds validateCIDR and validateIPAddress to guard values going into shell commands, but outputInterface — discovered by scanning ls /sys/class/net inside the VM — is interpolated into the sh -c delete command without a parallel check. Linux's IFNAMSIZ limit (15 usable characters) makes a working payload very hard to construct, and the interface must also begin with br-, so this is low-risk in practice. A simple allowlist check in the bridge-discovery loop (strings.ContainsAny(iface, " ;$'\"") returning an error) would complete the defense-in-depth pattern established elsewhere in this diff.
|
@copilot resolve the merge conflicts in this pull request |
Co-authored-by: rmvangun <85766511+rmvangun@users.noreply.github.com>
Resolved the merge conflict and pushed the fix in commit Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Note
Medium Risk
This PR touches core CLI commands (
up,down,bootstrap,configure), the provisioner'sonApplyhook interface, and the DNS config API — any of which could affect a broad set of operators on upgrade.Overview
The change removes the eager
EnsureNetworkPrivilegecall fromwindsor up, replacing it with a halt-signal mechanism: when the workstation Terraform component applies and cluster reachability would require a host route, theonApplyhook returns(true, nil)to stop further component applies cleanly and surface aprintDeferredWorksummary pointing the operator atwindsor configure network. Theconfigure networkcommand gains--dry-runand--revertflags, and a precondition gate keyed on the presence ofworkstation.yaml. Thedns.enabledfield is removed from both config API versions; DNS is now configured wheneverdns.domainand the resolver address are both present.The
dns.enabledremoval is a silent behavioral change: anywindsor.yamlthat carriesdns.enabled: falsetoday will have DNS configuration activated after upgrade ifdns.domainand the address are also set. The help text example--set dns.enabled=falsewas removed, but no migration notice is included. Operators who relied on this flag to suppress DNS forwarding should verify their configs. The halt-signal threading throughStack → Provisioner → Project → cmdis internally consistent;cmd/bootstrap.gocorrectly guardsprintDeferredWorkwith a nil check thatcmd/up.goandcmd/down.godo not need (both already return early whenWorkstationis nil).Reviewed by Claude for commit
1676ab38.