Skip to content

Commit

Permalink
Remove proxy-connection header from outgoing HTTP/2 messages (#2138)
Browse files Browse the repository at this point in the history 
Motivation:

Quote from the spec
(https://datatracker.ietf.org/doc/html/rfc7540#section-8.1.2.2):

   This means that an intermediary transforming an HTTP/1.x message to
   HTTP/2 will need to remove any header fields nominated by the
   Connection header field, along with the Connection header field
   itself.  Such intermediaries SHOULD also remove other connection-
   specific header fields, such as Keep-Alive, Proxy-Connection,
   Transfer-Encoding, and Upgrade, even if they are not nominated by the
   Connection header field.

`H2ToStH1Utils.h1HeadersToH2Headers()` removes all described headers
except `proxy-connection`.

Modifications:

- Remove `proxy-connection` header from the outgoing HTTP/2 messages;
- Test that HTTP/2 client and server remove prohibited headers;

Result:

Outgoing HTTP/2 messages never contain prohibited `proxy-connection`
header.
  • Loading branch information
@idelpivnitskiy
idelpivnitskiy authored Mar 10, 2022
1 parent a93fc9b commit dd94aea
Show file tree
Hide file tree
Showing 2 changed files with 82 additions and 0 deletions.
5 changes: 5 additions & 0 deletions servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/H2ToStH1Utils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@

import static io.netty.handler.codec.http.HttpHeaderNames.TE;
import static io.netty.handler.codec.http.HttpHeaderValues.TRAILERS;
import static io.servicetalk.buffer.api.CharSequences.newAsciiString;
import static io.servicetalk.http.api.HttpHeaderNames.CONNECTION;
import static io.servicetalk.http.api.HttpHeaderNames.COOKIE;
import static io.servicetalk.http.api.HttpHeaderNames.TRANSFER_ENCODING;
Expand All @@ -36,6 +37,9 @@
import static io.servicetalk.http.netty.HeaderUtils.indexOf;

final class H2ToStH1Utils {

static final CharSequence PROXY_CONNECTION = newAsciiString("proxy-connection");

private H2ToStH1Utils() {
// no instances.
}
Expand Down Expand Up @@ -140,6 +144,7 @@ static Http2Headers h1HeadersToH2Headers(HttpHeaders h1Headers) {
h1Headers.remove(KEEP_ALIVE);
h1Headers.remove(TRANSFER_ENCODING);
h1Headers.remove(UPGRADE);
h1Headers.remove(PROXY_CONNECTION);

// TE header is treated specially https://tools.ietf.org/html/rfc7540#section-8.1.2.2
// (only value of "trailers" is allowed).
Expand Down
77 changes: 77 additions & 0 deletions ...http-netty/src/test/java/io/servicetalk/http/netty/H2PriorKnowledgeFeatureParityTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import io.servicetalk.http.api.HttpEventKey;
import io.servicetalk.http.api.HttpExecutionStrategy;
import io.servicetalk.http.api.HttpHeaders;
import io.servicetalk.http.api.HttpMetaData;
import io.servicetalk.http.api.HttpRequest;
import io.servicetalk.http.api.HttpRequestMethod;
import io.servicetalk.http.api.HttpResponse;
Expand All @@ -53,6 +54,7 @@
import io.servicetalk.http.api.StreamingHttpServiceFilter;
import io.servicetalk.http.api.StreamingHttpServiceFilterFactory;
import io.servicetalk.http.netty.NettyHttp2ExceptionUtils.H2StreamResetException;
import io.servicetalk.logging.api.LogLevel;
import io.servicetalk.transport.api.ConnectionContext;
import io.servicetalk.transport.api.DelegatingConnectionAcceptor;
import io.servicetalk.transport.api.HostAndPort;
Expand All @@ -72,6 +74,7 @@
import io.netty.handler.codec.http2.DefaultHttp2DataFrame;
import io.netty.handler.codec.http2.DefaultHttp2Headers;
import io.netty.handler.codec.http2.DefaultHttp2HeadersFrame;
import io.netty.handler.codec.http2.DefaultHttp2ResetFrame;
import io.netty.handler.codec.http2.DefaultHttp2SettingsFrame;
import io.netty.handler.codec.http2.Http2DataFrame;
import io.netty.handler.codec.http2.Http2FrameCodecBuilder;
Expand All @@ -82,6 +85,7 @@
import io.netty.handler.codec.http2.Http2SettingsAckFrame;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;
Expand Down Expand Up @@ -110,6 +114,7 @@
import static io.netty.handler.codec.http.HttpHeaderNames.CONTENT_TYPE;
import static io.netty.handler.codec.http.HttpHeaderNames.TRAILER;
import static io.netty.handler.codec.http2.Http2CodecUtil.SMALLEST_MAX_CONCURRENT_STREAMS;
import static io.netty.handler.codec.http2.Http2Error.PROTOCOL_ERROR;
import static io.servicetalk.buffer.api.EmptyBuffer.EMPTY_BUFFER;
import static io.servicetalk.buffer.api.Matchers.contentEqualTo;
import static io.servicetalk.concurrent.api.Completable.completed;
Expand All @@ -120,25 +125,31 @@
import static io.servicetalk.context.api.ContextMap.Key.newKey;
import static io.servicetalk.http.api.HeaderUtils.isTransferEncodingChunked;
import static io.servicetalk.http.api.HttpEventKey.MAX_CONCURRENCY;
import static io.servicetalk.http.api.HttpHeaderNames.CONNECTION;
import static io.servicetalk.http.api.HttpHeaderNames.CONTENT_LENGTH;
import static io.servicetalk.http.api.HttpHeaderNames.COOKIE;
import static io.servicetalk.http.api.HttpHeaderNames.EXPECT;
import static io.servicetalk.http.api.HttpHeaderNames.SET_COOKIE;
import static io.servicetalk.http.api.HttpHeaderNames.TRANSFER_ENCODING;
import static io.servicetalk.http.api.HttpHeaderNames.UPGRADE;
import static io.servicetalk.http.api.HttpHeaderValues.CHUNKED;
import static io.servicetalk.http.api.HttpHeaderValues.CONTINUE;
import static io.servicetalk.http.api.HttpHeaderValues.KEEP_ALIVE;
import static io.servicetalk.http.api.HttpRequestMethod.GET;
import static io.servicetalk.http.api.HttpRequestMethod.POST;
import static io.servicetalk.http.api.HttpResponseStatus.EXPECTATION_FAILED;
import static io.servicetalk.http.api.HttpResponseStatus.INTERNAL_SERVER_ERROR;
import static io.servicetalk.http.api.HttpResponseStatus.OK;
import static io.servicetalk.http.api.HttpSerializers.textSerializerUtf8;
import static io.servicetalk.http.netty.H2ToStH1Utils.PROXY_CONNECTION;
import static io.servicetalk.http.netty.HttpClients.forSingleAddress;
import static io.servicetalk.http.netty.HttpProtocolConfigs.h1Default;
import static io.servicetalk.http.netty.HttpProtocolConfigs.h2Default;
import static io.servicetalk.http.netty.HttpTestExecutionStrategy.DEFAULT;
import static io.servicetalk.http.netty.HttpTestExecutionStrategy.NO_OFFLOAD;
import static io.servicetalk.test.resources.TestUtils.assertNoAsyncErrors;
import static io.servicetalk.transport.netty.internal.AddressUtils.localAddress;
import static io.servicetalk.transport.netty.internal.AddressUtils.serverHostAndPort;
import static io.servicetalk.transport.netty.internal.BuilderUtils.serverChannel;
import static io.servicetalk.transport.netty.internal.NettyIoExecutors.createIoExecutor;
import static java.lang.String.valueOf;
Expand All @@ -164,6 +175,9 @@
import static org.junit.jupiter.api.Assumptions.assumeTrue;

class H2PriorKnowledgeFeatureParityTest {

private static final CharSequence[] PROHIBITED_HEADERS = {CONNECTION, KEEP_ALIVE, TRANSFER_ENCODING, UPGRADE,
PROXY_CONNECTION};
private static final String EXPECT_FAIL_HEADER = "please_fail_expect";
private static final ContextMap.Key<String> K1 = newKey("k1", String.class);
private static final ContextMap.Key<String> K2 = newKey("k2", String.class);
Expand Down Expand Up @@ -1265,6 +1279,57 @@ private static void fullDuplexModeWrite(StreamingHttpClient client,
assertEquals(request2ToWrite, next.toString(UTF_8));
}

@Test
void h2LayerFiltersOutProhibitedH1HeadersOnClientSide() throws Exception {
setUp(DEFAULT, true);
serverAcceptorChannel = bindH2Server(serverEventLoopGroup, new ChannelInitializer<Channel>() {
@Override
protected void initChannel(final Channel ch) {
ch.pipeline().addLast(new EchoHttp2Handler());
}
}, __ -> { }, identity());
InetSocketAddress serverAddress = (InetSocketAddress) serverAcceptorChannel.localAddress();
try (BlockingHttpClient client = forSingleAddress(HostAndPort.of(serverAddress))
.protocols(HttpProtocol.HTTP_2.config)
.enableWireLogging("servicetalk-tests-wire-logger", LogLevel.TRACE, () -> true)
.executionStrategy(clientExecutionStrategy)
.buildBlocking()) {
HttpResponse response = client.request(addProhibitedHeaders(client.post("/"))
.payloadBody(client.executionContext().bufferAllocator().fromAscii("content")));
assertThat(response.status(), is(OK));
}
}

@Test
void h2LayerFiltersOutProhibitedH1HeadersOnServerSide() throws Exception {
setUp(DEFAULT, true);
try (ServerContext serverContext = HttpServers.forAddress(localAddress(0))
.protocols(HttpProtocol.HTTP_2.config)
.enableWireLogging("servicetalk-tests-wire-logger", LogLevel.TRACE, () -> true)
.listenBlockingAndAwait((ctx, request, responseFactory) -> addProhibitedHeaders(responseFactory.ok()));
BlockingHttpClient client = forSingleAddress(serverHostAndPort(serverContext))
.protocols(HttpProtocol.HTTP_2.config)
.enableWireLogging("servicetalk-tests-wire-logger", LogLevel.TRACE, () -> true)
.executionStrategy(clientExecutionStrategy)
.buildBlocking()) {
HttpResponse response = client.request(client.get("/"));
assertThat(response.status(), is(OK));
for (CharSequence headerName : PROHIBITED_HEADERS) {
assertThat("Unexpected headerName: " + headerName,
response.headers().contains(headerName), is(false));
}
}
}

private static <T extends HttpMetaData> T addProhibitedHeaders(T metaData) {
metaData.addHeader(CONNECTION, UPGRADE)
.addHeader(KEEP_ALIVE, "timeout=5")
.addHeader(TRANSFER_ENCODING, CHUNKED)
.addHeader(UPGRADE, "foo/2")
.addHeader(PROXY_CONNECTION, "close");
return metaData;
}

@ParameterizedTest(name = "{displayName} [{index}] client={0}, h2PriorKnowledge={1}")
@MethodSource("clientExecutors")
void clientRespectsSettingsFrame(HttpTestExecutionStrategy strategy,
Expand Down Expand Up @@ -1738,6 +1803,11 @@ private void onHeadersRead(ChannelHandlerContext ctx, Http2HeadersFrame headers)
outHeaders.status(io.netty.handler.codec.http.HttpResponseStatus.OK.codeAsText());
}

if (!allHeadersSanitized(headers.headers())) {
ctx.writeAndFlush(new DefaultHttp2ResetFrame(PROTOCOL_ERROR));
return;
}

CharSequence contentType = headers.headers().get(CONTENT_TYPE);
if (contentType != null) {
outHeaders.add(CONTENT_TYPE, contentType);
Expand All @@ -1747,6 +1817,13 @@ private void onHeadersRead(ChannelHandlerContext ctx, Http2HeadersFrame headers)
sentHeaders = true;
}
}

private static boolean allHeadersSanitized(Http2Headers headers) {
return !headers.contains(HttpHeaderNames.CONNECTION) && !headers.contains(HttpHeaderNames.KEEP_ALIVE)
&& !headers.contains(HttpHeaderNames.TRANSFER_ENCODING)
&& !headers.contains(HttpHeaderNames.UPGRADE)
&& !headers.contains(HttpHeaderNames.PROXY_CONNECTION);
}
}

private static <T> void assertEmptyIterator(Iterator<? extends T> itr) {
Expand Down

0 comments on commit dd94aea

Please sign in to comment.