andreacavalli/netty5
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Better error when first HTTP/2 frame is not SETTINGS

Browse Source 
Motivation:

Bootstrap of the HTTP/2 can take a lot of paths and a lot of things can go wrong in the initial handshakes leading up to establishment of HTTP/2 between client and server. There have been many times where handshakes have failed silently, leading to very cryptic errors that are hard to debug.

Modifications:

Changed the HTTP/2 handler and decoder to ensure that the very first data on the wire (WRT HTTP/2) is SETTINGS/preface+SETTINGS. When this is not the case, a connection error is thrown with the bytes that were found instead.

Result:

Fixes #3880
This commit is contained in:
nmittler 2015-06-16 15:11:28 -07:00
parent 33b37e350c
commit 045602bd76
2 changed files with 70 additions and 24 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

38
codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java
View File

@ -14,6 +14,7 @@
*/
package io.netty.handler.codec.http2;
import static io.netty.buffer.ByteBufUtil.hexDump;
import static io.netty.handler.codec.http2.Http2CodecUtil.HTTP_UPGRADE_STREAM_ID;
import static io.netty.handler.codec.http2.Http2CodecUtil.connectionPrefaceBuf;
import static io.netty.handler.codec.http2.Http2CodecUtil.getEmbeddedHttp2Exception;
@ -22,8 +23,10 @@ import static io.netty.handler.codec.http2.Http2Error.NO_ERROR;
import static io.netty.handler.codec.http2.Http2Error.PROTOCOL_ERROR;
import static io.netty.handler.codec.http2.Http2Exception.connectionError;
import static io.netty.handler.codec.http2.Http2Exception.isStreamError;
import static io.netty.handler.codec.http2.Http2FrameTypes.SETTINGS;
import static io.netty.util.CharsetUtil.UTF_8;
import static io.netty.util.internal.ObjectUtil.checkNotNull;
import static java.lang.Math.min;
import static java.lang.String.format;
import io.netty.buffer.ByteBuf;
import io.netty.buffer.ByteBufUtil;
@ -216,10 +219,10 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
@Override
public void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) throws Exception {
try {
if (readClientPrefaceString(in)) {
if (readClientPrefaceString(in) && verifyFirstFrameIsSettings(in)) {
// After the preface is read, it is time to hand over control to the post initialized decoder.
Http2ConnectionHandler.this.byteDecoder = new FrameDecoder();
Http2ConnectionHandler.this.byteDecoder.decode(ctx, in, out);
byteDecoder = new FrameDecoder();
byteDecoder.decode(ctx, in, out);
}
} catch (Throwable e) {
onException(ctx, e);
@ -268,12 +271,15 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
}
int prefaceRemaining = clientPrefaceString.readableBytes();
int bytesRead = Math.min(in.readableBytes(), prefaceRemaining);
int bytesRead = min(in.readableBytes(), prefaceRemaining);
// If the input so far doesn't match the preface, break the connection.
if (bytesRead == 0 || !ByteBufUtil.equals(in, in.readerIndex(),
clientPrefaceString, clientPrefaceString.readerIndex(), bytesRead)) {
throw connectionError(PROTOCOL_ERROR, "HTTP/2 client preface string missing or corrupt.");
String receivedBytes = hexDump(in, in.readerIndex(),
min(in.readableBytes(), clientPrefaceString.readableBytes()));
throw connectionError(PROTOCOL_ERROR, "HTTP/2 client preface string missing or corrupt. " +
"Hex dump for received bytes: %s", receivedBytes);
}
in.skipBytes(bytesRead);
clientPrefaceString.skipBytes(bytesRead);
@ -287,6 +293,28 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
return false;
}
/**
* Peeks at that the next frame in the buffer and verifies that it is a {@code SETTINGS} frame.
*
* @param in the inbound buffer.
* @return {@code} true if the next frame is a {@code SETTINGS} frame, {@code false} if more
* data is required before we can determine the next frame type.
* @throws Http2Exception thrown if the next frame is NOT a {@code SETTINGS} frame.
*/
private boolean verifyFirstFrameIsSettings(ByteBuf in) throws Http2Exception {
if (in.readableBytes() < 4) {
// Need more data before we can see the frame type for the first frame.
return false;
}
byte frameType = in.getByte(in.readerIndex() + 3);
if (frameType != SETTINGS) {
throw connectionError(PROTOCOL_ERROR, "First received frame was not SETTINGS. " +
"Hex dump for first 4 bytes: %s", hexDump(in, in.readerIndex(), 4));
}
return true;
}
/**
* Sends the HTTP/2 connection preface upon establishment of the connection, if not already sent.
*/

56
codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionHandlerTest.java
View File

@ -30,12 +30,14 @@ import static org.mockito.Matchers.anyBoolean;
import static org.mockito.Matchers.anyInt;
import static org.mockito.Matchers.anyLong;
import static org.mockito.Matchers.eq;
import static org.mockito.Mockito.atLeastOnce;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;
import static org.mockito.Mockito.when;
import io.netty.buffer.ByteBuf;
import io.netty.buffer.Unpooled;
import io.netty.buffer.UnpooledByteBufAllocator;
@ -47,9 +49,6 @@ import io.netty.channel.ChannelPromise;
import io.netty.channel.DefaultChannelPromise;
import io.netty.util.CharsetUtil;
import io.netty.util.concurrent.GenericFutureListener;
import java.util.List;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
@ -60,6 +59,8 @@ import org.mockito.MockitoAnnotations;
import org.mockito.invocation.InvocationOnMock;
import org.mockito.stubbing.Answer;
import java.util.List;
/**
* Tests for {@link Http2ConnectionHandler}
*/
@ -202,22 +203,27 @@ public class Http2ConnectionHandlerTest {
}
@Test
public void serverReceivingValidClientPrefaceStringShouldContinueReadingFrames() throws Exception {
public void serverReceivingClientPrefaceStringFollowedByNonSettingsShouldHandleException()
throws Exception {
when(connection.isServer()).thenReturn(true);
handler = newHandler();
ByteBuf preface = connectionPrefaceBuf();
ByteBuf prefacePlusSome = Unpooled.wrappedBuffer(new byte[preface.readableBytes() + 1]);
prefacePlusSome.resetWriterIndex().writeBytes(preface).writeByte(0);
handler.channelRead(ctx, prefacePlusSome);
verify(decoder, times(2)).decodeFrame(eq(ctx), any(ByteBuf.class), Matchers.<List<Object>>any());
// Create a connection preface followed by a bunch of zeros (i.e. not a settings frame).
ByteBuf buf = Unpooled.buffer().writeBytes(connectionPrefaceBuf()).writeZero(10);
handler.channelRead(ctx, buf);
ArgumentCaptor<ByteBuf> captor = ArgumentCaptor.forClass(ByteBuf.class);
verify(frameWriter, atLeastOnce()).writeGoAway(eq(ctx), eq(0), eq(PROTOCOL_ERROR.code()),
captor.capture(), eq(promise));
assertEquals(0, captor.getValue().refCnt());
}
@Test
public void serverReceivingValidClientPrefaceStringShouldOnlyReadWholeFrame() throws Exception {
public void serverReceivingValidClientPrefaceStringShouldContinueReadingFrames() throws Exception {
when(connection.isServer()).thenReturn(true);
handler = newHandler();
handler.channelRead(ctx, connectionPrefaceBuf());
verify(decoder).decodeFrame(any(ChannelHandlerContext.class),
ByteBuf prefacePlusSome = addSettingsHeader(Unpooled.buffer().writeBytes(connectionPrefaceBuf()));
handler.channelRead(ctx, prefacePlusSome);
verify(decoder, atLeastOnce()).decodeFrame(any(ChannelHandlerContext.class),
any(ByteBuf.class), Matchers.<List<Object>>any());
}
@ -228,6 +234,7 @@ public class Http2ConnectionHandlerTest {
// Only read the connection preface...after preface is read internal state of Http2ConnectionHandler
// is expected to change relative to the pipeline.
ByteBuf preface = connectionPrefaceBuf();
handler.channelRead(ctx, preface);
verify(decoder, never()).decodeFrame(any(ChannelHandlerContext.class),
any(ByteBuf.class), Matchers.<List<Object>>any());
@ -236,10 +243,9 @@ public class Http2ConnectionHandlerTest {
handler.handlerAdded(ctx);
// Now verify we can continue as normal, reading connection preface plus more.
ByteBuf prefacePlusSome = Unpooled.wrappedBuffer(new byte[preface.readableBytes() + 1]);
prefacePlusSome.resetWriterIndex().writeBytes(preface).writeByte(0);
ByteBuf prefacePlusSome = addSettingsHeader(Unpooled.buffer().writeBytes(connectionPrefaceBuf()));
handler.channelRead(ctx, prefacePlusSome);
verify(decoder, times(2)).decodeFrame(eq(ctx), any(ByteBuf.class), Matchers.<List<Object>>any());
verify(decoder, atLeastOnce()).decodeFrame(eq(ctx), any(ByteBuf.class), Matchers.<List<Object>>any());
}
@Test
@ -276,7 +282,8 @@ public class Http2ConnectionHandlerTest {
handler = newHandler();
handler.resetStream(ctx, NON_EXISTANT_STREAM_ID, STREAM_CLOSED.code(), promise);
verify(frameWriter, never())
.writeRstStream(any(ChannelHandlerContext.class), anyInt(), anyLong(), any(ChannelPromise.class));
.writeRstStream(any(ChannelHandlerContext.class), anyInt(), anyLong(),
any(ChannelPromise.class));
assertTrue(promise.isDone());
assertTrue(promise.isSuccess());
assertNull(promise.cause());
@ -291,7 +298,8 @@ public class Http2ConnectionHandlerTest {
// The stream is "closed" but is still known about by the connection (connection().stream(..)
// will return the stream). We should still write a RST_STREAM frame in this scenario.
handler.resetStream(ctx, STREAM_ID, STREAM_CLOSED.code(), promise);
verify(frameWriter).writeRstStream(eq(ctx), eq(STREAM_ID), anyLong(), any(ChannelPromise.class));
verify(frameWriter).writeRstStream(eq(ctx), eq(STREAM_ID), anyLong(),
any(ChannelPromise.class));
}
@SuppressWarnings("unchecked")
@ -346,7 +354,8 @@ public class Http2ConnectionHandlerTest {
handler.goAway(ctx, STREAM_ID, errorCode, data, promise);
verify(connection).goAwaySent(eq(STREAM_ID), eq(errorCode), eq(data));
verify(frameWriter).writeGoAway(eq(ctx), eq(STREAM_ID), eq(errorCode), eq(data), eq(promise));
verify(frameWriter).writeGoAway(eq(ctx), eq(STREAM_ID), eq(errorCode), eq(data),
eq(promise));
verify(ctx).close();
assertEquals(0, data.refCnt());
}
@ -358,7 +367,8 @@ public class Http2ConnectionHandlerTest {
long errorCode = Http2Error.INTERNAL_ERROR.code();
handler.goAway(ctx, STREAM_ID + 2, errorCode, data.retain(), promise);
verify(frameWriter).writeGoAway(eq(ctx), eq(STREAM_ID + 2), eq(errorCode), eq(data), eq(promise));
verify(frameWriter).writeGoAway(eq(ctx), eq(STREAM_ID + 2), eq(errorCode), eq(data),
eq(promise));
verify(connection).goAwaySent(eq(STREAM_ID + 2), eq(errorCode), eq(data));
promise = new DefaultChannelPromise(channel);
handler.goAway(ctx, STREAM_ID, errorCode, data, promise);
@ -398,4 +408,12 @@ public class Http2ConnectionHandlerTest {
private ByteBuf dummyData() {
return Unpooled.buffer().writeBytes("abcdefgh".getBytes(CharsetUtil.UTF_8));
}
private ByteBuf addSettingsHeader(ByteBuf buf) {
buf.writeMedium(Http2CodecUtil.SETTING_ENTRY_LENGTH);
buf.writeByte(Http2FrameTypes.SETTINGS);
buf.writeByte(0);
buf.writeInt(0);
return buf;
}
}