Update to oghttp2=true causes cookie-related failures

[ravenblackx]

With updating from v1.28.0 to v1.29.1 came runtime flag envoy.reloadable_features.http2_use_oghttp2 defaulting to true.

This caused failures in some of our integration tests, which, after several days of trying to track down what the problem was, I finally isolated to the fact that while envoy was still reporting in its debug log router decoding headers message "cookie: a; b; c", the headers at the upstream were getting interpreted as "cookie: c".

I think that what's probably happening is the codec is restructuring the headers as

cookie: a
cookie: b
cookie: c

and some library in the upstream is (in violation of http spec) interpreting this as three "set" operations that each overwrite the previous cookie value in a dictionary/map type structure.

Flipping the flag back to false fixes the immediate problem; I'm not sure if this is an issue that should be addressed in envoy, or if it's appropriate in these circumstances for envoy to say "the spec says it's okay to restructure that kind of header so the problem is at the upstream."

Note: I'm also not certain this is what's happening, but it's an interpretation that fits the data I have; that the upstream sees only the last cookie, and that envoy's router filter debug logs all the cookies, and that flipping this flag changes that. From this data it could be that the codec is where the lost-cookie flattening is happening, in which case that's a much bigger bug. :)

[ravenblackx]

@birenroy @diannahu

[ravenblackx]

Another possibility that occurs to me that would also fit the data is that maybe the downstream is sending

cookie: a
cookie: b
cookie: c

and the other codec is restructuring that to a single semicolon-separated header.

[yanavlasov]

@ravenblackx do you have repro case?

[birenroy]

I've confirmed that oghttp2 will crumble Cookie headers and deliver them to the application as individual (field name, field value) pairs. From rereading RFC 6265, I can see how this could be unexpected by applications:

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

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

I'll get a fix in shortly.

[birenroy]

/assign @birenroy

[birenroy]

Also relevant: RFC 9113 Section 8.2.3.

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.

[birenroy]

Oh, @ravenblackx , could you clarify what protocols are being spoken on the various hops?

If HTTP/2 is being used between the Envoy and the upstream, then the crumbled version of the cookie should be a valid representation. If it's HTTP/1, then multiple Cookie headers would not be valid.

It's relatively straightforward to consolidate Cookie values received by the codec. It will be a more invasive change to remove the crumbling behavior when serializing HTTP/2 HEADERS frames.

[ravenblackx]

I'm not 100% sure, it's a pretty tangled integration test, but I think we typically use HTTP/1.1 for all upstreams and only support HTTP/2 at the edge.

It looks like in this case the whole thing end to end is probably HTTP/1.1, because the access log entry includes POST /account/get_connected_devices HTTP/1.1

[birenroy]

HTTP/2 has to be in the mix somehow, or flipping the oghttp2 reloadable feature would not have any effect. Let me see what I can do.

[ravenblackx]

Moderate chance then that it's a misconfiguration in the integration test making the initial request http/1.1 and "upgrading" it, the opposite of what we do in production.

[sundarms]

I am also seeing the same behavior
Client send single cookie header with space (http1.1) -> Envoy Lua filter makes call to another service (http2) -> Other service receives multiple cookie header.

[birenroy]

Once Envoy updates to a version of QUICHE that includes google/quiche@82ff95e, I believe this issue will be resolved.

[juanmolle]

I'm sure if could be related. but I'm seeing incorrect behaviour in external processing filter too in 1.29.1 and 1.29.2
while in 1.25.11 in the external processing server using go sdk

&{headers:{headers:{key:":authority" value:"localhost:8000"} headers:{key:":path" value:"/headers"} headers:{key:":method" value:"GET"} headers:{key:":scheme" value:"http"} headers:{key:"user-agent" value:"curl/8.2.1"} headers:{key:"accept" value:"/"} headers:{key:"x-forwarded-proto" value:"http"} headers:{key:"x-request-id" value:"1a5ffd66-3dc3-4186-9e3e-2072dbf2d558"}}

in 1.29.1 I'm getting

pb.ProcessingRequest_RequestHeaders &{headers:{headers:{key:":authority" 3:"localhost:8000"} headers:{key:":path" 3:"/headers"} headers:{key:":method" 3:"GET"} headers:{key:":scheme" 3:"http"} headers:{key:"user-agent" 3:"curl/8.2.1"} headers:{key:"accept" 3:"/"} headers:{key:"x-forwarded-proto" 3:"http"} headers:{key:"x-request-id" 3:"dd5ef52f-249a-4191-a6b6-e3f54b8ac8cd"}}

could it be related to http2? or grpc library changed?

[birenroy]

I'm sure if could be related. but I'm seeing incorrect behaviour in external processing filter too in 1.29.1 and 1.29.2 while in 1.25.11 in the external processing server using go sdk

Can you clarify what incorrect behavior you're seeing, @juanmolle ? It's not clear to me from the snippets you included.

[birenroy]

FWIW, I believe this issue was fixed as of #32874, which included google/quiche@82ff95e.

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

[github-actions]

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted" or "no stalebot". Thank you for your contributions.