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.

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

Client-side h2c (insecure HTTP/1.1-to-h2) upgrade does not work #4518

Closed
raboof opened this issue May 30, 2018 · 7 comments · Fixed by #4612

Comments

@raboof
Copy link
Contributor

commented May 30, 2018

When specifying NegotiationType.PLAINTEXT_UPGRADE, the client should connect over HTTP/1.1 with headers indicating that this request should be upgraded to (still insecure) h2.

There seems to be quite a bit of machinery in place to make this work, but currently the client does not start sending the request after performing the TCP handshake. A possible fix for this has been previously proposed in https://github.com/grpc/grpc-java/pull/3529/files#diff-e805c3ddf2a8c848220b85c467f5f408R379 (and when I tried this locally it did appear to help), but the code has evolved since.

Our use case for this feature is that while in production you'd use TLS and ALPN to serve both HTTP/1.1 and h2 requests on the same port, during development it can be convenient to run without TLS. NegotiationType.PLAINTEXT is available for non-TLS connections, but does not (really) support HTTP/1.1 and h2 coexisting on the same port.

What version of gRPC are you using?

Checked on 1.11.0, 1.12.0 and master.

What did you expect to see?

I expected the grpc-java Netty client to successfully connect to a h2c endpoint when configured with NegotiationType.PLAINTEXT_UPGRADE.

@carl-mastrangelo

This comment has been minimized.

Copy link
Contributor

commented May 30, 2018

I'm not sure it actually did work, despite the correctly named symbols being present. The usePlaintext(boolean) setter on the ManagedChannelBuilder was recently modified to not take the upgrade parameter, since it was confusing (and didn't work).

I can't see any strong reason to disable it, but I don't think it was supported.

@ejona86

This comment has been minimized.

Copy link
Member

commented May 30, 2018

So is the only thing necessary to modernize the fix from #3529?

Since none of the gRPC implementations support Upgrade on server-side, how are you handling that in the testing environment?

@raboof

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2018

As indeed I don't have a full server-side implementation yet I'm not sure an updated version of #3529 would be sufficient, but I verified it did get further along with that change on 1.11.0.

@ericgribkoff

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2018

@raboof, if you'd like to send a PR that finishes/updates #3529 we can review it; we don't have any plans to actively work on this feature right now.

raboof added a commit to raboof/grpc-java that referenced this issue Jun 5, 2018
raboof added a commit to raboof/grpc-java that referenced this issue Jun 5, 2018
@raboof

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2018

Makes sense! I took a quick stab at it and master...raboof:h2c looks promising, but I'll do some more testing before I turn it into a PR.

@ejona86 ejona86 added this to the Next milestone Jun 12, 2018

@raboof

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2018

I looked into it further: the server should send the response to the request with the Upgrade header in stream 1 of the upgraded connection (https://http2.github.io/http2-spec/#rfc.section.3.2).

However, since that stream isn't started at the grpc-java side, when the first header frame comes in at

stream.transportHeadersReceived(headers, endStream);
it will trigger a NullPointerException.

I poked around a bit but didn't find an elegant way to pre-initialize stream 1 for connections upgraded in this way.

@ejona86

This comment has been minimized.

Copy link
Member

commented Jun 22, 2018

@raboof, hmmm... I think netty starts with stream id 3 independent of whether Upgrade is used. So it may be possible to just ignore the header response for stream 1.
https://github.com/netty/netty/blob/f9604eeff5019219ea2895556309735bb27077fe/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2Connection.java#L698

It'd probably be good to add a validation check that we aren't using 1 to:

int nextStreamId = connection().local().incrementAndGetNextStreamId();

raboof added a commit to raboof/grpc-java that referenced this issue Aug 16, 2018
raboof added a commit to raboof/grpc-java that referenced this issue Sep 26, 2018
raboof added a commit to raboof/grpc-java that referenced this issue Nov 9, 2018

@ejona86 ejona86 closed this in #4612 Dec 1, 2018

ejona86 added a commit that referenced this issue Dec 1, 2018

@ejona86 ejona86 modified the milestones: Next, 1.18 Dec 1, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Mar 1, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants
You can’t perform that action at this time.