Aspnetcore: "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

[anurse]

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

[benaadams]

Loose parsing if the internet facing server (Kestrel) then forwards the request to another server can cause security issues https://portswigger.net/research/http-desync-attacks-request-smuggling-reborn

Probably what to find out the reason its being rejected first (which hasn't been established) and then determine why that choice was made.

[anurse]

@benaadams Agreed, but the NGINX guidance for WebSockets explicitly configures it to unconditionally send Connection: upgrade (which still seems wrong...) so I'm just saying we need to evaluate a fix here.

[benaadams]

Yes, so Connection: upgrade is saying the the connection info is in the Upgrade header; which is likely fine. (I was wrong earlier)

It could be being rejected because its a upgrade with a body; or its a post with an upgrade, or it has no actual Upgrade header for the connection or something else.

[benaadams]

Its a debug level message; I assume not to fill the logs with junky requests.

"BadHttpRequestException: 'Requests with 'Connection: Upgrade' cannot have content in the request body.'"

dbug: Microsoft.AspNetCore.Server.Kestrel[39]
      Connection id "0HLRA36OL1VB1" accepted.
dbug: Microsoft.AspNetCore.Server.Kestrel[1]
      Connection id "0HLRA36OL1VB1" started.
dbug: Microsoft.AspNetCore.Server.Kestrel[17]
      Connection id "0HLRA36OL1VB1" bad request data: "Requests with 'Connection: Upgrade' cannot have content in the request body."
Microsoft.AspNetCore.Server.Kestrel.Core.BadHttpRequestException: Requests with 'Connection: Upgrade' cannot have content in the request body.
   at Microsoft.AspNetCore.Server.Kestrel.Core.BadHttpRequestException.Throw(RequestRejectionReason reason)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.Http1MessageBody.For(HttpVersion httpVersion, HttpRequestHeaders headers, Http1Connection context)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.Http1Connection.CreateMessageBody()
   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)
dbug: Microsoft.AspNetCore.Server.Kestrel[10]
      Connection id "0HLRA36OL1VB1" disconnecting.
dbug: Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets[6]
      Connection id "0HLRA36OL1VB1" received FIN.
dbug: Microsoft.AspNetCore.Server.Kestrel[2]
      Connection id "0HLRA36OL1VB1" stopped.
dbug: Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets[7]
      Connection id "0HLRA36OL1VB1" sending FIN because: "The Socket transport's send loop completed gracefully."

curl -X POST \
  https://.... \
  -H 'Connection: upgrade' \
  -H 'Content-Length: 3' \
  -H 'Content-Type: application/x-www-form-urlencoded' \
  -H 'cache-control: no-cache'

[benaadams]

Content-Length of 0 its happy with

curl -X POST \
  https://.... \
  -H 'Connection: upgrade' \
  -H 'Content-Length: 0' \
  -H 'Content-Type: application/x-www-form-urlencoded' \
  -H 'cache-control: no-cache'

[benaadams]

Don't particularly see anything in the spec saying its disallowed (as the method is POST)

The presence of a message-body in a request is signaled by the inclusion of a Content-Length or Transfer-Encoding header field in the request's message-headers. A message-body MUST NOT be included in a request if the specification of the request method (section 5.1.1) does not allow sending an entity-body in requests. A server SHOULD read and forward a message-body on any request; if the request method does not include defined semantics for an entity-body, then the message-body SHOULD be ignored when handling the request.

However it has also come up before https://github.com/aspnet/KestrelHttpServer/issues/1263#issuecomment-331264006

Our behavior is stricter than what's specified in the HTTP spec. That said, other servers (e.g. http.sys based servers) also reject upgrade requests that indicate the presence of a request body (by including a Transfer-Encoding: chunked or Content-Length header. cc @Tratcher

[delebru]

@sbrudenell I was having a similar issue where my original nginx config had proxy_set_header Connection keep-alive; which caused web sockets errors with blazor. Changing it to proxy_set_header Connection "upgrade"; fixed those WS errors but broke the app's api endpoints which all returned 400 errors.
Setting the connection header as specified on the blazor docs fixed the issue for me, now both WS and POST requests work fine: proxy_set_header Connection $http_connection;

https://docs.microsoft.com/en-us/aspnet/core/host-and-deploy/blazor/server?view=aspnetcore-3.1#linux-with-nginx

[JerryBian]

Just want to add for @delebru post, in my case in order to make $http_connection work, I have to declare it first. Otherwise, nginx will fail:

Error while running nginx -c /etc/nginx/nginx.conf -t.

nginx: [emerg] unknown "connection_upgrade" variable
nginx: configuration file /etc/nginx/nginx.conf test failed

My full nginx configuration is something like:

map $http_upgrade $connection_upgrade {
    default upgrade;
    '' close;
}

server {
    listen        80;
    server_name   xxx;

    location / {
        proxy_pass         http://localhost:5000
        proxy_http_version 1.1;
        proxy_set_header   Upgrade \$http_upgrade;
        proxy_set_header   Connection \$connection_upgrade;
        proxy_set_header   Host \$host;
        proxy_cache_bypass \$http_upgrade;
        proxy_set_header   X-Forwarded-For \$proxy_add_x_forwarded_for;
        proxy_set_header   X-Forwarded-Proto \$scheme;
        proxy_set_header   X-Real-IP \$remote_addr;
    }
}

[WeihanLi]

the same error when I use signalr, any solution here? is that a bug of asp.net core?

[jbgh2]

I've just hit this problem using the AspNetCore app docker sample on a container hosting company.
If you can't change the Nginx settings are their any workarounds in the container?

[cmcnab]

I've just hit this too, using AWS's Elastic Beanstalk with default settings for running in a Docker container. When I try to do a simple POST to a login action all I get is an empty 400 response, never hitting the application code. Checking the debug logs revealed that Connection: Upgrade error in Kestrel. Beanstalk uses nginx as the default proxy and sure enough disabling it "fixes" the problem. It's a managed platform, presumably using a stock nginx config, though it's abstracted away from the user and it doesn't look super straight-forward to be able to edit it.

[msftbot[bot]]

Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We will evaluate the request when we are planning the work for the next milestone. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[TjeuKayim]

I've experienced a similar issue. A java.net.HttpClient with default settings calls an ASP.NET 5.0 server. The request below returns status code 400, because of the headers that tries to upgrade to HTTP2. I'm using the default Kestrel options, so HTTP/2 is disabled.

PUT /file.json HTTP/1.1
Connection: Upgrade, HTTP2-Settings
Content-Length: 4
HTTP2-Settings: AAEAAEAAAAIAAAABAAMAAABkAAQBAAAAAAUAAEAA
Upgrade: h2c
User-Agent: Java-http-client/11.0.9

test

[Tratcher]

Kestrel doesn't support Connection upgrades to h2c, it only supports ALPN based h2 upgrades. The HTTP/2 connection upgrade mechanic is not widely supported, that's actually the first client I've seen attempt it.

However, we should make kestrel capable of ignoring such an upgrade request. The PUT request should be executed like a normal HTTP/1.1 request.

[halter73]

I was surprised that a client (rather than nginx) would send both a "Connection: Upgrade" header and a payload. But then @Tratcher pointed me to:

A client cannot begin using an upgraded protocol on the connection
   until it has completely sent the request message (i.e., the client
   can't change the protocol it is sending in the middle of a message).
   If a server receives both an Upgrade and an Expect header field with
   the "100-continue" expectation (Section 5.1.1 of [RFC7231]), the
   server MUST send a 100 (Continue) response before sending a 101
   (Switching Protocols) response.

https://tools.ietf.org/html/rfc7230#section-6.7 (Part of the HTTP/1.1 spec)

and

  Requests that contain a payload body MUST be sent in their entirety
   before the client can send HTTP/2 frames.  This means that a large
   request can block the use of the connection until it is completely
   sent.

https://tools.ietf.org/html/rfc7540#section-3.2 (HTTP/2 spec)

I don't think a payload is allowed for WebSockets since those need to be GET requests, but the RFCs make it clear that payloads are allowed with upgrade requests.

The simplest "fix" would be to basically ignore the "Connection: Upgrade" header when there's a payload. This means IHttpUpgradeFeature.IsUpgradableRequest will be false for such a request and IHttpUpgradeFeature.UpgradeAsync() would throw an InvalidOperationException.

The correct fix would be to allow upgrading these requests after the payload is consumed and throw if IHttpUpgradeFeature.UpgradeAsync() is called before then. That fix would be far more involved though.

I think it's fine to do the simpler "fix" since with all the reports so far no one has actually wanted to attempt a request upgrade on a request with a payload. Kestrel doesn't support upgrading an HTTP/1.1 request to HTTP/2. Instead, Kestrel supports negotiating HTTP/2 using ALPN and HTTP/2 prior knowledge.