-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Draft: RFC 6761 handling for invalid and *.localhost #120376
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
Conversation
|
Tagging subscribers to this area: @dotnet/ncl |
rzikm
left a comment
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.
I think the approach is fine, but needs a bit of improvement and testing.
| if (hostName.Equals("invalid", StringComparison.OrdinalIgnoreCase) || | ||
| hostName.EndsWith(".invalid", StringComparison.OrdinalIgnoreCase)) |
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.
I think we can put this to a helper method and use fewer comparison to check:
static bool MatchesReservedName(string name, string reservedName)
{
// check if equal to reserved name or is a subdomain of it
return name.Equals(reservedName, StringComparison.OrdinalIgnoreCase) ||
(name.Length > reservedName.Length && name[name.Length - reservedName.Length - 1] == '.');
}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.
@rzikm That doesn't sound like a correct comparison. It only checks for the . character and not the rest of the string, so it would also match foo.xxvalid with reservedName == invalid.
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.
You could probably do something like name.EndsWith(reservedName, StringComparison.OrdinalIgnoreCase) && (name.Length == reservedName.Length || (name.Length > reservedName.Length && name[name.Length - reservedName.Length - 1] == '.')). It's not very readable though...
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.
right, I messed it, it was supposed to be as you wrote.
As for readability, that's what the descriptive name of the helper function is for :)
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.
Thanks, everyone. I'll continue with the adjustments. Considering the example provided by @filipnavara , I'll write some tests to ensure the OS resolver is called when expected.
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.
Perf-wise, you're likely looking at sub-nanosecond differences between the two (the JIT will inline direct comparisons here, there's no call overhead), so I'd prefer whatever option we feel is more readable. I'd lean towards the pattern used in the PR.
| if (hostName.Equals("localhost", StringComparison.OrdinalIgnoreCase) || | ||
| hostName.EndsWith(".localhost", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| IPAddress[] loopbacks = new IPAddress[] { IPAddress.Loopback, IPAddress.IPv6Loopback }; | ||
| return justAddresses ? (object)loopbacks : new IPHostEntry { AddressList = loopbacks, HostName = hostName, Aliases = Array.Empty<string>() }; | ||
| } |
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.
This needs to handle cases where IPv6 is disabled. We should not return IPv6Loopback in those cases. Symmetrically for IPv4.
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.
I'm also wondering if one could pre-allocate immutable answer is this should probably not change through the process life.
| return resultOnFailure; | ||
| } | ||
|
|
||
| if (hostName.Equals("invalid", StringComparison.OrdinalIgnoreCase) || |
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.
I think the whole new block should be inside the resolution activity BeforeResolution and AfterResolution. Otherwise, there won't be any diagnostics about this special case.
And addressFamily should be taken into consideration, current code return both IP v4 and v6 regardless of this value.
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.
may be worth of diag log as well.
- Consolidated reserved name checks into MatchesReservedName helper - Return only supported IP versions in GetLoopbacksForAddressFamily - Added diagnostic logging for special-use domains - Adjusted handling to respect AddressFamily and OS IPv4/IPv6 support - Special-name handling now runs between BeforeResolution and AfterResolution
|
@rzikm based on the discussion I implemented the changes to consolidate reserved name checks, filter supported loopbacks by AddressFamily, and add diagnostic logging for special-use domains. During testing, I tried disabling IPv6 on my host, and it was still returned by GetLoopbacksForAddressFamily. My concern is that using SocketProtocolSupportPal.OSSupportsIPv6 directly might not reflect the actual interface capabilities. Perhaps it would be safer to check the network interfaces themselves via NetworkInterface.Supports(NetworkInterfaceComponent), as described here: I’d like some guidance on whether this approach makes sense, or if the current OS-level check is sufficient. |
@dotnet-policy-service agree |
|
Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it. |
rzikm
left a comment
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.
Sorry for ignoring this, it slipped my attention.
@wfurt do you have any thoughts on how to check for IPv4/IPv6 support reliably?
| return name.Equals(reservedName, StringComparison.OrdinalIgnoreCase) || | ||
| name.EndsWith("." + reservedName, StringComparison.OrdinalIgnoreCase); |
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.
"." + reservedName will always allocate a new string (and will penalize all calls)
| return name.Equals(reservedName, StringComparison.OrdinalIgnoreCase) || | |
| name.EndsWith("." + reservedName, StringComparison.OrdinalIgnoreCase); | |
| // name == reservedName or name.EndsWith($".{reservedName}"); | |
| return name.EndsWith(reservedName, StringComparison.OrdinalIgnoreCase) && | |
| (name.Length == reservedName.Length || name[name.Length - reservedName.Length - 1] == '.'); |
|
I think the only really reliable way is to check if the 127.0.0.1 and ::1 are configured on loopback. AFAIK they always are on Windows and the loopback does not really exist as interface. On Linux/Mac the Kernel may support IPv6 but of the address is not there connect/bind will fail. One could possibly do static check once. It may change in theory during process run but I feel that is less likely. If we accept it we can construct the response upfront. In general DNS can point to addresses that do not exist or work. For example, on Linux the "loopback" will always resolve to 172.0.0.1 if if you remove the address. Same for loopback6 AFAIK. |
The entire theory of 127.0.0.1 in v4 is to confirm IP install was correct- So admins start with ping 127 sweet tcp/ip installed correctly next item up for bid. if that fail they check the localloop compiled correctly. then that ip installed - at that point you can start looking at device communications - good old days when nothing worked as it was supposed to. just uninstall tcp/ip and 127 should stop working. Sure, we use it in other ways, but that was the heart of what we used it for; been long time since seen local loops fail and ip installs fail though. |
|
Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it. |
This is a draft PR for issue #118569.
Motivation
RFC 6761 defines special-use domain names:
invalidand*.invalidmust always return NXDOMAINlocalhostand*.localhostmust always resolve to the loopback addressCurrently, .NET relies fully on the OS resolver, which may allow
invalidto resolve if defined in/etc/hostsor DNS, and may not handle*.localhostconsistently across platforms.Proposed approach
invalidand*.invalidbefore OS resolution → throwSocketExceptionwithHostNotFound(simulate NXDOMAIN).localhostand*.localhostbefore OS resolution → returnIPAddress.LoopbackandIPAddress.IPv6Loopback.Status