Skip to content
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

feat(azure): Add new checks related to Network service #3402

Merged
merged 34 commits into from Feb 20, 2024

Conversation

pedrooot
Copy link
Contributor

@pedrooot pedrooot commented Feb 15, 2024

Context

This pr adds 7 new checks related to network service.

Description

The checks that are included in this pr are:

  • network_rdp_internet_access_restricted
  • network_ssh_internet_access_restricted
  • network_udp_internet_access_restricted
  • network_http_internet_access_restricted
  • network_flow_log_more_than_90_days
  • network_network_watcher_enabled
  • network_bastion_host_exists

License

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

@pedrooot pedrooot requested a review from a team as a code owner February 15, 2024 13:17
security_rules=security_group.security_rules,
network_watchers=network_watchers,
subscription_locations=subscription_locations,
flow_logs=self.__get_flow_logs__(subscription),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you do it up like the others?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

def execute(self) -> Check_Report_Azure:
findings = []
for subscription, bastion_hosts in network_client.bastion_hosts.items():
if bastion_hosts == []:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if bastion_hosts == []:
if not bastion_hosts:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

else:
status = "PASS"
status_extended = (
f"Bastion Host from subscription {subscription} exists"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, add here the bastion hosts that are available

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

report.status = "PASS"
report.status_extended = f"Security Group {security_group.name} from subscription {subscription} has flow logs enabled for more than 90 days"
has_failed = False
if not has_failed:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if not has_failed:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

report.subscription = subscription
report.resource_name = security_group.name
report.resource_id = security_group.id
if security_group.flow_logs and security_group.flow_logs.__len__() > 0:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if security_group.flow_logs and security_group.flow_logs.__len__() > 0:
if security_group.flow_logs:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Member

@sergargar sergargar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, review my comments.

report.status = "PASS"
report.status_extended = f"Security Group {security_group.name} from subscription {subscription} has HTTP internet access restricted."
rule_fail_condition = any(
rule.destination_port_range == "80"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, review if the rule accepts all ports or a range of ports that includes port 80.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -0,0 +1,30 @@
{
"Provider": "azure",
"CheckID": "network_network_watcher_enabled",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"CheckID": "network_network_watcher_enabled",
"CheckID": "network_watcher_enabled",

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

self.bastion_hosts = self.__get_bastion_hosts__()

def __get_security_groups__(self, token):
logger.info("SQL Server - Getting Network Security Groups...")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, remove SQL Server from the INFO logs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

security_groups.update({subscription: []})
security_groups_list = client.network_security_groups.list_all()
available_locations = {}
network_watchers = self.__get_network_watchers__(client, subscription)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, do a separate function and class for Network Watchers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

security_rules=security_group.security_rules,
network_watchers=network_watchers,
subscription_locations=subscription_locations,
flow_logs=self.__get_flow_logs__(subscription),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add it to the new Network Watchers class

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

report.status = "PASS"
report.status_extended = f"Security Group {security_group.name} from subscription {subscription} has RDP internet access restricted."
rule_fail_condition = any(
rule.destination_port_range == "3389"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as HTTP check

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

report.status = "PASS"
report.status_extended = f"Security Group {security_group.name} from subscription {subscription} has SSH internet access restricted."
rule_fail_condition = any(
rule.destination_port_range == "22"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as previous checks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

pyproject.toml Outdated
@@ -53,6 +53,7 @@ schema = "0.7.5"
shodan = "1.31.0"
slack-sdk = "3.26.2"
tabulate = "0.9.0"
azure-mgmt-network = "^25.2.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
azure-mgmt-network = "^25.2.0"
azure-mgmt-network = "25.2.0"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@github-actions github-actions bot added the provider/azure Issues/PRs related with the Azure provider label Feb 19, 2024
Copy link

codecov bot commented Feb 19, 2024

Codecov Report

Attention: 53 lines in your changes are missing coverage. Please review.

Comparison is base (753f32b) 85.85% compared to head (5920151) 85.76%.
Report is 1 commits behind head on master.

Files Patch % Lines
...roviders/azure/services/network/network_service.py 44.28% 39 Missing ⚠️
prowler/providers/azure/azure_provider.py 26.31% 14 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3402      +/-   ##
==========================================
- Coverage   85.85%   85.76%   -0.10%     
==========================================
  Files         609      618       +9     
  Lines       19369    19615     +246     
==========================================
+ Hits        16630    16823     +193     
- Misses       2739     2792      +53     

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

name=security_group.name,
location=security_group.location,
security_rules=security_group.security_rules,
subscription_locations=subscription_locations,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
subscription_locations=subscription_locations,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

security_groups_list = client.network_security_groups.list_all()
for security_group in security_groups_list:
subscription_id = security_group.id.split("/")[2]
subscription_locations = audit_info.locations[subscription_id]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
subscription_locations = audit_info.locations[subscription_id]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

name: str
location: str
security_rules: list
subscription_locations: list
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
subscription_locations: list

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

for network_watcher in network_watchers:
nw_locations.append(network_watcher.location)
for subscription, security_groups in network_client.security_groups.items():
for security_group in security_groups:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Iterate with the locations that are in audit info, please

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 86 to 88
assert network.security_groups[AZURE_SUBSCRIPTION][
0
].subscription_locations == ["location"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert network.security_groups[AZURE_SUBSCRIPTION][
0
].subscription_locations == ["location"]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -212,3 +213,31 @@ def get_identity(self):

def get_region_config(self):
return self.region_config

def get_locations(self, credentials, region_config):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, add tests to this function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@sergargar sergargar self-requested a review February 20, 2024 13:02
sergargar
sergargar previously approved these changes Feb 20, 2024
Copy link
Member

@sergargar sergargar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome PR!!

@sergargar sergargar self-requested a review February 20, 2024 13:13
sergargar
sergargar previously approved these changes Feb 20, 2024
@sergargar sergargar merged commit 9a22c2d into prowler-cloud:master Feb 20, 2024
8 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
provider/azure Issues/PRs related with the Azure provider
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants