"Connection: upgrade" causes 400 error that never reaches application code. Triggered by common nginx config.

[sbrudenell]

Describe the bug

If a Kestrel receives a Connection: upgrade header, in a POST request with a nonempty body, then Kestrel will never pass the request to application code. It will give a 400 "Bad Request" response with an empty body.

This gets triggered by a very common HTTPS reverse proxy configuration of nginx (see below).

To Reproduce

I have a Jellyfin 10.4.1 server set up at localhost:8096. Jellyfin uses Kestrel as its web server in a pretty straightforward configuration.

This curl line triggers the bug:

# curl -iv --raw --data foo -H 'Connection: upgrade' http://localhost:8096/notfound
*   Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 8096 (#0)
> POST /notfound HTTP/1.1
> Host: localhost:8096
> User-Agent: curl/7.58.0
> Accept: */*
> Connection: upgrade
> Content-Length: 3
> Content-Type: application/x-www-form-urlencoded
>
* upload completely sent off: 3 out of 3 bytes
< HTTP/1.1 400 Bad Request
HTTP/1.1 400 Bad Request
< Connection: close
Connection: close
< Date: Thu, 14 Nov 2019 05:25:52 GMT
Date: Thu, 14 Nov 2019 05:25:52 GMT
< Server: Kestrel
Server: Kestrel
< Content-Length: 0
Content-Length: 0

<
* Closing connection 0

Jellyfin logs all errors in its request handler, but no logs are ever output for this case. Application code never sees the request.

If I omit the header, or use a GET request, or an empty POST request with -X POST, then the bug is not triggered. I get the expected 404 error, and Jellyfin prints an error message.

Further technical details

I'm very new to .NET so I'm not sure how to get the version of ASP.NET Core being used here. I am using the official 10.4.1 Ubuntu release of Jellyfin.

I have checked out the Jellyfin source and done enough debugging to verify that the error seems to be in Kestrel, and it doesn't appear to be anywhere in Jellyfin handler code or in its Kestrel config.

Jellyfin configures its Kestrel server here: https://github.com/jellyfin/jellyfin/blob/v10.4.1/Emby.Server.Implementations/ApplicationHost.cs#L615

I'm using nginx as an HTTPS reverse proxy to Jellyfin. I'm using some standard nginx config for reverse proxying, which looks like this:

    proxy_set_header Upgrade $http_upgrade;
    proxy_set_header Connection "upgrade";

This is common in lots of nginx documentation. Here's an example.

Connection: upgrade is usually accompanied by Upgrade: websocket or so, but my read of the RFC is that specifying a missing header name shouldn't be grounds for a 400 response. Connection: upgrade should just mean that the server shouldn't forward the Upgrade header, if it exists.

In any case, I would expect that if Kestrel will catch malformed requests and not forward them to application code at all, that Kestrel will at least include some message indicating what's wrong. The empty response had me chasing my tail for days trying to figure out the problem.

[davidfowl]

The empty response had me chasing my tail for days trying to figure out the problem.

Logging is usually a solution when you end up in this situation. Crank up the verbosity of the logs in appsettings.json

[sbrudenell]

Logging is usually a solution when you end up in this situation. Crank up the verbosity of the logs in appsettings.json

I'm not sure if appsettings.json is a special .NET file used in the build process somewhere. I can't find any files named that.

Jellyfin does have a config file with log verbosity, which I maxed, but as I said, this request never gets to Jellyfin code, so that didn't help me.

I also enabled UseConnectionLogging(), but it didn't produce any extra log output for some reason.

I don't think any of this discounts what I said, anyway. I can't see what it would hurt for Kestrel to be a bit more helpful with its error message here. I didn't even start trying to increase log verbosity until an hour or two into investigating.

[sbrudenell]

FWIW, this nginx config is working for me as a workaround:

    proxy_set_header Upgrade $http_upgrade;
    proxy_set_header Connection $http_connection;

Not sure why this isn't the default in shipped config and documentation.

[benaadams]

It will throw a RequestRejectionReason.UpgradeRequestCannotHavePayload 400 if there is data:

https://github.com/aspnet/AspNetCore/blob/9abf4bfe3f09d0dafc9b7abd8441e6683e9bf612/src/Servers/Kestrel/Core/src/Internal/Http/Http1MessageBody.cs#L130-L135

[benaadams]

proxy_set_header Connection $http_connection;

Ah; it should be keep-alive or close? Don't think Upgrade is a valid value for the Connection header?

[davidfowl]

I'm not sure if appsettings.json is a special .NET file used in the build process somewhere. I can't find any files named that.

What does your application look like and how did you create it?

I also enabled UseConnectionLogging(), but it didn't produce any extra log output for some reason.

Likely because logging was not verbose enough.

I don't think any of this discounts what I said, anyway. I can't see what it would hurt for Kestrel to be a bit more helpful with its error message here. I didn't even start trying to increase log verbosity until an hour or two into investigating.

It doesn't but I didn't try to address that. I'm interested in why logging didn't immediately lead you to what's going on.

[sbrudenell]

Ah; it should be keep-alive or close? Don't think Upgrade is a valid value for the Connection header?

From RFC7230 section 6.1:

When a header field aside from Connection is used to supply control information for or about the current connection, the sender MUST list the corresponding field-name within the Connection header field.

I guess the intended interpretation is that Connection: upgrade is a no-op.

It will throw a RequestRejectionReason.UpgradeRequestCannotHavePayload 400 if there is data:

It seems that reason code isn't making it into the request output, at least in the AspNetCore version being used by Jellyfin. It would have been pretty helpful!

(If someone could help me figure out how to test what version is being used, I'd be happy to add that info here in the bug)

May I ask the reason why this test exists though? I don't see anything about payload requirements in RFC7230 section 6.7. I'm not an expert on interpreting RFCs, but I am a huge fan of Postel's law, which seems to be violated here. I can say that I've used the same nginx config to reverse proxy a dozen different http servers for years, and only Kestrel has choked on it.

[davidfowl]

May I ask the reason why this test exists though? I don't see anything about payload requirements in RFC7230 section 6.7. I'm not an expert on interpreting RFCs, but I am a huge fan of Postel's law, which seems to be violated here. I can say that I've used the same nginx config to reverse proxy a dozen different http servers for years, and only Kestrel has choked on it.

@sbrudenell The code is trying to determine which type of body to present to the application and it can't in this can't figure out which one to prefer, a content-length or an upgrade stream (raw body). That's why it throws.

(If someone could help me figure out how to test what version is being used, I'd be happy to add that info here in the bug)

Run dotnet --info on the command line.

[benaadams]

From RFC7230 section 6.1:

Ah my mistake :)

FWIW, this nginx config is working for me as a workaround:

What's in the $http_upgrade header from the original rules? Also in section 6.1:

A proxy or gateway MUST parse a received Connection header field before
a message is forwarded and, for each connection-option in this field,
remove any header field(s) from the message with the same name as the
connection-option, and then remove the Connection header field itself
(or replace it with the intermediary's own connection options for the
forwarded message).

Also I might be completely wrong about the reason for the request's rejection :)

appsettings.json is generally in the root with the cs.proj with the Program.cs and an optional file that you can control and change the settings with

I'm not sure if appsettings.json is a special .NET file used in the build process somewhere. I can't find any files named that.

So for example you might have (which should probably output more detail on the 400)

{
  "Logging": {
    "LogLevel": {
      "Default": "Information",
      "Microsoft": "Warning",
      "Microsoft.Hosting.Lifetime": "Information"
    }
  }
}

And you can turn it up with

{
  "Logging": {
    "LogLevel": {
      "Default": "Debug",
      "System": "Information",
      "Microsoft": "Information"
    }
  }

Though this is assuming you are using the DefaultBuilder for the webserver (i.e. what is in Program Main); which reads appsettings.json, as you don't have to read it if you construct it manually, or you may be overriding logging in .Configure()

[sbrudenell]

What's in the $http_upgrade header from the original rules?

Apparently the $http_x variables are just the original headers. From some nginx core documentation:

$http_name
arbitrary request header field; the last part of a variable name is the field name converted to lower case with dashes replaced by underscores

appsettings.json is generally in the root with the cs.proj with the Program.cs and an optional file that you can control and change the settings with

I don't find any appsettings.json anywhere in Jellyfin. I guess they do the embedded .Configure() method as you described.

If it helps clarify things somehow: I am a developer, but I am not a Jellyfin developer. I was just trying to get it to run and ended up needing to dig pretty deep in its source code to debug this issue. I'm not even sure I'm reproducing their official build process correctly.

[davidfowl]

Can you point us at jelly fin (I have no idea what it is)

[sbrudenell]

Jellyfin configures its Kestrel server here: https://github.com/jellyfin/jellyfin/blob/v10.4.1/Emby.Server.Implementations/ApplicationHost.cs#L615

[analogrelay]

It seems reasonable to ignore Connection: upgrade header when the app code isn't actually attempting to upgrade.