feat(azure containerregistry): gather service infos and checks disabled admin user #5191

johannes-engler-mw · 2024-09-25T09:37:42Z

Context

This PR adds support to checking Azure Container Registries for security relevant problems.

Description

This PR adds the following features:

Support for gathering informations about Azure Container Registries
Check if the admin user is disabled

Checklist

Are there new checks included in this PR? Yes
- If so, do we need to update permissions for the provider? Please review this carefully. No updated permissions necessary
Review if the code is being covered by tests.
Review if code is being documented following this specification https://github.com/google/styleguide/blob/gh-pages/pyguide.md#38-comments-and-docstrings
Review if backport is needed.

License

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

…isabled-admin-user

puchy22

Thaks for the contribution, it is a great job 💯 , please review my comments

prowler/providers/azure/services/containerregistry/containerregistry_service.py

puchy22 · 2024-09-27T11:54:38Z

prowler/providers/azure/services/containerregistry/containerregistry_service.py

+        super().__init__(ContainerRegistryManagementClient, provider)
+        self.registries = self._get_container_registries(provider)
+
+    def _get_container_registries(self, provider):


Suggested change

def _get_container_registries(self, provider):

def _get_container_registries(self):

It is mandatory in the father constructor, in the methods that you define is not mandatory because you are not using in any line of the method. The above case was a fail, I only wanted to change this one.

Ok understood. Is fixed.

puchy22 · 2024-09-27T12:11:22Z

tests/providers/azure/services/containerregistry/containerregistry_service_test.py

+)
+
+
+class Test_ContainerRegistry_Service:


Suggested change

class Test_ContainerRegistry_Service:

class TestContainerRegistryService:

puchy22 · 2024-09-27T12:17:20Z

tests/providers/azure/services/containerregistry/containerregistry_service_test.py

+
+
+class Test_ContainerRegistry_Service:
+    def test_container_registry(self):


Suggested change

def test_container_registry(self):

def test_get_container_registry(self):

puchy22 · 2024-09-27T12:23:28Z

...ainerregistry/containerregistry_admin_user_disabled/containerregistry_admin_user_disabled.py

+)
+
+
+class TestContainerRegistryAdminUserDisabled:


Suggested change

class TestContainerRegistryAdminUserDisabled:

class Test_containerregistry_admin_user_disabled:

...ry/containerregistry_admin_user_disabled/containerregistry_admin_user_disabled.metadata.json

puchy22 · 2024-09-27T13:11:22Z

...ry/containerregistry_admin_user_disabled/containerregistry_admin_user_disabled.metadata.json

+      "CLI": "az acr update --name <RegistryName> --resource-group <ResourceGroupName> --admin-user-enabled false",
+      "NativeIaC": "",
+      "Other": "",
+      "Terraform": "resource \"azurerm_container_registry\" \"example\" { admin_enabled = false }"


Suggested change

"Terraform": "resource \"azurerm_container_registry\" \"example\" { admin_enabled = false }"

"Terraform": ""

Why delete the example?

For now we are not hardcoding Terraform in this field, it is commonly used to add a link with the Terraform code. I think it could be weird and in a future could be changed but now I think it is better to leave it empty.

...ry/containerregistry_admin_user_disabled/containerregistry_admin_user_disabled.metadata.json

johannes-engler-mw · 2024-09-27T14:12:45Z

...ry/containerregistry_admin_user_disabled/containerregistry_admin_user_disabled.metadata.json

