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

chore(ec2): add EC2 Security group check to verify if at least one port is opened #3982

Closed
wants to merge 3 commits into from

Conversation

kagahd
Copy link
Contributor

@kagahd kagahd commented May 13, 2024

Context

Backport check ec2_securitygroup_allow_ingress_from_internet_to_any_port to prowler v3.

Description

Check ec2_securitygroup_allow_ingress_from_internet_to_any_port had been renamed to ec2_securitygroup_allow_ingress_from_internet_to_all_ports and an accurate check ec2_securitygroup_allow_ingress_from_internet_to_any_porthad been created in PR #3962.
This PR backports the changes to prowler v3.

License

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@kagahd kagahd requested review from a team as code owners May 13, 2024 12:52
@github-actions github-actions bot added the provider/aws Issues/PRs related with the AWS provider label May 13, 2024
@jfagoagas jfagoagas added the no-merge Please, DO NOT MERGE this PR. label May 17, 2024
@jfagoagas
Copy link
Member

Hi @kagahd to backport code from V4 to V3 we have a internal process and this PR #3962 is marked with the backport-v3 tag. Is this PR just picking the code from the PR I mentioned?

Thanks for using Prowler 🚀

@jfagoagas jfagoagas added the v3 Everything related to Prowler 3.0 label May 17, 2024
@jfagoagas jfagoagas changed the title backport v3 ec2_securitygroup_allow_ingress_from_internet_to_any_port feat(ec2): add EC2 Security group check to verify if at least one port is opened May 17, 2024
@kagahd
Copy link
Contributor Author

kagahd commented May 17, 2024

Hi @jfagoagas

Is this PR just picking the code from the PR I mentioned?

Yes, it is. In this PR I took the code from PR #3962 and adapted it to work with prowler v3.

@jfagoagas jfagoagas changed the title feat(ec2): add EC2 Security group check to verify if at least one port is opened chore(ec2): add EC2 Security group check to verify if at least one port is opened May 17, 2024
@jfagoagas
Copy link
Member

Hi @jfagoagas

Is this PR just picking the code from the PR I mentioned?

Yes, it is. In this PR I took the code from PR #3962 and adapted it to work with prowler v3.

If you don't mind we prefer to do the backport in our way, picking the PR commit and then doing following commits if something needs to be addressed, e.g.: breaking changes in v4. The way this PR is done requires us to verify again the content of all the files included and I see that the original PR included changes in 10 files but this one just touched 8.

@kagahd
Copy link
Contributor Author

kagahd commented May 17, 2024

If you don't mind we prefer to do the backport in our way,

Yes, of course. I thought I could help you by doing this to speed up the backport to v3 because we need this check in our company. But if you have your own mechanism for backporting, I fully understand that you prefer to use that.

I'm also very interested to know how to deal with the cases I described on prowler's slack channel related to this check ec2_securitygroup_allow_ingress_from_internet_to_any_port.

Copied from the slack channel:

prowler has checks to ensure that there are no AWS Security Groups without ingress filtering (0.0.0.0/0) being used.
In prowler v2 it's extra74 and in prowler v3/v4 it's ec2_securitygroup_allow_ingress_from_internet_to_any_port.
However, there are cases where an ec2 instance requires access from or to external IP addresses. In these cases, to reduce the attack surface, it's best practice to put a load balancer or an API gateway in front of the ec2 instance whereas the ec2 instance must only allow traffic from/to its load balancer. Furthermore, the load balancer or API gateway should allow traffic only on the IP range, the protocols and the ports that are required for the application running on the ec2 instance.
Additionally, separation of concerns should be applied: each resource or resource group should have its own security group. Especially if the security group allows public access, it must only be attached to the load balancer or an API gateway.
Additionally, security groups may get automatically created by AWS for VPC endpoints as for example GuardDuty. These, by AWS automatically created, security groups allow all in- and outbound traffic (0.0.0.0/0) but since their endpoint is on AWS side, we consider it's not our responsibility to protect AWS resources.
Therefore, we didn't want to get alarmed by the prowler checks in these special cases.
To achieve this goal, we patched prowler v2 check extra74 as follows, the changed code line is:
SG_NO_INGRESS_FILTER=$($AWSCLI ec2 describe-network-interfaces $PROFILE_OPT --region $regx --filters "Name=group-id,Values=$SG_ID" --query "length(NetworkInterfaces[?Attachment.InstanceOwnerId != 'amazon-elb' && InterfaceType != 'api_gateway_managed' && InterfaceType != 'vpc_endpoint'])" --output text)
instead of:
SG_NO_INGRESS_FILTER=$($AWSCLI ec2 describe-network-interfaces $PROFILE_OPT --region $regx --filters "Name=group-id,Values=$SG_ID" --query "length(NetworkInterfaces)" --output text)
How do you deal with these special cases?
Do you think it's worth to extend prowler v3/v4 check ec2_securitygroup_allow_ingress_from_internet_to_any_port or to write a new one to cover these special cases?

