do not exclude rbac filters from gateways #11894
Conversation
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rshriram The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@rshriram: The following tests failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
| return nil | ||
| } | ||
|
|
||
| return setupFilters(in, mutable) |
There was a problem hiding this comment.
Will this break any existing usage?
Let's say we have service foo and bar. Assume we only have rbac policy that allows GET access to foo for identity bar.
Before this PR, the rbac filter is generated only in the inbound listener of service foo. foo is still able to access other targets as there is no rbac filter on its outbound listener list.
After this PR, the rbac filter is generated in both inbound listener list and outbound listener list of foo. This has an side effect that foo is unable to access anyone in the mesh as its identity is always foo, not bar.
When generating the outbound listener, can we check the node.type and only generate if it's gateway? So that we exclude the sidecar in this case.
Also we need to test this in real deployment and add e2e tests. We can add e2e tests later but at least a manual test should be done before merging.
cc @liminw
There was a problem hiding this comment.
Also RBAC on outbound is very tricky - it doesn't really provide any security, but may impact performance.
There was a problem hiding this comment.
But since it's restricted to gateway - I think it is ok.
There was a problem hiding this comment.
I think this do provide more security when the egress gateway is the only entity that is allowed to talk to the external in the mesh, like it's enforced by underlying firewall rule. In this case, using RBAC could provide more control on the egress traffic. For example: https://preliminary.istio.io/docs/examples/advanced-gateways/egress-gateway/
Also the RBAC is not enabled by default so if the user doesn't enable it, there is no performance impact.
There was a problem hiding this comment.
So the in.Node.Type != model.Router will make sure it only apply to gateway right? If so I think we just need to test it. It shouldn't break any existing use case.
|
This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
/cc @quanjielin |
|
@rshriram I have done the manual test for this, feel free to merge to master. Or you can merge this one #12415 which includes the e2e test together. Thanks! I prefer to do this in master as we still need to update the documents and probably add a new task, and adding this new feature in the last minute is still too risky for 1.1. |
|
/cc @pitlv2109 |
This is essential in multicluster with gateway scenario where we could
apply RBAC at the ingress gateway to determine whether traffic for a
particular global service is allowed or not.
Signed-off-by: Shriram Rajagopalan shriramr@vmware.com