api: Add log action to RBAC filter api by davidraskin · Pull Request #11705 · envoyproxy/envoy

davidraskin · 2020-06-23T04:20:40Z

Commit Message: Add "LOG" action to RBAC filter api
Additional Description: The log action will be used to set the dynamic metadata key "envoy.log", which can be used to decide whether to log a request.
Risk Level: Low
Testing: Existing tests pass
Docs Changes: Updated RBAC configuration reference with new dynamic metadata, and updated RBAC api reference.
Release Notes: n/a
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]

repokitteh-read-only · 2020-06-23T04:20:46Z

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

🐱

Caused by: #11705 was opened by davidraskin.

see: more, trace.

davidraskin · 2020-06-23T14:52:17Z

Relates to #11594

htuch · 2020-06-23T23:02:46Z

docs/root/configuration/http/http_filters/rbac_filter.rst

Why is this a string and not a bool?

This could be used to indicate a reason for logging and presence of the key indicates whether to log. E.g. RBAC filter would put "rbac" here.

I think I would prefer we have a dynamic metadata key for the RBAC filter and make it a bool. This is the intended use of dynamic metadata, which avoids having to coordinate on key namespace across arbitrary number of filters.

+1 to make it a bool.

What about the scenarios where multiple filters can give a decision to logging filter to log/not. For example, we have a wasm sampling filter that also decides whether a request should be logged or not. It would be good to have a key that is generic enough and not filter specific so that the consuming filter can make a decision just based on the value of this key rather than needing to have knowledge of all filters from whom, it can get a decision of, to log or not.

Please specify in the docs how this interacts with Envoy's default file and gRPC access loggers. You might want to add a field that indicates what metadata controls logging to them potentially. I think you should also pick naming of the key space that indicates how this is supposed to be used when taking into account the variety of loggers; basically what I'm after is a solution that makes sense for Envoy independent of Istio.

Sounds good. To clarify, we'll use the same key for use in all access loggers. It will be called something like "access_log_policy", which conveys its use for determining whether to output access logs. All loggers have the option of checking this key when invoked. For envoy access loggers specifically, we'd provide an access log filter that can be specified in the logger config that checks this key.

OK, sounds good. Will this be a boolean or some string? What are the contents of this string if it's a policy?

For flexibility purposes @gargnupur and @kyessenov preferred a string. Currently I just have it set to "yes" or "no", but potential use case for a string would be instead of "yes" to put the deciding filter name, for debugging or some other reason. I could go either way on this, whatever the general consensus is.

We're going with a boolean value now.

htuch · 2020-06-23T23:03:14Z

api/envoy/config/rbac/v4alpha/rbac.proto

Does this still effectively allow?

It allows all requests. I'll make it more clear.

davidraskin · 2020-06-23T23:11:41Z

@liminw @yangminzhu

api/envoy/config/rbac/v3/rbac.proto

yangminzhu · 2020-06-23T23:22:02Z

api/envoy/config/rbac/v4alpha/rbac.proto

repokitteh-read-only · 2020-06-24T16:59:29Z

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

🐱

Caused by: #11705 was synchronize by davidraskin.

see: more, trace.

htuch · 2020-06-24T22:45:19Z

api/envoy/config/rbac/v3/rbac.proto

Should this key be qualified by RBAC filter?

This is probably the only comment I have still outstanding.

+1. The "envoy" part in "envoy.log" does not provide much value.

htuch · 2020-06-24T22:46:13Z

docs/root/configuration/http/http_filters/rbac_filter.rst

I think I would prefer we have a dynamic metadata key for the RBAC filter and make it a bool. This is the intended use of dynamic metadata, which avoids having to coordinate on key namespace across arbitrary number of filters.

api/envoy/config/rbac/v3/rbac.proto

davidraskin · 2020-06-25T03:09:19Z

api/envoy/config/rbac/v4alpha/rbac.proto

I say that all requests are allowed explicitly here. I think it's clear enough right @yangminzhu ?

liminw · 2020-06-25T19:29:55Z

api/envoy/config/rbac/v3/rbac.proto

