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

[Modules] Microsoft.Storage/StorageAccounts and Microsoft.KeyVault/vaults policy exemption #2997

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

shawntmeyer
Copy link
Contributor

@shawntmeyer shawntmeyer commented Mar 15, 2023

Description

This update adds the deployment of policy exemptions at the Storage Account and Key Vault resource levels which is a useful deployment orchestration when we must deploy storage accounts or Key Vaults with public access allowed and we have policy deployed at a higher scope that blocks that resource creation. Addresses Issue #2996.

Pipeline references

Storage - StorageAccounts

KeyVault - Vaults

Type of Change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • Update to documentation

Checklist

  • I'm sure there are no other open Pull Requests for the same update/change
  • My corresponding pipelines / checks run clean and green without any errors or warnings
  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (readme)
  • I did format my code

@shawntmeyer shawntmeyer requested a review from a team as a code owner March 15, 2023 01:05
@ahmadabdalla
Copy link
Contributor

@shawntmeyer This is a new feature that we are introducing like how we do RBAC on resource level.

may I suggest we put this one on hold to bring in this topic into one of internal discussions. I will tag the issue #2996. as well.
CC: @rahalan

Copy link
Contributor

@ahmadabdalla ahmadabdalla left a comment

Choose a reason for hiding this comment

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

Placeholder comment until feature is discussed

Copy link
Contributor

@AlexanderSehr AlexanderSehr left a comment

Choose a reason for hiding this comment

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

Create stuff in any case. The only caviats I'm seeing are

  • Increased file size (paying into the 4 MB limit)
  • In theory there could be one excemption per resource type, I guess

Also I hope normal contributors won't have the permissions to just deploy excemptions 😄

@eriqua eriqua changed the title [Module] Microsoft.Storage/StorageAccounts policy exemption [Modules] Microsoft.Storage/StorageAccounts policy exemption Mar 23, 2023
@AlexanderSehr AlexanderSehr linked an issue Mar 23, 2023 that may be closed by this pull request
@eriqua
Copy link
Contributor

eriqua commented Mar 31, 2023

Marked as draft as per offline discussion

@eriqua eriqua marked this pull request as draft March 31, 2023 18:50
@shawntmeyer shawntmeyer changed the title [Modules] Microsoft.Storage/StorageAccounts policy exemption [Modules] Microsoft.Storage/StorageAccounts and Microsoft.KeyVault/vaults policy exemption Apr 6, 2023
@@ -338,6 +349,21 @@ module storageAccount_roleAssignments '.bicep/nested_roleAssignments.bicep' = [f
}
}]

resource storageAccount_policyExemptions 'Microsoft.Authorization/policyExemptions@2022-07-01-preview' = [for policyExemption in policyExemptions: if (!empty(policyExemptions)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't the storage account deployment be already blocked before we even make it to the excemption?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is not, the storage account is deployed. The common tests for both storage accounts and key vaults deploy a policy assignment that denies the creation of a resource that doesn't meet the policy. I verified that the test is a true test in that the resource doesn't meet the policy, but the exception works and the deployment succeeds.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the policy a deny or an audit policy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. I'm inclined to see this as an unexpected behaviour of the policy. Probably due to timing? I'm very confused as I'd expect the deny policy to deny the creation of an uncompliant resource, by definition 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is exactly why I needed the PR. We can not create Resource Level exemptions without the resource ID and we can't create the resource with a deny policy in place without the exemption. The only way to do this is through ARM as a single deployment. ARM sorts it out and allows the resource to be created due to the exemption. Without this we would have to temporarily place an exemption at the RG level or disable the policy.

@shawntmeyer shawntmeyer marked this pull request as ready for review April 14, 2023 13:31
@AlexanderSehr AlexanderSehr marked this pull request as draft May 4, 2023 12:32
@AlexanderSehr
Copy link
Contributor

Marking as draft until we found the best way to integrate this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Allow Resource level policy exemption
4 participants