Repository navigation
Serve DHCP directly from Softnet and use predictable vmnet reservations - #209
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15c7557399
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Add private address space (as defined in RFC 1918[1]) | ||
| // | ||
| // [1]: https://datatracker.ietf.org/doc/html/rfc1918#section-3 | ||
| let private_subnets: Vec<Ipv4Net> = vec
There was a problem hiding this comment.
nit: no action needed but just wanted to understand
were you able to test if vmnet supports the 10.0.0.0/8 and 172.16.0.0/12 subnets? I recall only testing 192.168.0.0/16 but just wanted to confirm.
There was a problem hiding this comment.
Yes, all of them work.
vmnet seems to actually validate these subnets, because e.g. 192.169.0.0/16 results in an error.
| // Figure out which address space is already utilized on the host | ||
| let mut used_subnets = Vec::new(); | ||
|
|
||
| for interface in NetworkInterface::show()? { |
There was a problem hiding this comment.
nit: no action needed but just wanted to call this out
Since this is taking a snapshot of the current interfaces, and we don't refresh this on each retry, there could a super slim chance of a subnet collision with another softnet process that happens to want the same subnet. The retry should handle it, but I'm wondering if we should refresh the interface list on the retry attempt or not. Not a big deal.
There was a problem hiding this comment.
Nice catch.
Given macOS's two-VM limit, we could probably get away without NetworkInterface::show() altogether.
But let's keep it for now as a middle ground.
|
I may be missing something, but the note that default shared NAT can still provide inter-VM communication seems inconsistent with current func attachments() -> [VZNetworkDeviceAttachment] {
[VZNATNetworkDeviceAttachment()]
}There is no Tart-managed common virtual network or bridge created here that attaches multiple VMs to the same L2 segment. Softnet’s current README states that What is the currently recommended Tart setup for connecting 2 or more VMs to the same network while keeping Internet access?
|
This works on latest macOS Golden Gate, though, see openai/tart#1289 (comment). It seems that Apple has recently started enabling interface isolation by default for If there would be a way to disable it, this could solve your use-case easily. Perhaps, worth submitting a bug via Feedback Assistant? |
4 participants
The core idea here is to reduce host's attack surface by not allowing the VM to communicate its DHCP server at all.
To achieve that, we use
vmnet_network_configuration_add_dhcp_reservation(_:_:_:)to get a predictable IPv4 credentials when constructing avmnetinterface, and then we just these credentials over a small DHCP server directly built into Softnet.The nice thing is that
vmnetprevents IPv4 network collisions for us, so we just pick a random/30from a private IPv4 range, similarly to how Vetu does it.Also t's pretty easy to support this in Tart as well: openai/tart#1355.
The only downsides are:
--allow=0.0.0.0/0no longer enables inter-VM communication because each VM now uses a different bridge interfaceResolves #197, resolves #207.