Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added mask of rule action secret parameters #4788

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
cfe0aa7
Fix the visibility of action secret parameters on rule
jeansfelix Sep 12, 2019
0f1bae2
Fix PEP8
jeansfelix Sep 12, 2019
c604c15
Merge branch 'master' into fix/mask-secrets-rule-parameters
jeansfelix Sep 12, 2019
4f7f7a1
Add tests when secrets masking works correctly when ?include_attribut…
Nicodemos305 Oct 9, 2019
36980e5
Change name method test_get_one_parameters_mask_with_include_paramete…
Nicodemos305 Oct 9, 2019
6b9eb4e
add assertequal for length of json
Nicodemos305 Oct 9, 2019
42eb393
Add test model test_rule_with_secret_parameter_masked
Nicodemos305 Oct 9, 2019
76cd651
Fix syntax error
Nicodemos305 Oct 10, 2019
5defcda
Fix api path
Nicodemos305 Oct 10, 2019
421321d
Add default value p4 parameter
Nicodemos305 Oct 10, 2019
0cc4cb5
Include import constant
Nicodemos305 Oct 10, 2019
c07522a
Fix whitespace
Nicodemos305 Oct 10, 2019
8b01949
Fix delete before finish test method
Nicodemos305 Oct 10, 2019
9f3feaf
Fix parameters
Nicodemos305 Oct 10, 2019
7ff661e
Fix parameters
Nicodemos305 Oct 10, 2019
4182f4b
Remove blank lines
Nicodemos305 Oct 10, 2019
46dd3fa
Simple string in secret true parameter
Nicodemos305 Oct 10, 2019
7ad78a9
Add import RuleTypeDB
Nicodemos305 Oct 11, 2019
4c0fd03
Fix test_rule_with_secret_parameter_masked
Nicodemos305 Oct 11, 2019
3271499
Fix test_rule_with_secret_parameter_masked
Nicodemos305 Oct 11, 2019
4ffaa6f
Remove test test_rule_with_secret_parameter_masked
Nicodemos305 Oct 11, 2019
4d7e552
Remove parameters p4
Nicodemos305 Oct 11, 2019
ee220c1
Fix syntax error
Nicodemos305 Oct 11, 2019
b73e1c3
Remove unnecessary constant
Nicodemos305 Oct 11, 2019
a202c4b
Remove All tests test_get_all_parameters_mask_with_include_parameters…
Nicodemos305 Oct 11, 2019
1ca2854
Add tests with include_attribute and exclude_attributes
Nicodemos305 Oct 11, 2019
a644014
Model tests done, WIP API Tests
Nicodemos305 Oct 16, 2019
8faa9ab
Fix get api rule include attribute
Nicodemos305 Oct 17, 2019
a1b97c6
Include test key action in map rule
Nicodemos305 Oct 17, 2019
07d4eb7
Clean Lint errors
Nicodemos305 Oct 17, 2019
b3a7531
Fix lin rule.py
Nicodemos305 Oct 17, 2019
8277c78
Fix test rule
Nicodemos305 Oct 17, 2019
61b8b58
Fix Db rule
Nicodemos305 Oct 17, 2019
f1c4de4
Fix method ref_query_args
Nicodemos305 Oct 17, 2019
8b85474
Fix Lint
Nicodemos305 Oct 17, 2019
cb3b35c
Merge branch 'master' into fix/mask-secrets-rule-parameters
blag Oct 18, 2019
4614d44
Add changelog Added mask of rule action secret parameters #4788
Nicodemos305 Oct 18, 2019
8e80197
Merge branch 'fix/mask-secrets-rule-parameters' of github.com:pepepro…
Nicodemos305 Oct 18, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.rst
Expand Up @@ -50,6 +50,11 @@ 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 mask of rule action secret parameters:
When we had an action on a rule with secret type parameters, the parameters were visible. This behavior
has been resolved in PR4788.
Contributed by @Nicodemos305 and @jeansfelix


