Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
esmet wants to merge 13 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
b894b41
brotoli: fix build by adding category field to bazel BUILD files
esmet Feb 8, 2021
8285f44
ext_authz: support external authorization checks when a route contain…
esmet Feb 9, 2021
e90cb9b
Add changelog entry
esmet Feb 9, 2021
f6020cd
Merge remote-tracking branch 'upstream/main' into ext-authz-direct-re…
esmet Feb 9, 2021
3b939e3
Merge remote-tracking branch 'upstream/main' into ext-authz-direct-re…
esmet Feb 9, 2021
42460e3
Add test cases
esmet Feb 9, 2021
6b95ba1
Formatting
esmet Feb 9, 2021
46b3dae
Add more coverage
esmet Feb 10, 2021
b485073
Fix style
esmet Feb 11, 2021
076c5e6
Merge remote-tracking branch 'origin/main' into ext-authz-direct-resp…
esmet Feb 12, 2021
4f13d5e
Use a constexpr
esmet Feb 16, 2021
4a96811
Merge remote-tracking branch 'upstream/main' into ext-authz-direct-re…
esmet Mar 29, 2021
b9e977e
Fix up the changelog
esmet Mar 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/root/version_history/current.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ Minor Behavior Changes
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>`.
Copy link
Member

@junr03 junr03 Jun 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we call this out in the ext_authz docs?

* grpc_json_transcoder: filter now adheres to encoder and decoder buffer limits. Requests and responses
that require buffering over the limits will be directly rejected. The behavior can be reverted by
disabling runtime feature `envoy.reloadable_features.grpc_json_transcoder_adhere_to_buffer_limits`.
Expand Down
14 changes: 11 additions & 3 deletions source/extensions/filters/http/ext_authz/ext_authz.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,8 +376,16 @@ void Filter::continueDecoding() {
}

Filter::PerRouteFlags Filter::getPerRouteFlags(const Router::RouteConstSharedPtr& route) const {
if (route == nullptr || route->routeEntry() == nullptr) {
return PerRouteFlags{true /*skip_check_*/, false /*skip_request_body_buffering_*/};
if (route == nullptr) {
// If there's no route, skip authorization checks. This is an optimization to avoid work
// if we know the request won't be forwarded upstream anyway.
return PerRouteFlags::skipCheckFlags();
}

if (route->routeEntry() == nullptr && route->directResponseEntry() == nullptr) {
Copy link
Member

@dio dio Feb 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@esmet esmet Feb 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

@esmet esmet Mar 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// If there's a route, but no route entry nor direct response entry, then skip authorization
// checks. This is another optimization.
return PerRouteFlags::skipCheckFlags();
}

const auto* specific_per_route_config =
Expand All @@ -388,7 +396,7 @@ Filter::PerRouteFlags Filter::getPerRouteFlags(const Router::RouteConstSharedPtr
specific_per_route_config->disableRequestBodyBuffering()};
}

return PerRouteFlags{false /*skip_check_*/, false /*skip_request_body_buffering_*/};
return PerRouteFlags::defaultFlags();
}

} // namespace ExtAuthz
Expand Down
4 changes: 4 additions & 0 deletions source/extensions/filters/http/ext_authz/ext_authz.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,10 @@ class Filter : public Logger::Loggable<Logger::Id::filter>,
struct PerRouteFlags {
const bool skip_check_;
const bool skip_request_body_buffering_;

static constexpr PerRouteFlags defaultFlags() { return PerRouteFlags{false, false}; }
Copy link
Member

@junr03 junr03 Jun 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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


static constexpr PerRouteFlags skipCheckFlags() { return PerRouteFlags{true, false}; }
};
PerRouteFlags getPerRouteFlags(const Router::RouteConstSharedPtr& route) const;

Expand Down
37 changes: 35 additions & 2 deletions test/extensions/filters/http/ext_authz/ext_authz_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1599,9 +1599,42 @@ TEST_P(HttpFilterTestParam, DisabledOnRouteWithRequestBody) {
EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(request_trailers_));
}

// Test that the request continues when the filter_callbacks has no route.
TEST_P(HttpFilterTestParam, NoRoute) {
// Test that the request stops at decodeHeaders when filter_callbacks has a route entry.
TEST_P(HttpFilterTestParam, StopIterationWithRouteEntry) {
NiceMock<Router::MockRouteEntry> route_entry;
prepareCheck();
EXPECT_CALL(*client_, check(_, _, _, _));
EXPECT_CALL(*filter_callbacks_.route_, routeEntry()).WillRepeatedly(Return(&route_entry));
EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark,
filter_->decodeHeaders(request_headers_, true));
}

// 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;
Copy link
Member

@junr03 junr03 Jun 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we have a test with redirect?

prepareCheck();
EXPECT_CALL(*client_, check(_, _, _, _));
EXPECT_CALL(*filter_callbacks_.route_, routeEntry()).WillRepeatedly(Return(nullptr));
EXPECT_CALL(*filter_callbacks_.route_, directResponseEntry())
.WillOnce(Return(&direct_response_entry));
EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark,
filter_->decodeHeaders(request_headers_, true));
}

// Test that the request continues when filter_callbacks has no route
TEST_P(HttpFilterTestParam, ContinueIterationWithNoRoute) {
EXPECT_CALL(filter_callbacks_, route()).WillRepeatedly(Return(nullptr));
EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false));
EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(data_, false));
EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(request_trailers_));
}

// Test that the request continues when filter_callbacks has no route entry / direct response entry.
TEST_P(HttpFilterTestParam, ContinueIterationWithNoRouteEntryOrDirectResponseEntry) {
EXPECT_CALL(*client_, check(_, _, _, _)).Times(0);
EXPECT_CALL(*filter_callbacks_.route_, routeEntry()).WillOnce(Return(nullptr));
EXPECT_CALL(*filter_callbacks_.route_, directResponseEntry()).WillOnce(Return(nullptr));
EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false));
EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(data_, false));
EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(request_trailers_));
Expand Down