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(terraform): add CKV_AZURE_248 - Azure batch account network access restriction #6928

Merged
merged 7 commits into from
Jan 7, 2025

Conversation

emaohi
Copy link
Contributor

@emaohi emaohi commented Jan 2, 2025

User description

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

New/Edited policies (Delete if not relevant)

Description

CKV_AZURE_248 to arm, tf - when publicNetworkAccess is enabled, check for the networkProfile. accountAccess.defaultAction attribute, fail if "allow".

This check aligns to the run policy ID ea27ffec-c8ba-4dbb-95e4-159b5350c94f

Fix

either disable publicNetworkAccess or move networkProfile. accountAccess.defaultAction to "deny"

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my feature, policy, or fix is effective and works
  • New and existing tests pass locally with my changes



Generated description

Below is a concise technical summary of the changes proposed in this PR:

Implements a new security check (CKV_AZURE_248) for Azure Batch accounts in both ARM and Terraform. The check ensures that when public network access is enabled, the account access default action is set to 'deny'. Adds implementation files for both ARM and Terraform, along with corresponding test files and examples. Updates the common test assertion utility to support more detailed validations.

TopicDetails
Test Utils Update Updates the common test assertion utility for more detailed validations
Modified files (1)
  • tests/common/check_assertion_utils.py
Latest Contributors(0)
UserCommitDate
Security Check Impl Implements the new CKV_AZURE_248 check for Azure Batch accounts in ARM and Terraform
Modified files (2)
  • checkov/arm/checks/resource/AzureBatchAccountEndpointAccessDefaultAction.py
  • checkov/terraform/checks/resource/azure/AzureBatchAccountEndpointAccessDefaultAction.py
Latest Contributors(0)
UserCommitDate
Test Coverage Adds test files and examples for the new security check
Modified files (5)
  • tests/arm/checks/resource/example_AzureBatchAccountEndpointAccessDefaultAction.py/fail.json
  • tests/arm/checks/resource/test_AzureBatchAccountEndpointAccessDefaultAction.py
  • tests/terraform/checks/resource/azure/example_AzureBatchAccountEndpointAccessDefaultAction/main.tf
  • tests/terraform/checks/resource/azure/test_AzureBatchAccountEndpointAccessDefaultAction.py
  • tests/arm/checks/resource/example_AzureBatchAccountEndpointAccessDefaultAction.py/pass.json
Latest Contributors(0)
UserCommitDate
This pull request is reviewed by Baz. Join @emaohi and the rest of your team on (Baz).

@emaohi emaohi changed the title feat(arm): add CKV_AZURE_244 - Azure batch account network access restriction feat(arm): add CKV_AZURE_248 - Azure batch account network access restriction Jan 2, 2025
@emaohi emaohi changed the title feat(arm): add CKV_AZURE_248 - Azure batch account network access restriction feat(terraform): add CKV_AZURE_248 - Azure batch account network access restriction Jan 2, 2025
Copy link
Contributor

@lirshindalman lirshindalman left a comment

Choose a reason for hiding this comment

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

nice! 🚀

Copy link
Collaborator

@tsmithv11 tsmithv11 left a comment

Choose a reason for hiding this comment

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

Nice work! A few notes:

  1. I believe you need to adjust the checks for the default value of one of the fields (the other is accurately good when missing).
  2. In the description, can you mention that this is aligned to the run policy ID ea27ffec-c8ba-4dbb-95e4-159b5350c94f for when we platformize it?
  3. Make sure to add the evaluated_keys as @bo156 mentioned.

Comment on lines 21 to 23
public_network_access = conf.get('public_network_access_enabled', [None])[0]
if not public_network_access:
return CheckResult.PASSED
Copy link
Collaborator

Choose a reason for hiding this comment

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

The default for public_network_access_enabled is true which is bad. If this field is missing you need to check the default_action before passing/failing.

Looks like you'll need to adjust the ARM one as well for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct, updated the logic accordingly

FORBIDDEN_NETWORK_ACCESS_DEFAULT_ACTION = "allow"

def __init__(self) -> None:
name = "Ensure that Azure Batch account public network access is 'enabled' account access default action is 'ignore'"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
name = "Ensure that Azure Batch account public network access is 'enabled' account access default action is 'ignore'"
name = "Ensure that Azure Batch account public network access is disabled and account access default action is deny"

class AzureBatchAccountEndpointAccessDefaultAction(BaseResourceCheck):

def __init__(self) -> None:
name = "Ensure that Azure Batch account public network access is 'enabled' account access default action is 'ignore'"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
name = "Ensure that Azure Batch account public network access is 'enabled' account access default action is 'ignore'"
name = "Ensure that Azure Batch account public network access is disabled and account access default action is deny"

default_action = "allow"
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
}
}
resource "azurerm_batch_account" "fail_bad_default_action_no_public_network" {
name = "testbatchaccount"
resource_group_name = "group"
location = "azurerm_resource_group.example.location"
pool_allocation_mode = "BatchService"
network_profile {
account_access {
default_action = "allow"
}
}
}

I suggest adding this scenario and adding it to the test cases for both Terraform and ARM.

Copy link
Contributor Author

@emaohi emaohi Jan 5, 2025

Choose a reason for hiding this comment

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

added + in arm test

Copy link
Collaborator

@tsmithv11 tsmithv11 left a comment

Choose a reason for hiding this comment

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

Great job!

@emaohi emaohi merged commit f9bb95d into main Jan 7, 2025
34 of 35 checks passed
@emaohi emaohi deleted the azure-batch-account branch January 7, 2025 08:02
Saarett pushed a commit that referenced this pull request Jan 7, 2025
…ss restriction (#6928)

* add azure batch account network access validation - arm

* add azure batch account network access validation - terraform
Saarett pushed a commit that referenced this pull request Jan 7, 2025
…ss restriction (#6928)

* add azure batch account network access validation - arm

* add azure batch account network access validation - terraform
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants