diff --git a/api/src/main/java/io/grpc/Grpc.java b/api/src/main/java/io/grpc/Grpc.java index 628f6fac399..bb540559c3b 100644 --- a/api/src/main/java/io/grpc/Grpc.java +++ b/api/src/main/java/io/grpc/Grpc.java @@ -159,10 +159,41 @@ private static String authorityFromHostAndPort(String host, int port) { try { return new URI(null, null, host, port, null, null, null).getAuthority(); } catch (URISyntaxException ex) { + String placeholderHost = addPlaceholderLabelIfLastLabelStartsWithDigit(host); + if (placeholderHost != null) { + try { + new URI(null, null, placeholderHost, port, null, null, null); + return port < 0 ? host : host + ":" + port; + } catch (URISyntaxException ignored) { + // Fall through and throw original exception. + } + } throw new IllegalArgumentException("Invalid host or port: " + host + " " + port, ex); } } + /** + * Workaround for JDK-8188305: {@link URI} enforces RFC 2396's {@code toplabel} rule requiring + * the final label of a multi-label hostname to start with an ASCII letter, whereas RFC 1123 + * Section 2.1 and RFC 3986 allow it to start with an ASCII digit. + */ + private static String addPlaceholderLabelIfLastLabelStartsWithDigit(String host) { + if (host == null || host.indexOf('@') != -1) { + return null; + } + boolean trailingDot = host.endsWith("."); + int end = trailingDot ? host.length() - 1 : host.length(); + int lastDot = host.lastIndexOf('.', end - 1); + if (lastDot <= 0 || lastDot + 1 >= end) { + return null; + } + char firstCharOfLastLabel = host.charAt(lastDot + 1); + if (firstCharOfLastLabel < '0' || firstCharOfLastLabel > '9') { + return null; + } + return trailingDot ? host.substring(0, end) + ".a." : host + ".a"; + } + /** * Static factory for creating a new ServerBuilder. * diff --git a/core/src/main/java/io/grpc/internal/DnsNameResolver.java b/core/src/main/java/io/grpc/internal/DnsNameResolver.java index 1c1d95ed616..8960f18167c 100644 --- a/core/src/main/java/io/grpc/internal/DnsNameResolver.java +++ b/core/src/main/java/io/grpc/internal/DnsNameResolver.java @@ -163,14 +163,15 @@ protected DnsNameResolver( // Must prepend a "//" to the name when constructing a URI, otherwise it will be treated as an // opaque URI, thus the authority and host of the resulted URI would be null. URI nameUri = URI.create("//" + checkNotNull(name, "name")); - Preconditions.checkArgument(nameUri.getHost() != null, "Invalid DNS name: %s", name); + host = GrpcUtil.getHost(nameUri); + Preconditions.checkArgument(host != null, "Invalid DNS name: %s", name); authority = Preconditions.checkNotNull(nameUri.getAuthority(), "nameUri (%s) doesn't have an authority", nameUri); - host = nameUri.getHost(); - if (nameUri.getPort() == -1) { + int uriPort = GrpcUtil.getPort(nameUri); + if (uriPort == -1) { port = args.getDefaultPort(); } else { - port = nameUri.getPort(); + port = uriPort; } this.proxyDetector = checkNotNull(args.getProxyDetector(), "proxyDetector"); Executor offloadExecutor = args.getOffloadExecutor(); diff --git a/core/src/main/java/io/grpc/internal/GrpcUtil.java b/core/src/main/java/io/grpc/internal/GrpcUtil.java index bb52eaf0441..3ea7c1f26eb 100644 --- a/core/src/main/java/io/grpc/internal/GrpcUtil.java +++ b/core/src/main/java/io/grpc/internal/GrpcUtil.java @@ -547,10 +547,101 @@ public static String authorityFromHostAndPort(String host, int port) { try { return new URI(null, null, host, port, null, null, null).getAuthority(); } catch (URISyntaxException ex) { + String placeholderHost = addPlaceholderLabelIfLastLabelStartsWithDigit(host); + if (placeholderHost != null) { + try { + new URI(null, null, placeholderHost, port, null, null, null); + return port < 0 ? host : host + ":" + port; + } catch (URISyntaxException ignored) { + // Fall through and throw original exception. + } + } throw new IllegalArgumentException("Invalid host or port: " + host + " " + port, ex); } } + /** + * Returns the host of {@code uri}, working around JDK-8188305 where {@link URI#getHost()} returns + * {@code null} if the final label of a multi-label hostname starts with a digit. + */ + @Nullable + public static String getHost(URI uri) { + String host = uri.getHost(); + if (host != null) { + return host; + } + URI placeholderUri = getPlaceholderAuthorityUri(uri); + if (placeholderUri != null) { + String placeholderHost = placeholderUri.getHost(); + if (placeholderHost.endsWith(".a.")) { + return placeholderHost.substring(0, placeholderHost.length() - 3) + "."; + } + return placeholderHost.substring(0, placeholderHost.length() - 2); + } + return null; + } + + /** + * Returns the port of {@code uri}, working around JDK-8188305 where {@link URI#getPort()} returns + * {@code -1} if the final label of a multi-label hostname starts with a digit. + */ + public static int getPort(URI uri) { + int port = uri.getPort(); + if (port != -1) { + return port; + } + URI placeholderUri = getPlaceholderAuthorityUri(uri); + return placeholderUri != null ? placeholderUri.getPort() : -1; + } + + @Nullable + private static URI getPlaceholderAuthorityUri(URI uri) { + if (uri.getHost() != null) { + return uri; + } + String authority = uri.getAuthority(); + if (authority == null || authority.startsWith("[") || authority.indexOf('@') != -1) { + return null; + } + int colonIndex = authority.indexOf(':'); + String host = colonIndex != -1 ? authority.substring(0, colonIndex) : authority; + String placeholderHost = addPlaceholderLabelIfLastLabelStartsWithDigit(host); + if (placeholderHost == null) { + return null; + } + String placeholderAuthority = + colonIndex != -1 ? placeholderHost + authority.substring(colonIndex) : placeholderHost; + try { + URI placeholderUri = new URI(null, placeholderAuthority, null, null, null); + return placeholderUri.getHost() != null ? placeholderUri : null; + } catch (URISyntaxException e) { + return null; + } + } + + /** + * Workaround for JDK-8188305: {@link URI} enforces RFC 2396's {@code toplabel} rule requiring + * the final label of a multi-label hostname to start with an ASCII letter, whereas RFC 1123 + * Section 2.1 and RFC 3986 allow it to start with an ASCII digit. + */ + @Nullable + private static String addPlaceholderLabelIfLastLabelStartsWithDigit(String host) { + if (host == null || host.indexOf('@') != -1) { + return null; + } + boolean trailingDot = host.endsWith("."); + int end = trailingDot ? host.length() - 1 : host.length(); + int lastDot = host.lastIndexOf('.', end - 1); + if (lastDot <= 0 || lastDot + 1 >= end) { + return null; + } + char firstCharOfLastLabel = host.charAt(lastDot + 1); + if (firstCharOfLastLabel < '0' || firstCharOfLastLabel > '9') { + return null; + } + return trailingDot ? host.substring(0, end) + ".a." : host + ".a"; + } + /** * Shared executor for channels. */ diff --git a/core/src/main/java/io/grpc/internal/ProxyDetectorImpl.java b/core/src/main/java/io/grpc/internal/ProxyDetectorImpl.java index 2f9903eac03..cf19c50ae36 100644 --- a/core/src/main/java/io/grpc/internal/ProxyDetectorImpl.java +++ b/core/src/main/java/io/grpc/internal/ProxyDetectorImpl.java @@ -183,16 +183,15 @@ private ProxiedSocketAddress detectProxy(InetSocketAddress targetAddr) throws IO URI uri; String host = targetAddr.getHostString(); try { + String authority = GrpcUtil.authorityFromHostAndPort(host, targetAddr.getPort()); uri = new URI( PROXY_SCHEME, - null, /* userInfo */ - host, - targetAddr.getPort(), + authority, null, /* path */ null, /* query */ null /* fragment */); - } catch (final URISyntaxException e) { + } catch (final URISyntaxException | IllegalArgumentException e) { log.log( Level.WARNING, "Failed to construct URI for proxy lookup, proceeding without proxy", diff --git a/core/src/test/java/io/grpc/internal/DnsNameResolverTest.java b/core/src/test/java/io/grpc/internal/DnsNameResolverTest.java index c53863dcf5d..536a1b99128 100644 --- a/core/src/test/java/io/grpc/internal/DnsNameResolverTest.java +++ b/core/src/test/java/io/grpc/internal/DnsNameResolverTest.java @@ -220,6 +220,23 @@ public void invalidDnsName_containsUnderscore() { } } + @Test + public void validDnsName_lastLabelStartsWithDigit() { + DnsNameResolver resolverWithoutPort = + (DnsNameResolver) newResolver("otlp.1234-k8s-namespace", DEFAULT_PORT) + .getRetriedNameResolver(); + assertEquals("otlp.1234-k8s-namespace", resolverWithoutPort.getServiceAuthority()); + assertEquals("otlp.1234-k8s-namespace", resolverWithoutPort.getHost()); + assertEquals(DEFAULT_PORT, resolverWithoutPort.getPort()); + + DnsNameResolver resolverWithPort = + (DnsNameResolver) newResolver("otlp.1234-k8s-namespace:4317", DEFAULT_PORT) + .getRetriedNameResolver(); + assertEquals("otlp.1234-k8s-namespace:4317", resolverWithPort.getServiceAuthority()); + assertEquals("otlp.1234-k8s-namespace", resolverWithPort.getHost()); + assertEquals(4317, resolverWithPort.getPort()); + } + @Test public void resolve_neverCache() throws Exception { flagResetRule.setSystemPropertyForTest(NETWORKADDRESS_CACHE_TTL_PROPERTY, "0"); diff --git a/core/src/test/java/io/grpc/internal/GrpcUtilTest.java b/core/src/test/java/io/grpc/internal/GrpcUtilTest.java index dbc3051628f..9f6a613102b 100644 --- a/core/src/test/java/io/grpc/internal/GrpcUtilTest.java +++ b/core/src/test/java/io/grpc/internal/GrpcUtilTest.java @@ -39,6 +39,7 @@ import io.grpc.internal.ClientStreamListener.RpcProgress; import io.grpc.internal.GrpcUtil.Http2Error; import io.grpc.testing.TestMethodDescriptors; +import java.net.URI; import java.util.ArrayList; import org.junit.Rule; import org.junit.Test; @@ -239,6 +240,41 @@ public void checkAuthority_userInfoNotAllowed() { .isEqualTo("Userinfo must not be present on authority: 'foo@valid'"); } + @Test + public void authorityFromHostAndPort_lastLabelStartsWithDigit() { + assertEquals( + "otlp.1234-k8s-namespace:4317", + GrpcUtil.authorityFromHostAndPort("otlp.1234-k8s-namespace", 4317)); + assertEquals( + "otlp.1234-k8s-namespace.:4317", + GrpcUtil.authorityFromHostAndPort("otlp.1234-k8s-namespace.", 4317)); + assertThrows( + IllegalArgumentException.class, + () -> GrpcUtil.authorityFromHostAndPort("bad_host.1234", 4317)); + assertThrows( + IllegalArgumentException.class, + () -> GrpcUtil.authorityFromHostAndPort("otlp.1234-k8s-namespace", -2)); + } + + @Test + public void getHostAndPort_lastLabelStartsWithDigit() { + URI uriWithPort = GrpcUtil.authorityToUri("otlp.1234-k8s-namespace:4317"); + assertEquals("otlp.1234-k8s-namespace", GrpcUtil.getHost(uriWithPort)); + assertEquals(4317, GrpcUtil.getPort(uriWithPort)); + + URI uriWithoutPort = GrpcUtil.authorityToUri("otlp.1234-k8s-namespace"); + assertEquals("otlp.1234-k8s-namespace", GrpcUtil.getHost(uriWithoutPort)); + assertEquals(-1, GrpcUtil.getPort(uriWithoutPort)); + + URI uriWithTrailingDot = GrpcUtil.authorityToUri("otlp.1234-k8s-namespace.:4317"); + assertEquals("otlp.1234-k8s-namespace.", GrpcUtil.getHost(uriWithTrailingDot)); + assertEquals(4317, GrpcUtil.getPort(uriWithTrailingDot)); + + URI invalidUri = GrpcUtil.authorityToUri("bad_host.1234:4317"); + assertNull(GrpcUtil.getHost(invalidUri)); + assertEquals(-1, GrpcUtil.getPort(invalidUri)); + } + @Test public void httpStatusToGrpcStatus_messageContainsHttpStatus() { assertTrue(GrpcUtil.httpStatusToGrpcStatus(500).getDescription().contains("500")); diff --git a/core/src/test/java/io/grpc/internal/ProxyDetectorImplTest.java b/core/src/test/java/io/grpc/internal/ProxyDetectorImplTest.java index af0ed1f35d3..beead967aac 100644 --- a/core/src/test/java/io/grpc/internal/ProxyDetectorImplTest.java +++ b/core/src/test/java/io/grpc/internal/ProxyDetectorImplTest.java @@ -102,6 +102,24 @@ public void detectProxyForUnresolvedDestination() throws Exception { assertEquals(proxySocketAddress, detected); } + @Test + public void detectProxyForHostnameWithLastLabelStartingWithDigit() throws Exception { + InetSocketAddress digitToplevelDest = + InetSocketAddress.createUnresolved("otlp.1234-k8s-namespace", 4317); + Proxy proxy = new Proxy(Proxy.Type.HTTP, unresolvedProxy); + when(proxySelector.select(URI.create("https://otlp.1234-k8s-namespace:4317"))) + .thenReturn(ImmutableList.of(proxy)); + + ProxiedSocketAddress detected = proxyDetector.proxyFor(digitToplevelDest); + assertNotNull(detected); + HttpConnectProxiedSocketAddress expected = HttpConnectProxiedSocketAddress.newBuilder() + .setTargetAddress(digitToplevelDest) + .setProxyAddress( + new InetSocketAddress(InetAddress.getByName(unresolvedProxy.getHostName()), proxyPort)) + .build(); + assertEquals(expected, detected); + } + @Test public void detectProxyForResolvedDestination() throws Exception { InetSocketAddress resolved = new InetSocketAddress(InetAddress.getByName("10.1.2.3"), 10); diff --git a/netty/src/main/java/io/grpc/netty/ProtocolNegotiators.java b/netty/src/main/java/io/grpc/netty/ProtocolNegotiators.java index e825aa6a8bd..72deb9ccd61 100644 --- a/netty/src/main/java/io/grpc/netty/ProtocolNegotiators.java +++ b/netty/src/main/java/io/grpc/netty/ProtocolNegotiators.java @@ -749,11 +749,10 @@ private void propagateTlsComplete(ChannelHandlerContext ctx, SSLSession session) @VisibleForTesting static HostPort parseAuthority(String authority) { URI uri = GrpcUtil.authorityToUri(Preconditions.checkNotNull(authority, "authority")); - String host; + String host = GrpcUtil.getHost(uri); int port; - if (uri.getHost() != null) { - host = uri.getHost(); - port = uri.getPort(); + if (host != null) { + port = GrpcUtil.getPort(uri); } else { /* * Implementation note: We pick -1 as the port here rather than deriving it from the diff --git a/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java b/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java index 5a9875c3cc1..550a9509b5e 100644 --- a/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java @@ -24,6 +24,7 @@ import static org.mockito.Mockito.mock; import io.grpc.ChannelCredentials; +import io.grpc.Grpc; import io.grpc.InsecureChannelCredentials; import io.grpc.ManagedChannel; import io.grpc.Metadata; @@ -135,6 +136,29 @@ public void authorityIsReadable() throws Exception { } } + @Test + public void authorityFromHostWithLastLabelStartingWithDigit() throws Exception { + NettyChannelBuilder builder = + NettyChannelBuilder.forAddress("otlp.1234-k8s-namespace", 4317); + + ManagedChannel b = builder.build(); + try { + assertEquals("otlp.1234-k8s-namespace:4317", b.authority()); + } finally { + shutdown(b); + } + + ManagedChannel b2 = + Grpc.newChannelBuilderForAddress( + "otlp.1234-k8s-namespace", 4317, InsecureChannelCredentials.create()) + .build(); + try { + assertEquals("otlp.1234-k8s-namespace:4317", b2.authority()); + } finally { + shutdown(b2); + } + } + @Test public void overrideAuthorityIsReadableForAddress() throws Exception { NettyChannelBuilder builder = NettyChannelBuilder.forAddress("original", 1234); diff --git a/netty/src/test/java/io/grpc/netty/ProtocolNegotiatorsTest.java b/netty/src/test/java/io/grpc/netty/ProtocolNegotiatorsTest.java index fa16db891c3..63492865d81 100644 --- a/netty/src/test/java/io/grpc/netty/ProtocolNegotiatorsTest.java +++ b/netty/src/test/java/io/grpc/netty/ProtocolNegotiatorsTest.java @@ -1124,6 +1124,14 @@ public void tls_host() { assertEquals(-1, hostPort.port); } + @Test + public void tls_hostLastLabelStartsWithDigit() { + HostPort hostPort = ProtocolNegotiators.parseAuthority("otlp.1234-k8s-namespace:4317"); + + assertEquals("otlp.1234-k8s-namespace", hostPort.host); + assertEquals(4317, hostPort.port); + } + @Test public void tls_invalidHost() throws SSLException { HostPort hostPort = ProtocolNegotiators.parseAuthority("bad_host:1234"); diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpClientTransport.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpClientTransport.java index 4764a6a1387..8f5f110a981 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpClientTransport.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpClientTransport.java @@ -970,8 +970,9 @@ public InternalLogId getLogId() { @VisibleForTesting String getOverridenHost() { URI uri = GrpcUtil.authorityToUri(defaultAuthority); - if (uri.getHost() != null) { - return uri.getHost(); + String host = GrpcUtil.getHost(uri); + if (host != null) { + return host; } return defaultAuthority; @@ -980,8 +981,9 @@ String getOverridenHost() { @VisibleForTesting int getOverridenPort() { URI uri = GrpcUtil.authorityToUri(defaultAuthority); - if (uri.getPort() != -1) { - return uri.getPort(); + int port = GrpcUtil.getPort(uri); + if (port != -1) { + return port; } return address.getPort(); diff --git a/okhttp/src/test/java/io/grpc/okhttp/OkHttpChannelBuilderTest.java b/okhttp/src/test/java/io/grpc/okhttp/OkHttpChannelBuilderTest.java index 89d37536b70..681abbb989b 100644 --- a/okhttp/src/test/java/io/grpc/okhttp/OkHttpChannelBuilderTest.java +++ b/okhttp/src/test/java/io/grpc/okhttp/OkHttpChannelBuilderTest.java @@ -76,6 +76,14 @@ public void authorityIsReadable() { assertEquals("original:1234", channel.authority()); } + @Test + public void authorityFromHostWithLastLabelStartingWithDigit() { + OkHttpChannelBuilder builder = + OkHttpChannelBuilder.forAddress("otlp.1234-k8s-namespace", 4317); + ManagedChannel channel = grpcCleanupRule.register(builder.build()); + assertEquals("otlp.1234-k8s-namespace:4317", channel.authority()); + } + @Test public void overrideAuthorityIsReadableForAddress() { OkHttpChannelBuilder builder = OkHttpChannelBuilder.forAddress("original", 1234); diff --git a/okhttp/src/test/java/io/grpc/okhttp/OkHttpClientTransportTest.java b/okhttp/src/test/java/io/grpc/okhttp/OkHttpClientTransportTest.java index 0b571530db4..bfc2ceb3460 100644 --- a/okhttp/src/test/java/io/grpc/okhttp/OkHttpClientTransportTest.java +++ b/okhttp/src/test/java/io/grpc/okhttp/OkHttpClientTransportTest.java @@ -1827,6 +1827,22 @@ public void invalidAuthorityPropagates() { assertEquals(1234, port); } + @Test + public void authorityWithLastLabelStartingWithDigit() { + clientTransport = new OkHttpClientTransport( + channelBuilder.buildTransportFactory(), + new InetSocketAddress("localhost", 1234), + "otlp.1234-k8s-namespace:4317", + "userAgent", + EAG_ATTRS, + NO_PROXY, + tooManyPingsRunnable, + null); + + assertEquals("otlp.1234-k8s-namespace", clientTransport.getOverridenHost()); + assertEquals(4317, clientTransport.getOverridenPort()); + } + @Test public void unreachableServer() throws Exception { clientTransport = new OkHttpClientTransport(