feat(rbac): patterns on rbac policy objects #2171
Conversation
rgolangh
requested review from
a team,
AndrienkoAleksandr and
PatAKnight
as code owners
rgolangh
changed the title
|
need to figure out why it does not work with the tempEnforcer |
rgolangh
force-pushed
the
orchestrator-fine-grained-rbac
branch
4 times, most recently
from
ef8056c to
ff97ceb
Compare
|
@PatAKnight |
| @@ -352,7 +353,10 @@ function setupInternalRoutes( | |||
|
|
|||
| const decision = await authorize( | |||
| _req, | |||
| orchestratorWorkflowReadPermission, | |||
| createPermission({ | |||
There was a problem hiding this comment.
how would it work if the user put the global workflow viewing permission but not fine grained ones?
There was a problem hiding this comment.
what exactly is the global one?
There was a problem hiding this comment.
a 'global' can be set by the policy author, because now the permission object is matched by a regexp match
so if the policy says 'orchestrator.workflow' then every string that starts with 'orchestrator.workflow' is matched, unless there is a deny rule that matches.
There was a problem hiding this comment.
we would need to remove the current common permissions, they will not be in use and only leave the ones who are not workflow specific
There was a problem hiding this comment.
we would need to remove the current common permissions, they will not be in use and only leave the ones who are not workflow specific
WDYM?
There was a problem hiding this comment.
I added a test file to try to capture the global vs fine grained. there is a test there to let the workflowUser role execute all workflows except the one starting with 'ansible' .
orchestrator.workflows , use, allow // allow executing all workflows
orchestrator.workflows.ansible , use, deny // deny executing workflow starting with ansible. overrides the policy above
| @@ -1,11 +1,15 @@ | |||
| p, role:default/workflowViewer, orchestrator.workflowInstances.read, read, allow | |||
There was a problem hiding this comment.
I'm trying to understand how the policies will be used:
I think the most simple use case is that admins can view and run all workflows
And a group, let's say "developers-a" the admins will want to restrict to use workflow a
And another group, let's say "developers-b" the admins will want to restrict to use workflow b
How will this be done?
There was a problem hiding this comment.
in this case the policy should have the exact name of the workflow, so there will be no match for any other(*):
p, role:default/groupA, orchestrator.workflows.workflow-a, read, allow
p, role:default/groupB, orchestrator.workflows.workflow-b, read, allow
The regexp match will not match other workflows names againt this policy, hence it will be denied
* One thing to look out for is that since we're using a regexp match on the name of the workflow then a workflow with the name 'workflow-abc' will also be allowed.
just to make sure there is no confusion, it is not a role per workflow. it is a policy that can match against multiple objects. The object identifier in the orchestrator case is the a string of the form |
rgolangh
force-pushed
the
orchestrator-fine-grained-rbac
branch
from
ff97ceb to
97f2f19
Compare
| // this commented filter is not designed to work with resourceType which is dynamically constructed | ||
| // using the resouece id. The switch to mathing policies on regexMatch on the object (here is is resourceType under v1 key) means we can't load policies by resourceType because the query to the repository will not find it. e.g query where orchestrator.workflows.abc where the policy has the object orchestrator.workflows will return empty | ||
| // We can expand the enforce method to conditionally use the former filter in case a policy for a catalog entity resource is used where the object name represents the resouce type | ||
| filter.push({ ptype: 'p', v0: role, v2: action }); |
There was a problem hiding this comment.
@PatAKnight this is the change in the filter I had to make in order for the policies to be loaded.
rgolangh
force-pushed
the
orchestrator-fine-grained-rbac
branch
from
97f2f19 to
10a59e8
Compare
rgolangh
changed the title
Motivation:
There is no way to capture multiple objects in a single policy today and
match against it. The outcome is that policies for basic permission,
where the objects are not catalog entities, are not fine grained. Either
you have permission on an action on all the category, or not at all.
For example the workflows that the orchestrator plugin exposes are not
a catalog entity. They only have a workflow ID the plugin exposes and
there is no way today to deny or allow some of the workflows usage.
I.e, this is the permission a workflow have for read:
orchestrator.workflows.read, read, allow
Modification:
The only modification for the rbac plugin is to use a regexp match:
[matchers]
m = g(r.sub, p.sub) && regexMatch(r.obj, p.obj) && r.act == p.act
Result:
With this change we have the permission be matched by a regexp:
orchestrator.workflows , read, allow // catches all workflow
orchestrator.workflows.banned ,read, deny // catches 'banned' workflow id
Signed-off-by: Roy Golan <[email protected]>
rgolangh
force-pushed
the
orchestrator-fine-grained-rbac
branch
from
3fc22f5 to
d7a12c8
Compare
|
|
@AndrienkoAleksandr @PatAKnight please review |
There was a problem hiding this comment.
Sorry I am just now getting to this.
I left a couple of comments in the PR but also wanted to include some thoughts as well.
I realize now that I may have misunderstood what you were trying to accomplish with using pattern matching on permission policies. I was under the impression that you were actually trying to reduce the need to define many permission policies that all have a common resource type. An example in my head is something like the following:
# Assume role:default/example is already defined
# catalog-entity resource type has three different permissions associated with
# it so defining permission policies would look something like this
p, role:default/example, catalog-entity, read, allow
p, role:default/example, catalog-entity, delete, allow
p, role:default/example, catalog-entity, update, allow
# With the policy matching that I was thinking about it would instead match based
# on the resource type with the wildcard on the action itself
# thus granting read, update, and delete
p, role:default/example, catalog-entity, *, allow
I am not certain at the moment if this is possible though.
| // this commented filter is not designed to work with resourceType which is dynamically constructed | ||
| // using the resouece id. The switch to mathing policies on regexMatch on the object (here is is resourceType under v1 key) means we can't load policies by resourceType because the query to the repository will not find it. e.g query where orchestrator.workflows.abc where the policy has the object orchestrator.workflows will return empty | ||
| // We can expand the enforce method to conditionally use the former filter in case a policy for a catalog entity resource is used where the object name represents the resouce type | ||
| filter.push({ ptype: 'p', v0: role, v2: action }); |
There was a problem hiding this comment.
This change will lead to a negative impact on the performance of the RBAC backend plugin. It is hard to say how much of an impact, but reducing one of the filtering options will result in more matches that will have to be evaluated.
|
|
||
| export const orchestratorWorkflowInstanceAbortPermission = createPermission({ | ||
| name: 'orchestrator.workflowInstance.abort', | ||
| name: 'orchestrator.workflows', |
There was a problem hiding this comment.
As mentioned by @batzionb, couldn't conditional rules work instead? I don't have much experience defining them. But they could work for what you are trying to achieve.
From my basic understanding, you wound need to define a resource type, in this case something like orchestrator-workflows .Next, you would need to define your conditional rules, something like hasWorkflowID. And finally you would want to update your router to use the conditional rules.
I have not tried them out personally but I can supply some resources and a few plugins that currently use them at the moment.
Adding a resource permission check, this includes information on support for conditional decisions
wildcarding the action is useful on its own for composing roles. But yes, this is not what I was using it for.
The main difference in the orchestrator permission usage is that we don't have a resourceType, but instead we use basic or named permission since we don't have our object in the catalog. Correct me if I'm wrong, conditions could not be declared on the policy file itself right? you have to use the rbac ui to add those? |
we introduced separated external file for conditional policies for RHDH 1.3: https://github.com/janus-idp/backstage-plugins/blob/ef9ee685be72508eb6c5b174da2565076515cb56/plugins/rbac-backend/README.md#configuring-conditional-policies-via-file |
|
given the feedback I'm closing this PR for now. The future work might use conditions instead or just reopen and continue with this PR. I leave this to @batzionb |
|
cc @mareklibra |
Motivation:
There is no way to capture multiple objects in a single policy today and
match against it. The outcome is that policies for basic permission,
where the objects are not catalog entities, are not fine grained. Either
you have permission on an action on all the category, or not at all.
For example the workflows that the orchestrator plugin exposes are not
a catalog entities. They only have a workflow ID the plugin exposes and
there is no way today to deny or allow some of the workflows usage.
I.e, this is the permission a workflow have today for read:
Modification:
The only modification for the rbac plugin is to use a regexp match:
Result:
With this change we have the permission be matched by a regexp:
Resolves: #2025