Handle Transfer-Encoding as a hop-by-hop header

[ema]

Fixes issue #6907

[maskit]

I may be wrong because I'm not familiar with cache, but we may be able to handle Transfer-Encoding header as hop-by-hop header. Then the block that checks is_this_a_hop_by_hop_header would do the same thing.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Transfer-Encoding

FYI, sending Transfer-Encoding header in a response to a HEAD request seems like allowed.
https://tools.ietf.org/html/rfc7230#section-3.3.1

Transfer-Encoding MAY be sent in a response to a HEAD request or in a
304 (Not Modified) response (Section 4.1 of [RFC7232]) to a GET
request, neither of which includes a message body, to indicate that
the origin server would have applied a transfer coding to the message
body if the request had been an unconditional GET.

[ema]

I may be wrong because I'm not familiar with cache, but we may be able to handle Transfer-Encoding header as hop-by-hop header.

You're right, PR updated accordingly.

[randall]

Looks like the tests ./gold_tests/chunked_encoding are failing (due to a missing Transfer-Encoding: chunked header)

[ema]

Looks like the tests ./gold_tests/chunked_encoding are failing (due to a missing Transfer-Encoding: chunked header)

Yeah, thanks @randall. Unfortunately all chunked_encoding autests are broken for me with or without this patch, so I am not sure how to proceed really: https://phabricator.wikimedia.org/P11661

[randall]

[approve ci autest]

[shinrich]

Looking more closely into the Chunked test failure, the Transfer-Encoding: chunked header was not being sent in the GET request to the origin. Neither was a Content-length header. Which is wrong.

Not clear what is causing the problem with the http2 test.

[shinrich]

Looking into this more deeply, I don't think that marking Transfer-Encoding as hop-by-hop is the correct solution. That is what is breaking at least the chunked-encoding test. ATS is stripping the Transfer-Encoding header so the test origin has no indication of the length of the request body. Probably something similar is happening with the failing Http2 test.

Perhaps instead you could write a plugin (or use header_rewrite) to strip the offending Transfer-Encoding header?

Alternatively, we could add core logic to (or a plugin at the Cache write point) to resolve the content-length/transfer-encoding conflict.

[shinrich]

Talked with @SolidWallOfCode about this. We already do a check on request headers for conflicting content-length and transfer-encoding headers via the validate_hdr_content_length function in proxy/hdrs/HTTP.cc.

Doesn't looks like we invoke that method on response headers. And we should. I assume that would solve this issue. Either making the check as headers go into cache or come out of cache. And definitely as they are being returned to the user agent regardless of caching.

[shinrich]

PR #6992 adds the validate_hdr_content_length check on the response header. Maybe that solves the underlying issue?

[maskit]

Thank you for the deep dive, @shinrich . Calling validate_hdr_content_length for response sounds reasonable.

However, I'm curious if this change (marking Transfer-Encoding as hop-by-hop) breaks something even with #6992. This change looks like a reasonable change, because Transfer-Encoding is hop-by-hop according to the pages above. It would be a trap if we can't mark hop-by-hop headers as hop-by-hop.

[ema]

Thanks so much for looking into this @shinrich!

ATS is stripping the Transfer-Encoding header so the test origin has no indication of the length of the request body.

On TE:chunked responses, however, with this patch applied ATS is doing the right thing and adding TE:chunked when sending a chunked response for cache misses. On cache hit it properly returns a non-chunked response with Content-Length instead. We have been using the patch in production for a while now, and haven't had troubles: https://github.com/wikimedia/operations-debs-trafficserver/blob/master/debian/patches/0062-TE-hop-by-hop.patch

It seems to me that the proper solution would be ensuring that, if a chunked request is sent, ATS adds TE:chunked to the request headers.

I agree with @maskit when he says that it would be strange if we couldn't define Transfer-Encoding as hop-by-hop, given that it is clearly described as such by the RFC.

[shinrich]

Ok, so Transfer-Encoding is defined by the spec to be hop-by-hop, but the current ATS processing is evidently not smart enough to consider the need to re-add Transfer-encoding when sending the body to the next hop (at least for ATS to origin). Seems that that is the thing that needs to be fixed for this PR.

I think doing the valid_hdr_content_length on response headers is valuable, but perhaps just as protecting ATS and its clients from badly behaving origins. So a separate issue.

[vmamidi]

Ok, so Transfer-Encoding is defined by the spec to be hop-by-hop, but the current ATS processing is evidently not smart enough to consider the need to re-add Transfer-encoding when sending the body to the next hop (at least for ATS to origin). Seems that that is the thing that needs to be fixed for this PR.

I think doing the valid_hdr_content_length on response headers is valuable, but perhaps just as protecting ATS and its clients from badly behaving origins. So a separate issue.

@shinrich is this PR breaking POST requests with TE?

[bryancall]

@rob05c Can you please a look at this?

[shinrich]

I think I ran into the problem with the HTTP2 code while working through the HTTP/2 to origin branch. I'll try to pull that part back to the regular main branch.

[randall]

[approve ci autest]

[shinrich]

PR #7473 expands this PR to correct faulty assumptions in the HTTP/2 case.

[shinrich]

Since PR #7473 landed, closing this PR.