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

major: remove allow all private ip access #141

Conversation

venkatamutyala
Copy link
Contributor

No description provided.

@venkatamutyala venkatamutyala merged commit 27a8e58 into renovate/cloudposse-eks-node-group-aws-3.x Aug 15, 2024
3 of 5 checks passed
@venkatamutyala venkatamutyala deleted the venkatamutyala-patch-1 branch August 15, 2024 07:08
Copy link

Copy link
Contributor

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
🧪 No relevant tests
🔒 Security concerns

No security concerns. This PR actually improves security by removing an overly permissive security group rule that allowed all traffic from private IP ranges (10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16) on all ports. This change reduces the attack surface and follows the principle of least privilege.

⚡ Key issues to review

Security Improvement
Removal of overly permissive security group rule that allowed all traffic from private IP ranges

Copy link
Contributor

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Security
Add specific ingress rules to replace the removed broad private IP access

Consider adding more specific ingress rules to replace the removed broad private IP
access. This will improve security by limiting access to only necessary ports and IP
ranges.

network.tf [26-30]

 resource "aws_security_group" "captain" {
   name        = "captain-sg"
   description = "captain security group"
   vpc_id      = module.vpc.vpc_id
 }
 
+resource "aws_security_group_rule" "captain_ingress_specific" {
+  type              = "ingress"
+  from_port         = 22
+  to_port           = 22
+  protocol          = "tcp"
+  cidr_blocks       = ["10.0.0.0/24"]  # Adjust this to your specific subnet
+  security_group_id = aws_security_group.captain.id
+}
+
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why:

7
Best practice
Add a description to the security group rule for better documentation

Consider adding a description to the security group rule for better documentation
and easier management.

network.tf [32-35]

 resource "aws_security_group_rule" "captain_egress_all_ipv4" {
   type              = "egress"
   from_port         = 0
   to_port           = 65535
+  description       = "Allow all outbound traffic"
 
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why:

7

venkatamutyala added a commit that referenced this pull request Aug 15, 2024
* chore(deps): update terraform cloudposse/eks-node-group/aws to v3

* docs: automated update of terraform docs

* fix: create_before_destroy = false otherwise it adds a pet_name. We may want to set this to true later (#138)

* major: migrating to ami_release_name instead of having to pass in an ami each time (#139)

* major: migrating to ami_release_name instead of having to pass in an ami each time

* docs: automated update of terraform docs

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* fix: ami release name and update aws nuke (#140)

* fix: ami release name and update aws nuke

* docs: automated update of terraform docs

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* fix: aws-nuke to exclude OSPackage

* major: remove allow all private ip access (#141)

* docs: automated update of terraform docs

* chore: run tf fmt

* docs: automated update of terraform docs

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Venkat <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant