Don't disable HttpObjectDecoder on upgrade from HTTP/1.x to HTTP/1.x over TLS by pkolaczk · Pull Request #7298 · netty/netty

pkolaczk · 2017-10-13T17:17:41Z

Motivation:

Being able to upgrade plain HTTP 1.x connection to TLS according to RFC 2817.
Switching the transport layer to TLS should be possible without removing HttpClientCodec from the pipeline, because HTTP/1.x layer of the protocol remains untouched by the switch.

Modification:
Don't set HttpObjectDecoder into UPGRADED state if SWITCHING_PROTOCOLS response contains HTTP/1.0 or HTTP/1.1 in the protocol stack described by the Upgrade header.

Describe the modifications you've done.
Added a new method isSwitchingToNonHttp1Protocol and called it from resetNow.
Additionally I had to skip pairing comparison for HTTP 101, because otherwise it resulted with NPE during decoding. I can see no reason why HTTP 100 and 101 would be handled differently in terms of request-response pairing.

Result:

Fixes #7293.

normanmaurer · 2017-10-21T12:53:58Z

@pkolaczk can you please add a unit test ?

pkolaczk · 2017-10-22T12:43:16Z

Yes, sure, I'll do. Is the patch ok besides that?

normanmaurer · 2017-10-24T10:07:47Z

@pkolaczk yes... please also sign our ICLA:

https://netty.io/s/icla

pkolaczk · 2017-10-26T13:53:01Z

test added

normanmaurer · 2017-10-26T14:03:50Z

codec-http/src/test/java/io/netty/handler/codec/http/HttpClientCodecTest.java

assert the return value.

normanmaurer · 2017-10-26T14:04:10Z

codec-http/src/test/java/io/netty/handler/codec/http/HttpClientCodecTest.java

assert return value

normanmaurer · 2017-10-26T14:04:14Z

codec-http/src/test/java/io/netty/handler/codec/http/HttpClientCodecTest.java

assert return value

normanmaurer · 2017-10-26T14:04:21Z

codec-http/src/test/java/io/netty/handler/codec/http/HttpClientCodecTest.java

assert return value

normanmaurer · 2017-10-26T14:04:36Z

codec-http/src/test/java/io/netty/handler/codec/http/HttpClientCodecTest.java

you will need to release the FullHttpResponse

normanmaurer · 2017-10-26T14:04:44Z

codec-http/src/test/java/io/netty/handler/codec/http/HttpClientCodecTest.java

you will need to release the FullHttpResponse

normanmaurer · 2017-10-26T14:05:06Z

codec-http/src/test/java/io/netty/handler/codec/http/HttpClientCodecTest.java

consider using finishAndReleaseAll()

pkolaczk · 2017-10-26T14:36:51Z

ok, if I use finishAndReleaseAll do I still need these release loops at the end?

HTTP/1.x over TLS This change allows to upgrade a plain HTTP 1.x connection to TLS according to RFC 2817. Switching the transport layer to TLS should be possible without removing HttpClientCodec from the pipeline, because HTTP/1.x layer of the protocol remains untouched by the switch and the HttpClientCodec state must be retained for proper handling the remainder of the response message, per RFC 2817 requirement in point 3.3: Once the TLS handshake completes successfully, the server MUST continue with the response to the original request. After this commit, the upgrade can be established by simply inserting an SslHandler at the front of the pipeline after receiving 101 SWITCHING PROTOCOLS response, exactly as described in SslHander documentation. Modifications: - Don't set HttpObjectDecoder into UPGRADED state if 101 SWITCHING_PROTOCOLS response contains HTTP/1.0 or HTTP/1.1 in the protocol stack described by the Upgrade header. - Skip pairing comparison for 101 SWITCHING_PROTOCOLS, similar to 100 CONTINUE, since 101 is not the final response to the original request and the final response is expected after TLS handshake. Fixes netty#7293.

normanmaurer · 2017-10-26T15:27:42Z

Nope

…

Am 26.10.2017 um 16:36 schrieb Piotr Kołaczkowski ***@***.***>: ok, if I use finishAndReleaseAll do I still need these release loops at the end? — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

normanmaurer · 2017-10-26T15:58:48Z

codec-http/src/test/java/io/netty/handler/codec/http/HttpClientCodecTest.java

+
+        assertTrue("Channel inbound write failed.",
+                ch.writeInbound(Unpooled.copiedBuffer(SWITCHING_PROTOCOLS_RESPONSE, CharsetUtil.ISO_8859_1)));
+        Object switchingProtocolsResponse = ch.readInbound();


