From 3ab60c91c85bd903cb7cb0d8f128cbd6b2fab0f7 Mon Sep 17 00:00:00 2001 From: Ikhun Um Date: Fri, 2 Aug 2024 18:10:42 +0900 Subject: [PATCH] Loosen the validation rule for URI authority Motivation: The authority part in a URI was validated by `URI.parseServerAuthority()` which only allows alphanumeric characters, `.` and `-`. https://github.com/openjdk/jdk/blob/dc35f3e8a84c8f622a4cabb8aee0f96de2e2ea30/src/java.base/share/classes/java/net/URI.java#L3513-L3515 As a result, if underscore (`_`) is set in an authority, `URISyntaxException` is raised. We think the rule is too strict because a request can also be sent to an instance when CSLB is used. `_` is a valid character in a DNS record. Users may want to send a host whose name is `beta_api.cloud.somewhere.com`. Related: #5814 Modifications: - Remove the usage of `URI.parseServerAuthority()` in `SchemeAndAuthority` - Parse a hostname and a port from a raw authority. Result: - Validation is relaxed to permit underscores (_) in URI's authority. - Closes #5814 --- .../internal/common/SchemeAndAuthority.java | 20 ++++- .../client/UnderscoredAuthorityTest.java | 75 +++++++++++++++++++ .../common/SchemeAndAuthorityTest.java | 16 ++-- .../{ => server}/ServiceFinderTest.java | 7 +- 4 files changed, 103 insertions(+), 15 deletions(-) create mode 100644 core/src/test/java/com/linecorp/armeria/client/UnderscoredAuthorityTest.java rename core/src/test/java/com/linecorp/armeria/{ => server}/ServiceFinderTest.java (94%) diff --git a/core/src/main/java/com/linecorp/armeria/internal/common/SchemeAndAuthority.java b/core/src/main/java/com/linecorp/armeria/internal/common/SchemeAndAuthority.java index fc9592e789a..4d4ca46493f 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/common/SchemeAndAuthority.java +++ b/core/src/main/java/com/linecorp/armeria/internal/common/SchemeAndAuthority.java @@ -18,9 +18,12 @@ import static java.util.Objects.requireNonNull; +import java.net.IDN; import java.net.URI; import java.net.URISyntaxException; +import com.google.common.net.HostAndPort; + import com.linecorp.armeria.common.annotation.Nullable; public final class SchemeAndAuthority { @@ -50,8 +53,21 @@ public static SchemeAndAuthority of(@Nullable String scheme, String authority) { final String authorityWithoutUserInfo = removeUserInfo(authority); try { - final URI uri = new URI(null, authorityWithoutUserInfo, null, null, null).parseServerAuthority(); - return new SchemeAndAuthority(scheme, uri.getRawAuthority(), uri.getHost(), uri.getPort()); + final URI uri = new URI(null, authorityWithoutUserInfo, null, null, null); + String rawAuthority = uri.getRawAuthority(); + if (rawAuthority == null) { + throw new IllegalArgumentException("Invalid authority: " + authority); + } + rawAuthority = IDN.toASCII(rawAuthority, IDN.ALLOW_UNASSIGNED); + + final boolean isIpv6 = rawAuthority.startsWith("["); + final HostAndPort hostAndPort = HostAndPort.fromString(rawAuthority); + String host = hostAndPort.getHost(); + if (isIpv6) { + host = '[' + host + ']'; + } + final int port = hostAndPort.getPortOrDefault(-1); + return new SchemeAndAuthority(scheme, rawAuthority, host, port); } catch (URISyntaxException e) { throw new IllegalArgumentException(e); } diff --git a/core/src/test/java/com/linecorp/armeria/client/UnderscoredAuthorityTest.java b/core/src/test/java/com/linecorp/armeria/client/UnderscoredAuthorityTest.java new file mode 100644 index 00000000000..61bb3db253a --- /dev/null +++ b/core/src/test/java/com/linecorp/armeria/client/UnderscoredAuthorityTest.java @@ -0,0 +1,75 @@ +/* + * Copyright 2024 LINE Corporation + * + * LINE Corporation licenses this file to you under the Apache License, + * version 2.0 (the "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at: + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ + +package com.linecorp.armeria.client; + +import static org.assertj.core.api.Assertions.assertThat; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; + +import com.linecorp.armeria.common.AggregatedHttpResponse; +import com.linecorp.armeria.common.HttpResponse; +import com.linecorp.armeria.common.HttpStatus; +import com.linecorp.armeria.common.logging.RequestLog; +import com.linecorp.armeria.internal.testing.MockAddressResolverGroup; +import com.linecorp.armeria.server.ServerBuilder; +import com.linecorp.armeria.testing.junit5.server.ServerExtension; + +class UnderscoredAuthorityTest { + + @RegisterExtension + static ServerExtension server = new ServerExtension() { + @Override + protected void configure(ServerBuilder sb) { + sb.service("/", (ctx, req) -> HttpResponse.of("Hello, world!")); + } + }; + + @Test + void shouldAllowUnderscoreInAuthority() { + try (ClientFactory factory = ClientFactory.builder() + .addressResolverGroupFactory(unused -> { + return MockAddressResolverGroup.localhost(); + }) + .build()) { + final BlockingWebClient client = + WebClient.builder("http://my_key.z1.armeria.dev:" + server.httpPort()) + .factory(factory) + .build() + .blocking(); + + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + final AggregatedHttpResponse response = client.get("/"); + final ClientRequestContext ctx = captor.get(); + assertThat(response.status()).isEqualTo(HttpStatus.OK); + assertThat(response.contentUtf8()).isEqualTo("Hello, world!"); + final RequestLog log = ctx.log().whenComplete().join(); + + assertThat(log.requestHeaders().authority()) + .isEqualTo("my_key.z1.armeria.dev:" + server.httpPort()); + } + } + } + + @Test + void shouldAllowUnderscoreInEndpoint() { + final Endpoint endpoint = Endpoint.parse("my_key.z1.armeria.dev:" + server.httpPort()); + assertThat(endpoint.authority()).isEqualTo("my_key.z1.armeria.dev:" + server.httpPort()); + assertThat(endpoint.host()).isEqualTo("my_key.z1.armeria.dev"); + assertThat(endpoint.port()).isEqualTo(server.httpPort()); + } +} diff --git a/core/src/test/java/com/linecorp/armeria/internal/common/SchemeAndAuthorityTest.java b/core/src/test/java/com/linecorp/armeria/internal/common/SchemeAndAuthorityTest.java index 2df2eaec6dd..7a9a1c2e59d 100644 --- a/core/src/test/java/com/linecorp/armeria/internal/common/SchemeAndAuthorityTest.java +++ b/core/src/test/java/com/linecorp/armeria/internal/common/SchemeAndAuthorityTest.java @@ -25,13 +25,15 @@ class SchemeAndAuthorityTest { @ParameterizedTest @CsvSource({ - "0.0.0.0:80, 0.0.0.0:80, 0.0.0.0, 80", // IPv4 - "[::1]:8080, [::1]:8080, [::1], 8080", // IPv6 - "unix%3Afoo.sock, unix%3Afoo.sock, unix%3Afoo.sock, -1", // Domain socket - "foo.bar, foo.bar, foo.bar, -1", // Only host - "foo:, foo:, foo, -1", // Empty port - "bar:80, bar:80, bar, 80", // Host and port - "foo@bar:80, bar:80, bar, 80", // Userinfo and host and port + "0.0.0.0:80, 0.0.0.0:80, 0.0.0.0, 80", // IPv4 + "[::1]:8080, [::1]:8080, [::1], 8080", // IPv6 + "unix%3Afoo.sock, unix%3Afoo.sock, unix%3Afoo.sock, -1", // Domain socket + "foo.bar, foo.bar, foo.bar, -1", // Only host + "foo:, foo:, foo, -1", // Empty port + "bar:80, bar:80, bar, 80", // Host and port + "foo@bar:80, bar:80, bar, 80", // Userinfo and host and port + "foo_bar:80, foo_bar:80, foo_bar, 80", // Underscore in host + "한글.com:80, xn--bj0bj06e.com:80, xn--bj0bj06e.com, 80", // IDN }) void fromAuthority(String authority, String expectedAuthority, String expectedHost, int expectedPort) { diff --git a/core/src/test/java/com/linecorp/armeria/ServiceFinderTest.java b/core/src/test/java/com/linecorp/armeria/server/ServiceFinderTest.java similarity index 94% rename from core/src/test/java/com/linecorp/armeria/ServiceFinderTest.java rename to core/src/test/java/com/linecorp/armeria/server/ServiceFinderTest.java index 45df2d505ee..1c34a055aa2 100644 --- a/core/src/test/java/com/linecorp/armeria/ServiceFinderTest.java +++ b/core/src/test/java/com/linecorp/armeria/server/ServiceFinderTest.java @@ -14,7 +14,7 @@ * under the License. */ -package com.linecorp.armeria; +package com.linecorp.armeria.server; import static org.assertj.core.api.Assertions.assertThat; @@ -28,11 +28,6 @@ import com.linecorp.armeria.common.HttpStatus; import com.linecorp.armeria.common.annotation.Nullable; import com.linecorp.armeria.common.metric.MeterIdPrefixFunction; -import com.linecorp.armeria.server.HttpService; -import com.linecorp.armeria.server.Server; -import com.linecorp.armeria.server.ServerBuilder; -import com.linecorp.armeria.server.ServiceRequestContext; -import com.linecorp.armeria.server.SimpleDecoratingHttpService; import com.linecorp.armeria.server.cors.CorsService; import com.linecorp.armeria.server.encoding.EncodingService; import com.linecorp.armeria.server.logging.LoggingService;