http: switching from XFP to scheme by alyssawilk · Pull Request #17372 · envoyproxy/envoy

alyssawilk · 2021-07-15T20:42:23Z

Corrects all Envoy uses of ForwardedProto which actually want request URI over to :scheme

As a reminder, XFP indicates the encryption of the (original) downstream connection where :scheme is part of the URI and the resource requested. It's legal (though unusual) to request http:// urls over a TLS connection for HTTP/2. It's possible (if ill advised) to have an internal mesh forwarding https schemed requests in the clear.

Current uses of X-Forwarded-Proto are

in the HCM, clearing XFP from untrusted users (unchanged)
in the HCM, setting absent XFP based on downstream transport security (unchanged)
in the HCM setting absent :scheme to XFP (unchanged)
in buildOriginalUri, changing from using XFP to scheme (changed. new URIs should be based on original URIs not on transport security.
in the router, clearing default port based on XFP (unchanged)
in the router serving redirect URLs based on scheme (changed - used to be XFP but is now based on the scheme of the original URI)
in the router, applying SSL route redirect based on XFP (unchanged)
in the router, using :scheme for internal redirect url checks (changed - used to use XFP. new URIs should be based on original URI)
in the cache filter, using :scheme to serve content (changed we used to serve based on XFP but if http://foo.com/ differs from https://foo.com and the http version is requested over a TLS connection the http response should be served)
in oath2 serving redirect URLs based on scheme (changed this used to be based on SFP but URLs should be based on original URL scheme)

Risk Level: High
Testing: updated tests
Docs Changes: inline
Release Notes: inline
Runtime guard: envoy.reloadable_features.correct_scheme_and_xfp
Fixes #14587

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

mattklein123

Thanks for tackling this really tough set of changes. A few high level comments.

I would appreciate it if you could split out the rename from the other changes, since I think it would make the tricky stuff easier to review?
In terms of documentation, here is what I would recommend:
1. Potential changes to https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_conn_man/headers.html#scheme
2. Maybe a new HTTP FAQ section with a new entry on scheme/XFP handling?
3. Maybe a new section here on scheme handling? https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/http/http (FAQ and arch overview can cross link as you see fit.)

/wait

mattklein123 · 2021-07-16T20:00:46Z

source/common/http/utility.cc

+      !headers.getSchemeValue().empty()) {
+    return headers.getSchemeValue();
+  }
+  return headers.getXForwardedProtoValue();


What are the cases in which scheme is not but XFP is set? I thought we should always set scheme now early on in the request lifecycle if it's not otherwise set? Can you add more comments?

They're always set at the same point in the HCM so will only be absent with buggy filters.
I can remove the failover here and there and let folks with buggy filters get buggy behavior :-P

mattklein123 · 2021-07-16T20:01:36Z

source/common/router/config_impl.cc

+absl::string_view getXFPOrScheme(const Http::RequestHeaderMap& headers) {
+  absl::string_view xfp = headers.getXForwardedProtoValue();
+  return xfp.empty() ? headers.getSchemeValue() : xfp;
+}


Not sure if this is worth co-locating with Utility::getScheme for comment clarity? At first glance I thought it was the same vs. the inverse.

mattklein123 · 2021-07-16T20:05:51Z

source/common/router/config_impl.cc

+  // TODO(alyssar, mattklein123) is the goal here to force TLS, or force https:
+  // scheme? Maybe upstreams will get confused if seeing an http url.


The original intent here (from a very long time ago) was definitely to force TLS, not force the scheme. So I think this is probably correct?

yeah the only weirdness is then if you have an https:// request in cleartext would you then not serve a redirect to https://foo.com and functionally end up in a redirect loop?

mattklein123 · 2021-07-16T20:06:18Z

source/common/router/config_impl.cc

+    // No scheme header. This normally only happens when ActiveStream::decodeHeaders
+    // bails early (as it rejects a request), so there is no routing is going to happen anyway.


OK this answers my question above. Maybe just move those functions to next to each other and put this comment in each one?

mattklein123 · 2021-07-16T20:08:16Z

source/common/router/router.cc

+    absl::string_view xfp = headers.getXForwardedProtoValue();
+    if (Http::HeaderUtility::schemeIsValid(xfp)) {
+      headers.setScheme(xfp);


This is hurting my brain (should have done this earlier in the day). How come this case isn't using the scheme header if we are preserving downstream scheme? More comments?

mattklein123 · 2021-07-16T20:11:01Z

source/common/runtime/runtime_features.cc

    "envoy.reloadable_features.vhds_heartbeats",
    "envoy.reloadable_features.wasm_cluster_name_envoy_grpc",
    "envoy.reloadable_features.upstream_http2_flood_checks",
+    "envoy.reloadable_features.use_scheme_header",


nit: not sure what would be better, but making this a bit more descriptive would be good.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

mattklein123 · 2021-07-20T17:18:57Z

Thanks this LGTM. Can you merge main?

/wait

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2021-07-21T13:05:48Z

merged, but alas needs a merge stamp

mattklein123

🤞

Corrects all Envoy uses of ForwardedProto which actually want request URI over to :scheme As a reminder, XFP indicates the encryption of the (original) downstream connection where :scheme is part of the URI and the resource requested. It's legal (though unusual) to request http:// urls over a TLS connection for HTTP/2. It's possible (if ill advised) to have an internal mesh forwarding https schemed requests in the clear. Current uses of X-Forwarded-Proto are in the HCM, clearing XFP from untrusted users (unchanged) in the HCM, setting absent XFP based on downstream transport security (unchanged) in the HCM setting absent :scheme to XFP (unchanged) in buildOriginalUri, changing from using XFP to scheme (changed. new URIs should be based on original URIs not on transport security. in the router, clearing default port based on XFP (unchanged) in the router serving redirect URLs based on scheme (changed - used to be XFP but is now based on the scheme of the original URI) in the router, applying SSL route redirect based on XFP (unchanged) in the router, using :scheme for internal redirect url checks (changed - used to use XFP. new URIs should be based on original URI) in the cache filter, using :scheme to serve content (changed we used to serve based on XFP but if http://foo.com/ differs from https://foo.com and the http version is requested over a TLS connection the http response should be served) in oath2 serving redirect URLs based on scheme (changed this used to be based on SFP but URLs should be based on original URL scheme) Risk Level: High Testing: updated tests Docs Changes: inline Release Notes: inline Runtime guard: envoy.reloadable_features.correct_scheme_and_xfp Fixes envoyproxy#14587 Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

http: switching from XFP to scheme

fb9e01d

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk assigned wrowe and mattklein123 Jul 15, 2021

alyssawilk requested review from jmarantz, mattklein123 and snowp as code owners July 15, 2021 20:42

spelling

9427dfc

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

mattklein123 requested changes Jul 16, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 16, 2021

comments

8f69b06

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only bot removed the waiting label Jul 20, 2021

repokitteh-read-only bot added the waiting label Jul 20, 2021

Merge branch 'main' into xfp

dada476

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only bot removed the waiting label Jul 20, 2021

merge fix

7e3f851

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk force-pushed the xfp branch from 3540f3d to 7e3f851 Compare July 20, 2021 21:23

mattklein123 approved these changes Jul 21, 2021

View reviewed changes

alyssawilk merged commit 83e96c5 into envoyproxy:main Jul 21, 2021

Monkeyanator mentioned this pull request Oct 29, 2021

Investigate potentional breaking changes in Envoy 1.20 istio/istio#35480

Closed

alyssawilk deleted the xfp branch February 28, 2022 21:25

This was referenced Apr 25, 2022

envoy_reloadable_features_correct_scheme_and_xfp deprecation #20977

Closed

envoy_reloadable_features_disable_tls_inspector_injection deprecation #20978

Closed

		// TODO(alyssar, mattklein123) is the goal here to force TLS, or force https:
		// scheme? Maybe upstreams will get confused if seeing an http url.

		// No scheme header. This normally only happens when ActiveStream::decodeHeaders
		// bails early (as it rejects a request), so there is no routing is going to happen anyway.

Conversation

alyssawilk commented Jul 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Jul 20, 2021

Uh oh!

alyssawilk commented Jul 21, 2021

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alyssawilk commented Jul 15, 2021 •

edited

Loading