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

Make allow_all_egress a variable #126

Merged

Conversation

dlacosteGFM
Copy link
Contributor

What changes in this PR?

  • Default change is nothing (with this PR applied, nobody would have to change anything)
  • Makes a new parameter allow_all_egress which defaults to false
  • When creating the security group for the EFS volume, this line makes the security-group have an "allow egress to 0.0.0.0/0" rule entry. This PR makes that a configurable parameter instead

Why make this change?

  • EFS doesn't actually do egress, so this really makes no impact difference at all
  • ...but during a security audit we have a dangling "why do you allow egress to 0.0.0.0/0 on this?" question with no really good answer (so let's get rid of it as it doesn't do anything anyways)

References

  • PCI DSS 3.2.1 rule 1.1.7 - Requirement to review firewall and router rule sets every 6 months
  • PCI DSS 3.2.1 rule 1.2.1 - Restrict inbound and outbound traffic to that which is necessary for the environment

@hans-d
Copy link

hans-d commented Mar 2, 2024

/terratest

@hans-d hans-d added wip Work in Progress: Not ready for final review or merge and removed wip Work in Progress: Not ready for final review or merge labels Mar 2, 2024
@hans-d
Copy link

hans-d commented Mar 2, 2024

Can you follow up with the failed checks/? A the end of the ci-readme eg is noted wat to do.

@hans-d
Copy link

hans-d commented Mar 3, 2024

@dlacosteGFM Hi, can you update the pr so that it passes the tests? otherwise, it is likely to be closed due to staleness.

@hans-d hans-d added the stale label Mar 5, 2024
@dlacosteGFM
Copy link
Contributor Author

Sorry, missed the updated: looking at the output now (will update as required to make it pass)

This change does not alter existing behavior, but enables a user to
specify that the created security group should _not_ have an open egress
rule attached.
@dlacosteGFM dlacosteGFM force-pushed the make_allow_all_egress_a_variable branch from 78ad203 to 028583a Compare March 5, 2024 01:15
@dlacosteGFM
Copy link
Contributor Author

...and now that I rebased the test results are gone..... I wonder if it will let me run terratest? :)

@dlacosteGFM
Copy link
Contributor Author

/terratest

2 similar comments
@hans-d
Copy link

hans-d commented Mar 5, 2024

/terratest

@hans-d
Copy link

hans-d commented Mar 5, 2024

/terratest

@dlacosteGFM
Copy link
Contributor Author

I got it to run manually:

~/repos/personal/terraform-aws-efs/test:(1m|git@make_allow_all_egress_a_variable!)
10076 ± make                                                                                                                                                                                                                                                                             

Running tests in  ../
1..11
ok 1 check if terraform is installed
ok 2 check if terraform code needs formatting
ok 3 check if terraform modules are valid # skip Terraform no longer supports separate testing of module loading
ok 4 check if terraform modules are properly pinned
ok 5 check if terraform plugins are valid # skip Terraform no longer supports separate testing of plugins
ok 6 check if terraform providers are properly pinned
ok 7 check if terraform providers have explicit source locations for TF =>0.13
ok 8 check if terraform code is valid
ok 9 check if terraform-docs is installed
ok 10 check if terraform inputs have descriptions
ok 11 check if terraform outputs have descriptions
Running tests in  ../examples/complete
1..5
ok 1 check if terraform is installed
ok 2 check if terraform code needs formatting
ok 3 check if terraform modules are valid # skip Terraform no longer supports separate testing of module loading
ok 4 check if terraform plugins are valid # skip Terraform no longer supports separate testing of plugins
ok 5 check if terraform code is valid

@hans-d
Copy link

hans-d commented Mar 5, 2024

/terratest

@hans-d
Copy link

hans-d commented Mar 5, 2024

I got it to run manually:

Much appreciating the effort!

@hans-d hans-d enabled auto-merge (squash) March 5, 2024 16:25
@hans-d hans-d merged commit 9f7e8a3 into cloudposse:main Mar 5, 2024
11 checks passed
@hans-d
Copy link

hans-d commented Mar 5, 2024

@dlacosteGFM Thanks!

@hans-d hans-d removed the stale label Mar 5, 2024
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.

None yet

2 participants