Skip to content
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

Localhost is special #6955

Merged
merged 3 commits into from Dec 2, 2020
Merged

Conversation

daurnimator
Copy link
Collaborator

RFC 6761 Section 6.3:

Name resolution APIs and libraries SHOULD recognize localhost names as special and SHOULD always return the IP loopback address for address queries and negative responses for all other query types.

Fixes #6898

@daurnimator daurnimator added the standard library This issue involves writing Zig code for the standard library. label Nov 3, 2020
@LemonBoy
Copy link
Contributor

LemonBoy commented Nov 3, 2020

That's some serious dedication to avoid writing a single line in the chroot /etc/hosts...

Back on this PR. The RFC says more than what you quoted:

  1. Name resolution APIs and libraries SHOULD recognize localhost
    names as special and SHOULD always return the IP loopback address
    for address queries and negative responses for all other query
    types. Name resolution APIs SHOULD NOT send queries for
    localhost names to their configured caching DNS server(s).

If you want to follow the RFC the localhost check must be performed first and not as a last-resort when the resolution failed.
The follow up question is: do we want this? The loopback address for ipv4 is defined as 127.0.0.1/8 (while ipv6's is ::1/128), meaning that a user could set-up different localhost aliases for different loopback addresses, here we are artifically limiting the resolver and that may be unexpected. On the other hand this makes the resolver a bit "safer" as it ensures a local address is always returned no matter how misconfigured (I don't mean the chroot problem, I mean a messed-up DNS configuration) the system is.
And beside that, the behaviour is extremely implementation dependent as this is only enforced by the homegrown DNS resolver (borrowed from musl), linking to the libc means you'd potentially get a different set of results.

About the code:

  • It doesn't recognize all the localhost addresses: "localhost", "localhost.", "localhost.<something>", "<something>.localhost"
  • It doesn't take into account the family/flags options, it always returns a ipv6 and an ipv4 entry

Returning a v4/v6 combo when unsure (AF_UNSPEC) is wrong as you don't know if the system has v6 connectivity to some degree. Detecting this is no simple task and the only reliable way to do so is to (periodically, you never know when the connectivity may change) perform a bogus lookup on a hostname you know has a v6 address and use that info to derive the v6 readiness of the system.

You now may be tempted to return a v4 address only, but that'd make the implementation non-rfc compliant (and pandas sad).

@daurnimator
Copy link
Collaborator Author

daurnimator commented Nov 3, 2020

That's some serious dedication to avoid writing a single line in the chroot /etc/hosts...

I do not have permissions to modify that file; wearing my packager hat, the alternative is marking zig's test suite as incorrect and somehow disabling the test.

If you want to follow the RFC the localhost check must be performed first and not as a last-resort when the resolution failed. The follow up question is: do we want this?

Good question; I thought about this and decided the least objectionable decision would be to only avoid returning a null result. If you'd like me to move it up before the linuxLookupNameFromHosts call I'm happy to.

It doesn't recognize all the localhost addresses: "localhost", "localhost.", "localhost.", ".localhost"

I believed that the trailing . was not supported/intended? If it is then I can allow localhost. and x.localhost.
localhost.something should be resolved.

It doesn't take into account the family/flags options, it always returns a ipv6 and an ipv4 entry
Returning a v4/v6 combo when unsure (AF_UNSPEC) is wrong as you don't know if the system has v6 connectivity to some degree. Detecting this is no simple task and the only reliable way to do so is to (periodically, you never know when the connectivity may change) perform a bogus lookup on a hostname you know has a v6 address and use that info to derive the v6 readiness of the system.

The easy option is to return both: if a user attempts ipv4 and gets EAFNOSUPPORT then they should just move on to the next addr. This behaviour is required anyway to avoid the (rare) race condition where e.g. ipv6 connectivity drops (imagine a phone with 4G and wifi; the 4G is dual stack while the wifi is IPv4 only: the user moves to an area of their house with a 4G dead spot).


Test failure seems unrelated with 758/1598 crypto.gimli.test "std-mips-linux-none-Debug-bare-multi permute"... index 0 incorrect. expected 2756473303, found 3121727578 CC @jedisct1

@LemonBoy
Copy link
Contributor

LemonBoy commented Nov 3, 2020

I do not have permissions to modify that file; wearing my packager hat, the alternative is marking zig's test suite as incorrect and somehow disabling the test.

mkarchroot -f /etc/hosts (or a bogus one)?

Good question; I thought about this and decided the least objectionable decision would be to only avoid returning a null result. If you'd like me to move it up before the linuxLookupNameFromHosts call I'm happy to.

That should be the very first check if you want to adhere to the RFC.

I believed that the trailing . was not supported/intended? If it is then I can allow localhost. and x.localhost.
localhost.something should be resolved.

Well the trailing dot makes it an absolute domain name but it's still localhost. Curl is smart enough to do the right thing, Firefox new address bar does a google search (eww), I think chromium handles this just fine.

The easy option is to return both: if a user attempts ipv4 and gets EAFNOSUPPORT then they should just move on to the next addr.

Well if the DNS resolver API is kept an implementation detail and we control the flags passed to linuxLookupName and we carefully avoid the AI_ADDRCONFIG flag this is a reasonable solution.
Likewise if family is always AF_UNSPEC returning both makes sense.

Adding an assertion is enough to prevent accidental breakages.

// types.

// Check for equal to "localhost" or ends in ".localhost"
if (mem.endsWith(u8, name, "localhost") and (name.len == "localhost".len or name[name.len - "localhost".len] == '.')) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the check be non case-sensitive?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other functions (e.g. linuxLookupNameFromHosts) are also case sensitive: I think case normalisation is meant to happen before linuxLookupName is called.

@marler8997
Copy link
Contributor

marler8997 commented Nov 3, 2020

The loopback address for ipv4 is defined as 127.0.0.1/8 (while ipv6's is ::1/128), meaning that a user could set-up different localhost aliases for different loopback addresses, here we are artifically limiting the resolver and that may be unexpected.

An interesting use case, however, someone could still support this behavior without overloading the localhost domain.

EDIT: after thinking on this more, saying that this would be "overloading" the localhost domain isn't correct. Your proposed use case still adhere's to the spec, which I didn't realize till just now. So this is a valid point that we would be limiting a machine's ability to customize the "system-wide" behavior of "localhost" in a single location like /etc/hosts.

@andrewrk andrewrk merged commit db0cb54 into ziglang:master Dec 2, 2020
andrewrk pushed a commit that referenced this pull request Dec 2, 2020
* std: always return loopback address when looking up localhost

* std: also return Ipv6 loopback

* std: remove commented out obsolete code
@andrewrk
Copy link
Member

andrewrk commented Dec 2, 2020

cherry-picked into 0.7.x => d15a0ec

@daurnimator daurnimator deleted the localhost-is-special branch December 2, 2020 00:40
@LemonBoy
Copy link
Contributor

LemonBoy commented Dec 2, 2020

The PR is still wrong, but let's merge it anyway! One less item in the backlog, who cares about RFCs?

@daurnimator
Copy link
Collaborator Author

The PR is still wrong, but let's merge it anyway! One less item in the backlog, who cares about RFCs?

It improves the status quo. It doesn't have to be perfect.

@LemonBoy
Copy link
Contributor

LemonBoy commented Dec 3, 2020

what a fucking joke

@komuw
Copy link

komuw commented Dec 3, 2020

be kind to each other

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

net.test.test "std-native-Debug-bare-multi resolve DNS" failure
6 participants