chore(aws): Add failed_checks to track #4018

kagahd · 2024-05-16T10:09:30Z

Context

This PR makes the AWS ec2 service and around around 15 prowler checks, that verify whether ports are open to the Internet, more concise. This PR relates to PR #3977 and PR #3979.

Description

I find it more concise to introduce a state_manager instead to share with a dozen of other checks a specific property public_ports of a specific check ec2_securitygroup_allow_ingress_from_internet_to_all_ports within the ec2_service.
I find the the code now clearer, cleaner, easier to extend and easier to maintain. It also saves some CPU cycles because the method check_security_group in the ec2_service does not need to be called for each check anymore.

License

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

codecov · 2024-05-16T10:30:14Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.28%. Comparing base (248c7c5) to head (efa1c8f).
Report is 441 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4018      +/-   ##
==========================================
+ Coverage   86.27%   86.28%   +0.01%     
==========================================
  Files         783      783              
  Lines       24562    24599      +37     
==========================================
+ Hits        21190    21226      +36     
- Misses       3372     3373       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jfagoagas · 2024-05-17T07:10:51Z

Hi @kagahd I like your idea but we are not sure about including it at this time since we want to create an execution manager in top of Prowler's scan. Maybe it'll be easier for now to save something within the EC2 service, what do you think?

kagahd · 2024-05-17T07:43:09Z

Hi @jfagoagas,

Maybe it'll be easier for now to save something within the EC2 service, what do you think?

I preferred a separation of concerns by using the state_manager but we can put its functionality also in the ec2 service, if you prefer.
I just adapted the PR accordingly.

prowler/providers/aws/services/ec2/ec2_service.py

…d and region of the failed check

jfagoagas

Could you add a test for each check while checking that ec2_client.is_failed_check() returns true, thus no findings will be generated?

The work is this PR is neat 💯

prowler/providers/aws/lib/service/service.py

kagahd · 2024-05-17T08:51:14Z

Could you add a test for each check while checking that ec2_client.is_failed_check() returns true, thus no findings will be generated?

Sure, I will add them later today.

kagahd · 2024-05-17T16:09:06Z

Could you add a test for each check while checking that ec2_client.is_failed_check()

It's done.

…because all ports are already open

…ed to avoid reported twice because all ports are already open

jfagoagas

@kagahd I left some comments about the tests and a tiny detail in the service logic, please let me know if you need help. Thanks!

prowler/providers/aws/lib/service/service.py

..._from_internet_to_any_port/ec2_securitygroup_allow_ingress_from_internet_to_any_port_test.py

...gress_from_internet_to_any_port/ec2_securitygroup_allow_ingress_from_internet_to_any_port.py

...rom_internet_to_all_ports/ec2_securitygroup_allow_ingress_from_internet_to_all_ports_test.py

..._from_internet_to_any_port/ec2_securitygroup_allow_ingress_from_internet_to_any_port_test.py

jfagoagas · 2024-05-20T08:04:04Z

..._from_internet_to_any_port/ec2_securitygroup_allow_ingress_from_internet_to_any_port_test.py

+            check_all_ports = (
+                ec2_securitygroup_allow_ingress_from_internet_to_all_ports()
+            )
+            result_all_ports = check_all_ports.execute()
+
+            # Verify that the all ports check has detected the issue
+            assert any(
+                sg.status == "FAIL" and sg.resource_id == default_sg_id
+                for sg in result_all_ports
+            )


Maybe is easier just to call AWSService.set_failed_check and then run the ec2_securitygroup_allow_ingress_from_internet_to_any_port and verify that the finding's status extended is the new one.

This comment applies to all the tests.

Maybe is easier just to call AWSService.set_failed_check and then run the ec2_securitygroup_allow_ingress_from_internet_to_any_port and verify that the finding's status extended is the new one.

Yes, sure, it would be easier but it's cheating because it would not reflect the exact way how the values are set/get in real life by the app.
To be consistent, we need to first execute the check ec2_securitygroup_allow_ingress_from_internet_to_all_ports by checking an open security group to all ports. And after that, we have to verify that a specific port check such as ec2_securitygroup_allow_ingress_from_internet_to_any_port is in status PASS, only because the previous run check ec2_securitygroup_allow_ingress_from_internet_to_all_ports is in status FAIL, in order to avoid to report the specific open ports twice.
You need to do it this way to respect and verify by unit tests the call chain of the app.

kagahd · 2024-05-21T13:02:06Z

Hi @jfagoagas
could you please advise, why the pr-lint-test tests are failing after my last last commits from today? When I run them locally, only 2 of a total 3175 unit tests are failing (which were failing also before my todays commits):

poetry run pytest -n auto --cov=./prowler --cov-report=xml tests
...
=========================================================================================== short test summary info ===========================================================================================
FAILED tests/providers/aws/services/route53/route53_dangling_ip_subdomain_takeover/route53_dangling_ip_subdomain_takeover_test.py::Test_route53_dangling_ip_subdomain_takeover::test_hosted_zone_dangling_public_record - urllib.error.URLError: <urlopen error [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:997)>
FAILED tests/providers/aws/services/route53/route53_dangling_ip_subdomain_takeover/route53_dangling_ip_subdomain_takeover_test.py::Test_route53_dangling_ip_subdomain_takeover::test_hosted_zone_external_record - urllib.error.URLError: <urlopen error [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:997)>
=========================================================================== 2 failed, 3173 passed, 36 warnings in 399.73s (0:06:39) ===========================================================================
~

And these 2 failing unit tests are not from me. I didn't touch them.
Why the github workflow failed on so many unit tests?
Thanks for your help!

…tate_manager

jfagoagas · 2024-05-21T14:53:54Z

Hi @jfagoagas could you please advise, why the pr-lint-test tests are failing after my last last commits from today? When I run them locally, only 2 of a total 3175 unit tests are failing (which were failing also before my todays commits):

poetry run pytest -n auto --cov=./prowler --cov-report=xml tests
...
=========================================================================================== short test summary info ===========================================================================================
FAILED tests/providers/aws/services/route53/route53_dangling_ip_subdomain_takeover/route53_dangling_ip_subdomain_takeover_test.py::Test_route53_dangling_ip_subdomain_takeover::test_hosted_zone_dangling_public_record - urllib.error.URLError: <urlopen error [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:997)>
FAILED tests/providers/aws/services/route53/route53_dangling_ip_subdomain_takeover/route53_dangling_ip_subdomain_takeover_test.py::Test_route53_dangling_ip_subdomain_takeover::test_hosted_zone_external_record - urllib.error.URLError: <urlopen error [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:997)>
=========================================================================== 2 failed, 3173 passed, 36 warnings in 399.73s (0:06:39) ===========================================================================
~

And these 2 failing unit tests are not from me. I didn't touch them. Why the github workflow failed on so many unit tests? Thanks for your help!

I think your forked repo wasn't up to date with Prowler's master. I've sync'ed your master branch and then merge that branch against this. Probably failing test happened due to this and also Github actions are degraded today so that could be also.

kagahd · 2024-05-21T20:00:31Z

Hi @jfagoagas, the github workflow failed again with the same errors. Do you have another idea what the reason could be? In line 155 it says AttributeError: module 'prowler.providers.common' has no attribute 'common' but other unit tests do the same what's marked in line 105:

        with mock.patch(
            "prowler.providers.common.common.get_global_provider",
            return_value=aws_provider,
        ),

What's wrong with it?

jfagoagas · 2024-05-22T06:36:29Z

Hi @jfagoagas, the github workflow failed again with the same errors. Do you have another idea what the reason could be? In line 155 it says AttributeError: module 'prowler.providers.common' has no attribute 'common' but other unit tests do the same what's marked in line 105:
        with mock.patch(
            "prowler.providers.common.common.get_global_provider",
            return_value=aws_provider,
        ),
What's wrong with it?

Yesterday we merged a PR which changes the location of the get_global_provider function, that was the issue. I've pushed a fix.

kagahd · 2024-05-22T06:45:24Z

Yesterday we merged a PR which changes the location of the get_global_provider function, that was the issue. I've pushed a fix.

That makes sense, thanks a bunch!

jfagoagas

Thanks for this super improvement again @kagahd 👏

It's been a pleasure to work with you and review a PR with such a great level of quality 🚀

jfagoagas · 2024-05-22T06:39:29Z

...rom_internet_to_all_ports/ec2_securitygroup_allow_ingress_from_internet_to_all_ports_test.py

+            result = check.execute()
+
+            # Verify set_failed_check was called with the correct arguments
+            mock_set_failed_check.assert_called_with(


Pretty smart 💯

jfagoagas · 2024-05-22T06:50:52Z

I have just one question left because I'm not sure, what was the previous behaviour when running an specific port check with the public_ports set to True? I thought we were not raising a finding but I'm not sure I have to test it, I mean to not modify that behaviour now.

kagahd · 2024-05-22T07:03:55Z

what was the previous behaviour when running an specific port check with the public_ports set to True? I thought we were not raising a finding

Exactly, you were not raising a finding, the status was "PASS".
This behaviour is still the same, the status is still "PASS". However, I adapted the report.status_extended in order to mention that "has all ports open to the Internet and therefore was not checked against the specific XYZ port". Without my adaption of report.status_extended you would still have a message like "Security group ABC does not have XYZ port open to the Internet." which is quite wrong because all ports are open.

jfagoagas · 2024-05-22T07:18:31Z

what was the previous behaviour when running an specific port check with the public_ports set to True? I thought we were not raising a finding

Exactly, you were not raising a finding, the status was "PASS". This behaviour is still the same, the status is still "PASS". However, I adapted the report.status_extended in order to mention that "has all ports open to the Internet and therefore was not checked against the specific XYZ port". Without my adaption of report.status_extended you would still have a message like "Security group ABC does not have XYZ port open to the Internet." which is quite wrong because all ports are open.

Thanks for confirming that, maybe we can remove that PASS finding in the future, but at the end the important ones are the FAIL findings and not to raise 2 with the same information.

Co-authored-by: Pepe Fagoaga <[email protected]>

add state_manager to make code more concise

933ae49

kagahd requested review from a team May 16, 2024 10:09

github-actions bot added the provider/aws Issues/PRs related with the AWS provider label May 16, 2024

remove all_public_ports

9252b6d

jfagoagas self-assigned this May 17, 2024

jfagoagas changed the title ~~add state_manager to make code more concise~~ chore(ec2): Add state_manager to make code more concise while checking security groups May 17, 2024

move state_manager to ec2_service

b2dfabc

jfagoagas reviewed May 17, 2024

View reviewed changes

prowler/providers/aws/services/ec2/ec2_service.py Outdated Show resolved Hide resolved

move state_manager from ec2_service to aws service and add resource i…

5141b53

…d and region of the failed check

jfagoagas self-requested a review May 17, 2024 08:38

jfagoagas changed the title ~~chore(ec2): Add state_manager to make code more concise while checking security groups~~ chore(aws): Add failed_checks to track May 17, 2024

jfagoagas requested changes May 17, 2024

View reviewed changes

prowler/providers/aws/lib/service/service.py Outdated Show resolved Hide resolved

fix typo

5ad7150

add unit tests for set_failed_check() and is_failed_check()

320e49a

kagahd added 4 commits May 18, 2024 12:54

add unit test

e3d11ae

adapt report.status_extended in case specific ports were not checked …

928fa70

…because all ports are already open

add unit tests to cover the cases where specific ports were not check…

2fb14e2

…ed to avoid reported twice because all ports are already open

remove unused variable

ea881c1

jfagoagas self-requested a review May 20, 2024 07:36

jfagoagas requested changes May 20, 2024

View reviewed changes

kagahd added 2 commits May 21, 2024 12:39

use arn instead of resource_id and region, adapt unit tests

f57366d

make flake happy

95ba02f

Merge branch 'master' of https://github.com/kagahd/prowler into add_s…

ac5eab2

…tate_manager

chore(replace): Use new way to import provider

efa1c8f

jfagoagas self-requested a review May 22, 2024 06:36

jfagoagas approved these changes May 22, 2024

View reviewed changes

jfagoagas merged commit 69166a0 into prowler-cloud:master May 22, 2024
11 checks passed

jfagoagas added the backport-v3 label May 22, 2024

MrCloudSec removed the backport-v3 label May 30, 2024

MrCloudSec pushed a commit that referenced this pull request May 30, 2024

chore(aws): Add failed_checks to track (#4018)

94661a4

Co-authored-by: Pepe Fagoaga <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(aws): Add failed_checks to track #4018

chore(aws): Add failed_checks to track #4018

kagahd commented May 16, 2024

codecov bot commented May 16, 2024 •

edited

Loading

jfagoagas commented May 17, 2024 •

edited

Loading

kagahd commented May 17, 2024

jfagoagas left a comment

kagahd commented May 17, 2024

kagahd commented May 17, 2024

jfagoagas left a comment

jfagoagas May 20, 2024

jfagoagas May 20, 2024

kagahd May 21, 2024

kagahd commented May 21, 2024 •

edited

Loading

jfagoagas commented May 21, 2024

kagahd commented May 21, 2024 •

edited

Loading

jfagoagas commented May 22, 2024

kagahd commented May 22, 2024

jfagoagas left a comment

jfagoagas May 22, 2024

jfagoagas commented May 22, 2024

kagahd commented May 22, 2024

jfagoagas commented May 22, 2024

chore(aws): Add failed_checks to track #4018

chore(aws): Add failed_checks to track #4018

Conversation

kagahd commented May 16, 2024

Context

Description

License

codecov bot commented May 16, 2024 • edited Loading

Codecov Report

jfagoagas commented May 17, 2024 • edited Loading

kagahd commented May 17, 2024

jfagoagas left a comment

Choose a reason for hiding this comment

kagahd commented May 17, 2024

kagahd commented May 17, 2024

jfagoagas left a comment

Choose a reason for hiding this comment

jfagoagas May 20, 2024

Choose a reason for hiding this comment

jfagoagas May 20, 2024

Choose a reason for hiding this comment

kagahd May 21, 2024

Choose a reason for hiding this comment

kagahd commented May 21, 2024 • edited Loading

jfagoagas commented May 21, 2024

kagahd commented May 21, 2024 • edited Loading

jfagoagas commented May 22, 2024

kagahd commented May 22, 2024

jfagoagas left a comment

Choose a reason for hiding this comment

jfagoagas May 22, 2024

Choose a reason for hiding this comment

jfagoagas commented May 22, 2024

kagahd commented May 22, 2024

jfagoagas commented May 22, 2024

codecov bot commented May 16, 2024 •

edited

Loading

jfagoagas commented May 17, 2024 •

edited

Loading

kagahd commented May 21, 2024 •

edited

Loading

kagahd commented May 21, 2024 •

edited

Loading