-
Notifications
You must be signed in to change notification settings - Fork 267
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
Add protos for RBAC support #586
Conversation
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.
Thanks for getting this ball rolling! Wasn't sure how we felt at this point mutating the message schema's here, so there's a couple comments re the documentation + some related to the types used.
envoy/api/v2/auth/local_rbac.proto
Outdated
// }, | ||
// ] | ||
message LocalRBAC { | ||
// A list of ServicePolicy. One per service. |
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.
since this is 1-to-1, should it instead be a map to avoid having multiple ServicePolicies
with the same service name?
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.
Changed it to a map.
envoy/api/v2/auth/rbac.proto
Outdated
string group = 2; | ||
|
||
// Optional. The set of properties that identify the subject. | ||
map<string, string> properties = 3; |
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.
could we reuse the Constraint
message here instead?
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.
Or conversely for the constraints field above, could also use map<string, string>
instead?
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.
Or, why not use the AttributeContext directly? It seems to explicitly define the identifiers we want here.
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.
+1, let's make sure to use concrete attributes vs. abstract strings, and let's reuse the ones from extauth if possible.
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.
I removed all contents regarding abstract attributes.
The constraints above specifies that: for an attribute, what are the valid values. So it is 1:N.
Here in the binding, when we identify a subject using a set of attributes, we specify the value for the attributes. There is a single value for the attribute (1:1). BTW, I renamed "properties" to "attributes" based on @wenchenglu 's suggestion.
envoy/api/v2/auth/rbac.proto
Outdated
message RoleRef { | ||
// Required. The type of the role being referenced. | ||
// Currently, "ServiceRole" is the only supported value for "kind". | ||
string kind = 1 [(validate.rules).string.const = "ServiceRole"]; |
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.
can we use an enum here?
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.
+1
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.
Based on API style naming convention (https://cloud.google.com/apis/design/naming_convention#enum_names), "Enum values must use CAPITALIZED_NAMES_WITH_UNDERSCORES". But I want to use "ServiceRole" constant here, because it needs to match ServiceRole definition "kind: ServiceRole".
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.
The name is arbitrary (when serialized enums are represented as integers). We aren't doing string comparison between these are we?
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.
You don't even need this, you can always add this later or upgrade to oneof
without breaking backward compatibility. I'd quote STYLE.md again that prefers oneof
to enum + string.
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.
ok, let me remove "kind" for now. I can add it later if needed.
envoy/api/v2/auth/local_rbac.proto
Outdated
|
||
import "envoy/api/v2/auth/rbac.proto"; | ||
|
||
// LocalRBAC specifies Istio RBAC policies that apply to the local RBAC engine |
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.
Do we want to keep the Istio references throughout, or just link back to the existing Istio documentation for reference?
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.
I removed all Istio references.
envoy/api/v2/auth/local_rbac.proto
Outdated
// bindings: | ||
// - subjects: | ||
// - user: “cluster.local/ns/default/sa/admin” | ||
// roleRef: |
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.
since each Policy
holds its own ServiceRoleBinding
s, what is the need for the RoleRef
? Are there cases where the binding is independent of the Policy
?
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.
I made roleRef an optional field in ServiceRoleBinding. This way, when used in filter config, roleRef part can be skipped. RoleRef is needed when a user defines a ServiceRoleBinding object. I updated the example here and removed "roleRef" section.
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.
So you're saying when the Bindings is independent of the Policy? When does that happen?
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.
When a user defines ServiceRoleBinding object, which is independent of ServiceRole. See the example of ServiceRoleBinding in rbac.proto. RoleRef is included here for completeness of the proto.
Does the user interaction happen in control plane only? Is there a case an Envoy is deployed for a service without a control plane? In that case, a user may want to enter ServiceRole and ServiceRoleBinding independently, and the underlying tooling associate a binding to a role based on RoleRef.
I think @mattklein123 asked the same question.
Quick note (not full review). As part of this change, can we kill https://github.com/envoyproxy/data-plane-api/blob/master/envoy/api/v2/auth/auth.proto? It's currently not used and this functionality should be a full replacement for it. |
envoy/api/v2/auth/BUILD
Outdated
@@ -50,3 +50,31 @@ api_go_proto_library( | |||
"//envoy/api/v2/core:config_source_go_proto", | |||
], | |||
) | |||
|
|||
api_proto_library( |
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.
Shouldn't them go to api/filter/http/rbac/v2alpha
?
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.
So the RBAC filters will exist both at the listener (to support non-http binary protocols) as well as the router level, so placing them solely within the scope of the HTTP namespace would be misleading. Is there a better place you might suggest?
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.
Then create envoy/config/rbac/v2alpha
?
Per style guide here doesn't sound right.
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.
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.
I moved the files to the suggested location.
envoy/api/v2/auth/rbac.proto
Outdated
message RoleRef { | ||
// Required. The type of the role being referenced. | ||
// Currently, "ServiceRole" is the only supported value for "kind". | ||
string kind = 1 [(validate.rules).string.const = "ServiceRole"]; |
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.
+1
envoy/api/v2/auth/rbac.proto
Outdated
// gRPC methods must be presented as fully-qualified name in the form of | ||
// packageName.serviceName/methodName. | ||
// Exact match, prefix match, and suffix match are supported for paths. | ||
// For example, the path "/books/review" matches |
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.
Extract the combination of (exact match, prefix match, suffix match) to a message with oneof
like this (scroll down for oneof)? It is more self descriptive than a comment.
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.
This will change the language slightly. For example, with current proto, user can write
(1) rules:
- services: ["rullservicename", "prefix*", "suffix"]
(2) rules:
- services: [""]
If I change the proto to
oneof service_specifier {
string exact;
string prefix;
string suffix;
string any;
}
users will have to write the following:
(1) rules:
- services:
- exact: "fullservicename"
- prefix: "prefix*"
- suffix: "suffix"
(2) rules:
- services: [any: ""]
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.
Yes I understand this will change the language slightly though I think it's worth. The idea is keep this extensible and self describing. For example, someone may add regex later but you don't want always do regex.
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.
After a quick scan, my main feedback is that most of the docs can't be read independently of Istio's APIs. I think this is problematic, since Envoy's data plane API should be standalone. If the RBAC protos are intended to be used anywhere other than Istio, we need to have some way for the docs and semantics of fields to make sense without referencing the Istio API.
envoy/api/v2/auth/rbac.proto
Outdated
// object. | ||
message ServiceRoleBinding { | ||
// Required. List of subjects that are assigned the ServiceRole object. | ||
repeated Subject subjects = 1; |
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.
Can this be empty? Can we add a constrain that the list length is non-zero?
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.
Added a min_items constraint.
@liminw I agree that we need to purge all mention of Istio from this PR. I still haven't reviewed this and will do so by EOD but I wanted to +1 @htuch comment here. This inline RBAC API should be completely independent of Istio. |
envoy/api/v2/auth/local_rbac.proto
Outdated
@@ -0,0 +1,64 @@ | |||
syntax = "proto3"; | |||
|
|||
// [#proto-status: draft] |
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.
I think we can remove draft stuff as long as we go with v2alpha (same elsewhere). Doesn't really matter though as long as we do v2alpha.
envoy/api/v2/auth/rbac.proto
Outdated
// If set to ["*"] or not specified, it applies to any method. | ||
repeated string methods = 3; | ||
|
||
// Definition of a custom constraint. The key in the constraint refers to an Istio attribute name |
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.
For now my preference is to remove any abstract constraints. I don't see how they make sense for local execution in the proxy?
@liminw @rodaine @danielhochman @lizan at initial glance, I have some high level comments:
I know it's going to take time to sort this out, so thanks a bunch for getting this process going. This will be great when we converge. |
envoy/api/v2/auth/rbac.proto
Outdated
} | ||
|
||
// RoleRef refers to a role object. | ||
message RoleRef { |
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.
I think I'm probably missing something, but how will this information actually be used by the inline filter? Logging? Settings role headers? If so can we document more clearly?
envoy/config/rbac/v2alpha/rbac.proto
Outdated
// For example, destination.labels["app"], request.http.headers["version"]. | ||
// | ||
// Below is an example of ServiceRole object "product-viewer", which has "read" ("GET" and "HEAD") | ||
// access to "products.default.svc.cluster.local" service at versions "v1" and "v2". "path" is not |
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.
"path" is not specified, ...
Update either the comment or the example?
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.
Fixed.
envoy/config/rbac/v2alpha/rbac.proto
Outdated
// user: source.principal | ||
// group is currently undefined. It is reserved to be used in the future. | ||
// | ||
// Custom property name the attributes defined in AttributeContext |
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.
Custom property name should be the attributes...?
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.
This part of text is removed.
@@ -0,0 +1,63 @@ | |||
syntax = "proto3"; | |||
|
|||
package envoy.api.v2.auth; |
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.
update package to match the path.
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.
Fixed.
@mattklein123 @rodaine @htuch @lizan @yangminzhu Thanks for the reviews! I made the following major changes:
Question: |
@mattklein123 @rodaine I was trying to remove https://github.com/envoyproxy/data-plane-api/blob/master/envoy/api/v2/auth/auth.proto. It seems that it is currently used in https://github.com/envoyproxy/data-plane-api/blob/master/envoy/api/v2/route/route.proto. Can you check to make sure that you want to remove all the usage of auth.proto? I do not have knowledge how this proto is used today, and want to double confirm with you. |
@liminw the proto is not actually used in Envoy code. Go ahead and just remove from route.proto and just reserve the field that it was in (per the style.md docs). Thank you! |
@mattklein123 To answer your question on if RBAC covers all the functionality provided by existing auth.proto: RBAC defines rules based on attributes. The existing proto does access control on CertificateValidationContext. As long as we have all the relevant attributes, RBAC should be able to cover all the use cases. For the basic use cases like whitelisting calling service accounts for a service, you define a role with service name and allowed verbs, and list all the service accounts in the Binding. And it supports various AND/OR logics in Role and Binding definitions. In control plane, users can define roles that apply to all or a set of services (like the "service-admin" role and "tester" role examples https://istio.io/docs/concepts/security/rbac.html#servicerole). One thing keeping in mind is that, RBAC is deny by default (like many other authorization systems, e.g., K8S RBAC, Google Cloud IAM, AWS IAM). Users need to explicitly define RBAC policies to allow access. For the use case that you only want to restrict access for a few services, you can enable RBAC only for those services. The control to enable RBAC globally or only to a few services is normally configured in control plane (example for Istio: https://docs.google.com/document/d/1XkN9ls-ZDISz2uXGN20RaIre63DkJJOJNUOzcKCxbWA/edit#heading=h.lkkwo6ehmrav). |
@mattklein123 @rodaine @htuch @lizan @yangminzhu I think I have addressed all your comments. Please take a look when you get a chance. Thanks! |
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.
Thanks @liminw. This makes a lot more sense now, I have a bunch of feedback.
// | ||
// Here is an example of local RBAC policy, which contains policies for a single service. | ||
// { | ||
// "products.default.svc.cluster.local": { |
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.
Can you use pure YAML in your examples without superfluous JS braces? I think this will make it a bit easier to read.
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.
Will do. Thanks.
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. I moved this example to README.md.
// Optional. A list of HTTP verbs (e.g., "GET", "POST"). | ||
// It is ignored in gRPC case because the value is always "POST". | ||
// If set to ["*"] or not specified, it applies to any verb. | ||
repeated string verbs = 3; |
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.
This should probably be an enum.
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.
The most commonly used HTTP verbs are POST, GET, PUT, PATCH, and DELETE. But there are some uncommonly used ones, like HEAD, OPTIONS, TRACE, CONNECT. Customers may want to use their custom HTTP verbs, like PUBLISH. As long as the verb in policy matches the verb in the request, we will allow the request. For this reason, I do not want to limit the HTTP verbs users can use.
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.
AttributeContext uses the term "method", not "verb".
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.
I previously used the term "method", but I changed it to "verb" because I was trying to align with the terminology used in Kubernetes RBAC (https://kubernetes.io/docs/admin/authorization/rbac/).
envoy/config/rbac/v2alpha/rbac.proto
Outdated
// values: ["v1", "v2"] | ||
// | ||
// A ServiceRoleBinding specification includes two parts: | ||
// * "roleRef" refers to a ServiceRole object. |
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.
s/roleRef/role_ref/
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.
envoy/config/rbac/v2alpha/rbac.proto
Outdated
// path: request.http.path | ||
// | ||
// In addition, users can specify "constraints" on any attributes defined in AttributeContext | ||
// (//envoy/service/auth/v2/attribute_context.proto). |
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.
These links to proto files would benefit from being :api:
references (here and elsewhere).
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.
envoy/config/rbac/v2alpha/rbac.proto
Outdated
// * "roleRef" refers to a ServiceRole object. | ||
// * A list of "subjects" that are assigned the roles. | ||
// | ||
// A subject can be a "user" or a "group", or represented with a set of attributes. Specifically, |
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.
I think we need a really user friendly docs writeup of how this is all supposed to tie together. Thinking about this, and not knowing much about RBAC beyond the textbook parts, I'm guessing this all works because we can identify subjects via client certificates. How would this function without client certs? I think it would be helpful to explain overall context, provide an architecture diagram, explain how it works specifically for the kind of setup we have in places like Istio with mTLS, as well as how it can be adapted.
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.
+1 (my other comments)
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.
I added a README.md. Please take a look to see it answers all the questions here.
envoy/config/rbac/v2alpha/rbac.proto
Outdated
// * User (service account) "cluster.local/ns/default/sa/bookinfo-reviews" | ||
// * Service "reviews.default.svc.cluster.local" at version "v1". | ||
// | ||
// kind: ServiceRoleBinding |
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.
Where does this kind/metadata stuff come from?
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.
This is the case that ServiceRole and ServiceRoleBinding expressed in K8S CRD format. I explained it in the new README.md.
"The storage of ServiceRole and ServiceRoleBinding objects are platform specific. For example,
In Istio, they are stored as Kubernetes
CustomResourceDefinition (CRD)
objects. In the following document, we will see ServiceRole and ServiceRoleBinding examples
expressed in Kubernetes CRD format."
envoy/config/rbac/v2alpha/rbac.proto
Outdated
// - attributes: | ||
// source.service: "reviews.default.svc.cluster.local" | ||
// source.labels["version"]: "v1" | ||
// roleRef: |
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.
Please be consistent and use role_ref. We don't use the case switching in Envoy when going from proto name field to JSON/YAML.
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.
Fixed.
envoy/config/rbac/v2alpha/rbac.proto
Outdated
|
||
// ServiceRoleBinding assigns a ServiceRole to a list of subjects. | ||
// This represents the "Spec" part of the ServiceRoleBinding object. The name and namespace | ||
// of the ServiceRoleBinding is specified in "metadata" section of the ServiceRoleBinding |
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.
I don't see where metadata is..
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.
Removed this comment. "metadata" is really platform specific storage of ServiceRole/ServiceRoleBinding objects.
envoy/config/rbac/v2alpha/rbac.proto
Outdated
// "/books/review" (exact match), or "/books/*" (prefix match), | ||
// or "*/review" (suffix match). | ||
// If not specified, it applies to any path. | ||
repeated string paths = 2; |
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.
Do you want to perhaps use a RouteMatch
instead?
data-plane-api/envoy/api/v2/route/route.proto
Line 209 in f88047c
message RouteMatch { |
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.
The match applies to "services", "paths", "constraints" values. I added a StringMatch type, which is similar to RouteMatch.
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.
Thank you. This is definitely getting close. I added some more comments.
In general I'm still confused about how the things play together and how they will be used in the filters. It feels potentially overcomplicated to me. Perhaps more intro docs in the comments would help?
// } | ||
// }, | ||
// } | ||
message LocalRBAC { |
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.
I still honestly don't really understand the concepts here. Can you define before the example:
- Fully qualified service name
- Service policy
- RoleInfo
- ServiceRole
- ServiceRoleBindings
Basically, here is my 99% use case: I want to add some RBAC policies on a route/vhost, or that match various things at the global request level. I don't really care about services, roles, etc. Can you clarify how I accomplish my very basic case and then build from there?
At the documentation level, I still am confused on how "role" is actually going to be used. How will the filter actually operate on this information?
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.
I agree with Matt. We seem to introduce too many concepts for basic things. For most people, they want to let x do y. It can be as simple as:
- paths: ["/products/*"]
users: ["alice@yahoo.com"]
|
||
package envoy.config.rbac.v2alpha; | ||
|
||
// RBAC (Role Based Access Control) defines ServiceRole and ServiceRoleBinding |
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.
Hmm maybe in this section you are answering my questions. Can you merge the docs into one place and refer to the other place if needed?
envoy/config/rbac/v2alpha/rbac.proto
Outdated
// * "roleRef" refers to a ServiceRole object. | ||
// * A list of "subjects" that are assigned the roles. | ||
// | ||
// A subject can be a "user" or a "group", or represented with a set of attributes. Specifically, |
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.
+1 (my other comments)
envoy/config/rbac/v2alpha/rbac.proto
Outdated
// Definition of a custom constraint. The key in the constraint refers to an attribute defined in | ||
// AttributeContext (//envoy/service/auth/v2/attribute_context.proto). For example, | ||
// destination.labels["app"], request.http.headers["version"]. | ||
message Constraint { |
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.
There should be no string based constraints in here at all. IMO everything should be fully specified in protos. Otherwise the proxy will just have to translate strings to real things, which seems kind of silly. Let's just fully define the constraints we want, and then we can always add more later. Optimally you can use AttributeContext directly...
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.
RBAC should be modeled as "pattern matching on AttributeContext". The above comment seems to show the key refers to an attribute.
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.
The constraint is a condition defined on an attribute. For example, "destination.labels["app"] == "productpage"". For this purpose, users need to be able to refer to an attribute's name (which is the "key" here). The AttributeContext is a structure. Each field in the structure is an attribute.
I could introduce a map from attribute name to attribute values. The map is populated from the AttributeContext. I could either maintain the map in memory for RBAC implementation, or explicitly introduce a Attribute Map proto.
|
||
// Optional. Extra constraints in the ServiceRole specification. | ||
// The above ServiceRole examples shows an example of constraint "version". | ||
repeated Constraint constraints = 4; |
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.
I feel pretty strongly that we should allow both whitelist and blacklist modes, as its inevitably going to be asked for. Can we do something similar to the auth proto that we 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.
We need to decide how to pattern matching the attributes in an efficient way. Otherwise, the performance can be terrible. IMO, Envoy needs to process 10k requests per second, so 100us per request. We must have a performance goal when designing an infrastructure.
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.
@mattklein123 The existing RBAC systems, including Google Cloud IAM, Kubernetes RBAC, AWS IAM all use "deny by default". The policies explicitly allow access. We are just following the standard. On the other hand, if you have a real use case that requires blacklist, we can extend the current proto to address that use case.
@wora I was thinking of having the attributes in a map (attribute name->attribute value). This could either be a map in memory (for fast lookup), or explicitly define a attribute map proto.
@mattklein123 @htuch I will add a documentation of the end-to-end RBAC flow (from user input -> control plane -> enforcement at data plane). We are missing the architecture view of RBAC. That's where the main confusion is from, I think. |
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.
I am not sure if this design is actually better than AWS IAM Policy, which is status quo. This is a very significant design. We need to make sure we can compare to others for usability and performance.
|
||
import "envoy/config/rbac/v2alpha/rbac.proto"; | ||
|
||
// LocalRBAC specifies RBAC policies that apply to the local RBAC engine |
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.
Do we really need to know whether the RBAC is local or remote? Who should be reader of this info?
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.
"local" means the RBAC filter spec seen by the proxy. I explained it in README.md.
// } | ||
// }, | ||
// } | ||
message LocalRBAC { |
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.
I agree with Matt. We seem to introduce too many concepts for basic things. For most people, they want to let x do y. It can be as simple as:
- paths: ["/products/*"]
users: ["alice@yahoo.com"]
|
||
// AccessRule defines a permission to access a list of services. | ||
message AccessRule { | ||
// Required. A list of service names. |
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.
Where is the service name defined? Envoy is not Istio. In most case, Envoy seems an HTTP request, which consists of a URL plus a few headers. How does Envoy map a request to a service?
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.
service is mapped to "destination.service" attribute.
// Optional. A list of HTTP verbs (e.g., "GET", "POST"). | ||
// It is ignored in gRPC case because the value is always "POST". | ||
// If set to ["*"] or not specified, it applies to any verb. | ||
repeated string verbs = 3; |
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.
AttributeContext uses the term "method", not "verb".
envoy/config/rbac/v2alpha/rbac.proto
Outdated
// Definition of a custom constraint. The key in the constraint refers to an attribute defined in | ||
// AttributeContext (//envoy/service/auth/v2/attribute_context.proto). For example, | ||
// destination.labels["app"], request.http.headers["version"]. | ||
message Constraint { |
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.
RBAC should be modeled as "pattern matching on AttributeContext". The above comment seems to show the key refers to an attribute.
|
||
// Optional. Extra constraints in the ServiceRole specification. | ||
// The above ServiceRole examples shows an example of constraint "version". | ||
repeated Constraint constraints = 4; |
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.
We need to decide how to pattern matching the attributes in an efficient way. Otherwise, the performance can be terrible. IMO, Envoy needs to process 10k requests per second, so 100us per request. We must have a performance goal when designing an infrastructure.
Signed-off-by: Limin Wang <liminwang@google.com>
Signed-off-by: Limin Wang <liminwang@google.com>
Signed-off-by: Limin Wang <liminwang@google.com>
Signed-off-by: Limin Wang <liminwang@google.com>
Removed text that mentioned Istio and abstract constraints. Made RoleRef optional. A few other edits based on review comments. Signed-off-by: Limin Wang <liminwang@google.com>
And a few comments updates. Signed-off-by: Limin Wang <liminwang@google.com>
Signed-off-by: Limin Wang <liminwang@google.com>
Signed-off-by: Limin Wang <liminwang@google.com>
Signed-off-by: Limin Wang <liminwang@google.com>
Signed-off-by: Limin Wang <liminwang@google.com>
Signed-off-by: Limin Wang <liminwang@google.com>
Signed-off-by: Limin Wang <liminwang@google.com>
Signed-off-by: Limin Wang <liminwang@google.com>
Signed-off-by: Limin Wang <liminwang@google.com>
@wora Thanks for reviewing the change. Regarding your comments on AWS IAM. AWS IAM, as I understand, is for access control for AWS resources. RBAC we introduced here is to provide RPC (method) level access control for any custom services. They are designed for different goals. I added the "goals" and "backgrounds" part in the change description. And also added a README.md file. I hope it will help clarify a little. |
@mattklein123 @htuch @wora @lizan @rodaine I added a README.md, and made some minor changes to the proto. Could you please take a look to see if your comments are addressed? Thanks! |
@liminw I'm sorry but I think we are going to need to have a meeting. Can we set one up? If anyone out there wants to join can you speak up? I think the fundamental problem I'm having here is these protos are trying to do two things:
I think these two things need to be split, and I don't think (1) needs to live in data-plane-api. Essentially, from my perspective https://github.com/envoyproxy/data-plane-api/blob/master/envoy/api/v2/auth/auth.proto is all we need (plus some additions). We can use this to describe matching policies that lead to approve/deny decisions. The control plane can generate simple configurations in whatever way it wants. My vote is essentially to really focus on the split, and look at https://github.com/envoyproxy/data-plane-api/blob/master/envoy/api/v2/auth/auth.proto as a starting point for (2) above. How does it fall short for (2)? Do we need (1) in this repo at all? |
Please include me in the meeting invite @liminw. |
Please include me as well. |
@liminw given the repo reorg happening this weekend, and the fact that I think we agree this PR is not going to merge in this form, I'm going to go ahead and close. Please open a new Envoy side PR next week and reference this PR for prior discussion. Thank you! |
Added protos to support Role Based Access Control in Envoy. Ealier discussions at envoyproxy/data-plane-api#586. Signed-off-by: Limin Wang <liminwang@google.com>
Added protos to support Role Based Access Control in Envoy. Also removed existing auth.proto because the new RBAC proto is a replacement of it. Ealier discussions at envoyproxy/data-plane-api#586. Signed-off-by: Limin Wang <liminwang@google.com>
Added protos to support Role Based Access Control in Envoy. Also removed existing auth.proto because the new RBAC proto is a replacement of it. Ealier discussions at #586. Signed-off-by: Limin Wang <liminwang@google.com> Mirrored from https://github.com/envoyproxy/envoy @ 13de384ab34428af99c53201f6b3c95991b7ae10
Added protos to support Role Based Access Control in Envoy. Also removed existing auth.proto because the new RBAC proto is a replacement of it. Ealier discussions at envoyproxy/data-plane-api#586. Signed-off-by: Limin Wang <liminwang@google.com> Signed-off-by: Rama <rama.rao@salesforce.com>
Goals:
Proto files:
The proto here defines the full RBAC interface. Implementation will starts with basic support (without "constraints" and "custom properties") and grow into fully support RBAC.
Background reads: "Istio RBAC on Envoy" design doc (https://docs.google.com/document/d/1XkN9ls-ZDISz2uXGN20RaIre63DkJJOJNUOzcKCxbWA).