Merge pull request #5764 from StackStorm/action-execution-kvp-rbac

m4dcoder · web-flow · commit 9abb54b9d2e5 · 2022-10-24T09:54:23.000-07:00
Apply RBAC to rendering of KVP during action execution
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
@@ -39,6 +39,10 @@ Fixed
 * Fixed generation of `st2.conf.sample` to show correct syntax for `[sensorcontainer].partition_provider` (space separated `key:value` pairs). #5710
   Contributed by @cognifloyd
 
+* Fix access to key-value pairs in workflow and action execution where RBAC rules did not get applied
+
+  Contributed by @m4dcoder
+
 Added
 ~~~~~
 
diff --git a/st2common/st2common/services/keyvalues.py b/st2common/st2common/services/keyvalues.py
@@ -15,21 +15,26 @@
 
 from __future__ import absolute_import
 
+from oslo_config import cfg
+
 from st2common import log as logging
 
 from st2common.constants.keyvalue import DATASTORE_PARENT_SCOPE
 from st2common.constants.keyvalue import SYSTEM_SCOPE, FULL_SYSTEM_SCOPE
 from st2common.constants.keyvalue import USER_SCOPE, FULL_USER_SCOPE
 from st2common.constants.keyvalue import ALLOWED_SCOPES
 from st2common.constants.keyvalue import DATASTORE_KEY_SEPARATOR, USER_SEPARATOR
+from st2common.constants.types import ResourceType
 from st2common.exceptions.db import StackStormDBObjectNotFoundError
 from st2common.exceptions.keyvalue import InvalidScopeException, InvalidUserException
+from st2common.models.db.auth import UserDB
 from st2common.models.system.keyvalue import UserKeyReference
 from st2common.persistence.keyvalue import KeyValuePair
 from st2common.persistence.rbac import UserRoleAssignment
 from st2common.persistence.rbac import Role
 from st2common.persistence.rbac import PermissionGrant
