ext_authz: support external authorization checks when a route contains a direct response entry by esmet · Pull Request #14989 · envoyproxy/envoy

esmet · 2021-02-09T03:05:22Z

Commit Message: ext_authz: support external authorization checks when a route contains a direct response entry
Additional Description:
Risk Level: low
Testing: TODO
Docs Changes:
Release Notes: ext_authz: fix a bug that skipped external authorization checks for routes that contained only a direct response.
Fixes ##14984

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

…s a direct response entry Signed-off-by: John Esmet <john.esmet@gmail.com>

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

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

esmet · 2021-02-09T15:38:29Z

@dio do you mind providing a quick first pass while I work on test(s) and addressing feedback on other PRs?

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

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

dio · 2021-02-09T20:11:32Z

Thank you for working on this. Yeah, I think this makes sense.

esmet · 2021-02-09T23:56:01Z

There are tests so I'm pulling it out of draft 👍

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

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

dio

Thanks for working on this! Looks good overall. Asking for improving the docs and an opportunity for optimization.

dio · 2021-02-16T11:58:43Z

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

    const bool skip_check_;
    const bool skip_request_body_buffering_;
+
+    static PerRouteFlags defaultFlags() { return PerRouteFlags{false, false}; }


can we do static constexpr here?

dio · 2021-02-16T12:00:13Z

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

+    return PerRouteFlags::skipCheckFlags();
+  }
+
+  if (route->routeEntry() == nullptr && route->directResponseEntry() == nullptr) {


Could you quickly scan in the docs, if we have the opportunity to inform this explicitly in the docs?

@dio Good callout. I read the docs. I think we're actually going to skip authz checks for redirect actions still. Maybe this feature would be more complete if we:

Documented that only routes that proxy requests upstream will be covered by ext_authz by default

Add a knob to enable ext_authz on direct response actions

Add a knob to enable ext_authz on redirect actions

This way, this "bugfix" is now instead a new feature that won't break existing users that expect the current behavior. Principle of least surprise is violated, in theory, but we can mitigate that with documentation and I think this is better than possibly introducing a breaking change when we meant to make things better.

By the way, I was wrong. Redirects and direct replies are both "direct responses" on the route object so I believe this change will correctly apply to them all. I should probably add an integration test.

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

dio

Thanks! Could you update the PR description, so it will be easier for maintainers when landing this.

dio

Thanks! Could you update the PR description, so it will be easier for maintainers when landing this.

dio

Thanks! Could you update the PR description, so it will be easier for maintainers when landing this.

dio · 2021-02-19T11:25:53Z

Wow. GitHub put 3 approvals 😅. Sorry for that.

esmet · 2021-02-19T15:11:34Z

@dio thanks for the approval. But, I'm moving this back to draft because I have concerns that this bugfix could be considered a minor breaking change and also doesn't cover redirect responses (see #14989 (comment)). When I come back to working on this PR, I'll look into either updating the changelog to mention that this is a behavioral change or add config knobs to control whether direct / redirect responses are covered by ext_authz by default.

dio · 2021-02-19T22:07:22Z

Oops. OK. Sorry for missing that comment.

aidandj · 2021-03-05T00:25:12Z

@dio thanks for the approval. But, I'm moving this back to draft because I have concerns that this bugfix could be considered a minor breaking change and also doesn't cover redirect responses (see #14989 (comment)). When I come back to working on this PR, I'll look into either updating the changelog to mention that this is a behavioral change or add config knobs to control whether direct / redirect responses are covered by ext_authz by default.

I thought about this for a bit. Would it be odd behavior to have redirect/direct responses default differently than other routes. Right now you need to disable a filter on a route otherwise it automatically applies. It seems like this has to be a breaking change, otherwise you end up with an unintuitive UI. :/

esmet · 2021-03-23T20:32:06Z

@artificial-aidan I agree that the user experience will be surprising (in a bad way) unless we break the current behavior.

Can I get an expert opinion from @envoyproxy/api-shepherds on whether the behavioral bugfix in this ticket / PR is small enough to simply be included in "Minor behavioral changes" in the changelog, or if it is big enough to deserve its own API switch?

The more I think on it, the more I'm leaning on noting it as a minor behavioral change because the current behavior is indeed an unintentional bug and there is a clear "workaround" to achieve the current behavior (by setting per filter config, as one would usually expect).

htuch · 2021-03-25T02:45:55Z

@esmet not sure if I'm seeing the API concern here. Could you elaborate on how this might impact someone operating an ext_authz service or what the change in semantics are?

If we're just saying "we fixed this to work for direct response entries as anyone who read the xDS docs would have expected anyway", I think we are good.

esmet · 2021-03-28T23:27:25Z

@htuch The question is essentially whether we think fixing this bug is enough of a behavioral change to actually be considered a breaking change, so I was looking to get an opinion on whether that meant we needed to add an API flag for this. There may have been some precedent I was unaware of, for example.

The way I could see this breaking behavior is if someone set up a redirect (or direct response) that was never meant to have authorization checks, but those users may have never set a per-route config to skip it because this bug meant they never had to. Once we fix it, those users will need to fix their configs, and that could be an unexpected (even if deserved) surprise.

But I think you're saying that as long as this is a really just a bug fix and the fixed behavior is reasonable to expect given the docs, we shouldn't need to do that. I agree and I'm OK with that too. I'll keep this PR as is in that case 👍

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

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

github-actions · 2021-04-29T20:01:11Z

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!

aidandj · 2021-04-29T20:07:04Z

This would still be very helpful for us, and would simplify a lot of configs.

junr03

Thanks! Almost done

/wait

junr03 · 2021-06-04T15:38:21Z

docs/root/version_history/current.rst

  logging, :ref:`auto_host_rewrite <envoy_api_field_route.RouteAction.auto_host_rewrite>`, etc.
  Setting the hostname manually allows overriding the internal hostname used for such features while
  still allowing the original DNS resolution name to be used.
+* ext_authz: routes that contain a redirect or a direct response action are now properly subject to external authorization checks by default. To restore the original behavior and skip authorization checks, disable ext_authz on the :ref:`per-route config <envoy_v3_api_msg_extensions.filters.http.ext_authz.v3.ExtAuthzPerRoute>`.


Should we call this out in the ext_authz docs?

junr03 · 2021-06-04T15:40:50Z

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

    const bool skip_check_;
    const bool skip_request_body_buffering_;
+
+    static constexpr PerRouteFlags defaultFlags() { return PerRouteFlags{false, false}; }


can we add comments on what the bools are here and below?

e.g., false /*skip_check_*/, false /*skip_request_body_buffering_*/

junr03 · 2021-06-04T15:41:36Z

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

+// Test that the request stops at decodeHeaders when filter_callbacks has no route entry, but it
+// does have a direct response entry.
+TEST_P(HttpFilterTestParam, StopIterationWithDirectResponseEntry) {
+  NiceMock<Router::MockDirectResponseEntry> direct_response_entry;


should we have a test with redirect?

github-actions · 2021-07-04T16:00:58Z

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!

aidandj · 2021-07-04T17:18:11Z

Still hoping to see this get in.

github-actions · 2021-08-03T20:01:06Z

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!

aidandj · 2021-08-03T20:27:43Z

Not stale

soulxu · 2021-08-13T23:59:12Z

@artificial-aidan This should be already fixed by #17449 and #17546

@esmet sorry, I didn't notice your PR when I fixed the bug #17377

esmet · 2021-08-19T00:02:47Z

@soulxu thanks for picking this up. I let a bunch of my work go stale unfortunately, this included.

esmet added 4 commits February 9, 2021 02:57

brotoli: fix build by adding category field to bazel BUILD files

b894b41

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

ext_authz: support external authorization checks when a route contain…

8285f44

…s a direct response entry Signed-off-by: John Esmet <john.esmet@gmail.com>

Add changelog entry

e90cb9b

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

Merge remote-tracking branch 'upstream/main' into ext-authz-direct-re…

f6020cd

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

Merge remote-tracking branch 'upstream/main' into ext-authz-direct-re…

3b939e3

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

antoniovicente assigned dio Feb 9, 2021

antoniovicente requested a review from dio February 9, 2021 18:53

esmet added 2 commits February 9, 2021 19:44

Add test cases

42460e3

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

Formatting

6b95ba1

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

esmet marked this pull request as ready for review February 9, 2021 23:56

esmet added 3 commits February 10, 2021 01:23

Add more coverage

46b3dae

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

Fix style

b485073

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

Merge remote-tracking branch 'origin/main' into ext-authz-direct-resp…

076c5e6

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

dio suggested changes Feb 16, 2021

View reviewed changes

Use a constexpr

4f13d5e

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

dio approved these changes Feb 19, 2021

View reviewed changes

dio previously approved these changes Feb 19, 2021

View reviewed changes

esmet marked this pull request as draft February 19, 2021 15:09

mattklein123 added the waiting label Mar 2, 2021

esmet mentioned this pull request Mar 22, 2021

Direct response/Redirect with ext_authz #14984

Closed

esmet marked this pull request as ready for review March 23, 2021 20:32

esmet added 2 commits March 29, 2021 01:55

Merge remote-tracking branch 'upstream/main' into ext-authz-direct-re…

4a96811

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

Fix up the changelog

b9e977e

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

esmet dismissed dio’s stale review via b9e977e March 30, 2021 16:59

repokitteh-read-only bot removed the waiting label Mar 30, 2021

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Apr 29, 2021

github-actions bot removed the stale stalebot believes this issue/PR has not been touched recently label Apr 30, 2021

mattklein123 assigned junr03 and unassigned dio May 27, 2021

junr03 reviewed Jun 4, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 4, 2021

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

github-actions bot removed the stale stalebot believes this issue/PR has not been touched recently label Jul 4, 2021

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Aug 3, 2021

github-actions bot removed the stale stalebot believes this issue/PR has not been touched recently label Aug 4, 2021

esmet closed this Aug 19, 2021

esmet deleted the ext-authz-direct-response branch August 19, 2021 00:03

Conversation

esmet commented Feb 9, 2021

Uh oh!

esmet commented Feb 9, 2021

Uh oh!

dio commented Feb 9, 2021

Uh oh!

esmet commented Feb 9, 2021

Uh oh!

dio left a comment

Choose a reason for hiding this comment

Uh oh!

dio Feb 16, 2021

Choose a reason for hiding this comment

Uh oh!

dio Feb 16, 2021

Choose a reason for hiding this comment

Uh oh!

esmet Feb 16, 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!

esmet Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

dio left a comment

Choose a reason for hiding this comment

Uh oh!

dio left a comment

Choose a reason for hiding this comment

Uh oh!

dio left a comment

Choose a reason for hiding this comment

Uh oh!

dio commented Feb 19, 2021

Uh oh!

esmet commented Feb 19, 2021

Uh oh!

dio commented Feb 19, 2021

Uh oh!

aidandj commented Mar 5, 2021

Uh oh!

esmet commented Mar 23, 2021

Uh oh!

htuch commented Mar 25, 2021

Uh oh!

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

Uh oh!

Uh oh!

github-actions bot commented Apr 29, 2021

Uh oh!

aidandj commented Apr 29, 2021

Uh oh!

junr03 left a comment

Choose a reason for hiding this comment

Uh oh!

junr03 Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

junr03 Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

junr03 Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 4, 2021

Uh oh!

aidandj commented Jul 4, 2021

Uh oh!

github-actions bot commented Aug 3, 2021

Uh oh!

aidandj commented Aug 3, 2021

Uh oh!

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

Uh oh!

Uh oh!

esmet commented Aug 19, 2021

Uh oh!

Reviewers

Assignees

esmet Feb 16, 2021 •

edited

Loading

esmet commented Mar 28, 2021 •

edited

Loading

soulxu commented Aug 13, 2021 •

edited

Loading