r/storage_account_customer_managed_key - support for key rotation

[IanMoroney]

• Updated azure-sdk storage api (api update is required, as the new functionality (key_version optional) is only available in the updated API)
• Fixed go-lint issues
• Updated azurerm_storage_account_customer_managed_key and:
• • Made key_version optional
• • Allow for Automated key rotation for storage account encryption

Fixes #6149

Tests performed:

• Compiled provider
• Executed terraform code to create:
• • Resource Group
• • Key Vault
• • Key Vault Access Policies
• • Key Vault Key
• • Storage Account
• • Storage Account Customer Managed Key
• Created customer managed key with key_version - Confirmed Automated Key Rotation Disabled
• Created customer managed key with no key_version specified. The following are only required:
• • storage_account_id
• • key_vault_id
• • key_name
• Confirmed that customer managed key with no key_version specified enables Automated Key Rotation as expected.

Example code to test the new functionality:

data "azurerm_client_config" "current" {}

resource "azurerm_resource_group" "example" {
  name     = "example-resources"
  location = "West Europe"
}

resource "azurerm_key_vault" "example" {
  name                = "examplekv"
  location            = azurerm_resource_group.example.location
  resource_group_name = azurerm_resource_group.example.name
  tenant_id           = data.azurerm_client_config.current.tenant_id
  sku_name            = "standard"

  soft_delete_enabled      = true
  purge_protection_enabled = true
}

resource "azurerm_key_vault_access_policy" "storage" {
  key_vault_id = azurerm_key_vault.example.id
  tenant_id    = data.azurerm_client_config.current.tenant_id
  object_id    = azurerm_storage_account.example.identity.0.principal_id

  key_permissions    = ["get", "create", "list", "restore", "recover", "unwrapkey", "wrapkey", "purge", "encrypt", "decrypt", "sign", "verify"]
  secret_permissions = ["get"]
}

resource "azurerm_key_vault_access_policy" "client" {
  key_vault_id = azurerm_key_vault.example.id
  tenant_id    = data.azurerm_client_config.current.tenant_id
  object_id    = data.azurerm_client_config.current.object_id

  key_permissions    = ["get", "create", "delete", "list", "restore", "recover", "unwrapkey", "wrapkey", "purge", "encrypt", "decrypt", "sign", "verify"]
  secret_permissions = ["get"]
}


resource "azurerm_key_vault_key" "example" {
  name         = "tfex-key"
  key_vault_id = azurerm_key_vault.example.id
  key_type     = "RSA"
  key_size     = 2048
  key_opts     = ["decrypt", "encrypt", "sign", "unwrapKey", "verify", "wrapKey"]

  depends_on = [
    azurerm_key_vault_access_policy.client,
    azurerm_key_vault_access_policy.storage,
  ]
}


resource "azurerm_storage_account" "example" {
  name                     = "examplestor"
  resource_group_name      = azurerm_resource_group.example.name
  location                 = azurerm_resource_group.example.location
  account_tier             = "Standard"
  account_replication_type = "GRS"

  identity {
    type = "SystemAssigned"
  }
}

resource "azurerm_storage_account_customer_managed_key" "example" {
  storage_account_id = azurerm_storage_account.example.id
  key_vault_id       = azurerm_key_vault.example.id
  key_name           = azurerm_key_vault_key.example.name
}

[IanMoroney]

@jackofallops , any chance I could get a review on this? 🙏

[IanMoroney]

@mbfrahry or @katbyte could this be reviewed? as i'd like to begin implementing auto rotating keys for security purposes.
Many thanks!

[aristosvo]

Hi @IanMoroney ! I read your issue about not getting reviewed, I'm sorry to hear that. Thanks for the PR anyway, based on my limited experience the maintainers usually react within a few days. I don't think it is based on size that they prioritised other PRs, it is also depending on their priority and 👍 on the issue. My last PR was closed in just a few days, because it was fixing a bug a lot of people commented about.

I'm not a maintainer, but I'm more than happy to give a few comments. There is a Slack channel as well, it usually makes it a bit easier to contact the maintainers personally. I've noticed a little less activity the last few days, maybe because of the holiday season or something else we don't know about.

Upgrade of an API version is usually done in a separate commit by one of the maintainers. Is it necessary for this functionality to upgrade to the newer version? If that is the case, I would state the reason in your first comment. If the upgrade of the API version is done in a separate commit, it's probably just a size/M commit, which makes it easy for a maintainer to review.

With regard to your tests, is it possible to make it an acceptance test instead of a manual one? And if a manual test makes more sense, it is easier to replicate if the Terraform code is included instead of the functional steps.

[IanMoroney]

Hey @aristosvo , many thanks for your comment!
Sadly, yes this new functionality is included in the new storage api version from the Azure SDK.
I wasn't entirely sure of the process to get this part updated, so included it in the PR for completeness.

Agreed that the squeaky wheel gets the grease, and this may not be a pressing issue for many people at the moment (or not many people realise that automatic key cycling is at all possible now)

I'll have to search for the Slack channel invite instructions, as it would be quite convenient to get early feedback on things, and get some best practices on how to contribute in the most efficient way 👍

Noted on the test, I will see what is the best way to portray the results accurately. In my head, it may be easiest to show the code so it's easy to replicate and test.

I wouldn't mind separating out the API version update if it were absolutely necessary, but would love to get some feedback from a maintainer on that before investing the time.

Many thanks for your suggestions!

[jackofallops]

Hi @IanMoroney
Thanks for this PR, and as previously mentioned, apologies for the delay in getting this review back to you.

As @aristosvo mentioned, we do typically split SDK version bumps from changes as they often tend to have a wider effect than expected in the scope of the change they're used for. That said, this particular case appears to be fine. I'll run a regression test on the whole service shortly to be doubly sure.

On the changes themselves, there's a comment below on config behaviour. We'll also need a test adding that covers the update behaviour for switching between a versioned key and auto-rotation and back again. (You should be able to use the existing _updateKey test as a guide).

I'll circle back asap with any feedback from running the regression tests on the SDK.

Thanks again!

[IanMoroney]

@jackofallops ,
I've brought back the input validation as suggested, and also added a new test to test the functionality without key_version.
Build has passed!

[IanMoroney]

@jackofallops , is there anything else required for this or are we good to go?

[IanMoroney]

@jackofallops , i'll have a production requirement which will need this feature soon.
Is there anything left outstanding, or can it be merged?
Many thanks

[jackofallops]

Tests passing (failures transient/unrelated)

[IanMoroney]

@jackofallops , is there a link to this build so I can see what the failure was?

[jackofallops]

@jackofallops , is there a link to this build so I can see what the failure was?

Hi @IanMoroney
Really nothing to worry about - we run a really high volume of tests and I ran a full service regression there, which includes tests we know to fail for other outstanding issues, or just because we've not had time to update tests for changes at the service side on Azure. ("1 new" in this instance just means it passed on the previous build).

[ghost]

This has been released in version 2.28.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.28.0"
}
# ... other configuration ...

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!