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.

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

Don't disable HttpObjectDecoder on upgrade from HTTP/1.x to HTTP/1.x over TLS #7298

Closed
wants to merge 1 commit into
base: 4.1
from

Conversation

@pkolaczk

pkolaczk commented Oct 13, 2017

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

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Oct 21, 2017

Member

@pkolaczk can you please add a unit test ?

Member

normanmaurer commented Oct 21, 2017

@pkolaczk can you please add a unit test ?

@pkolaczk

This comment has been minimized.

Show comment
Hide comment
@pkolaczk

pkolaczk Oct 22, 2017

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

pkolaczk commented Oct 22, 2017

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

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Oct 24, 2017

Member

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

https://netty.io/s/icla

Member

normanmaurer commented Oct 24, 2017

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

https://netty.io/s/icla

@pkolaczk

This comment has been minimized.

Show comment
Hide comment
@pkolaczk

pkolaczk Oct 26, 2017

test added

pkolaczk commented Oct 26, 2017

test added

@pkolaczk

This comment has been minimized.

Show comment
Hide comment
@pkolaczk

pkolaczk Oct 26, 2017

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

pkolaczk commented Oct 26, 2017

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

Don't disable HttpObjectDecoder on upgrade from HTTP/1.x to
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 #7293.
@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Oct 26, 2017

Member
Member

normanmaurer commented Oct 26, 2017

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

This comment has been minimized.

@normanmaurer

normanmaurer Oct 26, 2017

Member

just do:

FullHttpResponse switchingProtocolsResponse = ch.readInbound();
@normanmaurer

normanmaurer Oct 26, 2017

Member

just do:

FullHttpResponse switchingProtocolsResponse = ch.readInbound();

This comment has been minimized.

@pkolaczk

pkolaczk Oct 26, 2017

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.

@pkolaczk

pkolaczk Oct 26, 2017

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.

This comment has been minimized.

@normanmaurer

normanmaurer Oct 26, 2017

Member

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

@normanmaurer

normanmaurer Oct 26, 2017

Member

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

This comment has been minimized.

@pkolaczk

pkolaczk Oct 26, 2017

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. :)

@pkolaczk

pkolaczk Oct 26, 2017

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. :)

This comment has been minimized.

@normanmaurer

normanmaurer Oct 26, 2017

Member

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

@normanmaurer

normanmaurer Oct 26, 2017

Member

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

This comment has been minimized.

@pkolaczk

pkolaczk Oct 27, 2017

Ok thanks, so I leave it as it is.

@pkolaczk

pkolaczk Oct 27, 2017

Ok thanks, so I leave it as it is.

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

This comment has been minimized.

@normanmaurer

normanmaurer Oct 26, 2017

Member

Just do:

FullHttpResponse finalResponse = ch.readInbound();
@normanmaurer

normanmaurer Oct 26, 2017

Member

Just do:

FullHttpResponse finalResponse = ch.readInbound();

This comment has been minimized.

@pkolaczk

pkolaczk Oct 26, 2017

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.

@pkolaczk

pkolaczk Oct 26, 2017

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

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Oct 27, 2017

Member

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

Member

normanmaurer commented Oct 27, 2017

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

@normanmaurer normanmaurer self-assigned this Oct 27, 2017

@normanmaurer normanmaurer added the defect label Oct 27, 2017

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

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Oct 29, 2017

Member

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

@pkolaczk thanks a lot

Member

normanmaurer commented Oct 29, 2017

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

@pkolaczk thanks a lot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment