Skip to content

Commit f81c42b

Browse files
zoewanggzoewangg
authored
Fix OneTimeReadTimeoutHandler removed by TLS handshake for 100-continue requests (#6803)
1 parent be935eb commit f81c42b

File tree

3 files changed

+232
-3
lines changed

3 files changed

+232
-3
lines changed

‎.changes/next-release/bugfix-NettyNIOHTTPClient-b32e774.json‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
{
2+
"type": "bugfix",
3+
"category": "Netty NIO HTTP Client",
4+
"contributor": "",
5+
"description": "Fixed an issue where requests with `Expect: 100-continue` over TLS could hang indefinitely when no response is received, because the read timeout handler was prematurely removed by TLS handshake data."
6+
}

‎http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/NettyRequestExecutor.java‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -265,10 +265,13 @@ private void writeRequest(HttpRequest request) {
265265

266266
if (shouldExplicitlyTriggerRead()) {
267267

268-
// Should only add an one-time ReadTimeoutHandler to 100 Continue request.
269268
if (is100ContinueExpected()) {
270-
channel.pipeline().addFirst(new OneTimeReadTimeoutHandler(Duration.ofMillis(context.configuration()
271-
.readTimeoutMillis())));
269+
// Should only add a one-time ReadTimeoutHandler to 100 Continue request.
270+
// Add before HttpStreamsClientHandler so that raw TLS handshake bytes cannot
271+
// prematurely remove this one-time handler. See Expect100ContinueReadTimeoutTest.
272+
channel.pipeline().addBefore(
273+
channel.pipeline().context(HttpStreamsClientHandler.class).name(), null,
274+
new OneTimeReadTimeoutHandler(Duration.ofMillis(context.configuration().readTimeoutMillis())));
272275
} else {
273276
channel.pipeline().addFirst(new ReadTimeoutHandler(context.configuration().readTimeoutMillis(),
274277
TimeUnit.MILLISECONDS));

‎http-clients/netty-nio-client/src/test/java/software/amazon/awssdk/http/nio/netty/fault/Expect100ContinueReadTimeoutTest.java‎

Lines changed: 220 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,220 @@
1+
/*
2+
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License").
5+
* You may not use this file except in compliance with the License.
6+
* A copy of the License is located at
7+
*
8+
* http://aws.amazon.com/apache2.0
9+
*
10+
* or in the "license" file accompanying this file. This file is distributed
11+
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
12+
* express or implied. See the License for the specific language governing
13+
* permissions and limitations under the License.
14+
*/
15+
16+
package software.amazon.awssdk.http.nio.netty.fault;
17+
18+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
19+
import static software.amazon.awssdk.http.SdkHttpConfigurationOption.TRUST_ALL_CERTIFICATES;
20+
21+
import io.netty.bootstrap.ServerBootstrap;
22+
import io.netty.channel.Channel;
23+
import io.netty.channel.ChannelHandlerContext;
24+
import io.netty.channel.ChannelInitializer;
25+
import io.netty.channel.ChannelPipeline;
26+
import io.netty.channel.SimpleChannelInboundHandler;
27+
import io.netty.channel.nio.NioEventLoopGroup;
28+
import io.netty.channel.socket.ServerSocketChannel;
29+
import io.netty.channel.socket.nio.NioServerSocketChannel;
30+
import io.netty.handler.codec.http.HttpRequest;
31+
import io.netty.handler.codec.http.HttpServerCodec;
32+
import io.netty.handler.ssl.SslContext;
33+
import io.netty.handler.ssl.SslContextBuilder;
34+
import io.netty.handler.ssl.SslHandler;
35+
import io.netty.handler.ssl.util.SelfSignedCertificate;
36+
import io.netty.handler.timeout.ReadTimeoutException;
37+
import io.reactivex.Flowable;
38+
import java.nio.ByteBuffer;
39+
import java.time.Duration;
40+
import java.util.concurrent.CompletableFuture;
41+
import java.util.concurrent.TimeUnit;
42+
import javax.net.ssl.SSLEngine;
43+
import javax.net.ssl.SSLEngineResult;
44+
import javax.net.ssl.SSLException;
45+
import org.junit.jupiter.api.AfterEach;
46+
import org.junit.jupiter.api.Assertions;
47+
import org.junit.jupiter.api.BeforeEach;
48+
import org.junit.jupiter.api.Test;
49+
import org.junit.jupiter.api.Timeout;
50+
import org.reactivestreams.Publisher;
51+
import software.amazon.awssdk.http.EmptyPublisher;
52+
import software.amazon.awssdk.http.Protocol;
53+
import software.amazon.awssdk.http.SdkHttpFullRequest;
54+
import software.amazon.awssdk.http.SdkHttpMethod;
55+
import software.amazon.awssdk.http.SdkHttpResponse;
56+
import software.amazon.awssdk.http.async.AsyncExecuteRequest;
57+
import software.amazon.awssdk.http.async.SdkAsyncHttpClient;
58+
import software.amazon.awssdk.http.async.SdkAsyncHttpResponseHandler;
59+
import software.amazon.awssdk.http.nio.netty.NettyNioAsyncHttpClient;
60+
import software.amazon.awssdk.http.nio.netty.SdkEventLoopGroup;
61+
import software.amazon.awssdk.utils.AttributeMap;
62+
63+
/**
64+
* Regression test for the bug where {@code OneTimeReadTimeoutHandler} was added before {@code SslHandler} in the pipeline
65+
* via {@code addFirst}, causing TLS handshake {@code channelRead} events to prematurely remove it. This left no read timeout
66+
* handler active during the wait for a {@code 100 Continue} response, causing the request to hang indefinitely.
67+
*
68+
* <p>The server deliberately delays the TLS handshake to ensure it is still in progress when the client's
69+
* {@code writeRequest()} adds the timeout handler. Without the fix ({@code addBefore} instead of {@code addFirst}),
70+
* the handler gets removed by handshake data and the test times out.
71+
*/
72+
public class Expect100ContinueReadTimeoutTest {
73+
74+
private SdkAsyncHttpClient netty;
75+
private Server server;
76+
77+
@BeforeEach
78+
public void setup() throws Exception {
79+
server = new Server();
80+
server.init();
81+
82+
netty = NettyNioAsyncHttpClient.builder()
83+
.readTimeout(Duration.ofMillis(500))
84+
.eventLoopGroup(SdkEventLoopGroup.builder().numberOfThreads(2).build())
85+
.protocol(Protocol.HTTP1_1)
86+
.buildWithDefaults(AttributeMap.builder().put(TRUST_ALL_CERTIFICATES, true).build());
87+
}
88+
89+
@AfterEach
90+
public void teardown() throws InterruptedException {
91+
if (server != null) {
92+
server.shutdown();
93+
}
94+
if (netty != null) {
95+
netty.close();
96+
}
97+
}
98+
99+
/**
100+
* Sends a PUT request with {@code Expect: 100-continue} to a TLS server that completes the handshake slowly
101+
* and then never responds. The read timeout must fire.
102+
*
103+
* <p>Without the fix, the {@code OneTimeReadTimeoutHandler} is removed by TLS handshake data and the request
104+
* hangs until the JUnit {@code @Timeout} kills it.
105+
*/
106+
@Test
107+
public void expect100Continue_slowTlsHandshake_serverNotResponding_shouldTimeout() {
108+
SdkHttpFullRequest request = SdkHttpFullRequest.builder()
109+
.method(SdkHttpMethod.PUT)
110+
.protocol("https")
111+
.host("localhost")
112+
.port(server.port())
113+
.putHeader("Expect", "100-continue")
114+
.putHeader("Content-Length", "1024")
115+
.build();
116+
117+
118+
CompletableFuture<Void> future = sendRequest(request);
119+
assertThatThrownBy(() -> future.get(2, TimeUnit.SECONDS))
120+
.hasRootCauseInstanceOf(ReadTimeoutException.class);
121+
}
122+
123+
private CompletableFuture<Void> sendRequest(SdkHttpFullRequest request) {
124+
return netty.execute(AsyncExecuteRequest.builder()
125+
.responseHandler(new SdkAsyncHttpResponseHandler() {
126+
@Override
127+
public void onHeaders(SdkHttpResponse headers) {
128+
}
129+
130+
@Override
131+
public void onStream(Publisher<ByteBuffer> stream) {
132+
Flowable.fromPublisher(stream).forEach(b -> {
133+
});
134+
}
135+
136+
@Override
137+
public void onError(Throwable error) {
138+
}
139+
})
140+
.request(request)
141+
.requestContentPublisher(new EmptyPublisher())
142+
.build());
143+
}
144+
145+
/**
146+
* A TLS server that delays the handshake and never sends an HTTP response.
147+
* The handshake delay ensures the client's {@code writeRequest()} executes while the handshake is still in progress,
148+
* which is the condition required to trigger the bug.
149+
*/
150+
private static class Server extends ChannelInitializer<Channel> {
151+
private static final long HANDSHAKE_DELAY_MS = 200;
152+
153+
private ServerBootstrap bootstrap;
154+
private ServerSocketChannel serverSock;
155+
private final NioEventLoopGroup group = new NioEventLoopGroup();
156+
private SslContext sslCtx;
157+
158+
void init() throws Exception {
159+
SelfSignedCertificate ssc = new SelfSignedCertificate();
160+
sslCtx = SslContextBuilder.forServer(ssc.certificate(), ssc.privateKey()).build();
161+
162+
bootstrap = new ServerBootstrap()
163+
.channel(NioServerSocketChannel.class)
164+
.group(group)
165+
.childHandler(this);
166+
167+
serverSock = (ServerSocketChannel) bootstrap.bind(0).sync().channel();
168+
}
169+
170+
@Override
171+
protected void initChannel(Channel ch) {
172+
ChannelPipeline pipeline = ch.pipeline();
173+
SSLEngine engine = sslCtx.newEngine(ch.alloc());
174+
pipeline.addLast(new SslHandler(new SlowHandshakeSslEngine(engine)));
175+
pipeline.addLast(new HttpServerCodec());
176+
pipeline.addLast(new SilentHttpHandler());
177+
}
178+
179+
void shutdown() throws InterruptedException {
180+
group.shutdownGracefully().await();
181+
serverSock.close();
182+
}
183+
184+
int port() {
185+
return serverSock.localAddress().getPort();
186+
}
187+
188+
/**
189+
* An SSLEngine wrapper that adds a delay to {@code wrap()} during the handshake,
190+
* slowing down the server's TLS responses so the client's handshake stays pending
191+
* while {@code writeRequest()} runs.
192+
*/
193+
private static class SlowHandshakeSslEngine extends DelegatingSslEngine {
194+
SlowHandshakeSslEngine(SSLEngine delegate) {
195+
super(delegate);
196+
}
197+
198+
@Override
199+
public SSLEngineResult wrap(ByteBuffer[] srcs, int offset, int length, ByteBuffer dst) throws SSLException {
200+
if (getHandshakeStatus() != SSLEngineResult.HandshakeStatus.NOT_HANDSHAKING
201+
&& getHandshakeStatus() != SSLEngineResult.HandshakeStatus.FINISHED) {
202+
try {
203+
Thread.sleep(HANDSHAKE_DELAY_MS);
204+
} catch (InterruptedException e) {
205+
Thread.currentThread().interrupt();
206+
}
207+
}
208+
return super.wrap(srcs, offset, length, dst);
209+
}
210+
}
211+
212+
/** Receives the HTTP request but never responds. */
213+
private static class SilentHttpHandler extends SimpleChannelInboundHandler<HttpRequest> {
214+
@Override
215+
protected void channelRead0(ChannelHandlerContext ctx, HttpRequest msg) {
216+
// Intentionally do nothing — simulate server not responding to 100-continue
217+
}
218+
}
219+
}
220+
}

0 commit comments

Comments
 (0)