Merge pull request #4807 from StackStorm/fix/mask-secrets-rule-parame…

…ters

/v1/rules API endpoint action parameters secrets masking (additional changes on top of #4788)

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/<ref>`` API endpoint. (bug fix) #4788 #4807
+
+  Contributed by @Nicodemos305 and @jeansfelix
 
 3.1.0 - June 27, 2019
 ---------------------

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

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)

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)

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

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

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

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)
 

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

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: