Skip to content

Conversation

@jakebarkerHO
Copy link

@jakebarkerHO jakebarkerHO commented Apr 14, 2025

Merging

This is not ready to be merged. There are some potentially breaking module input changes to discuss.

Changes

  • added aws:SourceAccount condition to trust policies
  • added AWS KMS encryption to CloudWatch log groups
  • added AWS KMS encryption
  • added alias to AWS KMS key
  • added input for AWS KMS key policy
  • set KMS key rotation to default to true
  • fixed filter bug with s3 bucket
  • further limited various IAM policies
  • enabled delete protection on load balancers
  • enabled access logging for load balancers
  • encrypted network firewall and firewall policies
  • added various data blocks to facilitate changes
  • ran terraform fmt

General feedback

  1. Identify what meets the bar for a module and review current modules. Some modules are a single resource block and may not be adding great value. There could be a process or a flowchart to assist with this. Example - KMS was a single resource block. Even with the additions in this PR, its likely that the module input (with the KMS key policy) will be larger than the module.
  2. Review consistency across modules. Resources and directories are configured differently across modules. Example - IAM policies are written in raw json, jsonencode, and hcl.
  3. Be prescriptive with HO policies where appropriate. If your security policy mandates gathering logs from load balancers, make it so in code.
  4. Explore implementing versioning on modules. Some of the changes in this PR will be breaking changes and you'll need a way to manage this,
  5. Consider readmes for modules.
  6. Review existing security tooling. checkov was able to pick up a lot of misconfiguration in the repo.
  7. Use terraform fmt. Many of the change in this PR are from running terraform fmt. Consider validating the fmt as part of the checks. This will help reduce the amount of noise in each PR.
  8. Build modules for common resources. Theres a real opportunity to build a library of easy-to-use compliant modules for common resources. It could be a real asset to the platform team and tenants ("core cloud approved module").

Common Findings

Restricting service calls

Some role trust policies for AWS services were missing restrictions on where the role can be assumed from. There are a few ways to solve this, but one of the easiest and most consistent is to use the aws:SourceAccount condition key.

condition {
      test     = "StringEquals"
      variable = "aws:SourceAccount"
      values = [
        data.aws_caller_identity.current.account_id
      ]
    }

AWS KMS keys

Encryption with AWS KMS customer managed keys was missing from a range of resources. Internal modules are a great opportunity to be prescriptive and build resources that conform with internal policies.

Other considerations

@samiwelthomasHO - I have added some commented changes to consider in some of the code (grep consider). These are regarded as good practices ... but may be in friction with a design decision. I dont have the context of those decisions. Example - dropping HTTP headers on load balancers.

Consider dropping providers from inside of modules (this may be incompatible with how terragrunt works). Once a module, with a provider, has been added it becomes very difficult to remove the resources. It can also limit the ability to run some arguments, like count and for_each. HashiCorp has released some guidance on providers in shared modules: https://developer.hashicorp.com/terraform/language/modules/develop/providers#legacy-shared-modules-with-provider-configurations.

@jakebarkerHO jakebarkerHO self-assigned this Apr 14, 2025
@jakebarkerHO
Copy link
Author

Will rebase tomorrow with my other GPG key 🙄

@jakebarkerHO
Copy link
Author

Rebased with gpg key.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants