auth: AttributeContext utilities #3325
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| licenses(["notice"]) # Apache 2 | ||
|
|
||
| load( | ||
| "//bazel:envoy_build_system.bzl", | ||
| "envoy_cc_library", | ||
| "envoy_package", | ||
| ) | ||
|
|
||
| envoy_package() | ||
|
|
||
| envoy_cc_library( | ||
| name = "attribute_context_lib", | ||
| srcs = ["attribute_context.cc"], | ||
| hdrs = ["attribute_context.h"], | ||
| deps = [ | ||
| "//include/envoy/http:filter_interface", | ||
| "//include/envoy/http:header_map_interface", | ||
| "//source/common/http:utility_lib", | ||
| "//source/common/network:utility_lib", | ||
| "@envoy_api//envoy/service/auth/v2alpha:attribute_context_cc", | ||
| ], | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| #include "extensions/filters/common/auth/attribute_context.h" | ||
|
|
||
| #include "common/http/utility.h" | ||
| #include "common/network/utility.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Extensions { | ||
| namespace Filters { | ||
| namespace Common { | ||
| namespace Auth { | ||
|
|
||
| void AttributeContextUtils::setSourcePeer(envoy::service::auth::v2alpha::AttributeContext& context, | ||
| const Network::Connection& connection, | ||
| const std::string& service) { | ||
| setPeer(*context.mutable_source(), connection, service, false); | ||
| } | ||
|
|
||
| void AttributeContextUtils::setDestinationPeer( | ||
| envoy::service::auth::v2alpha::AttributeContext& context, | ||
| const Network::Connection& connection) { | ||
| setPeer(*context.mutable_destination(), connection, "", true); | ||
| } | ||
|
|
||
| void AttributeContextUtils::setPeer(envoy::service::auth::v2alpha::AttributeContext_Peer& peer, | ||
| const Network::Connection& connection, | ||
| const std::string& service, const bool local) { | ||
|
|
||
| auto addr = local ? connection.localAddress() : connection.remoteAddress(); | ||
| Envoy::Network::Utility::addressToProtobufAddress(*addr, *peer.mutable_address()); | ||
|
|
||
| if (!service.empty()) { | ||
| peer.set_service(service); | ||
|
There was a problem hiding this comment. This can just be set universally I think. The empty check doesn't do much. |
||
| } | ||
|
|
||
| auto ssl = const_cast<Ssl::Connection*>(connection.ssl()); | ||
|
There was a problem hiding this comment. const_cast is an anti-pattern. Why is this needed? Fix the constness of methods if needed. |
||
| if (ssl) { | ||
| std::string principal = local ? ssl->uriSanLocalCertificate() : ssl->uriSanPeerCertificate(); | ||
| if (principal.empty()) { | ||
| principal = local ? ssl->subjectLocalCertificate() : ssl->subjectPeerCertificate(); | ||
| } | ||
| peer.set_principal(principal); | ||
| } | ||
| } | ||
|
|
||
| void AttributeContextUtils::setHttpRequest( | ||
| envoy::service::auth::v2alpha::AttributeContext& context, | ||
| const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, | ||
| const Envoy::Http::HeaderMap& headers) { | ||
|
|
||
| auto req = context.mutable_request()->mutable_http(); | ||
| auto sdfc = const_cast<Envoy::Http::StreamDecoderFilterCallbacks*>(callbacks); | ||
|
There was a problem hiding this comment. nit: Please go through and make local variables longer and easier to read. We prefer easier to read code than short names. This comment applies everywhere. |
||
|
|
||
| auto start = ProtobufUtil::TimeUtil::MicrosecondsToTimestamp( | ||
| sdfc->requestInfo().startTime().time_since_epoch().count()); | ||
| context.mutable_request()->mutable_time()->MergeFrom(start); | ||
|
|
||
| auto proto = sdfc->requestInfo().protocol(); | ||
| if (proto) { | ||
| req->set_protocol(Envoy::Http::Utility::getProtocolString(proto.value())); | ||
| } | ||
|
|
||
| req->set_id(std::to_string(sdfc->streamId())); | ||
| req->set_size(sdfc->requestInfo().bytesReceived()); | ||
|
There was a problem hiding this comment. Is this capturing the entire request size or only what has been received at the time of the call? There was a problem hiding this comment. It will be at the time of the call. If ext_authz and RBAC are applied in There was a problem hiding this comment. Or look for the content-length header? There was a problem hiding this comment. I'm not sure. This could be a setting in the RBAC filter to buffer requests, although I'm not sure why we would want to use request size in RBAC at all. There was a problem hiding this comment. I can't think of a use case that RBAC uses request.size. Maybe use "content-length header" makes more sense. The request.size value should not depend on where the filter is placed. |
||
|
|
||
| req->set_method(getHeaderStr(headers.Method())); | ||
| req->set_path(getHeaderStr(headers.Path())); | ||
| req->set_host(getHeaderStr(headers.Host())); | ||
| req->set_scheme(getHeaderStr(headers.Scheme())); | ||
| // TODO(rodaine): add query & fragment fields | ||
|
|
||
| auto mutable_headers = req->mutable_headers(); | ||
| headers.iterate( | ||
|
There was a problem hiding this comment. I understand why this is being done for the external auth code, but I'm just throwing it out there now that this type of copy will not be acceptable for the inline auth code. There is no reason to copy all the headers just to look through them again. So I'm just throwing this out there now in case that changes how you want to do things. (In general we won't do a layer of indirection that is not needed for the inline code). Given that, does this shared code add value? Just trying to understand the future intent here. There was a problem hiding this comment. Hm. yeah that makes sense. Early discussions about RBAC about consuming a There was a problem hiding this comment. TBH I would abandon this change and just start writing the filter. I don't see the value in copying to an intermediary when the check logic is trivial and can be done inline in a nice way. If you find there is coding sharing possible when you go do that, that's the time to do it IMO. There was a problem hiding this comment. I understand the point to avoid copying data to an intermediary. This totally makes sense. But the benefit here is that the code is more modularized, and potentially more reusable. The goal of using AttributeContext is to standardize the interface to any authorization module. If RBAC is implemented as a standalone library, it can potentially be reused in other environment, like shared between different filters, or used in a pure gRPC environment where there is no filter concept. Also, the AttributeContext could be filled directly from connection/request info (like being done in this PR), or some fields could be filled by an authentication filter in the future. IMO, it just makes the interface cleaner. There was a problem hiding this comment. @liminw let's cross this bridge when we come to it. This feature is so simple IMO that I really think we are greatly overcomplicating it. I would like to get the filter written since we need it at Lyft and then we can always come back and refactor/share code as needed if it makes sense (given that it's an internal implementation detail and I think we did a great job of getting the proto right). |
||
| [](const Envoy::Http::HeaderEntry& e, void* ctx) { | ||
| auto mutable_headers = static_cast< | ||
| Envoy::Protobuf::Map<Envoy::ProtobufTypes::String, Envoy::ProtobufTypes::String>*>(ctx); | ||
| (*mutable_headers)[std::string(e.key().getStringView())] = | ||
| std::string(e.value().getStringView()); | ||
| return Envoy::Http::HeaderMap::Iterate::Continue; | ||
| }, | ||
| mutable_headers); | ||
| } | ||
|
|
||
| std::string AttributeContextUtils::getHeaderStr(const Envoy::Http::HeaderEntry* entry) { | ||
| return entry ? std::string(entry->value().getStringView()) : ""; | ||
| } | ||
|
|
||
| } // namespace Auth | ||
| } // namespace Common | ||
| } // namespace Filters | ||
| } // namespace Extensions | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| #pragma once | ||
|
|
||
| #include <string> | ||
|
|
||
| #include "envoy/http/filter.h" | ||
| #include "envoy/http/header_map.h" | ||
| #include "envoy/network/connection.h" | ||
| #include "envoy/service/auth/v2alpha/attribute_context.pb.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Extensions { | ||
| namespace Filters { | ||
| namespace Common { | ||
| namespace Auth { | ||
|
|
||
| class AttributeContextUtils { | ||
| public: | ||
| static void setSourcePeer(envoy::service::auth::v2alpha::AttributeContext& context, | ||
| const Network::Connection& connection, const std::string& service); | ||
| static void setDestinationPeer(envoy::service::auth::v2alpha::AttributeContext& context, | ||
| const Network::Connection& connection); | ||
| static void setHttpRequest(envoy::service::auth::v2alpha::AttributeContext& context, | ||
| const Envoy::Http::StreamDecoderFilterCallbacks* callbacks, | ||
| const Envoy::Http::HeaderMap& headers); | ||
|
|
||
| private: | ||
| static void setPeer(envoy::service::auth::v2alpha::AttributeContext_Peer& peer, | ||
| const Network::Connection& connection, const std::string& service, | ||
| const bool local); | ||
|
|
||
| static std::string getHeaderStr(const Envoy::Http::HeaderEntry* entry); | ||
|
|
||
| }; | ||
|
|
||
| } // namespace Auth | ||
| } // namespace Common | ||
| } // namespace Filters | ||
| } // namespace Extensions | ||
| } // namespace Envoy | ||
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.
Please go through and constify local variables. Many of them can be const, won't comment on all of them.