-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
feat(EC2): Add new check for security group port restrictions #4594
feat(EC2): Add new check for security group port restrictions #4594
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4594 +/- ##
==========================================
+ Coverage 89.07% 89.15% +0.08%
==========================================
Files 918 921 +3
Lines 28088 28156 +68
==========================================
+ Hits 25019 25103 +84
+ Misses 3069 3053 -16 ☔ View full report in Codecov by Sentry. |
class ec2_securitygroup_allow_ingress_from_internet_to_high_risk_tcp_ports(Check): | ||
def execute(self): | ||
findings = [] | ||
check_ports = [25, 110, 135, 143, 445, 3000, 4333, 5000, 5500, 8080, 8088] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you make this list configurable please
and vpc_client.vpcs[security_group.vpc_id].in_use | ||
and len(security_group.network_interfaces) > 0 | ||
): | ||
report = Check_Report_AWS(self.metadata()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Give a finding for each port, change the status extended to reflect what port is affected.
…k-for-security-group-port-restrictions
…k-for-security-group-port-restrictions
…he findings to one for each port instead of one for each security group
class ec2_securitygroup_allow_ingress_from_internet_to_high_risk_tcp_ports(Check): | ||
def execute(self): | ||
findings = [] | ||
for security_group in ec2_client.security_groups: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a recent PR ec2_client.security_groups
have been changed into a dict instead of a list, please merge master in this branch to apply this changes and make the appropriate changes to make this work according to this changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job! 👏🏼
Context
To improve network security and comply with best practices, we are adding a new check within the EC2 framework. This check ensures that security groups do not allow unrestricted access to ports with high risk. Some of the ports that SH consider high risk are already implemented by other checks so only the ones in the following list will be taken care in this check ( 25(SMTP), 110(POP3), 135(RCP), 143(IMAP), 445(CIFS), 3000(Go, Node.js, and Ruby web developemnt frameworks), 4333(ahsp), 5000(Python web development frameworks), 5500(fcp-addr-srvr1), 8080(proxy), 8088(legacy HTTP port) ).
Description
I added
ec2_securitygroup_allow_ingress_from_internet_to_high_risk_tcp_ports
check with his respective unit test.License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.