hcm: forbid use of detection extensions with use_remote_addr/xff_num_trusted_hops by rgs1 · Pull Request #17558 · envoyproxy/envoy

rgs1 · 2021-07-31T13:03:55Z

Mixing extensions with previously existing knobs leads to undefined behavior,
so this removes the deprecation around xff_num_trusted_hops and ensures that
it's not mixed with extensions.

Note that a unit test already exists for the original bug report, where
use_remote_address is used with xff_num_trusted_hops > 0. However it uses
the old knob instead of XFF extension. Given this is now forbidden,
there's no need for additional tests wrt that config combination.

Fixes #17554.

Signed-off-by: Raul Gutierrez Segales rgs@pinterest.com

…um_trusted_hops Mixing extensions with previously existing knobs leads to undefined behavior, so this removes the deprecation around xff_num_trusted_hops and ensures that it's not mixed with extensions. Note that a unit test already exists for the original bug report, where use_remote_address is used with xff_num_trusted_hops > 0. However it uses the XFF extension instead of the old knob. Given this is now forbidden, there's no need for additional tests wrt that config combination. Fixes envoyproxy#17554. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

repokitteh-read-only · 2021-07-31T13:04:02Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #17558 was opened by rgs1.

see: more, trace.

rgs1 · 2021-07-31T18:08:45Z

One possible wrinkle is:

  // Edge request is the request from external clients to front Envoy.
  // Request from front Envoy to the internal service will be treated as not edge request.
  const bool edge_request = !internal_request && config.useRemoteAddress();

which means, if you are using an extension (or a list of them) you won't be able to treat requests as edge. However, if you are using an extension you most likely are not on the edge. So we can punt solving this one, until an extension writer comes along with the need to label requests as edge.

rgs1 · 2021-07-31T18:08:56Z

/retest

repokitteh-read-only · 2021-07-31T18:09:00Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17558 (comment) was created by @rgs1.

see: more, trace.

mattklein123

Thanks this makes sense to me. I feel like there should be more documentation around this. Can we explain this somewhere in the arch overview or HTTP docs where we already talk about XFF, num trusted hops, etc.?

/wait

docs/root/version_history/current.rst

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

rgs1 · 2021-08-03T02:17:25Z

Thanks this makes sense to me. I feel like there should be more documentation around this. Can we explain this somewhere in the arch overview or HTTP docs where we already talk about XFF, num trusted hops, etc.?

Added some, looking for additional spots that could use warnings around the mix of extensions and the pre-existing knobs.

rgs1 · 2021-08-03T02:18:32Z

/wait

…e-17554

rgs1 · 2021-08-03T18:07:53Z

/wait

…e-17554

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

* fix link * update old integration test to go back to not use extensions Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

mattklein123

Thanks!

mattklein123 · 2021-08-06T19:11:12Z

Oops looks like you need a main merge.

/wait

…e-17554

rgs1 · 2021-08-06T20:23:33Z

Err this seems unrelated:

[0 / 1] [Prepa] BazelWorkspaceStatusAction stable-status.txt
Target //tools/testing:python_coverage up-to-date:
  bazel-bin/tools/testing/python_coverage
INFO: Elapsed time: 0.275s, Critical Path: 0.11s
INFO: 3 processes: 3 internal.
INFO: Build completed successfully, 3 total actions
INFO: Running command line: bazel-bin/tools/testing/python_coverage /build/tmp/_bazel_envoybuild/b570b5ccd0454dc9af9f65ab1833764d/execroot/envoy/bazel-out/k8-fastbuild/bin/tools/testing/all_pytests.runfiles/envoy/.coverage-envoy /source/generated/tooling
INFO: Build completed successfully, 3 total actions
PytestChecker ERROR Summary
--------------------------------------------------------------------------------
PytestChecker pytests:
//tools/extensions:pytest_extensions_check failed

