Skip to content

Commit

Permalink
HTTP/2 enforce HTTP message flow
Browse files Browse the repository at this point in the history
Motivation:
codec-http2 currently does not strictly enforce the HTTP/1.x semantics with respect to the number of headers defined in RFC 7540 Section 8.1 [1]. We currently don't validate the number of headers nor do we validate that the trailing headers should indicate EOS.

[1] https://tools.ietf.org/html/rfc7540#section-8.1

Modifications:
- DefaultHttp2ConnectionDecoder should only allow decoding of a single headers and a single trailers
- DefaultHttp2ConnectionEncoder should only allow encoding of a single headers and optionally a single trailers

Result:
Constraints of RFC 7540 restricting the number of headers/trailers is enforced.
  • Loading branch information
Scottmitch committed Jul 19, 2017
1 parent 4af47f0 commit a91df58
Show file tree
Hide file tree
Showing 8 changed files with 419 additions and 64 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,27 @@ public static HttpStatusClass valueOf(int code) {
return UNKNOWN;
}

/**
* Returns the class of the specified HTTP status code.
* @param code Just the numeric portion of the http status code.
*/
public static HttpStatusClass valueOf(CharSequence code) {
if (code != null && code.length() == 3) {
char c0 = code.charAt(0);
return isDigit(c0) && isDigit(code.charAt(1)) && isDigit(code.charAt(2)) ? valueOf(digit(c0) * 100)
: UNKNOWN;
}
return UNKNOWN;
}

private static int digit(char c) {
return c - '0';
}

private static boolean isDigit(char c) {
return c >= '0' && c <= '9';
}

