feat(acm): Add new check for insecure algorithms in certificates #4551

MarioRgzLpz · 2024-07-26T08:04:01Z

Context

In order to enhance the security measures and ensure compliance with best practices in cryptographic operations, we are introducing a new check within the AWS Certificate Manager (ACM) framework. This new check specifically focuses on verifying that the algorithm used in the certificate is secure enough. By default it will check that RSA_1024 is not used as it is not considered a best practice. Algorithms chosen as insecure can be configured by the user to allow them to harden the certificates.

Description

I added acm_certificates_with_secure_key_algorithms check with his respective unit test. In order to do the check I modified the acm_service model Certificate adding the key_algorithm as str. I also modified the config.yaml to add the new insecure_algorithms parameter.

License

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

jfagoagas · 2024-07-26T11:31:02Z

...roviders/aws/services/acm/acm_certificates_rsa_key_length/acm_certificates_rsa_key_length.py

+            report.status = "PASS"
+            report.status_extended = f"ACM Certificate {certificate.id} for {certificate.name} meet minimum key size requirements."
+
+            if certificate.key_algorithm == "RSA_1024":


Just a comment about this, we should be able to configure the list of algorithms that are not secure enough for this check to pass.

I did not reviewed the PR but took a quick look at it. Great work 👏

added in commit 4d9d8f3

Maybe I was not clear enough. My point was to be able to configure the list of algorithms using Prowler configuration. You can see how could it be implemented at https://docs.prowler.com/projects/prowler-open-source/en/latest/developer-guide/checks/#using-the-audit-config

Thought about it at first but I wasn't sure if that was what you was asking about. Added in commit c8d3954

codecov · 2024-07-26T11:54:28Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.07%. Comparing base (6c3e451) to head (cf645a7).
Report is 523 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4551      +/-   ##
==========================================
- Coverage   89.08%   89.07%   -0.01%     
==========================================
  Files         910      911       +1     
  Lines       27741    27763      +22     
==========================================
+ Hits        24713    24731      +18     
- Misses       3028     3032       +4

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

…y RSA_1024

MrCloudSec · 2024-07-26T14:14:04Z

...s/services/acm/acm_certificates_rsa_key_length/acm_certificates_rsa_key_length.metadata.json

+  "ResourceType": "AwsCertificateManagerCertificate",
+  "Description": "Check if ACM Certificates use a secure RSA key size",
+  "Risk": "Certificates with weak RSA keys can be compromised.",
+  "RelatedUrl": "https://docs.aws.amazon.com/es_es/acm/latest/userguide/acm-certificate.html",


Please, can you add here the Security Hub link?
https://docs.aws.amazon.com/securityhub/latest/userguide/acm-controls.html#acm-2

Added in commit 74786c7

MrCloudSec · 2024-07-26T14:14:24Z

...s/services/acm/acm_certificates_rsa_key_length/acm_certificates_rsa_key_length.metadata.json

+    },
+    "Recommendation": {
+      "Text": "Ensure that all ACM certificates use RSA keys of at least 2048 bits. If any certificates use smaller keys, regenerate them with a secure key size and update any systems that rely on these certificates.",
+      "Url": "https://docs.aws.amazon.com/es_es/acm/latest/userguide/acm-certificate.html"


Here too, since the SecurityHub link also contains the Remediation.

Added in commit 74786c7

jfagoagas · 2024-07-29T07:37:09Z

What if we create a generic check regardless the algorithm? So we can use it for RSA, ECDSA or whatever.

@sergargar @puchy22 @MarioRgzLpz

puchy22 · 2024-07-29T07:48:22Z

What if we create a generic check regardless the algorithm? So we can use it for RSA, ECDSA or whatever.

@sergargar @puchy22 @MarioRgzLpz

All other algorithms allowed by AWS are secure, except for RSA 1024 bits, which is not considered a best practice. Creating a generic check is a good idea, as it will future-proof the system by allowing for the inclusion of new algorithms that may fall out of best practice over time.

… being RSA_1024

…e conflicts

…ength-in-certificates

MrCloudSec · 2024-07-29T15:31:03Z

...ws/services/acm/acm_certificates_insecure_algorithms/acm_certificates_insecure_algorithms.py

+            report.resource_tags = certificate.tags
+
+            report.status = "PASS"
+            report.status_extended = f"ACM Certificate {certificate.id} for {certificate.name} meet minimum key size requirements."


Suggested change

report.status_extended = f"ACM Certificate {certificate.id} for {certificate.name} meet minimum key size requirements."

report.status_extended = f"ACM Certificate {certificate.id} for {certificate.name} uses a secure key algorithm."

MrCloudSec · 2024-07-29T15:32:35Z

.../acm/acm_certificates_insecure_algorithms/acm_certificates_insecure_algorithms.metadata.json

+{
+  "Provider": "aws",
+  "CheckID": "acm_certificates_insecure_algorithms",
+  "CheckTitle": "Check if ACM Certificates use a secure RSA key size",


Suggested change

"CheckTitle": "Check if ACM Certificates use a secure RSA key size",

"CheckTitle": "Check if ACM Certificates use a secure key algorithm",

MrCloudSec · 2024-07-29T15:32:40Z

.../acm/acm_certificates_insecure_algorithms/acm_certificates_insecure_algorithms.metadata.json

+  "ResourceIdTemplate": "arn:partition:acm:region:account-id:certificate/resource-id",
+  "Severity": "high",
+  "ResourceType": "AwsCertificateManagerCertificate",
+  "Description": "Check if ACM Certificates use a secure RSA key size",


Suggested change

"Description": "Check if ACM Certificates use a secure RSA key size",

"Description": "Check if ACM Certificates use a secure key algorithm",

Can you elaborate it more? Mention for example what is considered to be a secure key algorithm :)

MrCloudSec · 2024-07-29T15:33:18Z

...ws/services/acm/acm_certificates_insecure_algorithms/acm_certificates_insecure_algorithms.py

+                "insecure_algorithms", ["RSA_1024"]
+            ):
+                report.status = "FAIL"
+                report.status_extended = f"ACM Certificate {certificate.id} for {certificate.name} uses a not secure key length."


Suggested change

report.status_extended = f"ACM Certificate {certificate.id} for {certificate.name} uses a not secure key length."

report.status_extended = f"ACM Certificate {certificate.id} for {certificate.name} does not use a secure key algorithm."

MrCloudSec · 2024-07-29T15:38:59Z

.../acm/acm_certificates_insecure_algorithms/acm_certificates_insecure_algorithms.metadata.json

@@ -0,0 +1,32 @@
+{
+  "Provider": "aws",
+  "CheckID": "acm_certificates_insecure_algorithms",


Suggested change

"CheckID": "acm_certificates_insecure_algorithms",

"CheckID": "acm_certificates_with_secure_key_algorithms",

MrCloudSec · 2024-07-29T15:39:51Z

.../acm/acm_certificates_insecure_algorithms/acm_certificates_insecure_algorithms.metadata.json

+  "ResourceType": "AwsCertificateManagerCertificate",
+  "Description": "Check if ACM Certificates use a secure RSA key size",
+  "Risk": "Certificates with weak RSA keys can be compromised.",
+  "RelatedUrl": "https://docs.aws.amazon.com/securityhub/latest/userguide/acm-controls.html#acm-2",


Suggested change

"RelatedUrl": "https://docs.aws.amazon.com/securityhub/latest/userguide/acm-controls.html#acm-2",

"RelatedUrl": "https://docs.aws.amazon.com/acm/latest/userguide/acm-certificate.html",

MrCloudSec

@MarioRgzLpz please, review my last comments. Thanks!

… description to make more sense with the check.

MarioRgzLpz · 2024-07-30T07:33:10Z

@sergargar All the changes you requested done in commit 42829a3. I changed the Recommendation URL to the AWS one that guides you on how to change the algorithm on a certificate.

MrCloudSec · 2024-07-30T11:30:49Z

...ficates_with_secure_key_algorithms/acm_certificates_with_secure_key_algorithms.metadata.json

+    },
+    "Recommendation": {
+      "Text": "Ensure that all ACM certificates use a secure key algorithm. If any certificates use smaller keys, regenerate them with a secure key size and update any systems that rely on these certificates.",
+      "Url": "https://docs.aws.amazon.com/acm/latest/userguide/gs.html"


Thanks for the changes @MarioRgzLpz ! However, I prefer the SecurityHub link here since we can map the check to the Security Hub one, and in that link there is the actual remediation of the check.

Allright added in commit ca090d8

puchy22

Please also change the naming convention for ACM methods to follow the standard naming convention.

puchy22 · 2024-07-31T07:55:47Z

...ficates_with_secure_key_algorithms/acm_certificates_with_secure_key_algorithms.metadata.json

+  "Severity": "high",
+  "ResourceType": "AwsCertificateManagerCertificate",
+  "Description": "Check if ACM Certificates use a secure key algorithm (RSA 2048 bits or more, or ECDSA 256 bits or more). For example certificates that use RSA_1024 can be compromised.",
+  "Risk": "Certificates with weak RSA or ECDSA keys can be compromised.",


Could you elaborate a bit on this information to make it clear why the certificates could be compromised

puchy22 · 2024-07-31T07:59:06Z

tests/config/config_test.py

@@ -262,6 +262,9 @@ def mock_prowler_get_latest_release(_, **kwargs):
    ],
    "check_rds_instance_replicas": False,
    "days_to_expire_threshold": 7,
