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 (apache/httpcomponents-client#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
#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!

[rafalh]

I don't see how the spec is ambiguous. It says the server should either accept the upgrade request or decline it by ignoring it. If Envoy does not do this it is clearly incompatible with RFC-2817. For me it sounds like a bug, not a feature.

[chlunde]

@alyssawilk, could you assist potential contributors by providing guidance on how the Envoy configuration should look if we want to add an option to ignore (specific?) upgrade requests? Alternatively, could you direct us to someone who can help? This would greatly clarify the effort needed to contribute a fix for this issue.

[alyssawilk]

@chlunde I'd suggest extending the UpgradeConfigs with a passthrough option.

[ggreenway]

I think the best option for ignoring an upgrade request is to strip the upgrade header before sending the request to the upstream, so that there can't be any confusion between envoy and the upstream about whether an upgrade is happening or not.

We can add configuration for this to UpgradeConfig to ignore specific upgrade types, or we could consider adding an option to ignore any unknown upgrade.

[oyvindhorneland]

I'm looking at https://www.rfc-editor.org/rfc/rfc2817#section-5 and trying to understand if a passthrough option makes sense in this context?

5. Upgrade across Proxies

As a hop-by-hop header, Upgrade is negotiated between each pair of
HTTP counterparties. If a User Agent sends a request with an Upgrade
header to a proxy, it is requesting a change to the protocol between
itself and the proxy, not an end-to-end change.

[ggreenway]

I'm working on a fix for this right now. If I don't have time to finish it, I'll at least post a draft PR that someone can take and finish this.

[DavidSchinazi]

Since I was tagged in #37150 and replied there, I'm adding the same context here:

There's an important distinction between HTTP versions here:

• Upgrade is used only in HTTP/1.1, it is documented in Section 7.8 of RFC 9110
• Extended CONNECT is used only in HTTP/2 and HTTP/3, it is documented in RFC 8441 and RFC 9220

They behave differently: in particular, in HTTP/1.1 the server can "ignore" the Upgrade and still send a successful HTTP/1.1 response without upgrading to a different protocol. In HTTP/2 and 3 however, that is not possible. If the server does not wish to use the other protocol, it has no choice but to reject the request.

This gets particularly tricky because Envoy normalizes every Extended CONNECT internally to look like Upgrade. So the information of whether ignoring is allowed or not gets somewhat hard to track down inside Envoy.

I worry that allowing Envoy to ignore upgrades will cause protocol confusion or request smuggling attacks, unless there are strong safeguards in place to ensure that this cannot happen in HTTP/2 or HTTP/3.

[chlunde]

Would it be a good idea to also propose that RFC 2817 optional client initiated upgrade mechanism be deprecated? Does anyone here have the resources to do so and think it is a good idea? https://github.com/httpwg/http-core/blob/main/CONTRIBUTING.md#proposing-new-work

I believe it has several security implications that make it problematic in modern contexts.

Some examples of issues:

1. Man-in-the-Middle (MITM) Vulnerability: An attacker could intercept the initial HTTP request and strip out the Upgrade header, preventing the connection from being upgraded to HTTPS. This leaves the communication vulnerable to eavesdropping and modification.

2. Downgrade Attacks: Similar to MITM attacks, an attacker could modify the client's request to remove the Upgrade header, forcing the communication to remain unencrypted.

3. Information Leakage: The initial request is sent over cleartext HTTP, potentially exposing sensitive information before the connection is upgraded.

4. Complexity: The upgrade mechanism adds complexity to both client and server implementations, potentially introducing new bugs or security flaws, such as request smuggling.

5. Potential for Confusion: Users may believe they're using a secure connection from the start when in reality, the initial request is sent in the clear.

6. Limited Adoption: Because of these security concerns, this mechanism hasn't been widely adopted, which means it's not well-tested in real-world scenarios.

What does it help for?

A misconfigured client and a passive eavesdroppers.

Alternatives

HSTS, HTTPS Everywhere, rejecting HTTP requests, automatic redirects from HTTP to HTTPS, and simply initiating requests on port https/port 443 instead of 80 by default

[DavidSchinazi]

@chlunde I totally agree with your intent here - upgrading to TLS is incredibly insecure, and clients should instead always connect directly over HTTPS.

Unfortunately, RFC 2817 is still used in the real-world by the Internet Printing Protocol, see Section 8.2 of RFC 8010. So it doesn't make sense to mark RFC 2817 as obsolete. Even if it should be avoided in almost all cases.

That said, deprecating the feature from Envoy is a different question, but I don't know if anyone is using it.

[ggreenway]

That said, deprecating the feature from Envoy is a different question, but I don't know if anyone is using it.

We don't have any intention to make TLS upgrades work; I'm just trying to make Envoy ignore those upgrade requests instead of failing requests, to increase compatibility with clients (such as https://issues.apache.org/jira/browse/HTTPCLIENT-2344) which insist on using this upgrade type.

[DavidSchinazi]

I see. Thanks for the context!

[ggreenway]

The approach in #37150 didn't work due to how the http1 codec changes behavior when it thinks its handling an upgrade.

A fix for this would probably have to be in the http1 codec_impl, but that doesn't have access to the configured upgrades which are processed later, so it would probably need to be a change that specifically ignores TLS upgrades, similar to how we ignore h2c upgrades now.

[ggreenway]

I'm out of time to address this; I'll leave this to someone else.

Here's a rough patch that could fix this for TLS upgrades specifically, but I don't like hard-coding another upgrade type to ignore; configuration would be better.

diff --git a/source/common/http/headers.h b/source/common/http/headers.h
index 17550b4a0b..b5e0d9d9ca 100644
--- a/source/common/http/headers.h
+++ b/source/common/http/headers.h
@@ -248,6 +248,8 @@ public:
 
   struct {
     const std::string H2c{"h2c"};
+    const std::string TlsPrefix{
+        "tls/"}; // A valid upgrade request includes a version, such as "TLS/1.2".
     const std::string WebSocket{"websocket"};
     const std::string ConnectUdp{"connect-udp"};
   } UpgradeValues;
diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc
index aea743c5f6..2187fe4cc2 100644
--- a/source/common/http/http1/codec_impl.cc
+++ b/source/common/http/http1/codec_impl.cc
@@ -65,6 +65,11 @@ static constexpr uint32_t kMaxOutboundResponses = 2;
 using Http1ResponseCodeDetails = ConstSingleton<Http1ResponseCodeDetailValues>;
 using Http1HeaderTypes = ConstSingleton<Http1HeaderTypesValues>;
 
+const StringUtil::CaseUnorderedSet& caseUnorderdSetContainingUpgrade() {
+  CONSTRUCT_ON_FIRST_USE(StringUtil::CaseUnorderedSet,
+                         Http::Headers::get().ConnectionValues.Upgrade);
+}
+
 const StringUtil::CaseUnorderedSet& caseUnorderdSetContainingUpgradeAndHttp2Settings() {
   CONSTRUCT_ON_FIRST_USE(StringUtil::CaseUnorderedSet,
                          Http::Headers::get().ConnectionValues.Upgrade,
@@ -848,12 +853,19 @@ StatusOr<CallbackResult> ConnectionImpl::onHeadersCompleteImpl() {
   if (Utility::isUpgrade(request_or_response_headers) && upgradeAllowed()) {
     // Ignore h2c upgrade requests until we support them.
     // See https://github.com/envoyproxy/envoy/issues/7161 for details.
-    if (absl::EqualsIgnoreCase(request_or_response_headers.getUpgradeValue(),
-                               header_values.UpgradeValues.H2c)) {
-      ENVOY_CONN_LOG(trace, "removing unsupported h2c upgrade headers.", connection_);
+    auto upgrade_value = request_or_response_headers.getUpgradeValue();
+    const bool is_h2c = absl::EqualsIgnoreCase(upgrade_value, header_values.UpgradeValues.H2c);
+
+    // A valid TLS upgrade could be
+    const bool is_tls =
+        absl::StartsWithIgnoreCase(upgrade_value, header_values.UpgradeValues.TlsPrefix);
+
+    if (is_h2c || is_tls) {
+      ENVOY_CONN_LOG(trace, "removing unsupported h2c or tls upgrade headers.", connection_);
       request_or_response_headers.removeUpgrade();
       if (request_or_response_headers.Connection()) {
-        const auto& tokens_to_remove = caseUnorderdSetContainingUpgradeAndHttp2Settings();
+        const auto& tokens_to_remove = is_h2c ? caseUnorderdSetContainingUpgradeAndHttp2Settings()
+                                              : caseUnorderdSetContainingUpgrade();
         std::string new_value = StringUtil::removeTokens(
             request_or_response_headers.getConnectionValue(), ",", tokens_to_remove, ",");
         if (new_value.empty()) {
@@ -862,7 +874,9 @@ StatusOr<CallbackResult> ConnectionImpl::onHeadersCompleteImpl() {
           request_or_response_headers.setConnection(new_value);
         }
       }
-      request_or_response_headers.remove(header_values.Http2Settings);
+      if (is_h2c) {
+        request_or_response_headers.remove(header_values.Http2Settings);
+      }
     } else {
       ENVOY_CONN_LOG(trace, "codec entering upgrade mode.", connection_);
       handling_upgrade_ = true;

[bgalek]

👋 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. 🙇

The same goes for my org!
@ggreenway kudos for your effort!

Is envoy team actively looking for a workaround?

[ggreenway]

I don't believe anyone is currently working on this.

[bgalek]

@ggreenway thank you, I'll try to find somebody to work on it

[Stono]

I agree wholeheartedly with this sentiment, but, frustratingly, I don't see apache httpclient moving on this at all. It's going to be chaos as more and more libraries upgrade, given how prolific the use of envoy is.

[keithmattix]

/cc @krinkinmu @grnmeira @Stevenjin8 do any of you have bandwidth to look into this?

[krinkinmu]

@keithmattix I don't have time at the moment and will look at this on Friday and see what I can do.

[Stono]

Hey, we've got someone working on this at the moment and he has a working prototype we think.

Let's see where he gets to, to save duplicated effort?

[tedgeat]

Hi, I've extended @ggreenway 's codec approach to make it configurable and remain as a rejection by default #37642