-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.
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
ext_authz: allow headers propagation when allow_debugging_failures is set #24845
Changes from all commits
6ccd206
d8cbb9b
2754dd7
ea16a3f
2e70f00
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -114,6 +114,9 @@ ClientConfig::ClientConfig(const envoy::extensions::filters::http::ext_authz::v3 | |
config.http_service().authorization_response().allowed_upstream_headers())), | ||
upstream_header_to_append_matchers_(toUpstreamMatchers( | ||
config.http_service().authorization_response().allowed_upstream_headers_to_append())), | ||
propagate_response_on_failure_(config.has_http_service() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Since the default is false you don't need the has check. You can just directly get the value from the default http_service() object. |
||
? config.http_service().propagate_response_on_failure() | ||
: false), | ||
cluster_name_(config.http_service().server_uri().cluster()), timeout_(timeout), | ||
path_prefix_(path_prefix), | ||
tracing_name_(fmt::format("async {} egress", config.http_service().server_uri().cluster())), | ||
|
@@ -275,7 +278,7 @@ ResponsePtr RawHttpClientImpl::toResponse(Http::ResponseMessagePtr message) { | |
// Set an error status if the call to the authorization server returns any of the 5xx HTTP error | ||
// codes. A Forbidden response is sent to the client if the filter has not been configured with | ||
// failure_mode_allow. | ||
if (Http::CodeUtility::is5xx(status_code)) { | ||
if (Http::CodeUtility::is5xx(status_code) && !config_->propagateResponseOnFailure()) { | ||
return std::make_unique<Response>(errorResponse()); | ||
} | ||
|
||
|
@@ -326,6 +329,29 @@ ResponsePtr RawHttpClientImpl::toResponse(Http::ResponseMessagePtr message) { | |
return std::move(ok.response_); | ||
} | ||
|
||
// Checks whether the status is 5XX and creates an Error authorization response with headers | ||
// status, body, and received from upstream. | ||
if (Http::CodeUtility::is5xx(status_code)) { | ||
SuccessResponse error{message->headers(), | ||
config_->clientHeaderMatchers(), | ||
config_->upstreamHeaderToAppendMatchers(), | ||
config_->clientHeaderOnSuccessMatchers(), | ||
config_->dynamicMetadataMatchers(), | ||
Response{CheckStatus::Error, | ||
Http::HeaderVector{}, | ||
Http::HeaderVector{}, | ||
Http::HeaderVector{}, | ||
Http::HeaderVector{}, | ||
Http::HeaderVector{}, | ||
{{}}, | ||
Http::Utility::QueryParamsVector{}, | ||
{}, | ||
message->bodyAsString(), | ||
static_cast<Http::Code>(status_code), | ||
ProtobufWkt::Struct{}}}; | ||
return std::move(error.response_); | ||
} | ||
|
||
// Create a Denied authorization response. | ||
SuccessResponse denied{message->headers(), | ||
config_->clientHeaderMatchers(), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -418,9 +418,52 @@ void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) { | |
*decoder_callbacks_, enumToInt(config_->statusOnError())); | ||
decoder_callbacks_->streamInfo().setResponseFlag( | ||
StreamInfo::ResponseFlag::UnauthorizedExternalService); | ||
decoder_callbacks_->sendLocalReply( | ||
config_->statusOnError(), EMPTY_STRING, nullptr, absl::nullopt, | ||
Filters::Common::ExtAuthz::ResponseCodeDetails::get().AuthzError); | ||
if (config_->propagateResponseOnFailure()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems odd to me that this logic is split across multiple files and that there is additional logic here for things like stats. Why do we additionally increment stats here? Also, if the header processing is done in the other file can't this file just stay a unified path? |
||
ENVOY_STREAM_LOG(trace, | ||
"ext_authz filter has failure debugging allowed. Propagating response " | ||
"body and headers back to the client.", | ||
*decoder_callbacks_); | ||
stats_.propagate_response_on_failure_.inc(); | ||
if (cluster_) { | ||
config_->incCounter(cluster_->statsScope(), | ||
config_->ext_authz_propagate_response_on_failure_); | ||
Http::CodeStats::ResponseStatInfo info{config_->scope(), | ||
cluster_->statsScope(), | ||
empty_stat_name, | ||
enumToInt(response->status_code), | ||
true, | ||
empty_stat_name, | ||
empty_stat_name, | ||
empty_stat_name, | ||
empty_stat_name, | ||
empty_stat_name, | ||
false}; | ||
config_->httpContext().codeStats().chargeResponseStat(info, false); | ||
} | ||
decoder_callbacks_->sendLocalReply( | ||
response->status_code, response->body, | ||
[&headers = response->headers_to_set, | ||
&callbacks = *decoder_callbacks_](Http::HeaderMap& response_headers) -> void { | ||
ENVOY_STREAM_LOG( | ||
trace, "ext_authz filter added header(s) to the local response:", callbacks); | ||
// Firstly, remove all headers requested by the ext_authz filter, to ensure that they | ||
// will override existing headers. | ||
for (const auto& header : headers) { | ||
response_headers.remove(header.first); | ||
} | ||
// Then set all the requested headers, allowing the same header to be set multiple | ||
// times, e.g. `Set-Cookie`. | ||
for (const auto& header : headers) { | ||
ENVOY_STREAM_LOG(trace, " '{}':'{}'", callbacks, header.first.get(), header.second); | ||
response_headers.setCopy(header.first, header.second); | ||
} | ||
}, | ||
absl::nullopt, Filters::Common::ExtAuthz::ResponseCodeDetails::get().AuthzError); | ||
} else { | ||
decoder_callbacks_->sendLocalReply( | ||
config_->statusOnError(), EMPTY_STRING, nullptr, absl::nullopt, | ||
Filters::Common::ExtAuthz::ResponseCodeDetails::get().AuthzError); | ||
} | ||
} | ||
break; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,7 +39,8 @@ namespace ExtAuthz { | |
COUNTER(denied) \ | ||
COUNTER(error) \ | ||
COUNTER(disabled) \ | ||
COUNTER(failure_mode_allowed) | ||
COUNTER(failure_mode_allowed) \ | ||
COUNTER(propagate_response_on_failure) | ||
|
||
/** | ||
* Wrapper struct for ext_authz filter stats. @see stats_macros.h | ||
|
@@ -60,6 +61,9 @@ class FilterConfig { | |
const std::string& stats_prefix, envoy::config::bootstrap::v3::Bootstrap& bootstrap) | ||
: allow_partial_message_(config.with_request_body().allow_partial_message()), | ||
failure_mode_allow_(config.failure_mode_allow()), | ||
propagate_response_on_failure_(config.has_http_service() | ||
? config.http_service().propagate_response_on_failure() | ||
: false), | ||
Comment on lines
+64
to
+66
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment |
||
clear_route_cache_(config.clear_route_cache()), | ||
max_request_bytes_(config.with_request_body().max_request_bytes()), | ||
pack_as_bytes_(config.with_request_body().pack_as_bytes()), | ||
|
@@ -88,7 +92,9 @@ class FilterConfig { | |
ext_authz_denied_(pool_.add(createPoolStatName(config.stat_prefix(), "denied"))), | ||
ext_authz_error_(pool_.add(createPoolStatName(config.stat_prefix(), "error"))), | ||
ext_authz_failure_mode_allowed_( | ||
pool_.add(createPoolStatName(config.stat_prefix(), "failure_mode_allowed"))) { | ||
pool_.add(createPoolStatName(config.stat_prefix(), "failure_mode_allowed"))), | ||
ext_authz_propagate_response_on_failure_( | ||
pool_.add(createPoolStatName(config.stat_prefix(), "propagate_response_on_failure"))) { | ||
auto labels_key_it = | ||
bootstrap.node().metadata().fields().find(config.bootstrap_metadata_labels_key()); | ||
if (labels_key_it != bootstrap.node().metadata().fields().end()) { | ||
|
@@ -127,6 +133,8 @@ class FilterConfig { | |
|
||
bool failureModeAllow() const { return failure_mode_allow_; } | ||
|
||
bool propagateResponseOnFailure() const { return propagate_response_on_failure_; } | ||
|
||
bool clearRouteCache() const { return clear_route_cache_; } | ||
|
||
uint32_t maxRequestBytes() const { return max_request_bytes_; } | ||
|
@@ -198,6 +206,7 @@ class FilterConfig { | |
|
||
const bool allow_partial_message_; | ||
const bool failure_mode_allow_; | ||
const bool propagate_response_on_failure_; | ||
const bool clear_route_cache_; | ||
const uint32_t max_request_bytes_; | ||
const bool pack_as_bytes_; | ||
|
@@ -231,6 +240,7 @@ class FilterConfig { | |
const Stats::StatName ext_authz_denied_; | ||
const Stats::StatName ext_authz_error_; | ||
const Stats::StatName ext_authz_failure_mode_allowed_; | ||
const Stats::StatName ext_authz_propagate_response_on_failure_; | ||
}; | ||
|
||
using FilterConfigSharedPtr = std::shared_ptr<FilterConfig>; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -179,6 +179,40 @@ class ExtAuthzHttpClientTest : public testing::Test { | |
NiceMock<StreamInfo::MockStreamInfo> stream_info_; | ||
}; | ||
|
||
// Verify that when a call to authorization server returns a 5xx and `propagate_response_on_failure` | ||
// is set then we preserve the original status code, headers, etc. | ||
Comment on lines
+182
to
+183
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add an integration test for this. |
||
TEST_F(ExtAuthzHttpClientTest, AuthorizationRequest5xxNoError) { | ||
const std::string yaml = R"EOF( | ||
http_service: | ||
server_uri: | ||
uri: "ext_authz:9000" | ||
cluster: "ext_authz" | ||
timeout: 0.25s | ||
authorization_response: | ||
dynamic_metadata_from_headers: | ||
patterns: | ||
- prefix: "X-Metadata-" | ||
ignore_case: true | ||
propagate_response_on_failure: true | ||
)EOF"; | ||
|
||
initialize(yaml); | ||
|
||
const auto expected_body = std::string{"test"}; | ||
const auto expected_headers = TestCommon::makeHeaderValueOption( | ||
{{":status", "500", false}, {"foo", "bar", false}, {"honey", "bee", false}}); | ||
const auto authz_response = TestCommon::makeAuthzResponse( | ||
CheckStatus::Error, Http::Code::InternalServerError, expected_body, expected_headers); | ||
|
||
envoy::service::auth::v3::CheckRequest request; | ||
client_->check(request_callbacks_, request, parent_span_, stream_info_); | ||
|
||
EXPECT_CALL(request_callbacks_, | ||
onComplete_(WhenDynamicCastTo<ResponsePtr&>(AuthzDeniedResponse(authz_response)))); | ||
client_->onSuccess(async_request_, | ||
TestCommon::makeMessageResponse(expected_headers, expected_body)); | ||
} | ||
|
||
// Test HTTP client config default values. | ||
TEST_F(ExtAuthzHttpClientTest, ClientConfig) { | ||
const Http::LowerCaseString foo{"foo"}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code appears to only do this for 5xx. Can you clarify this? Is there any reason not to do this for 4xx also? This seems generally useful as well?