filter: add conditions to access control filter #7716

kyessenov · 2019-07-25T05:15:44Z

Signed-off-by: Kuat Yessenov [email protected]

Introduces a generic expression-based admission filter using https://github.com/google/cel-cpp.
This is a follow-up to discussion in #6751.
The advantage of this approach is:

Un-opinionated about the policy structure since the only config is an expression. This is friendly towards control planes which can bear the complexity of translation, analysis, and evolution of policies.
Multi-language, CEL supports go, java, and c++ runtimes.
Inter-operability with other filters using request metadata. Companion filters can populate metadata about requests and resources that affect policy decisions.
Generic utility, it can be used for custom metric labels, access log entries, etc.

The dis-advantage of this approach is that its performance is lower than domain-optimized interpreters. On a fair example, the interpreter evaluates in around 1ms (see https://github.com/google/cel-cpp/blob/master/eval/tests/benchmark_test.cc#L591) vs ~150ns for hand-written C++ native code. There is space for improvement (especially if WASM can be used as a compilation target), but ultimately the generic expression form carries a cost.

Conditions are added to support RBAC filter for complementing the existing principal/permission model. They add support for the extended checks (e.g. time of query, resource-bound), but add no cost unless used.

Description: add expression-based admission filter
Risk Level: low
Testing:
Docs Changes:
Release Notes:

Signed-off-by: Kuat Yessenov <[email protected]>

kyessenov · 2019-07-25T06:24:37Z

PR needs the bazel update in #7622

mandarjog · 2019-07-25T06:41:41Z

source/extensions/filters/http/abac/abac_filter.cc

+
+  int size() const override { return 0; }
+  bool empty() const override { return false; }
+  const CelList* ListKeys() const override { return nullptr; }


Why are all listkeys returning null?

Not implemented. For wrappers, it's undesired. For headers, it might be useful for comprehension operations (forall, equality, or concatenating all header values), which I suppose is not common.

source/extensions/filters/http/abac/abac_filter.cc

lizan

quick skim. Merge master and I'll take another look.

bazel/repositories.bzl

source/common/common/logger.h

source/extensions/extensions_build_config.bzl

source/extensions/filters/http/abac/abac_filter.cc

lizan · 2019-07-26T01:43:08Z

source/extensions/filters/http/abac/abac_filter.cc

+      return {};
+    }
+    auto value = key.StringOrDie().value();
+    if (value == kRequestedServerName) {


can those be something like flat_hash_map<string_view, std::function>? I think we do similar in access_log_formatter.

I'll need to measure it. std::function are surprisingly expensive in terms of unexpected allocations. The way it's done here is trivially allocation-free.

You don't have to create a map every time, have a static map like: flat_hash_map<string_view, std::function<CelValue(const StreamInfo&)>>, so you just call them every time, not allocating.

I'm not sure that's more readable. Anyways, I need to measure before making the change.

Lizan, is this what you had in mind https://github.com/kyessenov/envoy/blob/abac_filter_feedback/test/extensions/filters/common/expr/context_speed_test.cc#L34?

yep, thanks for testing that out.

After adding all functions to the flat_hash_map, here is the performance comparison:

2019-08-05 14:33:56 Running bazel-bin/test/extensions/filters/common/expr/context_speed_test Run on (16 X 2200 MHz CPU s) CPU Caches: L1 Data 32K (x8) L1 Instruction 32K (x8) L2 Unified 256K (x8) L3 Unified 56320K (x1) Load Average: 1.14, 1.13, 1.19 --------------------------------------------------------- Benchmark Time CPU Iterations --------------------------------------------------------- BM_Context 17.6 ns 17.6 ns 39715762 BM_TestContext 17.2 ns 17.2 ns 42524637

So it's roughly the same (I couldn't trivially wire a memory allocation profiler), and it comes down to readability and code style.

WDYT? Should we change it to a hashmap of functions?

source/extensions/filters/http/well_known_names.h

source/extensions/filters/http/abac/abac_filter.cc

…_rebase

Signed-off-by: Kuat Yessenov <[email protected]>

source/extensions/filters/http/abac/abac_filter.cc

Signed-off-by: Kuat Yessenov <[email protected]>

api/envoy/config/filter/http/abac/v2alpha/abac.proto

…_rebase

Signed-off-by: Kuat Yessenov <[email protected]>

repokitteh-read-only · 2019-07-30T05:07:14Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7716 was synchronize by kyessenov.

see: more, trace.

Signed-off-by: Kuat Yessenov <[email protected]>

repokitteh-read-only · 2019-07-30T05:14:49Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7716 was synchronize by kyessenov.

see: more, trace.

Signed-off-by: Kuat Yessenov <[email protected]>

repokitteh-read-only · 2019-07-30T05:19:23Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7716 was synchronize by kyessenov.

see: more, trace.

source/extensions/filters/common/expr/context.h

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

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

…_rebase

Signed-off-by: Kuat Yessenov <[email protected]>

repokitteh-read-only · 2019-07-30T21:37:18Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7716 was synchronize by kyessenov.

see: more, trace.

Signed-off-by: Kuat Yessenov <[email protected]>

repokitteh-read-only · 2019-08-15T20:12:49Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7716 was synchronize by kyessenov.

see: more, trace.

lizan · 2019-08-15T21:47:36Z

Great, I was looking at https://264581-65214191-gh.circle-artifacts.com/0/coverage/index.html, can you try to improve coverage for expr/{context,evaluator}.cc a bit more, esp. for error or empty cases?

Signed-off-by: Kuat Yessenov <[email protected]>

repokitteh-read-only · 2019-08-15T22:18:58Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7716 was synchronize by kyessenov.

see: more, trace.

kyessenov · 2019-08-15T22:19:18Z

Cool, added more tests to bump up the coverage.

kyessenov · 2019-08-16T15:49:05Z

@lizan Anything else that needs to be done?

lizan

LGTM, one question.

lizan · 2019-08-19T21:19:31Z

bazel/repository_locations.bzl

+    com_googlesource_code_re2 = dict(
+        sha256 = "f31db9cd224d018a7e4fe88ef84aaa874b0b3ed91d4d98ee5a1531101d3fdc64",
+        strip_prefix = "re2-87e2ad45e7b18738e1551474f7ee5886ff572059",
+        urls = ["https://github.com/google/re2/archive/87e2ad45e7b18738e1551474f7ee5886ff572059.tar.gz"],


@kyessenov qq: does cel-cpp rely on specific commit of re2? Asking because it might conflict with #7878, or latest release (2019-08-01) is fine? cc @mattklein123

There should be no difference between which version is used. I think I chose the latest version which I started this PR.

OK that's fine.

I was going to ask the same question. I'll switch this back a release version of re2 on a subsequent dependency PR.

I will just fix this when I merge master.

Thanks. Happy to help if necessary. Google3 doesn't really have versions for its repositories, and the upstream cel-cpp is continuously tested against head.

htuch · 2019-08-21T19:55:41Z

@kyessenov do you know how well the CEL parser is fuzzed? CC @asraa.

kyessenov · 2019-08-21T20:02:24Z

There's no parser involved in this PR. The input is an AST in proto form. The AST is very simple, so it's human readable, and is generally correct by construction.

There're two parsers for CEL available to generate AST: an internal in Java, and an external ANTLR-based golang one https://github.com/google/cel-go/tree/master/parser.

htuch · 2019-08-21T20:10:46Z

@kyessenov two other security considerations:

What about fuzzing the evaluator? Do you know if this has been done?
Is it possible to bound or predict the performance of CEL evaluation based on expression and input data? I worry we might be introducing a similar concern as with introduce safe regex matcher based on re2 engine #7878.

kyessenov · 2019-08-21T20:16:08Z

@htuch I don't know if fuzzing was done on the evaluator. The code runs in production though, and I can share the relevant contacts.

CEL was designed for security policies:

CEL evaluates in linear time, is mutation free, and not Turing-complete. This limitation is a feature of the language design, which allows the implementation to evaluate orders of magnitude faster than equivalently sandboxed JavaScript.

We only use standard builtin functions so I think those have been inspected to conform to that goal.

htuch · 2019-08-21T20:18:47Z

Thanks, let's sync offline on this, I'm happy with the above answer on a security due diligence front.

TristonianJones · 2019-08-21T21:27:32Z

I can add some clarity here ...

Evaluation Cost

The cpu and memory impact of a CEL expression can be tuned by adjusting the environment (functions, macros) available to the expression itself. That is, if you don't want to support the function or macro, you can remove it from CEL environment declaration. Here are some common considerations with respect to the builtin features

Memory

Most operations are have no additional allocation costs beyond the memory required to represent the value within the runtime. There are a couple of exceptions to this rule:

Type conversion functions can result in fixed-size allocations, e.g. string to timestamp.
The re2-based matches performs its own allocations when compiling the input regex pattern.
The + operator supports string / list concat where the memory cost is variable with the size of the input.

CPU

The majority of CEL functions and macros are O(1) or O(n), with a couple of exceptions.

The re2-based matches is linear, but with a high-constant factor.
The in operator can be super-linear when testing for a string value in a non-literal list.
The comprehension macros all, exists, filter, map support bounded iteration and nesting.

Tuning

In many cases, the matches function is eschewed in favor of the simpler startsWith, endsWith, and contains functions.

The in operator can be linted to ensure that a list or map literal appears on the right-hand side of the expression, as this special case is treated as a set membership test rather than as a per-element comparison of the list elements.

Some environments disable the + operator due to its inclusion of string and list overloads. I think that CEL might need to deprecate the concatenation overloads in the near-term in favor of a concat function which can be expressly removed from the environment.

Most of the performance critical use cases for CEL disable macros entirely, though there are a couple of linear-time macros currently supported in Istio.

With the above recommendations, you'll have fixed allocation size operations that run in linear or sub-linear time. If you want to support in and +, I would recommend lint checks on the AST to ensure that they are used in a performance manner (in) or with limited frequency (+).

Fuzzing

Builtin functions are fuzzed within Google-internal tests., but we should open source the tests and augment them if desired.

htuch · 2019-08-22T18:10:44Z

@kyessenov do you think it's reasonable to add the restrictions suggested ^^ under "Tuning"? Ideally we are as conservative as possible and then relax these in a deliberative fashion.

@TristonianJones do we have plumbing to control something like https://github.com/envoyproxy/envoy/pull/7878/files#diff-c45c47ac26956b7b0f3f771bdf273fc2R25 in RE2? Or some CEL-level equivalent? Ideally we can provide operator control that bounds complexity.

htuch · 2019-08-22T18:11:05Z

(and BTW, thanks @TristonianJones for that super detailed and helpful writeup)

kyessenov · 2019-08-22T18:18:41Z

@htuch It might be better to delegate the restrictions to the management server. It's already expected that the expressions are type checked ahead of time, so it should be another pass to validate the bounded complexity. We can provide a library in go-control-plane to make it easier.

Thanks for the write-up! I am going to send a doc update PR for RBAC, and will include the considerations above.

htuch · 2019-08-22T18:24:17Z

@kyessenov a general Envoy principle that we've been trying to deduce recently is to try and simplify things on the management server; there will be O(100) or O(1k) management server implementations, and not all will use go-control-plane. As a result, making Envoy-side safer and more defensive here would make sense.

TristonianJones · 2019-08-22T18:25:32Z

@htuch there aren't any specific controls to limit the size of the regex pattern, but it is restricted to what RE2 supports. There are a handful of lint checks that should be easy to implement as an AST traversal and checked in management server, or wherever makes sense. We haven't built a linting framework, but it's on our wish list.

Note, removing the + and in operators might degrade the user experience, so if you wanted to lint these cases, I would recommend checking for string, list concatenation and for the super-linear in cases and surfacing them as errors after the type-check.

kyessenov · 2019-08-22T18:28:15Z

@htuch Sure, we can add extra controls in the C++ run-time. This work can be done in the upstream project. We can probably re-use some existing work from CEL CPP clients.

Some more sophisticated linting can be done in CEL GO. It's a shared library that should be easy to include in any management server.

htuch · 2019-08-23T04:00:44Z

@kyessenov where should we track this? In some Envoy issue, or do you folks have this scheduled?

kyessenov · 2019-08-23T17:36:13Z

Filed google/cel-cpp#37.
Once controls are added, we can update the filter with an extra config.

htuch · 2019-08-23T20:54:46Z

@kyessenov thanks, let's track there.

This adds a fuzzer for CEL expression matching. These conditions are used in the RBAC filter for complementing the existing principal/permission model. About a quarter of the execution time is coming from google::api::expr::runtime::RegisterBuiltinFunctions. The test runs locally at about 250 exec/sec. See: #7716 Risk Level: Low Testing: Converted unit tests into corpus entries Signed-off-by: Asra Ali <[email protected]>

kyessenov added 2 commits July 24, 2019 22:03

Initial ABAC filter

3d4f7aa

Signed-off-by: Kuat Yessenov <[email protected]>

typos

beb197c

Signed-off-by: Kuat Yessenov <[email protected]>

mandarjog reviewed Jul 25, 2019

View reviewed changes

kyessenov commented Jul 25, 2019

View reviewed changes

source/extensions/filters/http/abac/abac_filter.cc Outdated Show resolved Hide resolved

dio assigned lizan and rshriram Jul 25, 2019

lizan previously requested changes Jul 26, 2019

View reviewed changes

kyessenov added 3 commits July 25, 2019 19:33

Merge remote-tracking branch 'upstream/master' into abac_filter_needs…

e914f46

…_rebase

review

ec74571

Signed-off-by: Kuat Yessenov <[email protected]>

spelling

97efd6d

Signed-off-by: Kuat Yessenov <[email protected]>

kyessenov commented Jul 26, 2019

View reviewed changes

source/extensions/filters/http/abac/abac_filter.cc Outdated Show resolved Hide resolved

undo watermark

f3668b2

Signed-off-by: Kuat Yessenov <[email protected]>

yangminzhu reviewed Jul 26, 2019

View reviewed changes

api/envoy/config/filter/http/abac/v2alpha/abac.proto Outdated Show resolved Hide resolved

kyessenov added 3 commits July 29, 2019 15:45

Merge remote-tracking branch 'upstream/master' into abac_filter_needs…

dca0933

…_rebase

review feedback

8e80999

Signed-off-by: Kuat Yessenov <[email protected]>

review feedback

970b361

Signed-off-by: Kuat Yessenov <[email protected]>

review feedback

ebf4c4a

Signed-off-by: Kuat Yessenov <[email protected]>

add code owners

d1fd462

Signed-off-by: Kuat Yessenov <[email protected]>

kyessenov changed the title ~~filter: add ABAC based on protobuf expressions~~ filter: add conditions to access control filter Jul 30, 2019

rshriram reviewed Jul 30, 2019

View reviewed changes

source/extensions/filters/common/expr/context.h Show resolved Hide resolved

yangminzhu reviewed Jul 30, 2019

View reviewed changes

source/extensions/filters/http/rbac/rbac_filter.cc Outdated Show resolved Hide resolved

api/envoy/config/filter/http/rbac/v2/rbac.proto Outdated Show resolved Hide resolved

kyessenov added 3 commits July 30, 2019 13:16

Merge remote-tracking branch 'upstream/master' into abac_filter_needs…

1235c7d

…_rebase

update cel-cpp

41220da

Signed-off-by: Kuat Yessenov <[email protected]>

combine engines

982e3fd

Signed-off-by: Kuat Yessenov <[email protected]>

make arena explicit

c31b4a7

Signed-off-by: Kuat Yessenov <[email protected]>

merge fix

753b352

Signed-off-by: Kuat Yessenov <[email protected]>

bump up coverage

8df3414

Signed-off-by: Kuat Yessenov <[email protected]>

lizan approved these changes Aug 19, 2019

View reviewed changes

lizan merged commit f90e1b0 into envoyproxy:master Aug 19, 2019

htuch unassigned rshriram Aug 21, 2019

asraa mentioned this pull request Oct 24, 2019

[filters] Add RBAC condition fuzzer (CEL expressions) #8752

Closed

asraa mentioned this pull request Nov 22, 2019

[filters] Add RBAC condition fuzzer (CEL expressions) #9110

Merged

filter: add conditions to access control filter #7716

filter: add conditions to access control filter #7716

Conversation

kyessenov commented Jul 25, 2019 • edited Loading

kyessenov commented Jul 25, 2019

Choose a reason for hiding this comment

kyessenov Jul 25, 2019 • edited Loading

Choose a reason for hiding this comment

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

kyessenov Aug 5, 2019 • edited Loading

Choose a reason for hiding this comment

repokitteh-read-only bot commented Jul 30, 2019

repokitteh-read-only bot commented Jul 30, 2019

repokitteh-read-only bot commented Jul 30, 2019

repokitteh-read-only bot commented Jul 30, 2019

repokitteh-read-only bot commented Aug 15, 2019

lizan commented Aug 15, 2019

repokitteh-read-only bot commented Aug 15, 2019

kyessenov commented Aug 15, 2019

kyessenov commented Aug 16, 2019

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

htuch commented Aug 21, 2019

kyessenov commented Aug 21, 2019 • edited Loading

htuch commented Aug 21, 2019

kyessenov commented Aug 21, 2019 • edited Loading

htuch commented Aug 21, 2019

TristonianJones commented Aug 21, 2019

Evaluation Cost

Memory

CPU

Tuning

Fuzzing

htuch commented Aug 22, 2019

htuch commented Aug 22, 2019

kyessenov commented Aug 22, 2019

htuch commented Aug 22, 2019

TristonianJones commented Aug 22, 2019 • edited Loading

kyessenov commented Aug 22, 2019

htuch commented Aug 23, 2019

kyessenov commented Aug 23, 2019

htuch commented Aug 23, 2019

kyessenov commented Jul 25, 2019 •

edited

Loading

kyessenov Jul 25, 2019 •

edited

Loading

kyessenov Aug 5, 2019 •

edited

Loading

kyessenov commented Aug 21, 2019 •

edited

Loading

kyessenov commented Aug 21, 2019 •

edited

Loading

TristonianJones commented Aug 22, 2019 •

edited

Loading