Do you think that you could implement my suggestions in this check or a even create a new check?
Or do you prefer that my company uses a customized check?
I think such a check would make sense to many users of prowler so I would suggest to have such a check in the official prowler distribution. What do you think?

@jfagoagas
Copy link
Member

Yes, of course. I thought I could help you by doing this to speed up the backport to v3 because we need this check in our company. But if you have your own mechanism for backporting, I fully understand that you prefer to use that.

I'm also very interested to know how to deal with the cases I described on prowler's slack channel related to this check ec2_securitygroup_allow_ingress_from_internet_to_any_port. Do you think that you could implement my suggestions in this check or a even create a new check? Or do you prefer that my company uses a customized check? I think such a check would make sense to many users of prowler so I would suggest to have such a check in the official prowler distribution. What do you think?

This is an interesting point, I think probably we can include a modification in the EC2 service and tests to handle that case. Could you show me an example of a security group created automatically by the creation of a VPC endpoint?

What are the cases you want to cover? Just the above? Remember that the Mutelist is a powerful ally here.

@kagahd
Copy link
Contributor Author

kagahd commented May 21, 2024

Hi @jfagoagas, thanks for your reply.
Indeed, it would be great to cover these cases!
Until now, we just have the three cases that I mentioned above. Here is an example of an open security group created automatically by GuardDuty:

SG_withInbound0 0 0 0_masked

We are aware of the allowlist or mutelist and are using it heavily. But in this case I don't see how to "allowlist" these resources if not by implementing a filter on the Interface Type as we did for prowler v2:
Our patched version:
SG_NO_INGRESS_FILTER=$($AWSCLI ec2 describe-network-interfaces $PROFILE_OPT --region $regx --filters "Name=group-id,Values=$SG_ID" --query "length(NetworkInterfaces[?Attachment.InstanceOwnerId != 'amazon-elb' && InterfaceType != 'api_gateway_managed' && InterfaceType != 'vpc_endpoint'])" --output text)
The original one:
SG_NO_INGRESS_FILTER=$($AWSCLI ec2 describe-network-interfaces $PROFILE_OPT --region $regx --filters "Name=group-id,Values=$SG_ID" --query "length(NetworkInterfaces)" --output text)

What's your idea to implement this in prowler v3/v4?

@jfagoagas
Copy link
Member

Indeed, it would be great to cover these cases!
Until now, we just have the three cases that I mentioned above. Here is an example of an open security group created automatically by GuardDuty:

What if we include these filters in the EC2 service while retrieving security groups? We can store the InstanceOwnerId of the network interface and the InterfaceType and include a config argument to check it or not. What do you think?

Btw, the current PR was backported yesterday to v3 and released in https://github.com/prowler-cloud/prowler/releases/tag/3.16.5. This PR can be closed, but let's keep it open until we find a solution for the issue.

@kagahd
Copy link
Contributor Author

kagahd commented May 22, 2024

What if we include these filters in the EC2 service while retrieving security groups? We can store the InstanceOwnerId of the network interface and the InterfaceType and include a config argument to check it or not. What do you think?

This sounds really good!

Btw, the current PR was backported yesterday to v3 and released in https://github.com/prowler-cloud/prowler/releases/tag/3.16.5.

Nice, well done, thank you! 🚀

This PR can be closed, but let's keep it open until we find a solution for the issue.

Ok, but shouldn't we rather move it to "discussions"? But maybe it's not worth anymore since your proposal sounds very good! 😄

@jfagoagas
Copy link
Member

What if we include these filters in the EC2 service while retrieving security groups? We can store the InstanceOwnerId of the network interface and the InterfaceType and include a config argument to check it or not. What do you think?

This sounds really good!

Btw, the current PR was backported yesterday to v3 and released in https://github.com/prowler-cloud/prowler/releases/tag/3.16.5.

Nice, well done, thank you! 🚀

This PR can be closed, but let's keep it open until we find a solution for the issue.

Ok, but shouldn't we rather move it to "discussions"? But maybe it's not worth anymore since your proposal sounds very good! 😄

Makes sense 😂 You can open up an issue or a thread in the Slack channel, whatever you want. Let's work on that there. Thanks again 👏

@jfagoagas jfagoagas closed this May 22, 2024
@kagahd
Copy link
Contributor Author

kagahd commented May 22, 2024

The discussion continues here: #4065

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-merge Please, DO NOT MERGE this PR. provider/aws Issues/PRs related with the AWS provider v3 Everything related to Prowler 3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants