http: configurable ignore of HTTP/1.1 upgrades

[tedgeat]

Fixes #36305

Add configuration to ignore HTTP/1.1 Upgrade headers . See https://datatracker.ietf.org/doc/html/rfc7230#section-6.7:

Risk Level: Medium
Testing: new unit tests
Docs Changes: in with APIs
Release Notes: inline

[repokitteh-read-only]

Hi @tedgeat, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #37642 was opened by tedgeat.

see: more, trace.

[repokitteh-read-only]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @wbpcode
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #37642 was opened by tedgeat.

see: more, trace.

[ipoval]

@tedgeat 👋 , is it going to work for http2 upstreams too?

[wbpcode]

I don't think remove the unsupported upgrade type and treat the upgrade request as a normal http request is good choice. If clients ask envoy to upgrade but envoy cannot support, rejection would be best choice.

This change brings a new option to our core API and only for a special (or incorrect) hack.

cc @alyssawilk @envoyproxy/senior-maintainers for more input.

/wait

[wbpcode]

From the naming, the options will ignore all upgrade. But actually the implementation didn't works at that way.

[tedgeat]

ignore_http_11_tls_upgrade ?

[alyssawilk]

or repeated string of upgrade to ignore, so it can be extended without adding multiple config fields.

I'd strongly prefer if we're going to ignore we do what we do for h2c and remove the unsupported upgrade. If Envoy is not going to treat an upgrade as an upgrade, but an upstream is, I think you run into problems. If we want to handle the case that a client asks for an upgrade which neither Envoy or the upstream handle, removing the upgrade header is a clear signal that we're handling the upgrade.

If you have a reason to pass the upgrade header through, this PR should not land until you also add handling verifying that the upstream did not attempt to actually upgrade
/wait-any

[bplotnick]

Forwarding the Upgrade header is not valid by my reading of the spec:

When Upgrade is sent, the sender MUST also send a Connection header
field (Section 6.1) that contains an "upgrade" connection option, in
order to prevent Upgrade from being accidentally forwarded by
intermediaries that might not implement the listed protocols.

Connection header is hop-by-hop so it cannot be forwarded. Upgrade cannot be sent without a Connection header. Therefore Upgrade cannot be forwarded (or at the very least is extraneous).

[alyssawilk]

perfect, so a repeated string of upgrade headers to clear makes the most sense to me

[Stono]

I think it's more that new protos will get added and then it'll silently break again in the future when a library uses that.

The goal of this PR was to do what pretty much every webserver we've tested so far does, and ignore unsupported upgrade headers.

The "UX" experience here is that our devs don't run into this problem at all locally, but once their app is deployed onto our cluster, which is backed by Istio and Envoy, things start failing. It suddenly started after people did springboot upgrades, and became a massive rabbithole to debug. We'd like to avoid this in the future:

• dev works on their app locally, everythings fine
• dev deploys app to the cluster, starts seeing 403s
• dev spends hours trying to understand AUTH FAILURES
• dev comes to platform folks for help, speaks to someone who has either since forgotten about this, or is simply new to the team, they rabbithole for a bit too
• eventually we realise what it is, and we put in an additional header in the DestinationRule (assuming istio adds support for this list via DR)

To add to this, given this is a client problem - the change to the DestinationRule will need to be done on each destination service the client talks to, so you create a massive orchestration problem for them to release their change too (eg all their destinations would first need to update this config before they can roll out).

[keithmattix]

@Stono Thanks - I think this is persuasive. If I understand @alyssawilk and @bplotnick correctly, if the desired UX is to just ignore unsupported upgrades, the upgrade header needs to be removed from the upstream request so that the upstream doesn't attempt to upgrade. Is that the main thing missing from this PR?

[Stono]

I don't think it is, is it? Correct me if I'm wrong but you effectively have:

local app -> local envoy -------> remote envoy -> remote app

local app always sends the upgrade header, local envoy currently rejects it.

local envoy would need to ignore it, and if you've implemented the concept of ignore it there, remote envoy could simply ignore it too?

Saying that in my envoyfilter here i kind of do what you've suggested (first allow it on the inbound envoy, then strip it from the outbound request - i needed both because both local and remote envoy would reject)

