[Question/Bug] Cookie value separator changed to comma?

[Tornhoof]

It's more a question, since I can't readily reproduce it:

I'm proxying a basic GET request to a different server (in this case Tomcat) and once in awhile I get a log message in tomcat that an invalid cookie was received (org.apache.tomcat.util.http.parser.Cookie logInvalidHeader). This once in awhile happens if multiple cookies are in one header.
These cookie values should be delimited via semicolon (according to RFC 6265), older RFCs had it as comma. Tomcat's default cookie parser does not accept cookies delimited by comma.

Wiresharek dump

If I attach wireshark to the listening port, the received plaintext body is (from a test installation)

GET /test/?url=e508a3ba052148508d707682da9da951&locale=en HTTP/1.1
Host: localhost:61440
Accept: text/html, application/xhtml+xml, application/xml; q=0.9, image/avif, image/webp, image/apng, */*; q=0.8, application/signed-exchange; v=b3; q=0.9
Accept-Encoding: gzip, deflate, br
Accept-Language: de-DE, de; q=0.9, en-US; q=0.8, en; q=0.7
Cookie: access_token=2F1766A645002D0EFBA7E1924594174C3A550598EA5D4B3C3114C6674BE02542, test_data_e508a3ba052148508d707682da9da951=%7B%22id%22%3A%22CASdG2aURvjsWtHNWedyZsvRYPJnK15R2-NRsR1nOibZPQ%22%2C%22title%22%3A%221.2%20cover%22%2C%22url%22%3A%22https%3A%2F%2Flocalhost%3A44302%2Fapi%2FFile%2FASCF-3JBQErG_it4xRT-rbf_BKXVajYvYhsrvXcaUQeadg%2FContent%22%2C%22sequenceNumber%22%3A%220001%22%7D
Referer: https://localhost:44302/
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/85.0.4183.121 Safari/537.36
Upgrade-Insecure-Requests: 1
sec-fetch-site: same-origin
sec-fetch-mode: navigate
sec-fetch-dest: iframe
X-Forwarded-Proto: https
X-Forwarded-Host: localhost:44302
X-Forwarded-For: ::1

You see the comma separated cookie header values which are wrong.
Funny thing is, as soon as I e.g. attach Fiddler it appears that the cookie separator is still a semicolon, I don't know why ;)

The original request to Kestrel is HTTP2 and the Tomcat is HTTP/1.1 tough.

The best idea I currently have is that something is parsing the Header and reassembling it instead of a verbatim copy, as the Cookie Header is one of the few headers which require semicolon as the separator and things like StringValues.ToString() is using comma as an separator.
You can reproduce that behaviour with something like this:

var output = new HttpClient();
var message = new HttpRequestMessage(HttpMethod.Get, "http://localhost:45844");
message.Headers.Add(HeaderNames.Cookie, new string[]
{
    "test1=1", "test2=2"
});
await output.SendAsync(message);

and the headers sent over the wire are comma-separated and not semicolon-separated, that's why my best guess is something is parsing the header value and reassembling it, but I couldn't find any code which would do that and the Transforms Property in the ProxyRoute is null and SessionAffinity is also null (I suspected the CookieSessionAffinity logic).

Configuration

The configuration is done via code, via the InMemoryConfiguration code from the docs:

var routes = new[]
{
    new ProxyRoute()
    {
        RouteId = "proxyRoute",
        ClusterId = "Rewrite",
        Match =
        {
            Path = "/test"
        }
    }
};
var clusters = new[]
{
    new Cluster()
    {
        Id = "Rewrite",
        Destinations =
        {
            {"destination", new Destination() {Address = "http://localhost:61440"}}
        }
    }
};

Further technical details

• Version: 1.0.0-preview.5.20467.3
• Windows 10
• NET Core 3.1.8

[Tratcher]

Yeah, Cookie and Set-Cookie are headers we have to be very careful with. We've got #155 to make sure we add test coverage for these scenarios, but I'll leave this issue open as a specific example we need to investigate.

The relevant code is here:

reverse-proxy/src/ReverseProxy/Service/Proxy/HttpProxy.cs

Lines 431 to 449 in be99c24

	static void AddHeader(HttpRequestMessage request, string headerName, StringValues value)
	{
	if (value.Count == 1)
	{
	string headerValue = value;
	if (!request.Headers.TryAddWithoutValidation(headerName, headerValue))
	{
	request.Content?.Headers.TryAddWithoutValidation(headerName, headerValue);
	}
	}
	else
	{
	string[] headerValues = value;
	if (!request.Headers.TryAddWithoutValidation(headerName, headerValues))
	{
	request.Content?.Headers.TryAddWithoutValidation(headerName, headerValues);
	}
	}
	}

[Tratcher]

Nothing in the stack should be messing with the cookie header if it were in the Cookie: cookie1=value1; cookie2=value2 format. Most likely the incoming headers are in this format:

Cookie: cookie1=value1
Cookie: cookie2=value2

Then StringValues or request.Headers.TryAddWithoutValidation combine the separate headers with a comma.

[Tornhoof]

I think you're correct with your assessment, I patched some basic fix cookie header code into httpproxy.cs after the headers are added, this takes the cookie header and joins them with semicolon.

// Fix cookie header:
if (destination.Headers != null && destination.Headers.TryGetValues(HeaderNames.Cookie, out var values))
{
    var semicolonValues = string.Join("; ", values);
    destination.Headers.Remove(HeaderNames.Cookie);
    destination.Headers.TryAddWithoutValidation(HeaderNames.Cookie, semicolonValues);
}

if (destination.Content?.Headers != null && destination.Content.Headers.TryGetValues(HeaderNames.Cookie, out var contentValues))
{
    var semicolonValues = string.Join("; ", contentValues);
    destination.Content.Headers.Remove(HeaderNames.Cookie);
    destination.Content.Headers.TryAddWithoutValidation(HeaderNames.Cookie, semicolonValues);
}

and the comma separation problem is gone.

[samsp-msft]

is this a bug in the client sending requests with multiple cookie headers or YARP for not accepting/passing along the cookies?

[Tratcher]

It's still under investigation but I expect it's a bug in HttpClient where it's being passed an array of cookie headers and it combines them inappropriately. It could also be an issue with how YARP calls HttpClient.

[ManickaP]

@Tornhoof How does the original request look like? Do you have a capture of it?

We're using TryAddWithoutValidation for all headers, header values are in general concatenated with ,: https://github.com/dotnet/runtime/blob/270777119024f39d97fd02b9ef23fbdba96a8883/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaderParser.cs#L12 and there's no special casing for Cookie headers in runtime (no custom parser overriding the separator): https://github.com/dotnet/runtime/blob/270777119024f39d97fd02b9ef23fbdba96a8883/src/libraries/System.Net.Http/src/System/Net/Http/Headers/KnownHeaders.cs#L43.

IMO it's a bug in runtime that should get fixed but the timeline for it probably won't be sooner then 6.0. I'll file an issue there and in the meantime I suggest to handle it in YARP.

Note that the old RFC says the cookies might be concatenated with comma (,): https://tools.ietf.org/html/rfc2109#section-4.3.4:

A server should also accept comma (,) as the separator between cookie-values for future compatibility.

While the new one specifies only semi-colon as a separator (;): https://tools.ietf.org/html/rfc6265#section-4.2.1

cookie-string = cookie-pair *( ";" SP cookie-pair )

[Tornhoof]

@ManickaP I'm sorry I don't have a dump of the original request as I had no luck getting the tls data decoded via wireshark and as soon as I put Fiddler inbetween everything worked (probably because Fiddler rewrites it).

[ManickaP]

I don't have a dump of the original request

No worries, I'll manage without. I just wanted to make sure that the headers are coming as described above.

I'll take a stab at the fix in YARP as the next step.

BTW, dotnet/runtime issue filed: dotnet/runtime#42856

[Tratcher]

@ManickaP is there any way to make HttpClient send headers in this format?

Header1: value1
Header1: value2
Header1: value3

I think that's the format the client is sending them in. That would be preferable to concatenating them.

Also, are there any differences here between HTTP/1.x and HTTP/2?

[ManickaP]

is there any way to make HttpClient send headers in this format?

Not that I'm aware of.

are there any differences here between HTTP/1.x and HTTP/2?

Only when combining cookies from the container with the ones in headers; otherwise, no. I just tested it and results are in the runtime issue here: dotnet/runtime#42856 (comment)

[Tratcher]

@ManickaP I'm sorry I don't have a dump of the original request as I had no luck getting the tls data decoded via wireshark and as soon as I put Fiddler inbetween everything worked (probably because Fiddler rewrites it).

@Tornhoof What client are you using? Is the request from the client to server using HTTP/1 or HTTP/2? For HTTP/1 you can use Kestrel Connection Logging to get a raw dump of the request between decryption and parsing:
https://docs.microsoft.com/en-us/aspnet/core/fundamentals/servers/kestrel?view=aspnetcore-3.1#connection-logging

Some notes from the specs:

https://www.rfc-editor.org/rfc/rfc6265.html#section-5.4

When the user agent generates an HTTP request, the user agent MUST
NOT attach more than one Cookie header field.

If the client is HTTP/1.1 then sending multiple Cookie headers is invalid. However, that doesn't prevent YARP from mitigating it.

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

To allow for better compression efficiency, the Cookie header field
MAY be split into separate header fields, each with one or more
cookie-pairs. If there are multiple Cookie header fields after
decompression, these MUST be concatenated into a single octet string
using the two-octet delimiter of 0x3B, 0x20 (the ASCII string "; ")
before being passed into a non-HTTP/2 context, such as an HTTP/1.1
connection, or a generic HTTP server application.

I don't know if Kestrel properly accounts for this, we'd have to check. I don't see anything in the HPACK decoder or Kestrel to special case Cookies. If the client is using HTTP/2 in the reported scenario then a fix in Kestrel would address the issue.

[ManickaP]

@Tratcher Add your comment dotnet/runtime#42856 (comment)

https://tools.ietf.org/html/rfc7540#section-8.1.2.5

If there are multiple Cookie header fields after
decompression, these MUST be concatenated into a single octet string
using the two-octet delimiter of 0x3B, 0x20 (the ASCII string "; ")
before being passed into a non-HTTP/2 context, such as an HTTP/1.1
connection, or a generic HTTP server application.

Shouldn't then Kestrel expose a single Cookie header with ; concatenated values in context.Request.Headers???

Either way, runtime is not handling cookie headers well, there's no question about that.

[Tratcher]

@ManickaP if the client is using HTTP/2, yes. I've filed dotnet/aspnetcore#26461 for follow-up in Kestrel.

If the client is using HTTP/1.1 then they're the one doing something invalid, kestrel isn't required to do any special fixup.

EDIT: It seems likely that we'll mitigate this in YARP as well as fix both Kestrel and HttpClient.

[ManickaP]

we'll mitigate this in YARP as well

Yep, that's what I thought. I'm working on it.

[Tornhoof]

@Tratcher
The Client is Google Chrome and as far as I saw in the original request it was Http2

The original request to Kestrel is HTTP2 and the Tomcat is HTTP/1.1 tough.

I'll try to get the kestrel connection logging working and i'll post the request then.

[Tratcher]

Ah, missed that; Then this likely is an issue with Kestrel HTTP/2 that we should address.

The connection logging isn't going to help much with HTTP/2 since it's a binary format. It will capture the bytes but they're not easy to decode. We should be able to reproduce it directly though.

[Tornhoof]

Yeah I noticed that the HTTP2 isn't really readable, I managed to get the Wireshark TLS decrypt working on a different system, where I could get that sslkeylogfile variable working.
The HTTP2 header output looks like (it's a slightly different app version, but the cookies are the same)

Frame 4969: 147 bytes on wire (1176 bits), 147 bytes captured (1176 bits) on interface \Device\NPF_Loopback, id 0
Null/Loopback
Internet Protocol Version 4, Src: 127.0.0.1, Dst: 127.0.0.1
Transmission Control Protocol, Src Port: 53521, Dst Port: 44302, Seq: 10044, Ack: 56920, Len: 103
Transport Layer Security
    TLSv1.2 Record Layer: Application Data Protocol: http2
        Content Type: Application Data (23)
        Version: TLS 1.2 (0x0303)
        Length: 98
        Encrypted Application Data: 000000000000002184018f30a245117b51369687c2b257c9…
HyperText Transfer Protocol 2
    Stream: HEADERS, Stream ID: 55, Length 65, GET /test/?url=5e088a7100b54a8f97cdd90454743b6a&locale=en
        Length: 65
        Type: HEADERS (1)
        Flags: 0x25
        0... .... .... .... .... .... .... .... = Reserved: 0x0
        .000 0000 0000 0000 0000 0000 0011 0111 = Stream Identifier: 55
        [Pad Length: 0]
        1... .... .... .... .... .... .... .... = Exclusive: True
        .000 0000 0000 0000 0000 0000 0000 0000 = Stream Dependency: 0
        Weight: 255
        [Weight real: 256]
        Header Block Fragment: 82d88704ac607616a90b62d3d2663fcb6ca206ca079e1ba1…
        [Header Length: 1106]
        [Header Count: 15]
        Header: :method: GET
        Header: :authority: localhost:44302
        Header: :scheme: https
        Header: :path: /test/?url=5e088a7100b54a8f97cdd90454743b6a&locale=en
        Header: upgrade-insecure-requests: 1
        Header: user-agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/85.0.4183.121 Safari/537.36
        Header: accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3;q=0.9
        Header: sec-fetch-site: same-origin
        Header: sec-fetch-mode: navigate
        Header: sec-fetch-dest: iframe
        Header: referer: https://localhost:44302/
        Header: accept-encoding: gzip, deflate, br
        Header: accept-language: de-DE,de;q=0.9,en-US;q=0.8,en;q=0.7
        Header: cookie: access_token=AAEB9CE9F523BB4A4AB090B18E19AF10AAA95B2C4791551AAB8988A0AD18018D
        Header: cookie: test_data_5e088a7100b54a8f97cdd90454743b6a=%7B%22id%22%3A%22CASdG2aURvjsWtHNWedyZsvRYPJnK15R2-NRsR1nOibZPQ%22%2C%22title%22%3A%221.2%20test%22%2C%22url%22%3A%22https%3A%2F%2Flocalhost%3A44302%2Fapi%2FFile%2FASCF-3JB

[Tornhoof]

If i force HTTP 1.1 support in Kestrel via config then it's working, so it's apparently a problem for HTTP/2 only

serverOptions.Listen(IPAddress.Any, 44302, listenOptions =>
{
    listenOptions.Protocols = HttpProtocols.Http1;
    listenOptions.UseHttps();
    listenOptions.UseConnectionLogging();
});

the connection dump then looks like this (with line breaks visible)

GET /test?url=adeae64ff5f646ae8f9f3f253960986a&locale=en HTTP/1.1
Host: localhost:44302
Connection: keep-alive
Upgrade-Insecure-Requests: 1
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/85.0.4183.121 Safari/537.36
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3;q=0.9
Sec-Fetch-Site: same-origin
Sec-Fetch-Mode: navigate
Sec-Fetch-Dest: iframe
Referer: https://localhost:44302/
Accept-Encoding: gzip, deflate, br
Accept-Language: de-DE,de;q=0.9,en-US;q=0.8,en;q=0.7
Cookie: access_token=F41256095A7D2B93EC515DE3B93B59215DCCCD7559C9D4CF41173969457A0B29; test_data_adeae64ff5f646ae8f9f3f253960986a=%7B%22id%22%3A%22CASdG2aURvjsWtHNWedyZsvRYPJnK15R2-NRsR1nOibZPQ%22%2C%22title%22%3A%221.2%20test%22%2C%22url%22%3A%22https%3A%2F%2Flocalhost%3A44302%2Fapi%2FFile%2FASCF-3JBQErG_it4xRT-rbf_BKXVajYvYhsrvXcaUQeadg%2FContent%22%2C%22sequenceNumber%22%3A%220001%22%7D

[halter73]

Then this likely is an issue with Kestrel HTTP/2 that we should address

Agreed. In the meantime, this should be easy for YARP to work around this by manually merging the headers. This logic won't cause any problems after the Kestrel issue is addressed since the logic will no longer be triggered after the Cookie request headers are pre-merged by Kestrel. Getting the Kestrel fix into 5.0 GA or a 3.1 backport might be difficult Given how easy the workaround is and how long this went unnoticed.

[halter73]

If i force HTTP 1.1 support in Kestrel via config then it's working, so it's apparently a problem for HTTP/2 only

That's likely because Chrome avoids violating https://tools.ietf.org/html/rfc6265#section-5.4 by not sending multiple Cookie headers as part of an HTTP/1.1 request. As @Tratcher points out, the HTTP/2 spec supersedes this requirement "to allow for better compression efficiency" for HTTP/2 requests, but that doesn't change the requirements for HTTP/1.1 requests.

If you could force a client to send multiple Cookie headers as part of an HTTP/1.1 request, you'd probably see the same issue.