3.1.0 - June 27, 2019
---------------------
Expand Down
3 changes: 2 additions & 1 deletion st2api/st2api/controllers/v1/rule_views.py
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions st2api/st2api/controllers/v1/rules.py
Expand Up @@ -69,7 +69,7 @@ class RuleController(BaseResourceIsolationControllerMixin, ContentPackResourceCo

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}
from_model_kwargs = {'ignore_missing_trigger': True, 'mask_secrets': True}
return super(RuleController, self)._get_all(exclude_fields=exclude_attributes,
include_fields=include_attributes,
from_model_kwargs=from_model_kwargs,
Expand All @@ -80,7 +80,7 @@ def get_all(self, exclude_attributes=None, include_attributes=None, sort=None, o
requester_user=requester_user)

def get_one(self, ref_or_id, requester_user):
from_model_kwargs = {'ignore_missing_trigger': True}
from_model_kwargs = {'ignore_missing_trigger': True, 'mask_secrets': True}
return super(RuleController, self)._get_one(ref_or_id, from_model_kwargs=from_model_kwargs,
requester_user=requester_user,
permission_type=PermissionType.RULE_VIEW)
Expand Down
12 changes: 12 additions & 0 deletions st2api/tests/unit/controllers/v1/test_rules.py
Expand Up @@ -185,6 +185,18 @@ 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_parameters_mask_with_include_parameters(self):
post_resp_rule_1 = self.__do_post(RulesControllerTestCase.RULE_1)
resp = self.app.get('/v1/rules?include_attributes=action')
self.assertEqual('action' in resp.json[0], True)
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_one_by_id(self):
post_resp = self.__do_post(RulesControllerTestCase.RULE_1)
rule_id = self.__get_rule_id(post_resp)
Expand Down
54 changes: 54 additions & 0 deletions st2common/st2common/models/db/rule.py
Expand Up @@ -13,11 +13,16 @@
# limitations under the License.

from __future__ import absolute_import
import copy
import mongoengine as me

from mongoengine.queryset import Q
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):
Expand Down Expand Up @@ -101,6 +106,55 @@ class RuleDB(stormbase.StormFoundationDB, stormbase.TagsMixin,
stormbase.UIDFieldMixin.get_indexes())
}

def mask_secrets(self, value):
"""
Process the model dictionary and mask secret values.

:type value: ``dict``
:param value: Document dictionary.

:rtype: ``dict``
"""
result = copy.deepcopy(value)
if('action' not in result):
return result
action_db = self._get_referenced_models(rule=result)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this change looks good to me, my only concern is performance impact of this change.

This change means we now need to perform 1 additional query for each Rule DB object.

This is not an issue for "get one" operation (only one additional query), but when retrieving multiple rules aka "get all" API operations, this would result in N additional queries where N is number of rules on the page.


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_models(self, rule):
"""
Return the action model referenced from rule.

:type rule: ``dict``
:param rule: rule

:rtype: ``ActionDB``
"""
action_ref = rule['action']['ref']

def ref_query_args(ref):
return {'ref': ref}

action_db = self._get_entity(model_persistence=Action,
ref=action_ref,
query_args=ref_query_args)

return action_db

def _get_entity(self, model_persistence, ref, query_args):
q = Q(**query_args(ref))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we really only need parameters field from the action model, so we should update this query to only retrieve that field by utilizing only_fields argument / query.only(**fields) function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a related note - is there a specific reason why you used Q object directly?

To not break the abstraction, we should probably utilize Action.query() method instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a heads up. I will look into making the changes mentioned above^.

This way we can hopefully get this across the finish line and merge it soon.


if q:
return model_persistence._get_impl().model.objects(q).first()

return None

def __init__(self, *args, **values):
super(RuleDB, self).__init__(*args, **values)
self.ref = self.get_reference().ref
Expand Down
10 changes: 7 additions & 3 deletions st2common/tests/unit/test_db.py
Expand Up @@ -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
Expand Down Expand Up @@ -521,6 +520,10 @@ def _delete(model_objects):
"p3": {
"type": "boolean",
"default": False
},
"p4": {
"type": "string",
"secret": True
}
},
"additionalProperties": False
Expand Down Expand Up @@ -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)

Expand Down