LOG action does not impact allow or deny decision. Let's remove "All requests are allowed" to avoid confusion.

To avoid confusion, I think its best for the user to know that all requests are allowed if the action is LOG rather than wonder what will happen with their request

api/envoy/config/rbac/v3/rbac.proto

liminw · 2020-06-25T19:35:56Z

docs/root/configuration/http/http_filters/rbac_filter.rst

+1 to make it a bool.

liminw · 2020-06-25T19:37:59Z

api/envoy/config/rbac/v3/rbac.proto

+1. The "envoy" part in "envoy.log" does not provide much value.

repokitteh-read-only · 2020-07-07T01:07:11Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy[\w/]*/(v1alpha\d?|v1|v2alpha\d?|v2))|(api/envoy/type/(matcher/)?\w+.proto).

🐱

Caused by: #11705 was synchronize by davidraskin.

see: more, trace.

yangminzhu · 2020-08-05T05:37:37Z

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

   *                   empty map should be used if there are no headers available.
   * @param info       the per-request or per-connection stream info with additional information
-   *                   about the action/principal.
+   *                   about the action/principal. Can be modified by an Action.


Can we be more specific that it could be modified only by an LOG action.

yangminzhu · 2020-08-05T05:40:36Z

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

+    return true;
+  }
+  default:
+    return true;


I think you can replace return true with NOT_REACHED_GCOVR_EXCL_LINE;, and remove the return true below.

yangminzhu · 2020-08-05T05:44:57Z

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

      config_->engine(callbacks_->route(), Filters::Common::RBAC::EnforcementMode::Enforced);
  if (engine != nullptr) {
-    if (engine->allowed(*callbacks_->connection(), headers, callbacks_->streamInfo(), nullptr)) {
+    // Check authorization decision and do Action operations


This comment seems a bit redundant.

yangminzhu · 2020-08-05T05:54:27Z

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

+  checkEngine(engine, expected, empty_info, connection, headers);
+}
+
+void checkEngineLog(


Can we remove this function and combine the check into the checkEngine()? You can still pass the LogResult to it. This gives us more coverage with less code.

That would probably mean having every test check log metadata even when its an Allow or Deny test

isn't that better? we make sure the Allow/Deny doesn't write to the metadata accidentally.

Yeah I guess that's true.

yangminzhu · 2020-08-05T05:55:12Z

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

+  checkEngineLog(engine, RBAC::LogResult::Undecided, info, conn, headers);
+}
+
+TEST(RoleBasedAccessControlEngineImpl, LogAllowAll) {


This would be duplicated and can be removed if you combine checkEngineLog() to checkEngine(), see the comment above.

yangminzhu · 2020-08-05T05:59:05Z

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

+  EXPECT_EQ(Http::FilterHeadersStatus::Continue, log_filter_.decodeHeaders(headers, false));
+  auto filter_meta = req_info_.dynamicMetadata().filter_metadata().at(
+      Filters::Common::RBAC::DynamicMetadataKeysSingleton::get().CommonNamespace);
+  EXPECT_EQ(false, filter_meta.fields()


You can just use EXPECT_FALSE.

yangminzhu · 2020-08-05T06:02:50Z

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

  EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.decodeTrailers(trailers_));
+
+  // Check Log
+  setMetadata();


Call the setMetadata() in the beginning, also remove the extra call of EXPECT_EQ(Http::FilterHeadersStatus::Continue, ...) which is already called above.

This checks the log action in the same test as the regular action. After refactoring some stuff from before, it might make sense to do all log tests this way. Do you think its better to combine log tests with Allow tests or should we keep them separate?

let's combine them together so that we also cover the case that non-LOG action will not write to the metadata accidentally.

I meant testing that LOG action will log whenever ALLOW action allows. So I'm using two separate configs in these tests. This allows me to easily add checks for log action for other cases like Path and RequestedServerName. Do you think that's too confusing?

I think this is unnecessary and complicates the tests with two configs. Like I said before, just initialize the config to the action you want to test, and then check the behavior:

for ALLOW action, it should allow the request and deny by default, the metadata should not change

for DENY action, it should deny if explicitly matched and allow if not, the metadata should not change

for LOG action, it should always allow, and the metadata should change if matched

Sure sounds good.

yangminzhu · 2020-08-05T06:03:35Z

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

  };
  EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(headers, false));
