Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions docs/root/intro/version_history.rst
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ Version history
* rate-limit: added :ref:`configuration <envoy_api_field_config.filter.http.rate_limit.v2.RateLimit.rate_limited_as_resource_exhausted>`
to specify whether the `GrpcStatus` status returned should be `RESOURCE_EXHAUSTED` or
`UNAVAILABLE` when a gRPC call is rate limited.
* rbac: added dynamic metadata to the network level filter.
* rbac: added support for permission matching by :ref:`requested server name <envoy_api_field_config.rbac.v2alpha.Permission.requested_server_name>`.
* redis: static cluster configuration is no longer required. Redis proxy will work with clusters
delivered via CDS.
Expand Down
1 change: 1 addition & 0 deletions source/extensions/filters/common/rbac/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ envoy_cc_library(
deps = [
":engine_lib",
"//include/envoy/stats:stats_macros",
"//source/common/singleton:const_singleton",
"@envoy_api//envoy/config/filter/http/rbac/v2:rbac_cc",
"@envoy_api//envoy/config/filter/network/rbac/v2:rbac_cc",
],
Expand Down
7 changes: 6 additions & 1 deletion source/extensions/filters/common/rbac/engine.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,13 @@ class RoleBasedAccessControlEngine {
* Returns whether or not the current action is permitted.
*
* @param connection the downstream connection used to identify the action/principal.
* @param metadata the metadata with additional information about the action/principal.
* @param effective_policy_id it will be filled by the matching policy's ID,
* which is used to identity the source of the allow/deny.
*/
virtual bool allowed(const Network::Connection& connection) const PURE;
virtual bool allowed(const Network::Connection& connection,
const envoy::api::v2::core::Metadata& metadata,
std::string* effective_policy_id) const PURE;
};

} // namespace RBAC
Expand Down
9 changes: 4 additions & 5 deletions source/extensions/filters/common/rbac/engine_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,11 @@ bool RoleBasedAccessControlEngineImpl::allowed(const Network::Connection& connec
return matched == allowed_if_matched_;
}

bool RoleBasedAccessControlEngineImpl::allowed(const Network::Connection& connection) const {
bool RoleBasedAccessControlEngineImpl::allowed(const Network::Connection& connection,
const envoy::api::v2::core::Metadata& metadata,
std::string* effective_policy_id) const {
static const Http::HeaderMapImpl* empty_header = new Http::HeaderMapImpl();
static const envoy::api::v2::core::Metadata* empty_metadata =
new envoy::api::v2::core::Metadata();

return allowed(connection, *empty_header, *empty_metadata, nullptr);
return allowed(connection, *empty_header, metadata, effective_policy_id);
}

} // namespace RBAC
Expand Down
4 changes: 3 additions & 1 deletion source/extensions/filters/common/rbac/engine_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@ class RoleBasedAccessControlEngineImpl : public RoleBasedAccessControlEngine {
const envoy::api::v2::core::Metadata& metadata,
std::string* effective_policy_id) const override;

bool allowed(const Network::Connection& connection) const override;
bool allowed(const Network::Connection& connection,
const envoy::api::v2::core::Metadata& metadata,
std::string* effective_policy_id) const override;

private:
const bool allowed_if_matched_;
Expand Down
9 changes: 9 additions & 0 deletions source/extensions/filters/common/rbac/utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
#include "envoy/config/filter/network/rbac/v2/rbac.pb.h"
#include "envoy/stats/stats_macros.h"

#include "common/singleton/const_singleton.h"

#include "extensions/filters/common/rbac/engine_impl.h"

namespace Envoy {
Expand All @@ -12,6 +14,13 @@ namespace Filters {
namespace Common {
namespace RBAC {

class DynamicMetadataKeys {
public:
const std::string ShadowPolicyIdField{"shadow_effective_policyID"};
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.

Does this key and the other keys in this file need to be documented somewhere? Is the idea here that this is meant to be sent in logging data? If we have well known metadata keys I think they should be documented somewhere?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It may be useful to document these constants as other filters may be able to augment Envoy's behavior based on these. I think that should be done along with the rename suggested in this comment in another PR.

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.

OK if you plan on doing a follow up, SGTM.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Will do. Let me know if you have suggestions on where to document these.

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'm not sure, maybe a new section on "well known dynamic metadata" under here: https://www.envoyproxy.io/docs/envoy/latest/configuration/configuration? I would take a look and see what feels right.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Cool, thanks!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Tracked via #4981.

};

typedef ConstSingleton<DynamicMetadataKeys> DynamicMetadataKeysSingleton;

/**
* All stats for the RBAC filter. @see stats_macros.h
*/
Expand Down
22 changes: 14 additions & 8 deletions source/extensions/filters/http/rbac/rbac_filter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,14 @@ namespace Extensions {
namespace HttpFilters {
namespace RBACFilter {

static const std::string resp_code_200 = "200";
static const std::string resp_code_403 = "403";
static const std::string shadow_policy_id_field = "shadow_effective_policyID";
static const std::string shadow_resp_code_field = "shadow_response_code";
class DynamicMetadataKeys {
public:
const std::string ResponseCode200{"200"};
const std::string ResponseCode403{"403"};
const std::string ShadowResponseCodeField{"shadow_response_code"};
};

typedef ConstSingleton<DynamicMetadataKeys> DynamicMetadataKeysSingleton;

RoleBasedAccessControlFilterConfig::RoleBasedAccessControlFilterConfig(
const envoy::config::filter::http::rbac::v2::RBAC& proto_config,
Expand Down Expand Up @@ -71,25 +75,27 @@ Http::FilterHeadersStatus RoleBasedAccessControlFilter::decodeHeaders(Http::Head
config_->engine(callbacks_->route(), Filters::Common::RBAC::EnforcementMode::Shadow);

if (shadow_engine.has_value()) {
std::string shadow_resp_code = resp_code_200;
std::string shadow_resp_code = DynamicMetadataKeysSingleton::get().ResponseCode200;
if (shadow_engine->allowed(*callbacks_->connection(), headers,
callbacks_->streamInfo().dynamicMetadata(), &effective_policy_id)) {
ENVOY_LOG(debug, "shadow allowed");
config_->stats().shadow_allowed_.inc();
} else {
ENVOY_LOG(debug, "shadow denied");
config_->stats().shadow_denied_.inc();
shadow_resp_code = resp_code_403;
shadow_resp_code = DynamicMetadataKeysSingleton::get().ResponseCode403;
}

ProtobufWkt::Struct metrics;

auto& fields = *metrics.mutable_fields();
if (!effective_policy_id.empty()) {
*fields[shadow_policy_id_field].mutable_string_value() = effective_policy_id;
*fields[Filters::Common::RBAC::DynamicMetadataKeysSingleton::get().ShadowPolicyIdField]
.mutable_string_value() = effective_policy_id;
}

*fields[shadow_resp_code_field].mutable_string_value() = shadow_resp_code;
*fields[DynamicMetadataKeysSingleton::get().ShadowResponseCodeField].mutable_string_value() =
shadow_resp_code;

callbacks_->streamInfo().setDynamicMetadata(HttpFilterNames::get().Rbac, metrics);
}
Expand Down
37 changes: 34 additions & 3 deletions source/extensions/filters/network/rbac/rbac_filter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,15 @@ namespace Extensions {
namespace NetworkFilters {
namespace RBACFilter {

class DynamicMetadataKeys {
public:
const std::string EngineResultAllowed{"allowed"};
const std::string EngineResultDenied{"denied"};
const std::string ShadowEngineResultField{"shadow_engine_result"};
};

typedef ConstSingleton<DynamicMetadataKeys> DynamicMetadataKeysSingleton;

RoleBasedAccessControlFilterConfig::RoleBasedAccessControlFilterConfig(
const envoy::config::filter::network::rbac::v2::RBAC& proto_config, Stats::Scope& scope)
: stats_(Filters::Common::RBAC::generateStats(proto_config.stat_prefix(), scope)),
Expand All @@ -19,15 +28,17 @@ RoleBasedAccessControlFilterConfig::RoleBasedAccessControlFilterConfig(
Network::FilterStatus RoleBasedAccessControlFilter::onData(Buffer::Instance&, bool) {
ENVOY_LOG(
debug,
"checking connection: requestedServerName: {}, remoteAddress: {}, localAddress: {}, ssl: {}",
"checking connection: requestedServerName: {}, remoteAddress: {}, localAddress: {}, ssl: {}, "
"dynamicMetadata: {}",
callbacks_->connection().requestedServerName(),
callbacks_->connection().remoteAddress()->asString(),
callbacks_->connection().localAddress()->asString(),
callbacks_->connection().ssl()
? "uriSanPeerCertificate: " + callbacks_->connection().ssl()->uriSanPeerCertificate() +
", subjectPeerCertificate: " +
callbacks_->connection().ssl()->subjectPeerCertificate()
: "none");
: "none",
callbacks_->connection().streamInfo().dynamicMetadata().DebugString());

if (shadow_engine_result_ == Unknown) {
// TODO(quanlin): Pass the shadow engine results to other filters.
Expand All @@ -51,14 +62,32 @@ Network::FilterStatus RoleBasedAccessControlFilter::onData(Buffer::Instance&, bo
return Network::FilterStatus::Continue;
}

void RoleBasedAccessControlFilter::setDynamicMetadata(std::string shadow_engine_result,
std::string shadow_policy_id) {
ProtobufWkt::Struct metrics;
auto& fields = *metrics.mutable_fields();
if (!shadow_policy_id.empty()) {
*fields[Filters::Common::RBAC::DynamicMetadataKeysSingleton::get().ShadowPolicyIdField]
.mutable_string_value() = shadow_policy_id;
}
*fields[DynamicMetadataKeysSingleton::get().ShadowEngineResultField].mutable_string_value() =
shadow_engine_result;
callbacks_->connection().streamInfo().setDynamicMetadata(NetworkFilterNames::get().Rbac, metrics);
}

EngineResult
RoleBasedAccessControlFilter::checkEngine(Filters::Common::RBAC::EnforcementMode mode) {
const auto& engine = config_->engine(mode);
if (engine.has_value()) {
if (engine->allowed(callbacks_->connection())) {
std::string effective_policy_id;
if (engine->allowed(callbacks_->connection(),
callbacks_->connection().streamInfo().dynamicMetadata(),
&effective_policy_id)) {
if (mode == Filters::Common::RBAC::EnforcementMode::Shadow) {
ENVOY_LOG(debug, "shadow allowed");
config_->stats().shadow_allowed_.inc();
setDynamicMetadata(DynamicMetadataKeysSingleton::get().EngineResultAllowed,
effective_policy_id);
} else if (mode == Filters::Common::RBAC::EnforcementMode::Enforced) {
ENVOY_LOG(debug, "enforced allowed");
config_->stats().allowed_.inc();
Expand All @@ -68,6 +97,8 @@ RoleBasedAccessControlFilter::checkEngine(Filters::Common::RBAC::EnforcementMode
if (mode == Filters::Common::RBAC::EnforcementMode::Shadow) {
ENVOY_LOG(debug, "shadow denied");
config_->stats().shadow_denied_.inc();
setDynamicMetadata(DynamicMetadataKeysSingleton::get().EngineResultDenied,
effective_policy_id);
} else if (mode == Filters::Common::RBAC::EnforcementMode::Enforced) {
ENVOY_LOG(debug, "enforced denied");
config_->stats().denied_.inc();
Expand Down
3 changes: 3 additions & 0 deletions source/extensions/filters/network/rbac/rbac_filter.h
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ typedef std::shared_ptr<RoleBasedAccessControlFilterConfig>
*/
class RoleBasedAccessControlFilter : public Network::ReadFilter,
public Logger::Loggable<Logger::Id::rbac> {

public:
RoleBasedAccessControlFilter(RoleBasedAccessControlFilterConfigSharedPtr config)
: config_(config) {}
Expand All @@ -58,6 +59,8 @@ class RoleBasedAccessControlFilter : public Network::ReadFilter,
callbacks_ = &callbacks;
}

void setDynamicMetadata(std::string shadow_engine_result, std::string shadow_policy_id);

private:
RoleBasedAccessControlFilterConfigSharedPtr config_;
Network::ReadFilterCallbacks* callbacks_{};
Expand Down
4 changes: 3 additions & 1 deletion test/extensions/filters/common/rbac/mocks.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@ class MockEngine : public RoleBasedAccessControlEngineImpl {
bool(const Envoy::Network::Connection&, const Envoy::Http::HeaderMap&,
const envoy::api::v2::core::Metadata&, std::string* effective_policy_id));

MOCK_CONST_METHOD1(allowed, bool(const Envoy::Network::Connection&));
MOCK_CONST_METHOD3(allowed,
bool(const Envoy::Network::Connection&, const envoy::api::v2::core::Metadata&,
std::string* effective_policy_id));
};

} // namespace RBAC
Expand Down
1 change: 1 addition & 0 deletions test/extensions/filters/network/rbac/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ envoy_extension_cc_test(
extension_name = "envoy.filters.network.rbac",
deps = [
"//source/extensions/filters/common/rbac:utility_lib",
"//source/extensions/filters/network:well_known_names",
"//source/extensions/filters/network/rbac:rbac_filter",
"//test/mocks/network:network_mocks",
],
Expand Down
20 changes: 20 additions & 0 deletions test/extensions/filters/network/rbac/filter_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include "extensions/filters/common/rbac/utility.h"
#include "extensions/filters/network/rbac/rbac_filter.h"
#include "extensions/filters/network/well_known_names.h"

#include "test/mocks/network/mocks.h"

Expand Down Expand Up @@ -44,6 +45,9 @@ class RoleBasedAccessControlNetworkFilterTest : public testing::Test {
}

RoleBasedAccessControlNetworkFilterTest() : config_(setupConfig()) {
EXPECT_CALL(callbacks_, connection()).WillRepeatedly(ReturnRef(callbacks_.connection_));
EXPECT_CALL(callbacks_.connection_, streamInfo()).WillRepeatedly(ReturnRef(stream_info_));

filter_ = std::make_unique<RoleBasedAccessControlFilter>(config_);
filter_->initializeReadFilterCallbacks(callbacks_);
}
Expand All @@ -59,7 +63,17 @@ class RoleBasedAccessControlNetworkFilterTest : public testing::Test {
.WillByDefault(Return(requested_server_name_));
}

void setMetadata() {
ON_CALL(stream_info_, setDynamicMetadata(NetworkFilterNames::get().Rbac, _))
.WillByDefault(Invoke([this](const std::string&, const ProtobufWkt::Struct& obj) {
stream_info_.metadata_.mutable_filter_metadata()->insert(
Protobuf::MapPair<Envoy::ProtobufTypes::String, ProtobufWkt::Struct>(
NetworkFilterNames::get().Rbac, obj));
}));
}

NiceMock<Network::MockReadFilterCallbacks> callbacks_;
NiceMock<Envoy::StreamInfo::MockStreamInfo> stream_info_;
Stats::IsolatedStoreImpl store_;
Buffer::OwnedImpl data_;
RoleBasedAccessControlFilterConfigSharedPtr config_;
Expand Down Expand Up @@ -115,6 +129,7 @@ TEST_F(RoleBasedAccessControlNetworkFilterTest, AllowedWithNoPolicy) {

TEST_F(RoleBasedAccessControlNetworkFilterTest, Denied) {
setDestinationPort(456);
setMetadata();

EXPECT_CALL(callbacks_.connection_, close(Network::ConnectionCloseType::NoFlush)).Times(2);

Expand All @@ -125,6 +140,11 @@ TEST_F(RoleBasedAccessControlNetworkFilterTest, Denied) {
EXPECT_EQ(1U, config_->stats().denied_.value());
EXPECT_EQ(1U, config_->stats().shadow_allowed_.value());
EXPECT_EQ(0U, config_->stats().shadow_denied_.value());

auto filter_meta =
stream_info_.dynamicMetadata().filter_metadata().at(NetworkFilterNames::get().Rbac);
EXPECT_EQ("bar", filter_meta.fields().at("shadow_effective_policyID").string_value());
EXPECT_EQ("allowed", filter_meta.fields().at("shadow_engine_result").string_value());
}

} // namespace RBACFilter
Expand Down