Skip to content

Commit

Permalink
Better error when first HTTP/2 frame is not SETTINGS
Browse files Browse the repository at this point in the history 
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 netty#3880
  • Loading branch information
nmittler committed Jun 18, 2015
1 parent 33b37e3 commit 045602b
Show file tree
Hide file tree
Showing 2 changed files with 70 additions and 24 deletions.
38 changes: 33 additions & 5 deletions codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -22,8 +23,10 @@
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;
Expand Down Expand Up @@ -216,10 +219,10 @@ public boolean prefaceSent() {
@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);
Expand Down Expand Up @@ -268,12 +271,15 @@ private boolean readClientPrefaceString(ByteBuf in) throws Http2Exception {
}

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);
Expand All @@ -287,6 +293,28 @@ private boolean readClientPrefaceString(ByteBuf in) throws Http2Exception {
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.
*/
Expand Down
56 changes: 37 additions & 19 deletions codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionHandlerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,14 @@
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;
Expand All @@ -47,9 +49,6 @@
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;
Expand All @@ -60,6 +59,8 @@
import org.mockito.invocation.InvocationOnMock;
import org.mockito.stubbing.Answer;

import java.util.List;

/**
* Tests for {@link Http2ConnectionHandler}
*/
Expand Down Expand Up @@ -202,22 +203,27 @@ public void serverReceivingInvalidClientPrefaceStringShouldHandleException() thr
}

@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());
}

Expand All @@ -228,6 +234,7 @@ public void verifyChannelHandlerCanBeReusedInPipeline() throws Exception {
// 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());

Expand All @@ -236,10 +243,9 @@ public void verifyChannelHandlerCanBeReusedInPipeline() throws Exception {
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
Expand Down Expand Up @@ -276,7 +282,8 @@ public void writeRstOnNonExistantStreamShouldSucceed() throws Exception {
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());
Expand All @@ -291,7 +298,8 @@ public void writeRstOnClosedStreamShouldSucceed() throws Exception {
// 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")
Expand Down Expand Up @@ -346,7 +354,8 @@ public Void answer(InvocationOnMock invocation) throws Throwable {
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());
}
Expand All @@ -358,7 +367,8 @@ public void canSendGoAwayFramesWithDecreasingLastStreamIds() throws Exception {
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);
Expand Down Expand Up @@ -398,4 +408,12 @@ public void channelReadCompleteTriggersFlush() throws Exception {
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;
}
}

0 comments on commit 045602b

Please sign in to comment.