rbac: add metadata support to rbac filter by yangminzhu · Pull Request #3638 · envoyproxy/envoy

yangminzhu · 2018-06-15T02:54:17Z

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

title: rbac: add metadata support to rbac filter

Description:

Added a api/envoy/type/matcher/{metadata.proto, number.proto, string.proto} and namespace Envoy::Matcher for matcher
Added the new MetadataMatcher message to support a generic way to match a value in the Metadata
Modified the RBAC filter to use the MetadataMatcher and pass the metadata from the dynamicMetadata()

Note: This PR is actually different from my poc and the description in #3624. I changed to not to include a full Metadata in the RBAC policy, instead to use a more flexible way to retrieve a value from it and then do the custom match. The new way is much more simpler and gives us almost the same flexibility.

Risk Level: Low

Testing:
Added and ran unit tests.

Docs Changes:
N/A

Release Notes:
N/A

Fixes #3624

cc @lizan @liminw @diemtvu @qiwzhang @quanjielin

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

lizan

first pass on API level, this is in good direction

lizan · 2018-06-15T04:13:49Z

api/envoy/api/v2/core/base.proto

+
+      // If specified, value match will be performed based on double value. It's matched if and only
+      // if the target value is a double value and is equal to this field.
+      double number_match = 2;


floating number is (almost) impossible to do exact match, consider add a range match like envoy.type.Int64Range

Updated to use a DoubleMatcher.

lizan · 2018-06-15T04:14:01Z

api/envoy/api/v2/core/base.proto

+      // Examples:
+      //
+      // * *abc* will only match to string value *abc*.
+      string exact_match = 3;


Can we reuse StringMatch here?

lizan · 2018-06-15T04:19:56Z

api/envoy/api/v2/core/base.proto

+    // Specifies how to match a value. Only have effect on primitive value.
+    // For non-primitive value, it's always not matched.
+    oneof match_specifier {
+      option (validate.required) = true;


instead of listing all combination of type and its matcher here, would it be clearer to make a Matcher type (with oneof) for each type and have a oneof here?

i.e.:

message NullMatch { } message NumberMatch { oneof match { Int64Range range_match = 1; int64 exact_match = 2; } } message ValueMatch { oneof match { NullMatch null = 1; NumberMatch number = 2; StringMatch string = 3; ... } }

I think that makes sense for string, number or other non primitive type, for bool I just kept it there as it's much simpler to use and also easy to compare in the code.

mattklein123 · 2018-06-15T17:29:43Z

api/envoy/api/v2/core/base.proto

+//
+// An example use of MetadataMatcher is specifying additional metadata in envoy.filters.http.rbac to
+// enforce access control based on dynamic metadata in a request.
+message MetadataMatcher {


I have a general comment here about something that's been bothering me that I think we should get on top of now before it gets even worse.

We have a ton of different matching constructs in the API that at the end of the day are all basically the same. This includes and/or logic, headers, matching on various other things such as response flags, health check, etc.

I suggest we make a new namespace/folder called "matchers" and start moving all of the different matchers into that namespace. Also, I just looked, and is StringMatch used anywhere? I don't think so, so can we delete it as part of this change?

Thoughts? cc @htuch

I can start this change in this PR, (Initially only for MetadataMatcher and then move other Matchers to it in follow up PR).
As for StringMatch, I think it makes sense to make it a standalone Matcher under that new directory/namespace, at least it could be used by this MetadataMatcher.

Should I start it right in this PR? Thanks!
cc @lizan

+1 I would like to see more standardization across the entire API of these match patterns.

Added a new directory api/envoy/type/matchers and namespace Envoy::Matchers for the matchers. Currently it doesn't include and/or logic, headers, health check, etc. I prefer to do those in a different PR with a clear description of use case and goal.
Thanks!

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

lizan · 2018-06-19T19:04:05Z

api/envoy/type/matchers/metadata.proto

@@ -0,0 +1,59 @@
+syntax = "proto3";
+
+package envoy.type.matchers;


envoy.type.matcher (no -s)

lizan · 2018-06-19T19:05:06Z

api/envoy/type/matchers/metadata.proto

+  string filter = 1 [(validate.rules).string.min_bytes = 1];
+
+  // Required. The multi-key path to retrieve the Value from the Struct.
+  repeated string path = 2 [(validate.rules).repeated .min_items = 1];


it is unclear to me how this works for array, notes?

Added comment, it's actually not supported and will just result a not match.

lizan · 2018-06-19T19:06:39Z

api/envoy/type/matchers/metadata.proto

+      // If specified, value match will be performed based on whether the path is referring to a
+      // valid primitive value in the metadata. If the path is referring to a non-primitive value,
+      // the result is always not matched.
+      bool present_match = 7;


lizan · 2018-06-19T19:07:09Z

api/envoy/type/matchers/metadata.proto

+    oneof match_pattern {
+      option (validate.required) = true;
+
+      // If specified, it's matched if and only if the target value is a NullValue and this field is


what is the use case of setting null_match to false? (same for below)

Not too much real use case, I make it a bool to simplify the Value design and also feel it's easier to set a bool field for the user.

I would prefer using a empty message, like here:
https://github.com/envoyproxy/envoy/blob/master/api/envoy/api/v2/route/route.proto#L763

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

lizan · 2018-06-20T06:14:22Z

api/envoy/type/matchers/metadata.proto

@@ -0,0 +1,60 @@
+syntax = "proto3";
+
+package envoy.type.matcher;


You'll need to rename directories etc..

lizan · 2018-06-20T06:15:27Z

api/envoy/type/matchers/metadata.proto

+    oneof match_pattern {
+      option (validate.required) = true;
+
+      // If specified, it's matched if and only if the target value is a NullValue and this field is


I would prefer using a empty message, like here:
https://github.com/envoyproxy/envoy/blob/master/api/envoy/api/v2/route/route.proto#L763

lizan · 2018-06-20T06:19:00Z

api/envoy/type/matchers/metadata.proto

+  string filter = 1 [(validate.rules).string.min_bytes = 1];
+
+  // Required. The multi-key path to retrieve the Value from the Struct.
+  // Note: Array is not supported and will result a not match discarding any specified values.


Still questionable how this will be extended to support array. Will it make more sense that each segment be oneof (key, index)? Needs more thoughts.

Updated to a message with a oneof field. We only need string key for now and the current metadataValue() also only supports string keys.
It should be easy to extend to include index, I prefer to do that when we really need to use a index or at least in a different PR.

yeah that's no problem, metadataValue take Metadata as input, so it could contain array anyway. make this PathSegment will allow us to extend this to support array without breaking backward compatibility.

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

lizan

LGTM, thanks!

yangminzhu · 2018-06-21T18:02:33Z

@mattklein123 @htuch PTAL, Thank you!

mattklein123 · 2018-06-25T00:26:41Z

Sorry for the delay I will get to this tomorrow.

mattklein123

Thanks for working on this! Some comments to get started.

mattklein123 · 2018-06-25T16:31:27Z

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

    // A port number that describes the destination port connecting to.
    uint32 destination_port = 6 [(validate.rules).uint32.lte = 65535];
+
+    // A metadata that describes additional information about the action.


nit: s/A metadata/Metadata. Same below.

mattklein123 · 2018-06-25T16:32:28Z

api/envoy/type/matcher/metadata.proto

+// :ref:`Metadata <envoy_api_msg_core.Metadata>`. It uses `filter` and `path` to retrieve the value
+// from the Metadata and then check if it's matched to one of the specified values.
+//
+// An example use of MetadataMatcher is specifying additional metadata in envoy.filters.http.rbac to


nit: ref link to rbac message.

mattklein123 · 2018-06-25T16:33:19Z

api/envoy/type/matcher/metadata.proto

+// enforce access control based on dynamic metadata in a request.
+message MetadataMatcher {
+  // Specifies the segment in a path to retrieve value from Metadata.
+  // Note: Array is not supported for now and will result a not match directly.


What does this mean exactly? Clarify?

mattklein123 · 2018-06-25T16:33:35Z

api/envoy/type/matcher/metadata.proto

+    }
+  }
+
+  // Specifies the value to match. Only primitive value is supported. For non-primitive value, the


"are supported", "For non-primitive values,"

mattklein123 · 2018-06-25T16:35:00Z

api/envoy/type/matcher/metadata.proto

+    oneof match_pattern {
+      option (validate.required) = true;
+
+      // If specified, it's matched if and only if the target value is a NullValue.


s/it's matched/a match occurs (same in all cases below)

mattklein123 · 2018-06-25T16:38:59Z

api/envoy/type/matcher/metadata.proto

+  // Required. The filter name to retrieve the Struct from the Metadata.
+  string filter = 1 [(validate.rules).string.min_bytes = 1];
+
+  // Required. The path to retrieve the Value from the Struct.


I don't really understand what a path segment is in this context. Can you add a full formed config example at the top of this file and explain it?

mattklein123 · 2018-06-25T16:41:10Z

source/common/common/matchers.cc

+bool DoubleMatcher::match(double value) const {
+  switch (matcher_.match_pattern_case()) {
+  case envoy::type::matcher::DoubleMatcher::kRange:
+    return matcher_.range().start() <= value && value < matcher_.range().end();


The docs don't specify whether the ranges are inclusive or exclusive, and it seems a bit odd to to inclusive at start but not at end? Any reason for this? Either way please clearly document.

I did this to follow the current Int64Range, The document mentioned this in the definition of DoubleRange, also added it to DoubleMatcher.

mattklein123 · 2018-06-25T16:41:32Z

source/common/common/matchers.cc

+  case envoy::type::matcher::DoubleMatcher::kExact:
+    return matcher_.exact() == value;
+  default:
+    return false;


This is checked by schema, switch to NOT_REACHED. Same elsewhere.

mattklein123 · 2018-06-25T16:43:14Z

source/common/common/matchers.h

+
+private:
+  const envoy::type::matcher::StringMatcher matcher_;
+  std::regex regex_;


Why isn't this part of StringMatcher?

StringMatcher proto has this but only as a simple string field, here I copy the parsed regex to a local variable so that it could be used directly later.

mattklein123 · 2018-06-25T16:45:37Z

api/envoy/type/matcher/metadata.proto

+
+  // Required. A set of values to match. The MetadataMatcher is matched if at least one value is
+  // matched, in other words, it's matched with OR semantics.
+  repeated Value values = 3 [(validate.rules).repeated .min_items = 1];


From looking at the code, IMO it's kind of strange that you force a matcher to be all one top of match. Why can't people just use multiple metadata matchers and make each match singular? Then all the normal and/or/etc. logic applies? I'm not sure we really need repeated in here?

A repeated values make it easier to use when a user want to share the same common information in a metadata matcher, like the filter and path field and I think this is what we need in most cases.

For more advanced and/or/etc. logic, I feel it's better to make it out of the MetadataMatcher, just like the existing RBAC matcher behavior: each specific matcher do one simple thing to match a single value, an outer and/or wrapper matcher is used to support complex and/or logic.

In Envoy we don't optimize for human use but instead assume machine generation. I don't think there is any need to have multiple ways of doing things, so I think we can remove the repeated here unless there is something that cannot be done otherwise? Thank you!

I see, thanks for the explanation, removed repeated here.

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

liminw · 2018-06-26T00:05:31Z

api/envoy/type/matcher/metadata.proto

+  string filter = 1 [(validate.rules).string.min_bytes = 1];
+
+  // The path to retrieve the Value from the Struct.
+  repeated PathSegment path = 2 [(validate.rules).repeated .min_items = 1];


Shall we use fieldmask? You can write paths as "a.b.c".

Fieldmask have some disadvantages, from my investigations, it cannot support either list or map which means we cannot use it directly here, unless we implement it by ourselves in Envoy.

Second, the semantics would be much more complicated, it won't be a.b.c, you have to list all field names in the path, so it will be something like fields.a.struct_value.fields.b.struct_value.fields.c.string_value.

Last, Envoy already have a well implemented metadataValue() method which satisfy all our needs here, so I think no need to use the fieldmask anymore.

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

yangminzhu · 2018-06-26T03:25:16Z

@mattklein123
I had removed the repeated in values, PTAL, thank you!

mattklein123

Nice!

rbac: add metadata support to rbac filter.

91c2423

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

lizan self-assigned this Jun 15, 2018

lizan reviewed Jun 15, 2018

View reviewed changes

mattklein123 reviewed Jun 15, 2018

View reviewed changes

lizan mentioned this pull request Jun 16, 2018

Jwt_authn config update: Support per route Jwt requirements #3381

Merged

yangminzhu added 2 commits June 18, 2018 13:42

create seperate directory and namespace for matchers.

75fcb1f

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

fix api and docs.

5d009dc

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

yangminzhu force-pushed the rbac_metadata branch from c748067 to 5d009dc Compare June 18, 2018 22:46

yangminzhu mentioned this pull request Jun 18, 2018

Tracking: make local RBAC available in Istio 1.0 istio/istio#6377

Closed

17 tasks

lizan reviewed Jun 19, 2018

View reviewed changes

address comments.

4301c1d

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

lizan reviewed Jun 20, 2018

View reviewed changes

rename matchers -> matcher, add NullMatch and PathSegment.

5d4e01d

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

yangminzhu force-pushed the rbac_metadata branch from a40489a to 5d4e01d Compare June 20, 2018 21:29

lizan previously approved these changes Jun 21, 2018

View reviewed changes

mattklein123 self-assigned this Jun 22, 2018

mattklein123 reviewed Jun 25, 2018

View reviewed changes

yangminzhu dismissed lizan’s stale review via dad21f5 June 25, 2018 21:00

address comments, update docs and small updates.

530dedd

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

yangminzhu force-pushed the rbac_metadata branch from dad21f5 to 530dedd Compare June 25, 2018 21:51

liminw reviewed Jun 26, 2018

View reviewed changes

address comments, make metadata.value not repeated.

a9467d4

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

mattklein123 approved these changes Jun 27, 2018

View reviewed changes

mattklein123 merged commit 9e7a3ab into envoyproxy:master Jun 27, 2018

yangminzhu deleted the rbac_metadata branch June 27, 2018 21:23

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

		package envoy.type.matchers;

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

		package envoy.type.matcher;

Conversation

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

Uh oh!

Uh oh!

lizan left a comment • 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!

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!

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!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

yangminzhu commented Jun 21, 2018

Uh oh!

mattklein123 commented Jun 25, 2018

Uh oh!

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

yangminzhu commented Jun 15, 2018 •

edited

Loading

lizan left a comment •

edited

Loading