[Bug]: Separate Checks for Expired and Near-Expiration ACM Certificates

[MrMoshkovitz]

Steps to Reproduce

The current implementation of the ACM certificates expiration check in the Prowler tool does not distinguish between expired and near-expiration certificates. Both types are checked together, without differentiation in severity or metadata. This lack of distinction can lead to inadequate prioritization and handling of certificate-related issues.

Steps to Reproduce

prowler aws --profile $profile -f $region --checks acm_certificates_expiration_check

1. Run the Prowler tool against an AWS account that has both expired and near-expiration ACM certificates.
2. Observe the output of the acm_certificates_expiration_check.py file.

Expected behavior

• The check should be separated into two distinct checks:
  1. Expired Certificates Check:
     • High severity
     • Relevant metadata indicating the urgency
  2. Near-Expiration Certificates Check:
     • Lower severity
     • Relevant metadata indicating the upcoming expiration

Actual Result with Screenshots or Logs

• Both expired and near-expiration certificates are currently checked together without any differentiation in severity or metadata. This results in a single output that does not prioritize expired certificates over those nearing expiration.

How did you install Prowler?

From pip package (pip install prowler)

Environment Resource

1. ACM

OS used

1. MacOS

Prowler version

14

Pip version

latest

Context

Impact

• Combining both checks reduces the clarity and effectiveness of the tool in identifying and prioritizing certificate-related issues.
• It may cause delays in addressing critical security risks associated with expired certificates.

Additional Information

• Repository: prowler
• Cloud Provider: AWS
• File Path: prowler/providers/aws/services/acm/acm_certificates_expiration_check/acm_certificates_expiration_check.py

Proposed Solution

To address this issue, I propose creating two separate checks with distinct metadata files:

1. Files:
   • acm_certificates_near_expiration_check.py
   • acm_certificates_expired_check.py
2. Metadata:
   • acm_certificates_near_expiration_check.json
   • acm_certificates_expired_check.json

Example Logs/Error Messages

Next Steps

1. Create separate rules for expired and near-expiration ACM certificates.
2. Implement the changes in a new branch.
3. Submit a pull request with the proposed changes for review.

[rubtoa]

+1 - i agree. These tests should have different severities.

[jfagoagas]

Hi @MrMoshkovitz @rubtoa what do you think about keeping just one check but modifying the severity regarding the scenario? I think there is no need for two checks since we can handle it there.

Thanks for using Prowler 🚀

[MrMoshkovitz]

Hi @jfagoagas I appreciate your reply. I get the idea to maintain a single check and adjust the level of severity according to the circumstances. Nonetheless, I think the following are good reasons to separate the checks:

Needs a Different Status:

Expired Certificates: Since the certificate has already expired and could pose a security risk, this situation calls for an urgent action level.
Near-Expiration Certificates: In order to facilitate proactive management prior to the certificate's actual expiration, this scenario calls for a separate status that indicates an impending issue.

Distinct Results:

Expired Certificates: This is a serious discovery that suggests a breach in security protocols and requires immediate attention.
Near-Expiration Certificates: This discovery makes it possible to take preventative measures like planning and mitigation before a security problem gets out of hand.
We can give clearer and more useful insights into the condition of ACM certificates by having distinct tests. This strategy guarantees that every kind of issue receives the proper amount of attention and urgency by adhering to best practices in compliance and monitoring.

I think this makes the purpose of having two separate checks clear. Kindly inform me if you have any more queries or worries.

Kind regards,
Mr. Moshkovitz

[jfagoagas]

Hi @jfagoagas I appreciate your reply. I get the idea to maintain a single check and adjust the level of severity according to the circumstances. Nonetheless, I think the following are good reasons to separate the checks:

Needs a Different Status:

Expired Certificates: Since the certificate has already expired and could pose a security risk, this situation calls for an urgent action level. Near-Expiration Certificates: In order to facilitate proactive management prior to the certificate's actual expiration, this scenario calls for a separate status that indicates an impending issue.

Distinct Results:

Expired Certificates: This is a serious discovery that suggests a breach in security protocols and requires immediate attention. Near-Expiration Certificates: This discovery makes it possible to take preventative measures like planning and mitigation before a security problem gets out of hand. We can give clearer and more useful insights into the condition of ACM certificates by having distinct tests. This strategy guarantees that every kind of issue receives the proper amount of attention and urgency by adhering to best practices in compliance and monitoring.

I think this makes the purpose of having two separate checks clear. Kindly inform me if you have any more queries or worries.

Kind regards, Mr. Moshkovitz

As I pointed in the PR #3967 (comment) I think the best way is to modify the current check and handle both behaviours, we can also modify the severity if the certificate is near expiration.

Also we need to adjust/create tests accordingly.

Thanks!

[sergargar]

@MrMoshkovitz, we can handle both cases in the same check by setting two different status extended with different severities, if the certificate is not expired yet, we can set a medium one with: report.check_metadata.Severity = "medium"

[MrMoshkovitz]

Thanks for the feedback, @jfagoagas @sergargar

I appreciate the suggestion for a single check with severity levels. While technically sound, I believe separate checks offer advantages in terms of:

• Clarity: Separate checks clearly distinguish critical expired certificates (requiring immediate attention) from near-expiring certificates (needing renewal soon).
• Maintainability: Dedicated checks improve code organization and future modifications.
• Flexibility: They allow for potential future features like tailored notifications.

Absolutely understandable, keeping the existing acm_certificates_expiration_check.py name to avoid breaking changes makes sense.

As a compromise, how about we modify the check's logic to handle both scenarios with differentiated severities and titles/descriptions? We can:

• Retain the check name: Keep it as acm_certificates_expiration_check.py for continuity.
• Update titles and descriptions: Craft clear titles and descriptions for expired and near-expiration scenarios within the check.
• Introduce new status: Implement a new status within the check to distinguish between the two.
• Assign severity levels: Set "high" severity for expired certificates and "medium" severity for near-expiration.

This approach would provide clear distinction between the two scenarios while maintaining compatibility with existing Prowler configurations.

I'm happy to modify the pull request accordingly. Please let me know if this sounds like a workable solution.

Best regards,

Mr. Moshkovitz

[BlackGox]

+1 - Separated checks would be much more helpful for prioritizing actions.

[sergargar]

@MrMoshkovitz at the moment, let's first keep the existing acm_certificates_expiration_check to avoid breaking changes and we think about decoupling it into two different checks in the future. As they are going to be two different findings with different status extended and severities, we can improve this situation. So, please, can you update the PR by updating the metadata and the severity in the check?
Let us know if you need any help from our side, thanks 😄 !

[MrMoshkovitz]

@sergargar
#3967

Updated - acm_certificates_expiration_check, acm_certificates_expired_check

[jfagoagas]

@sergargar #3967

Updated - acm_certificates_expiration_check, acm_certificates_expired_check

Hi @MrMoshkovitz, what @sergargar meant is just to leave the acm_certificates_expiration_check and handle both scenarios within the check with two different finding.status_extended and changing the severity within the check if needed.

[jfagoagas]

Hi @MrMoshkovitz did you see the above message?

Thanks!

[rieck-srlabs]

Chiming in here with a related point:

The current check does not consider if the certificates in question are actually InUse. I've seen a number of AWS accounts with a number of expired certificates that were not actually used.

Does it make sense to still report unused, expired certificates as a (high / medium) issue, or should the check basically ignore such certificates? Happy to hear from you here @jfagoagas @MrMoshkovitz !