Aspnetcore: Useless error output when hitting HTTP endpoint over HTTPS

[muratg]

Just hit https://github.com/aspnet/KestrelHttpServer/issues/1321 myself and it took a few minutes to detect what's going on. Error message is not helpful.

We closed the original with "by design", but perhaps it's possible to detect this case and provide a more helpful message.

Repro

1. Create an app or an api
2. Remove HTTPS redirection in Startup.cs
3. Hit the HTTP endpoint with HTTPS (i.e. https://localhost:5000 when in fact port 5000 is running HTTP)

Output

info: Microsoft.AspNetCore.Server.Kestrel[17]
      Connection id "0HLNPJ5UGDB2D" bad request data: "Invalid request line: '\x16\x03\x01\x02\x00\x01\x00\x01\xFC\x03\x03ix\xCA\xE0u\xFD\x0A'"
Microsoft.AspNetCore.Server.Kestrel.Core.BadHttpRequestException: Invalid request line: '\x16\x03\x01\x02\x00\x01\x00\x01\xFC\x03\x03ix\xCA\xE0u\xFD\x0A'
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpParser`1.RejectRequestLine(Byte* requestLine, Int32 length)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpParser`1.GetUnknownMethod(Byte* data, Int32 length, Int32& methodLength)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpParser`1.ParseRequestLine(TRequestHandler handler, Byte* data, Int32 length)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpParser`1.ParseRequestLine(TRequestHandler handler, ReadOnlySequence`1& buffer, SequencePosition& consumed, SequencePosition& examined)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpParser`1.Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.IHttpParser<TRequestHandler>.ParseRequestLine(TRequestHandler handler, ReadOnlySequence`1& buffer, SequencePosition& consumed, SequencePosition& examined)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.Http1Connection.TakeStartLine(ReadOnlySequence`1 buffer, SequencePosition& consumed, SequencePosition& examined)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.Http1Connection.ParseRequest(ReadOnlySequence`1 buffer, SequencePosition& consumed, SequencePosition& examined)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.Http1Connection.TryParseRequest(ReadResult result, Boolean& endConnection)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol.ProcessRequests[TContext](IHttpApplication`1 application)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol.ProcessRequestsAsync[TContext](IHttpApplication`1 application)

[Eilon]

Your name sounds... familiar 😄

[blowdart]

@Eilon I mean either we trust the person who closed the issue, and leave it closed, or dismiss it, because some new guy thinks he was wrong ...

[anurse]

Clarifying the error message seems fine here. We just detect if the first few bytes are a TLS header and report a useful error. It's totally fine to "special-case" TLS here a bit. We know what it is and don't need an actual dependency.

[anurse]

We closed the original with "by design"

Oh WE closed it did we? 😛

[bashdx]

Hello,

if this one is still open I would like to tackle it. If I understood the issue correctly you want a more meaningful error message when TLS is detected although the endpoint does not speak HTTPS? I would start looking in https://github.com/aspnet/AspNetCore/blob/master/src/Servers/Kestrel/Core/src/Internal/Http/HttpParser.cs.
Thank you

[anurse]

Yeah, we'd be interested in taking this by a PR I think.

I think you've got the key idea. The idea is that after we detect an invalid request line, we would test the incoming data to see if it matches a TLS handshake. If so, we'd log something more useful like "detected a TLS handshake to an endpoint that does not have TLS enabled".

It's important to only do this checking after parsing as a request line fails so that we can keep the "happy path" (where the incoming data is valid) fast and efficient. Adding additional checks on the incoming data might impact our benchmark performance so it should only be done once we know we've hit a failure.

Another thing to consider would be parsing an incoming HTTP/2 connection on an HTTP/1 endpoint, and similarly detecting a plaintext HTTP request on a TLS encrypted endpoint (this would have to live in the Tls middleware). It's ok to keep those beyond the scope of this change though and stay focused on the problem at hand (TLS handshakes to non-TLS endpoints).

[davidfowl]

I would just check the first byte after the failure and if it's 0x16 fail with a better message. Like this https://github.com/aspnet/KestrelHttpServer/pull/1119/files

[bashdx]

@anurse
Thank you for your reply - here is my proposition: https://github.com/bashdx/AspNetCore/commit/ed624afe5c8afd93d5f686bb273fd90619cef967
I was not able to run tests on VS 2019 because I don't have .NET Core 3.0.0 Preview 8.

Could you please provide me with feedback for the implementation? I need to setup a test, but need to get a hang of the testing framework first.

[anurse]

I was not able to run tests on VS 2019 because I don't have .NET Core 3.0.0 Preview 8.

The ./build script in the repo root should set everything up, including a local copy of .NET Core 3.0. Once that's done you can either:

1. Run . .\activate.ps1 in the repo root from PowerShell (. .\activate.sh in Bash, there is no support in CMD) to put that dotnet on the path, then run tests or even launch VS from that command window/Terminal only

2. Run the startvs.cmd script next to the solution you want to launch, which will set up the path as appropriate and launch VS.

More details can be found here: https://github.com/aspnet/AspNetCore/blob/master/docs/BuildFromSource.md

Could you please provide me with feedback for the implementation? I need to setup a test, but need to get a hang of the testing framework first.

We'll review the PR as part of our triage process. Right now we are in the process of doing some lock-down for 3.0 so there may be some delay, but we'll take a look and provide feedback.

[bashdx]

Yeah, we'd be interested in taking this by a PR I think.

I think you've got the key idea. The idea is that after we detect an invalid request line, we would test the incoming data to see if it matches a TLS handshake. If so, we'd log something more useful like "detected a TLS handshake to an endpoint that does not have TLS enabled".

It's important to only do this checking after parsing as a request line fails so that we can keep the "happy path" (where the incoming data is valid) fast and efficient. Adding additional checks on the incoming data might impact our benchmark performance so it should only be done once we know we've hit a failure.

Another thing to consider would be parsing an incoming HTTP/2 connection on an HTTP/1 endpoint, and similarly detecting a plaintext HTTP request on a TLS encrypted endpoint (this would have to live in the Tls middleware). It's ok to keep those beyond the scope of this change though and stay focused on the problem at hand (TLS handshakes to non-TLS endpoints).

If you create own issues for those additional points I could take care of them as well. How about you check out my solution and if you think it fits we can talk about the other issues?