Update RBAC for Authorization v2 API. by yangminzhu · Pull Request #748 · istio/api

yangminzhu · 2019-01-04T22:57:18Z

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

rbac/v1alpha1/istio.rbac.v1alpha1.pb.html

rshriram

could we align on the workload selectors?
Also, the widespread use of use and notUse is going to make the API and the spec a bit clumsy.
I suggest something like this:

message xxx
  hosts
  principals
  groups
  addresses
allow:
 oneOf
   xxx
   all
disallow:
  oneOf
    xxx
    all

so that the config would look like

rbac:
 subject:
   allow:
     all
    disallow:
      principals: foo/bar/com
  
role:
  allow:
    hosts:
     - ns1/*
     - istio-system/*
   disallow:
     all

This is just a rough example, but you get the idea.

rbac/v1alpha1/rbac.proto

rshriram · 2019-01-08T16:05:23Z

rbac/v1alpha1/rbac.proto

+  // Deprecated. Use selector in authorization policy instead.
+  repeated string services = 1 [deprecated = true];
+
+  // Optional. A list of HTTP hosts. If not specified, it applies to any host.


why HTTP ? Just state that its a list of hosts corresponding to services in the registry.
Also, do you allow wildcard hosts like Pilot does? (*.foo.com)?

Secondly, do you think its useful to adopt the same convention as Pilot's sidecar format which has configNamespace/Hosts ? This allows people to import stuff in bulk, such as ns1/*, ns2/foo.com, etc. Note that the config namespace has nothing to do with k8s namespace.

By HTTP we're trying to make it clear it's the HOST header in a HTTP request. The deprecated services field refer to services in registry but we're deprecating it in favor of the workload label selectors. cc @liminw

Under this condition (it's the HOST header in a HTTP request) we could support the wildcard hosts easily.

Secondly, for now I think we're not going to support the configNamespace/Hosts syntax but would revisit this later.

rshriram · 2019-01-08T16:05:43Z

rbac/v1alpha1/rbac.proto

+  repeated string hosts = 5;
+
+  // Optional. A list of HTTP hosts that must not be matched.
+  repeated string not_hosts = 6;


Do you need this not? isn't it sufficient to specify hosts alone?

hmm I can see some use for this. But you should change the comments to state that you can either have hosts or notHosts, but not both.

We're not using one_of for this (and other fields) because we allow to use them at then same time. Like the following one (Allows all sub domains of abc.com except admin.abc.com):

hosts: ["*.abc.com"] notHosts: ["admin.abc.com"]

rbac/v1alpha1/rbac.proto

rshriram · 2019-01-08T16:07:02Z

rbac/v1alpha1/rbac.proto

+  repeated int32 ports = 9;
+
+  // Optional.  A list of port numbers that must not be matched.
+  repeated int32 not_ports = 10;


one of semantics must be specified in the comment.

Same reply with the paths field.

rbac/v1alpha1/rbac.proto

rshriram · 2019-01-08T16:09:28Z

rbac/v1alpha1/rbac.proto

+  repeated string namespaces = 8;
+
+  // Optional. A list of namespaces that must not be matched.
+  repeated string not_namespaces = 9;


You dont need namespace here because the hosts section would specify it (ns1/*)

We're treating the host field as the HOST field in a HTTP request, so we still need the namespace here. Also note the namespace field is in the Subject message which is for the identity while the host field is in the AccessRule message which is for the permission.

yangminzhu · 2019-01-10T00:31:29Z

@rshriram

Also, the widespread use of use and notUse is going to make the API and the spec a bit clumsy.

It may looks a bit clumsy in the API, but in reality, I think it's fine as most people won't use all these fields at the same time. cc @liminw

For example, the following yaml actually doesn't look too bad.

rules:
- notMethods: ["POST"]
  paths: ["/info"]
- methods: ["GET"]
  constraints:
  - key: "destination.labels[version]"
    value: ["test"]

For more background about the design, also see the design doc

yangminzhu · 2019-01-10T20:13:03Z

@rshriram @liminw
I rebased this PR to 1.1 branch in order to refer to the WorkloadSelector which is only available in 1.1 branch. We probably don't want to merge this to 1.1 as it's not implemented in 1.1. Instead it should go to master once it's unblocked.

rshriram

Thanks for making the changes. one small nit: rename selector to workloadSelector for consistency with other api.
Also, 1.1 is merged into master IIRC. So you should have workload selector even there as well.

yangminzhu · 2019-01-10T22:13:13Z

@rshriram
I updated the PR to include the v2 changes but hide them from the documentation. I will not do the deprecation in this PR so that we can merge this into the 1.1 branch.

liminw · 2019-01-11T00:39:21Z

rbac/v1alpha1/rbac.proto

+  // select all pods/VMs.
+  // The scope of label search is platform dependent. On Kubernetes, for example,
+  // the scope includes pods running in the same namespace as the authorization policy itself.
+  istio.networking.v1alpha3.WorkloadSelector workload_selector = 1;


Can we duplicate WorkloadSelector definition here to avoid creating dependency on networking API? IMO, if workload selector is a common concept used by all Istio APIs, it should eventually be moved to a common package so that all APIs can import.

Yes, I agree with you. @diemtvu also mentioned the same thing. Updated to fork the the WorkloadSelector

cc @rshriram

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

liminw

/lgtm

istio-testing · 2019-01-11T01:57:07Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: liminw, yangminzhu
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: kyessenov

If they are not already assigned, you can assign the PR to them by writing /assign @kyessenov in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

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

yangminzhu requested a review from liminw January 4, 2019 22:57

istio-testing requested review from frankbu and sebastienvas January 4, 2019 22:57

googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Jan 4, 2019

yangminzhu removed request for frankbu and sebastienvas January 4, 2019 22:57

yangminzhu force-pushed the authz_api_v2 branch from bae8570 to 1876f82 Compare January 4, 2019 23:02

liminw reviewed Jan 8, 2019

View reviewed changes

rshriram suggested changes Jan 8, 2019

View reviewed changes

yangminzhu force-pushed the authz_api_v2 branch from 1876f82 to 76fd48a Compare January 10, 2019 00:37

yangminzhu changed the base branch from master to release-1.1 January 10, 2019 00:37

rshriram approved these changes Jan 10, 2019

View reviewed changes

yangminzhu added the do-not-merge/hold Block automatic merging of a PR. label Jan 10, 2019

yangminzhu force-pushed the authz_api_v2 branch 2 times, most recently from f50076d to 20c8680 Compare January 10, 2019 22:10

yangminzhu removed the do-not-merge/hold Block automatic merging of a PR. label Jan 10, 2019

liminw reviewed Jan 11, 2019

View reviewed changes

Update RBAC for Authorization v2 API.

27c03b4

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

yangminzhu force-pushed the authz_api_v2 branch from 20c8680 to 27c03b4 Compare January 11, 2019 01:16

liminw approved these changes Jan 11, 2019

View reviewed changes

istio-testing assigned liminw Jan 11, 2019

istio-testing added the lgtm label Jan 11, 2019

rshriram merged commit 48ad354 into istio:release-1.1 Jan 11, 2019

yangminzhu deleted the authz_api_v2 branch January 11, 2019 18:56

yangminzhu added a commit to yangminzhu/api that referenced this pull request Jan 15, 2019

Update RBAC for Authorization v2 API. (istio#748)

c3cd40b

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

Conversation

yangminzhu commented Jan 4, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rshriram left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yangminzhu commented Jan 10, 2019

Uh oh!

yangminzhu commented Jan 10, 2019

Uh oh!

rshriram left a comment

Choose a reason for hiding this comment

Uh oh!

yangminzhu commented Jan 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liminw left a comment

Choose a reason for hiding this comment

Uh oh!

istio-testing commented Jan 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants