chore(aws): Add failed_checks to track

[kagahd]

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[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[jfagoagas]

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]

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.

[kagahd]

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]

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

It's done.

[kagahd]

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!

[jfagoagas]

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]

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]

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]

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]

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]

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]

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.