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

fix(aws): review checks with wrong attributes #5503

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from prowler.lib.check.models import Check, Check_Report_AWS
from prowler.lib.check.models import Check, Check_Report_AWS, Severity
from prowler.providers.aws.services.acm.acm_client import acm_client


Expand All @@ -22,10 +22,10 @@ def execute(self):
report.status = "FAIL"
if certificate.expiration_days < 0:
report.status_extended = f"ACM Certificate {certificate.id} for {certificate.name} has expired ({abs(certificate.expiration_days)} days ago)."
report.check_metadata.Severity = "high"
report.check_metadata.Severity = Severity.high
else:
report.status_extended = f"ACM Certificate {certificate.id} for {certificate.name} is about to expire in {certificate.expiration_days} days."
report.check_metadata.Severity = "medium"
report.check_metadata.Severity = Severity.medium

report.resource_id = certificate.id
report.resource_details = certificate.name
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"Provider": "aws",
"CheckID": "codebuild_project_no_secrets_in_variables",
"CheckTitle": "Ensure CodeBuild projects do not contain secrets on plaintext environmet variables",
"CheckTitle": "Ensure CodeBuild projects do not contain secrets on plaintext environment variables",
"CheckType": [
"Security Best Practices"
],
Expand All @@ -21,7 +21,7 @@
"Terraform": ""
},
"Recommendation": {
"Text": "",
"Text": "Do not store secrets in plaintext environment variables. Use AWS Secrets Manager or AWS Systems Manager Parameter Store to securely store and retrieve sensitive information.",
"Url": "https://docs.aws.amazon.com/codebuild/latest/userguide/change-project.html"
}
},
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from prowler.lib.check.models import Check, Check_Report_AWS
from prowler.lib.check.models import Check, Check_Report_AWS, Severity
from prowler.providers.aws.services.documentdb.documentdb_client import (
documentdb_client,
)
Expand All @@ -25,7 +25,7 @@ def execute(self):
else:
if cluster.backup_retention_period > 0:
report.status = "FAIL"
report.check_metadata.Severity = "low"
report.check_metadata.Severity = Severity.low
report.status_extended = f"DocumentDB Cluster {cluster.id} has backup enabled with retention period {cluster.backup_retention_period} days. Recommended to increase the backup retention period to a minimum of 7 days."

findings.append(report)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from prowler.lib.check.models import Check, Check_Report_AWS
from prowler.lib.check.models import Check, Check_Report_AWS, Severity
from prowler.providers.aws.services.documentdb.documentdb_client import (
documentdb_client,
)
Expand Down Expand Up @@ -27,7 +27,7 @@ def execute(self):
or "profiler" in cluster.cloudwatch_logs
):
report.status = "FAIL"
report.check_metadata.Severity = "low"
report.check_metadata.Severity = Severity.low
report.status_extended = f"DocumentDB Cluster {cluster.id} is only shipping {' '.join(cluster.cloudwatch_logs)} to CloudWatch Logs. Recommended to ship both Audit and Profiler logs."

findings.append(report)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,7 @@ def execute(self):
report = Check_Report_AWS(self.metadata())
report.region = registry.region
report.resource_id = registry.id
# A registry cannot have tags
report.resource_tags = []
report.resource_arn = ecr_client.audited_account_arn
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think here we should create our own ARN, what do you think?

report.status = "FAIL"
report.status_extended = f"ECR registry {registry.id} has {registry.scan_type} scanning without scan on push enabled."
if registry.rules:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from prowler.lib.check.models import Check, Check_Report_AWS
from prowler.lib.check.models import Check, Check_Report_AWS, Severity
from prowler.providers.aws.services.elasticache.elasticache_client import (
elasticache_client,
)
Expand All @@ -23,7 +23,7 @@ def execute(self):
else:
if repl_group.snapshot_retention > 0:
report.status = "FAIL"
report.check_metadata.Severity = "low"
report.check_metadata.Severity = Severity.low
report.status_extended = f"Elasticache Redis cache cluster {repl_group.id} has automated snapshot backups enabled with retention period {repl_group.snapshot_retention} days. Recommended to increase the snapshot retention period to a minimum of 7 days."

findings.append(report)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
"SubServiceName": "",
"ResourceIdTemplate": "arn:aws:guardduty:region:account-id/detector-id",
"Severity": "high",
"ResourceType": "",
"ResourceType": "AwsGuardDutyDetector",
"Description": "GuardDuty Lambda Protection helps you identify potential security threats when an AWS Lambda function gets invoked. After you enable Lambda Protection, GuardDuty starts monitoring Lambda network activity logs associated with the Lambda functions in your AWS account.",
"Risk": "If Lambda Protection is not enabled, GuardDuty will not be able to monitor Lambda network activity logs and may miss potential security threats.",
"RelatedUrl": "https://docs.aws.amazon.com/guardduty/latest/ug/lambda-protection.html",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from prowler.lib.check.models import Check, Check_Report_AWS
from prowler.lib.check.models import Check, Check_Report_AWS, Severity
from prowler.providers.aws.services.neptune.neptune_client import neptune_client


Expand All @@ -23,7 +23,7 @@ def execute(self):
else:
if cluster.backup_retention_period > 0:
report.status = "FAIL"
report.check_metadata.Severity = "low"
report.check_metadata.Severity = Severity.low
report.status_extended = f"Neptune Cluster {cluster.name} has backup enabled with retention period {cluster.backup_retention_period} days. Recommended to increase the backup retention period to a minimum of 7 days."

findings.append(report)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,9 +116,13 @@
except ClientError as error:
if error.response["Error"]["Code"] == "AccessDeniedException":
policies = None
logger.error(
f"{self.region} -- {error.__class__.__name__}[{error.__traceback__.tb_lineno}]: {error}"
)
logger.warning(

Check warning on line 119 in prowler/providers/aws/services/organizations/organizations_service.py

View check run for this annotation

Codecov / codecov/patch

prowler/providers/aws/services/organizations/organizations_service.py#L119

Added line #L119 was not covered by tests
f"{self.region} -- {error.__class__.__name__}[{error.__traceback__.tb_lineno}]: {error}"
)
else:
logger.error(

Check warning on line 123 in prowler/providers/aws/services/organizations/organizations_service.py

View check run for this annotation

Codecov / codecov/patch

prowler/providers/aws/services/organizations/organizations_service.py#L123

Added line #L123 was not covered by tests
f"{self.region} -- {error.__class__.__name__}[{error.__traceback__.tb_lineno}]: {error}"
)

except Exception as error:
logger.error(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
from dateutil import relativedelta
from pytz import utc

from prowler.lib.check.models import Check, Check_Report_AWS
from prowler.lib.check.models import Check, Check_Report_AWS, Severity
from prowler.providers.aws.services.rds.rds_client import rds_client


Expand All @@ -21,7 +21,7 @@
report.resource_arn = db_instance_arn
report.resource_tags = db_instance.tags
report.status = "FAIL"
report.check_metadata.Severity = "critical"
report.check_metadata.Severity = Severity.critical
report.status_extended = (
f"RDS Instance {db_instance.id} certificate has expired."
)
Expand All @@ -33,7 +33,7 @@
utc
) + relativedelta.relativedelta(months=6):
report.status = "PASS"
report.check_metadata.Severity = "informational"
report.check_metadata.Severity = Severity.informational
report.status_extended = f"RDS Instance {db_instance.id} certificate has over 6 months of validity left."
elif cert.valid_till < datetime.now(
utc
Expand All @@ -45,7 +45,7 @@
months=3
):
report.status = "PASS"
report.check_metadata.Severity = "low"
report.check_metadata.Severity = Severity.low
report.status_extended = f"RDS Instance {db_instance.id} certificate has between 3 and 6 months of validity."
elif cert.valid_till < datetime.now(
utc
Expand All @@ -57,7 +57,7 @@
months=1
):
report.status = "FAIL"
report.check_metadata.Severity = "medium"
report.check_metadata.Severity = Severity.medium
report.status_extended = f"RDS Instance {db_instance.id} certificate less than 3 months of validity."
elif cert.valid_till < datetime.now(
utc
Expand All @@ -67,11 +67,11 @@
utc
):
report.status = "FAIL"
report.check_metadata.Severity = "high"
report.check_metadata.Severity = Severity.high

Check warning on line 70 in prowler/providers/aws/services/rds/rds_instance_certificate_expiration/rds_instance_certificate_expiration.py

View check run for this annotation

Codecov / codecov/patch

prowler/providers/aws/services/rds/rds_instance_certificate_expiration/rds_instance_certificate_expiration.py#L70

Added line #L70 was not covered by tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, add a test to cover this scenarion.

report.status_extended = f"RDS Instance {db_instance.id} certificate less than 1 month of validity."
else:
report.status = "FAIL"
report.check_metadata.Severity = "critical"
report.check_metadata.Severity = Severity.critical
report.status_extended = (
f"RDS Instance {db_instance.id} certificate has expired."
)
Expand All @@ -80,7 +80,7 @@
utc
) + relativedelta.relativedelta(months=6):
report.status = "PASS"
report.check_metadata.Severity = "informational"
report.check_metadata.Severity = Severity.informational
report.status_extended = f"RDS Instance {db_instance.id} custom certificate has over 6 months of validity left."
elif cert.valid_till < datetime.now(
utc
Expand All @@ -92,7 +92,7 @@
months=3
):
report.status = "PASS"
report.check_metadata.Severity = "low"
report.check_metadata.Severity = Severity.low
report.status_extended = f"RDS Instance {db_instance.id} custom certificate has between 3 and 6 months of validity."
elif cert.valid_till < datetime.now(
utc
Expand All @@ -104,7 +104,7 @@
months=1
):
report.status = "FAIL"
report.check_metadata.Severity = "medium"
report.check_metadata.Severity = Severity.medium
report.status_extended = f"RDS Instance {db_instance.id} custom certificate less than 3 months of validity."
elif cert.valid_till < datetime.now(
utc
Expand All @@ -114,11 +114,11 @@
utc
):
report.status = "FAIL"
report.check_metadata.Severity = "high"
report.check_metadata.Severity = Severity.high
report.status_extended = f"RDS Instance {db_instance.id} custom certificate less than 1 month of validity."
else:
report.status = "FAIL"
report.check_metadata.Severity = "critical"
report.check_metadata.Severity = Severity.critical
report.status_extended = f"RDS Instance {db_instance.id} custom certificate has expired."
findings.append(report)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
"Terraform": ""
},
"Recommendation": {
"Text": "",
"Text": "Create a backup plan for the RDS instance to protect it from data loss, accidental deletion, or corruption.",
"Url": "https://docs.aws.amazon.com/aws-backup/latest/devguide/assigning-resources.html"
}
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ def execute(self) -> Check_Report_AWS:
for domain in route53domains_client.domains.values():
report = Check_Report_AWS(self.metadata())
report.resource_id = domain.name
report.resource_arn = route53domains_client.audited_account_arn
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same again to create an ARN for this one.

report.region = domain.region
report.resource_tags = domain.tags
if domain.admin_privacy:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ def execute(self) -> Check_Report_AWS:
for domain in route53domains_client.domains.values():
report = Check_Report_AWS(self.metadata())
report.resource_id = domain.name
report.resource_arn = route53domains_client.audited_account_arn
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same again to create an ARN for this one.

report.region = domain.region
report.resource_tags = domain.tags
if domain.status_list and "clientTransferProhibited" in domain.status_list:
Expand Down
24 changes: 15 additions & 9 deletions prowler/providers/aws/services/waf/waf_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,21 @@ def __init__(self, provider):
self.rules = {}
self.rule_groups = {}
self.web_acls = {}
self._list_rules()
self.__threading_call__(self._get_rule, self.rules.values())
self._list_rule_groups()
self.__threading_call__(
self._list_activated_rules_in_rule_group, self.rule_groups.values()
)
self._list_web_acls()
self.__threading_call__(self._get_web_acl, self.web_acls.values())
self.__threading_call__(self._get_logging_configuration, self.web_acls.values())
if self.audited_partition == "aws":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why only aws? China has also CloudFront, at least by our regions file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can see in https://docs.aws.amazon.com/general/latest/gr/waf-classic.html that only this partition is supported.

# AWS WAF is available globally for CloudFront distributions, but you must use the Region US East (N. Virginia) to create your web ACL and any resources used in the web ACL, such as rule groups, IP sets, and regex pattern sets.
self.region = "us-east-1"
self.client = self.session.client(self.service, self.region)
self._list_rules()
self.__threading_call__(self._get_rule, self.rules.values())
self._list_rule_groups()
self.__threading_call__(
self._list_activated_rules_in_rule_group, self.rule_groups.values()
)
self._list_web_acls()
self.__threading_call__(self._get_web_acl, self.web_acls.values())
self.__threading_call__(
self._get_logging_configuration, self.web_acls.values()
)

def _list_rules(self):
logger.info("WAF - Listing Global Rules...")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
ScanningRule,
)
from tests.providers.aws.utils import (
AWS_ACCOUNT_ARN,
AWS_ACCOUNT_NUMBER,
AWS_REGION_EU_WEST_1,
set_mocked_aws_provider,
Expand Down Expand Up @@ -66,6 +67,7 @@ def test_registry_no_repositories(self):

def test_registry_scan_on_push_enabled(self):
ecr_client = mock.MagicMock
ecr_client.audited_account_arn = AWS_ACCOUNT_ARN
ecr_client.registries = {}
ecr_client.registries[AWS_REGION_EU_WEST_1] = Registry(
id=AWS_ACCOUNT_NUMBER,
Expand Down Expand Up @@ -107,10 +109,12 @@ def test_registry_scan_on_push_enabled(self):
assert result[0].status == "PASS"
assert search("with scan on push", result[0].status_extended)
assert result[0].resource_id == AWS_ACCOUNT_NUMBER
assert result[0].resource_arn == AWS_ACCOUNT_ARN
assert result[0].region == AWS_REGION_EU_WEST_1

def test_scan_on_push_enabled_with_filters(self):
ecr_client = mock.MagicMock
ecr_client.audited_account_arn = AWS_ACCOUNT_ARN
ecr_client.registries = {}
ecr_client.registries[AWS_REGION_EU_WEST_1] = Registry(
id=AWS_ACCOUNT_NUMBER,
Expand Down Expand Up @@ -155,10 +159,12 @@ def test_scan_on_push_enabled_with_filters(self):
result[0].status_extended,
)
assert result[0].resource_id == AWS_ACCOUNT_NUMBER
assert result[0].resource_arn == AWS_ACCOUNT_ARN
assert result[0].region == AWS_REGION_EU_WEST_1

def test_scan_on_push_disabled(self):
ecr_client = mock.MagicMock
ecr_client.audited_account_arn = AWS_ACCOUNT_ARN
ecr_client.registries = {}
ecr_client.registries[AWS_REGION_EU_WEST_1] = Registry(
id=AWS_ACCOUNT_NUMBER,
Expand Down Expand Up @@ -195,4 +201,5 @@ def test_scan_on_push_disabled(self):
assert result[0].status == "FAIL"
assert search("scanning without scan on push", result[0].status_extended)
assert result[0].resource_id == AWS_ACCOUNT_NUMBER
assert result[0].resource_arn == AWS_ACCOUNT_ARN
assert result[0].region == AWS_REGION_EU_WEST_1
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
from unittest import mock

from prowler.providers.aws.services.route53.route53_service import Domain
from tests.providers.aws.utils import AWS_REGION_US_EAST_1
from tests.providers.aws.utils import AWS_ACCOUNT_ARN, AWS_REGION_US_EAST_1


class Test_route53_domains_privacy_protection_enabled:
Expand All @@ -25,6 +25,7 @@ def test_no_domains(self):

def test_domain_privacy_protection_disabled(self):
route53domains = mock.MagicMock
route53domains.audited_account_arn = AWS_ACCOUNT_ARN
domain_name = "test-domain.com"
route53domains.domains = {
domain_name: Domain(
Expand All @@ -46,6 +47,7 @@ def test_domain_privacy_protection_disabled(self):

assert len(result) == 1
assert result[0].resource_id == domain_name
assert result[0].resource_arn == AWS_ACCOUNT_ARN
assert result[0].region == AWS_REGION_US_EAST_1
assert result[0].status == "FAIL"
assert (
Expand All @@ -55,6 +57,7 @@ def test_domain_privacy_protection_disabled(self):

def test_domain_privacy_protection_enabled(self):
route53domains = mock.MagicMock
route53domains.audited_account_arn = AWS_ACCOUNT_ARN
domain_name = "test-domain.com"
route53domains.domains = {
domain_name: Domain(
Expand All @@ -76,6 +79,7 @@ def test_domain_privacy_protection_enabled(self):

assert len(result) == 1
assert result[0].resource_id == domain_name
assert result[0].resource_arn == AWS_ACCOUNT_ARN
assert result[0].region == AWS_REGION_US_EAST_1
assert result[0].status == "PASS"
assert (
Expand Down
Loading