Requests missing an :authority pseudo-header return PROTOCOL_ERROR

[zackangelo]

We are using nghttpx to upgrade requests from HTTP to H2. nghttpx omits the :authority pseudo-header when creating the request to forward on via H2, this appears to be consistent with the spec:

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.

To ensure that the HTTP/1.1 request line can be reproduced accurately, this pseudo-header field MUST be omitted when translating from an HTTP/1.1 request that has a request target in origin or asterisk form (see [RFC7230], Section 5.3). Clients that generate HTTP/2 requests directly SHOULD use the :authority pseudo-header field instead of the Host header field. An intermediary that converts an HTTP/2 request to HTTP/1.1 MUST create a Host header field if one is not present in a request by copying the value of the :authority pseudo-header field.

However, it appears that h2 incorrectly rejects the request with PROTOCOL_ERROR when this happens. It also seems like there should also be some logging around this code path, but I couldn't get it to trigger.

[seanmonstar]

Nice write-up, I agree with your assessment! (Both that it shouldn't error, and some logs would be nice).

[jkryl]

I hit similar problem related to authority header. Even more controversial. I'm using tower-grpc and k8s api client written in golang. Unix domain socket is used as the transport. golang client library inserts the name of the unix domain socket to authority header (i.e. /var/tmp/some.sock) and h2 fails the request (because slash is not allowed in authority header). I opened a ticket for the client side (grpc/grpc-go#2628) so that they don't set authority header unless they are sure that it is a valid value.

If they fix their part and we fix this ticket, then it should be good. Another option would be to fix h2 to be more tolerant and ignore the authority if invalid (or make such behaviour optional based on some configuration flag). Opinions?

[carllerche]

Well, I think we need to figure out who is in the wrong, it might be h2.

I think I misinterpreted:

The server MUST include a value in the :authority pseudo-header field for which the server is authoritative (see Section 10.1). A client MUST treat a PUSH_PROMISE for which the server is not authoritative as a stream error (Section 5.4.2) of type PROTOCOL_ERROR.

in this section.

That probably means that :authority is only required for push promises... So odds are it is a bug in h2.

[carllerche]

@jkryl is this something that you can look into. It probably is just a question of finding the code that checks if :authority is present and updating the check to only run if it is a push promise frame. Does that seem correct @seanmonstar?

[olix0r]

Though, I do think that h2 is correct to throw errors if :authority contains invalid characters like slashes... correct?

[carllerche]

@olix0r hmm, yeah that is a good point (i missed that part).

Welp... I guess the question is if we want h2 to be tolerant of spec violations.

[olix0r]

@carllerche http is supposed to be spec-compliant, right? If so, and if h2 uses http, then h2 probably needs to be as well.

[seanmonstar]

The HTTP2 spec urged implementors to be strict, to try to battle what happened with HTTP1 becoming a weak mess. I'm in favor of that.

[carllerche]

@seanmonstar I agree.

[jkryl]

Thanks for your opinions 👍 The ticket for go-grpc mentioned above has been flagged as a bug, so there is a good chance that it will be fixed in future. If we just tolerate unspecified authority header, which has been the original intent of this ticket, then we will be fine.

[seanmonstar]

I tried looking into fixing this, and came out unsure what the exact problem is. Here's what I found:

• In the server case, it appears that a request without an :authority is permitted just fine: https://github.com/carllerche/h2/blob/a3e59eb7e24a3bd4ff59806ead7842377eb34b78/src/server.rs#L1248.
• In the client case, if a request is sent with no :authority, then Request::version() cannot be Version::HTTP2, as per https://github.com/carllerche/h2/blob/a3e59eb7e24a3bd4ff59806ead7842377eb34b78/src/client.rs#L1515.

[jkryl]

Thanks for looking into this! 👍 It is possible that the issue does not exist. My problem was a bit different and I'm using a temporary fix to work around invalid authority header problem.

based on the following comment in client code:

// It is not possible to have a scheme but not an authority set (the
// `http` crate does not allow it).

I'm wondering what h2 client will do when support for UDS (unix domain socket) is added in the future. As it seems that authority is always required for http2 but there is no reasonable value to put into the header. Basically the same problem that golang http2 client suffers from. But that is off topic.

It might be worth writing a test case that covers missing authority header if there is none. Just my 2 cents.

[seanmonstar]

We finally found what was happening here. Even though the code in h2 allowed an optional :authority, the http::uri::Builder was unhappy about trying to build a Uri with a scheme and path, and so the error was still being triggered. Fix is in #372.