-
Notifications
You must be signed in to change notification settings - Fork 62
[LXC] Pin the proxy hostname instead of rewriting the URL host #789
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
+552
−2
Closed
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
8223abd
[LXC] Pin the proxy hostname instead of rewriting the URL host
dhoehna 0050a1b
Drop a dead bracket guard and document why unbracketing is load-bearing
dhoehna c7cca5c
[LXC] Make an unpinnable proxy address unrepresentable
dhoehna 1ba5a1c
[LXC] Update proxy address spec for the unpinnable-address fix
dhoehna File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, and I agree -- this is a real hole, not a theoretical one.
hosts_lineis written into/etc/hosts, so a newline in either field ends the record and starts a second, unauthorized mapping. A type whose whole job is to make the sandbox and the firewall agree on one endpoint cannot allow a value that denotes two.Fixed in c7cca5c (implementation) and 1ba5a1c (tests).
I took the strongest of the options you listed rather than the cheapest, since there is no caller yet and this only gets more expensive later:
hostname()andip().ProxyAddress::host_pinis the only way to obtain one.ipis anIpAddr, not aString. That makes the invalid state unrepresentable rather than merely rejected, and it drops the bracket-stripping the old code needed. It also lands the IPv6 requirement structurally:Display for IpAddrrenders bare, which is what a hosts file wants, so the asymmetry withto_url-- which brackets, because a URL host component requires it -- is no longer a convention someone can forget.-, and.. That set is chosen for what it excludes: whitespace and control characters, which are the injection vector.One thing worth flagging, because it changed the signature:
host_pinnow returnsResult<Option<ProxyHostPin>, WxcError>. An empty or malformed hostname used to returnNone, and I did not want to keep that.Nonemeans "no hosts entry required", so a malformed address would silently skip the pin, the sandbox would re-resolve the name freely, and the firewall could be bypassed. That is fail-open, which is the same defect flagged on the config path in the earlier iteration of this work.Ok(None)now means only "the address is an IP literal, nothing to resolve"; anything unpinnable isErr.Tests: 22 black-box tests, written against the documented contract by an author who has not read
models.rs. The empty-address test matches on all three arms by name --is_err() || is_none()would have passed either way, and that distinction is the entire point of the change. Verified with a mutation harness of 11 mutants, including "accept any hostname" and "returnOk(None)instead ofErr": 11 caught by a failing test, 0 survivors.