rbac: add rbac network filter. #4083

yangminzhu · 2018-08-08T00:58:03Z

Description:

Added a new RBAC network filter using the same config proto.
Refactored the RBAC HTTP filter to share some common config code.
Updated the RBAC matcher construction to ignore header and metadata for RBAC network filter.

Risk Level: Medium.

Testing: Added unit test and integration test for the new network filter. Also tested manually with Envoy built with the RBAC network filter.

Docs Changes: Added network filter docs.

Release Notes: Added for the RBAC network filter.

Fixes #3993

/cc @quanjielin I add a TODO for collecting metrics for shadow rules in the RBAC network filter.
/cc @liminw @lizan @mattklein123

Signed-off-by: Yangmin Zhu ymzhu@google.com

Signed-off-by: Yangmin Zhu <ymzhu@google.com>

lizan

a quick pass, will do another pass later today or tomorrow

lizan · 2018-08-08T01:05:22Z

api/envoy/config/filter/network/rbac/v2/rbac.proto

+  // If absent, no enforcing RBAC policy will be applied.
+  config.rbac.v2alpha.RBAC rules = 1;
+
+  // Shadow rules are not enforced by the filter (i.e., returning a 403)


not 403 for network filter.

lizan · 2018-08-08T01:09:58Z

source/extensions/filters/common/rbac/config.cc

+    const envoy::config::filter::http::rbac::v2::RBAC& proto_config,
+    const std::string& stats_prefix, Stats::Scope& scope)
+    : stats_(generateStats(stats_prefix, scope)),
+      INIT_RBAC_ENGINES(proto_config, false /* disable_http_rules */) {}


Why do you need the INIT_RBAC_ENGINES macro? You can call the other constructors here. Or extract common part to other class, commented in header.

The main reason is the type of proto_config are different.
For RoleBasedAccessControlRouteSpecificFilterConfig, the type is RBACPerRoute;
For RoleBasedAccessControlFilterConfig, the type is http::rbac::v2::RBAC and network::rbac::v2::RBAC respectively.
Do you prefer a template function instead of the INIT_RBAC_ENGINES macro?

yeah I would prefer a template function

lizan · 2018-08-08T01:12:00Z

source/extensions/filters/common/rbac/config.h

+
+enum class EnforcementMode { Enforced, Shadow };
+
+class RoleBasedAccessControlRouteSpecificFilterConfig : public Router::RouteSpecificFilterConfig {


these two classes are pretty similar, you might want to combine them, or extract common part out.

It's copied directly from the existing implementation. I remember there had been some discussions in previous PR to combine them but later decided to not to do so. Also looked at the content, it seems there is not too much thing to share, do you have any specific suggestions here?

hmm, ok then I would not combine those config classes here, rather keep them separately.

qiwzhang · 2018-08-08T01:42:31Z

source/extensions/filters/network/rbac/rbac_filter.cc

+                callbacks_->connection().ssl()->subjectPeerCertificate()
+          : "none");
+
+  const Envoy::Http::HeaderMapImpl empty_header;


onData is called multiple times. we should only do rbac check on the first one.

I can cache the check result, but just curious could it have any problems with the cache? I assume the cert, ip and port won't change on following onData()? Also whenever a newer RBAC config is pushed, envoy will create a new network filter, so it's also no problem to cache the result?

Envoy will call onData() for each buffer it receives. But you don't need to call rbac check on every buffer.

qiwzhang · 2018-08-08T01:43:22Z

source/extensions/filters/network/rbac/config.cc

+    const envoy::config::filter::network::rbac::v2::RBAC& proto_config,
+    Server::Configuration::FactoryContext& context) {
+  Filters::Common::RBAC::RoleBasedAccessControlFilterConfigSharedPtr config(
+      new Filters::Common::RBAC::RoleBasedAccessControlFilterConfig(proto_config, context.scope()));


use make_shared

Signed-off-by: Yangmin Zhu <ymzhu@google.com>

qiwzhang · 2018-08-08T23:24:30Z

LGTM

lizan · 2018-08-08T23:30:45Z

source/extensions/filters/common/rbac/config.cc

+    const envoy::config::filter::http::rbac::v2::RBAC& proto_config,
+    const std::string& stats_prefix, Stats::Scope& scope)
+    : stats_(generateStats(stats_prefix, scope)),
+      INIT_RBAC_ENGINES(proto_config, false /* disable_http_rules */) {}


yeah I would prefer a template function

lizan · 2018-08-08T23:35:54Z

source/extensions/filters/common/rbac/engine.h

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


https://google.github.io/styleguide/cppguide.html#Default_Arguments

Default arguments are banned on virtual functions, where they don't work properly, and in cases where the specified default might not evaluate to the same value depending on when it was evaluated.

I see, thanks for the reference!

lizan · 2018-08-08T23:38:34Z

source/extensions/filters/common/rbac/config.h

+
+enum class EnforcementMode { Enforced, Shadow };
+
+class RoleBasedAccessControlRouteSpecificFilterConfig : public Router::RouteSpecificFilterConfig {


hmm, ok then I would not combine those config classes here, rather keep them separately.

Signed-off-by: Yangmin Zhu <ymzhu@google.com>

lizan · 2018-08-09T22:39:16Z

source/extensions/filters/network/rbac/rbac_filter.cc

+RoleBasedAccessControlFilter::checkEngine(Filters::Common::RBAC::EnforcementMode mode) {
+  const auto& engine = config_->engine(nullptr, NetworkFilterNames::get().Rbac, mode);
+  if (engine.has_value()) {
+    if (engine->allowed(callbacks_->connection(), Envoy::Http::HeaderMapImpl(),


not a fan of leaking Http::HeaderMapImpl into network filters, can you add another allowed in to engine interface and call it?

lizan · 2018-08-09T22:43:11Z

source/extensions/filters/common/rbac/matchers.cc

  case envoy::config::rbac::v2alpha::Permission::RuleCase::kHeader:
+    if (disable_http_rules) {


Thinking about this again, why do we need disable_http_rules and ignore HTTP headers? When a HeaderMatch specified in TCP filter, shouldn't it just reject the configuration?

It makes sense for me to treat this simply as a deny, @liminw WDYT?

Agree. If a rules says "allow requests with header a=foo", the rules does not match in TCP filter.

Updated to reject invalid rules in config.cc

lizan · 2018-08-09T22:44:00Z

source/extensions/filters/common/rbac/config.h

+                                   : absl::nullopt;
+}
+
+class RoleBasedAccessControlRouteSpecificFilterConfig : public Router::RouteSpecificFilterConfig {


I don't think you should move this to Common, it is HTTP specific.

Moved back to the http filer.

lizan · 2018-08-09T22:50:11Z

source/extensions/filters/common/rbac/config.h

+/**
+ * Configuration for the RBAC filter.
+ */
+class RoleBasedAccessControlFilterConfig {


Still not convinced we should combine HTTP RBAC and TCP RBAC config in same class. You can have template functions and stats macros above in common/rbac/config_utility. having a filter_name in engine() below seems hacky.

Okay, Moved back to http and network filter respectively, renamed config to utility.

Signed-off-by: Yangmin Zhu <ymzhu@google.com>

liminw · 2018-08-09T20:55:07Z

api/envoy/config/filter/network/rbac/v2/rbac.proto

@@ -0,0 +1,31 @@
+syntax = "proto3";
+
+package envoy.config.filter.network.rbac.v2;


How is version defined? Is "v2" the correct version? @lizan @mattklein123

It's defined the same to rbac http proto

yes v2 is correct, all envoy protos are under v2 (or v2alpha, v2beta)

liminw · 2018-08-09T21:06:04Z

api/envoy/config/rbac/v2alpha/rbac.proto

@@ -159,7 +160,8 @@ message Principal {
    // A header (or psuedo-header such as :path or :method) on the incoming HTTP request.
    envoy.api.v2.route.HeaderMatcher header = 6;


"only available for HTTP requests" comments apply here as well.

liminw · 2018-08-09T21:06:58Z

docs/root/configuration/network_filters/rbac_filter.rst

+The RBAC network filter is used to authorize actions (permissions) by identified downstream clients
+(principals). This is useful to explicitly manage callers to an application and protect it from
+unexpected or forbidden agents. The filter supports configuration with either a safe-list (ALLOW) or
+block-list (DENY) set of policies based off properties of the connection (IPs, ports, SSL subject).


based off => based on?

liminw · 2018-08-09T21:07:25Z

docs/root/configuration/network_filters/rbac_filter.rst

+(principals). This is useful to explicitly manage callers to an application and protect it from
+unexpected or forbidden agents. The filter supports configuration with either a safe-list (ALLOW) or
+block-list (DENY) set of policies based off properties of the connection (IPs, ports, SSL subject).
+This filter also supports policy in both enforcement and shadow mode, shadow mode won't effect real


mode => modes.

shadow mode => Shadow mode (New sentence starts)

liminw · 2018-08-10T00:42:31Z

source/extensions/filters/common/rbac/matchers.cc

  case envoy::config::rbac::v2alpha::Permission::RuleCase::kHeader:
+    if (disable_http_rules) {


Agree. If a rules says "allow requests with header a=foo", the rules does not match in TCP filter.

Signed-off-by: Yangmin Zhu <ymzhu@google.com>

yangminzhu · 2018-08-10T03:54:55Z

@lizan @liminw I have addressed comments so far, PTAL, thank you for the review.

lizan · 2018-08-10T05:52:30Z

api/envoy/config/filter/network/rbac/v2/rbac.proto

@@ -0,0 +1,31 @@
+syntax = "proto3";
+
+package envoy.config.filter.network.rbac.v2;


yes v2 is correct, all envoy protos are under v2 (or v2alpha, v2beta)

lizan · 2018-08-10T06:00:12Z

test/extensions/filters/network/rbac/integration_test.cc

+    address:
+      socket_address:
+        address: 127.0.0.1
+        port_value: 8000


do not use fixed port number for tests (they couldn't be ran parallel, and fail for port conflict, which is hard to notice)

Updated, we don't really need port in rbac config.

lizan · 2018-08-10T06:03:09Z

source/extensions/filters/common/rbac/engine_impl.cc

@@ -37,6 +39,11 @@ bool RoleBasedAccessControlEngineImpl::allowed(const Network::Connection& connec
  return matched == allowed_if_matched_;
 }

+bool RoleBasedAccessControlEngineImpl::allowed(const Network::Connection& connection) const {
+  return allowed(connection, Envoy::Http::HeaderMapImpl(), envoy::api::v2::core::Metadata(),


make the HeaderMapImpl and Metadata static variable? HeaderMapImpl does memory allocations for inline headers, constructing everytime is not good. Ideally we should make the parameter a pointer instead of a reference.

liminw · 2018-08-10T18:26:38Z

source/extensions/filters/network/rbac/rbac_filter.h

+namespace NetworkFilters {
+namespace RBACFilter {
+
+enum EngineResult { UNKNOWN, NONE, ALLOW, DENY };


When is NONE result used?

NONE means no rbac policies, later we then don't increase metrics (allowed or denied).

Sounds good. Thanks for explaining.

liminw · 2018-08-10T19:21:10Z

test/extensions/filters/network/rbac/filter_test.cc

+namespace NetworkFilters {
+namespace RBACFilter {
+
+class RoleBasedAccessControlNetworkFilterTest : public testing::Test {


Do we have any tests on identity (SAN field in cert)?

There is no such test in both http and tcp rbac filter so far.

liminw · 2018-08-10T19:22:01Z

source/extensions/filters/network/rbac/config.cc

+namespace RBACFilter {
+
+static void validateFail(const std::string& header, const std::string& metadata) {
+  throw EnvoyException(fmt::format("Found header({}) or metadata({}) rule,"


What will users see if validation fail? The request being rejected? With what error code?

The error message should be returned to the entity trying to apply the config. In Istio, this should be Pilot. There is no request at this point, this happens in config stage.

I see. Good point.

TBH I'm not sure if it's really worth doing this, but up to you. It's going to be difficult to keep this in sync when new features are added to the HTTP filter and IMO it's fine to just ignore fields that are not supported at the network layer. But don't feel that strongly about it.

Thanks Matt, I'll keep the validation logic for now and also keep an eye on this if it becomes a big problem between the HTTP filter and the network filter.

Signed-off-by: Yangmin Zhu <ymzhu@google.com>

lizan · 2018-08-10T20:20:40Z

source/extensions/filters/network/rbac/rbac_filter.h

+namespace NetworkFilters {
+namespace RBACFilter {
+
+enum EngineResult { UNKNOWN, NONE, ALLOW, DENY };


Enum values using PascalCase (e.g., RoundRobin).

https://github.com/envoyproxy/envoy/blob/master/STYLE.md

Signed-off-by: Yangmin Zhu <ymzhu@google.com>

mattklein123

Looks great. Few small comments. Please check that you have 100% code coverage on the new filter.

mattklein123 · 2018-08-13T18:22:47Z

api/envoy/config/filter/network/rbac/v2/rbac.proto

+// RBAC network filter config.
+//
+// Header and Metadata should not be used in rules/shadow_rules in RBAC network filter as
+// these information are only available in :ref:`RBAC http filter <config_http_filters_rbac>`.


s/these information are/this information is

mattklein123 · 2018-08-13T18:24:02Z

source/extensions/extensions_build_config.bzl

@@ -69,6 +69,7 @@ EXTENSIONS = {
    "envoy.filters.network.ratelimit":                  "//source/extensions/filters/network/ratelimit:config",
    "envoy.filters.network.tcp_proxy":                  "//source/extensions/filters/network/tcp_proxy:config",
    "envoy.filters.network.thrift_proxy":               "//source/extensions/filters/network/thrift_proxy:config",
+    "envoy.filters.network.rbac":                       "//source/extensions/filters/network/rbac:config",


nit: please alpha order (can you check to see if anything else is not ordered)

Also reordered HTTP filters.

mattklein123 · 2018-08-13T18:28:40Z

source/extensions/filters/network/rbac/config.cc

+namespace RBACFilter {
+
+static void validateFail(const std::string& header, const std::string& metadata) {
+  throw EnvoyException(fmt::format("Found header({}) or metadata({}) rule,"


TBH I'm not sure if it's really worth doing this, but up to you. It's going to be difficult to keep this in sync when new features are added to the HTTP filter and IMO it's fine to just ignore fields that are not supported at the network layer. But don't feel that strongly about it.

mattklein123 · 2018-08-13T18:29:55Z

source/extensions/filters/network/rbac/rbac_filter.cc

+          : "none");
+
+  if (shadow_engine_result_ == Unknown) {
+    // TODO(quanlin): Support metric collection for RBAC network filter.


Don't you check metrics below? Is this comment still valid?

Ah, sorry the comment might be a bit misleading, I actually mean to pass the metrics to other filers. In HTTP filter this is done by using metadata. But there is no metadata for TCP filter so I created this TODO.

Signed-off-by: Yangmin Zhu <ymzhu@google.com>

yangminzhu · 2018-08-13T23:47:19Z

@mattklein123
I added one more test to cover the case when there is no RBAC policy and checked the default behavior is to allow access. After this it's 100% covered except for the ENVOY_LOG statements.

lizan · 2018-08-14T00:05:53Z

docs/root/intro/version_history.rst

@@ -49,6 +49,7 @@ Version history
  boolean flag in the ratelimit configuration.
  Support for the legacy proto :repo:`source/common/ratelimit/ratelimit.proto` is deprecated and will be removed at the start of the 1.9.0 release cycle.
 * rest-api: added ability to set the :ref:`request timeout <envoy_api_field_core.ApiConfigSource.request_timeout>` for REST API requests.
+* rbac network filter: a :ref:`role-based access control network filter <config_network_filters_rbac>` has been added.


nit: this should be one line above

Signed-off-by: Yangmin Zhu <ymzhu@google.com>

mattklein123

Thanks, very nice!

* origin/master: (38 commits) test: add tests for corner-cases around sending requests before run() starts or after run() ends. (envoyproxy#4114) perf: reduce the memory usage of LC Trie construction (envoyproxy#4117) test: moving redundant code in websocket_integration_test to utilities (envoyproxy#4127) test: make YamlLoadFromStringFail less picky about error msg. (envoyproxy#4141) rbac: add rbac network filter. (envoyproxy#4083) fuzz: route lookup and header finalization fuzzer. (envoyproxy#4116) Set content-type and content-length (envoyproxy#4113) fault: use FractionalPercent for percent (envoyproxy#3978) test: Fix inverted exact match logic in IntegrationTcpClient::waitForData() (envoyproxy#4134) Added cluster_name to load assignment config for static cluster (envoyproxy#4123) ssl: refactor ContextConfig to use TlsCertificateConfig (envoyproxy#4115) syscall: refactor OsSysCalls for deeper errno latching (envoyproxy#4111) thrift_proxy: fix oneway bugs (envoyproxy#4025) Do not crash when converting YAML to JSON fails (envoyproxy#4110) config: allow unknown fields flag (take 2) (envoyproxy#4096) Use a jittered backoff strategy for handling HdsDelegate stream/connection failures (envoyproxy#4108) bazel: use GCS remote cache (envoyproxy#4050) Add thread local cache of overload action states (envoyproxy#4090) Added TCP healthcheck capabilities to the HdsDelegate (envoyproxy#4079) secret: add secret provider interface and use it for TlsCertificates (envoyproxy#4086) ... Signed-off-by: Snow Pettersen <snowp@squareup.com>

rbac: add rbac network filter.

44f9a75

Signed-off-by: Yangmin Zhu <ymzhu@google.com>

lizan self-assigned this Aug 8, 2018

lizan reviewed Aug 8, 2018

View reviewed changes

qiwzhang reviewed Aug 8, 2018

View reviewed changes

address initial comments.

a0740b4

Signed-off-by: Yangmin Zhu <ymzhu@google.com>

mattklein123 mentioned this pull request Aug 8, 2018

source_ip_access: add source IP access filter #4091

Closed

only check RBAC engine on first call to onData().

e9fe514

Signed-off-by: Yangmin Zhu <ymzhu@google.com>

lizan suggested changes Aug 8, 2018

View reviewed changes

use template function and do not use default values on virtual function.

402136b

Signed-off-by: Yangmin Zhu <ymzhu@google.com>

lizan reviewed Aug 9, 2018

View reviewed changes

do not use header_map_impl.h in network filter.

7a33dc7

Signed-off-by: Yangmin Zhu <ymzhu@google.com>

liminw reviewed Aug 10, 2018

View reviewed changes

yangminzhu added 4 commits August 9, 2018 18:17

move filter config out of common.

ed6b7ca

Signed-off-by: Yangmin Zhu <ymzhu@google.com>

remove disable_http_rules.

058db1e

Signed-off-by: Yangmin Zhu <ymzhu@google.com>

rename common/config to common/utility

053e577

Signed-off-by: Yangmin Zhu <ymzhu@google.com>

reject invalid configs.

5e75d78

Signed-off-by: Yangmin Zhu <ymzhu@google.com>

lizan reviewed Aug 10, 2018

View reviewed changes

liminw reviewed Aug 10, 2018

View reviewed changes

do not use fixed port in test.

5698f6b

Signed-off-by: Yangmin Zhu <ymzhu@google.com>

lizan reviewed Aug 10, 2018

View reviewed changes

small fix

5729e2d

Signed-off-by: Yangmin Zhu <ymzhu@google.com>

lizan previously approved these changes Aug 10, 2018

View reviewed changes

lizan requested review from rodaine and mattklein123 August 10, 2018 21:48

mattklein123 self-assigned this Aug 11, 2018

mattklein123 reviewed Aug 13, 2018

View reviewed changes

improve test coverage, address comments.

e8af0fa

Signed-off-by: Yangmin Zhu <ymzhu@google.com>

yangminzhu dismissed lizan’s stale review via e8af0fa August 13, 2018 23:46

lizan reviewed Aug 14, 2018

View reviewed changes

fix coverage.

1b4b0ad

Signed-off-by: Yangmin Zhu <ymzhu@google.com>

mattklein123 approved these changes Aug 14, 2018

View reviewed changes

mattklein123 merged commit c283439 into envoyproxy:master Aug 14, 2018

yangminzhu deleted the rbac_tcp branch August 14, 2018 18:08

zuercher mentioned this pull request Aug 14, 2018

rbac/fuzz: fix build #4150

Merged

liminw mentioned this pull request Sep 17, 2018

Istio RBAC Improvements istio/istio#6984

Closed

8 tasks


		enum class EnforcementMode { Enforced, Shadow };

		class RoleBasedAccessControlRouteSpecificFilterConfig : public Router::RouteSpecificFilterConfig {

		case envoy::config::rbac::v2alpha::Permission::RuleCase::kHeader:
		if (disable_http_rules) {

		@@ -0,0 +1,31 @@
		syntax = "proto3";

		package envoy.config.filter.network.rbac.v2;

		@@ -159,7 +160,8 @@ message Principal {
		// A header (or psuedo-header such as :path or :method) on the incoming HTTP request.
		envoy.api.v2.route.HeaderMatcher header = 6;

rbac: add rbac network filter. #4083

rbac: add rbac network filter. #4083

Conversation

yangminzhu commented Aug 8, 2018 • edited Loading

lizan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yangminzhu Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiwzhang commented Aug 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yangminzhu commented Aug 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yangminzhu commented Aug 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

yangminzhu commented Aug 8, 2018 •

edited

Loading

yangminzhu Aug 8, 2018 •

edited

Loading