Detect HTTP/1.1 on HTTP/2 connection

[JamesNK]

Addresses #14884

What do we think? It's not perfect, but it should be a lot more informative in most cases. And extra work only happens if request is already been rejected so has no perf impact. Will add unit tests if this idea has merit.

Also, we can do something similar for HTTP/1.1 (on invalid request line, check if the content is an HTTP/2 connection preface)

[Tratcher]

Could we accomplish most of this by adding more content to Http2ErrorInvalidPreface without doing the additional checks? How much does it matter which specific version we detected? We could make multiple recommendations in the same error message.

A lot of the people that run into this aren't even looking at the server logs, they only notice a connection abort on the client. If we detected HTTP/1.x would it be possible to send back a hard coded 400 response before aborting the connection?

[TanayParikh]

/azp run

[TanayParikh]

Needs a full re-run given #39361.

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[JamesNK]

Is sending back HTTP/1.1 on a HTTP/2 connection be legal? Sure, the client is wrong, but do two wrongs make a right?

Changing the server response is a much bigger piece of work. To do that I think this would need to be completely robust (i.e. when detecting the error, read additional content until either the end of the line or max request line length is reached). Then send a valid HTTP/1.1 response. That's a lot more work.

Agree that returning an error to the client would help more people, but this is a start.

[Tratcher]

Is sending back HTTP/1.1 on a HTTP/2 connection be legal? Sure, the client is wrong, but do two wrongs make a right?

But it's not an HTTP/2 connection, is it? This check says the client is trying to do HTTP/1.x. The server hoped for HTTP/2 but didn't get it. The goal is to respond in a way the client will understand.

[JamesNK]

Ok.

I want to understand what is the ideal solution. It is more work than I expected to do so I want to make sure it is done right the first time and I don't spend time doing something that people don't want.

Proposal:

1. Read connection to prefix length
2. Check prefix
3. New: If invalid, instead of throwing connection exception, read connection until /n (or max request line length, or timeout is triggered)
4. New: Check if line is HTTP/1.x (how comprehensive do you want the check to be? Right now I'm just checking for the trailing "HTTP/1.1" on the request line. Do we care if the method or path is invalid?)
5. New: If the request is HTTP/1.x then return a 400 response and end the connection
6. If not HTTP1/x (no newline bytes, or exceeded request line length, or no HTTP/1.x version at end of the line) then throw connection error like it does today.

[Tratcher]

Sounds right.

The other case to consider in this code path is 5. Client (TLS) calling no TLS port, not for the same PR but keep it in mind while working here. If we don't detect HTTP/1.x then we could fall back to sniff for TLS frames (you only need ~5bytes to check for the basic structure). If that's a match then we could respond with a fatal TLS Alert like unexpected_message or illegal_parameter to make the error more searchable than a generic TCP RST. protocol_version is tempting, but that implies TLS protocol.

https://techcommunity.microsoft.com/t5/iis-support-blog/ssl-tls-alert-protocol-and-the-alert-codes/ba-p/377132

[JamesNK]

@Tratcher Updated. Needs tests and cache the hardcoded response. Otherwise, is this what you had in mind?

[Tratcher]

Exactly, it gives us a chance to tell the client what they did wrong without them having to go dig through the server logs.

[JamesNK]

Why isn't a connection error thrown from matching the preface if the reader ends before reading up to the preface length? In that case TryReadPrefaceAsync returns false, and the connection sends a GOAWAY of NO_ERROR.

Is that against the spec - https://httpwg.org/specs/rfc7540.html#rfc.section.3.5 - Bug?

I updated TryReadPrefaceAsync to throw Http2ConnectionErrorException with a PROTOCOL_ERROR when ReadResult.IsComplete is true.

[Tratcher]

Why isn't a connection error thrown from matching the preface if the reader ends before reading up to the preface length? In that case TryReadPrefaceAsync returns false, and the connection sends a GOAWAY of NO_ERROR.

Is that against the spec - https://httpwg.org/specs/rfc7540.html#rfc.section.3.5 - Bug?

I updated TryReadPrefaceAsync to throw Http2ConnectionErrorException with a PROTOCOL_ERROR when ReadResult.IsComplete is true.

If the reader ends then the connection is likely already closed. It's also common for port scanners to connect but not send any data. Neither case requires a response, or an exception log.

[JamesNK]

Why isn't a connection error thrown from matching the preface if the reader ends before reading up to the preface length? In that case TryReadPrefaceAsync returns false, and the connection sends a GOAWAY of NO_ERROR.
Is that against the spec - httpwg.org/specs/rfc7540.html#rfc.section.3.5 - Bug?
I updated TryReadPrefaceAsync to throw Http2ConnectionErrorException with a PROTOCOL_ERROR when ReadResult.IsComplete is true.

If the reader ends then the connection is likely already closed. It's also common for port scanners to connect but not send any data. Neither case requires a response, or an exception log.

Ok. FYI today the server returns a GOAWAY frame with NO_ERROR. If you think nothing should be sent then that would be a change. I'm going to leave current behavior as-is and update my tests in this area.

[Tratcher]

Have you tried it with HttpClient to make sure it works as expected?

[JamesNK]

Yes, but manually. Not in an integration test. I'll see if I can add one.

[halter73]

Nit: It's not super obvious from the name that ParseHttp1x returns true when the MaxRequestLineSize is exceeded before an \r could be found. Maybe ParseHttp1x should just update the ReadPrefaceState or return its own tri-state enum.