Deny requerst if :authority field is invalid only with CONNECT method

[arthurlm]

Looking at RFCs:

1. RFC 7540: HTTP V2@section 8.1.2.3. Request Pseudo-Header Fields.

The ":authority" pseudo-header field includes the authority portion of the target URI ([RFC3986], Section 3.2). The authority MUST NOT include the deprecated "userinfo" subcomponent for "http" or "https" schemed URIs.

All HTTP/2 requests MUST include exactly one valid value for the ":method", ":scheme", and ":path" pseudo-header fields, unless it is a CONNECT request (Section 8.3). An HTTP request that omits mandatory pseudo-header fields is malformed (Section 8.1.2.6).

2. RFC 3986: Uniform Resource Identifier@section 3.2. Authority

Many URI schemes include a hierarchical element for a naming authority so that governance of the name space defined by the remainder of the URI is delegated to that authority (which may, in turn, delegate it further).

The authority component is preceded by a double slash ("//") and is terminated by the next slash ("/"), question mark ("?"), or number sign ("#") character, or by the end of the URI.

Non-validating parsers (those that merely separate a URI reference into its major components) will often ignore the subcomponent structure of authority, treating it as an opaque string from the double-slash to the first terminating delimiter, until such time as the URI is dereferenced.

3. RFC 7540: HTTP [email protected]. The CONNECT Method

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:

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

So, from my understanding:

• Following (1):

  • :authority field is not mandatory in HTTP2.
  • It should be a valid URI without scheme and userinfo.

• Following (2):

  • URI can include sub path component (even if / character is not expected for regular authority).
  • There are HTTP2 clients in the wild that might send "invalid" :authority. It should be treated as opaque string.

• Following (3):

  • :authority field should be a valid host and port in the case of CONNECT method.

NOTE 1: I have not read the whole RFCs but just read in details the mentioned sections. If anyone have better understanding of this RFCs, please feel free to comment / edit this PR :wink: !

NOTE 2: Please have a look at my first comment about k8s usage with tonic for more details.

---

This change could fix a lot of already referenced issues / PRs using h2 and gRPC / tonic.

• https://github.com/hyperium/h2/issues/525
• https://github.com/hyperium/h2/issues/442
• https://github.com/hyperium/h2/pull/487
• https://github.com/projectcontour/contour/issues/4278

It will also:

• Improve support for gRPC call using go clients
• Allow people to develop nice k8s extension with tonic (which is not possible for now without forking h2)
• Help h2 to better fit to HTTP2 RFC specification :grin:

[bachp]

@seanmonstar do you see any issue with this change?

[Forsworns]

Any update on this one? Just met the same problem when communicate with k8s via tonic :(

[tiagolobocastro]

Any update on this? Would be good to stop using a fork :)

[mythi]

Any update on this one? Just met the same problem when communicate with k8s via tonic :(

k8s 1.26 kubelet sets localhost authority for device-plugin and CSI.

[arthurlm]

Even if recent version of k8s set localhost for :authority field:

• not everyone has access to latest version of k8s (and in big company it can takes years)
• in cloud as service provider (EKS for example) latest version of k8s is still 1.24
• h2 still does not respect HTTP2 RFC without this change

I do not really understand why maintainers do not want to review / merge / close this PR. I guess it is probably dues to some lack of time or PR visibility / interest.

I still hope this will be merged or discussed one day 😄. So this is why I have not closed this for now...

[mythi]

@arthurlm FWIW I agree with you. Just wanted to share what I know k8s kubelets can do today.