feat(sns): sns topics no http subscriptions #4095

Davidm4r · 2024-05-27T18:22:50Z

Context

Create a new check for the SNS service in the provider AWS

sns_subscription_not_using_http_endpoints

Description

This change ensures that AWS SNS topics are not subscribed to HTTP endpoints

Add a new type of Object for SNS, the Subscription.

License

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

sergargar · 2024-05-27T20:00:02Z

...viders/aws/services/sns/sns_topics_no_http_subscriptions/sns_topics_no_http_subscriptions.py

+                if subscription.SubscriptionArn == "PendingConfirmation":
+                    continue


You are already checking this in the service, right?

I've moved the PendingConfirmation logic to the check. I believe that all subscriptions, including those that are pending, should be included in the object. It should be up to the check to decide if it wants to do something with those types of subscriptions.

Great idea!

sergargar · 2024-05-27T20:00:26Z

prowler/providers/aws/services/sns/sns_service.py

+    SubscriptionArn: str
+    Owner: str
+    Protocol: str
+    Endpoint: str
+    TopicArn: str


Suggested change

SubscriptionArn: str

Owner: str

Protocol: str

Endpoint: str

TopicArn: str

arn: str

owner: str

protocol: str

endpoint: str

Please, use the pattern that we have for the rest of the classes and you don't need the TopicArn since it is already attached to a Topic class.

Good suggestion. Fixed!

sergargar · 2024-05-27T20:02:10Z

prowler/providers/aws/services/sns/sns_service.py

@@ -74,6 +75,39 @@ def __list_tags_for_resource__(self):
                f"{regional_client.region} -- {error.__class__.__name__}[{error.__traceback__.tb_lineno}]: {error}"
            )

+    def __get_subscriptions__(self):


Suggested change

def __get_subscriptions__(self):

def __list_subscriptions_by_topic__(self):

sergargar · 2024-05-27T20:02:22Z

prowler/providers/aws/services/sns/sns_service.py

@@ -74,6 +75,39 @@ def __list_tags_for_resource__(self):
                f"{regional_client.region} -- {error.__class__.__name__}[{error.__traceback__.tb_lineno}]: {error}"
            )

+    def __get_subscriptions__(self):
+        logger.info("SNS - Getting subscriptions...")


Suggested change

logger.info("SNS - Getting subscriptions...")

logger.info("SNS - Listing subscriptions by topic...")

sergargar

Thanks for the check @Davidm4r ! Please, review my comments.

jfagoagas

Thanks for the check @Davidm4r, I also left some comments regarding to the resource checked.

jfagoagas · 2024-05-28T06:27:26Z

...viders/aws/services/sns/sns_topics_no_http_subscriptions/sns_topics_no_http_subscriptions.py

+                report.resource_id = topic.name
+                report.resource_arn = topic.arn
+                report.resource_tags = topic.tags


What is the resource checked within the check? If it is the subscription, we need to modify the check name to something like sns_subscriptions_not_using_http_endpoints or similar and set the subscription as resource_id and resource_arn.

I put the subscription.arn as resource_id and resource_arn

jfagoagas · 2024-05-28T06:27:45Z

...viders/aws/services/sns/sns_topics_no_http_subscriptions/sns_topics_no_http_subscriptions.py

+                report.resource_tags = topic.tags
+                report.status = "PASS"
+                report.status_extended = (
+                    f"Subscription {subscription.SubscriptionArn} is HTTPS."


Suggested change

f"Subscription {subscription.SubscriptionArn} is HTTPS."

f"Subscription {subscription.SubscriptionArn} is using an HTTPS endpoint."

jfagoagas · 2024-05-28T06:28:02Z

...viders/aws/services/sns/sns_topics_no_http_subscriptions/sns_topics_no_http_subscriptions.py

+                if subscription.Protocol == "http":
+                    report.status = "FAIL"
+                    report.status_extended = (
+                        f"Subscription {subscription.SubscriptionArn} is HTTP."


Suggested change

f"Subscription {subscription.SubscriptionArn} is HTTP."

f"Subscription {subscription.SubscriptionArn} is using an HTTP endpoint."

jfagoagas · 2024-05-28T09:50:30Z

Forgot to say that we need to update service's test with the new function.

codecov · 2024-05-28T10:08:49Z

Codecov Report

Attention: Patch coverage is 90.69767% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 86.31%. Comparing base (d51fd0e) to head (2ee387b).
Report is 15 commits behind head on master.

Files	Patch %	Lines
prowler/providers/aws/services/sns/sns_service.py	80.95%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4095      +/-   ##
==========================================
- Coverage   86.39%   86.31%   -0.09%     
==========================================
  Files         794      795       +1     
  Lines       24874    24917      +43     
==========================================
+ Hits        21491    21508      +17     
- Misses       3383     3409      +26

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

Davidm4r · 2024-05-28T15:12:14Z

Forgot to say that we need to update service's test with the new function.

I created a new test for the service.

Davidm4r · 2024-05-28T15:13:20Z

@sergargar @jfagoagas I have reviewed all your comments. Can you check it again?

jfagoagas

🔝

feat(sns): sns topics no http subscriptions

a8e8cd7

Davidm4r requested review from a team May 27, 2024 18:22

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

Davidm4r added 2 commits May 27, 2024 20:32

forgot linter

022c525

E262 inline comment should start with '# '

dc97d73

sergargar reviewed May 27, 2024

View reviewed changes

sergargar requested changes May 27, 2024

View reviewed changes

jfagoagas requested changes May 28, 2024

View reviewed changes

Fix suggestions

e44da6c

Davidm4r added 2 commits May 28, 2024 16:55

Move PendingConfirmation logic to the check

bf1ca22

Add test for the service

43d9cad

Davidm4r requested review from jfagoagas and sergargar May 28, 2024 15:19

change check name

2ee387b

jfagoagas approved these changes May 28, 2024

View reviewed changes

sergargar approved these changes May 28, 2024

View reviewed changes

sergargar merged commit 98b7df6 into prowler-cloud:master May 28, 2024
10 of 11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sns): sns topics no http subscriptions #4095

feat(sns): sns topics no http subscriptions #4095

Davidm4r commented May 27, 2024 •

edited by sergargar

Loading

sergargar May 27, 2024

Davidm4r May 28, 2024 •

edited

Loading

sergargar May 28, 2024

sergargar May 27, 2024

sergargar May 27, 2024

Davidm4r May 28, 2024

sergargar May 27, 2024

Davidm4r May 28, 2024

sergargar May 27, 2024

Davidm4r May 28, 2024

sergargar left a comment

jfagoagas left a comment

jfagoagas May 28, 2024

Davidm4r May 28, 2024

jfagoagas May 28, 2024

Davidm4r May 28, 2024

jfagoagas May 28, 2024

Davidm4r May 28, 2024

jfagoagas commented May 28, 2024

codecov bot commented May 28, 2024 •

edited

Loading

Davidm4r commented May 28, 2024

Davidm4r commented May 28, 2024

jfagoagas left a comment

		if subscription.SubscriptionArn == "PendingConfirmation":
		continue

	def __get_subscriptions__(self):
	def __list_subscriptions_by_topic__(self):

	logger.info("SNS - Getting subscriptions...")
	logger.info("SNS - Listing subscriptions by topic...")

	f"Subscription {subscription.SubscriptionArn} is HTTPS."
	f"Subscription {subscription.SubscriptionArn} is using an HTTPS endpoint."

feat(sns): sns topics no http subscriptions #4095

feat(sns): sns topics no http subscriptions #4095

Conversation

Davidm4r commented May 27, 2024 • edited by sergargar Loading

Context

Description

License

Choose a reason for hiding this comment

Davidm4r May 28, 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

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

Choose a reason for hiding this comment

sergargar left a comment

Choose a reason for hiding this comment

jfagoagas left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfagoagas commented May 28, 2024

codecov bot commented May 28, 2024 • edited Loading

Codecov Report

Davidm4r commented May 28, 2024

Davidm4r commented May 28, 2024

jfagoagas left a comment

Choose a reason for hiding this comment

Davidm4r commented May 27, 2024 •

edited by sergargar

Loading

Davidm4r May 28, 2024 •

edited

Loading

codecov bot commented May 28, 2024 •

edited

Loading