Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

HttpConversionUtil.toHttp2Headers should strip http/2 incompatible headers #7355

Closed
mosesn opened this issue Oct 31, 2017 · 4 comments
Closed

HttpConversionUtil.toHttp2Headers should strip http/2 incompatible headers #7355

mosesn opened this issue Oct 31, 2017 · 4 comments
Assignees
@normanmaurer
Labels
defect
Milestone
4.1.18.Final

Comments

@mosesn
Copy link
Contributor

mosesn commented Oct 31, 2017

Expected behavior

When servers receive an h2c upgrade request, it's an http/1.1 message, and it quickly gets turned into an h2c message.

Actual behavior

If the upgrade request has illegal headers, like te: deflate,gzip;q=0.3 then it doesn't handle it gracefully and throws an exception.

W 1030 21:49:47.059 THREAD175 com.twitter.finagle.netty4.channel.ChannelExceptionHandler.exceptionCaught: Unhandled exception in connection with /10.56.221.119:54149, shutting down connection
java.lang.IllegalArgumentException: Invalid value for te: deflate,gzip;q=0.3
        at io.netty.handler.codec.http2.HttpConversionUtil.toHttp2Headers(HttpConversionUtil.java:423)
        at io.netty.handler.codec.http2.HttpConversionUtil.toHttp2Headers(HttpConversionUtil.java:399)
        at io.netty.handler.codec.http2.InboundHttpToHttp2Adapter.handle(InboundHttpToHttp2Adapter.java:61)
        at io.netty.handler.codec.http2.Http2FrameCodec.userEventTriggered(Http2FrameCodec.java:250)
        at io.netty.channel.AbstractChannelHandlerContext.invokeUserEventTriggered(AbstractChannelHandlerContext.java:329)
        at io.netty.channel.AbstractChannelHandlerContext.invokeUserEventTriggered(AbstractChannelHandlerContext.java:315)
        at io.netty.channel.AbstractChannelHandlerContext.fireUserEventTriggered(AbstractChannelHandlerContext.java:307)
        at io.netty.handler.codec.http.HttpServerUpgradeHandler$1.operationComplete(HttpServerUpgradeHandler.java:329)
        at io.netty.handler.codec.http.HttpServerUpgradeHandler$1.operationComplete(HttpServerUpgradeHandler.java:318)
        at io.netty.util.concurrent.DefaultPromise.notifyListener0(DefaultPromise.java:507)
        at io.netty.util.concurrent.DefaultPromise.notifyListenersNow(DefaultPromise.java:481)
        at io.netty.util.concurrent.DefaultPromise.notifyListeners(DefaultPromise.java:420)
        at io.netty.util.concurrent.DefaultPromise.addListener(DefaultPromise.java:163)
        at io.netty.channel.DefaultChannelPromise.addListener(DefaultChannelPromise.java:93)
        at io.netty.channel.DefaultChannelPromise.addListener(DefaultChannelPromise.java:28)
        at io.netty.handler.codec.http.HttpServerUpgradeHandler.upgrade(HttpServerUpgradeHandler.java:318)
        at io.netty.handler.codec.http.HttpServerUpgradeHandler.decode(HttpServerUpgradeHandler.java:239)
        at io.netty.handler.codec.http.HttpServerUpgradeHandler.decode(HttpServerUpgradeHandler.java:40)
        at io.netty.handler.codec.MessageToMessageDecoder.channelRead(MessageToMessageDecoder.java:88)
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362)
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348)
        at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:340)
        at io.netty.channel.CombinedChannelDuplexHandler$DelegatingChannelHandlerContext.fireChannelRead(CombinedChannelDuplexHandler.java:438)
        at io.netty.handler.codec.ByteToMessageDecoder.fireChannelRead(ByteToMessageDecoder.java:310)
        at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:284)
        at io.netty.channel.CombinedChannelDuplexHandler.channelRead(CombinedChannelDuplexHandler.java:253)
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362)
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348)
        at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:340)
        at io.netty.channel.ChannelInboundHandlerAdapter.channelRead(ChannelInboundHandlerAdapter.java:86)
        at com.twitter.finagle.netty4.channel.ChannelStatsHandler.channelRead(ChannelStatsHandler.scala:106)
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362)
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348)
        at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:340)
        at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1359)
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362)
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348)
        at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:340)
        at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1359)
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362)
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348)
        at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:935)
        at io.netty.channel.epoll.AbstractEpollStreamChannel$EpollStreamUnsafe.epollInReady(AbstractEpollStreamChannel.java:797)
        at io.netty.channel.epoll.AbstractEpollChannel$AbstractEpollUnsafe$1.run(AbstractEpollChannel.java:412)
        at io.netty.util.concurrent.AbstractEventExecutor.safeExecute(AbstractEventExecutor.java:163)
        at io.netty.util.concurrent.SingleThreadEventExecutor.runAllTasks(SingleThreadEventExecutor.java:403)
        at io.netty.channel.epoll.EpollEventLoop.run(EpollEventLoop.java:309)
        at io.netty.util.concurrent.SingleThreadEventExecutor$5.run(SingleThreadEventExecutor.java:858)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
        at com.twitter.finagle.util.BlockingTimeTrackingThreadFactory$$anon$1.run(BlockingTimeTrackingThreadFactory.scala:23)
        at java.lang.Thread.run(Thread.java:748)

