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

hyper should validate header fields on a header block with the CONNECT method #319

Open
alexwlchan opened this issue Sep 19, 2016 · 8 comments

Comments

@alexwlchan
Copy link
Contributor

RFC 7540 § 8.3:

In HTTP/2, the CONNECT method is used to establish a tunnel over a
single HTTP/2 stream to a remote host for similar purposes. The HTTP
header field mapping works as defined in Section 8.1.2.3 ("Request
Pseudo-Header Fields"), with a few differences. Specifically:

  • The ":method" pseudo-header field is set to "CONNECT".
  • The ":scheme" and ":path" pseudo-header fields MUST be omitted.
  • The ":authority" pseudo-header field contains the host and port to
    connect to (equivalent to the authority-form of the request-target
    of CONNECT requests (see [RFC7230], Section 5.3)).

A CONNECT request that does not conform to these restrictions is
malformed (Section 8.1.2.6).

Currently hyper doesn’t do anything to check this. We should add a check in utilities.py (probably somewhere in _reject_pseudo_header_fields()) that we conform to these restrictions.

@Lukasa
Copy link
Member

Lukasa commented Sep 20, 2016

Agreed, though I suspect that _reject_pseudo_header_fields is getting sufficiently complex that we may want to hoist this specific check out into a separate function.

@Lukasa
Copy link
Member

Lukasa commented Sep 20, 2016

Though that function may be called by _reject_pseudo_header_fields.

@alexwlchan
Copy link
Contributor Author

I favour putting it somewhere in _reject_pseudo_header_fields because it already has a list of all the pseudo-header fields in the header block, so restriction 2 falls out easily.

You then check if you see the :method header on the way through with the value CONNECT, and do a lookup in the set of pseudo-headers at the end.

We already have an extract_method_header function, but that requires another iteration over the header blocks, which I’m keen to avoid.

@Lukasa
Copy link
Member

Lukasa commented Sep 20, 2016

So I'm ok with taking a look at doing that, but it should be noted that we need to aggressively avoid complexity in these validation functions. We also do not do an extra loop over the headers: the reason the validation functions are written as generators is to ensure that we loop exactly once over the headers. Essentially, the validation pipeline could be recomposed as a series of function calls, once per header, along with some persisted state. That means that the real cost of doing this elsewhere is that we duplicate the checks for specific pseudo-headers.

That's not ideal, which is why I'm open to seeing the patch implemented in _reject_pseudo_header_fields, but the performance hit of doing it elsewhere is not so bad if we have to do it.

@alexwlchan
Copy link
Contributor Author

I had a quick stab at doing that in the awlc/connect-header branch, defining it as an extra validation function and ignoring _reject_pseudo_header_fields entirely. I tried it with the extra function, but you end up making the original more complicated and passing around lots of state.

@Lukasa
Copy link
Member

Lukasa commented Sep 20, 2016

Feel free to try doing it inline. =)

@optimusprime01
Copy link

optimusprime01 commented Jan 1, 2021

@Lukasa : I wanted to work on this item. Few questions on the original bug:

  1. My understanding from the section quoted is that ":authority" pseudo-header is mandatory in CONNECT method. Am I right?
  2. Is there a method in the library that validates "authority-form" in the value of ":authority" header? Also, do we even need to do this check?

optimusprime01 added a commit to optimusprime01/hyper-h2 that referenced this issue Jan 3, 2021
…isable RFC8441 extension through H2Configuration.
optimusprime01 added a commit to optimusprime01/hyper-h2 that referenced this issue Jan 4, 2021
@washeck
Copy link

washeck commented May 17, 2022

@Lukasa Is there a chance to have the work by @optimusprime01 reviewed? I encountered this issue and I'd like to know it this could be fixed in h2 upstream or we need to maintain a patched version ourselves.

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

No branches or pull requests

4 participants