Fix a bug where SNI doesn't reflect the :authority of a request

core/src/main/java/com/linecorp/armeria/client/HttpChannelPool.java

@@ -622,9 +622,7 @@ static final class PoolKey {
         private final int hashCode;
 
         PoolKey(Endpoint endpoint, ProxyConfig proxyConfig) {
-            // Remove the trailing dot of the host name because SNI does not allow it.
-            // https://lists.w3.org/Archives/Public/ietf-http-wg/2016JanMar/0430.html
-            this.endpoint = endpoint.withoutTrailingDot();
+            this.endpoint = endpoint;
             this.proxyConfig = proxyConfig;
             hashCode = endpoint.hashCode() * 31 + proxyConfig.hashCode();
         }

core/src/main/java/com/linecorp/armeria/client/HttpClientDelegate.java

@@ -42,12 +42,14 @@
 import com.linecorp.armeria.internal.client.HttpSession;
 import com.linecorp.armeria.internal.client.PooledChannel;
 import com.linecorp.armeria.internal.common.RequestContextUtil;
+import com.linecorp.armeria.internal.common.SchemeAndAuthority;
 import com.linecorp.armeria.server.ProxiedAddresses;
 import com.linecorp.armeria.server.ServiceRequestContext;
 
 import io.netty.channel.Channel;
 import io.netty.channel.EventLoop;
 import io.netty.resolver.AddressResolverGroup;
+import io.netty.util.NetUtil;
 import io.netty.util.concurrent.Future;
 
 final class HttpClientDelegate implements HttpClient {
@@ -214,6 +216,20 @@ private void acquireConnectionAndExecute0(ClientRequestContext ctx, Endpoint end
                                               HttpRequest req, DecodedHttpResponse res,
                                               ClientConnectionTimingsBuilder timingsBuilder,
                                               ProxyConfig proxyConfig) {
+        final SessionProtocol protocol = ctx.sessionProtocol();
+        if (protocol.isTls() && endpoint.isIpAddrOnly()) {
+            // The connection will be established with the IP address but `host` set to the `Endpoint`
+            // could be used for SNI. It would make users send HTTPS requests with CSLB or configure a reverse
+            // proxy based on an authority.
+            final String serverName = authorityToServerName(ctx.authority());
+            if (serverName != null) {
+                endpoint = endpoint.withHost(serverName);
+            }
+        }
+        // Remove the trailing dot of the host name because SNI does not allow it.
+        // https://lists.w3.org/Archives/Public/ietf-http-wg/2016JanMar/0430.html
+        endpoint = endpoint.withoutTrailingDot();
+
         final PoolKey key = new PoolKey(endpoint, proxyConfig);
         final HttpChannelPool pool;
         try {
@@ -222,7 +238,6 @@ private void acquireConnectionAndExecute0(ClientRequestContext ctx, Endpoint end
             earlyCancelRequest(t, ctx, timingsBuilder);
             return;
         }
-        final SessionProtocol protocol = ctx.sessionProtocol();
         final SerializationFormat serializationFormat = ctx.log().partial().serializationFormat();
         final PooledChannel pooledChannel = pool.acquireNow(protocol, serializationFormat, key);
         if (pooledChannel != null) {
@@ -242,6 +257,22 @@ private void acquireConnectionAndExecute0(ClientRequestContext ctx, Endpoint end
         }
     }
 
+    @Nullable
+    private static String authorityToServerName(@Nullable String authority) {
+        if (authority == null) {
+            return null;
+        }
+        String serverName = SchemeAndAuthority.of(null, authority).host();
+        if (NetUtil.isValidIpV4Address(serverName) || NetUtil.isValidIpV6Address(serverName)) {
+            return null;
+        }
+        serverName = serverName.trim();
+        if (serverName.isEmpty()) {
+            return null;
+        }
+        return serverName;
+    }
+
     private void resolveProxyConfig(SessionProtocol protocol, Endpoint endpoint, ClientRequestContext ctx,
                                     BiConsumer<@Nullable ProxyConfig, @Nullable Throwable> onComplete) {
         final ProxyConfig unresolvedProxyConfig = factory.proxyConfigSelector().select(protocol, endpoint);

core/src/main/java/com/linecorp/armeria/internal/client/DefaultClientRequestContext.java

@@ -97,7 +97,6 @@
 import io.netty.channel.EventLoop;
 import io.netty.handler.codec.http.HttpHeaderValues;
 import io.netty.util.AttributeKey;
-import io.netty.util.NetUtil;
 
 /**
  * Default {@link ClientRequestContext} implementation.
@@ -512,7 +511,9 @@ public void finishInitialization(boolean success) {
 
     private void updateEndpoint(@Nullable Endpoint endpoint) {
         this.endpoint = endpoint;
-        autoFillSchemeAuthorityAndOrigin();
+        internalRequestHeaders = computeInternalHeaders(defaultInternalRequestHeaders,
+                                                        endpoint, sessionProtocol,
+                                                        options().autoFillOriginHeader());
     }
 
     private void acquireEventLoop(EndpointGroup endpointGroup) {
@@ -569,7 +570,9 @@ private void failEarly(Throwable cause) {
         final UnprocessedRequestException wrapped = UnprocessedRequestException.of(cause);
         final HttpRequest req = request();
         if (req != null) {
-            autoFillSchemeAuthorityAndOrigin();
+            internalRequestHeaders = computeInternalHeaders(
+                    defaultInternalRequestHeaders, null, sessionProtocol,
+                    options().autoFillOriginHeader());
             req.abort(wrapped);
         }
 
@@ -579,34 +582,21 @@ private void failEarly(Throwable cause) {
         responseCancellationScheduler.finishNow(cause);
     }
 
-    // TODO(ikhoon): Consider moving the logic for filling authority to `HttpClientDelegate.exceute()`.
-    private void autoFillSchemeAuthorityAndOrigin() {
-        final String authority = authority();
-        if (authority != null && endpoint != null && endpoint.isIpAddrOnly()) {
-            // The connection will be established with the IP address but `host` set to the `Endpoint`
-            // could be used for SNI. It would make users send HTTPS requests with CSLB or configure a reverse
-            // proxy based on an authority.
-            final String host = SchemeAndAuthority.of(null, authority).host();
-            if (!NetUtil.isValidIpV4Address(host) && !NetUtil.isValidIpV6Address(host)) {
-                endpoint = endpoint.withHost(host);
-            }
-        }
-
-        final HttpHeadersBuilder headersBuilder = internalRequestHeaders.toBuilder();
-        headersBuilder.set(HttpHeaderNames.SCHEME, getScheme(sessionProtocol()));
+    private static HttpHeaders computeInternalHeaders(
+            HttpHeaders internalHeaders, @Nullable Endpoint endpoint,
+            SessionProtocol sessionProtocol, boolean autoFillOriginHeader) {
+        final HttpHeadersBuilder headersBuilder = internalHeaders.toBuilder();
+        headersBuilder.set(HttpHeaderNames.SCHEME, getScheme(sessionProtocol));
         if (endpoint != null) {
             final String endpointAuthority = endpoint.authority();
             headersBuilder.set(HttpHeaderNames.AUTHORITY, endpointAuthority);
-            final String origin = origin();
-            if (origin != null) {
-                headersBuilder.set(HttpHeaderNames.ORIGIN, origin);
-            } else if (options().autoFillOriginHeader()) {
-                final String uriText = sessionProtocol().isTls() ? SessionProtocol.HTTPS.uriText()
-                                                                 : SessionProtocol.HTTP.uriText();
+            if (autoFillOriginHeader) {
+                final String uriText = sessionProtocol.isTls() ? SessionProtocol.HTTPS.uriText()
+                                                               : SessionProtocol.HTTP.uriText();
                 headersBuilder.set(HttpHeaderNames.ORIGIN, uriText + "://" + endpointAuthority);
             }
         }
-        internalRequestHeaders = headersBuilder.build();
+        return headersBuilder.build();
     }
 
     /**
@@ -905,23 +895,6 @@ public String authority() {
         return authority;
     }
 
-    @Nullable
-    private String origin() {
-        final HttpHeaders additionalRequestHeaders = this.additionalRequestHeaders;
-        String origin = additionalRequestHeaders.get(HttpHeaderNames.ORIGIN);
-        final HttpRequest request = request();
-        if (origin == null && request != null) {
-            origin = request.headers().get(HttpHeaderNames.ORIGIN);
-        }
-        if (origin == null) {
-            origin = defaultRequestHeaders.get(HttpHeaderNames.ORIGIN);
-        }
-        if (origin == null) {
-            origin = internalRequestHeaders.get(HttpHeaderNames.ORIGIN);
-        }
-        return origin;
-    }
-
     @Nullable
     @Override
     public String host() {

core/src/test/java/com/linecorp/armeria/client/TrailingDotSniTest.java

@@ -24,6 +24,7 @@
 import org.junit.jupiter.api.extension.RegisterExtension;
 
 import com.linecorp.armeria.common.AggregatedHttpResponse;
+import com.linecorp.armeria.common.HttpHeaderNames;
 import com.linecorp.armeria.common.HttpResponse;
 import com.linecorp.armeria.common.HttpStatus;
 import com.linecorp.armeria.common.TlsProvider;
@@ -73,4 +74,24 @@ void shouldStripTrailingDotForSni() {
             assertThat(response.status()).isEqualTo(HttpStatus.OK);
         }
     }
+
+    @Test
+    void usesAuthorityHeader() {
+        try (ClientFactory factory = ClientFactory.builder()
+                                                  .tlsCustomizer(b -> b.trustManager(ssc.certificate()))
+                                                  .build()) {
+
+            final BlockingWebClient client =
+                    WebClient.builder(server.httpsUri())
+                             .factory(factory)
+                             .decorator((delegate, ctx, req) -> {
+                                 ctx.setAdditionalRequestHeader(HttpHeaderNames.AUTHORITY, "example.com");
+                                 return delegate.execute(ctx, req);
+                             })
+                             .build()
+                             .blocking();
+            final AggregatedHttpResponse response = client.get("/");
+            assertThat(response.status()).isEqualTo(HttpStatus.OK);
+        }
+    }
 }