http: fix ssl_redirect on external by lambdai · Pull Request #8664 · envoyproxy/envoy

lambdai · 2019-10-18T17:15:53Z

Description: ssl_redirect on external only is not doing as declared
The reason is that it doesn't determines whether the request is internal in the common approach.
Expect: check the header exists and the value is "true" as everywhere else in the code base.

Risk Level: HIGH, maybe. Since require_tls: EXTERNAL_ONLY user may see different(but correct!) behavior.

Testing:
Docs Changes:
Signed-off-by: Yuchen Dai silentdai@gmail.com

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai · 2019-10-18T17:18:08Z

source/common/router/config_impl.cc

  } else if (ssl_requirements_ == SslRequirements::EXTERNAL_ONLY &&
-             forwarded_proto_header->value() != "https" && !headers.EnvoyInternalRequest()) {
+             forwarded_proto_header->value() != "https" &&
+             not Http::HeaderUtility::isEnvoyInternalRequest(headers)) {


This is the core fix

LGTM. Nit: please use standard predicate syntax ! instead of this new-fangled one that is pretty confusing to most C/C++ folks.

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai · 2019-10-18T18:35:32Z

/assign @htuch

lambdai · 2019-10-22T00:27:14Z

ping @htuch
This is an easy one. The core change is one line as I popped in the comment. Thanks!

htuch

Thanks, this looks like the right fix. Can you add a regression test and then we can ship?
/wait

source/extensions/filters/http/ratelimit/ratelimit.cc

htuch · 2019-10-22T03:56:29Z

test/common/http/conn_manager_utility_test.cc

            ->asString();
    ConnectionManagerUtility::mutateTracingRequestHeader(headers, runtime_, config_, &route_);
-    ret.internal_ = headers.EnvoyInternalRequest() != nullptr;
+    ret.internal_ = HeaderUtility::isEnvoyInternalRequest(headers);


Is there a test that covers the actual regression being fixed?

Probably not. This is test.cc. I guess we don't need another test for test.cc :)
Here is the no brain fix.

I add a test case in router/config_impl_test.cc to cover the core change.

htuch · 2019-10-22T03:58:17Z

source/common/router/config_impl.cc

  } else if (ssl_requirements_ == SslRequirements::EXTERNAL_ONLY &&
-             forwarded_proto_header->value() != "https" && !headers.EnvoyInternalRequest()) {
+             forwarded_proto_header->value() != "https" &&
+             not Http::HeaderUtility::isEnvoyInternalRequest(headers)) {


LGTM. Nit: please use standard predicate syntax ! instead of this new-fangled one that is pretty confusing to most C/C++ folks.

source/common/http/header_utility.cc

lambdai · 2019-10-22T16:31:08Z

Thanks! Will add integration test and fix the nits

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai

Add the test case to cover the product code change.

lambdai · 2019-10-22T23:13:52Z

test/common/http/conn_manager_utility_test.cc

            ->asString();
    ConnectionManagerUtility::mutateTracingRequestHeader(headers, runtime_, config_, &route_);
-    ret.internal_ = headers.EnvoyInternalRequest() != nullptr;
+    ret.internal_ = HeaderUtility::isEnvoyInternalRequest(headers);


Probably not. This is test.cc. I guess we don't need another test for test.cc :)
Here is the no brain fix.

I add a test case in router/config_impl_test.cc to cover the core change.

lambdai · 2019-10-22T23:14:48Z

test/common/router/config_impl_test.cc

    EXPECT_EQ("https://api.lyft.com/foo",
              config.route(headers, 0)->directResponseEntry()->newPath(headers));
  }