+    "insecure_algorithms": [


Suggested change

"insecure_algorithms": [

"insecure_key_algorithms": [

puchy22 · 2024-07-31T07:59:21Z

tests/config/fixtures/config.yaml

@@ -271,6 +271,11 @@ aws:
  # AWS ACM Configuration
  # aws.acm_certificates_expiration_check
  days_to_expire_threshold: 7
+  # aws.acm_certificates_rsa_key_length
+  insecure_algorithms:


Suggested change

insecure_algorithms:

insecure_key_algorithms:

…e config, add some details in the description and risk in the metadata and rename the service methods to comply with Python convention

MarioRgzLpz · 2024-07-31T09:18:20Z

@puchy22 Everything you requested done in commit ee08174

MrCloudSec

👏🏼

puchy22

Great job!!🥇

puchy22

Please add the logic of unused resources to the check with unused certificates.

…gorithms including ECDSA and all key size RSA

…pped KeyAlgorithm_Size to KeyAlgorithm-Size as this is the format in which certificates are returned

…-for-rsa-key-length-in-certificates

MarioRgzLpz · 2024-08-05T08:26:37Z

Please add the logic of unused resources to the check with unused certificates.

@puchy22 added in commit 9697ead

MrCloudSec · 2024-08-05T20:24:58Z

...m/acm_certificates_with_secure_key_algorithms/acm_certificates_with_secure_key_algorithms.py

+                report.resource_tags = certificate.tags
+
+                report.status = "PASS"
+                report.status_extended = f"ACM Certificate {certificate.id} for {certificate.name} uses a secure key algorithm."


Can you add in the status extended the key algorithm at the end within parenthesis, please?

MrCloudSec · 2024-08-05T20:25:04Z

...m/acm_certificates_with_secure_key_algorithms/acm_certificates_with_secure_key_algorithms.py

+                    "insecure_algorithms", ["RSA-1024"]
+                ):
+                    report.status = "FAIL"
+                    report.status_extended = f"ACM Certificate {certificate.id} for {certificate.name} does not use a secure key algorithm."


…-for-rsa-key-length-in-certificates

…audit_config key format in the tests

MarioRgzLpz · 2024-08-07T07:38:47Z

@sergargar All done here cf645a7

MrCloudSec

Thanks for the PR!

puchy22

Good job @MarioRgzLpz !! 🥇

MarioRgzLpz added 2 commits July 26, 2024 09:53

chore(acm): Add RSA key length check

c3ff8c3

test(acm): Add RSA key length check tests

52a4224

MarioRgzLpz requested review from a team as code owners July 26, 2024 08:04

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

jfagoagas reviewed Jul 26, 2024

View reviewed changes

test(acm): Add the new atributte key_algorithm to old tests

8ea0807

feat(acm): Add a list of insecure algorithms instead of verifying onl…

4d9d8f3

…y RSA_1024

MrCloudSec reviewed Jul 26, 2024

View reviewed changes

MarioRgzLpz added 3 commits July 29, 2024 09:51

fix(acm): Change the URLs for the SecurityHub ones

74786c7

feat(acm): Make the insecure algorithm a configurable list by default…

c8d3954

… being RSA_1024

refactor(acm): I change the check name to a more suitable one

e1da46b

MarioRgzLpz changed the title ~~feat(acm): Add new check for RSA key length in certificates~~ feat(acm): Add new check for insecure algorithms in certificates Jul 29, 2024

MarioRgzLpz and others added 2 commits July 29, 2024 11:17

fix(acm): Add the eks_rquire_log_types in config_test to resolve merg…

49dae85

…e conflicts

Merge branch 'master' into PRWLR-4239-add-new-acm-check-for-rsa-key-l…

b9e698f

…ength-in-certificates

MrCloudSec reviewed Jul 29, 2024

View reviewed changes

MrCloudSec requested changes Jul 29, 2024

View reviewed changes

MarioRgzLpz added 2 commits July 30, 2024 09:12

refactor(acm): I change the name of the check and some texts like the…

42829a3

… description to make more sense with the check.

Merge branch

629ecff

MrCloudSec reviewed Jul 30, 2024

View reviewed changes

chore(acm): Change the recommendation url to the SecurityHub one

ca090d8

puchy22 requested changes Jul 31, 2024

View reviewed changes

chore(acm): Change insecure_algorithm to insecure_key_algorithm in th…

ee08174

…e config, add some details in the description and risk in the metadata and rename the service methods to comply with Python convention

MrCloudSec previously approved these changes Jul 31, 2024

View reviewed changes

puchy22 approved these changes Jul 31, 2024

View reviewed changes

puchy22 requested changes Jul 31, 2024

View reviewed changes

jfagoagas added the no-merge Please, DO NOT MERGE this PR. label Aug 1, 2024

MrCloudSec removed the no-merge Please, DO NOT MERGE this PR. label Aug 1, 2024

MarioRgzLpz added 2 commits August 5, 2024 09:22

chore(acm): Change method _list_certificates so it returns all key al…

9546adf

…gorithms including ECDSA and all key size RSA

chore(acm): Added logic for checking only in use certificates and swa…

9697ead

…pped KeyAlgorithm_Size to KeyAlgorithm-Size as this is the format in which certificates are returned

MarioRgzLpz dismissed MrCloudSec’s stale review via 9697ead August 5, 2024 07:35

Merge branch 'prowler-cloud:master' into PRWLR-4239-add-new-acm-check…

e797248

…-for-rsa-key-length-in-certificates

MrCloudSec reviewed Aug 5, 2024

View reviewed changes

MarioRgzLpz and others added 2 commits August 7, 2024 09:10

Merge branch 'prowler-cloud:master' into PRWLR-4239-add-new-acm-check…

b4dafb0

…-for-rsa-key-length-in-certificates

chore(acm): Add the key algorithm in the status extended and fix the …

cf645a7

…audit_config key format in the tests

MrCloudSec approved these changes Aug 7, 2024

View reviewed changes

MrCloudSec requested a review from puchy22 August 7, 2024 11:39

puchy22 approved these changes Aug 7, 2024

View reviewed changes

MrCloudSec merged commit 1b18aef into prowler-cloud:master Aug 7, 2024
11 checks passed

	report.status_extended = f"ACM Certificate {certificate.id} for {certificate.name} meet minimum key size requirements."
	report.status_extended = f"ACM Certificate {certificate.id} for {certificate.name} uses a secure key algorithm."

	"CheckTitle": "Check if ACM Certificates use a secure RSA key size",
	"CheckTitle": "Check if ACM Certificates use a secure key algorithm",

	"Description": "Check if ACM Certificates use a secure RSA key size",
	"Description": "Check if ACM Certificates use a secure key algorithm",

	"CheckID": "acm_certificates_insecure_algorithms",
	"CheckID": "acm_certificates_with_secure_key_algorithms",

	"RelatedUrl": "https://docs.aws.amazon.com/securityhub/latest/userguide/acm-controls.html#acm-2",
	"RelatedUrl": "https://docs.aws.amazon.com/acm/latest/userguide/acm-certificate.html",

feat(acm): Add new check for insecure algorithms in certificates #4551

feat(acm): Add new check for insecure algorithms in certificates #4551

Conversation

MarioRgzLpz commented Jul 26, 2024 • edited Loading

Context

Description

License

jfagoagas Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 26, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfagoagas commented Jul 29, 2024

puchy22 commented Jul 29, 2024 • edited Loading

MrCloudSec Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

MrCloudSec Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

MrCloudSec Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrCloudSec Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrCloudSec left a comment

Choose a reason for hiding this comment

MarioRgzLpz commented Jul 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

puchy22 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarioRgzLpz commented Jul 31, 2024

MrCloudSec left a comment

Choose a reason for hiding this comment

puchy22 left a comment

Choose a reason for hiding this comment

puchy22 left a comment

Choose a reason for hiding this comment

MarioRgzLpz commented Aug 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarioRgzLpz commented Aug 7, 2024

MrCloudSec left a comment

Choose a reason for hiding this comment

puchy22 left a comment

Choose a reason for hiding this comment

MarioRgzLpz commented Jul 26, 2024 •

edited

Loading

jfagoagas Jul 26, 2024 •

edited

Loading

codecov bot commented Jul 26, 2024 •

edited

Loading

puchy22 commented Jul 29, 2024 •

edited

Loading

MrCloudSec Jul 29, 2024 •

edited

Loading

MrCloudSec Jul 29, 2024 •

edited

Loading

MrCloudSec Jul 29, 2024 •

edited

Loading

MrCloudSec Jul 29, 2024 •

edited

Loading

puchy22 left a comment •

edited

Loading