Skip to content

Conversation

@dmork123
Copy link
Contributor

@dmork123 dmork123 commented Mar 23, 2022

Create AUDIT log entry when datastore keys are being decrypted either manually (via API which is used by the st2 CLI) or when a workflow/action is triggered and the action's metadata or parameters accesses a secret from pack config or decrypts a datastore key.

added an optional security_audit flag which users can specify in the st2,conf file, This is used to track when keys are being decrypted either manually in the container or when a workflow is triggered and contains an encrypted key in its config.
@pull-request-size pull-request-size bot added the size/S PR that changes 10-29 lines. Very easy to review. label Mar 23, 2022
@CLAassistant
Copy link

CLAassistant commented Mar 23, 2022

CLA assistant check
All committers have signed the CLA.

updated ChangeLog
removed update to conf
fixing for unit test
updated st2 conf sample
updated conf file
@pull-request-size pull-request-size bot added size/M PR that changes 30-99 lines. Good size to review. and removed size/S PR that changes 10-29 lines. Very easy to review. labels Mar 24, 2022
updated conf file
@pull-request-size pull-request-size bot added size/S PR that changes 10-29 lines. Very easy to review. and removed size/M PR that changes 30-99 lines. Good size to review. labels Mar 24, 2022
updated conf file
updated conf file
updated conf file
@pull-request-size pull-request-size bot added size/M PR that changes 30-99 lines. Good size to review. and removed size/S PR that changes 10-29 lines. Very easy to review. labels Mar 24, 2022
removed the need of having another flag in st2,conf, changed the log.info to a log.audit
@pull-request-size pull-request-size bot added size/S PR that changes 10-29 lines. Very easy to review. and removed size/M PR that changes 30-99 lines. Good size to review. labels Mar 25, 2022
reverted back conf file
@pull-request-size pull-request-size bot added size/XS PR that changes 0-9 lines. Quick fix/merge. and removed size/S PR that changes 10-29 lines. Very easy to review. labels Mar 27, 2022
DavidMorkos and others added 2 commits March 31, 2022 09:20
excluded jinja parts from the logs
Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

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

Looks good.

Thanks for the contribution!

Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

I reformatted with black for you :)

I don't think this is complete. Secrets can also be decrypted in ContentPackConfigLoader._get_datastore_value_for_expression() if the schema has secret: true. (see comment).

There may be other places we should add this audit log as well.
For instance, should this also be included in contrib/runners/orquesta_runner/orquesta_functions/st2kv.py which calls kvp_util.get_key() (ie st2common.util.keyvalue.get_key() which calls deserialize_key_value()).

updated location of where audit takes place to occur right after decryption of value occurs
added audit security check in get_all function as well
@pull-request-size pull-request-size bot added size/M PR that changes 30-99 lines. Good size to review. and removed size/S PR that changes 10-29 lines. Very easy to review. labels Mar 31, 2022
added if user decrypts the key
added condition for when the user audits decryption event.
Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

I'm sorry I keep requesting repeated changes - I just want to make sure the AUDIT log entry is as accurate/useful as possible and this isn't the easiest block of code to grok (for me).

Here are some suggestions on how to deal with the log entries in the controller.
The pack config entry looks great to me now.

dmork123 and others added 7 commits April 4, 2022 08:41
updated get_all to account for all decryption events
added log audit message after _get_one_by_scope_and_name
updated the get_all function added user scope
@cognifloyd
Copy link
Member

I was reviewing this on my phone before. I just pushed a commit to move the audit logs slightly so I don't have to keep annoying you (sorry!). @dmork123 what do you think about this? If you're happy with what I did, I'll merge it.

@dmork123
Copy link
Contributor Author

dmork123 commented Apr 5, 2022

I was reviewing this on my phone before. I just pushed a commit to move the audit logs slightly so I don't have to keep annoying you (sorry!). @dmork123 what do you think about this? If you're happy with what I did, I'll merge it.
Hi @cognifloyd
No worries thats all good thank you for your prompt responses I am happy with this, please merge it thank you :)

@cognifloyd cognifloyd enabled auto-merge April 5, 2022 01:39
@cognifloyd cognifloyd merged commit e2e4e26 into StackStorm:master Apr 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M PR that changes 30-99 lines. Good size to review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants