[filters] Add RBAC condition fuzzer (CEL expressions) by asraa · Pull Request #8752 · envoyproxy/envoy

asraa · 2019-10-24T15:03:09Z

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

asraa · 2019-10-24T15:28:01Z

/review @kyessenov

kyessenov · 2019-10-24T19:33:59Z

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

downstreamRemoteAddress?

kyessenov · 2019-10-24T19:35:36Z

test/extensions/filters/common/expr/evaluator_fuzz_test.cc

You can move the builder out of the loop. It reuses the constant arena, so you might want to disable constant folding to reduce interference.

kyessenov · 2019-10-24T19:37:10Z

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

This is fine, but the stream info header states both can never be null.

Ah OK. Should be fine then now since TestStreamInfo sets both. Originally it caught a ubsan bug when it wasn't set. Changed, since the fuzzer shouldn't hit that case.

kyessenov · 2019-10-24T19:38:55Z

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

Can we leave this to be int64? It makes inter-op with other int64 values better. Otherwise, we have to cast in the expressions to int64, which is annoying.

asraa · 2019-10-28T12:29:47Z

@kyessenov are comprehension expressions supported? It seems like they aren't because ListKeys isn't implemented for e.g. with request maps.
Is it documented which expressions aren't supported? I might throw an EnvoyException in createExpression if they aren't supported.

kyessenov · 2019-10-28T18:00:33Z

@asraa Comprehensions are explicitly disabled as of #8746 since they can cause non-linear resource complexity. Same for string/list concatenations. I think you should get an exception building an expression for comprehensions, but not string/list concatenations. That's because string/list concat shares the operator + with normal addition (which is a language issue, that is being discussed). In any case, the evaluator should still successfully produce an error as a result of create expression or eval expression.

Signed-off-by: Asra Ali <asraa@google.com>

asraa · 2019-10-28T19:13:10Z

Gotcha thanks! Thanks for that PR. I updated the branch and added the testcase that was failing from the comprehension expressions. Should be good now.

kyessenov · 2019-10-28T19:47:35Z

Changes look fine to me, but I'm not very familiar with the fuzzing framework. Let me know if the fuzzer detects something.

asraa · 2019-10-28T19:53:53Z

Will do. After about a day of being checked in to upstream, OSS-Fuzz will report any crashes that come up. I'll reach out to you with anything that comes up. Also feel free to ping me if you want to extend fuzzers for the filter.

stale · 2019-11-04T20:09:45Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

kyessenov · 2019-11-04T20:55:32Z

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

  if (value == Address) {
    if (local_) {
      return CelValue::CreateString(info_.downstreamLocalAddress()->asStringView());
+


extra new line

Done, thank you!

repokitteh-read-only · 2019-11-05T13:34:38Z

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

🐱

Caused by: #8752 was synchronize by asraa.

see: more, trace.

Signed-off-by: Asra Ali <asraa@google.com>

htuch

Generally this looks great, nice fuzzer design. Just some comments/clarification requests and then LGTM.

htuch · 2019-11-06T03:05:54Z

test/extensions/filters/common/expr/evaluator_fuzz_test.cc

+
+DEFINE_PROTO_FUZZER(const test::extensions::filters::common::expr::EvaluatorTestCase& input) {
+  // Create builder without constant folding.
+  static Expr::BuilderPtr builder = Expr::createBuilder(nullptr);


Is this stateless? I.e. it won't vary between test cases and so is safe to make static? If so, please update the above comment.

Builder can be shared between expressions.

Updated comment. Yep, it was time-consuming to create the builder between iterations as well, so I felt it was a worthy object to make static.

htuch · 2019-11-06T03:06:27Z

test/extensions/filters/common/expr/evaluator_fuzz_test.cc

+    Http::TestHeaderMapImpl response_trailers = Fuzz::fromHeaders(input.trailers());
+
+    // Evaluate the CEL expression.
+    Protobuf::Arena arena;


What is this for?

CEL creates all temporary objects on an arena. I think this is used for this purpose.

Updated, passed in to evaluate. I was playing around with which would be faster.

stale · 2019-11-13T19:20:19Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

stale · 2019-11-20T19:55:51Z

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

asraa · 2019-11-22T14:37:50Z

Sorry it passed through my attention until it was marked closed!! Should be ready now.

repokitteh-read-only bot requested a review from kyessenov October 24, 2019 15:28

asraa force-pushed the rbacmatcher branch from cc9b014 to 97616c3 Compare October 24, 2019 16:21

kyessenov reviewed Oct 24, 2019

View reviewed changes

source/extensions/filters/common/expr/context.cc Outdated

Copy link
Copy Markdown

Contributor

kyessenov Oct 24, 2019

Choose a reason for hiding this comment

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

downstreamRemoteAddress?

kyessenov reviewed Oct 24, 2019

View reviewed changes

mattklein123 assigned kyessenov Oct 25, 2019

asraa added 5 commits October 28, 2019 15:08

fuzz: add cel expression fuzzer

8fa6426

Signed-off-by: Asra Ali <asraa@google.com>

add spelling

c8c6b1c

Signed-off-by: Asra Ali <asraa@google.com>

fixup

886c859

Signed-off-by: Asra Ali <asraa@google.com>

address comments

225eaed

Signed-off-by: Asra Ali <asraa@google.com>

add testcase

4e42c3c

Signed-off-by: Asra Ali <asraa@google.com>

asraa force-pushed the rbacmatcher branch from 24af4ce to 4e42c3c Compare October 28, 2019 19:12

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Nov 4, 2019

kyessenov reviewed Nov 4, 2019

View reviewed changes

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Nov 4, 2019

repokitteh-read-only bot added the api label Nov 5, 2019

fix new line

3892a77

Signed-off-by: Asra Ali <asraa@google.com>

asraa force-pushed the rbacmatcher branch from 0b6655d to 3892a77 Compare November 5, 2019 13:57

htuch reviewed Nov 6, 2019

View reviewed changes

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Nov 13, 2019

stale bot closed this Nov 20, 2019

asraa mentioned this pull request Nov 22, 2019

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

Merged

Conversation

asraa commented Oct 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asraa commented Oct 24, 2019

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!

asraa commented Oct 28, 2019

Uh oh!

kyessenov commented Oct 28, 2019

Uh oh!

asraa commented Oct 28, 2019

Uh oh!

kyessenov commented Oct 28, 2019

Uh oh!

asraa commented Oct 28, 2019

Uh oh!

stale bot commented Nov 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

repokitteh-read-only bot commented Nov 5, 2019

Uh oh!

htuch 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!

stale bot commented Nov 13, 2019

Uh oh!

stale bot commented Nov 20, 2019

Uh oh!

asraa commented Nov 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

asraa commented Oct 24, 2019 •

edited

Loading