[keithmattix]

There's no guarantee the next hop is an envoy. Furthermore, you run into the problem @bplotnick describes earlier: #37642 (comment)

[Stono]

That makes sense actually, given connection is per hop, local envoy should see it, ignore it, and not forward it on 👍

[tedgeat]

I don't think remove the unsupported upgrade type and treat the upgrade request as a normal http request is good choice. If clients ask envoy to upgrade but envoy cannot support, rejection would be best choice.

This change brings a new option to our core API and only for a special (or incorrect) hack.

cc @alyssawilk @envoyproxy/senior-maintainers for more input.

/wait

The spec says either to ignore or upgrade and doesn't mention rejection at all. But as that's the current envoy behaviour this makes ignoring opt-in

[phlax]

note: envoy maintainers will be mostly on vacation until 6th jan

/wait-any

[wbpcode]

/wait

[DanielRosenkranz]

NOTE: since the release of spring boot 3.4 in combination with apache camel (http request) this issue also reases if it is used in combination the envoy proxy.

[shantanu-dahiya]

We are facing this issue with Apache Iceberg 1.7+ client which sends the Upgrade header by default, and the server's envoy proxy rejects it and closes the connection with 403.

[tedgeat]

I've renamed the config to ignore_http_11_tls_upgrade to make it clear this removes TLS upgrades specifically, in the spirit of #37150 (comment) . Do you have any thoughts on this @ggreenway ?

[howardjohn]

I've renamed the config to ignore_http_11_tls_upgrade to make it clear this removes TLS upgrades specifically, in the spirit of #37150 (comment) . Do you have any thoughts on this @ggreenway ?

Can we please make this generic with a string matcher? We already have use cases reported for other upgrades so hardcoding a single use case doesn't seem to make sense here.

[keithmattix]

I've renamed the config to ignore_http_11_tls_upgrade to make it clear this removes TLS upgrades specifically, in the spirit of #37150 (comment) . Do you have any thoughts on this @ggreenway ?

Can we please make this generic with a string matcher? We already have use cases reported for other upgrades so hardcoding a single use case doesn't seem to make sense here.

Unfortunately it seems like that would be a bigger change since according to @ggreenway in #37150 (comment), the codec doesn't have access to the configuration until after it's applied some upgrade specific logic

[alyssawilk]

+1 from an API perspective I don't think we want to add specific header knobs to the API if there's ever a use case for generic options (which in this case there clearly are)
/wait

[howardjohn]

I've renamed the config to ignore_http_11_tls_upgrade to make it clear this removes TLS upgrades specifically, in the spirit of #37150 (comment) . Do you have any thoughts on this @ggreenway ?

Can we please make this generic with a string matcher? We already have use cases reported for other upgrades so hardcoding a single use case doesn't seem to make sense here.

Unfortunately it seems like that would be a bigger change since according to @ggreenway in #37150 (comment), the codec doesn't have access to the configuration until after it's applied some upgrade specific logic

Does that comment apply to the current implementation? How could we pass a bool but not a string matcher?

[tedgeat]

@tedgeat I took a quick pass at trying to finish this up, since you had it so close

Thanks Greg!

[wbpcode]

I still cannot get the actual value of this PR. The client ask envoy to upgrade and the proxy cannot support, the correct way should be to reject the request to throw the error.

If Envoy remove the upgrade the proxy the request to upstream, it's maybe result in unexpected behavior for the clients.

I added some comments to the style. And hope someone could address my above worry.

cc @ggreenway

[shantanu-dahiya]

@wbpcode regarding your inquiry into the value of this PR, the HTTP Upgrade page of the Mozilla MDN docs states

The server can ignore the request, for any reason, in which case it should respond as though the Upgrade header had not been sent (for example, with a 200 OK).

This behavior is not possible when the envoy proxy rejects any request with the header set, which cannot be fulfilled. The expected default behavior is to ignore the header, and many services set these headers in their HTTP clients with this assumption.

[ggreenway]

@wbpcode This is ready for you to take another look at

[wbpcode]

LGTM. Thanks.