diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 7183254777..31c5db11e6 100644 --- 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 index 03ed248b67..824f4bf048 100644 --- a/st2common/st2common/services/keyvalues.py +++ b/st2common/st2common/services/keyvalues.py @@ -15,6 +15,8 @@ from __future__ import absolute_import +from oslo_config import cfg + from st2common import log as logging from st2common.constants.keyvalue import DATASTORE_PARENT_SCOPE @@ -22,14 +24,17 @@ 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 index 1fc538025f..e97e7fb549 100644 --- 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 index 90617a78d9..67fb83e9ac 100644 --- 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 index afcd76901a..4904368382 100644 --- 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 index 5a8c15a3f7..14c0996143 100644 --- 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 + )