feat(azure): New azure monitor check `monitor_ensure_diagnostic_setting_appropriate` #3421

Hugo966 · 2024-02-20T13:42:00Z

Context

New azure monitor check monitor_ensure_diagnostic_setting_appropriate

Description

New monitor check ensuring diagnostics setting are set correctly

License

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

codecov · 2024-02-20T13:53:52Z

Codecov Report

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

Project coverage is 85.72%. Comparing base (587b8af) to head (6af2c37).

Files	Patch %	Lines
...roviders/azure/services/monitor/monitor_service.py	73.07%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3421      +/-   ##
==========================================
- Coverage   85.73%   85.72%   -0.02%     
==========================================
  Files         634      637       +3     
  Lines       20007    20069      +62     
==========================================
+ Hits        17154    17204      +50     
- Misses       2853     2865      +12

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

sergargar · 2024-02-21T11:08:50Z

...ure_diagnostic_setting_appropiate/monitor_ensure_diagnostic_setting_appropiate.metadata.json

+  "RelatedUrl": "https://learn.microsoft.com/en-us/azure/azure-monitor/essentials/diagnostic-settings",
+  "Remediation": {
+    "Code": {
+      "CLI": "az monitor diagnostic-settings subscription list --subscription <subscription ID>",


Suggested change

"CLI": "az monitor diagnostic-settings subscription list --subscription <subscription ID>",

"CLI": "",

can you put the one in TrendMicro, please?

jfagoagas

Great one @Hugo966 !!

I left you some comments to clarify some parts of the code. Thanks!

jfagoagas · 2024-02-21T11:06:10Z

...ure_diagnostic_setting_appropiate/monitor_ensure_diagnostic_setting_appropiate.metadata.json

+    },
+    "Recommendation": {
+      "Text": "1. Go to Azure Monitor 2. Click Activity log 3. Click on Export Activity Logs 4. Select the Subscription from the drop down menu 5. Click on Add diagnostic setting 6. Enter a name for your new Diagnostic Setting 7. Check the following categories: Administrative, Alert, Policy, and Security 8. Choose the destination details according to your organization's needs.",
+      "Url": ""


You have to always fill this value here. Use a link from the official documentation.

jfagoagas · 2024-02-21T11:07:09Z

prowler/providers/azure/services/monitor/monitor_service.py

+        # self.activity_logs = self.__get_activity_logs__()
+        # self.activity_log_alerts = self.__get_activity_log_alerts__()


If this is not needed, please remove it from the service.

jfagoagas · 2024-02-21T11:07:23Z

prowler/providers/azure/services/monitor/monitor_service.py

+    # def __get_activity_logs__(self):
+    #    logger.info("Monitor - Getting activity logs...")
+    #    activity_logs = {}
+    #    for subscription, client in self.clients.items():
+    #        try:
+    #            activity_logs.update({subscription: []})
+    #            start_time = datetime.datetime.now() - datetime.timedelta(days=90)
+    #            end_time = datetime.datetime.now()
+    #            logs = client.activity_logs.list(
+    #                filter=f"eventTimestamp ge {start_time.isoformat()} and eventTimestamp le {end_time.isoformat()}"
+    #            )
+    #            for log in logs:
+    #                activity_logs[subscription].append(
+    #                    ActivityLog(
+    #                        id=log.id,
+    #                        resource_group=log.resource_group_name,
+    #                        resource_id=log.resource_id,
+    #                        operation_name=log.operation_name,
+    #                        status=log.status,
+    #                        event_timestamp=log.event_timestamp,
+    #                        caller=log.caller,
+    #                        correlation_id=log.correlation_id,
+    #                        category=log.category,
+    #                        level=log.level,
+    #                        event_data_id=log.event_data_id,
+    #                        sub_status=log.sub_status,
+    #                    )
+    #                )
+    #        except Exception as error:
+    #            logger.error(
+    #                f"Subscription name: {subscription} -- {error.__class__.__name__}[{error.__traceback__.tb_lineno}]: {error}"
+    #            )
+    #    #print(activity_logs)
+    #    print("HOLA")
+    #    return activity_logs
+
+    # def __get_activity_log_alerts__(self):
+    #    logger.info("Monitor - Getting activity log alerts...")
+    #    activity_log_alerts = {}
+    #    for subscription, client in self.clients.items():
+    #        try:
+    #            activity_log_alerts.update({subscription: []})
+    #            alerts = client.activity_log_alerts.list_by_subscription_id()
+    #            for alert in alerts:
+    #                print(alert.condition.all_of[0].equals[0].value)
+    #                activity_log_alerts[subscription].append(
+    #                    ActivityLogAlert(
+    #                        id=alert.id,
+    #                        name=alert.name,
+    #                        description=alert.description,
+    #                        enabled=alert.enabled,
+    #                        scopes=alert.scopes,
+    #                        condition=alert.condition,
+    #                        actions=alert.actions,
+    #                        last_updated_time=alert.last_updated_time,
+    #                        provisioning_state=alert.provisioning_state,
+    #                    )
+    #                )
+    #        except Exception as error:
+    #            logger.error(
+    #                f"Subscription name: {subscription} -- {error.__class__.__name__}[{error.__traceback__.tb_lineno}]: {error}"
+    #            )
+    #    return activity_log_alerts
+
+
+# @dataclass
+# class ActivityLog:
+#    id: str
+#    resource_group: str
+#    resource_id: str
+#    operation_name: str
+#    status: str
+#    event_timestamp: str
+#    caller: str
+#    correlation_id: str
+#    category: str
+#    level: str
+#    event_data_id: str
+#    sub_status: str


The same again.

jfagoagas · 2024-02-21T11:07:32Z

prowler/providers/azure/services/monitor/monitor_service.py

+# @dataclass
+# class ActivityLogAlert:
+#    id: str
+#    name: str
+#    description: str
+#    enabled: bool
+#    scopes: list
+#    condition: dict
+#    actions: list
+#    last_updated_time: str
+#    provisioning_state: str


Suggested change

# @dataclass

# class ActivityLogAlert:

# id: str

# name: str

# description: str

# enabled: bool

# scopes: list

# condition: dict

# actions: list

# last_updated_time: str

# provisioning_state: str

jfagoagas · 2024-02-21T11:07:43Z

prowler/providers/azure/services/monitor/monitor_service.py

+#    sub_status: str
+
+
+@dataclass


Why are you not using Pydantic here?

Because of the types, logs is a list that consists of 2 strings, a boolean and a RetentionPolicy that consists of an integer and a boolean, when I structure this using Pydantic somehow Python doesn't get the data correctly and execution fails.

Can you set the type for the logs? Like list[WhateverIsNamedInAzureMonitor]

That is what I tried, but Python wasn't getting the types correctly. This is what I did:

class RetentionPolicy(BaseModel):
enabled: bool
days: int

class LogSettings(BaseModel):
category: str
category_group: str
enabled: bool
retention_policy: RetentionPolicy

class DiagnosticSetting(BaseModel):
id: str
logs: List[LogSettings]

But you only have the DiagnosticSetting and logs: list, is this right? I mean that the LogSettings import is not present anymore.

That's correct, that was the old version trying to use Pydantic, the new one is:

from azure.mgmt.monitor.models import LogSettings from pydantic import BaseModel class DiagnosticSetting(BaseModel): id: str logs: LogSettings

That's good, push that change then!

jfagoagas · 2024-02-21T11:09:09Z

prowler/providers/azure/services/monitor/monitor_service.py

+                        DiagnosticSetting(
+                            id=setting.id,
+                            name=setting.name,
+                            type=setting.type,
+                            event_hub_authorization_rule_id=setting.event_hub_authorization_rule_id,
+                            event_hub_name=setting.event_hub_name,
+                            metrics=setting.metrics,
+                            logs=setting.logs,
+                            workspace_id=setting.workspace_id,
+                            storage_account_id=setting.storage_account_id,
+                            service_bus_rule_id=setting.service_bus_rule_id,
+                            marketplace_partner_id=setting.marketplace_partner_id,
+                            log_analytics_destination_type=setting.log_analytics_destination_type,


Are you sure that all of these values are always present?

Do you need all of these values for the check?

These values can be None, but I'll let only the only ones that I need for the check.
I though I should include all of them even if I don't need them.

Always try to only include the attributes you need for the checks you are coding, keep it simple.

jfagoagas · 2024-02-21T11:09:57Z

...monitor_ensure_diagnostic_setting_appropiate/monitor_ensure_diagnostic_setting_appropiate.py

+                    diagnostic_setting.logs[0].enabled
+                    and diagnostic_setting.logs[1].enabled
+                    and diagnostic_setting.logs[3].enabled
+                    and diagnostic_setting.logs[5].enabled


Why are you checking items 0, 1, 3 and 5 here? What is the reason behind?

Im iterating through the logs and checking if the ones asked to be enabled ('Administrative','Security','Alert' and 'Policy') are enabled. Those numbers correspond to the order that those categories follow in the Azure Portal.
However, Im going to change it and upgrade it by checking also the category name so that way is guaranteed that even if more categories are set in the future, the code will still work.

Perfect, thanks!

jfagoagas · 2024-02-21T11:10:23Z

tests/providers/azure/services/monitor/monitor_service_test.py

+                    mock.MagicMock(enabled=True),
+                    mock.MagicMock(enabled=True),
+                    mock.MagicMock(enabled=True),
+                    mock.MagicMock(enabled=True),
+                    mock.MagicMock(enabled=False),
+                    mock.MagicMock(enabled=True),
+                    mock.MagicMock(enabled=False),
+                    mock.MagicMock(enabled=False),


What is the meaning of this?

Creating a valid object for testing my check, with the new code, its also creates the category for each one.

sergargar · 2024-02-21T11:13:43Z

...ure_diagnostic_setting_appropiate/monitor_ensure_diagnostic_setting_appropiate.metadata.json

+  "Categories": [],
+  "DependsOn": [],
+  "RelatedTo": [],
+  "Notes": ""


Suggested change

"Notes": ""

"Notes": "When the diagnostic setting is created using Azure Portal, by default no categories are selected."

sergargar · 2024-02-21T11:17:20Z

...ure_diagnostic_setting_appropiate/monitor_ensure_diagnostic_setting_appropiate.metadata.json

@@ -0,0 +1,30 @@
+{
+  "Provider": "azure",
+  "CheckID": "monitor_ensure_diagnostic_setting_appropiate",


Suggested change

"CheckID": "monitor_ensure_diagnostic_setting_appropiate",

"CheckID": "monitor_diagnostic_setting_with_appropriate_categories",

toniblyx

there is a typo, it is appropriate not appropiate

sergargar · 2024-02-22T10:08:44Z

..._appropriate_categories/monitor_diagnostic_setting_with_appropriate_categories.metadata.json

+  "Categories": [],
+  "DependsOn": [],
+  "RelatedTo": [],
+  "Notes": ""


Suggested change

"Notes": ""

"Notes": "When the diagnostic setting is created using Azure Portal, by default no categories are selected."

..._appropriate_categories/monitor_diagnostic_setting_with_appropriate_categories.metadata.json

...etting_with_appropriate_categories/monitor_diagnostic_setting_with_appropriate_categories.py

jfagoagas · 2024-02-22T10:59:59Z

prowler/providers/azure/services/monitor/monitor_service.py

+#    sub_status: str
+
+
+@dataclass


But you only have the DiagnosticSetting and logs: list, is this right? I mean that the LogSettings import is not present anymore.

…tting_with_appropriate_categories/monitor_diagnostic_setting_with_appropriate_categories.metadata.json Co-authored-by: Pepe Fagoaga <pepe@verica.io>

…r into azure_monitor_5.1.2_check Merge necessary

jfagoagas · 2024-02-22T13:23:44Z

...etting_with_appropriate_categories/monitor_diagnostic_setting_with_appropriate_categories.py

+                        report.status = "PASS"
+                        report.status_extended = f"There is at least one diagnostic setting capturing appropiate categories in subscription {subscription_name}."
+                        break
+                if report.status == "PASS":


Can you change this and use:

administrative_enabled and security_enabled and service_health_enabled and alert_enabled

I don't understand what do you want me to change

I mean to change report.status == "PASS": for

if administrative_enabled and security_enabled and service_health_enabled and alert_enabled:

More readable for me, but not so important.

Now I get it, done

jfagoagas · 2024-02-22T13:24:54Z

tests/providers/azure/services/monitor/monitor_service_test.py

+        assert len(monitor.diagnostics_settings) == 1
+        assert monitor.diagnostics_settings[AZURE_SUBSCRIPTION][0].id == "id"
+        assert (
+            monitor.diagnostics_settings[AZURE_SUBSCRIPTION][0].logs[0].enabled is True


You don't need the is True when doing asserts

Suggested change

monitor.diagnostics_settings[AZURE_SUBSCRIPTION][0].logs[0].enabled is True

monitor.diagnostics_settings[AZURE_SUBSCRIPTION][0].logs[0].enabled

jfagoagas · 2024-02-22T13:26:16Z

tests/providers/azure/services/monitor/monitor_service_test.py

+            monitor.diagnostics_settings[AZURE_SUBSCRIPTION][0].logs[1].category
+            == "Security"
+        )
+        assert (


Can you also assert here the enable status? And for the rest not included.

assert ( monitor.diagnostics_settings[AZURE_SUBSCRIPTION][0].logs[2].enabled is True )

Also iterate through the logs like you do in the check not to break the test if the indexes changes.

Done, Im not checking enabled for the categories that are not include in the check because they can be enabled or disabled, both are a PASS for the check in that category.

Ah ok, good to know, thanks!

jfagoagas

Great work @Hugo966 👏

Thanks for the efforts to address all the suggestions!

sergargar

Good job on your first check @Hugo966, way to go 🚀

OOO

…butes

feat(azure): New azure monitor service

42624cb

Hugo966 requested a review from a team as a code owner February 20, 2024 13:42

github-actions bot added the provider/azure Issues/PRs related with the Azure provider label Feb 20, 2024

feat(monitor): Monitor check ensure diagnostic settings correctly

fea0576

Check_monitor_appropiate_diagnostics_settings_test

c70118e

sergargar reviewed Feb 21, 2024

View reviewed changes

jfagoagas requested changes Feb 21, 2024

View reviewed changes

sergargar reviewed Feb 21, 2024

View reviewed changes

toniblyx previously requested changes Feb 21, 2024

View reviewed changes

(fix:Monitor cheack):Check fixed and cleaned

bf682a8

Hugo966 requested a review from sergargar February 22, 2024 09:22

sergargar changed the title ~~feat(azure): New azure monitor check monitor_ensure_diagnostic_setting_appropiate~~ feat(azure): New azure monitor check monitor_ensure_diagnostic_setting_appropriate Feb 22, 2024

sergargar reviewed Feb 22, 2024

View reviewed changes

Hugo Gálvez Ureña added 2 commits February 22, 2024 11:37

(fix:Monitor check):Check metadata fixed

fd3f1ea

(fix:Monitor check):Check metadata fixed

b09dee4

jfagoagas self-requested a review February 22, 2024 10:49

jfagoagas requested changes Feb 22, 2024

View reviewed changes

Hugo966 and others added 4 commits February 22, 2024 12:08

Update prowler/providers/azure/services/monitor/monitor_diagnostic_se…

ac447b0

…tting_with_appropriate_categories/monitor_diagnostic_setting_with_appropriate_categories.metadata.json Co-authored-by: Pepe Fagoaga <pepe@verica.io>

(fix:Monitor check):Check break and using Azure class fix

6ce76b5

Merge branch 'azure_monitor_5.1.2_check' of github.com:Hugo966/prowle…

bcf0555

…r into azure_monitor_5.1.2_check Merge necessary

(fix:Monitor check):Check test extended

02a40da

jfagoagas self-requested a review February 22, 2024 13:22

jfagoagas requested changes Feb 22, 2024

View reviewed changes

Hugo Gálvez Ureña added 2 commits February 22, 2024 14:47

(fix:Monitor check):final changes

6b32110

(fix:Monitor check):successful tests

24f1753

jfagoagas previously approved these changes Feb 22, 2024

View reviewed changes

sergargar previously approved these changes Feb 23, 2024

View reviewed changes

sergargar requested a review from toniblyx February 23, 2024 10:35

(fix:Monitor check):correct imports

c14e699

Hugo966 dismissed stale reviews from sergargar and jfagoagas via c14e699 February 23, 2024 11:10

sergargar requested review from sergargar and jfagoagas February 26, 2024 11:03

sergargar and others added 2 commits February 26, 2024 12:04

Merge branch 'master' into azure_monitor_5.1.2_check

8549ba6

fix poetry lock

3f11a0d

sergargar previously approved these changes Feb 26, 2024

View reviewed changes

(fix:Checks updated): Check update with new diagnostic settings attri…

6fd07bc

…butes

Hugo966 dismissed sergargar’s stale review via 6fd07bc February 26, 2024 12:03

Hugo Gálvez Ureña and others added 3 commits February 26, 2024 14:25

(fix:Checks updated): Check update with new diagnostic settings attri…

62afa80

…butes

Merge branch 'master' into azure_monitor_5.1.2_check

5870385

fix poetry lock

6af2c37

sergargar self-requested a review February 26, 2024 13:29

sergargar approved these changes Feb 26, 2024

View reviewed changes

sergargar merged commit 963861d into prowler-cloud:master Feb 26, 2024
10 of 11 checks passed

	"CLI": "az monitor diagnostic-settings subscription list --subscription <subscription ID>",
	"CLI": "",

		# self.activity_logs = self.__get_activity_logs__()
		# self.activity_log_alerts = self.__get_activity_log_alerts__()

	"Notes": ""
	"Notes": "When the diagnostic setting is created using Azure Portal, by default no categories are selected."

	"CheckID": "monitor_ensure_diagnostic_setting_appropiate",
	"CheckID": "monitor_diagnostic_setting_with_appropriate_categories",

	monitor.diagnostics_settings[AZURE_SUBSCRIPTION][0].logs[0].enabled is True
	monitor.diagnostics_settings[AZURE_SUBSCRIPTION][0].logs[0].enabled

feat(azure): New azure monitor check monitor_ensure_diagnostic_setting_appropriate #3421

feat(azure): New azure monitor check monitor_ensure_diagnostic_setting_appropriate #3421

Conversation

Hugo966 commented Feb 20, 2024 • edited by sergargar

Context

Description

License

codecov bot commented Feb 20, 2024 • edited

Codecov Report

Choose a reason for hiding this 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hugo966 Feb 22, 2024 • edited by jfagoagas

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergargar Feb 21, 2024 • edited

Choose a reason for hiding this comment

toniblyx 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

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 left a comment

Choose a reason for hiding this comment

sergargar left a comment

Choose a reason for hiding this comment

feat(azure): New azure monitor check `monitor_ensure_diagnostic_setting_appropriate` #3421

feat(azure): New azure monitor check `monitor_ensure_diagnostic_setting_appropriate` #3421

Hugo966 commented Feb 20, 2024 •

edited by sergargar

codecov bot commented Feb 20, 2024 •

edited

Hugo966 Feb 22, 2024 •

edited by jfagoagas

sergargar Feb 21, 2024 •

edited