Matching engine: add a cookie matching input object (revival) #21496

aguinet · 2022-05-29T12:10:52Z

(Revival of #16344)

Commit Message: Matching: add HttpRequestCookieMatchInput data input
Additional Description:

This allows matching data extracted from cookies. It also adds a new parseCookieValues API in Http::Utility, which allows to return the potential multiple values that could be set in malicious HTTP requests, like in this case:

Cookie: mykey=a;mykey=b

To be in pair with what is already done with the HTTP header input matcher, this input matcher returns, in this case, a,b.

I've done only the request side, but if these patches are fine for you, I can also add the response side, and factorize the code in a similar way that what's done for headers!

Risk Level: Low
Testing: adapting current unit tests
Docs Changes: New API in Http::Testing documented
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

Create a parseCookieValues API that returns all the values that are potentially set for a cookie key. This could be done in malicious requests that try to bypass some filters. Moreover, modify parseCookieValue's API to use string_view objects. Indeed, the data from the "Cookies" headers is never transformed, only a view of it is returned. Signed-off-by: Adrien Guinet <adrien@reblaze.com>

This allows matching data extracted from cookies. Signed-off-by: Adrien Guinet <adrien@reblaze.com>

TODO: split this commit in two and squash it into the first two ones Signed-off-by: Adrien Guinet <adrien@reblaze.com>

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

Thanks clang-tidy! Signed-off-by: Adrien Guinet <adrien@reblaze.com>

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

snowp

Overall this seems good to me, main question I have is around the common code interface changes

snowp · 2022-05-31T03:52:13Z

source/common/http/utility.cc

+                    // max_vals == 0 => infinity, so the condition above will never be true
+                    // in this case.
+                    if (ret.size() == max_vals) {


If we default to std::numeric_limits::max instead of treating 0 as special I think this logic would seem a bit cleaner

Done in b7f3479

snowp · 2022-05-31T03:57:12Z

source/common/http/utility.h

@@ -300,9 +301,25 @@ absl::flat_hash_map<std::string, std::string> parseCookies(const RequestHeaderMa
 * Parse a particular value out of a set-cookie
 * @param headers supplies the headers to get the set-cookie from.
 * @param key the key for the particular set-cookie value to return
- * @return std::string the parsed set-cookie value, or "" if none exists
+ * @return absl::string_view the parsed set-cookie value, or "" if none exists


Here and below please document the lifetime of the returned string views

More generally I'd see if there are any concerns about this interface switch, given that a std::string ends up being a bit safer than a string_view, cc @alyssawilk @mattklein123

I'm OK with it, though I agree it makes the function more prone to error.

Maybe the safest option for now is indeed to keep std::string. I will revert back to that API.

Done in b7f3479

* cleaner API for the max_vals argument of parseCookieValues * get back to using std::string for the return value(s) of the parseCookie* functions Signed-off-by: Adrien Guinet <adrien@reblaze.com>

repokitteh-read-only · 2022-06-07T05:28:03Z

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

🐱

Caused by: #21496 was synchronize by aguinet.

see: more, trace.

mattklein123

API LGTM with small comment, thanks.

/wait

mattklein123 · 2022-06-07T15:40:20Z

api/envoy/type/matcher/v3/http_inputs.proto

@@ -60,3 +60,17 @@ message HttpResponseTrailerMatchInput {
  string header_name = 1
      [(validate.rules).string = {well_known_regex: HTTP_HEADER_NAME strict: false}];
 }
+
+// Match input indicates that matching should be done on a specific cookie key.


Please add a release note for this new feature. @kyessenov what do we need to do to get this matcher to show up in the docs? I forget where we landed on this in terms of what is an extension category, etc.

Add the new extension to the list of extensions in extension_manifest.yaml. Tag it as implementing the HTTP matcher category.

Add a comment to the proto as in // [#extension: envoy.matching.inputs.cookies]

Reference the extension here: https://github.com/envoyproxy/envoy/blob/main/docs/root/intro/arch_overview/advanced/matching/matching_api.rst#http-input-functions.

I'm working to get (1)-(2) validated once we merge #21413.

Thanks @kyessenov for this! Done in 3930b3a

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

zuriby · 2022-07-01T06:56:48Z

@aguinet can you take a quick look what else is missing to get it in?

github-actions · 2022-07-31T08:01:12Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

github-actions · 2022-08-07T08:01:17Z

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

aguinet added 19 commits May 5, 2021 22:12

Matching: add HttpRequestCookieMatchInput data input

16013f0

This allows matching data extracted from cookies. Signed-off-by: Adrien Guinet <adrien@reblaze.com>

Explicit the order of the cookie headers parsing

2891366

TODO: split this commit in two and squash it into the first two ones Signed-off-by: Adrien Guinet <adrien@reblaze.com>

parseCookieValues: add more "expected to fail" test

5406686

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

Merge branch 'main' into feature/matching_cookies

fa95e30

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

fix parseCookie API

c742db2

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

Adapt input data interface after merging with main

02ba70e

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

Fix test after merge

b92ad31

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

Runtime guard to get old parseCookieValue behavior

828e6a1

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

fix oauth integration test

e74bfc9

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

Add a release note

a2bdc88

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

Merge branch 'main' into feature/matching_cookies

dd00ed2

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

fixup! Add a release note

2b3a9be

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

Adapt to new matching interface

389a195

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

fixup! Merge branch 'main' into feature/matching_cookies

70e0d6e

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

use vec.empty() instead of vec.size() == 0

032d555

Thanks clang-tidy! Signed-off-by: Adrien Guinet <adrien@reblaze.com>

Merge branch 'main' into feature/matching_cookies

7342288

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

Merge branch 'main' into feature/matching_cookies

84d28cb

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

Adapt to new APIs & remove runtime guard

9f47412

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

aguinet requested review from wbpcode and zuercher as code owners May 29, 2022 12:10

aguinet mentioned this pull request May 29, 2022

Matching engine: add a cookie matching input object #16344

Closed

htuch assigned snowp May 31, 2022

snowp suggested changes May 31, 2022

View reviewed changes

KBaichoo added the waiting label Jun 3, 2022

repokitteh-read-only bot added the api label Jun 6, 2022

snowp review

b7f3479

* cleaner API for the max_vals argument of parseCookieValues * get back to using std::string for the return value(s) of the parseCookie* functions Signed-off-by: Adrien Guinet <adrien@reblaze.com>

repokitteh-read-only bot removed the waiting label Jun 7, 2022

repokitteh-read-only bot assigned mattklein123 Jun 7, 2022

mattklein123 requested changes Jun 7, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 7, 2022

aguinet added 2 commits June 27, 2022 20:33

Merge branch 'main' into feature/matching_cookies

0ebfb1d

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

Matching extension documentation

3930b3a

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

repokitteh-read-only bot removed the waiting label Jun 27, 2022

extension_metadata: fix cookie extension type

fc09315

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

mattklein123 added the waiting label Jun 28, 2022

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 31, 2022

github-actions bot closed this Aug 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Matching engine: add a cookie matching input object (revival) #21496

Matching engine: add a cookie matching input object (revival) #21496

aguinet commented May 29, 2022

snowp left a comment

snowp May 31, 2022

aguinet Jun 7, 2022

snowp May 31, 2022

mattklein123 Jun 6, 2022

aguinet Jun 7, 2022

aguinet Jun 7, 2022

repokitteh-read-only bot commented Jun 7, 2022

mattklein123 left a comment

mattklein123 Jun 7, 2022

kyessenov Jun 7, 2022

aguinet Jun 27, 2022

zuriby commented Jul 1, 2022

github-actions bot commented Jul 31, 2022

github-actions bot commented Aug 7, 2022

Matching engine: add a cookie matching input object (revival) #21496

Matching engine: add a cookie matching input object (revival) #21496

Conversation

aguinet commented May 29, 2022

snowp 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

repokitteh-read-only bot commented Jun 7, 2022

mattklein123 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

zuriby commented Jul 1, 2022

github-actions bot commented Jul 31, 2022

github-actions bot commented Aug 7, 2022