[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:
https://github.com/microsoft/reverse-proxy/blob/be99c24e78c34f5eb73c69212c20ce16a00e0367/src/ReverseProxy/Service/Proxy/HttpProxy.cs#L431-L449

[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?