Skip to content

Disallow decrypt_kv jinja filter for fields marked as secret for pack config #4709

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

Merged
merged 8 commits into from
Jun 26, 2019

Conversation

VineeshJain
Copy link
Contributor

This PR addresses the issue where in pack config if fields are marked as secret: true and if user specifies jinja expression with filter decrypt_kv, the values are decrypted twice. This is due to the fact that for all fields marked as secret, the values are auto decrypted. Specifying an additional decrypt_kv filter causes issue.

The fix is to raise exceptions if decrypt_kv is specified for any fields marked secret. This is done for all three st2 pack config, Pack config from the WebUI and for st2ctl reload --reload-configs

@VineeshJain
Copy link
Contributor Author

VineeshJain commented Jun 11, 2019

Here is the sample output for st2 pack config command:

(virtualenv) vagrant@ubuntu-xenial:~/st2$ st2 pack config hello_st2
name (secret):
name (secret): ***********************************
---
---
Do you want to preview the config in an editor before saving? [y]: n
---
Do you want me to save it? [y]: y
ERROR: 400 Client Error: Bad Request
MESSAGE: Validation Error: decrypt_kv jinja filter specified for auto decrypted fields marked with `secret: True` for url: http://127.0.0.1:9101/v1/configs/hello_st2

@VineeshJain
Copy link
Contributor Author

Here is the output for st2ctl reload --register-configs command:

vijain@vijain-virtual-machine:~/workspace/st2$ st2ctl reload --register-configs
Registering content...[flags = --config-file /etc/st2/st2.conf --register-configs]
2019-06-11 16:45:58,557 INFO [-] Connecting to database "st2" @ "127.0.0.1:27017" as user "stackstorm".
2019-06-11 16:45:58,564 INFO [-] Successfully connected to database "st2" @ "127.0.0.1:27017" as user "stackstorm".
2019-06-11 16:45:59,196 INFO [-] =========================================================
2019-06-11 16:45:59,196 INFO [-] ############## Registering configs ######################
2019-06-11 16:45:59,196 INFO [-] =========================================================
2019-06-11 16:45:59,442 WARNING [-] Failed to register configs: Failed to register config "/opt/stackstorm/configs/hello_st2.yaml" for pack "hello_st2": Validation Error: decrypt_kv jinja filter specified for auto decrypted fields marked with `secret: True`
Traceback (most recent call last):
  File "/usr/bin/st2-register-content", line 22, in <module>
    sys.exit(content_loader.main(sys.argv[1:]))
  File "/opt/stackstorm/st2/local/lib/python2.7/site-packages/st2common/content/bootstrap.py", line 408, in main
    register_content()
  File "/opt/stackstorm/st2/local/lib/python2.7/site-packages/st2common/content/bootstrap.py", line 391, in register_content
    register_configs()
  File "/opt/stackstorm/st2/local/lib/python2.7/site-packages/st2common/content/bootstrap.py", line 346, in register_configs
    raise e
ValueError: Failed to register config "/opt/stackstorm/configs/hello_st2.yaml" for pack "hello_st2": Validation Error: decrypt_kv jinja filter specified for auto decrypted fields marked with `secret: True`
##### st2 components status #####
st2actionrunner PID: 20873
st2actionrunner PID: 20875

This commit addresses the issue where in pack config if fields are marked as
secret: true and if user specifies jinja expression with filter decrypt_kv,
the values are decrypted twice. This is due to the fact that for all fields
marked as secret, the values are auto decrypted. Specifying an additional
decrypt_kv filter causes issue.

The commit raises exceptions if decrypt_kv is specified for any fields marked secret.
@VineeshJain VineeshJain force-pushed the issue/disallowDecryptKVForValuesMarkedSecret branch from cc863c8 to 5634bfb Compare June 12, 2019 00:45
@VineeshJain VineeshJain requested a review from Kami June 12, 2019 17:17
Copy link
Contributor

@m4dcoder m4dcoder left a comment

Choose a reason for hiding this comment

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

Where are the unit test(s)? Is this cover elsewhere? If so, please identify.

@VineeshJain VineeshJain force-pushed the issue/disallowDecryptKVForValuesMarkedSecret branch from eabc4a9 to d514dbf Compare June 20, 2019 19:11
This commit updates the error message shown to the user and also adds
unit testcase to check for exception when decrypt_kv filter is specified
for field marked as `secret: True`
@VineeshJain VineeshJain force-pushed the issue/disallowDecryptKVForValuesMarkedSecret branch from d514dbf to f7b4636 Compare June 20, 2019 19:28
@VineeshJain VineeshJain force-pushed the issue/disallowDecryptKVForValuesMarkedSecret branch from ed4fb3a to 2a00552 Compare June 20, 2019 22:14
@m4dcoder
Copy link
Contributor

@VineeshJain Please remove Address code review comment - from all of the commit title. Just focus the title on the actual change. Thanks.

@VineeshJain VineeshJain force-pushed the issue/disallowDecryptKVForValuesMarkedSecret branch from 9d86548 to 70d190a Compare June 24, 2019 22:55
@m4dcoder
Copy link
Contributor

@VineeshJain You still need to fix the titles for the earlier commits.

@VineeshJain
Copy link
Contributor Author

@VineeshJain You still need to fix the titles for the earlier commits.

I know, I am not done yet.

@VineeshJain VineeshJain force-pushed the issue/disallowDecryptKVForValuesMarkedSecret branch from 70d190a to 97c503c Compare June 24, 2019 23:19
VineeshJain added a commit that referenced this pull request Jun 24, 2019
@VineeshJain VineeshJain force-pushed the issue/disallowDecryptKVForValuesMarkedSecret branch from b44d83a to 719bddd Compare June 26, 2019 02:24
@m4dcoder m4dcoder merged commit 17febe1 into master Jun 26, 2019
@m4dcoder m4dcoder deleted the issue/disallowDecryptKVForValuesMarkedSecret branch June 26, 2019 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants