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

Follow up: CIS Compliance guide fixes #441

Merged
merged 19 commits into from
Jul 14, 2021
Merged

Conversation

ina-stoyanova
Copy link
Contributor

@ina-stoyanova ina-stoyanova commented Jun 16, 2021

This PR is a follow up from the previous CIS Compliance guide PR to address all the NITs and additional feedback. This should help us to manage the changes better and see them easier (the last one was getting too big).

The NITs are taken from comments in this PR and translated into these checkboxes below:

Cleanup tasks:

  • Change accounts declaration from accounts = {x,y,z} to accounts = jsondecode(file("accounts.json")) See comment
  • Update formatting of for-loops
  • Update local.accounts["stage"] to local.accounts.stage to be consistent across guides (e.g. the LZ guide) See comment
  • Review references of accounts such as in this comment
  • Remove specific regions mentioned - e.g. us-east-1, and replace with a generic placeholder. See comment
  • Replace acme-* specific placeholders with potentially <RESOURCE_ACCOUNT> like suggested here and here
  • Replace name_prefix = "stage-logs" with something unique like name_prefix = "<SOME UNIQUE IDENTIFIER>-logs" - see this comment
  • Replace specific ARNs with placeholders - e.g. kms_cmk_arn = "arn:aws:kms:us-east-1:${local.accounts.shared}:alias/ami-encryption" should be more generic - See this comment
  • Remove force_destroy_* params - we don't need those in a production guide - See this comment
  • Set defaults for max_session_duration_machine_users and max_session_duration_human_users - See this comment

Readability tasks:
More generic feedback to address (⚠️ Consider a separate PR for these if they're not small and quick changes)

  • A "very large number of sub-sections below deployment walkthrough - See this comment - potentially group into sections
  • Review the section "The Gruntwork solution" section a little hard to follow" and follow the suggestions by Jim - See comment here
  • Introduce a common.hcl to hold hard-coded values that are repeated across the code examples - See this comment
  • Call out explicitly that we have baselines for all types of accounts - root, logs, app and security - See this comment
  • Remove any mention of the infra-modules - See this comment
  • Transform the message around Security Hub showing 1.2 compliance in AWS console to be a callout format so it's easy for customers to spot - See this comment

Potential bugs: (⚠️ Consider a separate PR for these if they're not small and quick changes)

  • See if the right baseline is used in the terraform source on lines 978-979 - See this comment
  • Does the code comment on lines 1093-1094 make sense - See this comment
  • Don't allow dev permissions in the root account - See this comment
  • Do we need the auto-deploy permissions in the root baseline example? Remove it, as we shouldn't be needing this access - See this comment
  • We should remove the dev_services in the logs account example - See this comment
  • Review the logs account example - See this comment & this one
  • Remove auto-scaling configuration from logs account - do we need any? - See this comment
  • Review Security account example code - See this comment

Additional notes:

  • To make sure this PR is quick and easy, some of these tasks might need to be split out into separate PRs too
  • To make sure all the old PR comments are met - please review them one by one and assess if they still make sense to follow

@netlify
Copy link

netlify bot commented Jun 16, 2021

✔️ Deploy Preview for keen-clarke-470db9 ready!

🔨 Explore the source changes: 11a6d7e

🔍 Inspect the deploy log: https://app.netlify.com/sites/keen-clarke-470db9/deploys/60edb2374c7cbc0008ce485b

😎 Browse the preview: https://deploy-preview-441--keen-clarke-470db9.netlify.app

@rhoboat
Copy link
Contributor

rhoboat commented Jun 16, 2021

Holy cannoli, that's a lot of stuff in the checklist. Can I help, since I've made a lot of these suggestions, and I've already made these changes in the LZ guide?

@rhoboat rhoboat mentioned this pull request Jun 16, 2021
7 tasks
@rhoboat
Copy link
Contributor

rhoboat commented Jun 16, 2021

I took care of a few of these in #442 if that helps!

@rhoboat rhoboat mentioned this pull request Jun 16, 2021
@ina-stoyanova ina-stoyanova marked this pull request as draft June 17, 2021 14:12
@ina-stoyanova ina-stoyanova marked this pull request as ready for review July 2, 2021 10:57
@ina-stoyanova
Copy link
Contributor Author

Ok, so I would like to merge this asap, otherwise, this is getting too big again. Is there any big blocker for this PR that is left for us to fix?

@ina-stoyanova
Copy link
Contributor Author

Wohoo! Thanks for all the attention and feedback, @marinalimeira! Last round done! I hope to get this merged as soon as we can, so I guess if any other changes come up, I'm sure we can do separately. Thoughts?

Copy link
Contributor

@yorinasub17 yorinasub17 left a comment

Choose a reason for hiding this comment

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

LGTM!

@ina-stoyanova
Copy link
Contributor Author

Yay! Thanks, Yori ❤️

@ina-stoyanova ina-stoyanova merged commit a0c7d61 into master Jul 14, 2021
@ina-stoyanova ina-stoyanova deleted the cis-compliance-guide-fixes branch July 14, 2021 12:50
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.

4 participants