RBAC support for policy dark launch by quanjielin · Pull Request #3554 · envoyproxy/envoy

quanjielin · 2018-06-06T00:56:05Z

2nd POC for #3552(1st POC: #3548).

1st POC(#3548) adds an enforcement_mode property at policy level, the problem of this approach is it won't be able to test policy deletion/replacement scenarios; also the implementation isn't correct(evaluation uses OR_rule may returns earlier even without applying dark launch policy)

To address issues mentioned above, this PR groups policies into two independent rules(enforcement/darklaunch) in RBAC config.

Signed-off-by: Quanjie Lin quanlin@google.com

quanjielin · 2018-06-06T01:11:42Z

/cc @qiwzhang @liminw @diemtvu @mattklein123 @rodaine

rodaine

Nice! I like this PoC better than #3548 but some of my comments from there also apply (ie, the name)

rodaine · 2018-06-06T01:46:01Z

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

is_dark_launch (and elsewhere)

updated to use enum.

rodaine · 2018-06-06T01:52:42Z

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

IIRC per the C++ style guide, this should look like: ..., /*is_dark_launch=*/ true)) {

Alternatively, use the enum you added in the protos instead so it's explicit.

rodaine · 2018-06-06T01:53:45Z

test/extensions/filters/common/rbac/engine_impl_test.cc

Can we add a test for triggering the dark path?

added in rbac_filter_test.cc

qiwzhang · 2018-06-06T15:24:59Z

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

use const std::vector& to avoid data copy

thanks, done.

qiwzhang · 2018-06-06T15:26:36Z

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

per c++ style guide, the variable name should be is_dark_launch

updated to use enum.

diemtvu

High level question: can we dark launch from rbac-disable to rbac-enable+rules with this? I guess you can simulate rbac-disable with a dummy rule (i.e action = DENY with no policies), but I wonder if there could be a cleaner way?

quanjielin · 2018-06-06T17:58:52Z

@diemtvu, another option I could think:
rbac engine has a engine_disabled_ property, which is always set to false by filter right now; we could add a boolean in RBAC config proto, and set engine_disabled_ from that value.

diemtvu · 2018-06-06T18:14:32Z

From what I see, I think we can remove this flag (engine_disabled_) completely, and make allowed function returns true (in active mode) if the rules is empty (the RBAC message, not the vector of policies). You can use separated boolean for this but not sure if it has clearer usability.

diemtvu · 2018-06-06T18:20:14Z

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

Probably do this only when there is actually 'darklaunch' policies, otherwise it could be confusing.

I think probably it's fine to log what user expected to see if they switch the mode ?

What mode? This means users will see counter for permissive_allowed all the time, even they do not provide any permissive rules. It seems confusing to me. Anyway, it's just my opinion.

updated to only evaluate/log only when permissive policies is set

liminw · 2018-06-06T21:52:45Z

@quanjielin @diemtvu "engine_disabled_" is used by per-route RBAC config.

I think if we leave the RBAC filter spec empty, the RBAC evaluation logic will not kick in. That is basically the RBAC disabled scenario you are thinking about.

@rodaine Correct me if I am wrong.

quanjielin · 2018-06-06T22:05:49Z

@liminw, agree, just did another commit which includes

remove required checking for RBAC.rules (which is for enforcement mode), it's empty indicates rbac disabled; so that we could test rbac disabled -> rbac enabled only for permissive mode case(RBAC.rules is empty, RBAC.permissive_rules is set)
thread rbac config to engine to make the code cleaner

Keep engine_disabled_ for per-route config, probably it also could be removed to re-use logic above from RBACPerRoute.RBAC ?

yangminzhu · 2018-06-07T00:18:28Z

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

This could also be removed?

as mentioned in above comment, keep this for now because of per-route config; it could be removed if we could decide disable from RBACPerRoute.RBAC field(which this PR uses in other places). I'm not familiar about the RBACPerRoute use case, wait @rodaine to comment whether it's safe to remove disable field from RBACPerRoute proto.

I mean the if statement.

oh I misunderstand, done.

mattklein123 · 2018-06-07T04:04:27Z

Quick note that we will need docs on this feature in the main filter config docs / arch overview. I think it's definitely worth calling out. I don't think it's necessary to release note it as we just added the main filter.

I haven't looked at the code yet, but LMK when the first round of reviews is done and I can take a look. Also note that DCO will need to be fixed. Feel free to keep reviewing and then when you are all satisfied squash/rebase force push to fix DCO and then I will review. Thanks!

lizan · 2018-06-07T05:49:34Z

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

as config_ is const, this shouldn't be here but in cluster. You can have policies_ and permissive_policies_ as a member. Creating PolicyMatcher is inefficient.

lizan · 2018-06-07T05:50:32Z

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

the if and else is pretty similar, extract to a function.

seems no extra function needed after moving code around :)

