aysnc_client: use proper scheme header

[meierfra-ergon]

Commit Message:
derive scheme header of async client request from upstream config instead of non-existing downstream.

Additional Description:
I saw that the "router decoding headers:" log message in router.cc stated that the :scheme header is always 'http' regardless of the upstream protocol used.
The reason for this is, the stream_info of a async client request does not have a downstream and the :scheme header is by default derived from the downstream (FilterUtility::setUpstreamScheme() call in in router.cc Filter::decodeHeaders() is called with use_upstream=false since callbacks_->streamInfo().shouldSchemeMatchUpstream() is false).

The Fix for this is to set setShouldSchemeMatchUpstream(true) on the async clients stream_info. Then setUpstreamScheme() call will derive the :scheme header from the upstream config.

I`m not sure if this is a real problem, since the async client by default uses http11 and the :scheme header is not sent to the server. Perhaps if the server triggers an http2 upgrade this might be an issue.

Risk Level: low

Testing:

• Manual testing (with JWT Authentication with JwtProvider remote_jwks endpoint configured)
• AsyncClientImplTest::UpstreamTlsScheme test added

Docs Changes: -

Release Notes: Set async client :scheme header depending on upstream protocol

Platform Specific Features: -

[repokitteh-read-only]

Hi @meierfra-ergon, 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: #37588 was opened by meierfra-ergon.

see: more, trace.

[wbpcode]

considering the async client request has no downstream, it is reasonable to always use the upstream protocol to set the schema.

[wbpcode]

cc @adisuissa because @adisuissa merged the shouldSchemeMatchUpstream() feature.

[adisuissa]

My main concern is that this change seems to be impacting all HTTP async clients, and I'm not sure this is the intended outcome of this PR.
Question: should this be configurable for an async client?
If it doesn't need to be configurable, and seems more like a bug fix, I suggest having this under a runtime-guard as it may impact current behavior in an unexpected way.

[meierfra-ergon]

In my opinion this is a bug fix and the scheme should not be configurable. The :scheme pseudo header is directly tied to the scheme of the URI which defines if 'https' or 'http' is being used. So I cannot imagine someone wants to configure this. I even speculate, that a HTTP/2 requests might fail if this :scheme pseudo header is sett to 'http' while the transport is https. I only used the asysnc client with HTTP/1.1 where it does not matter, since the pseudo header is not included in the request.
If you agree I can add a runtime-guard, but to be honest I can not imagine, someone is relying on the behavior that is IMHO plain wrong. But of course you never know for sure.

[adisuissa]

In my opinion this is a bug fix and the scheme should not be configurable. The :scheme pseudo header is directly tied to the scheme of the URI which defines if 'https' or 'http' is being used. So I cannot imagine someone wants to configure this. I even speculate, that a HTTP/2 requests might fail if this :scheme pseudo header is sett to 'http' while the transport is https. I only used the asysnc client with HTTP/1.1 where it does not matter, since the pseudo header is not included in the request.

I tend to agree with your view here.

If you agree I can add a runtime-guard, but to be honest I can not imagine, someone is relying on the behavior that is IMHO plain wrong. But of course you never know for sure.

The intention of the runtime guard is to let users migrate "slowly", i.e., in case one finds that it breaks them, they can disable the change, and ensure that the upstreams behave correctly, before turning the flag on again. I think we had once a case where something that we thought was a bug-fix was broken due to a change, and then the flag was changed to a config-knob.

[adisuissa]

Please also add a release note.

[repokitteh-read-only]

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).

🐱

Caused by: #37588 was synchronize by meierfra-ergon.

see: more, trace.

[meierfra-ergon]

ok, I added a runtime guard and a changelog entry. I hope I`ve done it correctly.

On another note, I was able to 'force' the jwks fetch request (remote_jwks) to use http/2 (by setting http2_protocol_options: {} in the cluster config). With wireshark I could prove the :scheme header really is wrong. My test server (apache) did process the request without a problem. But I can imagine some other servers might not accept a http/2 request like this.

[adisuissa]

LGTM, thanks!
Assigning @yanjunxiang-google who recently worked on this domain, and @yanavlasov for senior-maintainer pass.
/assign @yanjunxiang-google @yanavlasov

[adisuissa]

LGTM, thanks!
/assign-from @envoyproxy/senior-maintainers

[adisuissa]

Please add a one-line comment describing what this test is validating.

[repokitteh-read-only]

@envoyproxy/senior-maintainers assignee is @alyssawilk

🐱

Caused by: a #37588 (review) was submitted by @adisuissa.

see: more, trace.

[yanjunxiang-google]

LGTM

[alyssawilk]

I think this makes sense for some (all?) envoy-initiated requests but I'm pretty sure we don't want to do this unilaterally.

For example, if an edge proxy is getting https requests from a user, but happens to not use TLS internally, the main request will retain the https scheme and shadowed requests will as well. I don't think we want the shadow/mirror request to have a different scheme than the primary.

What use case are you trying to address? Maybe this could be a configurable option for async streams and we can set it at those call sites?

[meierfra-ergon]

I'm not sure, how the shadowed requests are implemented exactly, but if they just use this AsyncClientImpl, then (without my patch) the ':scheme' header will always be 'http' (since there is no downstream reference in the stream_info (see PR description for explanation)). If I`m correct, this will also lead to the behavior that the :scheme header of the shadow request is not the same as the main request (if the main request uses TLS).

In my understanding the :scheme header value is tightly bound to connection of the performed request. If the request is proxied from a 'http' downstream to a 'https' upstream (or vice versa), it is correct that the header is changed, since it belongs to the connection. So I would argue this should also be the case for mirroring. But I see this could be argued differently.

In my "use-case", in a setup with jwt_authn filter and remote_jwks configured, the request fetching the jwks should have the correct :scheme header set. This is not the case when the request is using http/2 (with TLS). I think the same issue exists with other filters using the async client like the oauth2 filter.

In general I think, this this PR changes a hard-coded default (scheme header always 'http') to something adaptive to the (upstream) environment. I think this is a better default. If in some cases the :scheme header should be set to another value than that default I suggest a separate feature.

In any case if this change breaks anything, the runtime guard is still available to disable the new behavior.

[meierfra-ergon]

I have to revise some of my points of the former comment:
If a Http::RequestMessagePtr given to the httpAsyncClientImpl::send() includes a :scheme header, then the headers value is used in the request, otherwise the default is 'http' (due to non-existing downstream). With my patch an existing :scheme` header is ignored, since the upstream scheme is enforced, what could be problematic change in behavior :-/.

I checked shadowWriterImpl::shadow() where a RequestMessagePtr argument is taken.

envoy/source/common/router/shadow_writer_impl.cc

Line 31 in 2425431

void ShadowWriterImpl::shadow(const std::string& cluster, Http::RequestMessagePtr&& request,

So if this holds the original RequestMessage including a :scheme header (which is probable), the resulting shadow request will have the same :scheme header. For me the question remains if this is correct. But I see, that my PR introduces a change I have not anticipated.

[meierfra-ergon]

A different approach for 'my use-case' would be to not derive the ':scheme' header from the upstream cluster but extract the scheme from the uri.
In case of the jwks fetcher (JwksFetcherImpl::fetch()) the request message is prepared with the remote_jwks.uri

envoy/source/extensions/filters/http/common/jwks_fetcher.cc

Line 58 in 2425431

Http::RequestMessagePtr message = Http::Utility::prepareHeaders(remote_jwks_.http_uri());

unfortunately prepareHeaders() only sets :host and :path if there the :scheme also would be set, my problem would also be resolved I guess. For me the question remains if the source of the :scheme header primary be the upstream protocol (from the cluster config) or should it be set by other means.

Please let me know if you would prefer an approach in this direction. Or if the original approach is ok or should be refined. Or if you see a completely different approach.