ext_authz: do the authentication even the direct response is set #17546

soulxu · 2021-07-30T09:51:59Z

Commit Message: ext_authz: do the authentication even the direct response is set
Additional Description:
This PR fixed the case the authentication will be skipped when the direct response is set.
And add the runtime flag envoy.reloadable_features.http_ext_authz_do_not_skip_direct_response to enable the
revert of this behavior.

Also fix the merge config behavior, ensure the virtual host's typed config will be merged when
the direct response is set.

Risk Level: low
Testing: unittest
Docs Changes: n/a
Release Notes: bug fixes note
Runtime guard: envoy.reloadable_features.http_ext_authz_do_not_skip_direct_response
Fixes #17502
Related to #17377

Signed-off-by: He Jie Xu hejie.xu@intel.com

soulxu · 2021-07-30T09:52:37Z

/wait

waiting for #17449 merged

dmitri-d · 2021-07-30T17:59:54Z

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

@@ -374,7 +374,10 @@ void Filter::continueDecoding() {
 }

 Filter::PerRouteFlags Filter::getPerRouteFlags(const Router::RouteConstSharedPtr& route) const {
-  if (route == nullptr || route->routeEntry() == nullptr) {
+  if (route == nullptr ||
+      (!Runtime::runtimeFeatureEnabled(


Wouldn't this affect redirects too? Not sure if these be passed to ext_authz?

thanks for the good point, I tested the redirect case, this change affect it.

@mattklein123 WDYT?

Yeah I think it's correct to fix redirect also. You might want to rename the runtime flag and update the release notes?

/wait

right, let me update

mattklein123

Thanks LGTM with a small comment. Also needs a main merge.

If you are able to test a redirect case that would be great also.

/wait

docs/root/version_history/current.rst

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

Co-authored-by: Matt Klein <mattklein123@gmail.com> Signed-off-by: He Jie Xu <hejie.xu@intel.com> Signed-off-by: xuhj <hejie.xu@intel.com>

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu · 2021-08-08T09:00:52Z

Thanks LGTM with a small comment. Also needs a main merge.

If you are able to test a redirect case that would be great also.

/wait

I added integration test for redirect and direct response case.

The unittest cover those two cases since both of them return empty routeEntry
https://github.com/envoyproxy/envoy/pull/17546/files#diff-64a51bc8f4c7ce4f31a3420225e6fd4d0f14a187424148da4b9dc87e11d97c96L1607

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

mattklein123

Awesome, thanks. Sorry just one more release note nit.

/wait

mattklein123 · 2021-08-09T15:24:59Z

docs/root/version_history/current.rst

@@ -42,6 +42,7 @@ Bug Fixes
 * access log: fix ``%UPSTREAM_CLUSTER%`` when used in http upstream access logs. Previously, it was always logging as an unset value.
 * aws request signer: fix the AWS Request Signer extension to correctly normalize the path and query string to be signed according to AWS' guidelines, so that the hash on the server side matches. See `AWS SigV4 documentaion <https://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html>`_.
 * cluster: delete pools when they're idle to fix unbounded memory use when using PROXY protocol upstream with tcp_proxy. This behavior can be temporarily reverted by setting the ``envoy.reloadable_features.conn_pool_delete_when_idle`` runtime guard to false.
+* ext_authz: fixed skipping authentication when using returning either a direct response or a redirect. This behavior can be temporarily reverted by setting the ``envoy.reloadable_features.http_ext_authz_do_not_skip_direct_response_and_redirect`` runtime guard to false.


Suggested change

* ext_authz: fixed skipping authentication when using returning either a direct response or a redirect. This behavior can be temporarily reverted by setting the ``envoy.reloadable_features.http_ext_authz_do_not_skip_direct_response_and_redirect`` runtime guard to false.

* ext_authz: fixed skipping authentication when returning either a direct response or a redirect. This behavior can be temporarily reverted by setting the ``envoy.reloadable_features.http_ext_authz_do_not_skip_direct_response_and_redirect`` runtime guard to false.

thanks, done

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

mattklein123 · 2021-08-09T23:04:27Z

Sorry I think you need another main merge. :(

/wait

soulxu · 2021-08-10T01:05:14Z

Sorry I think you need another main merge. :(

/wait

haha, no worries, let me do a main merge

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

ggreenway · 2021-08-10T17:16:17Z

docs/root/version_history/current.rst

@@ -42,6 +42,7 @@ Bug Fixes
 * access log: fix ``%UPSTREAM_CLUSTER%`` when used in http upstream access logs. Previously, it was always logging as an unset value.
 * aws request signer: fix the AWS Request Signer extension to correctly normalize the path and query string to be signed according to AWS' guidelines, so that the hash on the server side matches. See `AWS SigV4 documentaion <https://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html>`_.
 * cluster: delete pools when they're idle to fix unbounded memory use when using PROXY protocol upstream with tcp_proxy. This behavior can be temporarily reverted by setting the ``envoy.reloadable_features.conn_pool_delete_when_idle`` runtime guard to false.
+* ext_authz: fixed skipping authentication when returning either a direct response or a redirect. This behavior can be temporarily reverted by setting the ``envoy.reloadable_features.http_ext_authz_do_not_skip_direct_response_and_redirect`` runtime guard to false.


I think this should also be mentioned in "incompatible behavior changes". This behavior has been present for at least a few years, and I know some people are relying on the old behavior.

that make sense, I can move it.

* 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>

…oyproxy#17546) Signed-off-by: He Jie Xu <hejie.xu@intel.com>

repokitteh-read-only bot added the waiting label Jul 30, 2021

mattklein123 self-assigned this Jul 30, 2021

dmitri-d reviewed Jul 30, 2021

View reviewed changes

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

mattklein123 requested changes Aug 6, 2021

View reviewed changes

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

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

soulxu added 6 commits August 8, 2021 08:56

Continue the auth when the routeEntry is empty

2a46240

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

Travel virtualhost typed filter config when direct response is set

ab03064

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

fix release note

627a3f8

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

rename the runtime flag

1f66fd6

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

Update docs/root/version_history/current.rst

6ea58a0

Co-authored-by: Matt Klein <mattklein123@gmail.com> Signed-off-by: He Jie Xu <hejie.xu@intel.com> Signed-off-by: xuhj <hejie.xu@intel.com>

Add integration test for redirect and direct response

a614861

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu force-pushed the fix_ext_authz branch from f77eec7 to a614861 Compare August 8, 2021 08:57

fix release note

372eaa9

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

fix the order of release note

571b269

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

mattklein123 requested changes Aug 9, 2021

View reviewed changes

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

fix release note

f5645a2

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

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

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

Merge branch 'main' into fix_ext_authz

92f0cf8

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

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

mattklein123 approved these changes Aug 10, 2021

View reviewed changes

mattklein123 merged commit 88705d7 into envoyproxy:main Aug 10, 2021

ggreenway reviewed Aug 10, 2021

View reviewed changes

soulxu mentioned this pull request Aug 10, 2021

move ext_authz release note to incompatible behavior changes section #17667

Merged

mattklein123 mentioned this pull request Aug 11, 2021

[route] Create route-table level typed_per_filter_config for "global" route config. #17662

Closed

soulxu mentioned this pull request Aug 14, 2021

ext_authz: support external authorization checks when a route contains a direct response entry #14989

Closed

soulxu mentioned this pull request Aug 17, 2021

Direct response/Redirect with ext_authz #14984

Closed

leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021

ext_authz: do the authentication even the direct response is set (env…

9198325

…oyproxy#17546) Signed-off-by: He Jie Xu <hejie.xu@intel.com>

djannot mentioned this pull request Jan 5, 2022

Allow Direct Response to happen after extauth and rate limiting kgateway-dev/kgateway#5761

Closed

mattklein123 mentioned this pull request Apr 25, 2022

envoy_reloadable_features_http_ext_authz_do_not_skip_direct_response_and_redirect deprecation #20980

Closed

LanceEa mentioned this pull request Oct 13, 2022

https redirect doesn't occur when AuthService applied emissary-ingress/emissary#4620

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ext_authz: do the authentication even the direct response is set #17546

ext_authz: do the authentication even the direct response is set #17546

soulxu commented Jul 30, 2021

soulxu commented Jul 30, 2021

dmitri-d Jul 30, 2021

soulxu Aug 5, 2021 •

edited

Loading

mattklein123 Aug 5, 2021

soulxu Aug 5, 2021

mattklein123 left a comment

soulxu commented Aug 8, 2021

mattklein123 left a comment

mattklein123 Aug 9, 2021

soulxu Aug 9, 2021

mattklein123 commented Aug 9, 2021

soulxu commented Aug 10, 2021

ggreenway Aug 10, 2021

soulxu Aug 10, 2021

	* ext_authz: fixed skipping authentication when using returning either a direct response or a redirect. This behavior can be temporarily reverted by setting the ``envoy.reloadable_features.http_ext_authz_do_not_skip_direct_response_and_redirect`` runtime guard to false.
	* ext_authz: fixed skipping authentication when returning either a direct response or a redirect. This behavior can be temporarily reverted by setting the ``envoy.reloadable_features.http_ext_authz_do_not_skip_direct_response_and_redirect`` runtime guard to false.

ext_authz: do the authentication even the direct response is set #17546

ext_authz: do the authentication even the direct response is set #17546

Conversation

soulxu commented Jul 30, 2021

soulxu commented Jul 30, 2021

dmitri-d Jul 30, 2021

Choose a reason for hiding this comment

soulxu Aug 5, 2021 • edited Loading

Choose a reason for hiding this comment

mattklein123 Aug 5, 2021

Choose a reason for hiding this comment

soulxu Aug 5, 2021

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

soulxu commented Aug 8, 2021

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 Aug 9, 2021

Choose a reason for hiding this comment

soulxu Aug 9, 2021

Choose a reason for hiding this comment

mattklein123 commented Aug 9, 2021

soulxu commented Aug 10, 2021

ggreenway Aug 10, 2021

Choose a reason for hiding this comment

soulxu Aug 10, 2021

Choose a reason for hiding this comment

soulxu Aug 5, 2021 •

edited

Loading