ext_authz: support response headers on OK authorization checks #14514

esmet · 2020-12-23T18:55:41Z

Support adding response headers on OK authorization checks from ext_authz

Commit Message: ext_authz: support response headers on OK authorization checks
Additional Description:
Risk Level: low (opt-in feature, does nothing by default)
Testing: Added code to existing unit tests
Docs Changes: API protos documented
Release Notes: ext_authz: added :ref:response_headers_to_add <envoy_v3_api_field_service.auth.v3.OkHttpResponse.response_headers_to_add> to support sending response headers to downstream clients on OK external authorization checks.
Platform Specific Features:
Fixes #7986

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

repokitteh-read-only · 2020-12-23T18:55:48Z

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

🐱

Caused by: #14514 was opened by esmet.

see: more, trace.

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

esmet · 2021-01-06T02:30:09Z

I'm pulling this out of draft to start getting feedback. I still need to take a pass at the docs and see if/how we can document this feature.

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

markdroth · 2021-01-06T15:46:33Z

/lgtm api

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

esmet · 2021-01-11T20:49:21Z

@dio CI is green and this is now ready for your critical review, if you don't mind :)

dio

Overall LGTM. Some notes and requesting integration test if possible.

source/extensions/filters/http/ext_authz/ext_authz.h

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

dio · 2021-01-20T03:47:09Z

docs/root/version_history/current.rst

@@ -70,6 +70,7 @@ New Features
 * config: added ability to flush stats when the admin's :ref:`/stats endpoint <operations_admin_interface_stats>` is hit instead of on a timer via :ref:`stats_flush_on_admin <envoy_v3_api_field_config.bootstrap.v3.Bootstrap.stats_flush_on_admin>`.
 * config: added new runtime feature `envoy.features.enable_all_deprecated_features` that allows the use of all deprecated features.
 * crash support: added the ability to dump L4 connection data on crash.
+* ext_authz: added :ref:`response_headers_to_add <envoy_v3_api_field_service.auth.v3.OkHttpResponse.response_headers_to_add>` to support sending response headers to downstream clients on OK external authorization checks.


While it is in the same spirit, seems like we have two changes here, for gRPC and HTTP implementation. I'm OK to have this in the same PR. For completeness, we need integration tests, since it involves response from the "external service". WDYT?

I'll add another changelog line to capture both the HTTP and gRPC service changes.

I'll look into what it'll take to add an integration test 👍

I took a look and there seems to be an established precedent that other similar features (e.g. allowed_client_headers for HTTP, request_headers_to_add for gRPC, and others) are tested using unit tests and appropriate mocks. I think that if unit tests are OK for those features, they should be good for this one as well. Do you agree?

Having said that, I don't think there are any integration tests for ext_authz (or if there are, I can't find them) so maybe I should look into following up with some work to add those tests down the road. Maybe I'll look at the coverage report to decide if that's worth it.

I think we need to have an integration test to make sure that, for gRPC implementation, when we do sendExtAuthzResponse (this is done by the authorization service) with this response_headers_to_add set, the downstream client receives it.

The same applies to the HTTP one, waitForExtAuthzRequest should provide response header entries that are allowed to be forwarded, and are inserted to response_->headers().

Ok I'll give that a try

I now see the integration test for ext_authz - don't know how I missed that before.

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

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

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

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

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

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

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

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

esmet · 2021-02-08T17:56:06Z

Overall LGTM. Some notes and requesting integration test if possible.

I added an integration test. Please take a look and let me know what you think.

dio

Awesome. Thank you!

dio · 2021-02-09T03:18:18Z

test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc

@@ -626,6 +640,38 @@ TEST_P(ExtAuthzGrpcIntegrationTest, DenyAtDisableWithMetadata) {
  expectFilterDisableCheck(/*deny_at_disable=*/true, /*disable_with_metadata=*/true, "403");
 }

+TEST_P(ExtAuthzGrpcIntegrationTest, DownstreamHeadersOnSuccess) {
+  XDS_DEPRECATED_FEATURE_TEST_SKIP;
+  // Set up ext_authz filter


Super nit: Some comment lines missing "." at the end.

dio · 2021-02-09T03:20:24Z

test/extensions/filters/common/ext_authz/ext_authz_http_impl_test.cc

+  const auto expected_headers = TestCommon::makeHeaderValueOption(
+      {{":status", "200", false}, {"x-downstream-ok", "1", false}, {"x-upstream-ok", "1", false}});
+  const auto authz_response = TestCommon::makeAuthzResponse(
+      CheckStatus::OK, Http::Code::OK, "", TestCommon::makeHeaderValueOption({}),


Nit. EMPTY_STRING

dio · 2021-02-09T03:22:44Z

Asking for @envoyproxy/senior-maintainers review. Probably, @lizan or @htuch. Thank you!

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

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

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

esmet · 2021-02-10T00:08:49Z

/retest

repokitteh-read-only · 2021-02-10T00:08:52Z

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

🐱

Caused by: a #14514 (comment) was created by @esmet.

see: more, trace.

esmet · 2021-02-11T15:08:55Z

@lizan / @htuch friendly poke. CI is 💚 - let me know if you think this is in an OK state to land 👍

…proxy#14514) Support adding response headers on OK authorization checks from ext_authz Commit Message: ext_authz: support response headers on OK authorization checks Additional Description: Risk Level: low (opt-in feature, does nothing by default) Testing: Added code to existing unit tests Docs Changes: API protos documented Release Notes: ext_authz: added :ref:`response_headers_to_add <envoy_v3_api_field_service.auth.v3.OkHttpResponse.response_headers_to_add>` to support sending response headers to downstream clients on OK external authorization checks. Platform Specific Features: Fixes envoyproxy#7986 Signed-off-by: John Esmet <john.esmet@gmail.com>

ext_authz: support response headers on OK authorization check

04f0998

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

repokitteh-read-only bot added the api label Dec 23, 2020

repokitteh-read-only bot assigned markdroth Dec 23, 2020

esmet added 5 commits January 5, 2021 04:10

Fix a test, add changelog entry

41e71d4

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

Add response header encoding, write a test, make it pass.

83e2299

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

Simplify by using a PassThroughEncoderFilter

47aa43e

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

Revert PassThroughEncoderFilter and increase test coverage instead

2d81472

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

Add coverage for allowed_client_headers_on_success

b5d31d3

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

esmet marked this pull request as ready for review January 6, 2021 02:30

esmet requested a review from dio as a code owner January 6, 2021 02:30

Kick CI

8ffbf7d

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

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

esmet added 2 commits January 6, 2021 18:05

Merge branch 'master' into ext-authz-headers-on-success

4880f64

Merge remote-tracking branch 'upstream/master' into ext-authz-headers…

aafe6a3

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

Base automatically changed from master to main January 15, 2021 23:02

esmet mentioned this pull request Jan 20, 2021

Question: set-cookie on client response in successful ExtAuthz response #7986

Closed

dio suggested changes Jan 20, 2021

View reviewed changes

esmet added 5 commits January 26, 2021 23:22

Merge remote-tracking branch 'upstream/main' into ext-authz-headers-o…

2773a24

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

Address CR comments

ff70736

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

Move headers instead of copying into a HeaderVector on the heap

65ce2bf

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

Fix changelog

178bea9

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

Fix proto generation

fa107b4

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

repokitteh-read-only bot added the api label Jan 27, 2021

esmet mentioned this pull request Jan 27, 2021

ext_authz: add dynamic_metadata_matchers and use them in the ext_authz HTTP implementation #14843

Closed

esmet requested a review from dio January 27, 2021 23:16

Update api proto comments to link back to HeaderValueOption

9f106c0

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

esmet added 6 commits January 28, 2021 21:27

Merge remote-tracking branch 'upstream/main' into ext-authz-headers-o…

d94054e

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

Fix ext_authz config to support the encoding path, and update a test.

fa27e7f

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

Merge remote-tracking branch 'origin/main' into ext-authz-headers-on-…

e8fc587

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

Merge remote-tracking branch 'upstream/main' into ext-authz-headers-o…

b374b12

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

Merge remote-tracking branch 'origin/main' into ext-authz-headers-on-…

17dcba7

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

Fix ext_authz config test

5175cec

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

dio self-assigned this Feb 7, 2021

Fix test

362e4a4

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

dio previously approved these changes Feb 9, 2021

View reviewed changes

esmet added 2 commits February 9, 2021 15:49

Fix CR comments

3052d02

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

Merge remote-tracking branch 'upstream/main' into ext-authz-headers-o…

39eac44

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

esmet dismissed dio’s stale review via 39eac44 February 9, 2021 15:56

Fix syntax

d826348

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

esmet mentioned this pull request Feb 11, 2021

ext_authz: How to add authorization response headers to downstream response? #14998

Closed

lizan approved these changes Feb 11, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Feb 11, 2021

lizan merged commit ac9a263 into envoyproxy:main Feb 11, 2021

esmet deleted the ext-authz-headers-on-success branch February 24, 2021 21:04

esmet mentioned this pull request Mar 29, 2021

Ambassador Megapatch on release/v1.17 datawire/envoy#20

Closed

fabiosimeoni mentioned this pull request Sep 17, 2021

Relay Set-Cookie headers to clients when AuthService returns them on a 200 response. emissary-ingress/emissary#3809

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ext_authz: support response headers on OK authorization checks #14514

ext_authz: support response headers on OK authorization checks #14514

esmet commented Dec 23, 2020 •

edited

Loading

repokitteh-read-only bot commented Dec 23, 2020

esmet commented Jan 6, 2021

markdroth commented Jan 6, 2021

esmet commented Jan 11, 2021

dio left a comment

dio Jan 20, 2021 •

edited

Loading

esmet Jan 20, 2021

esmet Jan 27, 2021

dio Jan 28, 2021 •

edited

Loading

esmet Jan 28, 2021

esmet Jan 28, 2021

esmet commented Feb 8, 2021

dio left a comment

dio Feb 9, 2021

dio Feb 9, 2021

dio commented Feb 9, 2021

esmet commented Feb 10, 2021

repokitteh-read-only bot commented Feb 10, 2021

esmet commented Feb 11, 2021

ext_authz: support response headers on OK authorization checks #14514

ext_authz: support response headers on OK authorization checks #14514

Conversation

esmet commented Dec 23, 2020 • edited Loading

repokitteh-read-only bot commented Dec 23, 2020

esmet commented Jan 6, 2021

markdroth commented Jan 6, 2021

esmet commented Jan 11, 2021

dio left a comment

Choose a reason for hiding this comment

dio Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

esmet Jan 20, 2021

Choose a reason for hiding this comment

esmet Jan 27, 2021

Choose a reason for hiding this comment

dio Jan 28, 2021 • edited Loading

Choose a reason for hiding this comment

esmet Jan 28, 2021

Choose a reason for hiding this comment

esmet Jan 28, 2021

Choose a reason for hiding this comment

esmet commented Feb 8, 2021

dio left a comment

Choose a reason for hiding this comment

dio Feb 9, 2021

Choose a reason for hiding this comment

dio Feb 9, 2021

Choose a reason for hiding this comment

dio commented Feb 9, 2021

esmet commented Feb 10, 2021

repokitteh-read-only bot commented Feb 10, 2021

esmet commented Feb 11, 2021

esmet commented Dec 23, 2020 •

edited

Loading

dio Jan 20, 2021 •

edited

Loading

dio Jan 28, 2021 •

edited

Loading