+
+  headers = Http::TestRequestHeaderMapImpl{


what's the purpose of this and the next line?

I added an extra test to Path, I thought it could maybe use a verify false test? I'll remove it. It's probably not necessary.

isn't it duplicate with the above one?

":path" is set to "prefix/suffix/next" for the first one and "/suffix#seg?param=value" for the second one.

yangminzhu · 2020-08-05T06:04:17Z

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

+  EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_.decodeHeaders(headers, false));
+
+  // Check Log
+  setMetadata();


same here, move the setMetadata() to the above, and remove the extra EXPECT_EQ() in the next line.

yangminzhu · 2020-08-05T06:23:19Z

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

  RoleBasedAccessControlFilterConfigSharedPtr config_;
-
  RoleBasedAccessControlFilter filter_;
+  RoleBasedAccessControlFilterConfigSharedPtr log_config_;


no, let's use only one filter_ and config_ otherwise it gets really complicated of which one is being tested in a test case.

You can have a custom setUp(envoy::config::rbac::v3::RBAC::Action action) that creates a new config/filter and assign to the config_/filter_ in the beginning of each test case, it also combine the SetUp().

Similar to what you are doing in the network/rbac/filter_test.cc

This makes more verbose to check log and non-log actions in the same test, which I do a couple times, but for clarity I can change it.

why more verbose? they are testing different things, for a non-LOG action, we also need to make sure it doesn't write to the metadata.

I'm testing both configs in some tests, see below comment.

Signed-off-by: davidraskin <draskin@google.com>

davidraskin · 2020-08-06T19:17:11Z

@mattklein123 @yangminzhu does this look good now? Thanks.

yangminzhu

Thanks! Looks good to me and only 1 small comment.

yangminzhu · 2020-08-06T19:22:11Z

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

+    NOT_REACHED_GCOVR_EXCL_LINE;
+  }
+
+  return true;


could this be removed since all cases are handled in the switch?

Yep. Thanks.

Signed-off-by: davidraskin <draskin@google.com>

davidraskin · 2020-08-07T17:44:37Z

@mattklein123 Could you please take a look? Thanks!

davidraskin · 2020-08-10T17:22:24Z

@mattklein123 Does this PR look okay now or is there more that needs to be done? Thanks.

mattklein123 · 2020-08-10T17:26:58Z

@mattklein123 Does this PR look okay now or is there more that needs to be done? Thanks.

This is in my review queue. Asking me repeatedly to review it is doing the opposite of what you are hoping for. Thank you for your understanding!

davidraskin · 2020-08-10T17:32:20Z

Apologies, and sorry for being obnoxious. This was just a blocking PR on my end and I just wanted to make sure you hadn't missed it for whatever reason. I fully understand that there's other PRs that are higher priority and need to be reviewed first. Thanks!

mattklein123

LGTM with small merge issue. Thank you!

/wait

mattklein123 · 2020-08-10T20:17:11Z

docs/root/version_history/current.rst

 * lua: added Lua APIs to access :ref:`SSL connection info <config_http_filters_lua_ssl_socket_info>` object.
 * postgres network filter: :ref:`metadata <config_network_filters_postgres_proxy_dynamic_metadata>` is produced based on SQL query.
-* ratelimit: added :ref:`enable_x_ratelimit_headers <envoy_v3_api_msg_extensions.filters.http.ratelimit.v3.RateLimit>` option to enable `X-RateLimit-*` headers as defined in `draft RFC <https://tools.ietf.org/id/draft-polli-ratelimit-headers-03.html>`_.
+* ratelimit: added :ref:`enable_x_ratelimit_headers <envoy_v3_api_msg_extensions.filters.http.ratelimit.v3.RateLimit>` option to enable `X-RateLimit-*` headers as defined in `draft RFC <https://tools.ietf.org/id/draft-polli-ratelimit-headers-02.html>`_.


