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

Forward h2c upgrade request downstream so that it gets a response [Fixes #5005] #6270

Merged
merged 4 commits into from
Oct 6, 2021

Conversation

yawkat
Copy link
Member

@yawkat yawkat commented Oct 4, 2021

From NettyServerUpgradeHandler: "The upgrade was successful, remove the message from the output list so that it's not propagated to the next handler. This request will be propagated as a user event instead."

So, the first request is discarded. This patch sends it downstream for normal handling.


Build is failing locally but that seems unrelated, so we'll see what CI says.

I'm not 100% on whether this patch could duplicate the request, but since it is a FullHttpRequest that is being forwarded, it should be safe – no partial data to worry about or anything. The fact that the upgradeRequest will still contain headers related to the upgrade may also be a concern, since it allows an attacker to send these headers further downstream than before. But I think that after the upgrade is complete, an attacker could send the same headers anyway.

@graemerocher
Copy link
Contributor

Can you add to your comments which issue this fixes with. Example Fixes #...

@graemerocher
Copy link
Contributor

Oh I see it is in the first line of the commit which was a bit too long so wrapped around

 #5005]

From NettyServerUpgradeHandler: "The upgrade was successful, remove the message from the output list so that it's not propagated to the next handler. This request will be propagated as a user event instead."

So, the first request is discarded. This patch sends it downstream for normal handling.
@yawkat yawkat force-pushed the h2c-first-request-response-0 branch from 6ab364b to dccb498 Compare October 4, 2021 15:10
@yawkat yawkat marked this pull request as draft October 4, 2021 16:34
@yawkat
Copy link
Member Author

yawkat commented Oct 4, 2021

Converting this to draft for now. Turns out the client will also require changes. The spec says:

After the empty line that terminates the 101 response, the server can begin sending HTTP/2 frames. These frames MUST include a response to the request that initiated the upgrade.

This is what my patch does. However, the micronaut h2c client does not expect a response for the first request that triggers the upgrade. Changing this has compatibility implications that I need to explore further.

@yawkat yawkat force-pushed the h2c-first-request-response-0 branch from 04d7145 to 8e31134 Compare October 6, 2021 06:27
@yawkat yawkat marked this pull request as ready for review October 6, 2021 06:55
@yawkat
Copy link
Member Author

yawkat commented Oct 6, 2021

@graemerocher This PR is ready for now. It only fixes the h2c server and normal client. The streaming client does not work yet, it never gets a response, I will work on that separately. But that was broken before this change anyway.

If possible, this PR should go into 3.1.0.

@yawkat yawkat merged commit 902818f into 3.1.x Oct 6, 2021
@yawkat yawkat deleted the h2c-first-request-response-0 branch October 6, 2021 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants