bridgecrewio / checkov

Prevent cloud misconfigurations and find vulnerabilities during build-time in infrastructure as code, container images and open source packages with Checkov by Bridgecrew.
https://www.checkov.io/
Apache License 2.0
7.1k stars 1.12k forks source link

Terraform azurerm role definition check CKV_AZURE_39 matches to broadly on * wildcard #4191

Open robinsmidsrod opened 1 year ago

robinsmidsrod commented 1 year ago

Describe the issue

I'm getting this check failure:

Check: CKV_AZURE_39: "Ensure that no custom subscription owner roles are created"
    FAILED for resource: azurerm_role_definition.data_manager_devops
    File: /terraform/roles/data-manager/role_assignments.tf:20-28
    Guide: https://docs.bridgecrew.io/docs/do-not-create-custom-subscription-owner-roles

In https://github.com/bridgecrewio/checkov/blob/b47dc024fd42dbec5f6cc27c5bd3b5f57ca09421/checkov/arm/checks/resource/CustomRoleDefinitionSubscriptionOwner.py#L34 I notice there is an in check instead of an equals check on the * symbol. Adding all permissions explicitly makes for a very long list, and quite tricky to manage. Some amount of wildcard usage is commonly seen in Microsoft documentation, so I wouldn't consider it bad practice.

I'd also change the text here and there to name it role definition, not just role.

Examples

resource "azurerm_role_definition" "data_manager_devops" {
  name        = "${var.ad_group_prefix}-data-manager-devops"
  scope       = data.azurerm_subscription.devops.id
  description = "Administrative access to all data storage and transfer services"
  permissions {
    actions     = var.data_manager_permissions
    not_actions = []
  }
}
variable "data_manager_permissions" {
  description = "Permissions assigned to data manager role definitions"
  type        = list(string)
  nullable    = false
  default = [
    "Microsoft.Storage/*",
    "Microsoft.DBforMariaDB/*",
    "Microsoft.DBforMySQL/*",
    "Microsoft.DBforPostgreSQL/*",
    "Microsoft.Cache/*",
    "Microsoft.DocumentDB/*",
    "Microsoft.DataLakeStore/*",
    "Microsoft.KeyVault/*",
  ]
}

This fails the above check, and since it doesn't contain a single * in the actions parameter, I wouldn't expect it to fail.

Version (please complete the following information):

Docker hub image bridgecrew/checkov:2 as of 2022-12-29 03:50.

stale[bot] commented 1 year ago

Thanks for contributing to Checkov! We've automatically marked this issue as stale to keep our issues list tidy, because it has not had any activity for 6 months. It will be closed in 14 days if no further activity occurs. Commenting on this issue will remove the stale tag. If you want to talk through the issue or help us understand the priority and context, feel free to add a comment or join us in the Checkov slack channel at https://slack.bridgecrew.io Thanks!

robinsmidsrod commented 1 year ago

The problem statement above still stands. This is still relevant.

stale[bot] commented 10 months ago

Thanks for contributing to Checkov! We've automatically marked this issue as stale to keep our issues list tidy, because it has not had any activity for 6 months. It will be closed in 14 days if no further activity occurs. Commenting on this issue will remove the stale tag. If you want to talk through the issue or help us understand the priority and context, feel free to add a comment or join us in the Checkov slack channel at codifiedsecurity.slack.com Thanks!

robinsmidsrod commented 10 months ago

I'd still like to get this issue resolved.

Saarett commented 4 months ago

Hi @robinsmidsrod , thanks for reaching out. Unfortunately, I don’t see any engagement on this issue, so it will likely have a very low priority among the many other issues we have. Would you like to contribute a fix? It would be much appreciated 🙂

robinsmidsrod commented 4 months ago

Unfortunately I'm not very well versed in Python, so I'd have to have help from someone else to fix this.

Saarett commented 4 months ago

@robinsmidsrod It seems like you’ve found the relevant check file. I’d suggest using an AI tool to assist with the fix; it shouldn’t be very difficult, I'm saying it without diving into the details. Worth the shot  🙂