Steps to reproduce

Send an h2c upgrade request to a server that has illegal h2c headers (like te: deflate,gzip;q=0.3).

Minimal yet complete reproducer code (or URL to code)

Haven't had a chance to repro

Netty version

4.1.16.Final

JVM version (e.g. java -version)

jdk8

OS version (e.g. uname -a)

Darwin tw-mbp13-mnakamura.local 16.7.0 Darwin Kernel Version 16.7.0: Thu Jun 15 17:36:27 PDT 2017; root:xnu-3789.70.16~2/RELEASE_X86_64 x86_64

workaround

I haven't found a workaround for this, so it would be lovely if you had any ideas here. I think it just needs to be fixed in netty. Here's what we normally do in finagle:
https://github.com/twitter/finagle/blob/develop/finagle-http2/src/main/scala/com/twitter/finagle/http2/transport/RichHttp2ServerDowngrader.scala#L10-L20
@normanmaurer
Copy link
Member

@mosesn sounds right... want to do a PR against HttpConversionUtil.toHttp2Headers

@Scottmitch
Copy link
Member

we could enhance the conversion to extract out the trailers value from the TE header instead of just throwing. +1 for a PR.

@mosesn mosesn mentioned this issue Nov 10, 2017
Closed
@mosesn
Copy link
Contributor Author

mosesn commented Nov 10, 2017 •
edited

I made a PR that extracted the trailers value and also cleaned up the behavior around "connection", which I think was wrong. #7399

mosesn added a commit to mosesn/netty that referenced this issue Nov 16, 2017
@mosesn
codec-http2: Improve h1 to h2 header conversion
2831e52 
Motivation:

Netty could handle "connection" or "te" headers more gently when
converting from http/1.1 to http/2 headers.  Http/2 headers don't
support single-hop headers, so when we convert from http/1.1 to http/2,
we should drop all single-hop headers.  This includes headers like
"transfer-encoding" and "connection", but also the headers that
"connection" points to, since "connection" can be used to designate
other headers as single-hop headers.  For the "te" header, we can more
permissively convert it by just dropping non-conforming headers (ie
non-"trailers" headers) which is what we do for all other headers when
we convert.

Modifications:

Add a new blacklist to the http/1.1 to http/2 conversion, which is
constructed from the values of the "connection" header, and stop
throwing an exception when a "te" header is passed with a non-"trailers"
value.  Instead, drop all values except for "trailers".  Add unit tests
for "connection" and "te" headers when converting from http/1.1 to http/2.

Result:

This will improve the h2c upgrade request, and also conversions from
http/1.1 to http/2.  This will simplify implementing spec-compliant
http/2 servers that want to share code between their http/1.1 and http/2
implementations.

[Fixes netty#7355]
@normanmaurer
Copy link
Member

Fixed by #7399

@normanmaurer normanmaurer self-assigned this Nov 17, 2017
@normanmaurer normanmaurer added this to the 4.1.18.Final milestone Nov 17, 2017
kiril-me pushed a commit to kiril-me/netty that referenced this issue Feb 28, 2018
@mosesn @kiril-me
codec-http2: Improve h1 to h2 header conversion
ab63abd 
Motivation:

Netty could handle "connection" or "te" headers more gently when
converting from http/1.1 to http/2 headers.  Http/2 headers don't
support single-hop headers, so when we convert from http/1.1 to http/2,
we should drop all single-hop headers.  This includes headers like
"transfer-encoding" and "connection", but also the headers that
"connection" points to, since "connection" can be used to designate
other headers as single-hop headers.  For the "te" header, we can more
permissively convert it by just dropping non-conforming headers (ie
non-"trailers" headers) which is what we do for all other headers when
we convert.

Modifications:

Add a new blacklist to the http/1.1 to http/2 conversion, which is
constructed from the values of the "connection" header, and stop
throwing an exception when a "te" header is passed with a non-"trailers"
value.  Instead, drop all values except for "trailers".  Add unit tests
for "connection" and "te" headers when converting from http/1.1 to http/2.

Result:

This will improve the h2c upgrade request, and also conversions from
http/1.1 to http/2.  This will simplify implementing spec-compliant
http/2 servers that want to share code between their http/1.1 and http/2
implementations.

[Fixes netty#7355]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@normanmaurer normanmaurer

Labels
defect
Projects
None yet
Milestone
4.1.18.Final
Development

No branches or pull requests
3 participants
@mosesn @normanmaurer @Scottmitch