Apply RBAC to rendering of KVP during action execution

[m4dcoder]

RBAC for key-value pairs in the datastore was recently introduced. The RBAC policy needs to be applied to rendering of KVP during action and workflow execution.

[cognifloyd]

Looks sane based on reading the code.

[amanda11]

I wasn't sure if within a workflow if it uses a Jinja expression to execute a key/value pair should it check that the user has permission or not.
If workflow A calls actions B&C. Then if a user has permission to execute workflowA but not actions B&C, they can still run workflowA - they don't have to explicitly also be given permissions for individual actions.
So if workflowA accesses keyD, if a user has permission to execute workflowA, should they be able to run it - even if they don't have access to keyD.

If I thought the rules for key access were same as action access then I would expect them to have permission, even without explicit permission to keyD. This allows them to run a workflow that might need to say access sensitive information to perform its function, but we don't want them to actually be able to inquire and get the value of that sensitive info.

We would definitely want the RBAC check on the inputs to the workflow, but if the body of the workflow accessed them, then I'm not so sure.

I'm not sure which is best, so not added as a review comment that needs to be addressed. It maybe that the TSC thinks it's better to also need the key/value permission (even though for actions you don't need access to each individual action) - but we'll need to document whichever way we go.
For me, it would be more consistent with the current RBAC permissions with workflows if the RBAC change on key/value was only on the parameters to the workflow and actions, but not within the workflow itself.

[hnanchahal]

@armab Is there a plan to add this to a release. We have urgent need of this getting to master and test it out as its a big security flaw in the current setup.

[arm4b]

@hnanchahal Yes, it's scheduled as part of the upcoming v3.8 release.

[amanda11]

LGTM

[arm4b]

Here is the conclusion based on internal @StackStorm/tsc conversations about whether granting RBAC access to the workflow should give access to subsequent K/V lookups in that workflow as we do for actions:

Looking at the docs it is consistent that Workflow run grants permissions to dependent actions. Here, workflow is considered an action:

Keep in mind that in StackStorm a workflow is just an action so if you want someone to be able to execute a particular workflow, you simply need to grant them action_execute permission on that workflow.

Then, because of the execution inheritance, workflow run inherits subsequent action runs:
https://docs.stackstorm.com/rbac.html?highlight=rbac#permission-inheritance

Permission Inheritance
Executions
Executions inherit permissions from the action they belong to and from the action’s parent pack. This means that if you grant action_view permission on a particular action, the user will be able to view all the executions which belong to that action. Similarly, if you grant action_view to the parent pack which the action execution belongs to, the user will be able to view all the executions which belong to the action with that parent pack.
On top of that, granting action_execute on a particular pack or action also grants execution_rerun and execution_stop to all the executions which belong to that action.

Next, for the K/V pairs RBAC: https://docs.stackstorm.com/rbac.html?highlight=rbac#key-value-pairs we have this documentation which makes explicit that K/V permissions will need an explicit RBAC grant:

By default, a user has access to his/her own user scoped KVPs without requiring specific permission grants. A non-admin user by default cannot access system scoped KVPs or other users’ KVPs. A non-admin user can be explicitly granted permission to one or more system scoped KVPs similar to how access to other resources are granted to users. Currently, there is no option or plan to grant non-admin user access to another user’s set of KVPs.

This however has some usability issues highlighted by @amanda11 when granting RBAC access to the workflow run,
StackStorm administrator needs to also grant access to all the K/V pairs manually used by this workflow and actions.

[amanda11]

@armab In the upgrade notes section we should probably add something about granting key/value permission to users that can run workflows that use key/values. Similar to last bullet we put in 3.7 in regard to change needed for sensors: https://docs.stackstorm.com/upgrade_notes.html#st2-v3-7.

[arm4b]

+1 adding some hints to the upgrade notes

[hnanchahal]

@armab @m4dcoder Have a question on this RBAC:

Does the config support tagging a role to multiple secrets?

Doc says:

name: key1_write_role
description: Role that allow users to set system key1
enabled: true
permission_grants:
- resource_uid: "key_value_pair:st2kv.system:key1"
permission_types:
- "key_value_pair_set"

is something like this supported?

--
name: key1_write_role
description: Role that allow users to set system key1
enabled: true
permission_grants:
- resource_uid: "key_value_pair:st2kv.system:key1, key_value_pair:st2kv.system:key1"
permission_types:
- "key_value_pair_set"