diemtvu · 2018-06-07T17:10:19Z

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

What mode? This means users will see counter for permissive_allowed all the time, even they do not provide any permissive rules. It seems confusing to me. Anyway, it's just my opinion.

diemtvu · 2018-06-07T17:20:08Z

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

-                                               const Envoy::Http::HeaderMap& headers) const {
+                                               const Envoy::Http::HeaderMap& headers,
+                                               EnforcementMode mode) const {
  if (engine_disabled_) {


I'm still don't understand the purpose of this boolean. Can we just use the logic with the next two if you've just added?

I also feel it's not needed, it could be inferred from whether rules is set; removed it in c4f5e47. @rodaine let's know if I miss anything here.

liminw · 2018-06-07T18:44:29Z

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

-  config.rbac.v2alpha.RBAC rules = 1 [(validate.rules).message.required = true];
+  // Specify the enforcement mode RBAC rules to be applied globally.
+  // Disable the filter for enforcement mode if rules isn't set.
+  config.rbac.v2alpha.RBAC rules = 1;


Before we remove "[(validate.rules).message.required = true]", @rodaine can you please review this part? I think you had it there on purpose.

For RBAC disabled case, we can just leave the entire RBAC filter spec empty. RBAC filter will not be invoked in that case.

See my other comment on the removal of disabled

liminw · 2018-06-07T18:52:10Z

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

Why is this removed?

as just mentioned in another comment, this field could be inferred from whether rules is set(same as logic used in other places in this PR), need @rodaine confirm though.

This was originally added as an explicit indicator that the filter is disabled for the vhost/route. Likewise, making it by-convention (ie, the config.rbac.v2alpha.RBAC being unset in the above config) permits the pathological configuration of RBAC where the global filter has no policies defined at all.

rodaine · 2018-06-11T18:39:12Z

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

Can we just keep rules = 1? Otherwise, this would be a breaking change to anyone already consuming them. I understand the desire to clearly indicate that these are the actually enforced rules, but I think that can be accomplished in the comments.

rodaine · 2018-06-11T18:41:52Z

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

please also reserve disabled

rodaine · 2018-06-11T18:43:31Z

docs/root/configuration/http_filters/rbac_filter.rst

Can we change the last clause to: ...it is used to test how a new set of policies work before rolling out to production.?

rodaine · 2018-06-11T18:46:11Z

docs/root/configuration/http_filters/rbac_filter.rst

For clarity, please drop the by the filter enforcement mode phrase from allowed/denied, and change the shadow ones to that would be allowed/denied access by the filter's shadow rules.

rodaine · 2018-06-11T18:49:42Z

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

The passed in config cannot be nil (due to the reference in the engine's constructor), and the RBAC proto has validation to prevent it being set with no policies. We're just bypassing that by returning the zero value in the route specific config's constructor. The engine does not need to know about enabled/disabled as that's a function of the filter, though. This should be encoded in the filter's config logic, not the engine impl, IMO.

rodaine · 2018-06-11T19:05:59Z

test/extensions/filters/http/rbac/rbac_filter_integration_test.cc

can remove the raw string markers: "{}"

seems {} is needed(tried to remove it then test timeout..)

sorry i meant replace R"EOF({})EOF" with just "{}"

rodaine · 2018-06-11T22:32:31Z

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

please change back to 1 (and remove the reserved).

rodaine · 2018-06-11T22:32:56Z

docs/root/configuration/http_filters/rbac_filter.rst

remove /denied

rodaine · 2018-06-11T22:33:16Z

docs/root/configuration/http_filters/rbac_filter.rst

remove allowed/ (sorry wasn't clearer!)

rodaine · 2018-06-11T23:25:03Z

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

      const std::string& stats_prefix, Stats::Scope& scope);

  RoleBasedAccessControlFilterStats& stats() { return stats_; }
+  const envoy::config::filter::http::rbac::v2::RBAC& config() { return config_; }


is this used?

rodaine · 2018-06-11T23:26:23Z

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

+
+  bool isEnabled(EnforcementMode mode) const {
+    return mode == EnforcementMode::ENFORCED ? config_.has_rules() : config_.has_shadow_rules();
+  }


can this be resolved at construction so we don't need to hold onto the RBAC config?

use two boolean member variables instead of holding config _? we could do that that but personally I don't feel too much differences (run-time also should be quick ?)

+1, you can make engine_ and shadow_engien_ an absl::optional

done, looks much cleaner now, thanks !

lizan · 2018-06-12T00:38:20Z

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


+bool RoleBasedAccessControlFilterConfig::isEnabled(const Router::RouteConstSharedPtr route,
+                                                   EnforcementMode mode) const {
+  if (!route || !route->routeEntry()) {


can you extract this part getRouteLocalConfig to another function? It is similar with engine above.

not necessary now as after getting rid of this function.

lizan · 2018-06-12T00:40:59Z

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

+
+  bool isEnabled(EnforcementMode mode) const {
+    return mode == EnforcementMode::ENFORCED ? config_.has_rules() : config_.has_shadow_rules();
+  }


+1, you can make engine_ and shadow_engien_ an absl::optional

yangminzhu · 2018-06-12T01:00:11Z

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

+  if (!config_->isEnabled(callbacks_->route(), EnforcementMode::ENFORCED) ||
+      config_->engine(callbacks_->route(), EnforcementMode::ENFORCED)
+          .allowed(*callbacks_->connection(), headers)) {
    config_->stats().allowed_.inc();


Can we do not increase the stats when config_->isEnabled() is false? Otherwise I'm afraid the stats is not very useful as it is increased when rbac is not enabled.

updated the logic.

yangminzhu · 2018-06-12T01:02:14Z

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

+// RBAC filter config.
+// User sees the shadow rules as replacement to the enforced rules.
+// For example, policy A runs in prod in enforced mode, user wants
+// to test policy B; in the config, enforced_rules contains {A.mode=enforced},


encorced_rules -> rules in the comment?

thanks, done.

diemtvu · 2018-06-12T00:59:17Z

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

+RoleBasedAccessControlRouteSpecificFilterConfig::RoleBasedAccessControlRouteSpecificFilterConfig(
+    const envoy::config::filter::http::rbac::v2::RBACPerRoute& per_route_config)
+    : engine_(per_route_config.rbac().rules()),
+      shadow_engine_(per_route_config.rbac().shadow_rules()), config_(per_route_config.rbac()) {}


I suggest to use pointer (unique_ptr<Filters::Common::RBAC::RoleBasedAccessControlEngine>) for engine / shadow_engine, and create them conditionally in the constructor. Later, you then can simply check if engine_ == nullptr before running it. E.g:

RoleBasedAccessControlRouteSpecificFilterConfig::RoleBasedAccessControlRouteSpecificFilterConfig(const envoy::config::filter::http::rbac::v2::RBACPerRoute& per_route_config) config_(per_route_config.rbac()) { if (config_.has_rules()) { engine_.reset(new Filters::Common::RBAC::RoleBasedAccessControlEngineImpl(config_.rules()); } }

then later in decodeHeader

if (shadow_engine_) { if (shadow_engine_.allowed(*callbacks_->connection(), headers)) { config_->stats().shadow_allowed_.inc(); } else { config_->stats().shadow_denied_.inc(); } } if (engine_ == nullptr) { if (engine_.allowed(*callbacks_->connection(), headers)) { config_->stats().allowed_.inc(); return Http::FilterHeadersStatus::Continue; } else { config_->stats().denied_.inc(); //... return Http::FilterHeadersStatus::StopIteration; } } // engine_ == nullptr, equivalent to rbac disable. Always accept request. return Http::FilterHeadersStatus::Continue; }

See my comment above, you can use absl::optional instead of a pointer, and check if they are nullopt.

diemtvu · 2018-06-12T01:04:52Z

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

+  }

-  if (engine.allowed(*callbacks_->connection(), headers)) {
+  if (!config_->isEnabled(callbacks_->route(), EnforcementMode::ENFORCED) ||


I would break this if to separate config_.isEnabled() == false to the different part that just return Continue (without counter) to be consistent. See example in the suggestion above.

thanks, done.

lizan

LGTM modulo 2 nits

lizan · 2018-06-12T16:54:22Z

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

lizan · 2018-06-12T16:54:27Z

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

lizan

Thanks. PTAL @rodaine @mattklein123

Signed-off-by: Quanjie Lin <quanlin@google.com>

lizan

You didn't have to squash again since DCO was OK.

mattklein123

LGTM, just some small comments. Cool!

mattklein123 · 2018-06-12T22:05:28Z

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

+// RBAC filter config.
+// User sees the shadow rules as replacement to the enforced rules.
+// For example, policy A runs in prod in enforced mode, user wants
+// to test policy B; in the config, rules contains {A.mode=enforced},


I think this text is out of data for an old implementation? It doesn't make sense. I think you can just remove and leave to the documentation below to cover.

mattklein123 · 2018-06-12T22:06:48Z

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

-  // Specify the RBAC rules to be applied globally
-  config.rbac.v2alpha.RBAC rules = 1 [(validate.rules).message.required = true];
+  // Specify the enforced RBAC rules to be applied globally.
+  // Filter's decision based on enforced rules has impact on user experience.


I would replace these two sentences with: "If absent, no enforcing RBAC policy will be applied."

mattklein123 · 2018-06-12T22:07:56Z

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

+  config.rbac.v2alpha.RBAC rules = 1;
+
+  // Specify the shadow RBAC rules to be applied globally.
+  // Filter's decision based on shadow rules doesn't impact on user experience,


Replace two sentences with: "Shadow rules are not enforced by the filter (i.e., returning a 403) but will emit stats and logs and can be used for rule testing. If absent, no shadow RBAC policy with be applied."

mattklein123 · 2018-06-12T22:08:43Z

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

-    // Override the global configuration of the filter with this new config.
-    RBAC rbac = 2 [(validate.rules).message.required = true];
-  }
+  // Override the global configuration of the filter with this new config.


Add: "If absent, the global RBAC policy will be disabled for this route."

mattklein123 · 2018-06-12T22:09:24Z

docs/root/configuration/http_filters/rbac_filter.rst

 block-list (DENY) set of policies based off properties of the connection (IPs, ports, SSL subject)
-as well as the incoming request's HTTP headers.
+as well as the incoming request's HTTP headers. This filter also supports policy in both enforcement
+and shadow mode, shadow mode won't effect real users, it is used to test a new set of policies work


"it is used to test that a..."

mattklein123 · 2018-06-12T22:09:32Z

docs/root/configuration/http_filters/rbac_filter.rst

-as well as the incoming request's HTTP headers.
+as well as the incoming request's HTTP headers. This filter also supports policy in both enforcement
+and shadow mode, shadow mode won't effect real users, it is used to test a new set of policies work
+before rolling out to prod.


s/prod/production here and elsewhere.

mattklein123 · 2018-06-12T22:11:30Z

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

  COUNTER(allowed)                                                                                 \
-  COUNTER(denied)
+  COUNTER(denied)                                                                                  \
+  COUNTER(shadow_allowed)                                                                      \


nit: slash indent

mattklein123 · 2018-06-12T22:11:43Z

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

  ALL_RBAC_FILTER_STATS(GENERATE_COUNTER_STRUCT)
 };

+enum class EnforcementMode { ENFORCED, SHADOW };


nit: Enforced, Shadow

mattklein123 · 2018-06-12T22:12:55Z

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

+  engine(const Router::RouteConstSharedPtr route, EnforcementMode mode) const;

 private:
+  const absl::optional<Filters::Common::RBAC::RoleBasedAccessControlEngineImpl>&


Can this derive from RoleBasedAccessControlRouteSpecificFilterConfig? all the code is basically the same.

Since RoleBasedAccessControlRouteSpecificFilterConfig is derived from RouteSpecificFilterConfig , I may prefer to keep RoleBasedAccessControlFilterConfig as it is.

Signed-off-by: Quanjie Lin <quanlin@google.com>

mattklein123

LGTM. Will defer to @rodaine for any final comments.

rodaine

Awesome work! LGTM

quanjielin force-pushed the quanlinplay2 branch from 13fae9d to ddc20ec Compare June 6, 2018 00:59

quanjielin mentioned this pull request Jun 6, 2018

Add PolicyEnforcementMode to RBAC policy for policy dark launch #3552

Closed

quanjielin force-pushed the quanlinplay2 branch from ddc20ec to 2bede03 Compare June 6, 2018 01:04

quanjielin mentioned this pull request Jun 6, 2018

[WIP, POC] Add PolicyEnforcementMode to RBAC policy definition #3548

Closed

rodaine reviewed Jun 6, 2018

View reviewed changes

qiwzhang reviewed Jun 6, 2018

View reviewed changes

quanjielin force-pushed the quanlinplay2 branch from f802f7e to cc776af Compare June 6, 2018 16:35

diemtvu reviewed Jun 6, 2018

View reviewed changes

quanjielin force-pushed the quanlinplay2 branch from cc776af to 4ad654c Compare June 6, 2018 17:50

diemtvu reviewed Jun 6, 2018

View reviewed changes

quanjielin force-pushed the quanlinplay2 branch from 76d51a9 to 5e68bc3 Compare June 6, 2018 21:50

quanjielin changed the title ~~[WIP, POC] RBAC support for policy dark launch~~ RBAC support for policy dark launch Jun 6, 2018

yangminzhu reviewed Jun 7, 2018

View reviewed changes

quanjielin force-pushed the quanlinplay2 branch 2 times, most recently from 55deaa0 to 603d35c Compare June 7, 2018 00:51

mattklein123 assigned rodaine and mattklein123 Jun 7, 2018

lizan reviewed Jun 7, 2018

View reviewed changes

quanjielin force-pushed the quanlinplay2 branch 3 times, most recently from d8b2b02 to 6f312b9 Compare June 7, 2018 16:59

diemtvu reviewed Jun 7, 2018

View reviewed changes

liminw reviewed Jun 7, 2018

View reviewed changes

rodaine reviewed Jun 11, 2018

View reviewed changes

quanjielin force-pushed the quanlinplay2 branch 2 times, most recently from cfaf034 to 4cc9c0a Compare June 11, 2018 22:19

rodaine reviewed Jun 11, 2018

View reviewed changes

quanjielin force-pushed the quanlinplay2 branch 3 times, most recently from 331fec8 to 1765bc7 Compare June 11, 2018 23:19

rodaine reviewed Jun 12, 2018

View reviewed changes

lizan reviewed Jun 12, 2018

View reviewed changes

yangminzhu reviewed Jun 12, 2018

View reviewed changes

diemtvu reviewed Jun 12, 2018

View reviewed changes

lizan reviewed Jun 12, 2018

View reviewed changes

quanjielin force-pushed the quanlinplay2 branch from 1a1e9b7 to d1e9823 Compare June 12, 2018 16:59

lizan previously approved these changes Jun 12, 2018

View reviewed changes

RBAC support for policy dark launch

0a67d58

Signed-off-by: Quanjie Lin <quanlin@google.com>

quanjielin dismissed lizan’s stale review via 0a67d58 June 12, 2018 17:06

quanjielin force-pushed the quanlinplay2 branch from d1e9823 to 0a67d58 Compare June 12, 2018 17:06

lizan previously approved these changes Jun 12, 2018

View reviewed changes

mattklein123 reviewed Jun 12, 2018

View reviewed changes

quanjielin dismissed lizan’s stale review via 826e321 June 12, 2018 22:45

address comments

f2e9fff

Signed-off-by: Quanjie Lin <quanlin@google.com>

quanjielin force-pushed the quanlinplay2 branch from 826e321 to f2e9fff Compare June 12, 2018 22:49

mattklein123 approved these changes Jun 12, 2018

View reviewed changes

rodaine approved these changes Jun 13, 2018

View reviewed changes

mattklein123 merged commit 4b41931 into envoyproxy:master Jun 13, 2018

quanjielin mentioned this pull request Aug 6, 2018

collect metrics for RBAC shadow policy #4062

Merged

This was referenced Aug 27, 2018

Supporting "dark launch" Mode for Security policies istio/istio#5345

Closed

support permissive mode for global RBAC config istio/istio#8359

Merged

add example rbac policy in permissive mode istio/istio#8284

Merged

Conversation

quanjielin commented Jun 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

quanjielin commented Jun 6, 2018

Uh oh!

rodaine left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diemtvu left a comment

Choose a reason for hiding this comment

Uh oh!

quanjielin commented Jun 6, 2018

Uh oh!

diemtvu commented Jun 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liminw commented Jun 6, 2018

Uh oh!

quanjielin commented Jun 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Jun 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quanjielin commented Jun 6, 2018 •

edited

Loading