Skip to content
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
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
18 changes: 18 additions & 0 deletions include/envoy/ext_authz/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
licenses(["notice"]) # Apache 2

load(
"//bazel:envoy_build_system.bzl",
"envoy_cc_library",
"envoy_package",
)

envoy_package()

envoy_cc_library(
name = "ext_authz_interface",
hdrs = ["ext_authz.h"],
deps = [
"//include/envoy/tracing:http_tracer_interface",
"@envoy_api//envoy/service/auth/v2:external_auth_cc",
],
)
67 changes: 67 additions & 0 deletions include/envoy/ext_authz/ext_authz.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
#pragma once

#include <chrono>
#include <memory>
#include <string>
#include <vector>

#include "envoy/common/pure.h"
#include "envoy/service/auth/v2/external_auth.pb.h"
#include "envoy/tracing/http_tracer.h"

namespace Envoy {
namespace ExtAuthz {

/**
* Possible async results for a check call.
*/
enum class CheckStatus {
// The request is authorized.
OK,
// The authz service could not be queried.
Error,
// The request is denied.
Denied
};

/**
* Async callbacks used during check() calls.
*/
class RequestCallbacks {
public:
virtual ~RequestCallbacks() {}

/**
* Called when a check request is complete. The resulting status is supplied.
*/
virtual void onComplete(CheckStatus status) PURE;
};

class Client {
public:
// Destructor
virtual ~Client() {}

/**
* Cancel an inflight Check request.
*/
virtual void cancel() PURE;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Question: can there only ever be a single inflight request per client with this interface? Does this imply that each request must have its own Client?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thinking about this a bit more, maybe we don't need to do the TLS thing at all here. This is unary RPC, so we don't have a significant cost in rebuilding the client each time, it should be a fairly small construction/destruction cost. I think what you have in this PR is fine, I would be more concerned if we had a bidi streaming auth check.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I was thinking that the per-thread gRPC would save us setup time and hence would help even in the unary gRPC case. But if the cluster is going to do that for us then I agree changing it would not buy us much.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, for Envoy gRPC, this will come for free from the cluster pool management. In theory, Google gRPC also does this, based on good authority, but I haven't validated. Let's work on your other PRs first and we can circle back to this later.


/**
* Request a check call to an external authorization service which can use the
* passed request parameters to make a permit/deny decision.
* @param callback supplies the completion callbacks.
* NOTE: The callback may happen within the calling stack.
* @param request is the proto message with the attributes of the specific payload.
* @param parent_span source for generating an egress child span as part of the trace.
*
*/
virtual void check(RequestCallbacks& callback,
const envoy::service::auth::v2::CheckRequest& request,
Tracing::Span& parent_span) PURE;
};

typedef std::unique_ptr<Client> ClientPtr;

} // namespace ExtAuthz
} // namespace Envoy
4 changes: 3 additions & 1 deletion include/envoy/request_info/request_info.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,10 @@ enum ResponseFlag {
FaultInjected = 0x400,
// Request was ratelimited locally by rate limit filter.
RateLimited = 0x800,
// Request was unauthorized.
Unauthorized = 0x1000,
// ATTENTION: MAKE SURE THIS REMAINS EQUAL TO THE LAST FLAG.
LastFlag = RateLimited
LastFlag = Unauthorized
};

/**
Expand Down
2 changes: 1 addition & 1 deletion source/common/access_log/grpc_access_log_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ void HttpGrpcAccessLog::responseFlagsToAccessLogResponseFlags(
envoy::config::filter::accesslog::v2::AccessLogCommon& common_access_log,
const RequestInfo::RequestInfo& request_info) {

static_assert(RequestInfo::ResponseFlag::LastFlag == 0x800,
static_assert(RequestInfo::ResponseFlag::LastFlag == 0x1000,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems like gRPC access logging protos should be enhanced for unauthorized? Can you add a TODO and take care of this in one of your follow ups?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes. I'll do it in the follow up pr; thanks.

"A flag has been added. Fix this code.");

if (request_info.getResponseFlag(RequestInfo::ResponseFlag::FailedLocalHealthCheck)) {
Expand Down
9 changes: 7 additions & 2 deletions source/common/config/well_known_names.h
Original file line number Diff line number Diff line change
Expand Up @@ -78,13 +78,15 @@ class NetworkFilterNameValues {
const std::string REDIS_PROXY = "envoy.redis_proxy";
// IP tagging filter
const std::string TCP_PROXY = "envoy.tcp_proxy";
// Authorization filter
const std::string EXT_AUTHORIZATION = "envoy.ext_authz";

// Converts names from v1 to v2
const V1Converter v1_converter_;

NetworkFilterNameValues()
: v1_converter_({CLIENT_SSL_AUTH, ECHO, HTTP_CONNECTION_MANAGER, MONGO_PROXY, RATE_LIMIT,
REDIS_PROXY, TCP_PROXY}) {}
REDIS_PROXY, TCP_PROXY, EXT_AUTHORIZATION}) {}
};

typedef ConstSingleton<NetworkFilterNameValues> NetworkFilterNames;
Expand Down Expand Up @@ -131,13 +133,16 @@ class HttpFilterNameValues {
const std::string LUA = "envoy.lua";
// Squash filter
const std::string SQUASH = "envoy.squash";
// External Authorization filter
const std::string EXT_AUTHORIZATION = "envoy.ext_authz";

// Converts names from v1 to v2
const V1Converter v1_converter_;

HttpFilterNameValues()
: v1_converter_({BUFFER, CORS, DYNAMO, FAULT, GRPC_HTTP1_BRIDGE, GRPC_JSON_TRANSCODER,
GRPC_WEB, HEALTH_CHECK, IP_TAGGING, RATE_LIMIT, ROUTER, LUA}) {}
GRPC_WEB, HEALTH_CHECK, IP_TAGGING, RATE_LIMIT, ROUTER, LUA,
EXT_AUTHORIZATION}) {}
};

typedef ConstSingleton<HttpFilterNameValues> HttpFilterNames;
Expand Down
35 changes: 35 additions & 0 deletions source/common/ext_authz/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
licenses(["notice"]) # Apache 2

load(
"//bazel:envoy_build_system.bzl",
"envoy_cc_library",
"envoy_package",
)

envoy_package()

envoy_cc_library(
name = "ext_authz_lib",
srcs = ["ext_authz_impl.cc"],
hdrs = ["ext_authz_impl.h"],
deps = [
"//include/envoy/ext_authz:ext_authz_interface",
"//include/envoy/grpc:async_client_interface",
"//include/envoy/grpc:async_client_manager_interface",
"//include/envoy/http:filter_interface",
"//include/envoy/http:header_map_interface",
"//include/envoy/http:protocol_interface",
"//include/envoy/network:address_interface",
"//include/envoy/network:connection_interface",
"//include/envoy/network:filter_interface",
"//include/envoy/ssl:connection_interface",
"//include/envoy/upstream:cluster_manager_interface",
"//source/common/common:assert_lib",
"//source/common/grpc:async_client_lib",
"//source/common/http:headers_lib",
"//source/common/http:utility_lib",
"//source/common/network:utility_lib",
"//source/common/protobuf",
"//source/common/tracing:http_tracer_lib",
],
)
191 changes: 191 additions & 0 deletions source/common/ext_authz/ext_authz_impl.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,191 @@
#include "common/ext_authz/ext_authz_impl.h"

#include <chrono>
#include <cstdint>
#include <string>
#include <vector>

#include "envoy/access_log/access_log.h"
#include "envoy/ssl/connection.h"

#include "common/common/assert.h"
#include "common/grpc/async_client_impl.h"
#include "common/http/headers.h"
#include "common/http/utility.h"
#include "common/network/utility.h"
#include "common/protobuf/protobuf.h"

#include "fmt/format.h"

namespace Envoy {
namespace ExtAuthz {

GrpcClientImpl::GrpcClientImpl(Grpc::AsyncClientPtr&& async_client,
const Optional<std::chrono::milliseconds>& timeout)
: service_method_(*Protobuf::DescriptorPool::generated_pool()->FindMethodByName(
"envoy.service.auth.v2.Authorization.Check")),
async_client_(std::move(async_client)), timeout_(timeout) {}

GrpcClientImpl::~GrpcClientImpl() { ASSERT(!callbacks_); }

void GrpcClientImpl::cancel() {
ASSERT(callbacks_ != nullptr);
request_->cancel();
callbacks_ = nullptr;
}

void GrpcClientImpl::check(RequestCallbacks& callbacks,
const envoy::service::auth::v2::CheckRequest& request,
Tracing::Span& parent_span) {
ASSERT(callbacks_ == nullptr);
callbacks_ = &callbacks;

request_ = async_client_->send(service_method_, request, *this, parent_span, timeout_);
}

void GrpcClientImpl::onSuccess(std::unique_ptr<envoy::service::auth::v2::CheckResponse>&& response,
Tracing::Span& span) {
CheckStatus status = CheckStatus::OK;
ASSERT(response->status().code() != Grpc::Status::GrpcStatus::Unknown);
if (response->status().code() != Grpc::Status::GrpcStatus::Ok) {
status = CheckStatus::Denied;
span.setTag(Constants::get().TraceStatus, Constants::get().TraceUnauthz);
} else {
span.setTag(Constants::get().TraceStatus, Constants::get().TraceOk);
}

callbacks_->onComplete(status);
callbacks_ = nullptr;
}

void GrpcClientImpl::onFailure(Grpc::Status::GrpcStatus status, const std::string&,
Tracing::Span&) {
ASSERT(status != Grpc::Status::GrpcStatus::Ok);
UNREFERENCED_PARAMETER(status);
callbacks_->onComplete(CheckStatus::Error);
callbacks_ = nullptr;
}

void CreateCheckRequest::setAttrContextPeer(envoy::service::auth::v2::AttributeContext_Peer& peer,
const Network::Connection& connection,
const std::string& service, const bool local) {

// Set the address
auto addr = peer.mutable_address();
if (!local) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: can you invert this such that it reads if (local) {, same below, it's a bit cleaner.

Envoy::Network::Utility::addressToProtobufAddress(*connection.remoteAddress(), *addr);
} else {
Envoy::Network::Utility::addressToProtobufAddress(*connection.localAddress(), *addr);
}

// Set the principal
// Preferably the SAN from the peer's cert or
// Subject from the peer's cert.
std::string principal;
Ssl::Connection* ssl = const_cast<Ssl::Connection*>(connection.ssl());
if (ssl != nullptr) {
if (!local) {
principal = ssl->uriSanPeerCertificate();

if (principal.empty()) {
principal = ssl->subjectPeerCertificate();
}
} else {
principal = ssl->uriSanLocalCertificate();

if (principal.empty()) {
principal = ssl->subjectLocalCertificate();
}
}
}
peer.set_principal(principal);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We probably need some format normalization here. At least document what is the format used here?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: you're actually doing an additional string copy here that could be skipped if you did the set_principal directly in the if cases.


if (!service.empty()) {
peer.set_service(service);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Also document the format?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@wora: The fulfillment of the AttributeContext is as-per the documentation in the data-plane-api specification 1. The code here is adhering to that interface agreement.
I can make a note of that at the top of this class.

}
}

std::string CreateCheckRequest::getHeaderStr(const Envoy::Http::HeaderEntry* entry) {
if (entry) {
return entry->value().getString();
}
return "";
}

void CreateCheckRequest::setHttpRequest(
::envoy::service::auth::v2::AttributeContext_HttpRequest& httpreq,
const Envoy::Http::StreamDecoderFilterCallbacks* callbacks,
const Envoy::Http::HeaderMap& headers) {

// Set id
// The streamId is not qualified as a const. Although it is as it does not modify the object.
Envoy::Http::StreamDecoderFilterCallbacks* sdfc =
const_cast<Envoy::Http::StreamDecoderFilterCallbacks*>(callbacks);
httpreq.set_id(std::to_string(sdfc->streamId()));

// Set method
httpreq.set_method(getHeaderStr(headers.Method()));
// Set path
httpreq.set_path(getHeaderStr(headers.Path()));
// Set host
httpreq.set_host(getHeaderStr(headers.Host()));
// Set scheme
httpreq.set_scheme(getHeaderStr(headers.Scheme()));

// Set size
// need to convert to google buffer 64t;
httpreq.set_size(sdfc->requestInfo().bytesReceived());

// Set protocol
if (sdfc->requestInfo().protocol().valid()) {
httpreq.set_protocol(
Envoy::Http::Utility::getProtocolString(sdfc->requestInfo().protocol().value()));
}

// Fill in the headers
auto mutable_headers = httpreq.mutable_headers();
headers.iterate(
[](const Envoy::Http::HeaderEntry& e, void* ctx) {
Envoy::Protobuf::Map<::std::string, ::std::string>* mutable_headers =
static_cast<Envoy::Protobuf::Map<::std::string, ::std::string>*>(ctx);
(*mutable_headers)[e.key().getString()] = e.value().getString();
return Envoy::Http::HeaderMap::Iterate::Continue;
},
mutable_headers);
}

@htuch htuch Jan 25, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it would be instructive to compare with https://github.com/envoyproxy/envoy/blob/master/source/common/access_log/grpc_access_log_impl.cc#L251. There is a lot of overlap between the access log proto data and construction and the auth request.

As a non-actionable reflection, it would have been great if we could have converged the common objects for both access logs and auth, but I don't think this is going to happen (@mattklein123 @wora).

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@htuch This really depends on how big the scale we are targeting. If we target millions of services, billions of requests per second, we can surely refactor the code, but it will take quite a bit work.

The general approach would be something like this:

  • Reach an agreement on the standard Envoy attributes, and define them as a proto like AttributeContext.
  • Envoy natively generate the standard attributes.
  • Design the standard logs, metrics, events, traces , and other telemetry schemas.
  • Generate standard telemetry data based on the standard attributes.
  • Optional. Generate legacy telemetry data, such as access logs, from the standard attributes.

New users of Envoy won't need the legacy telemetry data, so it would just be disabled by a flag. We can delete the legacy code after a year, or just keep them on the side.

This refactoring would only be useful if we can agree on a rich set of standard telemetry data that appeal to wide range of users, so a customer can correlate telemetry data across services written by different vendors.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@htuch Ref to the first paragraph, are you suggesting that I write this in a way that it's done in https://github.com/envoyproxy/envoy/blob/master/source/common/access_log/grpc_access_log_impl.cc#L251.
I am happy to do that but just want to confirm.

Separately, thanks for all the other review comments. I am working on making those changes.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@wora agreed, I'm just waxing philosophical.

@saumoh just suggesting you give that code a pass and see if there's anything in there that your code would benefit from following.


void CreateCheckRequest::setAttrContextRequest(
::envoy::service::auth::v2::AttributeContext_Request& req,
const Envoy::Http::StreamDecoderFilterCallbacks* callbacks,
const Envoy::Http::HeaderMap& headers) {
setHttpRequest(*req.mutable_http(), callbacks, headers);
}

void CreateCheckRequest::createHttpCheck(const Envoy::Http::StreamDecoderFilterCallbacks* callbacks,
const Envoy::Http::HeaderMap& headers,
envoy::service::auth::v2::CheckRequest& request) {

auto attrs = request.mutable_attributes();

Envoy::Http::StreamDecoderFilterCallbacks* cb =
const_cast<Envoy::Http::StreamDecoderFilterCallbacks*>(callbacks);

std::string service = getHeaderStr(headers.EnvoyDownstreamServiceCluster());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: const std::string


setAttrContextPeer(*attrs->mutable_source(), *cb->connection(), service, false);
setAttrContextPeer(*attrs->mutable_destination(), *cb->connection(), "", true);
setAttrContextRequest(*attrs->mutable_request(), callbacks, headers);
}

void CreateCheckRequest::createTcpCheck(const Network::ReadFilterCallbacks* callbacks,
envoy::service::auth::v2::CheckRequest& request) {

auto attrs = request.mutable_attributes();

Network::ReadFilterCallbacks* cb = const_cast<Network::ReadFilterCallbacks*>(callbacks);
setAttrContextPeer(*attrs->mutable_source(), cb->connection(), "", false);
setAttrContextPeer(*attrs->mutable_destination(), cb->connection(), "", true);
}

} // namespace ExtAuthz
} // namespace Envoy
Loading