strongdm / comply

Compliance automation framework, focused on SOC2
https://comply.strongdm.com
Apache License 2.0
1.33k stars 248 forks source link

Default controls to unsatisfied #79

Closed joepurdy closed 4 years ago

joepurdy commented 4 years ago

This would fix https://github.com/strongdm/comply/issues/77

By default the template policies are mapped to TSC 2017 criteria using satisfies front matter. Because the logic in comply is to mark controls as satisfied based on this front matter a newly initialized set of templates show all controls as satisfied.

This is confusing to new users as it makes tracking the work to implement the compliance program less obvious and there's no mechanism to manually mark controls/policies as satisfied.

This PR adds a new boolean field to the Document and Procedure structs for Live to determine if a document/procedure is live and in-place in an organizations control environment. I've also updated the ControlsSatisfied() func to only mark controls as satisfied if the document/procedure has this new field set to true.

To help guide users on the workflow I've also updated the default README to include a section about satisfying controls that describes setting live: true once the policy is implemented. To make it even more clear I updated the template files that are already mapped to criteria as having live: false added to the front matter to ensure new users have an example of the right front matter.

joepurdy commented 4 years ago

@jmccarthy 👋

Sorry to bug you direct so close to the holidays. Feel free to ignore and I'll follow-up in the new year. I like to wait a week or so before mentioning project maintainers about PRs. I couldn't find much in the way of contributing guidelines for Comply aside from https://comply.strongdm.com/contribute/ which doesn't go into too much detail on what your desired process is for new PRs.

Let me know if you have any Qs about the PR or the related issue. Happy to discuss further and make adjustments. Also if adding an additional field to the structs doesn't seem reasonable there's alternate ideas I have in mind.

The primary concern I have with this implementation is that every Comply site running in the wild would lack live: true in their front matter and when/if this PR landed in a future release it would be a breaking change since the zero value will be false.

In other words, by not adding live: true to existing policies folks would have their controls start showing unsatisfied.

That said, breaking changes happen and life goes on. If that's not a risk that seems reasonable the alternative would be to default the template policies and narratives to leave the satisfies front matter commented out much in the way I suggested here: https://github.com/strongdm/comply/issues/77#issuecomment-565884444

That effectively accomplishes the same outcome while only impacting newly generated content.

Looking forward to your thoughts!

masonhensley commented 4 years ago

@joepurdy (not a maintainer) but I've made a contribution in the past. The maintainers are super nice!

I think they hang out in this slack group that you can join if you need to have a convo with them about breaking changes ... link from readme.md

joepurdy commented 4 years ago

Thanks @masonhensley! I've been hanging around the Slack workspace for some time. Just didn't want to be obnoxious with a "Hey! Look at my PR!" message for something low priority and more on the quality of life side of things.

I'll raise it later today as something to review and discuss. After sitting with it I think this approach isn't great. The breaking change aspect is no good and changing the default templates seems better given that only impacts newly initialized comply projects.

joepurdy commented 4 years ago

Closing this PR as unmerged. I believe https://github.com/strongdm/comply/pull/85 is a better approach as it is backwards compatible. Adding new frontmatter would definitely cause existing Comply backed projects to start reporting no controls satisfied when the maintainers of those projects fail to pickup the config change.

As the issue in #77 is really an issue only new users run into it makes more sense to fix it in a manner that only affects newly generated projects.