merge issue on this line

You didn't actually fix the issue. This line needs to get reverted.

/wait

Oops, should be good now.

Signed-off-by: davidraskin <draskin@google.com>

mattklein123

Thanks!

davidraskin · 2020-08-11T00:22:48Z

Thank you!

repokitteh-read-only bot added the api label Jun 23, 2020

davidraskin force-pushed the api_change branch from ceb6fd7 to 7f21b1e Compare June 23, 2020 04:29

asraa assigned htuch Jun 23, 2020

htuch reviewed Jun 23, 2020

View reviewed changes

yangminzhu reviewed Jun 23, 2020

View reviewed changes

davidraskin force-pushed the api_change branch from 35746d9 to 6ed3d02 Compare June 24, 2020 17:02

htuch reviewed Jun 24, 2020

View reviewed changes

api/envoy/config/rbac/v3/rbac.proto Outdated Show resolved Hide resolved

api/envoy/config/rbac/v3/rbac.proto Outdated Show resolved Hide resolved

davidraskin commented Jun 25, 2020

View reviewed changes

liminw reviewed Jun 25, 2020

View reviewed changes

mattklein123 self-assigned this Jun 28, 2020

davidraskin force-pushed the api_change branch 2 times, most recently from f254b36 to aa28d67 Compare July 7, 2020 01:06

davidraskin requested review from PiotrSikora, alyssawilk, dio, jmarantz, junr03, lizan, snowp and zuercher as code owners July 7, 2020 01:06

repokitteh-read-only bot added the v2-freeze label Jul 7, 2020

davidraskin force-pushed the api_change branch 3 times, most recently from a5ccc9c to 7f21b1e Compare July 7, 2020 03:41

yangminzhu reviewed Aug 5, 2020

View reviewed changes

fix format

31d5e60

Signed-off-by: davidraskin <draskin@google.com>

mattklein123 added waiting:any and removed v2-freeze labels Aug 5, 2020

repokitteh-read-only bot removed the waiting:any label Aug 6, 2020

davidraskin and others added 4 commits August 6, 2020 16:03

Update test logic

b2881d2

Signed-off-by: davidraskin <draskin@google.com>

Merge remote-tracking branch 'upstream/master' into api_change

4599a19

Merge branch 'master' into api_change

11f3d03

Merge remote-tracking branch 'origin/api_change' into api_change

f1e7b1c

davidraskin mentioned this pull request Aug 6, 2020

Add audit log stream to stackdriver filter istio/proxy#2970

Merged

clang-tidy fix

6c11ca5

Signed-off-by: davidraskin <draskin@google.com>

yangminzhu previously approved these changes Aug 6, 2020

View reviewed changes

Remove unneeded return

879729c

Signed-off-by: davidraskin <draskin@google.com>

davidraskin dismissed yangminzhu’s stale review via 879729c August 6, 2020 19:54

mattklein123 requested changes Aug 10, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Aug 10, 2020

Merge remote-tracking branch 'upstream/master' into api_change

582f5ad

repokitteh-read-only bot added waiting and removed waiting labels Aug 10, 2020

Hopefully fixed merge issue

37b7425

Signed-off-by: davidraskin <draskin@google.com>

repokitteh-read-only bot removed the waiting label Aug 10, 2020

mattklein123 approved these changes Aug 11, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Aug 11, 2020

mattklein123 merged commit 89b594e into envoyproxy:master Aug 11, 2020

Conversation

davidraskin commented Jun 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Jun 23, 2020

Uh oh!

davidraskin commented Jun 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyessenov Jun 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

davidraskin Jun 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidraskin commented Jun 23, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

repokitteh-read-only bot commented Jun 24, 2020

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!

Uh oh!

Uh oh!

davidraskin Jun 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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 Jul 7, 2020

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!

davidraskin commented Jun 23, 2020 •

edited

Loading

kyessenov Jun 24, 2020 •

edited

Loading

davidraskin Jun 23, 2020 •

edited

Loading

davidraskin Jun 25, 2020 •

edited

Loading

davidraskin Aug 5, 2020 •

edited

Loading