ext_authz: support modifying and removing query string parameters when using a gRPC authorization server by esmet · Pull Request #18009 · envoyproxy/envoy

esmet · 2021-09-06T22:25:34Z

This PR currently only implements query string modifications when using a gRPC authorization server.

Commit Message: ext_authz: support modifying and removing query string parameters when using a gRPC authorization server
Additional Description:
Risk Level: low, new opt-in feature to an extension
Testing: new unit tests
Docs Changes: Proto spec documented
Release Notes: ext_authz: added :ref:query_parameters_to_set <envoy_v3_api_field_service.auth.v3.CheckResponse.query_parameters_to_set> and :ref:query_parameters_to_remove <envoy_v3_api_field_service.auth.v3.CheckResponse.query_parameters_to_remove> for adding and removing query string parameters when using a gRPC authorization server.
Platform Specific Features:
Fixes #3266

Signed-off-by: John Esmet <john.esmet@gmail.com>

repokitteh-read-only · 2021-09-06T22:25:37Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #18009 was opened by esmet.

see: more, trace.

repokitteh-read-only · 2021-09-06T22:25:41Z

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

🐱

Caused by: #18009 was opened by esmet.

see: more, trace.

Signed-off-by: John Esmet <john.esmet@gmail.com>

esmet · 2021-09-07T14:52:04Z

api/envoy/service/auth/v3/external_auth.proto

  string body = 3;
 }

+// TODO: Should this be in the core API?


This is an open question. I feel like it could be valuable alongside HeaderValueOption.

Yes, that would be a good location, but I think the remove semantics are a bit weird. Would it be cleaner to structure this similar to headers, with query_parameters_to_add, query_parameter_to_remove?

I think I agree with @htuch, though with introducing a new structure the same as HeaderValue? But with the QueryParameter (?) as its name. We can do that via what is suggested (adding query_parameters_to_add, query_parameter_to_remove) by Harvey.

Signed-off-by: John Esmet <john.esmet@gmail.com>

esmet · 2021-09-09T14:40:26Z

source/extensions/filters/http/ext_authz/ext_authz.cc

+      const auto path_without_query =
+          Http::Utility::stripQueryString(request_headers_->Path()->value());
+      // TODO: These two lines should probably be abstracted as
+      // Http::Utility::formatPathAndQueryParams


I still need to do this.

esmet · 2021-09-09T14:41:05Z

source/extensions/filters/http/ext_authz/ext_authz.cc

+      for (const auto& [key, value] : response->query_parameters_to_set) {
+        ENVOY_STREAM_LOG(trace, "'{}={}'", *decoder_callbacks_, key, value);
+        // TODO(esmet): Sanitize key/value and/or declare the security posture that we trust the
+        // authorization server.


Trusting the auth server seems obvious but I still need to circle back to this TODO

File an issue and link it here will be better I think?

Signed-off-by: John Esmet <john.esmet@gmail.com>

htuch · 2021-09-10T14:32:57Z

/assign-from @envoyproxy/first-pass-reviewers

repokitteh-read-only · 2021-09-10T14:33:00Z

@envoyproxy/first-pass-reviewers assignee is @jmarantz

🐱

Caused by: a #18009 (comment) was created by @htuch.

see: more, trace.

source/extensions/filters/common/ext_authz/ext_authz.h

dio · 2021-09-13T07:05:09Z

source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc

+          // TODO(esmet): It might make more sense to store query_parameters_to_set as a vector
+          // instead of a map since we will likely only ever iterate them linearly.


I agree with this, should we go ahead with this PR?

I'll work on adding QueryParamsVector for this.

dio · 2021-09-13T07:09:01Z

source/extensions/filters/http/ext_authz/ext_authz.cc

  switch (response->status) {
  case CheckStatus::OK: {
-    // Any changes to request headers can affect how the request is going to be
+    // Any changes to request headers or query parameters can affect how the request is going to be


Thanks for this. I totally forgot that we have a route matcher to match query params.

envoy/api/envoy/config/route/v3/route_components.proto

Lines 1995 to 1998 in 7d3057f

// [#next-free-field: 7]

message QueryParameterMatcher {

option (udpa.annotations.versioning).previous_message_type =

"envoy.api.v2.route.QueryParameterMatcher";

.

Signed-off-by: John Esmet <john.esmet@gmail.com>

jmarantz · 2021-09-15T12:13:41Z

Looks like dio is on this; I'lll drop off this one.

Signed-off-by: John Esmet <john.esmet@gmail.com>

esmet · 2021-09-29T15:36:15Z

@rojkov thanks! I ended up needing to specify envoy_v3_field_service. which I think means "a field type" in the "service." namespace, since the qualified name is service.auth.v3... etc

api/envoy/service/auth/v3/external_auth.proto

Signed-off-by: John Esmet <john.esmet@gmail.com>

htuch · 2021-09-30T20:15:35Z

/lgtm api

Signed-off-by: John Esmet <john.esmet@gmail.com>

mattklein123 · 2021-10-06T16:37:38Z

Needs a main merge. @rojkov can you do a final pass on this please?

/wait

Signed-off-by: John Esmet <john.esmet@gmail.com>

rojkov · 2021-10-07T07:36:14Z

Sorry, needs a merge again.

/wait

Signed-off-by: John Esmet <john.esmet@gmail.com>

esmet · 2021-10-07T16:48:57Z

@rojkov done!

rojkov

Thanks! Looks perfect to me.

@htuch could you please reinstate your API approval?

rojkov · 2021-10-08T07:33:20Z

test/common/http/utility_test.cc

+  EXPECT_EQ(Utility::stripQueryString(HeaderString("/?x=1")), "/");
+  EXPECT_EQ(Utility::stripQueryString(HeaderString("/foo")), "/foo");
+  EXPECT_EQ(Utility::stripQueryString(HeaderString("/foo?")), "/foo");
+  EXPECT_EQ(Utility::stripQueryString(HeaderString("/foo?hello=there")), "/foo");
+  EXPECT_EQ(Utility::stripQueryString(HeaderString("/foo/?")), "/foo/");


Nit: I know it's too pedantic, but I feel incompleteness without having these two lines added

EXPECT_EQ(Utility::stripQueryString(HeaderString("/?x=1&y=2")), "/"); EXPECT_EQ(Utility::stripQueryString(HeaderString("/foo?hello=there&good=bye")), "/foo");

that's fair - will add them

Signed-off-by: John Esmet <john.esmet@gmail.com>

rojkov

LGTM!

htuch · 2021-10-08T21:41:16Z

/lgtm api

esmet added 3 commits September 6, 2021 22:13

Add query_parameters_to_add

12230f0

Signed-off-by: John Esmet <john.esmet@gmail.com>

Fix format

a934913

Signed-off-by: John Esmet <john.esmet@gmail.com>

Fix format

cad93ec

Signed-off-by: John Esmet <john.esmet@gmail.com>

repokitteh-read-only bot added the api label Sep 6, 2021

repokitteh-read-only bot assigned htuch Sep 6, 2021

Clean up some things

426a467

Signed-off-by: John Esmet <john.esmet@gmail.com>

esmet commented Sep 7, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/main' into ext-authz-query-string

25b2eeb

Signed-off-by: John Esmet <john.esmet@gmail.com>

esmet commented Sep 9, 2021

View reviewed changes

esmet added 2 commits September 9, 2021 16:41

Add a test for the grpc implementation.

5c9c01e

Signed-off-by: John Esmet <john.esmet@gmail.com>

Formatting

06a6338

Signed-off-by: John Esmet <john.esmet@gmail.com>

esmet changed the title ~~ext_authz: support modifying and removing query string parameters~~ ext_authz: support modifying and removing query string parameters when using a gRPC authorization server Sep 10, 2021

esmet marked this pull request as ready for review September 10, 2021 01:04

esmet requested a review from dio as a code owner September 10, 2021 01:04

esmet added 2 commits September 10, 2021 01:04

Add release notes

b9fc09e

Signed-off-by: John Esmet <john.esmet@gmail.com>

Fix labels in release notes

251655e

Signed-off-by: John Esmet <john.esmet@gmail.com>

repokitteh-read-only bot assigned jmarantz Sep 10, 2021

htuch assigned dio Sep 10, 2021

dio reviewed Sep 13, 2021

View reviewed changes

source/extensions/filters/common/ext_authz/ext_authz.h Outdated Show resolved Hide resolved

dio reviewed Sep 13, 2021

View reviewed changes

esmet added 3 commits September 13, 2021 15:19

Try fixing labels again

7686b3c

Signed-off-by: John Esmet <john.esmet@gmail.com>

Fix one more time

9edf4c0

Signed-off-by: John Esmet <john.esmet@gmail.com>

Merge remote-tracking branch 'upstream/main' into ext-authz-query-string

591f75a

Signed-off-by: John Esmet <john.esmet@gmail.com>

Merge remote-tracking branch 'upstream/main' into ext-authz-query-string

181dcc1

Signed-off-by: John Esmet <john.esmet@gmail.com>

htuch reviewed Sep 29, 2021

View reviewed changes

api/envoy/service/auth/v3/external_auth.proto Show resolved Hide resolved

Merge remote-tracking branch 'upstream/main' into ext-authz-query-string

4474842

Signed-off-by: John Esmet <john.esmet@gmail.com>

repokitteh-read-only bot removed the api label Sep 30, 2021

Merge remote-tracking branch 'upstream/main' into ext-authz-query-string

ddcd3aa

Signed-off-by: John Esmet <john.esmet@gmail.com>

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

esmet added 2 commits October 6, 2021 15:38

Fix initialization bugs

a2674e3

Signed-off-by: John Esmet <john.esmet@gmail.com>

Merge remote-tracking branch 'upstream/main' into ext-authz-query-string

020918e

Signed-off-by: John Esmet <john.esmet@gmail.com>

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

esmet added 3 commits October 6, 2021 19:52

Fix bad merge on the changelog

19383d6

Signed-off-by: John Esmet <john.esmet@gmail.com>

Fix include in query_params.h

7b52929

Signed-off-by: John Esmet <john.esmet@gmail.com>

Fix order

8837536

Signed-off-by: John Esmet <john.esmet@gmail.com>

rojkov self-assigned this Oct 7, 2021

repokitteh-read-only bot added the waiting label Oct 7, 2021

Merge remote-tracking branch 'upstream/main' into ext-authz-query-string

5fe5ee3

Signed-off-by: John Esmet <john.esmet@gmail.com>

repokitteh-read-only bot removed the waiting label Oct 7, 2021

rojkov previously approved these changes Oct 8, 2021

View reviewed changes

More test cases

a9d7f5a

Signed-off-by: John Esmet <john.esmet@gmail.com>

esmet dismissed rojkov’s stale review via a9d7f5a October 8, 2021 14:25

rojkov approved these changes Oct 8, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Oct 8, 2021

rojkov merged commit c42a018 into envoyproxy:main Oct 11, 2021

esmet deleted the ext-authz-query-string branch October 11, 2021 13:44

		// TODO(esmet): It might make more sense to store query_parameters_to_set as a vector
		// instead of a map since we will likely only ever iterate them linearly.

	// [#next-free-field: 7]
	message QueryParameterMatcher {
	option (udpa.annotations.versioning).previous_message_type =
	"envoy.api.v2.route.QueryParameterMatcher";

Conversation

esmet commented Sep 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Sep 6, 2021

Uh oh!

repokitteh-read-only bot commented Sep 6, 2021

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!

dio Sep 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch commented Sep 10, 2021

Uh oh!

repokitteh-read-only bot commented Sep 10, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dio Sep 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarantz commented Sep 15, 2021

Uh oh!

esmet commented Sep 29, 2021

Uh oh!

Uh oh!

htuch commented Sep 30, 2021

Uh oh!

mattklein123 commented Oct 6, 2021

Uh oh!

rojkov commented Oct 7, 2021

Uh oh!

esmet commented Oct 7, 2021

Uh oh!

rojkov 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!

rojkov left a comment

Choose a reason for hiding this comment

Uh oh!

htuch commented Oct 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

esmet commented Sep 6, 2021 •

edited

Loading

dio Sep 13, 2021 •

edited

Loading

dio Sep 13, 2021 •

edited

Loading