Envoy not removing HTTP/1.1 `TE` and `Connection` header for HTTP/2 backend. #8623

doughd · 2019-10-15T21:00:19Z

Title: HTTP/1.1 TE and connection headers are not being removed when making calls to HTTP/2 backends.

Description:
If you make a HTTP/1.1 request than includes a TE header with a valid HTTP/1.1 value like TE: gzip or TE: deflate,gzip;q=0.3 along with Connection: TE, close, it will be passed along to the HTTP/2 backend where it causes a 503 since it's invalid for HTTP/2.

Shouldn't Envoy be removing the TE and Connection header when making the backend HTTP/2 request? The RFC mentions

  The only exception to this is the TE header field, which MAY be
   present in an HTTP/2 request; when it is, it MUST NOT contain any
   value other than "trailers".

   This means that an intermediary transforming an HTTP/1.x message to
   HTTP/2 will need to remove any header fields nominated by the
   Connection header field, along with the Connection header field
   itself.  Such intermediaries SHOULD also remove other connection-
   specific header fields, such as Keep-Alive, Proxy-Connection,
   Transfer-Encoding, and Upgrade, even if they are not nominated by the
   Connection header field.

Repro steps:

You can see this with a frontend and service envoy. A HTTP/1.1 request goes to the frontend and it creates a HTTP/2 connection to the backend with the TE: gzip header.

HTTP/1.1 envoy front *:8080 -> HTTP/2 envoy service *:9080 (static response)

test-envoy-frontend.yaml

$ cat test-envoy-frontend.yaml
static_resources:
  listeners:
  - address:
      socket_address:
        address: 0.0.0.0
        port_value: 8080
    filter_chains:
    - filters:
      - name: envoy.http_connection_manager
        typed_config:
          "@type": type.googleapis.com/envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager
          codec_type: auto
          stat_prefix: ingress_http
          route_config:
            name: local_route
            virtual_hosts:
            - name: backend
              domains:
              - "*"
              routes:
              - match:
                  prefix: "/"
                route:
                  cluster: backend_service
          http_filters:
          - name: envoy.router
            typed_config: {}
  clusters:
  - name: backend_service
    connect_timeout: 0.25s
    type: static
    lb_policy: round_robin
    http2_protocol_options: {}
    load_assignment:
      cluster_name: backend_service
      endpoints:
      - lb_endpoints:
        - endpoint:
            address:
              socket_address:
                address: 127.0.0.1
                port_value: 9080

test-envoy-backend.yaml

$ cat test-envoy-backend.yaml
static_resources:
  listeners:
  - address:
      socket_address:
        address: 0.0.0.0
        port_value: 9080
    filter_chains:
    - filters:
      - name: envoy.http_connection_manager
        typed_config:
          "@type": type.googleapis.com/envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager
          codec_type: auto
          stat_prefix: ingress_http
          route_config:
            name: local_route
            virtual_hosts:
            - name: backend
              domains:
              - "*"
              routes:
              - match:
                  prefix: "/"
                direct_response:
                  status: 200
                  body:
                    inline_string: "ok\n"
          http_filters:
          - name: envoy.router
            typed_config: {}

Run the envoys (I happened to use this build from Oct 9th. It also happens in the v1.11.2 release):

$ ~/envoy.3582d02f6600057e676629c2afefe5265e669b10 --base-id 1 -c test-envoy-backend.yaml  -l trace

$ ~/envoy.3582d02f6600057e676629c2afefe5265e669b10 --base-id 2 -c test-envoy-frontend.yaml -l trace

Actual without TE header:

This works as expected.

$ time curl -vvv 127.0.0.1:8080/
*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to 127.0.0.1 (127.0.0.1) port 8080 (#0)
> GET / HTTP/1.1
> Host: 127.0.0.1:8080
> User-Agent: curl/7.54.0
> Accept: */*
>
< HTTP/1.1 200 OK
< content-length: 3
< content-type: text/plain
< location: http://127.0.0.1:8080/
< date: Tue, 15 Oct 2019 20:25:42 GMT
< server: envoy
< x-envoy-upstream-service-time: 42
<
ok
* Connection #0 to host 127.0.0.1 left intact

real    0m0.090s
user    0m0.005s
sys     0m0.012s

Actual with TE trailers:

This works because TE: trailers is the only valid value in HTTP/2 per section 8.1.2.2.

$ time curl -vvv -H "TE: trailers" -H "Connection: TE, close" 127.0.0.1:8080/
*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to 127.0.0.1 (127.0.0.1) port 8080 (#0)
> GET / HTTP/1.1
> Host: 127.0.0.1:8080
> User-Agent: curl/7.54.0
> Accept: */*
> TE: trailers
> Connection: TE, close
>
< HTTP/1.1 200 OK
< content-length: 3
< content-type: text/plain
< location: http://127.0.0.1:8080/
< date: Tue, 15 Oct 2019 20:26:31 GMT
< server: envoy
< x-envoy-upstream-service-time: 21
<
ok
* Connection #0 to host 127.0.0.1 left intact

real    0m0.054s
user    0m0.006s
sys     0m0.009s

Actual with TE gzip

This does not work because Envoy is passing the TE: gzip header to a HTTP/2 backend and that's not valid.

$ time curl -vvv -H "TE: gzip" -H "Connection: TE, close" 127.0.0.1:8080/
*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to 127.0.0.1 (127.0.0.1) port 8080 (#0)
> GET / HTTP/1.1
> Host: 127.0.0.1:8080
> User-Agent: curl/7.54.0
> Accept: */*
> TE: gzip
> Connection: TE, close
>
< HTTP/1.1 503 Service Unavailable
< content-length: 95
< content-type: text/plain
< date: Tue, 15 Oct 2019 20:27:11 GMT
< server: envoy
<
* Connection #0 to host 127.0.0.1 left intact
upstream connect error or disconnect/reset before headers. reset reason: connection termination
real    0m1.028s
user    0m0.005s
sys     0m0.008s

When running in trace on the backend Envoy, it is complaining about gzip:

[2019-10-15 13:27:10.557][2740140][trace][http2] [source/common/http/http2/nghttp2.cc:20] nghttp2: inflatehd: header emission: te: gzip
[2019-10-15 13:27:10.557][2740140][trace][http2] [source/common/http/http2/nghttp2.cc:20] nghttp2: recv: proclen=6
[2019-10-15 13:27:10.557][2740140][trace][http2] [source/common/http/http2/nghttp2.cc:20] nghttp2: recv: HTTP error: type=1, id=5, header te: gzip
[2019-10-15 13:27:10.557][2740140][debug][http2] [source/common/http/http2/codec_impl.cc:643] [C2] invalid frame: Invalid HTTP header field was received on stream 5
[2019-10-15 13:27:10.558][2740140][debug][http] [source/common/http/conn_manager_impl.cc:264] [C2] dispatch error: The user callback function failed
[2019-10-15 13:27:10.558][2740140][debug][http] [source/common/http/conn_manager_impl.cc:1734] [C2][S3055301157300998685] stream reset
[2019-10-15 13:27:10.558][2740140][trace][main] [source/common/event/dispatcher_impl.cc:158] item added to deferred deletion list (size=1)

Workaround 1:
A simple workaround is to have the frontend Envoy strip out the TE header with the request_headers_to_remove: ["TE"] option so it isn't passed along to the HTTP/2 connection.

That seems acceptable since RFC 7230 says:

  If the TE field-value is empty or if no TE field is present, the only
   acceptable transfer coding is chunked.  A message with no transfer
   coding is always acceptable.

Workaround 2
I could also disable the HTTP/2 backend connections via commenting out http2_protocol_options: {}.

The text was updated successfully, but these errors were encountered:

mattklein123 · 2019-10-16T17:12:51Z

I thought we had fixed this previously but maybe not. @alyssawilk @PiotrSikora any comment?

alyssawilk · 2019-10-16T18:26:43Z

I had a similar (edit: not quite the same) complaint over on #5541
Turns out I was wrong so didn't do any work

abaptiste · 2019-10-16T22:56:31Z

I'll take a look at this

abaptiste · 2019-10-17T17:32:35Z

I'm reading through #5541 and one comment stands out:

By the H2 spec the only legal value of TE is "trailers", so we could theoretically pass that through coming from H2 and going to H2 (basically strip incoming and outgoing in the HTTP/1.1 codec).

I feel we should limit the sanitization of the TE header only if the outbound connection is H2 and the header contains a value other than "trailers". This change would be a subset of what was proposed in #5541.

Rfc7540 https://tools.ietf.org/html/rfc7540#section-8.1.2 does give some guidance on how these headers should be handled by a proxy, though I'd prefer to take a conservative approach:

This means that an intermediary transforming an HTTP/1.x message to
HTTP/2 will need to remove any header fields nominated by the
Connection header field, along with the Connection header field
itself. Such intermediaries SHOULD also remove other connection-
specific header fields, such as Keep-Alive, Proxy-Connection,
Transfer-Encoding, and Upgrade, even if they are not nominated by the
Connection header field.

doughd · 2019-10-17T20:20:40Z

If you only sanitize TE for outbound H2 connections, then what about the case of HTTP/1.1 downstream and upstream? If you don't sanitize TE in that case, it implies Envoy can handle arbitrary transfer codings for HTTP/1.1. Shouldn't it also sanitize any values that it doesn't understand like arbitrary transfer-extension?

https://tools.ietf.org/html/rfc7230#section-4.3

     TE        = #t-codings
     t-codings = "trailers" / ( transfer-coding [ t-ranking ] )
     t-ranking = OWS ";" OWS "q=" rank
     rank      = ( "0" [ "." 0*3DIGIT ] )
                / ( "1" [ "." 0*3("0") ] )
...
transfer-coding = "chunked" / "compress" / "deflate" / "gzip" /
    transfer-extension
   transfer-extension = token *( OWS ";" OWS transfer-parameter )
   transfer-parameter = token BWS "=" BWS ( token / quoted-string )

The approach by the RFC for HTTP/1.1 and HTTP/2 seem to make sense to me. Remove the associated connection-option headers and optionally replace them with values that the intermediary/proxy understands because these are hop-by-hop parameters.

re: conservative approach. Do you mean not doing the SHOULD part and only doing the MUST? The MUST is above where you quoted:

   HTTP/2 does not use the Connection header field to indicate
   connection-specific header fields; in this protocol, connection-
   specific metadata is conveyed by other means.  An endpoint MUST NOT
   generate an HTTP/2 message containing connection-specific header
   fields; any message containing connection-specific header fields MUST
   be treated as malformed (Section 8.1.2.6).

   The only exception to this is the TE header field, which MAY be
   present in an HTTP/2 request; when it is, it MUST NOT contain any
   value other than "trailers".

doughd · 2019-10-17T20:42:09Z

For background, it turns out this happens in a default user agent configuration. I was seeing the values TE: deflate,gzip;q=0.3 and Connection: TE, close with the 503s. I looked around and Perl's LWP uses those headers by default.

$ perl -MLWP::Simple -e 'getprint "http://127.0.0.1:8000/"'
Request was
GET / HTTP/1.1
TE: deflate,gzip;q=0.3
Connection: TE, close
Host: 127.0.0.1:8000
User-Agent: LWP::Simple/6.00 libwww-perl/6.05

abaptiste · 2019-10-17T22:32:18Z

What I meant by being conservative was that this change would strictly operate on and remove the TE, and Connection headers. In another RFC there were additional headers that it identified can be removed.

In rfc7230 (https://tools.ietf.org/html/rfc7230#section-4.3), it says:

Since the TE header field only applies to the immediate connection, a
sender of TE MUST also send a "TE" connection option within the
Connection header field (Section 6.1) in order to prevent the TE
field from being forwarded by intermediaries that do not support its
semantics.

To me, this justifies the header removal if we find any value for it other than "trailers".

I think my earlier idea of restricting this just to HTTP/2 might be too restrictive. I agree that HTTP/1 requests should be sanitized also.

doughd · 2019-10-21T23:30:12Z

What I meant by being conservative was that this change would strictly operate on and remove the TE, and Connection headers.

Based on your PR, it looks like headers nominated besides TE in the Connection header field are outside of consideration. TE is a special case, but it seems like other named headers should also be handled since this operation is listed as a must for HTTP/1.1 and HTTP/2.

e.g. I think Connection: TE, XXX, close would also need to consider XXX for removal or replacement. connection-option allows for 1#token which is a list where elements may be special values like close or a header name with a restriction than it shouldn't nominate headers intended for the end-to-end connection.

I was thinking of this process to handle the RFC requirements:

For every token mentioned in the Connection header field for a HTTP/1.1 downstream:

For any upstream
- If it names a header that is intended for all recipients like Cache-Control, possibly ignore it? Or return an error (seems extreme)? Would need to check how the RFCs say to handle this.
For HTTP/2 upstream (per RFC 7540 sec 8.1.2.2)
- If it is TE, then check the TE header field value.
  - If it includes trailers and trailers is not set to rank 0, then set the TE header to trailers which implicitly deletes other values in that header field since only the value trailers is acceptable.
  - If it doesn't include trailers or trailers rank is set to 0 (unacceptable), remove the TE header.
- For any other name besides TE, if it names a header, remove that nominated header field.
For HTTP/1.1 upstream (per RFC 7230 sec 6.1)
- If it names a header
  - Envoy could replace it, append or remove elements from the named header field
  - Envoy could also remove the named header field in which case it should also remove the header name from the Connection header value.

If it's a HTTP/2 backend, remove the Connection header field itself. For HTTP/1.1, it could be removed or replaced with a value that Envoy wants.

The above doesn't include the SHOULD part for HTTP/2 where some headers not named in Connection should be removed as well. Seems like a judgement call for Envoy.

PiotrSikora · 2019-10-22T00:52:57Z

Yes, we should remove all headers nominated by the Connection header and then some... I thought this was already fixed in #7981, but it might have been dropped in one of the code reviews.

Regarding the TE header, the only value that's ever used is TE: trailers, and I'm really surprised that Perl is sending those encodings in TE instead of Accept-Encoding.

Description: Sanitize headers nominated by the Connection header Risk Level: Medium Testing: Added several test cases to verify the header manipulation. Also executed bazel test //test/... Docs Changes: N/A Release Notes: N/A Fixes: #8623 Signed-off-by: Alvin Baptiste <[email protected]>

upgle · 2023-07-11T18:28:31Z

I'm having this problem with envoy version 1.25.4.

envoy 1.25.4
client Kotlin Fuel Client (it sends TE header)
Flow
- Client --(http1)--> envoy --(http2)--> envoy

curl -vvv --http1.1 -H "te: gzip, deflate; q=0.5" http://mylocal.com
* Rebuilt URL to: http://mylocal.com/
*   Trying 10.***.***.**
* TCP_NODELAY set
* Connected to mylocal.com (10.***.***.**) port 80 (#0)
> GET / HTTP/1.1
> Host: ***.com
> User-Agent: curl/7.58.0
> Accept: */*
> te: gzip, deflate; q=0.5
>
< HTTP/1.1 503 Service Unavailable
< content-length: 95
< content-type: text/plain
< date: Tue, 11 Jul 2023 18:20:10 GMT
<
* Connection #0 to host mylocal.com left intact
upstream connect error or disconnect/reset before headers. reset reason: connection termination

error log in second envoy

2023-07-11T17:39:15.822246Z debug envoy http2 external/envoy/source/common/http/http2/codec_impl.cc:1263 [C51764] invalid http2: Invalid HTTP header field was received: frame type: 1, stream: 1, name: [te], value: [gzip, deflate; q=0.5] thread=189

dvilaverde · 2023-10-23T00:08:13Z

I'm having this problem with envoy version 1.25.4.

envoy 1.25.4

client Kotlin Fuel Client (it sends TE header)

Flow

Client --(http1)--> envoy --(http2)--> envoy
curl -vvv --http1.1 -H "te: gzip, deflate; q=0.5" http://mylocal.com
* Rebuilt URL to: http://mylocal.com/
*   Trying 10.***.***.**
* TCP_NODELAY set
* Connected to mylocal.com (10.***.***.**) port 80 (#0)
> GET / HTTP/1.1
> Host: ***.com
> User-Agent: curl/7.58.0
> Accept: */*
> te: gzip, deflate; q=0.5
>
< HTTP/1.1 503 Service Unavailable
< content-length: 95
< content-type: text/plain
< date: Tue, 11 Jul 2023 18:20:10 GMT
<
* Connection #0 to host mylocal.com left intact
upstream connect error or disconnect/reset before headers. reset reason: connection termination
error log in second envoy

2023-07-11T17:39:15.822246Z debug envoy http2 external/envoy/source/common/http/http2/codec_impl.cc:1263 [C51764] invalid http2: Invalid HTTP header field was received: frame type: 1, stream: 1, name: [te], value: [gzip, deflate; q=0.5] thread=189

Looks like the same problem I reported here: #30362

mattklein123 added bug help wanted Needs help! labels Oct 16, 2019

mattklein123 added this to the 1.12.0 milestone Oct 16, 2019

This was referenced Oct 21, 2019

Remove per-hop TE and Connection headers #8697

Closed

Remove the TE and Connection headers for unsupported encoding #8700

Closed

alyssawilk modified the milestones: 1.12.0, 1.13.0 Oct 30, 2019

abaptiste mentioned this issue Nov 2, 2019

Sanitize headers nominated by the Connection header #8862

Merged

alyssawilk closed this as completed in #8862 Nov 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Envoy not removing HTTP/1.1 `TE` and `Connection` header for HTTP/2 backend. #8623

Envoy not removing HTTP/1.1 `TE` and `Connection` header for HTTP/2 backend. #8623

doughd commented Oct 15, 2019

mattklein123 commented Oct 16, 2019

alyssawilk commented Oct 16, 2019 •

edited

Loading

abaptiste commented Oct 16, 2019

abaptiste commented Oct 17, 2019

doughd commented Oct 17, 2019

doughd commented Oct 17, 2019

abaptiste commented Oct 17, 2019

doughd commented Oct 21, 2019

PiotrSikora commented Oct 22, 2019

upgle commented Jul 11, 2023

dvilaverde commented Oct 23, 2023 •

edited

Loading

Envoy not removing HTTP/1.1 TE and Connection header for HTTP/2 backend. #8623

Envoy not removing HTTP/1.1 TE and Connection header for HTTP/2 backend. #8623

Comments

doughd commented Oct 15, 2019

mattklein123 commented Oct 16, 2019

alyssawilk commented Oct 16, 2019 • edited Loading

abaptiste commented Oct 16, 2019

abaptiste commented Oct 17, 2019

doughd commented Oct 17, 2019

doughd commented Oct 17, 2019

abaptiste commented Oct 17, 2019

doughd commented Oct 21, 2019

PiotrSikora commented Oct 22, 2019

upgle commented Jul 11, 2023

dvilaverde commented Oct 23, 2023 • edited Loading

Envoy not removing HTTP/1.1 `TE` and `Connection` header for HTTP/2 backend. #8623

Envoy not removing HTTP/1.1 `TE` and `Connection` header for HTTP/2 backend. #8623

alyssawilk commented Oct 16, 2019 •

edited

Loading

dvilaverde commented Oct 23, 2023 •

edited

Loading