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

fix(http_server): in http2 host is not passed in headers #866

Merged
merged 1 commit into from
Sep 1, 2022

Conversation

polachok
Copy link
Contributor

In http2 host is not passed in headers which leads to this function always returning None and all http2 requests erroring with Parse error.

@polachok polachok requested a review from a team as a code owner August 29, 2022 14:34
Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, we haven't really tested and targeted HTTP2 yet.

However, I'm not sure if hyper rejects HTTP requests without the host header for HTTP1.1 so
I would prefer to check that request.version() == Version::HTTP_2 and then disable the host filtering.

/cc @lexnv another reason to get rid of the access control stuff in favor of tower http :P

@polachok
Copy link
Contributor Author

Wouldn't that be a security flaw? http server supports both protocols over the same port, so skipping the check would create a way to get around access control restrictions.

@niklasad1
Copy link
Member

niklasad1 commented Aug 29, 2022

I didn't mean to disable host filtering just use the host from the URI as you did before (I see my explanation was bad):

let host = match http_helpers::read_header_value(request.headers(), "host") {
  Some(host) => host,
  None if request.version() == Version::HTTP_2 => request.uri().host(),
  None => return response::malformed(),
};

so I think it was possible to circumvent the access control in your code if hyper doesn't reject a HTTP 1.1 request when the request is missing the host header....

Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, LGTM

Copy link
Contributor

@lexnv lexnv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for contributing!

@niklasad1
Copy link
Member

niklasad1 commented Sep 1, 2022

@polachok

dumb question: I can't find anything regarding that http2 host is not sent via the host header and I tested with curl http2 which indeed is sending the host header, when is the host header not sent in HTTP2?

I tried a simple request with cURL and it indeed works:

http2 request

 $ curl --http2 http://127.0.0.1:46399 -X POST -H 'Content-Type: application/json' -d '{"id":"1","method":"add","jsonrpc":"2.0" }'

server

2022-09-01T09:23:54.440026Z TRACE jsonrpsee_server::server: Request { method: POST, uri: /, version: HTTP/1.1, headers: {"host": "127.0.0.1:46399", "user-agent": "curl/7.85.0", "accept": "*/*", "connection": "Upgrade, HTTP2-Settings", "upgrade": "h2c", "http2-settings": "AAMAAABkAAQCAAAAAAIAAAAA", "content-type": "application/json", "content-length": "42"}, body: Body(Streaming) }
2022-09-01T09:23:54.440283Z TRACE method_call{method=add}: jsonrpsee_core::tracing: recv="{\"jsonrpc\":\"2.0\",\"id\":\"1\",\"method\":\"add\",\"params\":null}"

To conclude we need some tests for http2 as well which can be done in another PR.

@polachok
Copy link
Contributor Author

polachok commented Sep 1, 2022

http2 request

Not a http2 request. In your own output it shows version HTTP/1.1.

To make a http2 request with curl you have to add --http2-prior-knowledge

@niklasad1 niklasad1 merged commit 5a2f6f1 into paritytech:master Sep 1, 2022
@niklasad1 niklasad1 mentioned this pull request Sep 1, 2022
@polachok polachok deleted the http2-host-header branch September 6, 2022 04:39
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

3 participants