ip-tagging filter: add support for an optional ip-tag-header field #36434

Radha13 · 2024-10-03T15:06:52Z

Description

This change adds support for specifying an optional header to ip-tagging filter instead of the default header that it uses (x-envoy-ip-tags).

example

   http_filters
     - name: ip.tagging
       typed_config:
         "@type": type.googleapis.com/envoy.extensions.filters.http.ip_tagging.v3.IPTagging
         request_type: BOTH
         ip_tag_header: "x-slack-foo-bar"
         ip_tags: []

Why

Currently, the ip-tagging filter always writes its output into the x-envoy-ip-tags header. When this filter is used for more than one purpose in the same filter chain which we do at Slack in our production environment, we do need to take care of cleaning up the header in between which has been proven a bit tricky sometimes. Leaking the values in between the filter chain is bad so we try to avoid that.

We would like this to be configurable. This way we can use the optional header instead of trying the use the same header however many times the same filter is used on the same filter chain in a listener.

Risk Level

this is a new feature, doesn't affect the existing functionality so guessing low but open to changing if I'm wrong.

Testing

added test

Docs Changes: Added, alongside release notes

Co-authored-by: Ariane van der Steldt [email protected]
Signed-off-by: Radha Kumari [email protected]

Signed-off-by: Radha Kumari <[email protected]>

rest Signed-off-by: Radha Kumari <[email protected]>

Signed-off-by: Radha Kumari <[email protected]>

repokitteh-read-only · 2024-10-03T15:07:02Z

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

🐱

Caused by: #36434 was opened by Radha13.

see: more, trace.

adisuissa

Thanks!
Overall LGTM, left a few minor comments.
Please also add a release note as this is a user-facing change.

api/envoy/extensions/filters/http/ip_tagging/v3/ip_tagging.proto

adisuissa · 2024-10-07T12:46:32Z

source/extensions/filters/http/ip_tagging/ip_tagging_filter.h

@@ -71,6 +73,7 @@ class IpTaggingFilterConfig {
  const Stats::StatName total_;
  const Stats::StatName unknown_tag_;
  std::unique_ptr<Network::LcTrie::LcTrie<std::string>> trie_;
+  const Http::LowerCaseString header_;


nit: rename to alt_header_ or something ip_tag_header_ to make it more readable.

test/extensions/filters/http/ip_tagging/ip_tagging_filter_test.cc

source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc

adisuissa · 2024-10-07T12:51:30Z

source/extensions/filters/http/ip_tagging/ip_tagging_filter.h

@@ -39,6 +39,8 @@ class IpTaggingFilterConfig {
  FilterRequestType requestType() const { return request_type_; }
  const Network::LcTrie::LcTrie<std::string>& trie() const { return *trie_; }

+  const Http::LowerCaseString& ip_tag_header() const { return header_; }


Please add a comment here, stating that empty string implies that no ip_tag_header is set.

I do wonder if it just makes more sense to return the correct ip_tag_header here (either the default or the alternative). On the one hand I think it may make the code a bit more readable, but then the calls to the headers.append will need to be a bit different.

Yeah I added the conditional in decodeHeaders as first, it was easier and second the decision on which header to use is made when we are appending the tags but I'm open to returning the correct header from ip_tag_header(). I will have to see how involved that change is but let me know if that's what you prefer.

Co-authored-by: Adi (Suissa) Peleg <[email protected]> Signed-off-by: Radha <[email protected]>

Signed-off-by: Radha Kumari <[email protected]>

Radha13 · 2024-10-08T19:15:55Z

Almost forgot, don't really know how to add release notes but will check the docs 🙇🏽‍♀️

adisuissa

Thanks!
A release note should be added here: https://github.com/envoyproxy/envoy/blob/main/changelogs/current.yaml

Assigning @alyssawilk for code-owner review
/assign @alyssawilk

api/envoy/extensions/filters/http/ip_tagging/v3/ip_tagging.proto

Co-authored-by: Adi (Suissa) Peleg <[email protected]> Signed-off-by: Radha <[email protected]>

alyssawilk · 2024-10-09T13:09:01Z

weird, that assign didn't work.
@JuniorHsu can you do first pass?

Signed-off-by: Radha Kumari <[email protected]>

JuniorHsu · 2024-10-09T18:47:39Z

ci fails

CodeChecker ERROR [yamllint] changelogs/current.yaml (line-length 484:141): line too long (152 > 140 characters)
CodeChecker ERROR [yamllint] Check failed

Signed-off-by: Radha Kumari <[email protected]>

JuniorHsu · 2024-10-09T21:43:23Z

i'll review after ci is happy but this looks like a replacement of #35628
and ideally we want to modify docs/root/configuration/http/http_filters/ip_tagging_filter.rst since the behavior might be changed by your pr

cqi1217 · 2024-10-09T21:53:58Z

source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc

-    headers.appendEnvoyIpTags(tags_join, ",");
+
+    // Use the optional header instead of x-envoy-ip-tags if it's not empty.
+    if (!config_->ip_tag_header().get().empty()) {


I think we need to sanitize this header so others can not spoof it. When you configure ip_tag_header as x-foo-tags, and the requests come in with x-foo-tags header, if the requests does not get any tag from envoy ip tagging filter, they still have x-foo-tags header and the value can be anything.

at least we want to default to sanitize otherwise we need to set security level to untrusted downstream. I didn't check metadata tho.

@JuniorHsu @cqi1217 I think @nahratzah meant to post #36434 (comment) here. let me know what you folks think about this.

@alyssawilk we hit a dilemma here.
currently ip_tagging is robust to untrusted downstream

envoy/source/extensions/extensions_metadata.yaml

Line 438 in a684860

security_posture: robust_to_untrusted_downstream

however my understanding is non x- prefixed header won't be sanitized by envoy, which is vulnerable, so appending header could downgrade the security_posture.

However, @nahratzah mention the asymmetric behavior in #36434 (comment)

What do you think?

it'd help if I had clarity around why the user wants a custom header. was it to bypass the x-envoy checks? If it's just "we want a custom header" we could require an x-envoy prefix. If they want a non-x-envoy header I agree this would be changing the security model and either that needs to be fixed, or (if intentioanl) needs a lot more documentation

Radha13 · 2024-10-09T21:55:07Z

@JuniorHsu Apologies but the CI failures looks to be Java exceptions and looks unrelated to the change unless I'm mistaken.

Error: ERROR: /source/WORKSPACE:17:19: fetching http_archive rule //external:com_googlesource_googleurl: Traceback (most recent call last):
Error: ERROR: @com_googlesource_googleurl//build_config:system_icu :: Error loading option @com_googlesource_googleurl//build_config:system_icu: java.io.IOException: Error downloading [https://storage.googleapis.com/quiche-envoy-integration/dd4080fec0b443296c0ed0036e1e776df8813aa7.tar.gz] to /build/bazel_root/base/external/com_googlesource_googleurl/temp14106256169062325536/dd4080fec0b443296c0ed0036e1e776df8813aa7.tar.gz: GET returned 403 Forbidden
Error: ERROR: An error occurred during the fetch of repository 'com_googlesource_googleurl':
Warning: WARNING: Download from https://storage.googleapis.com/quiche-envoy-integration/dd4080fec0b443296c0ed0036e1e776df8813aa7.tar.gz failed: class java.io.IOException GET returned 403 Forbidden

Also, wasn't really aware of #35628 before you mentioned. Looks like it was closed due to inactivity. Let me make the change to docs.

adisuissa · 2024-10-10T12:33:54Z

@JuniorHsu Apologies but the CI failures looks to be Java exceptions and looks unrelated to the change unless I'm mistaken.

Error: ERROR: /source/WORKSPACE:17:19: fetching http_archive rule //external:com_googlesource_googleurl: Traceback (most recent call last):
Error: ERROR: @com_googlesource_googleurl//build_config:system_icu :: Error loading option @com_googlesource_googleurl//build_config:system_icu: java.io.IOException: Error downloading [https://storage.googleapis.com/quiche-envoy-integration/dd4080fec0b443296c0ed0036e1e776df8813aa7.tar.gz] to /build/bazel_root/base/external/com_googlesource_googleurl/temp14106256169062325536/dd4080fec0b443296c0ed0036e1e776df8813aa7.tar.gz: GET returned 403 Forbidden
Error: ERROR: An error occurred during the fetch of repository 'com_googlesource_googleurl':
Warning: WARNING: Download from https://storage.googleapis.com/quiche-envoy-integration/dd4080fec0b443296c0ed0036e1e776df8813aa7.tar.gz failed: class java.io.IOException GET returned 403 Forbidden

Also, wasn't really aware of #35628 before you mentioned. Looks like it was closed due to inactivity. Let me make the change to docs.

Please try to merge latest main. There's ongoing work migrating the CI infra in the last couple of weeks.

nahratzah · 2024-11-04T15:15:09Z

Test failure appears unrelated, and to be due to a connection-timed-out while downloading `kafka_server_binary`.

ERROR: An error occurred during the fetch of repository 'kafka_server_binary'
Error in download_and_extract: java.io.IOException: Error downloading [https://downloads.apache.org/kafka/3.8.0/kafka_2.13-3.8.0.tgz] to /build/bazel_root/base/external/kafka_server_binary/temp1195429097495657859/kafka_2.13-3.8.0.tgz: connect timed out

alyssawilk · 2024-11-04T15:37:16Z

source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc

+  OptRef<const Http::LowerCaseString> header_name = config_->ip_tag_header();
+
+  if (tags.empty()) {
+    bool mustSanitize =


this isn't really mustSanitize given we'll only do it if there's a header name configured. Also snake case for Envoy variables
maybe_sanitize?

(I wish there was a linter for this.) <3

alyssawilk

Looks good - just a few more nits and you're good to go
/wait

alyssawilk · 2024-11-04T15:40:19Z

source/extensions/filters/http/ip_tagging/ip_tagging_filter.h

@@ -39,6 +42,14 @@ class IpTaggingFilterConfig {
  FilterRequestType requestType() const { return request_type_; }
  const Network::LcTrie::LcTrie<std::string>& trie() const { return *trie_; }

+  OptRef<const Http::LowerCaseString> ip_tag_header() const {


and for functions ipTagHeader and ipTagHeaderAction

alyssawilk · 2024-11-04T15:41:27Z

api/envoy/extensions/filters/http/ip_tagging/v3/ip_tagging.proto

+    // (DEFAULT) The header specified in :ref:`ip_tag_header <envoy_v3_api_field_extensions.filters.http.ip_tagging.v3.IPTagging.ip_tag_header>`
+    // will be dropped, before the tags are applied. Sanitize will be run regardless of if the source is a trusted IP or not.
+    //
+    // Note that the header will be visible unsanitized to any filters downstream of the ip-tag-header filter.


oh, mind adding
unless it has an x-envoy prefix

alyssawilk · 2024-11-04T15:42:04Z

test/extensions/filters/http/ip_tagging/ip_tagging_filter_test.cc

+  initializeFilter(internal_request_yaml);
+
+  Http::TestRequestHeaderMapImpl request_headers{
+      {"x-envoy-internal", "true"}, {"x-envoy-optional-header", "foo"}}; // foo will be removed


mind adding multiple just so we regression test that rmove does remove all?

Signed-off-by: Ariane van der Steldt <[email protected]>

nahratzah · 2024-11-04T16:14:30Z

Updated the PR. <3

alyssawilk

wohoo!

alyssawilk · 2024-11-04T16:16:55Z

@adisuissa any final shepherd comments?

adisuissa · 2024-11-04T19:03:59Z

api/envoy/extensions/filters/http/ip_tagging/v3/ip_tagging.proto

+  // Control if the header in :ref:`ip_tag_header <envoy_v3_api_field_extensions.filters.http.ip_tagging.v3.IPTagging.ip_tag_header>`
+  // will be sanitized, or be appended to.
+  //
+  // This is ignored if :ref:`ip_tag_header <envoy_v3_api_field_extensions.filters.http.ip_tagging.v3.IPTagging.ip_tag_header>` is empty.


As this field is closely related to the ip_tag_header field, it will be better to have them both under the same message type, and make the ip_tag_header field in that message type required (and probably move the HeaderAction to that inner message as well).

adisuissa · 2024-11-04T19:09:02Z

api/envoy/extensions/filters/http/ip_tagging/v3/ip_tagging.proto

+  // Describes how to apply the tags to the headers.
+  enum HeaderAction {
+    // (DEFAULT) The header specified in :ref:`ip_tag_header <envoy_v3_api_field_extensions.filters.http.ip_tagging.v3.IPTagging.ip_tag_header>`
+    // will be dropped, before the tags are applied. Sanitize will be run regardless of if the source is a trusted IP or not.


Suggested change

// will be dropped, before the tags are applied. Sanitize will be run regardless of if the source is a trusted IP or not.

// will be dropped, before the tags are applied. The incoming header will be "sanitized" regardless of whether the request is an internal or external.

I'm trying to stick to the same terminology introduced in https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_conn_man/header_sanitizing. I'm assuming that is the goal, and if so, I suggest trying to stick with the same terms.

adisuissa · 2024-11-04T19:12:53Z

api/envoy/extensions/filters/http/ip_tagging/v3/ip_tagging.proto

+    // (DEFAULT) The header specified in :ref:`ip_tag_header <envoy_v3_api_field_extensions.filters.http.ip_tagging.v3.IPTagging.ip_tag_header>`
+    // will be dropped, before the tags are applied. Sanitize will be run regardless of if the source is a trusted IP or not.
+    //
+    // Note that the header will be visible unsanitized to any filters downstream of the ip-tag-header filter, unless it has an *x-envoy* prefix.


The word "downstream" may be ambiguous here.
I'm assuming it implies the filters that are invoked after this filter, but I'm not sure. Can you clarify it (probably by not using the word "downstream")?

adisuissa · 2024-11-04T19:13:34Z

api/envoy/extensions/filters/http/ip_tagging/v3/ip_tagging.proto

+    // :ref:`ip_tag_header <envoy_v3_api_field_extensions.filters.http.ip_tagging.v3.IPTagging.ip_tag_header>`,
+    // leaving original values in place.


Suggested change

// :ref:`ip_tag_header <envoy_v3_api_field_extensions.filters.http.ip_tagging.v3.IPTagging.ip_tag_header>`,

// leaving original values in place.

// :ref:`ip_tag_header <envoy_v3_api_field_extensions.filters.http.ip_tagging.v3.IPTagging.ip_tag_header>`.

adisuissa · 2024-11-04T19:15:28Z

api/envoy/extensions/filters/http/ip_tagging/v3/ip_tagging.proto

+    // :ref:`ip_tag_header <envoy_v3_api_field_extensions.filters.http.ip_tagging.v3.IPTagging.ip_tag_header>`,
+    // leaving original values in place.
+    //
+    // Please note that this could cause the header to retain values set by the http client even if the client is from an untrusted IP.


RE "untrusted IP", see comment above regarding the terms.

adisuissa · 2024-11-04T19:18:51Z

api/envoy/extensions/filters/http/ip_tagging/v3/ip_tagging.proto

+    // leaving original values in place.
+    //
+    // Please note that this could cause the header to retain values set by the http client even if the client is from an untrusted IP.
+    APPEND_FORWARD = 1;


What does "FORWARD" imply?

I'm assuming the goal is to have something similar to APPEND_IF_EXISTS_OR_ADD. Will it make sense to either use the same enum (probably not as the one suggested adds sanitization), or use the same enum name?

Signed-off-by: Ariane van der Steldt <[email protected]>

…hat we want. Signed-off-by: Ariane van der Steldt <[email protected]>

Signed-off-by: Ariane van der Steldt <[email protected]>

nahratzah · 2024-11-05T16:38:13Z

I applied the changes. <3

adisuissa

/lgtm api

Signed-off-by: Ariane van der Steldt <[email protected]>

Radha13 added 8 commits October 2, 2024 13:06

add the new header field in proto

49b77ae

Signed-off-by: Radha Kumari <[email protected]>

use the new header filed + make a getter

7febf6a

Signed-off-by: Radha Kumari <[email protected]>

use the new header if it's provided instead of x-envoy-ip-tags

0da4790

Signed-off-by: Radha Kumari <[email protected]>

fix format

131ee4d

Signed-off-by: Radha Kumari <[email protected]>

fix syntax here as well

68bb688

Signed-off-by: Radha Kumari <[email protected]>

add tests

e59ea3a

Signed-off-by: Radha Kumari <[email protected]>

change the name of header field to something more consistent with the

5ea7f0f

rest Signed-off-by: Radha Kumari <[email protected]>

fix format

ab02a10

Signed-off-by: Radha Kumari <[email protected]>

Radha13 requested a review from alyssawilk as a code owner October 3, 2024 15:06

repokitteh-read-only bot added the api label Oct 3, 2024

repokitteh-read-only bot assigned adisuissa Oct 3, 2024

Radha13 changed the title ~~ip-tagging filter: add support for an optional ip-tagging-header field~~ ip-tagging filter: add support for an optional ip-tag-header field Oct 3, 2024

adisuissa reviewed Oct 7, 2024

View reviewed changes

Radha13 and others added 2 commits October 8, 2024 11:32

Update test/extensions/filters/http/ip_tagging/ip_tagging_filter_test.cc

8da6c4f

Co-authored-by: Adi (Suissa) Peleg <[email protected]> Signed-off-by: Radha <[email protected]>

address review comments

eccb613

Signed-off-by: Radha Kumari <[email protected]>

adisuissa reviewed Oct 9, 2024

View reviewed changes

api/envoy/extensions/filters/http/ip_tagging/v3/ip_tagging.proto Outdated Show resolved Hide resolved

Update api/envoy/extensions/filters/http/ip_tagging/v3/ip_tagging.proto

8b21da5

Co-authored-by: Adi (Suissa) Peleg <[email protected]> Signed-off-by: Radha <[email protected]>

alyssawilk assigned JuniorHsu and alyssawilk Oct 9, 2024

add release notes

3711e4b

Signed-off-by: Radha Kumari <[email protected]>

make yaml lint test happy

5f62aea

Signed-off-by: Radha Kumari <[email protected]>

cqi1217 reviewed Oct 9, 2024

View reviewed changes

alyssawilk added the waiting label Oct 10, 2024

alyssawilk reviewed Nov 4, 2024

View reviewed changes

repokitteh-read-only bot added the waiting label Nov 4, 2024

Use camelcase for local variable.

c1cc54b

Signed-off-by: Ariane van der Steldt <[email protected]>

repokitteh-read-only bot removed the waiting label Nov 4, 2024

Ariane van der Steldt added 3 commits November 4, 2024 16:09

add multiple headers, to confirm they're all removed

8dc3bdd

Signed-off-by: Ariane van der Steldt <[email protected]>

Add "unless it has an x-envoy prefix"

649d54d

Signed-off-by: Ariane van der Steldt <[email protected]>

Function names are camel-case.

3033d27

Signed-off-by: Ariane van der Steldt <[email protected]>

alyssawilk previously approved these changes Nov 4, 2024

View reviewed changes

adisuissa reviewed Nov 4, 2024

View reviewed changes

Ariane van der Steldt added 2 commits November 5, 2024 13:28

Documentation fixes for proto, per review comments.

a9c92b7

Signed-off-by: Ariane van der Steldt <[email protected]>

rename append action to APPEND_IF_EXISTS_OR_ADD

159fcf0

Signed-off-by: Ariane van der Steldt <[email protected]>

alyssawilk added the waiting:any label Nov 5, 2024

Move header fields to its own message.

97b059c

Signed-off-by: Ariane van der Steldt <[email protected]>

nahratzah dismissed alyssawilk’s stale review via 97b059c November 5, 2024 14:09

repokitteh-read-only bot removed the waiting:any label Nov 5, 2024

Ariane van der Steldt added 4 commits November 5, 2024 14:15

Update docs with new references/names.

d6b807b

Signed-off-by: Ariane van der Steldt <[email protected]>

Apparently v3 protobuf all messages are optional? Anyway, this does w…

c611942

…hat we want. Signed-off-by: Ariane van der Steldt <[email protected]>

Reorder items in proto file.

99819b6

Signed-off-by: Ariane van der Steldt <[email protected]>

Fixed broken reference.

44f3d4e

Signed-off-by: Ariane van der Steldt <[email protected]>

adisuissa reviewed Nov 6, 2024

View reviewed changes

repokitteh-read-only bot removed the api label Nov 6, 2024

Merge branch 'main' into add-optional-header-ip-tagging-filter

7f9ccea

Signed-off-by: Ariane van der Steldt <[email protected]>

alyssawilk approved these changes Nov 6, 2024

View reviewed changes

alyssawilk merged commit 414ad34 into envoyproxy:main Nov 6, 2024
21 checks passed

Radha13 deleted the add-optional-header-ip-tagging-filter branch November 25, 2024 11:36

	// will be dropped, before the tags are applied. Sanitize will be run regardless of if the source is a trusted IP or not.
	// will be dropped, before the tags are applied. The incoming header will be "sanitized" regardless of whether the request is an internal or external.

		// :ref:`ip_tag_header <envoy_v3_api_field_extensions.filters.http.ip_tagging.v3.IPTagging.ip_tag_header>`,
		// leaving original values in place.

ip-tagging filter: add support for an optional ip-tag-header field #36434

ip-tagging filter: add support for an optional ip-tag-header field #36434

Conversation

Radha13 commented Oct 3, 2024 • edited Loading

Description

Why

Risk Level

Testing

repokitteh-read-only bot commented Oct 3, 2024

adisuissa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Radha13 commented Oct 8, 2024

adisuissa left a comment

Choose a reason for hiding this comment

alyssawilk commented Oct 9, 2024

JuniorHsu commented Oct 9, 2024

JuniorHsu commented Oct 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Radha13 commented Oct 9, 2024

adisuissa commented Oct 10, 2024

nahratzah commented Nov 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nahratzah commented Nov 4, 2024

alyssawilk left a comment

Choose a reason for hiding this comment

alyssawilk commented Nov 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nahratzah commented Nov 5, 2024

adisuissa left a comment

Choose a reason for hiding this comment

Radha13 commented Oct 3, 2024 •

edited

Loading