+  "RelatedUrl": "https://docs.microsoft.com/en-us/azure/container-registry/administrative-user",
+  "Remediation": {
+    "Code": {
+      "CLI": "az acr update --name <RegistryName> --resource-group <ResourceGroupName> --admin-user-enabled false",


I think the parameter is --admin-user-enabled, see here.

Here indicates that the flag is --admin-enabled

Ok, you are right. Fixed.

puchy22

Thaks for the contribution, it is a great job 💯 , please review my comments

codecov · 2024-09-27T13:36:50Z

Codecov Report

Attention: Patch coverage is 85.07463% with 10 lines in your changes missing coverage. Please review.

Project coverage is 89.08%. Comparing base (da87c0d) to head (8d8242b).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
...ces/containerregistry/containerregistry_service.py	77.77%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5191      +/-   ##
==========================================
- Coverage   89.08%   89.08%   -0.01%     
==========================================
  Files         989      992       +3     
  Lines       30407    30474      +67     
==========================================
+ Hits        27089    27148      +59     
- Misses       3318     3326       +8

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

Co-authored-by: Rubén De la Torre Vico <[email protected]>

johannes-engler-mw

Pleases check my comments.

johannes-engler-mw · 2024-09-27T14:12:45Z

...ry/containerregistry_admin_user_disabled/containerregistry_admin_user_disabled.metadata.json

+  "RelatedUrl": "https://docs.microsoft.com/en-us/azure/container-registry/administrative-user",
+  "Remediation": {
+    "Code": {
+      "CLI": "az acr update --name <RegistryName> --resource-group <ResourceGroupName> --admin-user-enabled false",


I think the parameter is --admin-user-enabled, see here.

johannes-engler-mw · 2024-09-27T14:13:05Z

...ry/containerregistry_admin_user_disabled/containerregistry_admin_user_disabled.metadata.json

+      "CLI": "az acr update --name <RegistryName> --resource-group <ResourceGroupName> --admin-user-enabled false",
+      "NativeIaC": "",
+      "Other": "",
+      "Terraform": "resource \"azurerm_container_registry\" \"example\" { admin_enabled = false }"


Why delete the example?

johannes-engler-mw · 2024-09-27T14:24:02Z

prowler/providers/azure/services/containerregistry/containerregistry_service.py

+        registries = {}
+        for subscription, client in self.clients.items():
+            try:
+                registries.update({subscription: []})


You mean like this?

for subscription, client in self.clients.items(): try: + registries.update({subscription: {}}) registries_list = client.registries.list() for registry in registries_list: resource_group = self._get_resource_group(registry.id) + registries[registry.id].append( ContainerRegistryInfo( id=getattr(registry, "id", ""), name=getattr(registry, "name", ""), location=getattr(registry, "location", ""), resource_group=resource_group, sku=getattr(registry.sku, "name", ""), login_server=getattr(registry, "login_server", ""), public_network_access=getattr( registry, "public_network_access", "" ), admin_user_enabled=getattr( registry, "admin_user_enabled", "" ), monitor_diagnostic_settings=self._get_registry_monitor_settings( registry.name, resource_group, subscription ), ) )

…-user' of https://github.com/johannes-engler-mw/prowler into feat/azure-containerregistry-service-and-disabled-admin-user

…ng convention

puchy22

Looks great, thanks for the contribution 🔝 🚀

johannes-engler-mw · 2024-09-30T13:51:58Z

...ainerregistry/containerregistry_admin_user_disabled/containerregistry_admin_user_disabled.py

@@ -9,18 +9,18 @@ def execute(self) -> list[Check_Report_Azure]:
        findings = []

        for subscription, registries in containerregistry_client.registries.items():
-            for registry_id, ContainerRegistryInfo in registries.items():
+            for registry_id, conatiner_registry_info in registries.items():


Misspelling of container, please adapt @puchy22

MrCloudSec · 2024-09-30T14:57:34Z

...ry/containerregistry_admin_user_disabled/containerregistry_admin_user_disabled.metadata.json

+{
+  "Provider": "azure",
+  "CheckID": "containerregistry_admin_user_disabled",
+  "CheckTitle": "Admin User is disabled for Azure Container Registry",


Suggested change

"CheckTitle": "Admin User is disabled for Azure Container Registry",

"CheckTitle": "Ensure admin User is disabled for Azure Container Registry",

MrCloudSec · 2024-09-30T14:58:55Z

...ry/containerregistry_admin_user_disabled/containerregistry_admin_user_disabled.metadata.json

+  "ResourceIdTemplate": "",
+  "Severity": "high",
+  "ResourceType": "ContainerRegistry",
+  "Description": "Enabling the admin user for Azure Container Registry (ACR) can pose security risks as it grants unrestricted access to the registry. It is recommended to disable the admin user and utilize Role-Based Access Control (RBAC) instead.",


Suggested change

"Description": "Enabling the admin user for Azure Container Registry (ACR) can pose security risks as it grants unrestricted access to the registry. It is recommended to disable the admin user and utilize Role-Based Access Control (RBAC) instead.",

"Description": "Ensure that the admin user is disabled and Role-Based Access Control (RBAC) is used instead since it could grant unrestricted access to the registry",

MrCloudSec · 2024-09-30T14:59:10Z

...ainerregistry/containerregistry_admin_user_disabled/containerregistry_admin_user_disabled.py

+        findings = []
+
+        for subscription, registries in containerregistry_client.registries.items():
+            for registry_id, conatiner_registry_info in registries.items():


Suggested change

for registry_id, conatiner_registry_info in registries.items():

for registry_id, container_registry_info in registries.items():

…isabled-admin-user

MrCloudSec

Thanks for your contribution @johannes-engler-mw !

johannes-engler-mw added 2 commits September 24, 2024 15:48

added container registry service and first check

e3b3d12

added unit tests for service and first check

4088a4f

johannes-engler-mw requested review from a team as code owners September 25, 2024 09:37

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

johannes-engler-mw changed the title ~~Feat/azure containerregistry service and disabled admin user~~ Feat(azure containerregistry) gather service infos and checks disabled admin user Sep 25, 2024

johannes-engler-mw changed the title ~~Feat(azure containerregistry) gather service infos and checks disabled admin user~~ feat(azure containerregistry) gather service infos and checks disabled admin user Sep 25, 2024

johannes-engler-mw changed the title ~~feat(azure containerregistry) gather service infos and checks disabled admin user~~ feat(azure containerregistry): gather service infos and checks disabled admin user Sep 25, 2024

MrCloudSec assigned puchy22 Sep 25, 2024

johannes-engler-mw and others added 3 commits September 27, 2024 11:11

adapted JSON metadata

f2d3dd3

Merge branch 'master' into feat/azure-containerregistry-service-and-d…

16e6c53

…isabled-admin-user

solve poetry conflicts

2329267

puchy22 requested changes Sep 27, 2024

View reviewed changes

johannes-engler-mw and others added 4 commits September 27, 2024 16:10

Updated description for check

122e8b1

Co-authored-by: Rubén De la Torre Vico <[email protected]>

Updated relatedURL for check

f9845be

Co-authored-by: Rubén De la Torre Vico <[email protected]>

Changed URL in metadata

b4647d2

Co-authored-by: Rubén De la Torre Vico <[email protected]>

fixed return type

df9a922

Co-authored-by: Rubén De la Torre Vico <[email protected]>

johannes-engler-mw commented Sep 27, 2024

View reviewed changes

johannes-engler-mw and others added 7 commits September 30, 2024 11:47

adapted datastructure for registries

9088eb2

Merge branch 'feat/azure-containerregistry-service-and-disabled-admin…

122a39f

…-user' of https://github.com/johannes-engler-mw/prowler into feat/azure-containerregistry-service-and-disabled-admin-user

adapted metadata

9f45c4b

adapted container registry service test

a9b4bab

chore(poetry): solve conflicts

5f1bfc5

chore(containerregistry): change check tittle

6433c54

chore(containerregistry): change variables names to adapt to out nami…

d009b06

…ng convention

This comment was marked as duplicate.

Sign in to view

puchy22 previously approved these changes Sep 30, 2024

View reviewed changes

johannes-engler-mw commented Sep 30, 2024

View reviewed changes

MrCloudSec reviewed Sep 30, 2024

View reviewed changes

MrCloudSec added 3 commits September 30, 2024 11:01

Merge branch 'master' into feat/azure-containerregistry-service-and-d…

33f5ca4

…isabled-admin-user

fix: poetry lock

8c81edf

fix: typo

17e392a

MrCloudSec dismissed puchy22’s stale review via 17e392a September 30, 2024 15:03

chore: revision

8d8242b

MrCloudSec self-requested a review September 30, 2024 15:12

MrCloudSec approved these changes Sep 30, 2024

View reviewed changes

MrCloudSec merged commit 234f8c2 into prowler-cloud:master Sep 30, 2024
10 of 11 checks passed

johannes-engler-mw deleted the feat/azure-containerregistry-service-and-disabled-admin-user branch October 2, 2024 14:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(azure containerregistry): gather service infos and checks disabled admin user #5191

feat(azure containerregistry): gather service infos and checks disabled admin user #5191

johannes-engler-mw commented Sep 25, 2024

puchy22 left a comment

puchy22 Sep 27, 2024

johannes-engler-mw Sep 27, 2024

puchy22 Sep 30, 2024

johannes-engler-mw Sep 30, 2024

puchy22 Sep 27, 2024

puchy22 Sep 27, 2024

puchy22 Sep 27, 2024

puchy22 Sep 27, 2024

johannes-engler-mw Sep 27, 2024

puchy22 Sep 30, 2024

johannes-engler-mw Sep 27, 2024

puchy22 Sep 30, 2024

johannes-engler-mw Sep 30, 2024

puchy22 left a comment

codecov bot commented Sep 27, 2024 •

edited

Loading

johannes-engler-mw left a comment

johannes-engler-mw Sep 27, 2024

johannes-engler-mw Sep 27, 2024

johannes-engler-mw Sep 27, 2024

This comment was marked as duplicate.

puchy22 left a comment

johannes-engler-mw Sep 30, 2024 •

edited

Loading

MrCloudSec Sep 30, 2024

MrCloudSec Sep 30, 2024

MrCloudSec Sep 30, 2024

MrCloudSec left a comment

	def _get_container_registries(self, provider):
	def _get_container_registries(self):

	class Test_ContainerRegistry_Service:
	class TestContainerRegistryService:



		class Test_ContainerRegistry_Service:
		def test_container_registry(self):

	def test_container_registry(self):
	def test_get_container_registry(self):

	class TestContainerRegistryAdminUserDisabled:
	class Test_containerregistry_admin_user_disabled:

	"Terraform": "resource \"azurerm_container_registry\" \"example\" { admin_enabled = false }"
	"Terraform": ""

	"CheckTitle": "Admin User is disabled for Azure Container Registry",
	"CheckTitle": "Ensure admin User is disabled for Azure Container Registry",

	"Description": "Enabling the admin user for Azure Container Registry (ACR) can pose security risks as it grants unrestricted access to the registry. It is recommended to disable the admin user and utilize Role-Based Access Control (RBAC) instead.",
	"Description": "Ensure that the admin user is disabled and Role-Based Access Control (RBAC) is used instead since it could grant unrestricted access to the registry",

	for registry_id, conatiner_registry_info in registries.items():
	for registry_id, container_registry_info in registries.items():

feat(azure containerregistry): gather service infos and checks disabled admin user #5191

feat(azure containerregistry): gather service infos and checks disabled admin user #5191

Conversation

johannes-engler-mw commented Sep 25, 2024

Context

Description

Checklist

License

puchy22 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

puchy22 left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 27, 2024 • edited Loading

Codecov Report

johannes-engler-mw 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

This comment was marked as duplicate.

puchy22 left a comment

Choose a reason for hiding this comment

johannes-engler-mw Sep 30, 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

MrCloudSec left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 27, 2024 •

edited

Loading

johannes-engler-mw Sep 30, 2024 •

edited

Loading