Remove ApplicationProtocolNegotiationHandler when no SslHandler is present (#11503)

Motivation:

750d235 did introduce a change in ApplicationProtocolNegotiationHandler that let the handler buffer all inbound data until the SSL handshake was completed. Due of this change a user might get buffer data forever if the SslHandler is not in the pipeline but the ApplicationProtocolNegotiationHandler was added. We should better guard the user against this "missconfiguration" of the ChannelPipeline.

Modifications:

- Remove the ApplicationProtocolNegotiationHandler when no SslHandler is present when the first message was received
- Add unit test

Result:

No possible that we buffer forever
This commit is contained in:
Norman Maurer 2021-07-26 20:20:40 +02:00 committed by GitHub
parent 165a035a15
commit c423891fb5
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 53 additions and 1 deletions

View File

@ -72,6 +72,7 @@ public abstract class ApplicationProtocolNegotiationHandler extends ChannelInbou
private final String fallbackProtocol; private final String fallbackProtocol;
private final RecyclableArrayList bufferedMessages = RecyclableArrayList.newInstance(); private final RecyclableArrayList bufferedMessages = RecyclableArrayList.newInstance();
private ChannelHandlerContext ctx; private ChannelHandlerContext ctx;
private boolean sslHandlerChecked;
/** /**
* Creates a new instance with the specified fallback protocol name. * Creates a new instance with the specified fallback protocol name.
@ -100,6 +101,14 @@ public abstract class ApplicationProtocolNegotiationHandler extends ChannelInbou
public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception { public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception {
// Let's buffer all data until this handler will be removed from the pipeline. // Let's buffer all data until this handler will be removed from the pipeline.
bufferedMessages.add(msg); bufferedMessages.add(msg);
if (!sslHandlerChecked) {
sslHandlerChecked = true;
if (ctx.pipeline().get(SslHandler.class) == null) {
// Just remove ourself if there is no SslHandler in the pipeline and so we would otherwise
// buffer forever.
removeSelfIfPresent(ctx);
}
}
} }
/** /**

View File

@ -43,6 +43,33 @@ import static org.junit.jupiter.api.Assertions.fail;
public class ApplicationProtocolNegotiationHandlerTest { public class ApplicationProtocolNegotiationHandlerTest {
@Test
public void testRemoveItselfIfNoSslHandlerPresent() throws NoSuchAlgorithmException {
ChannelHandler alpnHandler = new ApplicationProtocolNegotiationHandler(ApplicationProtocolNames.HTTP_1_1) {
@Override
protected void configurePipeline(ChannelHandlerContext ctx, String protocol) {
fail();
}
};
SSLEngine engine = SSLContext.getDefault().createSSLEngine();
// This test is mocked/simulated and doesn't go through full TLS handshake. Currently only JDK SSLEngineImpl
// client mode will generate a close_notify.
engine.setUseClientMode(true);
EmbeddedChannel channel = new EmbeddedChannel(alpnHandler);
String msg = "msg";
String msg2 = "msg2";
assertTrue(channel.writeInbound(msg));
assertTrue(channel.writeInbound(msg2));
assertNull(channel.pipeline().context(alpnHandler));
assertEquals(msg, channel.readInbound());
assertEquals(msg2, channel.readInbound());
assertFalse(channel.finishAndReleaseAll());
}
@Test @Test
public void testHandshakeFailure() { public void testHandshakeFailure() {
ChannelHandler alpnHandler = new ApplicationProtocolNegotiationHandler(ApplicationProtocolNames.HTTP_1_1) { ChannelHandler alpnHandler = new ApplicationProtocolNegotiationHandler(ApplicationProtocolNames.HTTP_1_1) {
@ -63,6 +90,15 @@ public class ApplicationProtocolNegotiationHandlerTest {
@Test @Test
public void testHandshakeSuccess() throws NoSuchAlgorithmException { public void testHandshakeSuccess() throws NoSuchAlgorithmException {
testHandshakeSuccess0(false);
}
@Test
public void testHandshakeSuccessWithSslHandlerAddedLater() throws NoSuchAlgorithmException {
testHandshakeSuccess0(true);
}
private static void testHandshakeSuccess0(boolean addLater) throws NoSuchAlgorithmException {
final AtomicBoolean configureCalled = new AtomicBoolean(false); final AtomicBoolean configureCalled = new AtomicBoolean(false);
ChannelHandler alpnHandler = new ApplicationProtocolNegotiationHandler(ApplicationProtocolNames.HTTP_1_1) { ChannelHandler alpnHandler = new ApplicationProtocolNegotiationHandler(ApplicationProtocolNames.HTTP_1_1) {
@Override @Override
@ -77,7 +113,14 @@ public class ApplicationProtocolNegotiationHandlerTest {
// client mode will generate a close_notify. // client mode will generate a close_notify.
engine.setUseClientMode(true); engine.setUseClientMode(true);
EmbeddedChannel channel = new EmbeddedChannel(new SslHandler(engine), alpnHandler); EmbeddedChannel channel = new EmbeddedChannel();
if (addLater) {
channel.pipeline().addLast(alpnHandler);
channel.pipeline().addFirst(new SslHandler(engine));
} else {
channel.pipeline().addLast(new SslHandler(engine));
channel.pipeline().addLast(alpnHandler);
}
channel.pipeline().fireUserEventTriggered(SslHandshakeCompletionEvent.SUCCESS); channel.pipeline().fireUserEventTriggered(SslHandshakeCompletionEvent.SUCCESS);
assertNull(channel.pipeline().context(alpnHandler)); assertNull(channel.pipeline().context(alpnHandler));
// Should produce the close_notify messages // Should produce the close_notify messages