private final int min;
private final int max;
private final AsciiString defaultReasonPhrase;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -373,13 +373,16 @@ final DefaultPropertyKey verifyKey(PropertyKey key) {
* Simple stream implementation. Streams can be compared to each other by priority.
*/
private class DefaultStream implements Http2Stream {
private static final byte SENT_STATE_RST = 0x1;
private static final byte SENT_STATE_HEADERS = 0x2;
private static final byte SENT_STATE_PUSHPROMISE = 0x4;
private static final byte META_STATE_SENT_RST = 1;
private static final byte META_STATE_SENT_HEADERS = 1 << 1;
private static final byte META_STATE_SENT_TRAILERS = 1 << 2;
private static final byte META_STATE_SENT_PUSHPROMISE = 1 << 3;
private static final byte META_STATE_RECV_HEADERS = 1 << 4;
private static final byte META_STATE_RECV_TRAILERS = 1 << 5;
private final int id;
private final PropertyMap properties = new PropertyMap();
private State state;
private byte sentState;
private byte metaState;

DefaultStream(int id, State state) {
this.id = id;
Expand All @@ -398,35 +401,60 @@ public final State state() {

@Override
public boolean isResetSent() {
return (sentState & SENT_STATE_RST) != 0;
return (metaState & META_STATE_SENT_RST) != 0;
}

@Override
public Http2Stream resetSent() {
sentState |= SENT_STATE_RST;
metaState |= META_STATE_SENT_RST;
return this;
}

@Override
public Http2Stream headersSent() {
sentState |= SENT_STATE_HEADERS;
public Http2Stream headersSent(boolean isInformational) {
if (!isInformational) {
metaState |= isHeadersSent() ? META_STATE_SENT_TRAILERS : META_STATE_SENT_HEADERS;
}
return this;
}

@Override
public boolean isHeadersSent() {
return (sentState & SENT_STATE_HEADERS) != 0;
return (metaState & META_STATE_SENT_HEADERS) != 0;
}

@Override
public boolean isTrailersSent() {
return (metaState & META_STATE_SENT_TRAILERS) != 0;
}

@Override
public Http2Stream headersReceived(boolean isInformational) {
if (!isInformational) {
metaState |= isHeadersReceived() ? META_STATE_RECV_TRAILERS : META_STATE_RECV_HEADERS;
}
return this;
}

@Override
public boolean isHeadersReceived() {
return (metaState & META_STATE_RECV_HEADERS) != 0;
}

@Override
public boolean isTrailersReceived() {
return (metaState & META_STATE_RECV_TRAILERS) != 0;
}

@Override
public Http2Stream pushPromiseSent() {
sentState |= SENT_STATE_PUSHPROMISE;
metaState |= META_STATE_SENT_PUSHPROMISE;
return this;
}

@Override
public boolean isPushPromiseSent() {
return (sentState & SENT_STATE_PUSHPROMISE) != 0;
return (metaState & META_STATE_SENT_PUSHPROMISE) != 0;
}

@Override
Expand Down Expand Up @@ -599,7 +627,7 @@ public Http2Stream closeRemoteSide() {
}

@Override
public Http2Stream headersSent() {
public Http2Stream headersSent(boolean isInformational) {
throw new UnsupportedOperationException();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,15 @@

import io.netty.buffer.ByteBuf;
import io.netty.channel.ChannelHandlerContext;
import io.netty.handler.codec.http.HttpStatusClass;
import io.netty.handler.codec.http2.Http2Connection.Endpoint;
import io.netty.util.internal.UnstableApi;
import io.netty.util.internal.logging.InternalLogger;
import io.netty.util.internal.logging.InternalLoggerFactory;

import java.util.List;

import static io.netty.handler.codec.http.HttpStatusClass.INFORMATIONAL;
import static io.netty.handler.codec.http2.Http2CodecUtil.DEFAULT_PRIORITY_WEIGHT;
import static io.netty.handler.codec.http2.Http2Error.INTERNAL_ERROR;
import static io.netty.handler.codec.http2.Http2Error.PROTOCOL_ERROR;
Expand Down Expand Up @@ -282,6 +284,14 @@ public void onHeadersRead(ChannelHandlerContext ctx, int streamId, Http2Headers
return;
}

boolean isInformational = !connection.isServer() &&
HttpStatusClass.valueOf(headers.status()) == INFORMATIONAL;
if ((isInformational || !endOfStream) && stream.isHeadersReceived() || stream.isTrailersReceived()) {
throw streamError(streamId, PROTOCOL_ERROR,
"Stream %d received too many headers EOS: %s state: %s",
streamId, endOfStream, stream.state());
}

switch (stream.state()) {
case RESERVED_REMOTE:
stream.open(endOfStream);
Expand All @@ -305,6 +315,7 @@ public void onHeadersRead(ChannelHandlerContext ctx, int streamId, Http2Headers
stream.state());
}

stream.headersReceived(isInformational);
encoder.flowController().updateDependencyTree(streamId, streamDependency, weight, exclusive);

listener.onHeadersRead(ctx, streamId, headers, streamDependency, weight, exclusive, padding, endOfStream);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,12 @@
import io.netty.channel.ChannelHandlerContext;
import io.netty.channel.ChannelPromise;
import io.netty.channel.CoalescingBufferQueue;
import io.netty.handler.codec.http.HttpStatusClass;
import io.netty.util.internal.UnstableApi;

import java.util.ArrayDeque;

import static io.netty.handler.codec.http.HttpStatusClass.INFORMATIONAL;
import static io.netty.handler.codec.http2.Http2CodecUtil.DEFAULT_PRIORITY_WEIGHT;
import static io.netty.handler.codec.http2.Http2Error.PROTOCOL_ERROR;
import static io.netty.handler.codec.http2.Http2Exception.connectionError;
Expand Down Expand Up @@ -145,6 +147,15 @@ public ChannelFuture writeHeaders(ChannelHandlerContext ctx, int streamId, Http2
return writeHeaders(ctx, streamId, headers, 0, DEFAULT_PRIORITY_WEIGHT, false, padding, endStream, promise);
}

private static boolean validateHeadersSentState(Http2Stream stream, Http2Headers headers, boolean isServer,
boolean endOfStream) {
boolean isInformational = isServer && HttpStatusClass.valueOf(headers.status()) == INFORMATIONAL;
if ((isInformational || !endOfStream) && stream.isHeadersSent() || stream.isTrailersSent()) {
throw new IllegalStateException("Stream " + stream.id() + " sent too many headers EOS: " + endOfStream);
}
return isInformational;
}

@Override
public ChannelFuture writeHeaders(final ChannelHandlerContext ctx, final int streamId,
final Http2Headers headers, final int streamDependency, final short weight,
Expand Down Expand Up @@ -180,6 +191,7 @@ public ChannelFuture writeHeaders(final ChannelHandlerContext ctx, final int str
// for this stream.
Http2RemoteFlowController flowController = flowController();
if (!endOfStream || !flowController.hasFlowControlled(stream)) {
boolean isInformational = validateHeadersSentState(stream, headers, connection.isServer(), endOfStream);
if (endOfStream) {
final Http2Stream finalStream = stream;
final ChannelFutureListener closeStreamLocalListener = new ChannelFutureListener() {
Expand All @@ -190,14 +202,15 @@ public void operationComplete(ChannelFuture future) throws Exception {
};
promise = promise.unvoid().addListener(closeStreamLocalListener);
}

ChannelFuture future = frameWriter.writeHeaders(ctx, streamId, headers, streamDependency,
weight, exclusive, padding, endOfStream, promise);
// Writing headers may fail during the encode state if they violate HPACK limits.
Throwable failureCause = future.cause();
if (failureCause == null) {
// Synchronously set the headersSent flag to ensure that we do not subsequently write
// other headers containing pseudo-header fields.
stream.headersSent();
stream.headersSent(isInformational);
} else {
lifecycleManager.onError(ctx, failureCause);
}
Expand Down Expand Up @@ -451,6 +464,7 @@ public void error(ChannelHandlerContext ctx, Throwable cause) {

@Override
public void write(ChannelHandlerContext ctx, int allowedBytes) {
boolean isInformational = validateHeadersSentState(stream, headers, connection.isServer(), endOfStream);
if (promise.isVoid()) {
promise = ctx.newPromise();
}
Expand All @@ -461,7 +475,7 @@ public void write(ChannelHandlerContext ctx, int allowedBytes) {
// Writing headers may fail during the encode state if they violate HPACK limits.
Throwable failureCause = f.cause();
if (failureCause == null) {
stream.headersSent();
stream.headersSent(isInformational);
} else {
lifecycleManager.onError(ctx, failureCause);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,15 +130,41 @@ public boolean remoteSideOpen() {
<V> V removeProperty(Http2Connection.PropertyKey key);

/**
* Indicates that headers has been sent to the remote on this stream.
* Indicates that headers have been sent to the remote endpoint on this stream. The first call to this method would
* be for the initial headers (see {@link #isHeadersSent()}} and the second call would indicate the trailers
* (see {@link #isTrailersReceived()}).
* @param isInformational {@code true} if the headers contain an informational status code (for responses only).
*/
Http2Stream headersSent();
Http2Stream headersSent(boolean isInformational);

/**
* Indicates whether or not headers was sent to the remote endpoint.
* Indicates whether or not headers were sent to the remote endpoint.
*/
boolean isHeadersSent();

/**
* Indicates whether or not trailers were sent to the remote endpoint.
*/
boolean isTrailersSent();

/**
* Indicates that headers have been received. The first call to this method would be for the initial headers
* (see {@link #isHeadersReceived()}} and the second call would indicate the trailers
* (see {@link #isTrailersReceived()}).
* @param isInformational {@code true} if the headers contain an informational status code (for responses only).
*/
Http2Stream headersReceived(boolean isInformational);

/**
* Indicates whether or not the initial headers have been received.
*/
boolean isHeadersReceived();

/**
* Indicates whether or not the trailers have been received.
*/
boolean isTrailersReceived();

/**
* Indicates that a push promise was sent to the remote endpoint.
*/
Expand Down
Loading

0 comments on commit a91df58

Please sign in to comment.