fix DoH error when using ip address as hostname #7073

TaoZang · 2022-02-11T15:47:04Z

Fix the issue that when network trying to do a DNS lookup with an IP numeric hostname, the DnsOverHttps will throw an UnknownHostException instead of returning the InetAddress with this IP address.
For example, If user runs a http proxy, the DnsOverHttps module will try to lookup the local Proxy address such as 127.0.0.x.

yschimke · 2022-02-11T15:52:09Z

okhttp-dnsoverhttps/src/main/kotlin/okhttp3/dnsoverhttps/DnsOverHttps.kt

@@ -67,7 +67,11 @@ class DnsOverHttps internal constructor(
      }
    }

-    return lookupHttps(hostname)
+    return try {
+      listOf(InetAddressUtil.forString(hostname))


Can you add a test?

We need to avoid an exception in most calls here. Maybe reverse this logic (try hostname first) and in the normal case call lookupHttps, and catch and exception, check if it's a ip address and then return forString.

I think it would be better if we try converting by forString first because we can avoid a potential https connection if the hostname is an numerical IP address.

ok, well avoid the exception then. 99+% of calls will be for hostnames, not IPs.

If most of the calls will be hostname, maybe call the lookupHttps first would be better. I will update the code.

yschimke · 2022-02-11T15:53:39Z

okhttp-dnsoverhttps/src/main/kotlin/okhttp3/dnsoverhttps/InetAddressUtil.kt

+import java.net.UnknownHostException
+import java.util.*
+
+object InetAddressUtil {


This looks like a lot of supporting code. Are there existing methods that do this, or would a simple regex as a first parse be simpler (\d+.\d+...) or (\d+:...)?

I implement the InetAddressUtil kotlin version refer to the com.google.common.net.InetAddresses class. I just concern about that a simple regex may cause some bugs due to the complex format of IPv4/IPv6. A common parse algorithm would be better.

If it's sourced from there, you can just commit it here with a new copyright. In other places we've derived new code from other projects but keep that copyright. Let me look into this tonight, there may be simpler ways to do this.

I also updated the copyright just same as the raw implement file.

yschimke · 2022-02-11T16:27:28Z

I'll backport this to 4.9.x when we have a fix, but until that happens you might want to have your own Dns wrapper that does this logic yourself.

Thanks for the report and PR.

TaoZang · 2022-02-11T16:38:31Z

I'll backport this to 4.9.x when we have a fix, but until that happens you might want to have your own Dns wrapper that does this logic yourself.

Thanks for the report and PR.

Thank you for your review:)

… failed

yschimke · 2022-02-12T13:22:06Z

Looking at the logic of InetAddress.getAllByName, would the following be simpler?

  override fun lookup(hostname: String): List<InetAddress> {
    if (isIpAddress(hostname)) {
      return listOf(InetAddress.getByName(hostname))
    }
    ...
  private fun isIpAddress(hostname: String): Boolean {
    return hostname[0].digitToIntOrNull(16) != null
      || hostname[0] == ':' || hostname[0] == '['
  }

getAllByName shortcircuits and avoids Dns if the hostname is an ip address. Using similar logic in isIpAddress.

yschimke · 2022-02-12T13:23:30Z

Please add some tests to DnsOverHttpsTest.

…with an IP address hostname

TaoZang · 2022-02-12T17:14:25Z

Looking at the logic of InetAddress.getAllByName, would the following be simpler?
  override fun lookup(hostname: String): List<InetAddress> {
    if (isIpAddress(hostname)) {
      return listOf(InetAddress.getByName(hostname))
    }
    ...
  private fun isIpAddress(hostname: String): Boolean {
    return hostname[0].digitToIntOrNull(16) != null
      || hostname[0] == ':' || hostname[0] == '['
  }
getAllByName shortcircuits and avoids Dns if the hostname is an ip address. Using similar logic in isIpAddress.

It seems that the getAllByName function also needs some support functions such as textToNumericFormatV4/textToNumericFormatV6 from sun.net.util.IPAddressUtil class, I thought it might be btter to avoid import a class from sun.net package here.

TaoZang · 2022-02-12T17:14:58Z

Please add some tests to DnsOverHttpsTest.

I just added some tests in DnsOverHttpsText.

yschimke · 2022-02-13T08:13:01Z

I didn't mean import it, I just meant the method I showed or similar.

TaoZang · 2022-02-14T03:29:14Z

I just removed the complex support class.
Because the getAllName will shortcircuits and directly convert the hostname to InetAddress, so we only check the failed looking-up hostname when it is possible a valid IP address and convert it to InetAddress by getAllName.

okhttp-dnsoverhttps/src/test/java/okhttp3/dnsoverhttps/DnsOverHttpsTest.java

swankjesse

I’m unconvinced that this is the right layer for this fix. I think it’d be better if we never call Dns.lookup() when we have an IP address.

swankjesse · 2022-02-15T01:22:36Z

okhttp-dnsoverhttps/src/main/kotlin/okhttp3/dnsoverhttps/DnsOverHttps.kt

+    }
+  }
+
+  private fun checkIsIpAddress(hostname: String): Boolean {


Should use our existing function, canParseAsIpAddress() from okhttp3.internal. We definitely don’t want to functions to convert IP addresses to strings.

Sure, I missed that there is a existing function.

swankjesse · 2022-02-15T01:23:00Z

okhttp-dnsoverhttps/src/main/kotlin/okhttp3/dnsoverhttps/DnsOverHttps.kt

+      return true
+    }
+
+    if (hostname[0].digitToIntOrNull(16) != null || hostname[0] == ':' || hostname[0] == '[') {


If the hostname is [, that’s malformed. We do not pass the square braces to Dns.

I thought that the getAllByName will strip the square braces so I just pass the string to getAllByName directly.
Due to the canParseAsIpAddress will not handle [, I will strip the square braces and use the existing one to check if the hostname is a IP address.

…r function

yschimke · 2022-02-15T07:11:01Z

okhttp-dnsoverhttps/src/main/kotlin/okhttp3/dnsoverhttps/DnsOverHttps.kt

+      } else {
+        hostname
+      }
+      if (address.canParseAsIpAddress()) {


Nice find, I forgot this was a thing. Can you invert this now?

Also if that function doesn't worry about [], we can probably skip that here also.

See

okhttp/okhttp/src/jvmTest/java/okhttp3/HttpUrlTest.java

Line 514 in 3ad1912

assertThat(parse("http://[::1]/").host()).isEqualTo("::1");

Which confirms the HttpUrl strips the [].

Do you mean that the HttpUrl will handle the square braces before calling dns lookup, so we don't need to check and strips the [] here and just remove the test case for hostname "[::1]"?

swankjesse · 2022-02-15T12:07:01Z

At a risk of repeating myself, what happens if we never call any Dns implementation when the input is already an IP address?

yschimke · 2022-02-15T12:16:05Z

At a risk of repeating myself, what happens if we never call any Dns implementation when the input is already an IP address?

Yep - this makes sense. @TaoZang sorry to give you bad advice, and thanks for you patience. What do you think about make this change to avoid using Dns for IP Addresses.

TaoZang · 2022-02-15T12:48:20Z

Yep - this makes sense. @TaoZang sorry to give you bad advice, and thanks for you patience. What do you think about make this change to avoid using Dns for IP Addresses.

At a risk of repeating myself, what happens if we never call any Dns implementation when the input is already an IP address?

Yeah, maybe I just misunderstood your meaning before. Avoid to call DNS lookup in higher layer when the input is already an IP addres seems be make more sense. I will try to change the fix by this way.

yschimke · 2022-02-16T07:06:58Z

okhttp/src/jvmMain/kotlin/okhttp3/internal/connection/RouteSelector.kt

+      val addresses = if (socketHost.canParseAsIpAddress()) {
+        listOf(InetAddress.getByName(socketHost))
+      } else {
+        eventListener.dnsStart(call, socketHost)


This seems like a nice benefit of this change. that DNS start event isn't emitted for an IP address.

yschimke · 2022-02-16T07:07:54Z

okhttp-dnsoverhttps/src/main/kotlin/okhttp3/dnsoverhttps/DnsOverHttps.kt

@@ -53,6 +53,7 @@ class DnsOverHttps internal constructor(
  @get:JvmName("resolvePrivateAddresses") val resolvePrivateAddresses: Boolean,
  @get:JvmName("resolvePublicAddresses") val resolvePublicAddresses: Boolean
 ) : Dns {
+


nit: Can you revert this line.

yschimke

Please add a test that confirms this behaviour. Something in EventListenerTest like



  @Test public void successfulCallEventSequenceForIpAddress() throws IOException {
    server.enqueue(new MockResponse()
      .setBody("abc"));

    String ipAddress = InetAddress.getLocalHost().getHostAddress();

    Call call = client.newCall(new Request.Builder()
      .url(server.url("/").newBuilder().host(ipAddress).build())
      .build());
    Response response = call.execute();
    assertThat(response.code()).isEqualTo(200);
    assertThat(response.body().string()).isEqualTo("abc");
    response.body().close();

    assertThat(listener.recordedEventTypes()).containsExactly("CallStart",
      "ProxySelectStart", "ProxySelectEnd",
      "ConnectStart", "ConnectEnd", "ConnectionAcquired", "RequestHeadersStart",
      "RequestHeadersEnd", "ResponseHeadersStart", "ResponseHeadersEnd", "ResponseBodyStart",
      "ResponseBodyEnd", "ConnectionReleased", "CallEnd");
  }

TaoZang · 2022-02-16T07:24:58Z

Please add a test that confirms this behaviour. Something in EventListenerTest like



  @Test public void successfulCallEventSequenceForIpAddress() throws IOException {
    server.enqueue(new MockResponse()
      .setBody("abc"));

    String ipAddress = InetAddress.getLocalHost().getHostAddress();

    Call call = client.newCall(new Request.Builder()
      .url(server.url("/").newBuilder().host(ipAddress).build())
      .build());
    Response response = call.execute();
    assertThat(response.code()).isEqualTo(200);
    assertThat(response.body().string()).isEqualTo("abc");
    response.body().close();

    assertThat(listener.recordedEventTypes()).containsExactly("CallStart",
      "ProxySelectStart", "ProxySelectEnd",
      "ConnectStart", "ConnectEnd", "ConnectionAcquired", "RequestHeadersStart",
      "RequestHeadersEnd", "ResponseHeadersStart", "ResponseHeadersEnd", "ResponseBodyStart",
      "ResponseBodyEnd", "ConnectionReleased", "CallEnd");
  }

Sure, it seems a reasonable test case to cover this change.

swankjesse

Great PR, Great Test. Thank you!

JoshuaJamesOng · 2023-01-10T13:24:37Z

I'll backport this to 4.9.x when we have a fix, but until that happens you might want to have your own Dns wrapper that does this logic yourself.

Thanks for the report and PR.

@yschimke I see this change was accepted into the 5.x alphas. Are there any plans to backport this to 4.x, now?

) (cherry picked from commit 631a29e)

yschimke · 2023-01-11T09:18:44Z

Trying on #7648

…7648) * Cherrypick: fix DoH error when using ip address as hostname (#7073) (cherry picked from commit 631a29e) Co-authored-by: Tao.Zang <[email protected]>

fix DoH error when using ip address as hostname

f74fa03

yschimke reviewed Feb 11, 2022

View reviewed changes

lookup hostname first and try to parse as IP address only when lookup…

37be7f2

… failed

add unit test in DnsOverHttpsTest for the case that Doing DNS lookup …

f26f1cd

…with an IP address hostname

use simpler ipAddress check instead of complex support functions

1adcb89

yschimke reviewed Feb 14, 2022

View reviewed changes

okhttp-dnsoverhttps/src/test/java/okhttp3/dnsoverhttps/DnsOverHttpsTest.java Outdated Show resolved Hide resolved

swankjesse reviewed Feb 15, 2022

View reviewed changes

TaoZang added 2 commits February 15, 2022 10:42

add test case for some special ipv6 addresses

2cf4de1

use existing ipAddress check functions instead of adding a new simila…

47f50f5

…r function

yschimke reviewed Feb 15, 2022

View reviewed changes

TaoZang added 2 commits February 16, 2022 13:32

avoid to do dns lookup when the hostname is already an IP address

d0fb591

code style update

8158e31

yschimke reviewed Feb 16, 2022

View reviewed changes

yschimke approved these changes Feb 16, 2022

View reviewed changes

yschimke requested changes Feb 16, 2022

View reviewed changes

add test case

d9bb9fd

use loopback address instead of localhost

9614b50

yschimke approved these changes Feb 16, 2022

View reviewed changes

swankjesse approved these changes Feb 16, 2022

View reviewed changes

swankjesse merged commit 631a29e into square:master Feb 16, 2022

martinbonnin mentioned this pull request Aug 8, 2022

add kotlin("test") to commonTest yschimke/okhttp#18

Open

yschimke pushed a commit to yschimke/okhttp that referenced this pull request Jan 11, 2023

Cherrypick: fix DoH error when using ip address as hostname (square#7073

a35cc22

) (cherry picked from commit 631a29e)

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix DoH error when using ip address as hostname #7073

fix DoH error when using ip address as hostname #7073

TaoZang commented Feb 11, 2022

yschimke Feb 11, 2022

TaoZang Feb 11, 2022

yschimke Feb 11, 2022

TaoZang Feb 11, 2022

yschimke Feb 11, 2022

TaoZang Feb 11, 2022

yschimke Feb 11, 2022

TaoZang Feb 11, 2022

yschimke commented Feb 11, 2022

TaoZang commented Feb 11, 2022

yschimke commented Feb 12, 2022 •

edited

Loading

yschimke commented Feb 12, 2022

TaoZang commented Feb 12, 2022

TaoZang commented Feb 12, 2022

yschimke commented Feb 13, 2022

TaoZang commented Feb 14, 2022

swankjesse left a comment

swankjesse Feb 15, 2022

TaoZang Feb 15, 2022

swankjesse Feb 15, 2022

TaoZang Feb 15, 2022

yschimke Feb 15, 2022

TaoZang Feb 15, 2022

swankjesse commented Feb 15, 2022

yschimke commented Feb 15, 2022

TaoZang commented Feb 15, 2022

yschimke Feb 16, 2022 •

edited

Loading

yschimke Feb 16, 2022

yschimke left a comment

TaoZang commented Feb 16, 2022

swankjesse left a comment

JoshuaJamesOng commented Jan 10, 2023

yschimke commented Jan 11, 2023

fix DoH error when using ip address as hostname #7073

fix DoH error when using ip address as hostname #7073

Conversation

TaoZang commented Feb 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yschimke commented Feb 11, 2022

TaoZang commented Feb 11, 2022

yschimke commented Feb 12, 2022 • edited Loading

yschimke commented Feb 12, 2022

TaoZang commented Feb 12, 2022

TaoZang commented Feb 12, 2022

yschimke commented Feb 13, 2022

TaoZang commented Feb 14, 2022

swankjesse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swankjesse commented Feb 15, 2022

yschimke commented Feb 15, 2022

TaoZang commented Feb 15, 2022

yschimke Feb 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yschimke left a comment

Choose a reason for hiding this comment

TaoZang commented Feb 16, 2022

swankjesse left a comment

Choose a reason for hiding this comment

JoshuaJamesOng commented Jan 10, 2023

yschimke commented Jan 11, 2023

yschimke commented Feb 12, 2022 •

edited

Loading

yschimke Feb 16, 2022 •

edited

Loading