+  {


The test case to cover the core 1 line change

htuch

Great, thanks!

mattklein123 · 2019-10-23T17:35:28Z

@lambdai thanks this is a nice cleanup. I just want to clarify for my own understanding though that I don't think there was any actual bug here? The internal header was always sanitized and the only value it was ever filled with was true, so I don't think there should be any actual user behavior change here unless some trusted intermediary was actually changing the value to something else, correct?

* master: (54 commits) Update dependencies - Go, Bazel toos, xxHash, nanopb, rules_go, curl, protobuf, Jinja, yaml-cpp, re2 (envoyproxy#8728) test: increase coverage of listener_manager_impl.cc (envoyproxy#8737) test: modify some macros to reduce number of uncovered lines reported (envoyproxy#8725) build: add a script to setup clang (envoyproxy#8682) http: fix ssl_redirect on external (envoyproxy#8664) docs: update fedora build requirements (envoyproxy#8641) fix draining listener removal logs (envoyproxy#8733) dubbo: fix router doc (envoyproxy#8734) server: provide server health status when stats disabled (envoyproxy#8482) router: adding a knob to configure a cap on buffering for shadowing/retries (envoyproxy#8574) tcp proxy: add default 1 hour idle timeout (envoyproxy#8705) thrift: fix filter names in docs (envoyproxy#8726) Quiche changes to avoid gcc flags on Windows (envoyproxy#8514) test: increase test coverage in Router::HeaderParser (envoyproxy#8721) admin: add drain listeners endpoint (envoyproxy#8415) buffer filter: add content-length when ending stream with trailers (envoyproxy#8609) clarify draining option docs (envoyproxy#8712) build: ignore go-control-plane mirror git commit error code (envoyproxy#8703) api: remove API type database from checked in artifacts. (envoyproxy#8716) admin: correct help strings (envoyproxy#8710) ... Signed-off-by: Spencer Lewis <slewis@squareup.com>

ssl_redirect on external only is not doing as declared The reason is that it doesn't determines whether the request is internal in the common approach. Expect: check the header exists and the value is "true" as everywhere else in the code base. Risk Level: HIGH, maybe. Since require_tls: EXTERNAL_ONLY user may see different(but correct!) behavior. Testing: Unit test added. Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai · 2019-10-24T23:31:39Z

@mattklein123 Thanks!

The internal header was always sanitized and the only value it was ever filled with was true, so I don't think there should be any actual user behavior change here unless some trusted intermediary was actually changing the value to something else, correct?

That's a good point. No other components should set "X-envoy-internal" to false. We should assume the assert since require_tls: EXTERNAL_ONLY should be applied only behind trusted LB.

http: fix ssl_redirect on external

ff4afc8

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai commented Oct 18, 2019

View reviewed changes

fix test

7f6ebec

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

repokitteh-read-only bot assigned htuch Oct 18, 2019

htuch suggested changes Oct 22, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Oct 22, 2019

add tests and fix nits

8b88524

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

repokitteh-read-only bot removed the waiting label Oct 22, 2019

lambdai commented Oct 22, 2019

View reviewed changes

Merge branch 'master' into iheader

e7d5f93

htuch approved these changes Oct 23, 2019

View reviewed changes

htuch merged commit 19f7871 into envoyproxy:master Oct 23, 2019

lambdai deleted the iheader branch October 23, 2019 16:41

Conversation

lambdai commented Oct 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lambdai Oct 18, 2019

Choose a reason for hiding this comment

Uh oh!

htuch Oct 22, 2019

Choose a reason for hiding this comment

Uh oh!

lambdai commented Oct 18, 2019

Uh oh!

lambdai commented Oct 22, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

htuch Oct 22, 2019

Choose a reason for hiding this comment

Uh oh!

lambdai Oct 22, 2019

Choose a reason for hiding this comment

Uh oh!

htuch Oct 22, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lambdai commented Oct 22, 2019

Uh oh!

lambdai left a comment

Choose a reason for hiding this comment

Uh oh!

lambdai Oct 22, 2019

Choose a reason for hiding this comment

Uh oh!

lambdai Oct 22, 2019

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Oct 23, 2019

Uh oh!

lambdai commented Oct 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lambdai commented Oct 18, 2019 •

edited

Loading