-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[Bug]: Separate Checks for Expired and Near-Expiration ACM Certificates #3966
Comments
+1 - i agree. These tests should have different severities. |
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 🚀 |
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. Distinct Results:Expired Certificates: This is a serious discovery that suggests a breach in security protocols and requires immediate attention. 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, |
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! |
@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: |
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:
Absolutely understandable, keeping the existing As a compromise, how about we modify the check's logic to handle both scenarios with differentiated severities and titles/descriptions? We can:
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 |
+1 - Separated checks would be much more helpful for prioritizing actions. |
@MrMoshkovitz at the moment, let's first keep the existing |
Updated - acm_certificates_expiration_check, acm_certificates_expired_check |
Hi @MrMoshkovitz, what @sergargar meant is just to leave the |
Hi @MrMoshkovitz did you see the above message? Thanks! |
Chiming in here with a related point: The current check does not consider if the certificates in question are actually 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 ! |
Good point, this is a great example to add the https://docs.prowler.com/projects/prowler-open-source/en/latest/tutorials/scan-unused-services/ logic to the check, so by default not used won't be audited but could be using the |
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
acm_certificates_expiration_check.py
file.Expected behavior
Actual Result with Screenshots or Logs
How did you install Prowler?
From pip package (pip install prowler)
Environment Resource
OS used
Prowler version
14
Pip version
latest
Context
Impact
Additional Information
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:
acm_certificates_near_expiration_check.py
acm_certificates_expired_check.py
acm_certificates_near_expiration_check.json
acm_certificates_expired_check.json
Example Logs/Error Messages
Next Steps
The text was updated successfully, but these errors were encountered: