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

Update security baseline components and add encrypted/custom AMI support #203

Closed
wants to merge 37 commits into from

Conversation

justicel
Copy link

@justicel justicel commented Oct 28, 2021

Background

The current release of the module neither passes checkov security scans, nor does it pass Disney security baseline requirements. This merge request fixes both of those at the same time.

Additionally it also adds the ability to specify a custom AMI (search) as well as encrypted AMIs if an AMI KMS key is specified.

How Has This Been Tested

The testing was done by spinning up a TFE environment with the previous release of the module from the main branch and applying. Then the diff set here for the merge was applied to the environment to make sure no resources were munged or destroyed. Finally the existing EC2 instances for the active-active pair were terminated and new spun up in their place and they automatically stood up a working cluster.

Test Configuration

  • Terraform Version: 0.15.1
  • Any additional relevant variables: No

This PR makes me feel

Secure!

@justicel justicel requested a review from a team as a code owner October 28, 2021 23:03
@hashicorp-cla
Copy link

CLA assistant check

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Learn more about why HashiCorp requires a CLA and what the CLA includes


Justice London seems not to be a GitHub user.
You need a GitHub account to be able to sign the CLA. If you already have a GitHub account, please add the email address used for this commit to your account.

Have you signed the CLA already but the status is still pending? Recheck it.

Copy link
Contributor

@aaron-lane aaron-lane left a comment

Choose a reason for hiding this comment

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

Hi @justicel! Thank you for your interest in the project 🎉

Improvements to the security of the module infrastructure are much appreciated, but please be aware that we do not incorporate checkov security scans or the Disney security baseline in our development process at this time, so there is no guarantee that these scans will continue to pass in the future.

More broadly, all of the terraform-*-terraform-enterprise modules are still in active development, so there will likely be additional breaking changes to the module that may invalidate some of these changes as we finalize the design. Also, please be aware that the modules are not supported for any use case at this point.

With all of that said, I have some requested changes below 🥳

.pre-commit-config.yaml Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
modules/vm/main.tf Outdated Show resolved Hide resolved
versions.tf Outdated Show resolved Hide resolved
@@ -10,6 +10,13 @@ resource "aws_instance" "proxy" {
aws_security_group.proxy.id,
]

monitoring = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert the changes under tests; we don't need to harden the supporting test infrastructure.

Copy link
Author

Choose a reason for hiding this comment

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

I removed the monitoring and EBS optimized lines, but encryption should honestly be there as it will flag most any security system monitoring the accounts the test infrastructure is deployed to... also the test should try to match what would be deployed to production generally.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the consideration, but realistic production infrastructure is not our primary concern for the tests at this point. Please revert all changes under /test.

Copy link
Author

Choose a reason for hiding this comment

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

Indeed, which is why I reverted the monitoring and ebs optimized flags. Disabling encrypted status for the EBS volumes though will be a monitoring triggering event so if you want that to be left disabled in this case, unfortunately I'll have to withdraw this merge request and fork the module.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you help me understand the specific concern about the configuration of the test module? It is used as part of the CI process for this module to verify its correctness. It should not be used for any deployments outside of that context, including manual deployments, so it is not clear to me in what circumstances it would trigger a monitoring event. Regardless, if this is a hard requirement for your organization then please do fork the module and modify it as you require.

@aaron-lane
Copy link
Contributor

aaron-lane commented Nov 2, 2021

/test all destroy=false

❌ Terraform Public Active/Active Test Report

🔗 Action Summary Page

  • ✅ Terraform Init

  • ✅ Terraform Validate

  • ❌ Terraform Apply

  • ❌ Run k6 Smoke Test

✅ Terraform Private Active/Active Test Report

🔗 Action Summary Page

  • ✅ Terraform Init

  • ✅ Terraform Validate

  • ✅ Terraform Apply

  • ✅ Run k6 Smoke Test

❌ Terraform Private TCP Active/Active Test Report

🔗 Action Summary Page

  • ✅ Terraform Init

  • ✅ Terraform Validate

  • ✅ Terraform Apply

  • ❌ Run k6 Smoke Test

@aaron-lane
Copy link
Contributor

aaron-lane commented Nov 2, 2021

/destroy private-active-active

✅ Terraform Private Active/Active Destruction Report

🔗 Action Summary Page

  • ✅ Terraform Init

  • ✅ Terraform Destroy

@aaron-lane
Copy link
Contributor

aaron-lane commented Nov 2, 2021

/test private-tcp-active-active destroy=false

✅ Terraform Private TCP Active/Active Test Report

🔗 Action Summary Page

  • ✅ Terraform Init

  • ✅ Terraform Validate

  • ✅ Terraform Apply

  • ✅ Run k6 Smoke Test

tests/public-active-active/main.tf Outdated Show resolved Hide resolved
@aaron-lane
Copy link
Contributor

aaron-lane commented Nov 2, 2021

/destroy private-tcp-active-active

✅ Terraform Private TCP Active/Active Destruction Report

🔗 Action Summary Page

  • ✅ Terraform Init

  • ✅ Terraform Destroy

@aaron-lane
Copy link
Contributor

aaron-lane commented Nov 3, 2021

/test all

✅ Terraform Public Active/Active Test Report

🔗 Action Summary Page

  • ✅ Terraform Init

  • ✅ Terraform Validate

  • ✅ Terraform Apply

  • ✅ Run k6 Smoke Test

  • ✅ Terraform Destroy

✅ Terraform Private TCP Active/Active Test Report

🔗 Action Summary Page

  • ✅ Terraform Init

  • ✅ Terraform Validate

  • ✅ Terraform Apply

  • ✅ Run k6 Smoke Test

  • ✅ Terraform Destroy

❌ Terraform Private Active/Active Test Report

🔗 Action Summary Page

  • ✅ Terraform Init

  • ✅ Terraform Validate

  • ✅ Terraform Apply

  • ❌ Run k6 Smoke Test

  • ✅ Terraform Destroy

@aaron-lane
Copy link
Contributor

aaron-lane commented Nov 4, 2021

/test private-active-active destroy=false

✅ Terraform Private Active/Active Test Report

🔗 Action Summary Page

  • ✅ Terraform Init

  • ✅ Terraform Validate

  • ✅ Terraform Apply

  • ✅ Run k6 Smoke Test

@aaron-lane
Copy link
Contributor

aaron-lane commented Nov 4, 2021

/destroy private-active-active

✅ Terraform Private Active/Active Destruction Report

🔗 Action Summary Page

  • ✅ Terraform Init

  • ✅ Terraform Destroy

Copy link
Contributor

@aaron-lane aaron-lane left a comment

Choose a reason for hiding this comment

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

@justicel the changes look good and the tests passed. Looks like you just need to sign the CLA and then we can merge this branch.

@justicel
Copy link
Author

justicel commented Nov 4, 2021

Sounds good. Thanks for reviewing. I just have to get it through our legal/OSS process which should happen in a couple days.

@aaron-lane
Copy link
Contributor

@justicel have you had an opportunity to review the CLA?

@aaron-lane aaron-lane requested review from a team and aaron-lane and removed request for aaron-lane February 25, 2022 15:21
@nikolasrieble
Copy link
Contributor

@justicel Thank you for your contribution! As there has been no activity for a while on this PR, I shall close it. If you find your original concern and motivation to still be relevant, please consider creating an issue on which we can discuss the next steps.

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