diff --git a/source/extensions/filters/http/ext_authz/ext_authz.cc b/source/extensions/filters/http/ext_authz/ext_authz.cc index b8d4a5c27a85..d67faa1e5d25 100644 --- a/source/extensions/filters/http/ext_authz/ext_authz.cc +++ b/source/extensions/filters/http/ext_authz/ext_authz.cc @@ -36,6 +36,8 @@ void Filter::initiateCall(const Http::HeaderMap& headers) { check_request_); state_ = State::Calling; + // Don't let the filter chain continue as we are going to invoke check call. + filter_return_ = FilterReturn::StopDecoding; initiating_call_ = true; client_->check(*this, check_request_, callbacks_->activeSpan()); initiating_call_ = false; @@ -43,18 +45,19 @@ void Filter::initiateCall(const Http::HeaderMap& headers) { Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool) { initiateCall(headers); - return state_ == State::Calling ? Http::FilterHeadersStatus::StopIteration - : Http::FilterHeadersStatus::Continue; + return filter_return_ == FilterReturn::StopDecoding ? Http::FilterHeadersStatus::StopIteration + : Http::FilterHeadersStatus::Continue; } Http::FilterDataStatus Filter::decodeData(Buffer::Instance&, bool) { - return state_ == State::Calling ? Http::FilterDataStatus::StopIterationAndWatermark - : Http::FilterDataStatus::Continue; + return filter_return_ == FilterReturn::StopDecoding + ? Http::FilterDataStatus::StopIterationAndWatermark + : Http::FilterDataStatus::Continue; } Http::FilterTrailersStatus Filter::decodeTrailers(Http::HeaderMap&) { - return state_ == State::Calling ? Http::FilterTrailersStatus::StopIteration - : Http::FilterTrailersStatus::Continue; + return filter_return_ == FilterReturn::StopDecoding ? Http::FilterTrailersStatus::StopIteration + : Http::FilterTrailersStatus::Continue; } void Filter::setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) { @@ -82,6 +85,7 @@ void Filter::onComplete(Filters::Common::ExtAuthz::CheckStatus status) { case CheckStatus::Error: cluster_->statsScope().counter("ext_authz.error").inc(); break; + case CheckStatus::Denied: cluster_->statsScope().counter("ext_authz.denied").inc(); Http::CodeUtility::ResponseStatInfo info{config_->scope(), @@ -106,12 +110,14 @@ void Filter::onComplete(Filters::Common::ExtAuthz::CheckStatus status) { callbacks_->requestInfo().setResponseFlag( RequestInfo::ResponseFlag::UnauthorizedExternalService); } else { + // Let the filter chain continue. + filter_return_ = FilterReturn::ContinueDecoding; if (config_->failureModeAllow() && status == CheckStatus::Error) { // Status is Error and yet we are allowing the request. Click a counter. cluster_->statsScope().counter("ext_authz.failure_mode_allowed").inc(); } - // We can get completion inline, so only call continue if that isn't happening. if (!initiating_call_) { + // We got completion async. Let the filter chain continue. callbacks_->continueDecoding(); } } diff --git a/source/extensions/filters/http/ext_authz/ext_authz.h b/source/extensions/filters/http/ext_authz/ext_authz.h index 794a614c898a..8220bf4a327c 100644 --- a/source/extensions/filters/http/ext_authz/ext_authz.h +++ b/source/extensions/filters/http/ext_authz/ext_authz.h @@ -80,14 +80,23 @@ class Filter : public Http::StreamDecoderFilter, void onComplete(Filters::Common::ExtAuthz::CheckStatus status) override; private: + // State of this filter's communication with the external authorization service. + // The filter has either not started calling the external service, in the middle of calling + // it or has completed. enum class State { NotStarted, Calling, Complete }; + // FilterReturn is used to capture what the return code should be to the filter chain. + // if this filter is either in the middle of calling the service or the result is denied then + // the filter chain should stop. Otherwise the filter chain can continue to the next filter. + enum class FilterReturn { ContinueDecoding, StopDecoding }; void initiateCall(const Http::HeaderMap& headers); FilterConfigSharedPtr config_; Filters::Common::ExtAuthz::ClientPtr client_; Http::StreamDecoderFilterCallbacks* callbacks_{}; State state_{State::NotStarted}; + FilterReturn filter_return_{FilterReturn::ContinueDecoding}; Upstream::ClusterInfoConstSharedPtr cluster_; + // Used to identify if the callback to onComplete() is synchronous (on the stack) or asynchronous. bool initiating_call_{}; envoy::service::auth::v2alpha::CheckRequest check_request_{}; }; diff --git a/source/extensions/filters/network/ext_authz/ext_authz.cc b/source/extensions/filters/network/ext_authz/ext_authz.cc index 6ea35d2eab78..d0d2ca335bfe 100644 --- a/source/extensions/filters/network/ext_authz/ext_authz.cc +++ b/source/extensions/filters/network/ext_authz/ext_authz.cc @@ -35,8 +35,8 @@ Network::FilterStatus Filter::onData(Buffer::Instance&, bool /* end_stream */) { // sufficient information to fillout the checkRequest_. callCheck(); } - return status_ == Status::Calling ? Network::FilterStatus::StopIteration - : Network::FilterStatus::Continue; + return filter_return_ == FilterReturn::Stop ? Network::FilterStatus::StopIteration + : Network::FilterStatus::Continue; } Network::FilterStatus Filter::onNewConnection() { @@ -78,6 +78,8 @@ void Filter::onComplete(Filters::Common::ExtAuthz::CheckStatus status) { config_->stats().cx_closed_.inc(); filter_callbacks_->connection().close(Network::ConnectionCloseType::NoFlush); } else { + // Let the filter chain continue. + filter_return_ = FilterReturn::Continue; if (config_->failureModeAllow() && status == Filters::Common::ExtAuthz::CheckStatus::Error) { // Status is Error and yet we are configured to allow traffic. Click a counter. config_->stats().failure_mode_allowed_.inc(); diff --git a/source/extensions/filters/network/ext_authz/ext_authz.h b/source/extensions/filters/network/ext_authz/ext_authz.h index 025e74935d32..ef97b822c2b3 100644 --- a/source/extensions/filters/network/ext_authz/ext_authz.h +++ b/source/extensions/filters/network/ext_authz/ext_authz.h @@ -93,13 +93,22 @@ class Filter : public Network::ReadFilter, void onComplete(Filters::Common::ExtAuthz::CheckStatus status) override; private: + // State of this filter's communication with the external authorization service. + // The filter has either not started calling the external service, in the middle of calling + // it or has completed. enum class Status { NotStarted, Calling, Complete }; + // FilterReturn is used to capture what the return code should be to the filter chain. + // if this filter is either in the middle of calling the external service or the result is denied + // then the filter chain should stop. Otherwise the filter chain can continue to the next filter. + enum class FilterReturn { Stop, Continue }; void callCheck(); ConfigSharedPtr config_; Filters::Common::ExtAuthz::ClientPtr client_; Network::ReadFilterCallbacks* filter_callbacks_{}; Status status_{Status::NotStarted}; + FilterReturn filter_return_{FilterReturn::Stop}; + // Used to identify if the callback to onComplete() is synchronous (on the stack) or asynchronous. bool calling_check_{}; envoy::service::auth::v2alpha::CheckRequest check_request_{}; }; diff --git a/test/extensions/filters/http/ext_authz/ext_authz_test.cc b/test/extensions/filters/http/ext_authz/ext_authz_test.cc index d1eaef9a1d9c..972ae716b25f 100644 --- a/test/extensions/filters/http/ext_authz/ext_authz_test.cc +++ b/test/extensions/filters/http/ext_authz/ext_authz_test.cc @@ -186,6 +186,31 @@ TEST_P(HttpExtAuthzFilterParamTest, ImmediateOkResponse) { cm_.thread_local_cluster_.cluster_.info_->stats_store_.counter("ext_authz.ok").value()); } +// Test that an synchronous denied response from the authorization service, on the call stack, +// results in request not continuing. +TEST_P(HttpExtAuthzFilterParamTest, ImmediateDeniedResponse) { + InSequence s; + + ON_CALL(filter_callbacks_, connection()).WillByDefault(Return(&connection_)); + EXPECT_CALL(connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); + EXPECT_CALL(connection_, localAddress()).WillOnce(ReturnRef(addr_)); + EXPECT_CALL(*client_, check(_, _, _)) + .WillOnce( + WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { + callbacks.onComplete(Filters::Common::ExtAuthz::CheckStatus::Denied); + }))); + + EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); + EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, + filter_->decodeHeaders(request_headers_, false)); + EXPECT_EQ(Http::FilterDataStatus::StopIterationAndWatermark, filter_->decodeData(data_, false)); + EXPECT_EQ(Http::FilterTrailersStatus::StopIteration, filter_->decodeTrailers(request_headers_)); + + EXPECT_EQ( + 1U, + cm_.thread_local_cluster_.cluster_.info_->stats_store_.counter("ext_authz.denied").value()); +} + // Test that a denied response results in the connection closing with a 403 response to the client. TEST_P(HttpExtAuthzFilterParamTest, DeniedResponse) { InSequence s; diff --git a/test/extensions/filters/network/ext_authz/ext_authz_test.cc b/test/extensions/filters/network/ext_authz/ext_authz_test.cc index ca426baefcb1..310a90983a3f 100644 --- a/test/extensions/filters/network/ext_authz/ext_authz_test.cc +++ b/test/extensions/filters/network/ext_authz/ext_authz_test.cc @@ -154,7 +154,7 @@ TEST_F(ExtAuthzFilterTest, DeniedWithOnData) { EXPECT_CALL(*client_, cancel()).Times(0); request_callbacks_->onComplete(Filters::Common::ExtAuthz::CheckStatus::Denied); - EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data, false)); + EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onData(data, false)); EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.error").value()); @@ -314,6 +314,32 @@ TEST_F(ExtAuthzFilterTest, ImmediateOK) { EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.cx_closed").value()); } +// Test to verify that on stack denied response from the authorization service does +// result in stopage of the filter chain. +TEST_F(ExtAuthzFilterTest, ImmediateNOK) { + InSequence s; + + EXPECT_CALL(filter_callbacks_.connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); + EXPECT_CALL(filter_callbacks_.connection_, localAddress()).WillOnce(ReturnRef(addr_)); + EXPECT_CALL(filter_callbacks_, continueReading()).Times(0); + EXPECT_CALL(*client_, check(_, _, _)) + .WillOnce( + WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { + callbacks.onComplete(Filters::Common::ExtAuthz::CheckStatus::Denied); + }))); + + EXPECT_EQ(Network::FilterStatus::Continue, filter_->onNewConnection()); + Buffer::OwnedImpl data("hello"); + EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onData(data, false)); + + EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.error").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.failure_mode_allowed").value()); + EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.denied").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.ok").value()); + EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.cx_closed").value()); +} + // Test to verify that on stack Error response when failure_mode_allow is configured // result in request being allowed. TEST_F(ExtAuthzFilterTest, ImmediateErrorFailOpen) {