diff --git a/CHANGELOG.rst b/CHANGELOG.rst index b93d5f05cc..2a6f4937ea 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -15,7 +15,7 @@ Added * Add support for `immutable_parameters` on Action Aliases. This feature allows default parameters to be supplied to the action on every execution of the alias. #4786 * Add ``get_entrypoint()`` method to ``ActionResourceManager`` attribute of st2client. - #4791 + #4791 Changed ~~~~~~~ @@ -61,6 +61,10 @@ Fixed doesn't depend on internal pip API and so it works with latest pip version. (bug fix) #4750 * Fix dependency conflicts in pack CI runs: downgrade requests dependency back to 0.21.0, update internal dependencies and test expectations (amqp, pyyaml, prance, six) (bugfix) #4774 +* Fix secrets masking in action parameters section defined inside the rule when using + ``GET /v1/rules`` and ``GET /v1/rules/`` API endpoint. (bug fix) #4788 #4807 + + Contributed by @Nicodemos305 and @jeansfelix 3.1.0 - June 27, 2019 --------------------- diff --git a/st2api/st2api/controllers/v1/rule_views.py b/st2api/st2api/controllers/v1/rule_views.py index c607dac0cb..04c1114bad 100644 --- a/st2api/st2api/controllers/v1/rule_views.py +++ b/st2api/st2api/controllers/v1/rule_views.py @@ -99,8 +99,9 @@ def get_all(self, exclude_attributes=None, include_attributes=None, sort=None, o return rules def get_one(self, ref_or_id, requester_user): + from_model_kwargs = {'mask_secrets': True} rule = self._get_one(ref_or_id, permission_type=PermissionType.RULE_VIEW, - requester_user=requester_user) + requester_user=requester_user, from_model_kwargs=from_model_kwargs) result = self._append_view_properties([rule.json])[0] rule.json = result return rule diff --git a/st2api/st2api/controllers/v1/rules.py b/st2api/st2api/controllers/v1/rules.py index d64fb73732..4921b7b279 100644 --- a/st2api/st2api/controllers/v1/rules.py +++ b/st2api/st2api/controllers/v1/rules.py @@ -20,6 +20,7 @@ from st2common import log as logging from st2common.exceptions.apivalidation import ValueValidationException from st2common.exceptions.triggers import TriggerDoesNotExistException +from st2api.controllers.base import BaseRestControllerMixin from st2api.controllers.resource import BaseResourceIsolationControllerMixin from st2api.controllers.resource import ContentPackResourceController from st2api.controllers.controller_transforms import transform_to_bool @@ -39,7 +40,8 @@ LOG = logging.getLogger(__name__) -class RuleController(BaseResourceIsolationControllerMixin, ContentPackResourceController): +class RuleController(BaseRestControllerMixin, BaseResourceIsolationControllerMixin, + ContentPackResourceController): """ Implements the RESTful web endpoint that handles the lifecycle of Rules in the system. @@ -68,8 +70,11 @@ class RuleController(BaseResourceIsolationControllerMixin, ContentPackResourceCo mandatory_include_fields_retrieve = ['pack', 'name', 'trigger'] def get_all(self, exclude_attributes=None, include_attributes=None, sort=None, offset=0, - limit=None, requester_user=None, **raw_filters): - from_model_kwargs = {'ignore_missing_trigger': True} + limit=None, show_secrets=False, requester_user=None, **raw_filters): + from_model_kwargs = { + 'ignore_missing_trigger': True, + 'mask_secrets': self._get_mask_secrets(requester_user, show_secrets=show_secrets) + } return super(RuleController, self)._get_all(exclude_fields=exclude_attributes, include_fields=include_attributes, from_model_kwargs=from_model_kwargs, @@ -79,8 +84,11 @@ def get_all(self, exclude_attributes=None, include_attributes=None, sort=None, o raw_filters=raw_filters, requester_user=requester_user) - def get_one(self, ref_or_id, requester_user): - from_model_kwargs = {'ignore_missing_trigger': True} + def get_one(self, ref_or_id, requester_user, show_secrets=False): + from_model_kwargs = { + 'ignore_missing_trigger': True, + 'mask_secrets': self._get_mask_secrets(requester_user, show_secrets=show_secrets) + } return super(RuleController, self)._get_one(ref_or_id, from_model_kwargs=from_model_kwargs, requester_user=requester_user, permission_type=PermissionType.RULE_VIEW) diff --git a/st2api/tests/unit/controllers/v1/test_rules.py b/st2api/tests/unit/controllers/v1/test_rules.py index ae8063095f..3651bef6ac 100644 --- a/st2api/tests/unit/controllers/v1/test_rules.py +++ b/st2api/tests/unit/controllers/v1/test_rules.py @@ -20,6 +20,7 @@ from st2common.constants.rules import RULE_TYPE_STANDARD, RULE_TYPE_BACKSTOP from st2common.constants.pack import DEFAULT_PACK_NAME +from st2common.constants.secrets import MASKED_ATTRIBUTE_VALUE from st2common.persistence.trigger import Trigger from st2common.models.system.common import ResourceReference from st2common.transport.publishers import PoolPublisher @@ -185,6 +186,58 @@ def test_get_all_enabled(self): self.__do_delete(self.__get_rule_id(post_resp_rule_1)) self.__do_delete(self.__get_rule_id(post_resp_rule_3)) + def test_get_all_action_parameters_secrets_masking(self): + post_resp_rule_1 = self.__do_post(RulesControllerTestCase.RULE_1) + + # Verify parameter is masked by default + resp = self.app.get('/v1/rules') + self.assertEqual('action' in resp.json[0], True) + self.assertEqual(resp.json[0]['action']['parameters']['action_secret'], + MASKED_ATTRIBUTE_VALUE) + + # Verify ?show_secrets=true works + resp = self.app.get('/v1/rules?include_attributes=action&show_secrets=true') + self.assertEqual('action' in resp.json[0], True) + self.assertEqual(resp.json[0]['action']['parameters']['action_secret'], 'secret') + + self.__do_delete(self.__get_rule_id(post_resp_rule_1)) + + def test_get_all_parameters_mask_with_exclude_parameters(self): + post_resp_rule_1 = self.__do_post(RulesControllerTestCase.RULE_1) + resp = self.app.get('/v1/rules?exclude_attributes=action') + self.assertEqual('action' in resp.json[0], False) + self.__do_delete(self.__get_rule_id(post_resp_rule_1)) + + def test_get_all_parameters_mask_with_include_parameters(self): + post_resp_rule_1 = self.__do_post(RulesControllerTestCase.RULE_1) + + # Verify parameter is masked by default + resp = self.app.get('/v1/rules?include_attributes=action') + self.assertEqual('action' in resp.json[0], True) + self.assertEqual(resp.json[0]['action']['parameters']['action_secret'], + MASKED_ATTRIBUTE_VALUE) + + # Verify ?show_secrets=true works + resp = self.app.get('/v1/rules?include_attributes=action&show_secrets=true') + self.assertEqual('action' in resp.json[0], True) + self.assertEqual(resp.json[0]['action']['parameters']['action_secret'], 'secret') + + self.__do_delete(self.__get_rule_id(post_resp_rule_1)) + + def test_get_one_action_parameters_secrets_masking(self): + post_resp_rule_1 = self.__do_post(RulesControllerTestCase.RULE_1) + + # Verify parameter is masked by default + resp = self.app.get('/v1/rules/%s' % (post_resp_rule_1.json['id'])) + self.assertEqual(resp.json['action']['parameters']['action_secret'], + MASKED_ATTRIBUTE_VALUE) + + # Verify ?show_secrets=true works + resp = self.app.get('/v1/rules/%s?show_secrets=true' % (post_resp_rule_1.json['id'])) + self.assertEqual(resp.json['action']['parameters']['action_secret'], 'secret') + + self.__do_delete(self.__get_rule_id(post_resp_rule_1)) + def test_get_one_by_id(self): post_resp = self.__do_post(RulesControllerTestCase.RULE_1) rule_id = self.__get_rule_id(post_resp) diff --git a/st2common/st2common/models/db/rule.py b/st2common/st2common/models/db/rule.py index 7492b5abaa..e56d66cea9 100644 --- a/st2common/st2common/models/db/rule.py +++ b/st2common/st2common/models/db/rule.py @@ -13,11 +13,15 @@ # limitations under the License. from __future__ import absolute_import +import copy import mongoengine as me +from st2common.persistence.action import Action from st2common.models.db import MongoDBAccess from st2common.models.db import stormbase from st2common.constants.types import ResourceType +from st2common.util.secrets import get_secret_parameters +from st2common.util.secrets import mask_secret_parameters class RuleTypeDB(stormbase.StormBaseDB): @@ -101,6 +105,55 @@ class RuleDB(stormbase.StormFoundationDB, stormbase.TagsMixin, stormbase.UIDFieldMixin.get_indexes()) } + def mask_secrets(self, value): + """ + Process the model dictionary and mask secret values. + + NOTE: This method results in one addition "get one" query where we retrieve corresponding + action model so we can correctly mask secret parameters. + + :type value: ``dict`` + :param value: Document dictionary. + + :rtype: ``dict`` + """ + result = copy.deepcopy(value) + + action_ref = result.get('action', {}).get('ref', None) + + if not action_ref: + return result + + action_db = self._get_referenced_action_model(action_ref=action_ref) + + if not action_db: + return result + + secret_parameters = get_secret_parameters(parameters=action_db.parameters) + result['action']['parameters'] = mask_secret_parameters( + parameters=result['action']['parameters'], + secret_parameters=secret_parameters) + + return result + + def _get_referenced_action_model(self, action_ref): + """ + Return Action object for the action referenced in a rule. + + :param action_ref: Action reference. + :type action_ref: ``str`` + + :rtype: ``ActionDB`` + """ + # NOTE: We need to retrieve pack and name since that's needed for the PK + action_dbs = Action.query(only_fields=['pack', 'ref', 'name', 'parameters'], + ref=action_ref, limit=1) + + if action_dbs: + return action_dbs[0] + + return None + def __init__(self, *args, **values): super(RuleDB, self).__init__(*args, **values) self.ref = self.get_reference().ref diff --git a/st2common/st2common/openapi.yaml b/st2common/st2common/openapi.yaml index 91e299ff05..0896532f74 100644 --- a/st2common/st2common/openapi.yaml +++ b/st2common/st2common/openapi.yaml @@ -2649,6 +2649,10 @@ paths: in: query description: Enabled filter type: string + - name: show_secrets + in: query + description: Show secrets in plain text + type: boolean x-parameters: - name: user in: context @@ -2710,6 +2714,10 @@ paths: description: Entity reference or id type: string required: true + - name: show_secrets + in: query + description: Show secrets in plain text + type: boolean x-parameters: - name: user in: context diff --git a/st2common/st2common/openapi.yaml.j2 b/st2common/st2common/openapi.yaml.j2 index 253f45cfde..3387564e8a 100644 --- a/st2common/st2common/openapi.yaml.j2 +++ b/st2common/st2common/openapi.yaml.j2 @@ -2645,6 +2645,10 @@ paths: in: query description: Enabled filter type: string + - name: show_secrets + in: query + description: Show secrets in plain text + type: boolean x-parameters: - name: user in: context @@ -2706,6 +2710,10 @@ paths: description: Entity reference or id type: string required: true + - name: show_secrets + in: query + description: Show secrets in plain text + type: boolean x-parameters: - name: user in: context diff --git a/st2common/tests/unit/test_db.py b/st2common/tests/unit/test_db.py index 8db54e7cf4..462ab8fa3e 100644 --- a/st2common/tests/unit/test_db.py +++ b/st2common/tests/unit/test_db.py @@ -21,7 +21,6 @@ import mongoengine.connection from oslo_config import cfg from pymongo.errors import ConnectionFailure - from st2common.constants.triggers import TRIGGER_INSTANCE_PROCESSED from st2common.models.system.common import ResourceReference from st2common.transport.publishers import PoolPublisher @@ -521,6 +520,10 @@ def _delete(model_objects): "p3": { "type": "boolean", "default": False + }, + "p4": { + "type": "string", + "secret": True } }, "additionalProperties": False @@ -661,12 +664,13 @@ def _create_save_action(runnertype, metadata=False): runner_type={'name': runnertype.name}) if not metadata: - created.parameters = {'p1': None, 'p2': None, 'p3': None} + created.parameters = {'p1': None, 'p2': None, 'p3': None, 'p4': None} else: created.parameters = { 'p1': {'type': 'string', 'required': True}, 'p2': {'type': 'number', 'default': 2868}, - 'p3': {'type': 'boolean', 'default': False} + 'p3': {'type': 'boolean', 'default': False}, + 'p4': {'type': 'string', 'secret': True} } return Action.add_or_update(created) diff --git a/st2tests/st2tests/fixtures/generic/actions/action1.yaml b/st2tests/st2tests/fixtures/generic/actions/action1.yaml index 26522c26e3..15422c71b7 100644 --- a/st2tests/st2tests/fixtures/generic/actions/action1.yaml +++ b/st2tests/st2tests/fixtures/generic/actions/action1.yaml @@ -18,6 +18,9 @@ parameters: async_test: default: false type: boolean + action_secret: + type: string + secret: true runnerdummy: default: actiondummy immutable: true diff --git a/st2tests/st2tests/fixtures/generic/rules/rule1.yaml b/st2tests/st2tests/fixtures/generic/rules/rule1.yaml index a8c0809138..0e4302e7ca 100644 --- a/st2tests/st2tests/fixtures/generic/rules/rule1.yaml +++ b/st2tests/st2tests/fixtures/generic/rules/rule1.yaml @@ -3,6 +3,7 @@ action: parameters: ip1: '{{trigger.t1_p}}' ip2: '{{trigger}}' + action_secret: 'secret' ref: wolfpack.action-1 criteria: trigger.t1_p: