[IAC-1753] Update CIS compliance guide #426

ina-stoyanova · 2021-05-24T17:30:43Z

Add (if missing) and review the below sections:
-> we can copy& paste directly from the LZ guide on the website, but be careful of links, code & any information that needs changing.

netlify · 2021-05-24T17:31:49Z

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

🔨 Explore the source changes: 3473f35

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

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

_posts/2019-10-17-how-to-achieve-cis-benchmark-compliance.adoc

…unt passwords & securing IAM users in child acocunts

rhoboat · 2021-06-15T18:28:16Z

_posts/2019-10-17-how-to-achieve-cis-benchmark-compliance.adoc

+  accounts = {
+    logs     = "409800740445"
+    security = "673123100581"
+    shared   = "384759303421"
+    dev      = "293847503945"
+    stage    = "384924092834"
+    prod     = "784260063686"
+    root     = "216044045972"
+  }


So, in the LZ guide, I've moved these into the root accounts.json that gets included in the root common.hcl as accounts = jsondecode(file("accounts.json")). Then every terragrunt.hcl that needs it does

locals { # A local for more convenient access to the accounts map. accounts = local.common_vars.locals.accounts ... }

It's a nit, but wonder if it's something you want to follow here.

+1. Seems worth doing in a follow-up PR.

BTW, this comment applies to all the accounts = { ... } in all the terragrunt.hcl files in this guide.

rhoboat · 2021-06-15T18:29:54Z

_posts/2019-10-17-how-to-achieve-cis-benchmark-compliance.adoc

+  # All of the other accounts send logs to this account.
+  config_linked_accounts = [
+  for name, id in local.accounts :
+  id if name != "logs"


NIT: In my view, this is a line under the for loop, so it seems like it should be further indented. (We need to update the ref arch generation to reflect this too.)

rhoboat · 2021-06-15T18:30:02Z

_posts/2019-10-17-how-to-achieve-cis-benchmark-compliance.adoc

+  cloudtrail_allow_kms_describe_key_to_external_aws_accounts = true
+  cloudtrail_external_aws_account_ids_with_write_access = [
+  for name, id in local.accounts :
+  id if name != "logs"


NIT: In my view, this is a line under the for loop, so it seems like it should be further indented. (We need to update the ref arch generation to reflect this too.)

rhoboat · 2021-06-15T18:30:51Z

_posts/2019-10-17-how-to-achieve-cis-benchmark-compliance.adoc

+    logs     = "409800740445"
+    security = "673123100581"
+    shared   = "384759303421"
+    dev      = "293847503945"
+    stage    = "384924092834"
+    prod     = "784260063686"
+    root     = "216044045972"
+  }


NIT: See comment in the logs section on DRYing up this part.

_posts/2019-10-17-how-to-achieve-cis-benchmark-compliance.adoc

rhoboat · 2021-06-15T18:31:40Z

_posts/2019-10-17-how-to-achieve-cis-benchmark-compliance.adoc

+    logs     = "409800740445"
+    security = "673123100581"
+    shared   = "384759303421"
+    dev      = "293847503945"
+    stage    = "384924092834"
+    prod     = "784260063686"
+    root     = "216044045972"
+  }


NIT: See comment in logs section about DRYing this up.

_posts/2019-10-17-how-to-achieve-cis-benchmark-compliance.adoc

rhoboat · 2021-06-15T18:32:54Z

_posts/2019-10-17-how-to-achieve-cis-benchmark-compliance.adoc

+  accounts = {
+    root = "216044045972"
+  }


NIT: Perhaps this ID could be stored as a root key in the accounts.json that I mention in the comment under the logs section below.

_posts/2019-10-17-how-to-achieve-cis-benchmark-compliance.adoc

rhoboat · 2021-06-15T18:35:43Z

_posts/2019-10-17-how-to-achieve-cis-benchmark-compliance.adoc

+  # Both buckets are created in the logs account by account-baseline-root
+  config_s3_bucket_name     = "acme-config-bucket-logs"
+  cloudtrail_s3_bucket_name = "acme-cloudtrail-logs"
+
+  # The Cloudtrail KMS Key is deployed at the logs account but it's value is an output from the root account.
+  cloudtrail_kms_key_arn = "arn:aws:kms:us-east-1:216044045972:alias/cloudtrail-acme"


NIT: Is it worth replacing these hardcoded values with placeholder values, with a comment above them telling them how to get them? Like:

# Use the S3 bucket and KMS key that were already created in the logs account by account-baseline-root cloudtrail_s3_bucket_name = "<CLOUDTRAIL_BUCKET_NAME>" cloudtrail_kms_key_arn = "<CLOUDTRAIL_KMS_KEY_ARN>"

I think what you have currently works fine. In either case, it's up to the customer whether they want to store the values in locals, and if you have them set it there, then it makes no difference that you reference the local here.

I made this modification in #442, so we store the bucket names and arns in common.hcl. Updating the LZ guide, too, in #443.

rhoboat · 2021-06-15T18:37:12Z

_posts/2019-10-17-how-to-achieve-cis-benchmark-compliance.adoc

+  # Both buckets are created in the logs account by account-baseline-root
+  config_s3_bucket_name     = "acme-config-bucket-logs"
+  cloudtrail_s3_bucket_name = "acme-cloudtrail-logs"
+
+  # The Cloudtrail KMS Key is deployed at the logs account but it's value is an output from the root account.
+  cloudtrail_kms_key_arn = "arn:aws:kms:us-east-1:409800740445:alias/cloudtrail-acme"


NIT: Same nit as below on replacing hardcoded values with placeholders.

NIT: Or, alternatively, extract into the same common.hcl.

rhoboat · 2021-06-15T18:38:17Z

_posts/2019-10-17-how-to-achieve-cis-benchmark-compliance.adoc

+  # Both buckets are created in the logs account by account-baseline-root
+  config_s3_bucket_name     = "acme-config-bucket-logs"
+  cloudtrail_s3_bucket_name = "acme-cloudtrail-logs"
+
+  # The Cloudtrail KMS Key is deployed at the logs account but it's value is an output from the root account.
+  cloudtrail_kms_key_arn = "arn:aws:kms:us-east-1:409800740445:alias/cloudtrail-acme"


NIT: Same nit about hardcoded values.

rhoboat · 2021-06-15T18:42:54Z

_posts/2019-10-17-how-to-achieve-cis-benchmark-compliance.adoc


  # Prefix all resources with this name
-  name_prefix = "<COMPANY_NAME>-stage"
+  name_prefix = "stage-logs"


Hm, this one I'm not sure about. Setting the name_prefix for the environment + account is not what we're currently doing in the ref arch deploys. See the LZ guide for how we set name_prefix (in common.hcl, usually as the company name, and then in the root terragrunt.hcl, so that no downstream terragrunt.hcls need to set it). It makes me wonder if our ref arch is setting the name_prefix a little too generic? Pinging @bwhaley for another opinion.

It does need to be something unique as it will ultimately be used to create the S3 bucket, which is a global namespace among all AWS users. Identifying a good rule of thumb is tricky because it must be both (a) short enough to not run in to length constraints, and (b) globally unique. <COMPANY_NAME> is usually unique enough, but runs the risk of being too long. Many customers have chosen a prefix that is an acronym or shortened identifiable version of the company name, like phc or mf.

I think we ought to share this context in the comment on the preceding line, and go with something like:

name_prefix = "<SOME UNIQUE IDENTIFIER>-logs"

Thanks Ben. Seems like we need to update the LZ guide and the Ref Arch code, too. Looking at the terraform-aws-architecture-catalog, we set name_prefix in only one place, which gets passed around from the root terragrunt (and actually it's duplicated here, which we can simplify). That means it's not unique for the S3 buckets, right?

I feel like I must be missing something. It can't be that we use the same name_prefix everywhere. Maybe it does get overridden in each account's terragrunt, but I'm not seeing it.

@bwhaley Thoughts on this?

rhoboat · 2021-06-15T18:44:11Z

_posts/2019-10-17-how-to-achieve-cis-benchmark-compliance.adoc

+  }
+  kms_grants = {
+    ami_encryption_key = {
+      kms_cmk_arn       = "arn:aws:kms:us-east-1:${local.accounts.shared}:alias/ami-encryption"


NIT: I might remove the reference to us-east-1 here, using local.aws_region or some such, instead.
Or maybe this whole arn should be a placeholder text.

rhoboat

I just left a few comments after looking more in depth at the terragrunt code.

brikis98

OK, just did a review pass. I found a bunch of NITs, which you can safely ignore for now, and perhaps implement in follow-up PRs. I also found a few places that look like legit bugs. These should probably be fixed ASAP. That said, I'll still mark this with a "ship it," as this PR has been open for ages, is still an improvement over the current guide, and even the "ASAP" fixes can still be done in follow-up PRs.

One more NIT: There are a very large number of sub-sections below deployment walkthrough:

That might scare users off... Is it worth grouping some of those into slightly larger sections?

brikis98 · 2021-06-16T14:37:04Z

_posts/2019-10-17-how-to-achieve-cis-benchmark-compliance.adoc


 [[gruntwork_solution]]
 === The Gruntwork solution
 Gruntwork offers battle-tested infrastructure as code modules to help you create production grade infrastructure in a
-fraction of the time it would take to develop from scratch. Each of the code modules are "compliance-ready"; they are
+fraction of the time it would take to develop from scratch. Each of the core modules are "compliance-ready"; they are


NIT: I found this "The Gruntwork solution" section a little hard to follow, especially with the mix of terms like modules, services, architectures, wrapper modules, etc.

I'd propose the following structure for a follow-up PR:

We should define, in bullet points, the (a) module catalog, (b) service catalog, (c) architecture catalog. These definitions should be short, and point to the full blog post for more details.

Next, we should explain that we offer a standard service catalog and a CIS service catalog. The former is the baseline: it has all the core services ready for most usage. The latter has the subset of services that must be configured in a specific way to be compliant with the CIS benchmark.

To deploy a CIS compliant infra, you first look in the CIS service catalog, and if the service you need is there, use it; otherwise, use the service from the standard service catalog; if that doesn't have the service you need, you can create your own service by combining modules from the module catalog.

Alternatively, you can get an out-of-the-box CIS compliant architecture from our arch catalog (link to Ref Arch page) in one day.

The key difference from the current structure is:

This section can be shorter.

We should focus on CIS Service Catalog instead of "wrapper modules." The latter is technically accurate, but not as effective of a way to present the concept to customers as Service Catalog.

That said, I don't feel strongly about this, so feel free to leave as is or tweak or whatever else you decide.

brikis98 · 2021-06-16T14:48:52Z

_posts/2019-10-17-how-to-achieve-cis-benchmark-compliance.adoc

+  config_s3_bucket_name = local.config_s3_bucket_name
+
+  # Do not allow objects in the Config S3 bucket to be forcefully removed during destroy operations.
+  config_force_destroy = false


NIT: isn't false the default for all the xxx_force_destroy params? If so, perhaps we don't need to include them here?

This same comment applies to the same xxx_force_destroy params in all the terragrunt.hcl files below, too.

brikis98 · 2021-06-16T14:49:39Z

_posts/2019-10-17-how-to-achieve-cis-benchmark-compliance.adoc

-provider "aws" {
-  region = var.aws_region
+terraform {
+  source = "git::[email protected]/gruntwork-io/terraform-aws-cis-service-catalog//modules/landingzone/account-baseline-app?ref=v0.22.0"


Shouldn't this be account-baseline-root?

brikis98 · 2021-06-16T14:50:25Z

_posts/2019-10-17-how-to-achieve-cis-benchmark-compliance.adoc

+  # By granting access to the root ARN of the Security account in each of the roles below,
+  # we allow administrators to further delegate access to other IAM entities


What roles below? What security account? It doesn't exist yet... Copy/paste issue?

brikis98 · 2021-06-16T14:50:43Z

_posts/2019-10-17-how-to-achieve-cis-benchmark-compliance.adoc

+  # Assuming the developers role will grant access to these services.
+  dev_permitted_services = [
+    "ec2",
+    "ecs",
+    "lambda",
+    "rds",
+    "elasticache",
+    "route53",
+  ]


There should be no devs in the root account and no need to grant these permissions

brikis98 · 2021-06-16T15:01:02Z

_posts/2019-10-17-how-to-achieve-cis-benchmark-compliance.adoc

+[.exceptional]
+IMPORTANT: You must be a [js-subscribe-cta]#Gruntwork subscriber# to access `terraform-aws-cis-service-catalog`.
+
+Next, we'll configure a security baseline for the root account that is responsible for creating all the child accounts.


NIT: we should call out explicitly somewhere that:

We have security baselines for root, security, and app accounts.

These security baselines implement most of the requirements from CIS: e.g., CloudTrail, AWS Config, IAM roles, etc.

So 99% of the work is configuring a security baseline for each account and running apply on it.

brikis98 · 2021-06-16T15:01:38Z

_posts/2019-10-17-how-to-achieve-cis-benchmark-compliance.adoc

+  accounts = {
+    logs     = "409800740445"
+    security = "673123100581"
+    shared   = "384759303421"
+    dev      = "293847503945"
+    stage    = "384924092834"
+    prod     = "784260063686"
+    root     = "216044045972"
+  }


BTW, this comment applies to all the accounts = { ... } in all the terragrunt.hcl files in this guide.

brikis98 · 2021-06-16T15:02:50Z

_posts/2019-10-17-how-to-achieve-cis-benchmark-compliance.adoc

+  # A list of account root ARNs that should be able to assume the auto deploy role.
+  allow_auto_deploy_from_other_account_arns = [
+    # External CI/CD systems may use an IAM user in the security account to perform deployments.
+    "arn:aws:iam::${local.accounts.security}:root",
+
+    # The shared account contains automation and infrastructure tools, such as CI/CD systems.
+    "arn:aws:iam::${local.accounts.shared}:root",
+  ]
+  auto_deploy_permissions = [
+    "iam:GetRole",
+    "iam:GetRolePolicy",
+  ]


Again, what auto deploy do we do here? Also, you're allowing access from the security account... But this is the security account, so this doesn't quite make sense.

brikis98 · 2021-06-16T15:03:35Z

_posts/2019-10-17-how-to-achieve-cis-benchmark-compliance.adoc

+You can re-use the `account-baseline-app` module you created earlier in your `infrastructure-modules` repo for all of
+these child accounts; this module can be used interchangeably between app accounts and log accounts as they deploy most
+of the same resources.


This should be talking about the account-baseline-app from the CIS Service Catalog, not anything with infra-modules.

brikis98 · 2021-06-16T15:05:32Z

_posts/2019-10-17-how-to-achieve-cis-benchmark-compliance.adoc

+Hub will show a low security score for the CIS AWS Foundations Benchmark v1.2.0. This is due to AWS limitations on
+checking compliance standards for cross-region/cross-account rules. This does not indicate that the accounts are not in
+compliance; it is a failure of the AWS audit tool. Note also that the accounts are configured for the latest version of
+the benchmark, v1.3.0; the AWS Security Hub does not support this version at the current time.


NIT: might be worth putting this in a callout to make it more visible. Lots of customers stumble on this exact point.

ina-stoyanova · 2021-06-16T15:36:09Z

Thanks @brikis98 @bwhaley & @rhoboat for the thorough reviews!

I think here we should follow your suggestion:

OK, just did a review pass. I found a bunch of NITs, which you can safely ignore for now, and perhaps implement in follow-up PRs. I also found a few places that look like legit bugs. These should probably be fixed ASAP. That said, I'll still mark this with a "ship it," as this PR has been open for ages, is still an improvement over the current guide, and even the "ASAP" fixes can still be done in follow-up PRs.

In this morning's EU team catchup we discussed exactly that - there are many valuable suggestions in the last few rounds of review, but follow up PRs will be easier to manage, and with this state, we still add value to the customers. I'll go ahead and merge it if everyone's ok with this.

ina-stoyanova · 2021-06-16T15:39:03Z

Oops - before posting the last comment I pressed the wrong button and closed the PR by mistake! Apologies

Add "Create root account section"

bcee964

ina-stoyanova requested review from eak12913, josh-padnick, oredavids and tsmit291 as code owners May 24, 2021 17:30

ina-stoyanova changed the title ~~IAC-1753 - Update CIS compliance guide~~ [IAC-1753] WIP - Update CIS compliance guide May 24, 2021

robmorgan and others added 7 commits May 25, 2021 16:32

start adding my updates to the cis guide

a465043

rewrite more content

b28af95

finalize gruntwork solution section

d8c377a

cis guide updates

832573d

improve line height

6c453c3

Update guide to not have references for CIS 1.3 scattered

327b86c

Adding account-baseline-root to guide

6471a7b

ina-stoyanova commented May 26, 2021

View reviewed changes

_posts/2019-10-17-how-to-achieve-cis-benchmark-compliance.adoc Outdated Show resolved Hide resolved

Updating reference to be service-catalog

f1a2a11