[AVM Module Issue]: Support autorotation for customer managed key in storage account

thasimongyldendal commented 3 months ago

Check for previous/existing GitHub issues

[X] I have checked for previous/existing GitHub issues

Issue Type?

Feature Request

Module Name

avm/res/storage/storage-account

(Optional) Module Version

No response

Description

Currently if specifying a customer managed key it defaults to an explicit version (either specified in the AVM-module or using latest if not specified). From /res/storage/storage-account/main.bicep

  keyvaultproperties: !empty(customerManagedKey)
          ? {
              keyname: customerManagedKey!.keyName
              keyvaulturi: cMKKeyVault.properties.vaultUri
              keyversion: !empty(customerManagedKey.?keyVersion ?? '')
                ? customerManagedKey!.keyVersion
                : last(split(cMKKeyVault::cMKKey.properties.keyUriWithVersion, '/'))
            }
          : null

This leads to autorotation of the encryption key being disabled on the created storage account:

It should be possible to not specify the keyversion to have automated key rotation work as desired. I believe when you use the resource directly and don't specify a keyversion automated rotation is supported.

(Optional) Correlation Id

No response

avm-team-linter[bot] commented 3 months ago

@thasimongyldendal, thanks for submitting this issue for the avm/res/storage/storage-account module!

[!IMPORTANT] A member of the @Azure/avm-res-storage-storageaccount-module-owners-bicep or @Azure/avm-res-storage-storageaccount-module-contributors-bicep team will review it soon!

AlexanderSehr commented 3 months ago

Hey @thasimongyldendal, thanks for raising this issue. Have you been able to confirm that autorotation is enabled when not specifying a version? If so, we should update the AVM spec and update this as well as other modules to allow users to specify that they want auto-rotation enabled (e.g., by specifying as such in the interface / as a value).

Edit: Here's the link to the official docs: https://learn.microsoft.com/en-us/azure/storage/common/customer-managed-keys-overview#automatically-update-the-key-version

thasimongyldendal commented 3 months ago

Hello @AlexanderSehr I have confirmed that not specifying a keyversion gives you automatic key rotation.

I used this bicep code to confirm it:

resource storageAccount 'Microsoft.Storage/storageAccounts@2023-05-01' = {
  kind: 'StorageV2'
  name: name
  location: resourceGroup().location
  sku: {
    name: sku
  }
  identity: {
    type: 'UserAssigned'
    userAssignedIdentities: {
      '${managedIdentity.id}' : {}
    }
  }
  properties: {
    encryption: {
      keyvaultproperties: { //Here I did not set the keyversion property
        keyname: keyName
        keyvaulturi: keyVaultUri 
      }
      identity: {
        userAssignedIdentity: managedIdentity.id
      }
      keySource: 'Microsoft.Keyvault'
    }
  }
}

And the result in the portal:

AlexanderSehr commented 2 months ago

Alright, @ktremain then we should implement this and discuss how best to update the schema. For this module (as I'm not sure if the same feature exists across all RPs (I actually very much doubt it)), it may be enough to remove to automatic fetching of the latest version. In other words, to update the implementation from:

keyversion: !empty(customerManagedKey.?keyVersion ?? '')
  ? customerManagedKey!.keyVersion
  : last(split(cMKKeyVault::cMKKey.properties.keyUriWithVersion, '/'))

to something like:

keyversion: !empty(customerManagedKey.?keyVersion ?? '')
  ? customerManagedKey!.keyVersion
  : null

or maybe even

keyversion: customerManagedKey!.?keyVersion

which would go hand in hand with the removal of the key reference (as not needed anymore) etc. Thoughts?

The above assumes though that keyversion can be set with null to resemble a undefined state. If that doesn't work, we'd need to complicate the logic slightly with a union() to have bicep not include the property at all. But let's only go there if absolutely necessary.

AlexanderSehr commented 2 months ago

I'll see to create a PR once the new RBAC schema was roled out to the storage account module via #2008 to avoid conflicts

Dekor86 commented 1 week ago

Is there any updates on this? We've just spotted this issue and need to make a decision on whether to move to a custom module for storage accounts depending on time scales.

AlexanderSehr commented 1 week ago

Is there any updates on this? We've just spotted this issue and need to make a decision on whether to move to a custom module for storage accounts depending on time scales.

Hey @Dekor86, I took a closer look just recently. As this may have an impact on the spec (which in turn would affect all modules with CMK) I want to have collect the feedback from the core team before going ahead and changing the implementation. The alternatives would be to either

Remove the KeyVersion autofetch completely, leaving it up to the module owner to either require that parameter (if it is required by the Resource Provider), or to default to null (as asked for above)
Or to add another boolean switch that enables/disables the functionality that would automatically fetch the latest version at deployment time for the user

There's a call happening this Thursday that I hope will bring a decision after which we can then go ahead and implement the change. It may take until next week to release the update though.

AlexanderSehr commented 1 week ago

The decision is unfortunately still pending as we did not come to an agreement yet and are still discussing how to fix the issue

Edit: Alright, here's the plan Well allow module owners to use one of 2 interfaces

If auto-rotation is supported (like in case of the storage account): Use the current interface but add a boolean switch to enable/disable the auto-fetch which would be disabled by default (i.e., pass null into the RP)
If auto-rotation is not supported, use the interface as today. By default it will fetch a version at deployment time for you, unless you provide a version that it will use that

This way we can support both variants - depending on what the RP supports. Will make a note to update the spec and create an issue to update the modules after.

cc: @eriqua, @ChrisSidebotham, @jtracey93, @rahalan

Azure / bicep-registry-modules