-
Notifications
You must be signed in to change notification settings - Fork 5.5k
rbac: add metadata support to rbac filter #3638
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 5 commits
91c2423
75fcb1f
5d009dc
4301c1d
5d4e01d
530dedd
a9467d4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| load("//bazel:api_build_system.bzl", "api_go_proto_library", "api_proto_library") | ||
|
|
||
| licenses(["notice"]) # Apache 2 | ||
|
|
||
| api_proto_library( | ||
| name = "metadata", | ||
| srcs = ["metadata.proto"], | ||
| visibility = ["//visibility:public"], | ||
| deps = [ | ||
| ":number", | ||
| ":string", | ||
| ], | ||
| ) | ||
|
|
||
| api_go_proto_library( | ||
| name = "metadata", | ||
| proto = ":metadata", | ||
| deps = [ | ||
| ":number_go_proto", | ||
| ":string_go_proto", | ||
| ], | ||
| ) | ||
|
|
||
| api_proto_library( | ||
| name = "number", | ||
| srcs = ["number.proto"], | ||
| visibility = ["//visibility:public"], | ||
| deps = [ | ||
| "//envoy/type:range", | ||
| ], | ||
| ) | ||
|
|
||
| api_go_proto_library( | ||
| name = "number", | ||
| proto = ":number", | ||
| deps = [ | ||
| "//envoy/type:range_go_proto", | ||
| ], | ||
| ) | ||
|
|
||
| api_proto_library( | ||
| name = "string", | ||
| srcs = ["string.proto"], | ||
| visibility = ["//visibility:public"], | ||
| ) | ||
|
|
||
| api_go_proto_library( | ||
| name = "string", | ||
| proto = ":string", | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| syntax = "proto3"; | ||
|
|
||
| package envoy.type.matcher; | ||
| option go_package = "matcher"; | ||
|
|
||
| import "envoy/type/matcher/string.proto"; | ||
| import "envoy/type/matcher/number.proto"; | ||
|
|
||
| import "validate/validate.proto"; | ||
|
|
||
| // [#protodoc-title: MetadataMatcher] | ||
|
|
||
| // MetadataMatcher provides a general interface to check if a given value is matched in | ||
| // :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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: ref link to rbac message.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
| // 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. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does this mean exactly? Clarify?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| message PathSegment { | ||
| oneof segment { | ||
| option (validate.required) = true; | ||
|
|
||
| // If specified, use the key to retrieve the value in a Struct. | ||
| string key = 1 [(validate.rules).string.min_bytes = 1]; | ||
| } | ||
| } | ||
|
|
||
| // Specifies the value to match. Only primitive value is supported. For non-primitive value, the | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "are supported", "For non-primitive values,"
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
| // result is always not matched. | ||
| message Value { | ||
| // NullMatch is an empty message to specify a null value. | ||
| message NullMatch { | ||
| } | ||
|
|
||
| // Specifies how to match a value. Only have effect on primitive value. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you can drop "Only have effect on primitive value.". I would describe/clarify above that we don't currently support embedded structs (or however you want to say it).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually we support embedded structs, but don't support list for now. |
||
| oneof match_pattern { | ||
| option (validate.required) = true; | ||
|
|
||
| // If specified, it's matched if and only if the target value is a NullValue. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/it's matched/a match occurs (same in all cases below)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
| NullMatch null_match = 1; | ||
|
|
||
| // If specified, it's matched if and only if the target value is a double value and is matched | ||
| // to this field. | ||
| DoubleMatcher double_match = 2; | ||
|
|
||
| // If specified, it's matched if and only if the target value is a string value and is matched | ||
| // to this field. | ||
| StringMatcher string_match = 3; | ||
|
|
||
| // If specified, it's matched if and only if the target value is a bool value and is equal to | ||
| // this field. | ||
| bool bool_match = 4; | ||
|
|
||
| // 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 = 5; | ||
| } | ||
| } | ||
|
|
||
| // Required. The filter name to retrieve the Struct from the Metadata. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Required." is done automatically and inferred from validations, you can remove. Here and below.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
| string filter = 1 [(validate.rules).string.min_bytes = 1]; | ||
|
|
||
| // Required. The path to retrieve the Value from the Struct. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
| repeated PathSegment path = 2 [(validate.rules).repeated .min_items = 1]; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shall we use fieldmask? You can write paths as "a.b.c".
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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. |
||
|
|
||
| // 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]; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A repeated values make it easier to use when a user want to share the same common information in a metadata matcher, like the 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, thanks for the explanation, removed repeated here. |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| syntax = "proto3"; | ||
|
|
||
| package envoy.type.matcher; | ||
| option go_package = "matcher"; | ||
|
|
||
| import "envoy/type/range.proto"; | ||
|
|
||
| import "validate/validate.proto"; | ||
|
|
||
| // [#protodoc-title: NumberMatcher] | ||
|
|
||
| // Specifies the way to match a double value. | ||
| message DoubleMatcher { | ||
| oneof match_pattern { | ||
| option (validate.required) = true; | ||
|
|
||
| // If specified, the input double value must be in the range specified here. | ||
| envoy.type.DoubleRange range = 1; | ||
|
|
||
| // If specified, the input double value must be equal to the value specified here. | ||
| double exact = 2; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| syntax = "proto3"; | ||
|
|
||
| package envoy.type.matcher; | ||
| option go_package = "matcher"; | ||
|
|
||
| import "validate/validate.proto"; | ||
|
|
||
| // [#protodoc-title: StringMatcher] | ||
|
|
||
| // Specifies the way to match a string. | ||
| message StringMatcher { | ||
| oneof match_pattern { | ||
| option (validate.required) = true; | ||
|
|
||
| // The input string must match exactly the string specified here. | ||
| // | ||
| // Examples: | ||
| // | ||
| // * *abc* only matches the value *abc*. | ||
| string exact = 1; | ||
|
|
||
| // The input string must have the prefix specified here. | ||
| // Note: empty prefix is not allowed, please use regex instead. | ||
| // | ||
| // Examples: | ||
| // | ||
| // * *abc* matches the value *abc.xyz* | ||
| string prefix = 2 [(validate.rules).string.min_bytes = 1]; | ||
|
|
||
| // The input string must have the suffix specified here. | ||
| // Note: empty prefix is not allowed, please use regex instead. | ||
| // | ||
| // Examples: | ||
| // | ||
| // * *abc* matches the value *xyz.abc* | ||
| string suffix = 3 [(validate.rules).string.min_bytes = 1]; | ||
|
|
||
| // The input string must match the regular expression specified here. | ||
| // The regex grammar is defined `here | ||
| // <http://en.cppreference.com/w/cpp/regex/ecmascript>`_. | ||
| // | ||
| // Examples: | ||
| // | ||
| // * The regex *\d{3}* matches the value *123* | ||
| // * The regex *\d{3}* does not match the value *1234* | ||
| // * The regex *\d{3}* does not match the value *123.456* | ||
| string regex = 4; | ||
| } | ||
| } |
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: s/A metadata/Metadata. Same below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done