-from st2common.constants.types import ResourceType
+from st2common.rbac.backends import get_rbac_backend
+from st2common.rbac.types import PermissionType
 
 __all__ = [
     "get_kvp_for_name",
@@ -103,7 +108,12 @@ class KeyValueLookup(BaseKeyValueLookup):
     scope = SYSTEM_SCOPE
 
     def __init__(
-        self, prefix=None, key_prefix=None, cache=None, scope=FULL_SYSTEM_SCOPE
+        self,
+        prefix=None,
+        key_prefix=None,
+        cache=None,
+        scope=FULL_SYSTEM_SCOPE,
+        context=None,
     ):
         if not scope:
             scope = FULL_SYSTEM_SCOPE
@@ -116,6 +126,18 @@ def __init__(
         self._value_cache = cache or {}
         self._scope = scope
 
+        self._context = context if context else dict()
+        self._user = (
+            context["user"]
+            if context and "user" in context and context["user"]
+            else cfg.CONF.system_user.user
+        )
+        self._user = (
+            context["api_user"]
+            if context and "api_user" in context and context["api_user"]
+            else self._user
+        )
+
     def __str__(self):
         return self._value_cache[self._key_prefix]
 
@@ -154,6 +176,7 @@ def _get(self, name):
             key_prefix=key,
             cache=self._value_cache,
             scope=self._scope,
+            context=self._context,
         )
 
     def _get_kv(self, key):
@@ -167,6 +190,19 @@ def _get_kv(self, key):
 
         if kvp:
             LOG.debug("Got value %s from datastore.", kvp.value)
+
+        # Check that user has permission to the key value pair.
+        # If RBAC is enabled, this check will verify if user has system role with all access.
+        # If RBAC is enabled, this check guards against a user accessing another user's kvp.
+        # If RBAC is enabled, user needs to be explicitly granted permission to view a system kvp.
+        # The check is sufficient to allow decryption of the system kvp.
+        rbac_utils = get_rbac_backend().get_utils_class()
+        rbac_utils.assert_user_has_resource_db_permission(
+            user_db=UserDB(name=self._user),
+            resource_db=kvp,
+            permission_type=PermissionType.KEY_VALUE_PAIR_VIEW,
+        )
+
         return kvp.value if kvp else ""
 
 
@@ -175,7 +211,13 @@ class UserKeyValueLookup(BaseKeyValueLookup):
     scope = USER_SCOPE
 
     def __init__(
-        self, user, prefix=None, key_prefix=None, cache=None, scope=FULL_USER_SCOPE
+        self,
+        user,
+        prefix=None,
+        key_prefix=None,
+        cache=None,
+        scope=FULL_USER_SCOPE,
+        context=None,
     ):
         if not scope:
             scope = FULL_USER_SCOPE
@@ -188,6 +230,7 @@ def __init__(
         self._value_cache = cache or {}
         self._user = user
         self._scope = scope
+        self._context = context if context else dict()
 
     def __str__(self):
         return self._value_cache[self._key_prefix]
diff --git a/st2common/st2common/util/keyvalue.py b/st2common/st2common/util/keyvalue.py
@@ -21,17 +21,14 @@
 
 from st2common.constants.keyvalue import ALL_SCOPE, DATASTORE_PARENT_SCOPE
 from st2common.constants.keyvalue import DATASTORE_SCOPE_SEPARATOR
-from st2common.rbac.backends import get_rbac_backend
+from st2common.constants.keyvalue import FULL_SYSTEM_SCOPE, FULL_USER_SCOPE
+from st2common.constants.keyvalue import USER_SCOPE, ALLOWED_SCOPES
+from st2common.exceptions.rbac import AccessDeniedError
+from st2common.models.db.auth import UserDB
 from st2common.persistence.keyvalue import KeyValuePair
 from st2common.services.config import deserialize_key_value
-from st2common.constants.keyvalue import (
-    FULL_SYSTEM_SCOPE,
-    FULL_USER_SCOPE,
-    USER_SCOPE,
-    ALLOWED_SCOPES,
-)
-from st2common.models.db.auth import UserDB
-from st2common.exceptions.rbac import AccessDeniedError
+from st2common.rbac.backends import get_rbac_backend
+from st2common.rbac.types import PermissionType
 
 __all__ = ["get_datastore_full_scope", "get_key"]
 
@@ -106,17 +103,21 @@ def get_key(key=None, user_db=None, scope=None, decrypt=False):
 
     _validate_scope(scope=scope)
 
-    rbac_utils = get_rbac_backend().get_utils_class()
-    is_admin = rbac_utils.user_is_admin(user_db=user_db)
-
-    # User needs to be either admin or requesting item for itself
-    _validate_decrypt_query_parameter(
-        decrypt=decrypt, scope=scope, is_admin=is_admin, user_db=user_db
-    )
-
     # Get the key value pair by scope and name.
     kvp = KeyValuePair.get_by_scope_and_name(scope, key_id)
 
+    # Check that user has permission to the key value pair.
+    # If RBAC is enabled, this check will verify if user has system role with all access.
+    # If RBAC is enabled, this check guards against a user accessing another user's kvp.
+    # If RBAC is enabled, user needs to be explicitly granted permission to view a system kvp.
+    # The check is sufficient to allow decryption of the system kvp.
+    rbac_utils = get_rbac_backend().get_utils_class()
+    rbac_utils.assert_user_has_resource_db_permission(
+        user_db=user_db,
+        resource_db=kvp,
+        permission_type=PermissionType.KEY_VALUE_PAIR_VIEW,
+    )
+
     # Decrypt in deserialize_key_value cannot handle NoneType.
     if kvp.value is None:
         return kvp.value
diff --git a/st2common/st2common/util/param.py b/st2common/st2common/util/param.py
@@ -92,7 +92,9 @@ def _create_graph(action_context, config):
     Creates a generic directed graph for depencency tree and fills it with basic context variables
     """
     G = nx.DiGraph()
-    system_keyvalue_context = {SYSTEM_SCOPE: KeyValueLookup(scope=FULL_SYSTEM_SCOPE)}
+    system_keyvalue_context = {
+        SYSTEM_SCOPE: KeyValueLookup(scope=FULL_SYSTEM_SCOPE, context=action_context)
+    }
 
     # If both 'user' and 'api_user' are specified, this prioritize 'api_user'
     user = action_context["user"] if "user" in action_context else None
@@ -107,7 +109,7 @@ def _create_graph(action_context, config):
         )
 
     system_keyvalue_context[USER_SCOPE] = UserKeyValueLookup(
-        scope=FULL_USER_SCOPE, user=user
+        scope=FULL_USER_SCOPE, user=user, context=action_context
     )
     G.add_node(DATASTORE_PARENT_SCOPE, value=system_keyvalue_context)
     G.add_node(ACTION_CONTEXT_KV_PREFIX, value=action_context)
diff --git a/st2common/tests/unit/test_keyvalue_lookup.py b/st2common/tests/unit/test_keyvalue_lookup.py
@@ -14,15 +14,39 @@
 # limitations under the License.
 
 from __future__ import absolute_import
+
+import mock
+
+from oslo_config import cfg
+
 from st2tests.base import CleanDbTestCase
 from st2common.constants.keyvalue import FULL_SYSTEM_SCOPE, FULL_USER_SCOPE
 from st2common.constants.keyvalue import SYSTEM_SCOPE, USER_SCOPE
+from st2common.constants.types import ResourceType
+from st2common.exceptions.rbac import ResourceAccessDeniedError
+from st2common.models.db.auth import UserDB
 from st2common.models.db.keyvalue import KeyValuePairDB
 from st2common.persistence.keyvalue import KeyValuePair
+from st2common.rbac.backends.noop import NoOpRBACUtils
+from st2common.rbac.types import PermissionType
 from st2common.services.keyvalues import KeyValueLookup, UserKeyValueLookup
+from st2tests import config
+
+USER = "stanley"
+RESOURCE_UUID = "%s:%s:%s" % (
+    ResourceType.KEY_VALUE_PAIR,
+    FULL_USER_SCOPE,
+    "stanley:foobar",
+)
 
 
 class TestKeyValueLookup(CleanDbTestCase):
+    @classmethod
+    def setUpClass(cls):
+        super(TestKeyValueLookup, cls).setUpClass()
+        config.parse_args()
+        cfg.CONF.set_override(name="backend", override="noop", group="rbac")
+
     def test_lookup_with_key_prefix(self):
         KeyValuePair.add_or_update(
             KeyValuePairDB(
@@ -171,3 +195,27 @@ def test_lookup_cast(self):
         self.assertEqual(str(lookup.count), "5.5")
         self.assertEqual(float(lookup.count), 5.5)
         self.assertEqual(int(lookup.count), 5)
+
+    @mock.patch.object(
+        NoOpRBACUtils,
+        "assert_user_has_resource_db_permission",
+        mock.MagicMock(
+            side_effect=ResourceAccessDeniedError(
+                user_db=UserDB(name=USER),
+                resource_api_or_db=KeyValuePairDB(uid=RESOURCE_UUID),
+                permission_type=PermissionType.KEY_VALUE_PAIR_VIEW,
+            )
+        ),
+    )
+    def test_system_kvp_lookup_unauthorized(self):
+        secret_value = (
+            "0055A2D9A09E1071931925933744965EEA7E23DCF59A8D1D7A3"
+            + "64338294916D37E83C4796283C584751750E39844E2FD97A3727DB5D553F638"
+        )
+
+        KeyValuePair.add_or_update(
+            KeyValuePairDB(name="k1", value=secret_value, secret=True)
+        )
+
+        lookup = KeyValueLookup()
+        self.assertRaises(ResourceAccessDeniedError, getattr, lookup, "k1")
diff --git a/st2common/tests/unit/test_util_keyvalue.py b/st2common/tests/unit/test_util_keyvalue.py
@@ -16,6 +16,7 @@
 import mock
 
 import unittest2
+from oslo_config import cfg
 
 from st2common.util import keyvalue as kv_utl
 from st2common.constants.keyvalue import (
@@ -26,14 +27,30 @@
     DATASTORE_PARENT_SCOPE,
     DATASTORE_SCOPE_SEPARATOR,
 )
+from st2common.constants.types import ResourceType
 from st2common.exceptions.rbac import AccessDeniedError
+from st2common.exceptions.rbac import ResourceAccessDeniedError
 from st2common.models.db import auth as auth_db
-
+from st2common.models.db.keyvalue import KeyValuePairDB
+from st2common.rbac.backends.noop import NoOpRBACUtils
+from st2common.rbac.types import PermissionType
+from st2tests import config
 
 USER = "stanley"
+RESOURCE_UUID = "%s:%s:%s" % (
+    ResourceType.KEY_VALUE_PAIR,
+    FULL_USER_SCOPE,
+    "stanley:foobar",
+)
 
 
 class TestKeyValueUtil(unittest2.TestCase):
+    @classmethod
+    def setUpClass(cls):
+        super(TestKeyValueUtil, cls).setUpClass()
+        config.parse_args()
+        cfg.CONF.set_override(name="backend", override="noop", group="rbac")
+
     def test_validate_scope(self):
         scope = FULL_USER_SCOPE
         kv_utl._validate_scope(scope)
@@ -126,3 +143,35 @@ def test_get_key(self, deseralize_key_value, KeyValuePair):
     def test_get_key_invalid_input(self):
         self.assertRaises(TypeError, kv_utl.get_key, key=1)
         self.assertRaises(TypeError, kv_utl.get_key, key="test", decrypt="yep")
+
+    @mock.patch("st2common.util.keyvalue.KeyValuePair")
+    @mock.patch("st2common.util.keyvalue.deserialize_key_value")
+    @mock.patch.object(
+        NoOpRBACUtils,
+        "assert_user_has_resource_db_permission",
+        mock.MagicMock(
+            side_effect=ResourceAccessDeniedError(
+                user_db=auth_db.UserDB(name=USER),
+                resource_api_or_db=KeyValuePairDB(uid=RESOURCE_UUID),
+                permission_type=PermissionType.KEY_VALUE_PAIR_VIEW,
+            )
+        ),
+    )
+    def test_get_key_unauthorized(self, deseralize_key_value, KeyValuePair):
+        key, value = ("foobar", "fubar")
+        decrypt = False
+
+        KeyValuePair.get_by_scope_and_name().value = value
+        deseralize_key_value.return_value = value
+
+        self.assertRaises(
+            ResourceAccessDeniedError,
+            kv_utl.get_key,
+            key=key,
+            user_db=auth_db.UserDB(name=USER),
+            decrypt=decrypt,
+        )
+
+        KeyValuePair.get_by_scope_and_name.assert_called_with(
+            FULL_USER_SCOPE, "stanley:%s" % key
+        )
