HTTP/1.1 TLS Upgrade (RFC-2817) causes upgrade_failed

[bplotnick]

Title: HTTP/1.1 TLS Upgrade (RFC-2817) causes upgrade_failed

Description: RFC-2817 specifies two ways for a client to indicate an optional TLS upgrade in HTTP/1.1. The first is via a CONNECT and the second is by sending the Connection: upgrade header and an Upgrade header field with the upgrade protocols. However, sending this to Envoy currently will cause a 403 with upgrade_failed.

Notably the RFC states:

In this case, the server MAY respond to the clear HTTP operation normally, OR switch to secured operation (as detailed in the next section).

This is a little bit ambiguous, but my reading is that the options are to either ignore it or to upgrade, but rejecting the request entirely isn't an option.

RFC-9110 gives us the same ambiguity:

A server MAY ignore a received Upgrade header field if it wishes to continue using the current protocol on that connection. Upgrade cannot be used to insist on a protocol change.

The envoy source tells us why it might be rejecting the upgrade entirely:

      // While downstream servers should not send upgrade payload without the upgrade being
      // accepted, err on the side of caution and refuse to process any further requests on this
      // connection, to avoid a class of HTTP/1.1 smuggling bugs where Upgrade or CONNECT payload
      // contains a smuggled HTTP request.

Repro steps: Make a request over plaintext HTTP/1.1 with 'Connection: Upgrade' and 'Upgrade: TLS/1.3' headers set

This came up because Apache HttpComponents HttpClient added RFC-2817 headers as default in 5.4 (https://github.com/apache/httpcomponents-client/pull/542). This was released as GA this past week, so should start to hit people as they upgrade.

The bigger issue is the default behavior of HttpClient. I have filed an issue there https://issues.apache.org/jira/browse/HTTPCLIENT-2344. However, we should consider whether we are being spec compliant vs the conservative approach to rejecting these requests.

[ggreenway]

cc @alyssawilk

[alyssawilk]

So Envoy doesn't support this feature by default, but I think you could in fact get it to work by configuring Envoy to accept this type of upgrade: https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/http/upgrades and configuring Envoy to route the "upgrade terminated" request back through a second pass of Envoy to a TLS listener (or internal listener)

[ggreenway]

I don't think the request here is to have TLS upgrades succeed, but to have Envoy ignore the upgrade request and continue with no upgrade and no error.

[bplotnick]

What @ggreenway said, but just to add on, while i think the spec is ambiguous, i tested a few other proxies, and they all ignore it. The Apache HttpClient folks have closed the issue as "invalid" (which i disagree with). Given all this, I think de-facto we should ignore it.

[alyssawilk]

Ah, when it comes to issues of security I lean towards being strict, rather than possibly increasing attack surface. I'd say if someone were enthusiastic enough to contribute a "silently allow" default off config knob I'd allow it, but I'm not inclined to add one myself :-)

[broman1234]

I don't think the request here is to have TLS upgrades succeed, but to have Envoy ignore the upgrade request and continue with no upgrade and no error.

Sounds good, but how to have Envoy ignore the upgrade request?

[alyssawilk]

specify the upgrade type in upgrade configs: https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/http/upgrades https://github.com/envoyproxy/envoy/issues/36469 also relevant - I think the reporter plans to work on it

[Jayden-Lind]

The current fix is to do the below at the HTTP Connection Manager, which will allow these requests to pass through

filter_chains:
  - filters:
    - name: "envoy.http_connection_manager"
      typed_config:
        upgrade_configs:
          - upgrade_type: "TLS/1.3"
            enabled: true

[ipoval]

👋 team, we are a large consumer of Envoy proxy and have started to experience the production impact of this issue in our service mesh.

I would like to kindly encourage the Envoy team to prioritise formulating a position on this issue (plans to fix it, if any, how, and maybe a rough ETA), as it will help many users devise the appropriate guardrails in their enterprise workloads. 🙇

[alyssawilk]

I know this was originally filed as a bug, but given the ambiguity of the spec (and that allowing upgrades by default has negative security implications) I think it's a feature request. Unfortunately Envoy is supported on a volunteer basis - we don't have a core team dedicated to features, so any additions to Envoy are contributed by the companies that desire those features. If your service mesh is negatively impacted we encourage you to become envoy contributors so you can address it!