From 11e836d3bb40a847e013e015ea6b9f78eba7bf2a Mon Sep 17 00:00:00 2001 From: W Chan Date: Sun, 9 Oct 2022 14:27:22 -0700 Subject: [PATCH 1/8] Apply RBAC to rendering of KVP during action execution RBAC for key-value pairs in the datastore was recently instroduced. The RBAC policy needs to be applied to rendering of KVP during action and workflow execution. --- st2common/st2common/services/keyvalues.py | 32 +++++++++++++++++++-- st2common/st2common/util/keyvalue.py | 35 ++++++++++++----------- st2common/st2common/util/param.py | 4 +-- 3 files changed, 49 insertions(+), 22 deletions(-) diff --git a/st2common/st2common/services/keyvalues.py b/st2common/st2common/services/keyvalues.py index 03ed248b67..387d2674a8 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,8 @@ 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 +122,10 @@ def __init__( self._value_cache = cache or {} self._scope = scope + self._context = context if context else dict() + self._user = context["user"] if "user" in context and context["user"] else cfg.CONF.system_user.user + self._user = context["api_user"] if "api_user" in context and context["api_user"] else self._user + def __str__(self): return self._value_cache[self._key_prefix] @@ -154,6 +164,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 +178,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 +199,8 @@ 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 +213,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..2955703557 100644 --- a/st2common/st2common/util/param.py +++ b/st2common/st2common/util/param.py @@ -92,7 +92,7 @@ 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 +107,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) From 7b77c629c5d7731dbe13e74ac6edf85e4d054784 Mon Sep 17 00:00:00 2001 From: W Chan Date: Sun, 9 Oct 2022 14:44:33 -0700 Subject: [PATCH 2/8] Changes made by black format --- st2common/st2common/services/keyvalues.py | 29 ++++++++++++++++++----- st2common/st2common/util/param.py | 4 +++- 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/st2common/st2common/services/keyvalues.py b/st2common/st2common/services/keyvalues.py index 387d2674a8..2393993a0a 100644 --- a/st2common/st2common/services/keyvalues.py +++ b/st2common/st2common/services/keyvalues.py @@ -108,8 +108,12 @@ class KeyValueLookup(BaseKeyValueLookup): scope = SYSTEM_SCOPE def __init__( - self, prefix=None, key_prefix=None, cache=None, - scope=FULL_SYSTEM_SCOPE, context=None + self, + prefix=None, + key_prefix=None, + cache=None, + scope=FULL_SYSTEM_SCOPE, + context=None, ): if not scope: scope = FULL_SYSTEM_SCOPE @@ -123,8 +127,16 @@ def __init__( self._scope = scope self._context = context if context else dict() - self._user = context["user"] if "user" in context and context["user"] else cfg.CONF.system_user.user - self._user = context["api_user"] if "api_user" in context and context["api_user"] else self._user + self._user = ( + context["user"] + if "user" in context and context["user"] + else cfg.CONF.system_user.user + ) + self._user = ( + context["api_user"] + if "api_user" in context and context["api_user"] + else self._user + ) def __str__(self): return self._value_cache[self._key_prefix] @@ -199,8 +211,13 @@ class UserKeyValueLookup(BaseKeyValueLookup): scope = USER_SCOPE def __init__( - self, user, prefix=None, key_prefix=None, cache=None, - scope=FULL_USER_SCOPE, context=None + self, + user, + prefix=None, + key_prefix=None, + cache=None, + scope=FULL_USER_SCOPE, + context=None, ): if not scope: scope = FULL_USER_SCOPE diff --git a/st2common/st2common/util/param.py b/st2common/st2common/util/param.py index 2955703557..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, context=action_context)} + 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 From 4bab328756261fc251e1644598ba2699ed6d5447 Mon Sep 17 00:00:00 2001 From: W Chan Date: Tue, 11 Oct 2022 10:59:39 -0700 Subject: [PATCH 3/8] WIP --- conf/st2.dev.conf | 5 +++-- st2auth/conf/htpasswd_dev | 2 ++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/conf/st2.dev.conf b/conf/st2.dev.conf index cf2b5b6596..dc25ea396c 100644 --- a/conf/st2.dev.conf +++ b/conf/st2.dev.conf @@ -42,14 +42,15 @@ logging = st2actions/conf/logging.conf stream_output = True [rbac] -enable = False +enable = True +backend = default [auth] host = 127.0.0.1 port = 9100 use_ssl = False debug = False -enable = False +enable = True logging = st2auth/conf/logging.conf mode = standalone diff --git a/st2auth/conf/htpasswd_dev b/st2auth/conf/htpasswd_dev index 09e3701940..3e20471402 100644 --- a/st2auth/conf/htpasswd_dev +++ b/st2auth/conf/htpasswd_dev @@ -1 +1,3 @@ testu:{SHA}V1t6eZLxnehb7CTBuj61Nq3lIh4= +parker:$apr1$Dt/7pKEL$8OUMcZMh6J5MmAkZtuKH0. +jarvis:$apr1$7/C/VVtX$TSL3Ivsk3eLB0DhJDAhzG1 From 7d65a64359f4f23965bdb655a4baa74d9e89f794 Mon Sep 17 00:00:00 2001 From: W Chan Date: Sat, 15 Oct 2022 15:06:50 -0700 Subject: [PATCH 4/8] Fix unit tests and add change log entry to pass checks --- CHANGELOG.rst | 4 ++++ st2common/st2common/services/keyvalues.py | 4 ++-- st2common/tests/unit/test_util_keyvalue.py | 9 ++++++++- 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index b0aafea9c0..6c8583c37f 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 2393993a0a..824f4bf048 100644 --- a/st2common/st2common/services/keyvalues.py +++ b/st2common/st2common/services/keyvalues.py @@ -129,12 +129,12 @@ def __init__( self._context = context if context else dict() self._user = ( context["user"] - if "user" in context and context["user"] + if context and "user" in context and context["user"] else cfg.CONF.system_user.user ) self._user = ( context["api_user"] - if "api_user" in context and context["api_user"] + if context and "api_user" in context and context["api_user"] else self._user ) diff --git a/st2common/tests/unit/test_util_keyvalue.py b/st2common/tests/unit/test_util_keyvalue.py index 5a8c15a3f7..0574877138 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 ( @@ -28,12 +29,18 @@ ) from st2common.exceptions.rbac import AccessDeniedError from st2common.models.db import auth as auth_db - +from st2tests import config USER = "stanley" 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) From b88580fdbcce0f86c2f58c4b94cb376fd86c0d0c Mon Sep 17 00:00:00 2001 From: W Chan Date: Sat, 15 Oct 2022 18:54:28 -0700 Subject: [PATCH 5/8] Remove test data in htpasswd_dev --- st2auth/conf/htpasswd_dev | 2 -- 1 file changed, 2 deletions(-) diff --git a/st2auth/conf/htpasswd_dev b/st2auth/conf/htpasswd_dev index 3e20471402..09e3701940 100644 --- a/st2auth/conf/htpasswd_dev +++ b/st2auth/conf/htpasswd_dev @@ -1,3 +1 @@ testu:{SHA}V1t6eZLxnehb7CTBuj61Nq3lIh4= -parker:$apr1$Dt/7pKEL$8OUMcZMh6J5MmAkZtuKH0. -jarvis:$apr1$7/C/VVtX$TSL3Ivsk3eLB0DhJDAhzG1 From 60d6ed6a7ad47cd174ffedc4a156dd2e3cf22f27 Mon Sep 17 00:00:00 2001 From: W Chan Date: Sat, 15 Oct 2022 19:10:13 -0700 Subject: [PATCH 6/8] Add unit tests to cover rbac unauthorized error on kvp lookup --- st2common/tests/unit/test_keyvalue_lookup.py | 48 ++++++++++++++++++++ st2common/tests/unit/test_util_keyvalue.py | 42 +++++++++++++++++ 2 files changed, 90 insertions(+) diff --git a/st2common/tests/unit/test_keyvalue_lookup.py b/st2common/tests/unit/test_keyvalue_lookup.py index afcd76901a..cf86007a46 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" + ) + + k1 = 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 0574877138..14c0996143 100644 --- a/st2common/tests/unit/test_util_keyvalue.py +++ b/st2common/tests/unit/test_util_keyvalue.py @@ -27,11 +27,21 @@ 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): @@ -133,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 + ) From 30d69c2516460591b0e03aae4e05c8bc5e3e3b83 Mon Sep 17 00:00:00 2001 From: W Chan Date: Sat, 15 Oct 2022 19:12:05 -0700 Subject: [PATCH 7/8] Rollback rbac test config --- conf/st2.dev.conf | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/conf/st2.dev.conf b/conf/st2.dev.conf index dc25ea396c..cf2b5b6596 100644 --- a/conf/st2.dev.conf +++ b/conf/st2.dev.conf @@ -42,15 +42,14 @@ logging = st2actions/conf/logging.conf stream_output = True [rbac] -enable = True -backend = default +enable = False [auth] host = 127.0.0.1 port = 9100 use_ssl = False debug = False -enable = True +enable = False logging = st2auth/conf/logging.conf mode = standalone From 713e5c5406915c7c78975640a90afecd8bf23ec6 Mon Sep 17 00:00:00 2001 From: W Chan Date: Sat, 15 Oct 2022 19:20:55 -0700 Subject: [PATCH 8/8] Fix lint error --- st2common/tests/unit/test_keyvalue_lookup.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/st2common/tests/unit/test_keyvalue_lookup.py b/st2common/tests/unit/test_keyvalue_lookup.py index cf86007a46..4904368382 100644 --- a/st2common/tests/unit/test_keyvalue_lookup.py +++ b/st2common/tests/unit/test_keyvalue_lookup.py @@ -213,7 +213,7 @@ def test_system_kvp_lookup_unauthorized(self): + "64338294916D37E83C4796283C584751750E39844E2FD97A3727DB5D553F638" ) - k1 = KeyValuePair.add_or_update( + KeyValuePair.add_or_update( KeyValuePairDB(name="k1", value=secret_value, secret=True) )