|
|
DescriptionResolve RFC 6761 localhost names to loopback
Per section 6.3, HostResolverImpl should recognize localhost names as
special and resolve as loopback without sending the query over the
network.
This CL also modifies IsLocalhost() to allow trailing dots (bug 460331),
so that IsLocalhost() can be used to detect localhost names that should
resolve to loopback.
BUG=496479, 460331
Committed: https://crrev.com/d1bc206e4635403d342e40d6e04845c029f1ada7
Cr-Commit-Position: refs/heads/master@{#335402}
Patch Set 1 #
Total comments: 8
Patch Set 2 : add unit tests for IsLocalhost change #Patch Set 3 : rsleevi, mmenke comments #
Total comments: 7
Patch Set 4 : mmenke comments #
Total comments: 1
Patch Set 5 : reshuffle tests #Patch Set 6 : mmenke's suggestion: ResolveLocalHostname(name, &addresses) #Patch Set 7 : style tweaks, simplification #
Total comments: 10
Patch Set 8 : rsleevi comments #
Total comments: 2
Patch Set 9 : rebase #Patch Set 10 : namespace qualification for EndsWith #Patch Set 11 : resolve test server addresses to IPv4 #
Total comments: 2
Patch Set 12 : rsleevi nit #
Messages
Total messages: 44 (11 generated)
estark@chromium.org changed reviewers: + ttuttle@chromium.org
ttuttle, could you please take a look?
added a few unit tests for net::IsLocalhost
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
https://codereview.chromium.org/1177933002/diff/1/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/1177933002/diff/1/net/base/net_util.cc#newcod... net/base/net_util.cc:737: host == "localhost6." || host == "localhost6.localdomain6." || :( on more .localdomain (these are not reserved by any means) https://codereview.chromium.org/1177933002/diff/1/net/dns/host_resolver_impl.cc File net/dns/host_resolver_impl.cc (right): https://codereview.chromium.org/1177933002/diff/1/net/dns/host_resolver_impl.... net/dns/host_resolver_impl.cc:2003: if (IsLocalhost(base::StringToLowerASCII(key.hostname))) { BUG: Should IsLocalhost be doing the case conversions? Probably https://codereview.chromium.org/1177933002/diff/1/net/dns/host_resolver_impl.... net/dns/host_resolver_impl.cc:2006: ParseIPLiteralToNumber("127.0.0.1", &ip_address); BUG 1: This isn't correct for localhost6 (which should be ::1) BUG 2: This isn't respecting |info.address_family()| or |key.address_family| BUG 3: Parsing is slow for a constant! boo-urns! const unsigned char kLocalhostIPv4 = { 127, 0, 0, 1}; addresses->push_back(IPEndpoint(IPAddressNumber(kLocalhostIPv4, kLocalhostIPv4 + arraysize(kLocalhostIPv4)), info.port())); [for example]
mmenke@chromium.org changed reviewers: + mmenke@chromium.org
https://codereview.chromium.org/1177933002/diff/1/net/dns/host_resolver_impl.cc File net/dns/host_resolver_impl.cc (right): https://codereview.chromium.org/1177933002/diff/1/net/dns/host_resolver_impl.... net/dns/host_resolver_impl.cc:2223: hostname = kLocalhost; Peppering this file with multiple localhost checks seems weird. IsLocalhost also calls IsLocalhostTLD. Do we need logic here and the new code, too?
Thanks sleevi, mmenke. See question inline about what to do with ADDRESS_FAMILY_UNSPECIFIED. https://codereview.chromium.org/1177933002/diff/1/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/1177933002/diff/1/net/base/net_util.cc#newcod... net/base/net_util.cc:737: host == "localhost6." || host == "localhost6.localdomain6." || On 2015/06/11 00:34:03, Ryan Sleevi wrote: > :( on more .localdomain (these are not reserved by any means) Do we need to keep these .localdomain things? Why are they even here? https://codereview.chromium.org/1177933002/diff/1/net/dns/host_resolver_impl.cc File net/dns/host_resolver_impl.cc (right): https://codereview.chromium.org/1177933002/diff/1/net/dns/host_resolver_impl.... net/dns/host_resolver_impl.cc:2003: if (IsLocalhost(base::StringToLowerASCII(key.hostname))) { On 2015/06/11 00:34:03, Ryan Sleevi wrote: > BUG: Should IsLocalhost be doing the case conversions? Probably Done. https://codereview.chromium.org/1177933002/diff/1/net/dns/host_resolver_impl.... net/dns/host_resolver_impl.cc:2006: ParseIPLiteralToNumber("127.0.0.1", &ip_address); On 2015/06/11 00:34:03, Ryan Sleevi wrote: > BUG 1: This isn't correct for localhost6 (which should be ::1) Done by factoring out the hostname check of IsLocalhost() into IsLocalhostHostname() with an output parameter that indicates if it was a IPv6-y name. (Feels kinda gross... open to other ideas.) > BUG 2: This isn't respecting |info.address_family()| or |key.address_family| Done. Wasn't sure what to do with ADDRESS_FAMILY_UNSPECIFIED, so I returned both, but no idea if that's correct or not? > BUG 3: Parsing is slow for a constant! boo-urns! > > const unsigned char kLocalhostIPv4 = { 127, 0, 0, 1}; > addresses->push_back(IPEndpoint(IPAddressNumber(kLocalhostIPv4, kLocalhostIPv4 + > arraysize(kLocalhostIPv4)), info.port())); > > [for example] Done. Third Simpsons reference today that I've had to look up. :( https://codereview.chromium.org/1177933002/diff/1/net/dns/host_resolver_impl.... net/dns/host_resolver_impl.cc:2223: hostname = kLocalhost; On 2015/06/11 00:36:51, mmenke wrote: > Peppering this file with multiple localhost checks seems weird. IsLocalhost > also calls IsLocalhostTLD. Do we need logic here and the new code, too? Ah, that's nice, we don't need both. Removed. https://codereview.chromium.org/1177933002/diff/40001/net/dns/host_resolver_i... File net/dns/host_resolver_impl.cc (right): https://codereview.chromium.org/1177933002/diff/40001/net/dns/host_resolver_i... net/dns/host_resolver_impl.cc:2174: // If the family was restricted to IPv4 due to a detected lack of IPv6 Just wanted to flag this slightly weird thing. Apparently localhost should be able to resolve to IPv6 loopback even if the lookup was restricted to IPv4 due to not detecting IPv6 support. Similar logic is scattered elsewhere and there's a unit test (DualFamilyLocalhost) that catches it.
https://codereview.chromium.org/1177933002/diff/40001/net/dns/host_resolver_i... File net/dns/host_resolver_impl.cc (right): https://codereview.chromium.org/1177933002/diff/40001/net/dns/host_resolver_i... net/dns/host_resolver_impl.cc:2174: // If the family was restricted to IPv4 due to a detected lack of IPv6 On 2015/06/11 02:35:32, estark wrote: > Just wanted to flag this slightly weird thing. Apparently localhost should be > able to resolve to IPv6 loopback even if the lookup was restricted to IPv4 due > to not detecting IPv6 support. Similar logic is scattered elsewhere and there's > a unit test (DualFamilyLocalhost) that catches it. One option would be to make IsLocalhostHostname take an optional AddressList* parameter, though we'd still have to filter out IPv6 addresses under certain circumstance. https://codereview.chromium.org/1177933002/diff/40001/net/dns/host_resolver_i... net/dns/host_resolver_impl.cc:2177: bool useIPv6 = isLocalhost6 || key.address_family == ADDRESS_FAMILY_IPV6 || If the address family is ADDRESS_FAMILY_IPV4 and the HOST_RESOLVER_DEFAULT_FAMILY_SET_DUE_TO_NO_IPV6 flag is not set, we must not return IPv6 addresses, as some consumers expect us only to return IPv4 addresses when they ask for only IPv4 addresses. Think this should be: bool use_ipv6 = (key.address_family == ADDRESS_FAMILY_IPV6 || key.address_family == ADDRESS_FAMILY_UNSPECIFIED || (key.address_family == ADDRESS_FAMILY_IPV4 && (key.host_resolver_flags & 2180 HOST_RESOLVER_DEFAULT_FAMILY_SET_DUE_TO_NO_IPV6)); Alternatively... bool no_ipv6 = key.address_family == ADDRESS_FAMILY_IPV4 && !(key.host_resolver_flags & HOST_RESOLVER_DEFAULT_FAMILY_SET_DUE_TO_NO_IPV6); So if ADDRESS_FAMILY_IPV4 was passed in as input when trying to look up localhost6, we should end up returning an empty address list. When using SOCKSv4, we'll crash otherwise. PAC files also have an IPv4-only lookup method, which probably shouldn't be returning IPv6 IPs. There should also be a test for this case.
https://codereview.chromium.org/1177933002/diff/40001/net/dns/host_resolver_i... File net/dns/host_resolver_impl.cc (right): https://codereview.chromium.org/1177933002/diff/40001/net/dns/host_resolver_i... net/dns/host_resolver_impl.cc:2174: // If the family was restricted to IPv4 due to a detected lack of IPv6 On 2015/06/11 14:45:59, mmenke wrote: > On 2015/06/11 02:35:32, estark wrote: > > Just wanted to flag this slightly weird thing. Apparently localhost should be > > able to resolve to IPv6 loopback even if the lookup was restricted to IPv4 due > > to not detecting IPv6 support. Similar logic is scattered elsewhere and > there's > > a unit test (DualFamilyLocalhost) that catches it. > > One option would be to make IsLocalhostHostname take an optional AddressList* > parameter, though we'd still have to filter out IPv6 addresses under certain > circumstance. Hmm? Sorry, I don't understand what problem this will solve. https://codereview.chromium.org/1177933002/diff/40001/net/dns/host_resolver_i... net/dns/host_resolver_impl.cc:2177: bool useIPv6 = isLocalhost6 || key.address_family == ADDRESS_FAMILY_IPV6 || On 2015/06/11 14:45:59, mmenke wrote: > If the address family is ADDRESS_FAMILY_IPV4 and the > HOST_RESOLVER_DEFAULT_FAMILY_SET_DUE_TO_NO_IPV6 flag is not set, we must not > return IPv6 addresses, as some consumers expect us only to return IPv4 addresses > when they ask for only IPv4 addresses. > > Think this should be: > > bool use_ipv6 = (key.address_family == ADDRESS_FAMILY_IPV6 || > key.address_family == ADDRESS_FAMILY_UNSPECIFIED || > (key.address_family == ADDRESS_FAMILY_IPV4 && > (key.host_resolver_flags & > 2180 HOST_RESOLVER_DEFAULT_FAMILY_SET_DUE_TO_NO_IPV6)); Done. > > Alternatively... > > bool no_ipv6 = key.address_family == ADDRESS_FAMILY_IPV4 && > !(key.host_resolver_flags & HOST_RESOLVER_DEFAULT_FAMILY_SET_DUE_TO_NO_IPV6); > > > So if ADDRESS_FAMILY_IPV4 was passed in as input when trying to look up > localhost6, we should end up returning an empty address list. > > When using SOCKSv4, we'll crash otherwise. PAC files also have an IPv4-only > lookup method, which probably shouldn't be returning IPv6 IPs. > > There should also be a test for this case. Done. https://codereview.chromium.org/1177933002/diff/60001/net/dns/host_resolver_i... File net/dns/host_resolver_impl.cc (right): https://codereview.chromium.org/1177933002/diff/60001/net/dns/host_resolver_i... net/dns/host_resolver_impl.cc:1993: if (ServeLocalhost(key, info, addresses)) Another thing to flag: one consequence of this change is that foo.localhost will use the /etc/hosts entry if there is one (it currently doesn't). I predict this will make users happy (there are quite a few complaining that they can't use foo.localhost /etc/hosts entries anymore), but does seem to be less compliant with RFC 6761. If we want foo.localhost and friends to always resolve to loopback and ignore /etc/hosts, I can just move this above the ServeFromCache() and ServeFromHosts() lines above.
https://codereview.chromium.org/1177933002/diff/40001/net/dns/host_resolver_i... File net/dns/host_resolver_impl.cc (right): https://codereview.chromium.org/1177933002/diff/40001/net/dns/host_resolver_i... net/dns/host_resolver_impl.cc:2174: // If the family was restricted to IPv4 due to a detected lack of IPv6 On 2015/06/11 17:16:47, estark wrote: > On 2015/06/11 14:45:59, mmenke wrote: > > On 2015/06/11 02:35:32, estark wrote: > > > Just wanted to flag this slightly weird thing. Apparently localhost should > be > > > able to resolve to IPv6 loopback even if the lookup was restricted to IPv4 > due > > > to not detecting IPv6 support. Similar logic is scattered elsewhere and > > there's > > > a unit test (DualFamilyLocalhost) that catches it. > > > > One option would be to make IsLocalhostHostname take an optional AddressList* > > parameter, though we'd still have to filter out IPv6 addresses under certain > > circumstance. > > Hmm? Sorry, I don't understand what problem this will solve. It's an alternative to passing the bool pointer, which makes the function a bit more natural. bool ResolveLocalHostName(name, &addresses); Seems a reasonable method (Even if it's actually called IsLocalHostname). bool IsLocalHostname(name, &is_localhost_ipv6); Seems a bit weird...Anyhow, neither's great. Just an option.
Patchset #6 (id:100001) has been deleted
https://codereview.chromium.org/1177933002/diff/40001/net/dns/host_resolver_i... File net/dns/host_resolver_impl.cc (right): https://codereview.chromium.org/1177933002/diff/40001/net/dns/host_resolver_i... net/dns/host_resolver_impl.cc:2174: // If the family was restricted to IPv4 due to a detected lack of IPv6 On 2015/06/11 17:32:25, mmenke wrote: > On 2015/06/11 17:16:47, estark wrote: > > On 2015/06/11 14:45:59, mmenke wrote: > > > On 2015/06/11 02:35:32, estark wrote: > > > > Just wanted to flag this slightly weird thing. Apparently localhost should > > be > > > > able to resolve to IPv6 loopback even if the lookup was restricted to IPv4 > > due > > > > to not detecting IPv6 support. Similar logic is scattered elsewhere and > > > there's > > > > a unit test (DualFamilyLocalhost) that catches it. > > > > > > One option would be to make IsLocalhostHostname take an optional > AddressList* > > > parameter, though we'd still have to filter out IPv6 addresses under certain > > > circumstance. > > > > Hmm? Sorry, I don't understand what problem this will solve. > > It's an alternative to passing the bool pointer, which makes the function a bit > more natural. > > bool ResolveLocalHostName(name, &addresses); > > Seems a reasonable method (Even if it's actually called IsLocalHostname). > > bool IsLocalHostname(name, &is_localhost_ipv6); > > Seems a bit weird...Anyhow, neither's great. Just an option. Ah, I see! Done.
https://codereview.chromium.org/1177933002/diff/140001/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/1177933002/diff/140001/net/base/net_util.cc#n... net/base/net_util.cc:151: std::string lowercased_host = base::StringToLowerASCII(host); Suggestion: To reduce comparisons for the forms, and thus reduce copy paste, since you're copying the string regardless here, you could do std::string lowercase_host = base::StringToLowerASCII(host); if (!lowercase_host.empty() && lowercase_host.rbegin() == '.') lowercase_host.resize(lowercase_host.size() - 1); return (lowercased_host == "localhost" || lowercased_host == "localhost.localdomain" || IsLocalhostTLD(lowercased_host)); [Incidentally, this is similar to the suggestion I made on IsLocalhostTLD] You could further optimize by having an IsNormalizedLocalhostTLD() internal function (e.g. not part of the .h) that expects the string to already be lowercased and w/ the trailing . removed. That would prevent further additional copies of the string being made, which is presumably going to be the hot-path general case here (that is, we expect *most* names going through the resolver to *not* be localhost). IsLocalhostTLD() would just normalize first. [Further refactoring would be] std::string NormalizeHostname(const std::string& host) { std::string result = base::StringToLowerASCII(host); if (!result.empty() && result.rbegin() == '.') result.resize(result.size() - 1); return result; } bool IsNormalizedLocalhostTLD(host) { return EndsWith(host, ".localhost", true); } bool IsLocalHostname(const std::string& host) { return host == "localhost" || host == "localhost.localdomain" || IsNormalizedLocalhostTLD(host); } bool IsLocal6Hostname(const std::string& host) { return host == "localhost6" || host == "localhost6.localdomain6" } } // namespace bool IsLocalhost(const std::string& host) { std::string normalized_host = NormalizeHostname(host); if (IsLocalHostname(normalized_host) || IsLocal6Hostname(normalized_host)) return true; ... // stuff here } bool IsLocalhostTLD(const std::string& host) { return IsNormalizedLocalhostTLD(NormalizeHostname(host)); } This reduces it to 1 copy, 1 character conversion, and a more efficient string comparison (because it can now be done case sensitively) If we had an EndsWith that took iterators, we could get away with avoiding a string copy for the IsLocalhostTLD, which could offer more minor perf gains. I started writing up a version of what that would look like, and then I looked in aghast horror at the template hell I wrote (it involves "template typename template" declarations, the use of std::distance(), and other gross things). Hopefully the above isn't 'too' gross a refactoring. https://codereview.chromium.org/1177933002/diff/140001/net/base/net_util.cc#n... net/base/net_util.cc:756: 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1}; nit: Can make these static (so that they're in the rodata section of the executable) https://codereview.chromium.org/1177933002/diff/140001/net/base/net_util.cc#n... net/base/net_util.cc:760: bool isLocal6 = IsLocal6Hostname(host); naming: is_local6 https://codereview.chromium.org/1177933002/diff/140001/net/base/net_util.h File net/base/net_util.h (right): https://codereview.chromium.org/1177933002/diff/140001/net/base/net_util.h#ne... net/base/net_util.h:361: // the names that indicate a loopback and false otherwise. Special IPv6 Documentation: You seem to mix localhost with loopback here in the description, without quantifying the distinction. Without poking in the implementation, it seems you use them interchangably - perhaps align on one (with the function name, it would seem 'local hostname' is probably the preferred term) https://codereview.chromium.org/1177933002/diff/140001/net/dns/host_resolver_... File net/dns/host_resolver_impl.cc (right): https://codereview.chromium.org/1177933002/diff/140001/net/dns/host_resolver_... net/dns/host_resolver_impl.cc:2178: } else if (key.address_family != ADDRESS_FAMILY_IPV6) { I don't understand this clause - could you elaborate? It would *seem* like the logic would be something like for (const auto& address : resolved_addresses) { if (key.address_family == ADDRESS_FAMILY_UNSPECIFIED || key.address_family == address.GetFamily() || (address.GetFamily() == ADDRESS_FAMILY_IPV6 && key.address_family == ADDRESS_FAMILY_IPV4 && (key.host_resolver_flags & HOST_RESOLVER_DEFAULT_FAMILY_SET_DUE_TO_NO_IPV6))) addresses->push_back(address); } That is - - Return all addresses if they don't care - Return all addresses that specifically match what they care about - Return IPv6 addresses if they requested IPv4 only due to detected lack of support - Otherwise, don't include the address Is that a fair summary of the intended logic?
https://codereview.chromium.org/1177933002/diff/140001/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/1177933002/diff/140001/net/base/net_util.cc#n... net/base/net_util.cc:151: std::string lowercased_host = base::StringToLowerASCII(host); On 2015/06/12 01:00:04, Ryan Sleevi wrote: > Suggestion: To reduce comparisons for the forms, and thus reduce copy paste, > since you're copying the string regardless here, you could do > > std::string lowercase_host = base::StringToLowerASCII(host); > if (!lowercase_host.empty() && lowercase_host.rbegin() == '.') > lowercase_host.resize(lowercase_host.size() - 1); > > return (lowercased_host == "localhost" || > lowercased_host == "localhost.localdomain" || > IsLocalhostTLD(lowercased_host)); > > [Incidentally, this is similar to the suggestion I made on IsLocalhostTLD] > > You could further optimize by having an IsNormalizedLocalhostTLD() internal > function (e.g. not part of the .h) that expects the string to already be > lowercased and w/ the trailing . removed. That would prevent further additional > copies of the string being made, which is presumably going to be the hot-path > general case here (that is, we expect *most* names going through the resolver to > *not* be localhost). IsLocalhostTLD() would just normalize first. Done. > > [Further refactoring would be] > > std::string NormalizeHostname(const std::string& host) { > std::string result = base::StringToLowerASCII(host); > if (!result.empty() && result.rbegin() == '.') > result.resize(result.size() - 1); > return result; > } > > bool IsNormalizedLocalhostTLD(host) { > return EndsWith(host, ".localhost", true); > } > > bool IsLocalHostname(const std::string& host) { > return host == "localhost" || host == "localhost.localdomain" || > IsNormalizedLocalhostTLD(host); > } > > bool IsLocal6Hostname(const std::string& host) { > return host == "localhost6" || host == "localhost6.localdomain6" > } > > } // namespace > > bool IsLocalhost(const std::string& host) { > std::string normalized_host = NormalizeHostname(host); > if (IsLocalHostname(normalized_host) || IsLocal6Hostname(normalized_host)) > return true; > ... // stuff here > } > > bool IsLocalhostTLD(const std::string& host) { > return IsNormalizedLocalhostTLD(NormalizeHostname(host)); > } > > This reduces it to 1 copy, 1 character conversion, and a more efficient string > comparison (because it can now be done case sensitively) > > If we had an EndsWith that took iterators, we could get away with avoiding a > string copy for the IsLocalhostTLD, which could offer more minor perf gains. I > started writing up a version of what that would look like, and then I looked in > aghast horror at the template hell I wrote (it involves "template typename > template" declarations, the use of std::distance(), and other gross things). > Hopefully the above isn't 'too' gross a refactoring. https://codereview.chromium.org/1177933002/diff/140001/net/base/net_util.cc#n... net/base/net_util.cc:756: 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1}; On 2015/06/12 01:00:04, Ryan Sleevi wrote: > nit: Can make these static (so that they're in the rodata section of the > executable) Done. https://codereview.chromium.org/1177933002/diff/140001/net/base/net_util.cc#n... net/base/net_util.cc:760: bool isLocal6 = IsLocal6Hostname(host); On 2015/06/12 01:00:04, Ryan Sleevi wrote: > naming: is_local6 Done. https://codereview.chromium.org/1177933002/diff/140001/net/base/net_util.h File net/base/net_util.h (right): https://codereview.chromium.org/1177933002/diff/140001/net/base/net_util.h#ne... net/base/net_util.h:361: // the names that indicate a loopback and false otherwise. Special IPv6 On 2015/06/12 01:00:04, Ryan Sleevi wrote: > Documentation: You seem to mix localhost with loopback here in the description, > without quantifying the distinction. > > Without poking in the implementation, it seems you use them interchangably - > perhaps align on one (with the function name, it would seem 'local hostname' is > probably the preferred term) Done. https://codereview.chromium.org/1177933002/diff/140001/net/dns/host_resolver_... File net/dns/host_resolver_impl.cc (right): https://codereview.chromium.org/1177933002/diff/140001/net/dns/host_resolver_... net/dns/host_resolver_impl.cc:2178: } else if (key.address_family != ADDRESS_FAMILY_IPV6) { On 2015/06/12 01:00:04, Ryan Sleevi wrote: > I don't understand this clause - could you elaborate? > > It would *seem* like the logic would be something like > > for (const auto& address : resolved_addresses) { > if (key.address_family == ADDRESS_FAMILY_UNSPECIFIED || > key.address_family == address.GetFamily() || > (address.GetFamily() == ADDRESS_FAMILY_IPV6 && > key.address_family == ADDRESS_FAMILY_IPV4 && > (key.host_resolver_flags & > HOST_RESOLVER_DEFAULT_FAMILY_SET_DUE_TO_NO_IPV6))) > addresses->push_back(address); > } > > That is - > - Return all addresses if they don't care > - Return all addresses that specifically match what they care about > - Return IPv6 addresses if they requested IPv4 only due to detected lack of > support > - Otherwise, don't include the address > > > Is that a fair summary of the intended logic? This is the logic I intended to write: If the address is IPv6: - include it if they didn't specifically ask for IPv4 OR if they specifically asked for IPv4 due to detected lack of IPv6 support Otherwise (i.e. if the address is IPv4): - include it if they didn't specifically ask for IPv6 Which I think gives the same results as yours, just written differently. Yours is probably easier to read though, so I changed it to that.
I think this LGTM. Make sure to ping ttuttle (surprised he hasn't replied), perhaps on Hangouts, to make sure he's looking at this.
I'll take a look on Monday; I was holding off while Matt's comments were dealt with. On Fri, Jun 12, 2015, 19:10 <rsleevi@chromium.org> wrote: > I think this LGTM. Make sure to ping ttuttle (surprised he hasn't replied), > perhaps on Hangouts, to make sure he's looking at this. > > https://codereview.chromium.org/1177933002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/06/13 00:39:09, ttuttle wrote: > I'll take a look on Monday; I was holding off while Matt's comments were > dealt with. ttuttle: friendly ping > > On Fri, Jun 12, 2015, 19:10 <mailto:rsleevi@chromium.org> wrote: > > > I think this LGTM. Make sure to ping ttuttle (surprised he hasn't replied), > > perhaps on Hangouts, to make sure he's looking at this. > > > > https://codereview.chromium.org/1177933002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
lgtm modulo one question. https://codereview.chromium.org/1177933002/diff/160001/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/1177933002/diff/160001/net/base/net_util.cc#n... net/base/net_util.cc:157: bool IsNormalizedLocalhostTLD(const std::string& host) { Do we want to also catch hostnames like "foo.localhost6"?
ttuttle: thank you mmenke: do you want to take another look before I land this? Also, just in case this got buried earlier in the thread, wanted to flag this issue: one consequence of this change is that foo.localhost will use the /etc/hosts entry if there is one (it currently doesn't). I predict this will make users happy (there are quite a few complaining that they can't use foo.localhost /etc/hosts entries anymore), but does seem to be less compliant with RFC 6761. If we want foo.localhost and friends to always resolve to loopback and ignore /etc/hosts, I can just move the ServeLocalhost() call above the ServeFromCache() and ServeFromHosts() lines above. https://codereview.chromium.org/1177933002/diff/160001/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/1177933002/diff/160001/net/base/net_util.cc#n... net/base/net_util.cc:157: bool IsNormalizedLocalhostTLD(const std::string& host) { On 2015/06/16 14:53:39, ttuttle wrote: > Do we want to also catch hostnames like "foo.localhost6"? Hmm, I'm not sure. My preference is to stick as close as possible to names that are in the RFC, even though we already have some that aren't (e.g. .localdomain).
On 2015/06/16 15:40:15, estark wrote: > mmenke: do you want to take another look before I land this? I'll defer to tuttle and sleevi. > Also, just in case this got buried earlier in the thread, wanted to flag this > issue: one consequence of this change is that foo.localhost will use the > /etc/hosts entry if there is one (it currently doesn't). I predict this will > make users happy (there are quite a few complaining that they can't use > foo.localhost /etc/hosts entries anymore), but does seem to be less compliant > with RFC 6761. > > If we want foo.localhost and friends to always resolve to loopback and ignore > /etc/hosts, I can just move the ServeLocalhost() call above the ServeFromCache() > and ServeFromHosts() lines above. My understanding is that the security concern motivating this change is with a malicious remote DNS server returning results for a localhost domain. Moving ServeLocalhost() around doesn't affect this case. Moving ServeFromHosts() up just prevents users from mapping trusted "local" domains to remote (Possibly untrusted) systems. This seems a bit less concerning, but does allow those remote systems to live in the trusted domain. I do think we're safest moving ServeFromHosts() up, but don't feel strongly about it. The behavior does cause user pain, but we've already done that, so we (And our users) have presumably already paid much of that price. I'll happily defer to sleevi and ttuttle on this.
On 2015/06/16 15:56:08, mmenke wrote: > Moving ServeFromHosts() up just prevents users from mapping trusted "local" > domains to remote (Possibly untrusted) systems. This seems a bit less > concerning, but does allow those remote systems to live in the trusted domain. > I do think we're safest moving ServeFromHosts() up, but don't feel strongly > about it. The behavior does cause user pain, but we've already done that, so we > (And our users) have presumably already paid much of that price. You meant moving "ServeLocalhost" up, right? ServeFromHosts currently is called first, which is what allows the remapping. As it stands, the host file is protected and requires administrative access to use. So, on one hand, it should naturally be protected (by the OS) from general tampering (much like, say, the cookiejar passphrases). On the other, we know lots of malware and jerkware already futz with localhost to do their thing. The "evil jerkware" scenario would be some app that modifies the hosts file to map something in (foo.localhost) to a remote site, as a way of bypassing some HTTPS requirements. However, there's already several other ways this can be done - for example, adding a command-line flag (and a separate user data dir), or, on Windows, adding a Winsock LSP that intercepts loopback and tags it to go to some remote site. This really is a question of security policy, so it may help to get feedback from the chrome-security folks. I suspect Mike, Chris, and Justin will all have feelings about this, and I suspect Joel and Alex may feel... differently :) If we keep it where it is, then (a) we don't break people that rely on this behaviour [although incidentally, we broke them with the bug] (b) more people may come to rely on this behaviour, making it hard to change If we move ServeLocalhost above ServeFromHost then (a) We're committing to breaking that use case, without fully knowing the implications (b) We're ensuring people don't come to rely on this behaviour, and we can introduce it again later. As much as I love the second option (break it, fix it later), I suspect that because "Every Other Modern Browser" defers to the hosts file first, we'll continue to get bug reports unless/until we convince other browsers to follow our path (presumably, in the fetch spec). Which is my long-winded way of saying personal preference is for ServeLocalhost -> ServeFromHost, but I suspect for user happiness and long-term interop, it should be ServeFromHost -> ServeLocalhost (as it is right now), and if you're still not sure, take it up with the rest of the team who will have to be the target of user hate :)
On 2015/06/16 19:13:04, Ryan Sleevi wrote: > On 2015/06/16 15:56:08, mmenke wrote: > > Moving ServeFromHosts() up just prevents users from mapping trusted "local" > > domains to remote (Possibly untrusted) systems. This seems a bit less > > concerning, but does allow those remote systems to live in the trusted domain. > > > I do think we're safest moving ServeFromHosts() up, but don't feel strongly > > about it. The behavior does cause user pain, but we've already done that, so > we > > (And our users) have presumably already paid much of that price. > > You meant moving "ServeLocalhost" up, right? ServeFromHosts currently is called > first, which is what allows the remapping. Erm, yea, typo on my part. > As it stands, the host file is protected and requires administrative access to > use. So, on one hand, it should naturally be protected (by the OS) from general > tampering (much like, say, the cookiejar passphrases). On the other, we know > lots of malware and jerkware already futz with localhost to do their thing. > > The "evil jerkware" scenario would be some app that modifies the hosts file to > map something in (foo.localhost) to a remote site, as a way of bypassing some > HTTPS requirements. However, there's already several other ways this can be done > - for example, adding a command-line flag (and a separate user data dir), or, on > Windows, adding a Winsock LSP that intercepts loopback and tags it to go to some > remote site. It's not actually jerkware modifying those hosts file I'm concerned about - if it has the power to do that, the user has bigger problems on their hands. It's actually people modifying the security properties of pages without understanding that they're doing so, since localhost domains grant special magic permissions, and then potentially getting into through that way, though I'm admittedly not familiar with all the details of that. > This really is a question of security policy, so it may help to get feedback > from the chrome-security folks. I suspect Mike, Chris, and Justin will all have > feelings about this, and I suspect Joel and Alex may feel... differently :) > > If we keep it where it is, then > (a) we don't break people that rely on this behaviour [although incidentally, we > broke them with the bug] > (b) more people may come to rely on this behaviour, making it hard to change > > If we move ServeLocalhost above ServeFromHost then > (a) We're committing to breaking that use case, without fully knowing the > implications > (b) We're ensuring people don't come to rely on this behaviour, and we can > introduce it again later. > > As much as I love the second option (break it, fix it later), I suspect that > because "Every Other Modern Browser" defers to the hosts file first, we'll > continue to get bug reports unless/until we convince other browsers to follow > our path (presumably, in the fetch spec). > > Which is my long-winded way of saying personal preference is for ServeLocalhost > -> ServeFromHost, but I suspect for user happiness and long-term interop, it > should be ServeFromHost -> ServeLocalhost (as it is right now), and if you're > still not sure, take it up with the rest of the team who will have to be the > target of user hate :)
On 2015/06/16 19:24:27, mmenke wrote: > It's not actually jerkware modifying those hosts file I'm concerned about - if > it has the power to do that, the user has bigger problems on their hands. It's > actually people modifying the security properties of pages without understanding > that they're doing so, since localhost domains grant special magic permissions, > and then potentially getting into through that way, though I'm admittedly not > familiar with all the details of that. Well, sure, but a user who can be coerced into doing so (which requires an elevation on Windows or a sudo on other platforms) can likely be coerced into, say, installing a root cert. Or they can be prompted when installing an app to elevate (which they inevitably will) and the jerkware will do it to them. Certainly, I agree, there's an element of user education. That's why the command-line flag requires a separate user-data-dir. But that's why I meant it's more about policy - whether or not we want to "fight" a user with admin access on their machine. As much as I want to prevent them from self-harm, I suspect the answer within both the policy and threat model is "If you can modify hosts, you can modify hosts". A probably-overkill solution would be to add the "dangerous flags" warning if we determined resolving a hostname in the .localhost TLD came from the hosts file, but I'm not sure how much that would help, vs the complexity/usability tradeoffs. That's why I'm happy to say "I don't really care either way" and punt the problem of policy to the people who will have to deal with the fallout the most (on Twitter and the product support forums)
On 2015/06/16 19:31:09, Ryan Sleevi wrote: > On 2015/06/16 19:24:27, mmenke wrote: > > It's not actually jerkware modifying those hosts file I'm concerned about - if > > it has the power to do that, the user has bigger problems on their hands. > It's > > actually people modifying the security properties of pages without > understanding > > that they're doing so, since localhost domains grant special magic > permissions, > > and then potentially getting into through that way, though I'm admittedly not > > familiar with all the details of that. > > Well, sure, but a user who can be coerced into doing so (which requires an > elevation on Windows or a sudo on other platforms) can likely be coerced into, > say, installing a root cert. Or they can be prompted when installing an app to > elevate (which they inevitably will) and the jerkware will do it to them. > > Certainly, I agree, there's an element of user education. That's why the > command-line flag requires a separate user-data-dir. But that's why I meant it's > more about policy - whether or not we want to "fight" a user with admin access > on their machine. As much as I want to prevent them from self-harm, I suspect > the answer within both the policy and threat model is "If you can modify hosts, > you can modify hosts". > > A probably-overkill solution would be to add the "dangerous flags" warning if we > determined resolving a hostname in the .localhost TLD came from the hosts file, > but I'm not sure how much that would help, vs the complexity/usability > tradeoffs. That's why I'm happy to say "I don't really care either way" and punt > the problem of policy to the people who will have to deal with the fallout the > most (on Twitter and the product support forums) Ok, so it appears that no one on the security team has a strong opinion about this, or any opinion at all actually. Given that, I think I'd like to go ahead and land this as is (ServeFromHosts and then ServeLocalhost). If we want to stop users from putting localhost in /etc/hosts, I think maybe the right way to do it is to get another browser or two on board; otherwise users will probably just get frustrated with Chrome and switch browsers, I'd imagine.
The CQ bit was checked by estark@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1177933002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
The CQ bit was checked by estark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ttuttle@chromium.org, rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/1177933002/#ps200001 (title: "namespace qualification for EndsWith")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1177933002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
On 2015/06/19 04:12:31, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) I'm a little stumped by this test failure. What's happening is that, when connecting to the test server, the DNS lookup for localhost is returning ::1 as well as 127.0.0.1, so we try to connect to ::1 first and then 127.0.0.1 when that fails. That means the netlog offset is different from what the test expects (because the test expects to connect straight to 127.0.0.1). I'm not sure what the correct behavior is and what a correct fix would be. (We could accept that the failed IPv6 connection is going to happen and just adjust the netlog offset that the test looks at... or we could assume the test server is listening on 127.0.0.1 and request address family IPv4 when connecting to it from the test... or we could have ServeLocalhost() return only IPv4 for address family unspecified... but those all feel like weird fixes.)
On 2015/06/19 20:48:26, estark wrote: > I'm not sure what the correct behavior is and what a correct fix would be. (We > could accept that the failed IPv6 connection is going to happen and just adjust > the netlog offset that the test looks at... or we could assume the test server > is listening on 127.0.0.1 and request address family IPv4 when connecting to it > from the test... or we could have ServeLocalhost() return only IPv4 for address > family unspecified... but those all feel like weird fixes.) Option 1: Change https://code.google.com/p/chromium/codesearch#chromium/src/net/test/spawned_t... to do the ADDRESS_FAMILY_IPv4, which is internally consistent in that file with kLocalhost being 127.0.0.1 Option 2: Change https://code.google.com/p/chromium/codesearch#chromium/src/net/test/spawned_t... to return 127.0.0.2 (this only works if the test server is binding to 0.0.0.0, which isn't immediately clear) Option 3: Change the test server to be agnostic as to the IP layer. After looking through //net/tools/testserver/testserver.py, this would mostly be about stripping out passing the host param explicitly (which is typically localhost) and instead passing an empty string (thus having it globally bind on that particular port, on all available interfaces). That one looks to be a little messier, and may have side-effects. In particular, there are unquestionably elements of testserver.py not well suited to handle IPv6 because the construct URLs as part of the test responses. This is more one of those long-term "maybe" nice to have. Option 4: Make the NetLog scan code lenient. Of these, I think 1 >> 4 > 2 >>>> 3
On 2015/06/19 21:22:43, Ryan Sleevi wrote: > On 2015/06/19 20:48:26, estark wrote: > > I'm not sure what the correct behavior is and what a correct fix would be. (We > > could accept that the failed IPv6 connection is going to happen and just > adjust > > the netlog offset that the test looks at... or we could assume the test server > > is listening on 127.0.0.1 and request address family IPv4 when connecting to > it > > from the test... or we could have ServeLocalhost() return only IPv4 for > address > > family unspecified... but those all feel like weird fixes.) > > Option 1: Change > https://code.google.com/p/chromium/codesearch#chromium/src/net/test/spawned_t... > to do the ADDRESS_FAMILY_IPv4, which is internally consistent in that file with > kLocalhost being 127.0.0.1 > > Option 2: Change > https://code.google.com/p/chromium/codesearch#chromium/src/net/test/spawned_t... > to return 127.0.0.2 (this only works if the test server is binding to 0.0.0.0, > which isn't immediately clear) > > Option 3: Change the test server to be agnostic as to the IP layer. After > looking through //net/tools/testserver/testserver.py, this would mostly be about > stripping out passing the host param explicitly (which is typically localhost) > and instead passing an empty string (thus having it globally bind on that > particular port, on all available interfaces). That one looks to be a little > messier, and may have side-effects. In particular, there are unquestionably > elements of testserver.py not well suited to handle IPv6 because the construct > URLs as part of the test responses. This is more one of those long-term "maybe" > nice to have. > > Option 4: Make the NetLog scan code lenient. > > Of these, I think 1 >> 4 > 2 >>>> 3 Ah, ok, that makes sense, thanks. Did #1.
Patchset 11 LGTM. A nit, since you didn't CQ right away. https://codereview.chromium.org/1177933002/diff/220001/net/test/spawned_test_... File net/test/spawned_test_server/base_test_server.cc (right): https://codereview.chromium.org/1177933002/diff/220001/net/test/spawned_test_... net/test/spawned_test_server/base_test_server.cc:239: // Do an IPv4 lookup since kLocalhost is IPv4. nit: maybe expand/reword the comment // Limit the lookup to IPv4. When started with the default // address of kLocalhost, testserver.py only supports IPv4. // If a custom hostname is used, it's possible that the test // server will listen on both IPv4 and IPv6, so this will // still work. The testserver does not support explicit // IPv6 literal hostnames.
https://codereview.chromium.org/1177933002/diff/220001/net/test/spawned_test_... File net/test/spawned_test_server/base_test_server.cc (right): https://codereview.chromium.org/1177933002/diff/220001/net/test/spawned_test_... net/test/spawned_test_server/base_test_server.cc:239: // Do an IPv4 lookup since kLocalhost is IPv4. On 2015/06/19 23:08:16, Ryan Sleevi wrote: > nit: maybe expand/reword the comment > > // Limit the lookup to IPv4. When started with the default > // address of kLocalhost, testserver.py only supports IPv4. > // If a custom hostname is used, it's possible that the test > // server will listen on both IPv4 and IPv6, so this will > // still work. The testserver does not support explicit > // IPv6 literal hostnames. Done.
The CQ bit was checked by estark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ttuttle@chromium.org, rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/1177933002/#ps240001 (title: "rsleevi nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1177933002/240001
Message was sent while issue was closed.
Committed patchset #12 (id:240001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/d1bc206e4635403d342e40d6e04845c029f1ada7 Cr-Commit-Position: refs/heads/master@{#335402} |