-
Notifications
You must be signed in to change notification settings - Fork 5.5k
authz_filter: extended ext_authz to support v2alpha api #3162
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
Changes from 2 commits
9501403
8419745
e6d6df0
af3925b
33e7eba
0ed4639
ab5cf4c
615b64c
050f75f
7e9cf7c
09e4125
eb45964
5bd6581
559c8fb
f717cda
69ca997
568b5c1
fe9fa0c
d6e008c
dc61d76
982a210
7acf696
4b3520d
dd9d091
f1aa15f
c454122
681bc64
f8155ab
0250a49
14889bc
e17a6fc
a760b45
e2ba046
298a696
8d05a5d
b7aaef1
6058248
91db962
5c320bb
a5fcd62
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 |
|---|---|---|
|
|
@@ -24,21 +24,24 @@ message CheckRequest { | |
| AttributeContext attributes = 1; | ||
| } | ||
|
|
||
| // An message that contains HTTP response attributes. This message is | ||
| // used when the authorization service needs to send custom responses to the | ||
| // downstream client or, to modify/add request headers being dispatched to the upstream. | ||
| message HttpResponse { | ||
| // Http status code. | ||
| uint32 status_code = 1 [(validate.rules).uint32 = {gte: 100, lt: 600}]; | ||
|
|
||
| // Http entity headers. | ||
| map<string, string> headers = 2; | ||
|
|
||
| // Http entity body. | ||
| string body = 3; | ||
| } | ||
|
|
||
| message CheckResponse { | ||
| // Status `OK` allows the request. Any other status indicates the request should be denied. | ||
| google.rpc.Status status = 1; | ||
|
|
||
| // An optional message that contains HTTP response attributes. This message is | ||
| // used when the authorization service needs to send custom responses to the | ||
| // downstream client or, to modify/add request headers being dispatched to the upstream. | ||
| message HttpResponse { | ||
| // Http status code. | ||
| uint32 status_code = 1 [(validate.rules).uint32 = {gte: 100, lt: 600}]; | ||
|
|
||
| // Http entity headers. | ||
| map<string, string> headers = 2; | ||
|
|
||
| // Http entity body. | ||
| string body = 3; | ||
| } | ||
| // A field that supplies the attributes for an HTTP response. | ||
|
Contributor
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. could u please add a comment about how this is not going to be used for TCP filters since the
Member
Author
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. Good call! I will do that. |
||
| HttpResponse http_response = 2; | ||
|
Contributor
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. The comment here needs to explain whether this affects the response to the downstream client, or upstream server. It also needs to explain the relationship between this field and the
Member
Author
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. Good point too! I will add comments to explain the difference and when to use them. |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,24 @@ enum class CheckStatus { | |
| Denied | ||
| }; | ||
|
|
||
| typedef std::vector<std::pair<Http::LowerCaseString, std::string>> KeyValueHeaders; | ||
|
Member
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. This could be refactored with
Member
Author
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. Sorry, I'm not sure what is needed here. Would you mind to give a bit more context?
Member
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. Just introducing this type in a common header to dedupe it with other identical defs.
Member
Author
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. Ok, I will do. |
||
|
|
||
| /** | ||
| * Authorization response object for a RequestCallback. | ||
| */ | ||
| struct Response { | ||
| // Call status. | ||
| CheckStatus status; | ||
| // Optional http headers attribute. | ||
| KeyValueHeaders headers; | ||
|
Member
Author
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. I think this could be HeaderMapPtr.
Member
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. If it makes thing easier or more efficient, sure.
Member
Author
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. I think both if possible to move it later. I'll need to take a closer look at the HeaderMap api though. |
||
| // Optional http body attribute. | ||
| Buffer::InstancePtr body; | ||
| // Extra http status code attribute. | ||
| uint32_t status_code; | ||
|
Member
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: prefer to zero initialize this.
Member
Author
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. 👍 |
||
| }; | ||
|
|
||
| typedef std::unique_ptr<Response> ResponsePtr; | ||
|
|
||
| /** | ||
| * Async callbacks used during check() calls. | ||
| */ | ||
|
|
@@ -35,9 +53,9 @@ class RequestCallbacks { | |
| virtual ~RequestCallbacks() {} | ||
|
|
||
| /** | ||
| * Called when a check request is complete. The resulting status is supplied. | ||
| * Called when a check request is complete. The resulting ResponsePtr is supplied. | ||
| */ | ||
| virtual void onComplete(CheckStatus status) PURE; | ||
| virtual void onComplete(ResponsePtr&& response) PURE; | ||
| }; | ||
|
|
||
| class Client { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ | |
| #include "envoy/access_log/access_log.h" | ||
| #include "envoy/ssl/connection.h" | ||
|
|
||
| #include "common/buffer/buffer_impl.h" | ||
| #include "common/common/assert.h" | ||
| #include "common/grpc/async_client_impl.h" | ||
| #include "common/http/headers.h" | ||
|
|
@@ -47,23 +48,39 @@ void GrpcClientImpl::check(RequestCallbacks& callbacks, | |
|
|
||
| void GrpcClientImpl::onSuccess( | ||
| std::unique_ptr<envoy::service::auth::v2alpha::CheckResponse>&& response, Tracing::Span& span) { | ||
| CheckStatus status = CheckStatus::OK; | ||
|
|
||
| ASSERT(response->status().code() != Grpc::Status::GrpcStatus::Unknown); | ||
| ResponsePtr authz_response = std::make_unique<Response>(); | ||
|
|
||
| if (response->has_http_response()) { | ||
| for (const auto& header : response->http_response().headers()) { | ||
| authz_response->headers.emplace_back( | ||
| std::make_pair(Http::LowerCaseString(header.first), header.second)); | ||
| } | ||
| if (!response->http_response().body().empty()) { | ||
| authz_response->body = std::make_unique<Buffer::OwnedImpl>(response->http_response().body()); | ||
| } | ||
| } | ||
|
|
||
| if (response->status().code() != Grpc::Status::GrpcStatus::Ok) { | ||
| status = CheckStatus::Denied; | ||
| span.setTag(Constants::get().TraceStatus, Constants::get().TraceUnauthz); | ||
| authz_response->status = CheckStatus::Denied; | ||
| authz_response->status_code = response->http_response().status_code(); | ||
|
Contributor
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.
|
||
| } else { | ||
| span.setTag(Constants::get().TraceStatus, Constants::get().TraceOk); | ||
| authz_response->status = CheckStatus::OK; | ||
| } | ||
|
|
||
| callbacks_->onComplete(status); | ||
| callbacks_->onComplete(std::move(authz_response)); | ||
| callbacks_ = nullptr; | ||
| } | ||
|
|
||
| void GrpcClientImpl::onFailure(Grpc::Status::GrpcStatus status, const std::string&, | ||
| Tracing::Span&) { | ||
| ASSERT(status != Grpc::Status::GrpcStatus::Ok); | ||
| callbacks_->onComplete(CheckStatus::Error); | ||
| ResponsePtr response = std::make_unique<Response>(); | ||
| response->status = CheckStatus::Error; | ||
| callbacks_->onComplete(std::move(response)); | ||
| callbacks_ = nullptr; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,13 +20,11 @@ namespace HttpFilters { | |
| namespace ExtAuthz { | ||
|
|
||
| namespace { | ||
|
|
||
| const Http::HeaderMap* getDeniedHeader() { | ||
| static const Http::HeaderMap* header_map = new Http::HeaderMapImpl{ | ||
| {Http::Headers::get().Status, std::to_string(enumToInt(Http::Code::Forbidden))}}; | ||
| return header_map; | ||
| } | ||
|
|
||
| } // namespace | ||
|
|
||
| void Filter::initiateCall(const Http::HeaderMap& headers) { | ||
|
|
@@ -52,6 +50,7 @@ void Filter::initiateCall(const Http::HeaderMap& headers) { | |
| } | ||
|
|
||
| Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool) { | ||
| request_headers_ = &headers; | ||
| initiateCall(headers); | ||
| return state_ == State::Calling ? Http::FilterHeadersStatus::StopIteration | ||
| : Http::FilterHeadersStatus::Continue; | ||
|
|
@@ -78,14 +77,16 @@ void Filter::onDestroy() { | |
| } | ||
| } | ||
|
|
||
| void Filter::onComplete(Filters::Common::ExtAuthz::CheckStatus status) { | ||
| void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) { | ||
| ASSERT(cluster_); | ||
|
|
||
| state_ = State::Complete; | ||
|
|
||
| using Filters::Common::ExtAuthz::CheckStatus; | ||
|
|
||
| switch (status) { | ||
| const bool update_status_code = response->status_code >= enumToInt(Http::Code::Continue) && | ||
|
Member
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. Not sure I follow the logic here, can you add some comments?
Contributor
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. may be save the specific code we do want to use and base rest of the logic off it? (in case u think that will help)
Member
Author
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. Agreed! That's super confusing and, I definitely change it. The idea is that when the authz service defines a new status code for "reject/denied", this logic will checks whether the code is valid and not equal to 403. If that evaluates to true, set a custom Status Code header and ResponseStatInfo. I will simplify this. |
||
| response->status_code != enumToInt(Http::Code::Forbidden); | ||
|
|
||
| switch (response->status) { | ||
| case CheckStatus::OK: | ||
| cluster_->statsScope().counter("ext_authz.ok").inc(); | ||
| break; | ||
|
|
@@ -94,35 +95,59 @@ void Filter::onComplete(Filters::Common::ExtAuthz::CheckStatus status) { | |
| break; | ||
| case CheckStatus::Denied: | ||
| cluster_->statsScope().counter("ext_authz.denied").inc(); | ||
| Http::CodeUtility::ResponseStatInfo info{config_->scope(), | ||
| cluster_->statsScope(), | ||
| EMPTY_STRING, | ||
| enumToInt(Http::Code::Forbidden), | ||
| true, | ||
| EMPTY_STRING, | ||
| EMPTY_STRING, | ||
| EMPTY_STRING, | ||
| EMPTY_STRING, | ||
| false}; | ||
| Http::CodeUtility::ResponseStatInfo info{ | ||
| config_->scope(), | ||
| cluster_->statsScope(), | ||
| EMPTY_STRING, | ||
| (update_status_code ? response->status_code : enumToInt(Http::Code::Forbidden)), | ||
| true, | ||
| EMPTY_STRING, | ||
| EMPTY_STRING, | ||
| EMPTY_STRING, | ||
| EMPTY_STRING, | ||
| false}; | ||
| Http::CodeUtility::chargeResponseStat(info); | ||
| break; | ||
| } | ||
|
|
||
| // We fail open/fail close based of filter config | ||
| // if there is an error contacting the service. | ||
| if (status == CheckStatus::Denied || | ||
| (status == CheckStatus::Error && !config_->failureModeAllow())) { | ||
| Http::HeaderMapPtr response_headers{new Http::HeaderMapImpl(*getDeniedHeader())}; | ||
| callbacks_->encodeHeaders(std::move(response_headers), true); | ||
| if (response->status == CheckStatus::Denied || | ||
| (response->status == CheckStatus::Error && !config_->failureModeAllow())) { | ||
| Http::HeaderMapPtr response_headers; | ||
| if (update_status_code) { | ||
|
Member
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. This logic is a bit hard to follow due to the multiple reasons we can get here combined with the above code. Comments and/or restructure would be appreciated.
Member
Author
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. See my comment above. I will simplify this. |
||
| response_headers = std::make_unique<Http::HeaderMapImpl>(Http::HeaderMapImpl{ | ||
| {Http::Headers::get().Status, std::to_string(response->status_code)}}); | ||
| } else { | ||
| response_headers = std::make_unique<Http::HeaderMapImpl>(*getDeniedHeader()); | ||
| } | ||
|
|
||
| for (const auto& header : response->headers) { | ||
| response_headers->setReferenceKey(header.first, header.second); | ||
| } | ||
|
|
||
| if (response->body) { | ||
| callbacks_->encodeHeaders(std::move(response_headers), false); | ||
| callbacks_->encodeData(*response->body.get(), true); | ||
| } else { | ||
| callbacks_->encodeHeaders(std::move(response_headers), true); | ||
| } | ||
|
Contributor
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. may be wrapping the encoding of response headers and body into a function would help with parsing of the logic here.
Member
Author
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. I think so too. I will try to simplify this logic, but first I will make sure that we have tests for all the permutations. |
||
|
|
||
| callbacks_->requestInfo().setResponseFlag( | ||
| RequestInfo::ResponseFlag::UnauthorizedExternalService); | ||
| } else { | ||
| if (config_->failureModeAllow() && status == CheckStatus::Error) { | ||
| if (config_->failureModeAllow() && response->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_) { | ||
| if (response->status == CheckStatus::OK) { | ||
|
Contributor
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. The conditional check and addition of headers should be outside the check for
Member
Author
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. Oh, I see. Thanks! |
||
| for (const auto& header : response->headers) { | ||
| request_headers_->setReferenceKey(header.first, header.second); | ||
| } | ||
| } | ||
| callbacks_->continueDecoding(); | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,7 +38,9 @@ namespace ExtAuthz { | |
|
|
||
| class MockRequestCallbacks : public RequestCallbacks { | ||
| public: | ||
| MOCK_METHOD1(onComplete, void(CheckStatus status)); | ||
| void onComplete(ResponsePtr&& response) override { onComplete_(response); } | ||
|
|
||
| MOCK_METHOD1(onComplete_, void(ResponsePtr& response)); | ||
| }; | ||
|
|
||
| class ExtAuthzGrpcClientTest : public testing::Test { | ||
|
|
@@ -68,7 +70,7 @@ TEST_F(ExtAuthzGrpcClientTest, BasicOK) { | |
| auto status = response->mutable_status(); | ||
| status->set_code(Grpc::Status::GrpcStatus::Ok); | ||
| EXPECT_CALL(span_, setTag("ext_authz_status", "ext_authz_ok")); | ||
| EXPECT_CALL(request_callbacks_, onComplete(CheckStatus::OK)); | ||
| EXPECT_CALL(request_callbacks_, onComplete_(_)); | ||
| client_.onSuccess(std::move(response), span_); | ||
| } | ||
|
|
||
|
|
@@ -98,7 +100,7 @@ TEST_F(ExtAuthzGrpcClientTest, BasicDenied) { | |
| auto status = response->mutable_status(); | ||
| status->set_code(Grpc::Status::GrpcStatus::PermissionDenied); | ||
| EXPECT_CALL(span_, setTag("ext_authz_status", "ext_authz_unauthorized")); | ||
| EXPECT_CALL(request_callbacks_, onComplete(CheckStatus::Denied)); | ||
| EXPECT_CALL(request_callbacks_, onComplete_(_)); | ||
| client_.onSuccess(std::move(response), span_); | ||
| } | ||
|
|
||
|
|
@@ -108,7 +110,7 @@ TEST_F(ExtAuthzGrpcClientTest, BasicError) { | |
|
|
||
| client_.check(request_callbacks_, request, Tracing::NullSpan::instance()); | ||
|
|
||
| EXPECT_CALL(request_callbacks_, onComplete(CheckStatus::Error)); | ||
| EXPECT_CALL(request_callbacks_, onComplete_(_)); | ||
|
Contributor
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. we should be checking the Response's
Member
Author
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. I will address the tests after all the design changes. |
||
| client_.onFailure(Grpc::Status::Unknown, "", span_); | ||
| } | ||
|
|
||
|
|
||
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.
Non-actionable, but if we were to redo the APIs from the ground up, I think I would add a dedicated
HttpStatusCodeobject with the related constraints. We've done this now in a lot of places.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.
Sounds good. Should I place it here in a separated file like the
http_uri.proto? https://github.com/gsagula/envoy/tree/e6d6df09c84a189a43762a1e6839103e8d02c51e/api/envoy/api/v2/core