PytestChecker ERROR {'success': 17, 'errors': 1, 'warnings': 0, 'failed': {'pytests': 1}, 'warned': {}, 'succeeded': {'pytests': 17}}

cc: @phlax

rgs1 · 2021-08-06T20:23:45Z

/retest

repokitteh-read-only · 2021-08-06T20:23:48Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17558 (comment) was created by @rgs1.

see: more, trace.

rgs1 · 2021-08-06T23:27:30Z

In the case of ARM:

//test/integration:sds_dynamic_integration_test                          FAILED in 2 out of 2 in 15.0s
  Stats over 2 runs: max = 15.0s, min = 14.8s, avg = 14.9s, dev = 0.1s
  /build/tmp/_bazel_envoybuild/b570b5ccd0454dc9af9f65ab1833764d/execroot/envoy/bazel-out/aarch64-opt/testlogs/test/integration/sds_dynamic_integration_test/test.log
  /build/tmp/_bazel_envoybuild/b570b5ccd0454dc9af9f65ab1833764d/execroot/envoy/bazel-out/aarch64-opt/testlogs/test/integration/sds_dynamic_integration_test/test_attempts/attempt_1.log

rgs1 · 2021-08-06T23:27:40Z

/retest

repokitteh-read-only · 2021-08-06T23:27:43Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17558 (comment) was created by @rgs1.

see: more, trace.

adisuissa

/lgtm api

mattklein123

Thanks!

* main: (687 commits) ci: set build debug information from env (envoyproxy#17635) ext_authz: do the authentication even the direct response is set (envoyproxy#17546) upstream: various cleanups in connection pool code (envoyproxy#17644) owners: promote Dmitry to maintainer (envoyproxy#17642) quiche: client session supports creating bidi stream (envoyproxy#17543) Update HTTP/2 METADATA documentation. (envoyproxy#17637) ext_proc: Check validity of the :status header (envoyproxy#17596) test: add ASSERT indicating that gRPC stream has not been started yet (envoyproxy#17614) ensure that the inline cookie header will be folded correctly (envoyproxy#17560) cluster_manager: Make ClusterEntry a class instead of a struct (envoyproxy#17616) owners: make Raúl a Thrift senior extension maintainer (envoyproxy#17641) quiche: update QUICHE dependency (envoyproxy#17618) Delete mock for removed RouteEntry::perFilterConfig() method (envoyproxy#17623) REPO_LAYOUT.md: fix outdated link (envoyproxy#17626) hcm: forbid use of detection extensions with use_remote_addr/xff_num_trusted_hops (envoyproxy#17558) thrift proxy: add request shadowing support (envoyproxy#17544) ext_proc: Ensure that timer is always cancelled (envoyproxy#17569) Proposal: Add CachePolicy interface to allow for custom cache behavior (envoyproxy#17362) proto: fix verify to point at v3 only (envoyproxy#17622) api: move generic matcher proto to its own package (envoyproxy#17096) ...

* main: (687 commits) ci: set build debug information from env (envoyproxy#17635) ext_authz: do the authentication even the direct response is set (envoyproxy#17546) upstream: various cleanups in connection pool code (envoyproxy#17644) owners: promote Dmitry to maintainer (envoyproxy#17642) quiche: client session supports creating bidi stream (envoyproxy#17543) Update HTTP/2 METADATA documentation. (envoyproxy#17637) ext_proc: Check validity of the :status header (envoyproxy#17596) test: add ASSERT indicating that gRPC stream has not been started yet (envoyproxy#17614) ensure that the inline cookie header will be folded correctly (envoyproxy#17560) cluster_manager: Make ClusterEntry a class instead of a struct (envoyproxy#17616) owners: make Raúl a Thrift senior extension maintainer (envoyproxy#17641) quiche: update QUICHE dependency (envoyproxy#17618) Delete mock for removed RouteEntry::perFilterConfig() method (envoyproxy#17623) REPO_LAYOUT.md: fix outdated link (envoyproxy#17626) hcm: forbid use of detection extensions with use_remote_addr/xff_num_trusted_hops (envoyproxy#17558) thrift proxy: add request shadowing support (envoyproxy#17544) ext_proc: Ensure that timer is always cancelled (envoyproxy#17569) Proposal: Add CachePolicy interface to allow for custom cache behavior (envoyproxy#17362) proto: fix verify to point at v3 only (envoyproxy#17622) api: move generic matcher proto to its own package (envoyproxy#17096) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

…trusted_hops (envoyproxy#17558) Mixing extensions with previously existing knobs leads to undefined behavior, so this removes the deprecation around xff_num_trusted_hops and ensures that it's not mixed with extensions. Note that a unit test already exists for the original bug report, where use_remote_address is used with xff_num_trusted_hops > 0. However it uses the XFF extension instead of the old knob. Given this is now forbidden, there's no need for additional tests wrt that config combination. Fixes envoyproxy#17554. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

rgs1 requested review from alyssawilk and mattklein123 as code owners July 31, 2021 13:03

repokitteh-read-only bot added the api label Jul 31, 2021

repokitteh-read-only bot assigned adisuissa Jul 31, 2021

mattklein123 self-assigned this Aug 2, 2021

mattklein123 requested changes Aug 2, 2021

View reviewed changes

docs/root/version_history/current.rst Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Aug 2, 2021

Docs

20c7cb9

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

repokitteh-read-only bot removed the waiting label Aug 3, 2021

repokitteh-read-only bot added the waiting label Aug 3, 2021

Merge remote-tracking branch 'upstream/main' into xff-regression-issu…

89ca3c0

…e-17554

repokitteh-read-only bot removed the waiting label Aug 3, 2021

repokitteh-read-only bot added the waiting label Aug 3, 2021

Raul Gutierrez Segales added 2 commits August 4, 2021 17:56

Merge remote-tracking branch 'upstream/main' into xff-regression-issu…

c7e9621

…e-17554

Add warning to the original_ip_detection_extensions field

87d818d

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

repokitteh-read-only bot removed the waiting label Aug 4, 2021

Fixes

7431126

* fix link * update old integration test to go back to not use extensions Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

mattklein123 previously approved these changes Aug 6, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Aug 6, 2021

repokitteh-read-only bot added the waiting label Aug 6, 2021

Merge remote-tracking branch 'upstream/main' into xff-regression-issu…

e6175d7

…e-17554

rgs1 dismissed mattklein123’s stale review via e6175d7 August 6, 2021 19:14

repokitteh-read-only bot added api and removed waiting labels Aug 6, 2021

adisuissa reviewed Aug 7, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Aug 7, 2021

mattklein123 approved these changes Aug 9, 2021

View reviewed changes

mattklein123 merged commit 57c172f into envoyproxy:main Aug 9, 2021

sunjayBhatia mentioned this pull request Apr 14, 2022

Envoy in PROXY mode uses untrusted x-forwarded-for header information projectcontour/contour#4449

Closed

Conversation

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

Uh oh!

Uh oh!

repokitteh-read-only bot commented Jul 31, 2021

Uh oh!

rgs1 commented Jul 31, 2021

Uh oh!

rgs1 commented Jul 31, 2021

Uh oh!

repokitteh-read-only bot commented Jul 31, 2021

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rgs1 commented Aug 3, 2021

Uh oh!

rgs1 commented Aug 3, 2021

Uh oh!

rgs1 commented Aug 3, 2021

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Aug 6, 2021

Uh oh!

rgs1 commented Aug 6, 2021

Uh oh!

rgs1 commented Aug 6, 2021

Uh oh!

repokitteh-read-only bot commented Aug 6, 2021

Uh oh!

rgs1 commented Aug 6, 2021

Uh oh!

rgs1 commented Aug 6, 2021

Uh oh!

repokitteh-read-only bot commented Aug 6, 2021

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

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

rgs1 commented Jul 31, 2021 •

edited

Loading