authz: RBAC filter config PBs + flexibility changes by rodaine · Pull Request #3477 · envoyproxy/envoy

rodaine · 2018-05-23T21:27:38Z

This patch splits out the proto config changes for the RBAC filter from #3455 for easier review, as well as to unblock implementors from consuming these configs ASAP. Of note, these changes make the rules for permissions/principals more explicit in terms of their application (AND vs OR) as well as permits mixing and matching different types of matchers together within those logical operators. This facilitates inflation of these rules into the matchers within the RBAC engine inside Envoy as well as affords an extra level of flexibility to defining the config. These changes are compatible with the previous definitions, if more verbose.

Risk Level: Low (unused proto changes)
Testing: N/A
Docs: N/A
Release Notes: N/A

Signed-off-by: Chris Roche croche@lyft.com

Signed-off-by: Chris Roche <croche@lyft.com>

liminw · 2018-05-23T22:11:52Z

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

@@ -68,90 +74,70 @@ message Policy {
  repeated Principal principals = 2 [(validate.rules).repeated .min_items = 1];


It seems that there is space between "repeated" and ".min_items"? Should it be removed?

The clang-format rules doesn't distinguish between the option name and the repeated decorator for a field. Unfortunately this has to remain but it's fortunately compatible with protoc

liminw · 2018-05-23T22:21:22Z

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


-      // Source IP addresses.
-      IpMatch source_ips = 2;
+    // A set of identifiers that all must match in order to define the principal.


"all must match"->"at least one must match".

doh good catch

liminw · 2018-05-23T22:27:25Z

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

-    // ":ref: `AttributeContext <envoy_api_msg_service.auth.v2alpha.AttributeContext>`.
-    // If unset, it applies to any user.
+    // The name of the principal. If set, the URI SAN is used from the certificate, otherwise the
+    // subject field is used. If unset, it applies to any user that is authenticated.


What is the "subject field"? Is "unset" of this field equivalent to "any" field set to "true"?

This is the same behavior as in ext authz / AttributeContext, it uses the URI SAN if present, otherwise, it uses the subject field off the certificate.

if a principal has authenticated set with its name unset it means that the request must be authenticated, but does not care who the subject is.

liminw · 2018-05-23T22:28:27Z

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

 message Principal {
+
+  message Set {
+    repeated Principal ids = 1 [(validate.rules).repeated .min_items = 1];


space between "repeated" and ".min_items"?

Signed-off-by: Chris Roche <croche@lyft.com>

mattklein123

LGTM, this is great, thanks. Just some small doc asks.

mattklein123 · 2018-05-24T00:24:55Z

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

-  repeated envoy.type.StringMatch values = 2;
-}
+  message Set {
+    repeated Permission rules = 1 [(validate.rules).repeated .min_items = 1];


Per offline convo can we add some more docs/explanation of what the AND/OR behavior is of this repeated lists? I think that would be helpful.

Ah, these are just wrappers since you can't have a repeated field in a oneof. Adding a comment to specify that the application is dependent on whether it's in and_rules/ids vs or_rules/ids

Sorry, I commented in the wrong place. :(

I meant here: https://github.com/rodaine/envoy/blob/c18561dfce1acc3e9cd101149f21b786a8384626/api/envoy/config/rbac/v2alpha/rbac.proto#L71-L74

(But more comments here fine too)

mattklein123 · 2018-05-24T00:25:51Z

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


-  // Optional. Custom conditions.
-  repeated Condition conditions = 3;
+    // When any is set, it matches any action.


On this field (and the same one on principal) can we add a bit more docs explaining the usage (effectively that it's explicit and a policy needs to have at least one permission and principal).

Signed-off-by: Chris Roche <croche@lyft.com>

mattklein123

This is awesome, just found a small typo, thanks.

mattklein123 · 2018-05-24T03:54:12Z

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

 //
 message RBAC {
-  // Should we do white-list or black-list style access control.
+  // Should we do white-list or black-list style access control?


I think you had a typo here?

Nevermind I see you made this into a question.

Fix the description of an `RBAC` Protobuf message example, so that it matches the changes made to the YAML code block in PR envoyproxy#3477. Risk Level: Low Testing: N/A Docs Changes: N/A Release Notes: N/A Signed-off-by: Tal Nordan <tal.nordan@solo.io>

Fix the description of an `RBAC` Protobuf message example, so that it matches the changes made to the YAML code block in PR #3477. Risk Level: Low Testing: N/A Docs Changes: N/A Release Notes: N/A Signed-off-by: Tal Nordan <tal.nordan@solo.io>

authz: RBAC filter config PBs + flexibility changes

14c768c

Signed-off-by: Chris Roche <croche@lyft.com>

rodaine mentioned this pull request May 23, 2018

authz: RBAC HTTP filter + utilities #3455

Merged

liminw reviewed May 23, 2018

View reviewed changes

fix or_ids

c18561d

Signed-off-by: Chris Roche <croche@lyft.com>

rodaine force-pushed the rbac-protos branch from 28ef9f6 to c18561d Compare May 23, 2018 23:17

liminw previously approved these changes May 23, 2018

View reviewed changes

mattklein123 reviewed May 24, 2018

View reviewed changes

improve comments per comments

90e86ce

Signed-off-by: Chris Roche <croche@lyft.com>

rodaine dismissed liminw’s stale review via 90e86ce May 24, 2018 00:55

mattklein123 reviewed May 24, 2018

View reviewed changes

mattklein123 approved these changes May 24, 2018

View reviewed changes

mattklein123 merged commit 9d0f0a8 into envoyproxy:master May 24, 2018

rodaine deleted the rbac-protos branch May 25, 2018 22:07

talnordan mentioned this pull request Jul 19, 2018

authz: fix RBAC filter config PB docs #3895

Merged

		@@ -68,90 +74,70 @@ message Policy {
		repeated Principal principals = 2 [(validate.rules).repeated .min_items = 1];

Conversation

rodaine commented May 23, 2018

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants