Authz template sub messages#1877
Conversation
Update latest changes from mater
|
/assign @mandarjog @guptasu |
Codecov Report
@@ Coverage Diff @@
## master #1877 +/- ##
=========================================
+ Coverage 81.23% 81.84% +0.6%
=========================================
Files 191 195 +4
Lines 19344 15949 -3395
=========================================
- Hits 15715 13053 -2662
+ Misses 3173 2430 -743
- Partials 456 466 +10
Continue to review full report at Codecov.
|
guptasu
left a comment
There was a problem hiding this comment.
Hi @mangchiandjjoe , has this template gone through a formal design review. Basically every new template needs to have a 1 pager that is shared by the istio-integration group and reviewed widely. Here is an example:
https://docs.google.com/document/d/1jRiRLdyufYDcBvR5wjIBmPLUTzrtjnBqnTd4P9_JCGQ/edit?ts=59fa07ee
mixer/template/authz/template.proto
Outdated
| map<string, istio.mixer.v1.config.descriptor.ValueType> extra = 5; | ||
| } | ||
|
|
||
| message Template { |
There was a problem hiding this comment.
Consider adding more description and example config. sample : https://github.com/istio/istio/blob/master/mixer/template/listentry/template.proto#L23
mixer/template/authz/template.proto
Outdated
| // The user. | ||
| string user = 1; | ||
| // Groups the subject belongs to. | ||
| repeated string groups = 2; |
There was a problem hiding this comment.
repeated. This is the first template we have used repeated. repeated fields are not completely supported yet. I can make it work in 1/2 day (just need to add its procession in the bootstraping code (loading yaml and generating instance objects), the code for generating artifacts to be used by the adapter is already present).
This should not block your adapter implementation thought. But unfortunately without change from me, you won't be able to test your adapter e2e with Mixer.
There was a problem hiding this comment.
That should be fine. We can keep this field repeated for now and write a e2e test after the implementation.
|
I created a design document for the authz template https://goo.gl/1y3Z1v |
|
@guptasu Sunny, the template was originally from Istio RBAC design doc. It has been reviewed and approved. There is one thing Jae noticed is that, "name" under "subject" is not allowed. The error message says that "name" is a reserved field. Do you think it is a bug? "name" is allowed to be a field name in messages. |
|
It is intentional. We have reserved the name field for instance name value.
…On Nov 28, 2017 11:55 AM, "Limin Wang" ***@***.***> wrote:
@guptasu <https://github.com/guptasu> Sunny, the template was originally
from Istio RBAC design doc
<https://docs.google.com/document/d/1dKXUEOxrj4TWZKrW7fx_A-nrOdVD4tYolpjgT8DYBTY/edit#heading=h.uy42ya2lsfue>.
It has been reviewed and approved.
There is one thing Jae noticed is that, "name" under "subject" is not
allowed. The error message says that "name" is a reserved field. Do you
think it is a bug? "name" is allowed to be a field name in messages.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1877 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AK6xEwqG_digpYu4BTPu7FmRzxuWtqR5ks5s7GUWgaJpZM4QseUU>
.
|
|
/retest |
guptasu
left a comment
There was a problem hiding this comment.
Overall looks good. Once your one page design is approved and the field names are approved, you can submit this PR. I am not super familiar on what should be the content of this template.
| map<string, istio.mixer.v1.config.descriptor.ValueType> extra = 3; | ||
| } | ||
|
|
||
| message Action { |
|
|
||
| option (istio.mixer.v1.template.template_variety) = TEMPLATE_VARIETY_CHECK; | ||
|
|
||
| message Subject { |
| import "mixer/v1/template/extensions.proto"; | ||
|
|
||
| option (istio.mixer.v1.template.template_variety) = TEMPLATE_VARIETY_CHECK; | ||
|
|
There was a problem hiding this comment.
The template looks good from the structure point of view. For the field names and comments, I will let others (@liminw and other auth experts) to comment on how intuitive they are.
There was a problem hiding this comment.
Field names and comments are from the Istio Native RBAC Design V2 https://goo.gl/kof8o2 which was approved
|
/assign @geeknoid |
mixer/template/authz/template.proto
Outdated
| message Subject { | ||
| // The user. | ||
| string user = 1; | ||
| // Groups the subject belongs to. |
There was a problem hiding this comment.
What are the valid groups, where are they defined? Can you clarify in the comments?
There was a problem hiding this comment.
Depending on the authentication mechanism, "groups" are normally populated from JWT claim or client certificate. The operator can define how it is populated when creating an instance of the template.
There was a problem hiding this comment.
Updated comment
mixer/template/authz/template.proto
Outdated
|
|
||
| syntax = "proto3"; | ||
|
|
||
| package authz; |
There was a problem hiding this comment.
Could we replace all uses of authz with authorization?
mixer/template/authz/template.proto
Outdated
| map<string, istio.mixer.v1.config.descriptor.ValueType> extra = 3; | ||
| } | ||
|
|
||
| // List of actions |
There was a problem hiding this comment.
This is not a list, it's just one single action.
Could you provide a comment explaining what this is for?
There was a problem hiding this comment.
Action defines "how the resource is accessed".
mixer/template/authz/template.proto
Outdated
|
|
||
| option (istio.mixer.v1.template.template_variety) = TEMPLATE_VARIETY_CHECK; | ||
|
|
||
| // List of subjects |
There was a problem hiding this comment.
This is not a list, it's a single subject.
Could you provide a comment explaining what this is for?
There was a problem hiding this comment.
"Subject" contains a list of attributes that identify the caller identity.
| // Namespace the target action is taking place in. | ||
| string namespace = 1; | ||
| // The Service the action is being taken on. | ||
| string service = 2; |
There was a problem hiding this comment.
Is this the right way to reference a service?
mixer/template/authz/template.proto
Outdated
| } | ||
|
|
||
| // Authz template defines subject and action. Adapters using authz template | ||
| // should validate “subject” allowed to do an “action”. |
There was a problem hiding this comment.
should validate that the "subject" is allowed to do the given "action"
There was a problem hiding this comment.
is allowed to perform the specified action ?
mixer/template/authz/template.proto
Outdated
| // Authz template defines subject and action. Adapters using authz template | ||
| // should validate “subject” allowed to do an “action”. | ||
| // | ||
| // When writing the configuration, the value for the fields associated with this template can either be a |
There was a problem hiding this comment.
Let's remove this paragraph, that's how templates work in general and we don't need to explain that here.
mixer/template/authz/template.proto
Outdated
| // metadata: | ||
| // name: authinfo | ||
| // namespace: istio-system | ||
| // Spec: |
| // groups: request.auth.token[groups] | ||
| // extra: | ||
| // iss: request.auth.token["iss"] | ||
| // action: |
There was a problem hiding this comment.
I worry about reusing the word action here, since that has a generic meaning in Mixer. Maybe "demand"? A subject makes a demand?
| // version: destination.labels[version] | "" | ||
| // ``` | ||
| message Template { | ||
| Subject subject = 1; |
There was a problem hiding this comment.
Comments on the fields please.
douglas-reid
left a comment
There was a problem hiding this comment.
There are generated files in this PR, but the generatedfiles file is not updated. I think that is problematic.
mixer/template/authz/template.proto
Outdated
| // Groups the subject belongs to. | ||
| repeated string groups = 2; | ||
| // Additional attributes about the subject. | ||
| map<string, istio.mixer.v1.config.descriptor.ValueType> extra = 3; |
There was a problem hiding this comment.
extra is not a great name, imho. can we use something like attributes or even something like metadata instead? It might help to have a better comment on Subject that explains what a subject is (that doesn't use the word subject in the description) and what extra you might need/want for a subject.
There was a problem hiding this comment.
It was previously named "attributes". It was renamed because Martin thought that "attributes" conflicts with Istio terms of "attributes". It is not "metadata" either. This field represents the custom attributes (in addition to "user" and "groups") that can be used to identify the subject. It is hard to come up with a good name. Do you have other suggestions for the name?
| // Namespace the target action is taking place in. | ||
| string namespace = 1; | ||
| // The Service the action is being taken on. | ||
| string service = 2; |
There was a problem hiding this comment.
is this the destination.service ?
mixer/template/authz/template.proto
Outdated
| // The resource within the service. | ||
| string path = 4; | ||
| // Additional data about the action for use in policy. | ||
| map<string, istio.mixer.v1.config.descriptor.ValueType> extra = 5; |
There was a problem hiding this comment.
again, i think we want a more descriptive name than extra. Is this stuff metadata about the action?
mixer/template/authz/template.proto
Outdated
| } | ||
|
|
||
| // Authz template defines subject and action. Adapters using authz template | ||
| // should validate “subject” allowed to do an “action”. |
There was a problem hiding this comment.
is allowed to perform the specified action ?
mixer/template/authz/template.proto
Outdated
| map<string, istio.mixer.v1.config.descriptor.ValueType> extra = 5; | ||
| } | ||
|
|
||
| // Authz template defines subject and action. Adapters using authz template |
There was a problem hiding this comment.
Can we add more detail about the purpose here? This will end up in docs on the website, and while authorization is reasonably clear to sophisticated users, I think we could benefit from some lingo clarity. Maybe something like:
The authorization template defines parameters for performing policy enforcement within Istio. It is primarily concerned with enabling Mixer adapters to make decisions about who is allowed to do what. In this template, the who is defined in a Subject message. The what is defined in the Action message. During a Mixer Check call, these values will be populated based on configuration from request attributes and passed to individual authorization adapters to adjudicate.
You can probably describe it better, but I think that would be helpful (and maybe eliminate the need for the following paragraph).
| // service: target.service | "" | ||
| // path: request.path | "/" | ||
| // method: request.method | "post" | ||
| // extra: |
| // subject: | ||
| // user: source.user | request.auth.token[user] | "" | ||
| // groups: request.auth.token[groups] | ||
| // extra: |
| // What action is being taken. | ||
| string method = 3; | ||
| // The resource within the service. | ||
| string path = 4; |
There was a problem hiding this comment.
Should this be called resource? or resource_path?
There was a problem hiding this comment.
It is the HTTP REST path within the service. Updated comment.
|
/retest all |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: geeknoid The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. |
What this PR does / why we need it:
We need a authorization template for mixer adapters
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #Special notes for your reviewer:
authz template has sub messages
Release note: