-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
base: main
Are you sure you want to change the base?
Conversation
Tagging subscribers to this area: @dotnet/ncl |
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 |
This is a draft PR for issue #118569.
Motivation
RFC 6761 defines special-use domain names:
invalid
and*.invalid
must always return NXDOMAINlocalhost
and*.localhost
must always resolve to the loopback addressCurrently, .NET relies fully on the OS resolver, which may allow
invalid
to resolve if defined in/etc/hosts
or DNS, and may not handle*.localhost
consistently across platforms.Proposed approach
invalid
and*.invalid
before OS resolution → throwSocketException
withHostNotFound
(simulate NXDOMAIN).localhost
and*.localhost
before OS resolution → returnIPAddress.Loopback
andIPAddress.IPv6Loopback
.Status