just do:

FullHttpResponse switchingProtocolsResponse = ch.readInbound();

Well, but wouldn't it be a bit too implicit and hide the real purpose of the test? This test is basically checking if we do get back a decoded response and not just a plain buffer, like it was before the patch.

Honestly I am not sure why this would be any different then the assertThat that would fail below. Both will fail because the type is not what you expect. So I prefer what I suggested as then you can just remove the assertThas and also the cast later on when releasing

Both would fail, but with different message, and in the second case - this would be the assertion failing and not just some code. An assertion failing is a strong indicator to me that the code breaks something this test was really testing for, and not just an accidental breaking of the test caused by e.g. refactoring. I just want to avoid a situation that in the future this call returns an object of a different type, and someone just fixes the test by changing the type in the test instead of thinking why the test was like that.

But anyway, I can change it if you insist. :)

I am not super strong on it... if you are more happy with your way then its fine.

Ok thanks, so I leave it as it is.

normanmaurer · 2017-10-26T15:59:16Z

codec-http/src/test/java/io/netty/handler/codec/http/HttpClientCodecTest.java

+
+        assertTrue("Channel inbound write failed",
+                ch.writeInbound(Unpooled.copiedBuffer(RESPONSE, CharsetUtil.ISO_8859_1)));
+        Object finalResponse = ch.readInbound();


Just do:

FullHttpResponse finalResponse = ch.readInbound();

If my patch did not work, this would throw CCE, which would not be very obvious and could suggest (to someone not knowing the test purpose) the test was broken, and not the code under test.

ejona86

Didn't really dig into the test, but this looks about right to me.

normanmaurer · 2017-10-27T17:51:08Z

@Scottmitch @carl-mastrangelo want to have a look as well ? If I not hear back from you till monday I will merge.

normanmaurer · 2017-10-29T12:38:23Z

Cherry-picked into 4.1 (7995afe) and 4.0 (efcc6c8).

@pkolaczk thanks a lot

pkolaczk force-pushed the ISSUE-7293-4.1 branch from 78dc2b0 to 96e25c5 Compare October 16, 2017 08:05

pkolaczk force-pushed the ISSUE-7293-4.1 branch from 96e25c5 to 7959b83 Compare October 26, 2017 13:47

normanmaurer requested changes Oct 26, 2017

View reviewed changes

normanmaurer requested review from Scottmitch, carl-mastrangelo and ejona86 October 26, 2017 14:05

pkolaczk force-pushed the ISSUE-7293-4.1 branch from 7959b83 to a80187f Compare October 26, 2017 14:41

pkolaczk force-pushed the ISSUE-7293-4.1 branch from a80187f to 16adb6d Compare October 26, 2017 14:54

normanmaurer reviewed Oct 26, 2017

View reviewed changes

normanmaurer requested changes Oct 26, 2017

View reviewed changes

ejona86 approved these changes Oct 26, 2017

View reviewed changes

normanmaurer approved these changes Oct 27, 2017

View reviewed changes

normanmaurer self-assigned this Oct 27, 2017

normanmaurer added the defect label Oct 27, 2017

normanmaurer added this to the 4.1.17.Final milestone Oct 27, 2017

normanmaurer closed this Oct 29, 2017

normanmaurer modified the milestones: 4.1.17.Final, 4.0.53.Final Oct 29, 2017

This was referenced Oct 29, 2017

Don't disable HttpObjectDecoder on upgrade from HTTP/1.x to HTTP/1.x over TLS (Netty 4.0) #7299

Closed

HttpClientCodec cannot upgrade to TLS according to RFC2817 #7293

Closed

pkolaczk mentioned this pull request Oct 30, 2017

Don't disable HttpObjectDecoder on upgrade from HTTP/1.x to riptano/netty#2

Merged

Uh oh!

Conversation

pkolaczk commented Oct 13, 2017

Uh oh!

normanmaurer commented Oct 21, 2017

Uh oh!

pkolaczk commented Oct 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

normanmaurer commented Oct 24, 2017

Uh oh!

pkolaczk commented Oct 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pkolaczk commented Oct 26, 2017

Uh oh!

normanmaurer commented Oct 26, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ejona86 left a comment

Choose a reason for hiding this comment

Uh oh!

normanmaurer commented Oct 27, 2017

Uh oh!

normanmaurer commented Oct 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pkolaczk commented Oct 22, 2017 •

edited

Loading