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.

Sign up for GitHub

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

azurerm_storage_account_customer_managed_key - support for cross-tenant customer-managed keys #20356

Merged
merged 16 commits into from
Sep 21, 2023
Merged

azurerm_storage_account_customer_managed_key - support for cross-tenant customer-managed keys #20356

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
0c1fdaf
Implement federated_identity_client_id && key_vault_uri within SA CMK…
Sewci0 Feb 7, 2023
c66a8aa
Fix required with
Sewci0 Feb 14, 2023
0e5a109
Add acceptance tests
Sewci0 Feb 17, 2023
eeae8e2
Fix docs
Sewci0 Feb 17, 2023
5bb43e2
Fix tests
Sewci0 May 4, 2023
484d530
Ensure right permission
Sewci0 May 4, 2023
2b37a46
Merge remote-tracking branch 'origin/main' into feature/federated_cmk
Sewci0 May 9, 2023
0390f74
Format code
Sewci0 May 15, 2023
c127236
Fix linting
Sewci0 May 23, 2023
1c51368
Merge remote-tracking branch 'upstream/main' into feature/federated_cmk
Sewci0 May 25, 2023
ae3f97b
Update website/docs/r/storage_account_customer_managed_key.html.markdown
Sewci0 Aug 31, 2023
5f735be
Merge remote-tracking branch 'upstream/main' into feature/federated_cmk
Sewci0 Aug 31, 2023
0d6b847
Merge branch 'feature/federated_cmk' of github.com:Sewci0/terraform-p…
Sewci0 Aug 31, 2023
d9a14d3
azurerm_storage_account_customer_managed_key: fix up test config (sec…
manicminer Sep 20, 2023
707a94b
TeamCity configuration for secondary Azure tenant and subscription
manicminer Sep 20, 2023
490a9aa
Merge branch 'main' into feature/federated_cmk
manicminer Sep 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions .teamcity/components/build_azure.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ class ClientConfiguration(var clientId: String,
val clientSecretAlt: String,
val subscriptionIdAlt : String,
val subscriptionIdDevTest : String,
val tenantIdAlt : String,
val subscriptionIdAltTenant : String,
val principalIdAltTenant : String,
val vcsRootId : String,
val enableTestTriggersGlobally : Boolean) {
}
Expand All @@ -16,10 +19,10 @@ class LocationConfiguration(var primary : String, var secondary : String, var te
}

fun ParametrizedWithType.ConfigureAzureSpecificTestParameters(environment: String, config: ClientConfiguration, locationsForEnv: LocationConfiguration, useAltSubscription: Boolean = false, useDevTestSubscription: Boolean = false) {
hiddenPasswordVariable("env.ARM_CLIENT_ID", config.clientId, "The ID of the Service Principal used for Testing")
hiddenPasswordVariable("env.ARM_CLIENT_ID_ALT", config.clientIdAlt, "The ID of the Alternate Service Principal used for Testing")
hiddenPasswordVariable("env.ARM_CLIENT_SECRET", config.clientSecret, "The Client Secret of the Service Principal used for Testing")
hiddenPasswordVariable("env.ARM_CLIENT_SECRET_ALT", config.clientSecretAlt, "The Client Secret of the Alternate Service Principal used for Testing")
hiddenPasswordVariable("env.ARM_CLIENT_ID", config.clientId, "The Client ID of the Application used for Testing")
hiddenPasswordVariable("env.ARM_CLIENT_ID_ALT", config.clientIdAlt, "The Client ID of the Alternate Application used for Testing")
hiddenPasswordVariable("env.ARM_CLIENT_SECRET", config.clientSecret, "The Client Secret of the Application used for Testing")
hiddenPasswordVariable("env.ARM_CLIENT_SECRET_ALT", config.clientSecretAlt, "The Client Secret of the Alternate Application used for Testing")
hiddenVariable("env.ARM_ENVIRONMENT", environment, "The Azure Environment in which the tests are running")
hiddenVariable("env.ARM_PROVIDER_DYNAMIC_TEST", "%b".format(locationsForEnv.rotate), "Should tests rotate between the supported regions?")
if (useAltSubscription) {
Expand All @@ -33,6 +36,9 @@ fun ParametrizedWithType.ConfigureAzureSpecificTestParameters(environment: Strin
hiddenPasswordVariable("env.ARM_SUBSCRIPTION_ID_ALT", config.subscriptionIdAlt, "The ID of the Alternate Azure Subscription used for Testing")
}
hiddenPasswordVariable("env.ARM_TENANT_ID", config.tenantId, "The ID of the Azure Tenant used for Testing")
hiddenPasswordVariable("env.ARM_TENANT_ID_ALT", config.tenantIdAlt, "The ID of the Secondary Azure Tenant used for Testing")
hiddenPasswordVariable("env.ARM_SUBSCRIPTION_ID_ALT_TENANT", config.subscriptionIdAltTenant, "The ID of the Azure Subscription attached to the Secondary Azure Tenant used for Testing")
hiddenPasswordVariable("env.ARM_PRINCIPAL_ID_ALT_TENANT", config.principalIdAltTenant, "The Object ID of the Service Principal in the Secondary Azure Tenant representing the Application used for Testing")
hiddenVariable("env.ARM_TEST_LOCATION", locationsForEnv.primary, "The Primary region which should be used for testing")
hiddenVariable("env.ARM_TEST_LOCATION_ALT", locationsForEnv.secondary, "The Secondary region which should be used for testing")
hiddenVariable("env.ARM_TEST_LOCATION_ALT2", locationsForEnv.tertiary, "The Tertiary region which should be used for testing")
Expand Down
5 changes: 4 additions & 1 deletion .teamcity/settings.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,12 @@ var tenantId = DslContext.getParameter("tenantId", "")
var environment = DslContext.getParameter("environment", "public")
var clientIdAlt = DslContext.getParameter("clientIdAlt", "")
var clientSecretAlt = DslContext.getParameter("clientSecretAlt", "")
var tenantIdAlt = DslContext.getParameter("tenantIdAlt", "")
var subscriptionIdAltTenant = DslContext.getParameter("subscriptionIdAltTenant", "")
var principalIdAltTenant = DslContext.getParameter("principalIdAltTenant", "")
var vcsRootId = DslContext.getParameter("vcsRootId", "TF_HashiCorp_AzureRM_Repository")
var enableTestTriggersGlobally = DslContext.getParameter("enableTestTriggersGlobally", "true").equals("true", ignoreCase = true)

var clientConfig = ClientConfiguration(clientId, clientSecret, subscriptionId, tenantId, clientIdAlt, clientSecretAlt, subscriptionIdAlt, subscriptionIdDevTest, vcsRootId, enableTestTriggersGlobally)
var clientConfig = ClientConfiguration(clientId, clientSecret, subscriptionId, tenantId, clientIdAlt, clientSecretAlt, subscriptionIdAlt, subscriptionIdDevTest, tenantIdAlt, subscriptionIdAltTenant, principalIdAltTenant, vcsRootId, enableTestTriggersGlobally)

project(AzureRM(environment, clientConfig))
2 changes: 1 addition & 1 deletion .teamcity/tests/helpers.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,5 @@ package tests
import ClientConfiguration

fun TestConfiguration() : ClientConfiguration {
return ClientConfiguration("clientId", "clientSecret", "subscriptionId", "tenantId", "clientIdAlt", "clientSecretAlt", "subscriptionIdAlt", "subscriptionIdDevTest", "vcsRootId", true)
return ClientConfiguration("clientId", "clientSecret", "subscriptionId", "tenantId", "clientIdAlt", "clientSecretAlt", "subscriptionIdAlt", "subscriptionIdDevTest", "tenantIdAlt", "subscriptionIdAltTenant", "principalIdAltTenant", "vcsRootId", true)
}
2 changes: 1 addition & 1 deletion internal/acceptance/testcase.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ func (td TestData) providers() map[string]func() (*schema.Provider, error) {
func (td TestData) externalProviders() map[string]resource.ExternalProvider {
return map[string]resource.ExternalProvider{
"azuread": {
VersionConstraint: "=2.8.0",
VersionConstraint: "=2.38.0",
Source: "registry.terraform.io/hashicorp/azuread",
},
"time": {
Expand Down
98 changes: 69 additions & 29 deletions internal/services/storage/storage_account_customer_managed_key_resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,22 @@ func resourceStorageAccountCustomerManagedKey() *pluginsdk.Resource {
"key_vault_id": {
// TODO: should this be split into two resources, since Key Vault and Managed HSM behave subtly differently in places already
Type: pluginsdk.TypeString,
Required: true,
Optional: true,
ValidateFunc: validation.Any(
// Storage Account Customer Managed Keys support both Key Vault and Key Vault Managed HSM keys:
// https://learn.microsoft.com/en-us/azure/storage/common/customer-managed-keys-overview
commonids.ValidateKeyVaultID,
managedhsms.ValidateManagedHSMID,
),
ExactlyOneOf: []string{"key_vault_id", "key_vault_uri"},
},

"key_vault_uri": {
Sewci0 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW we're intentionally not exposing key_vault_uri as a field across the Provider, since we're unable to determine within the Provider when the Key Vault is (temporarily) inaccessible or (permanently) gone - so unfortunately that's not something we can ship without introducing other issues (and even if we made an exception for this resource, we'd get asks for this on other resources, which would have the same issue).

Instead, we should be able to look this up given the tenant_id, which would solve this in a different manner - WDYT?

Copy link
Contributor Author

@Sewci0 Sewci0 May 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the replay @tombuildsstuff, I've replied to a similar ask previously here:
#20356 (comment)

The bottom line is that this key vault is completely outside of the control of the entity that sets up the encryption. Common use case would be a customer providing a key which they want to use to encrypt their own data. This means we don't have access to lookup any metadata for that key.

This also mimics Azure's UI used for setting up storage account encryption, where you can either specify the keyvault using ID or URI. URI is the only possible option when setting up the cross tenant AD App.
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation @Sewci0. Looking at the docs for this configuration, it seems like referencing the URI is the best/only way forward in this case, so I think it's ok to add this.

Type: pluginsdk.TypeString,
Optional: true,
ValidateFunc: validation.IsURLWithHTTPS,
ExactlyOneOf: []string{"key_vault_id", "key_vault_uri"},
Computed: true,
},

"key_name": {
Expand All @@ -76,6 +85,13 @@ func resourceStorageAccountCustomerManagedKey() *pluginsdk.Resource {
Optional: true,
ValidateFunc: commonids.ValidateUserAssignedIdentityID,
},

"federated_identity_client_id": {
Type: pluginsdk.TypeString,
Optional: true,
ValidateFunc: validation.IsUUID,
RequiredWith: []string{"user_assigned_identity_id"},
},
},
}
}
Expand Down Expand Up @@ -113,37 +129,46 @@ func resourceStorageAccountCustomerManagedKeyCreateUpdate(d *pluginsdk.ResourceD
}
}

keyVaultID, err := commonids.ParseKeyVaultID(d.Get("key_vault_id").(string))
if err != nil {
return err
}
keyVault, err := vaultsClient.Get(ctx, *keyVaultID)
if err != nil {
return fmt.Errorf("retrieving Key Vault %q (Resource Group %q): %+v", keyVaultID.VaultName, keyVaultID.ResourceGroupName, err)
}
keyVaultURI := ""
if keyVaultURIRaw := d.Get("key_vault_uri").(string); keyVaultURIRaw != "" {
keyVaultURI = keyVaultURIRaw
} else {
keyVaultID, err := commonids.ParseKeyVaultID(d.Get("key_vault_id").(string))
if err != nil {
return err
}

keyVault, err := vaultsClient.Get(ctx, *keyVaultID)
if err != nil {
return fmt.Errorf("retrieving Key Vault %q (Resource Group %q): %+v", keyVaultID.VaultName, keyVaultID.ResourceGroupName, err)
}

softDeleteEnabled := false
purgeProtectionEnabled := false
if model := keyVault.Model; model != nil {
if esd := model.Properties.EnableSoftDelete; esd != nil {
softDeleteEnabled = *esd
softDeleteEnabled := false
purgeProtectionEnabled := false
if model := keyVault.Model; model != nil {
if esd := model.Properties.EnableSoftDelete; esd != nil {
softDeleteEnabled = *esd
}
if epp := model.Properties.EnablePurgeProtection; epp != nil {
purgeProtectionEnabled = *epp
}
}
if epp := model.Properties.EnablePurgeProtection; epp != nil {
purgeProtectionEnabled = *epp
if !softDeleteEnabled || !purgeProtectionEnabled {
return fmt.Errorf("Key Vault %q (Resource Group %q) must be configured for both Purge Protection and Soft Delete", keyVaultID.VaultName, keyVaultID.ResourceGroupName)
}
}
if !softDeleteEnabled || !purgeProtectionEnabled {
return fmt.Errorf("Key Vault %q (Resource Group %q) must be configured for both Purge Protection and Soft Delete", keyVaultID.VaultName, keyVaultID.ResourceGroupName)
}

keyVaultBaseURL, err := keyVaultsClient.BaseUriForKeyVault(ctx, *keyVaultID)
if err != nil {
return fmt.Errorf("looking up Key Vault URI from %s: %+v", *keyVaultID, err)
keyVaultBaseURL, err := keyVaultsClient.BaseUriForKeyVault(ctx, *keyVaultID)
if err != nil {
return fmt.Errorf("looking up Key Vault URI from %s: %+v", *keyVaultID, err)
}

keyVaultURI = *keyVaultBaseURL
}

keyName := d.Get("key_name").(string)
keyVersion := d.Get("key_version").(string)
userAssignedIdentity := d.Get("user_assigned_identity_id").(string)
federatedIdentityClientID := d.Get("federated_identity_client_id").(string)

props := storage.AccountUpdateParameters{
AccountPropertiesUpdateParameters: &storage.AccountPropertiesUpdateParameters{
Expand All @@ -163,12 +188,16 @@ func resourceStorageAccountCustomerManagedKeyCreateUpdate(d *pluginsdk.ResourceD
KeyVaultProperties: &storage.KeyVaultProperties{
KeyName: utils.String(keyName),
KeyVersion: utils.String(keyVersion),
KeyVaultURI: utils.String(*keyVaultBaseURL),
KeyVaultURI: utils.String(keyVaultURI),
},
},
},
}

if federatedIdentityClientID != "" {
props.Encryption.EncryptionIdentity.EncryptionFederatedIdentityClientID = utils.String(federatedIdentityClientID)
}

if _, err = storageClient.Update(ctx, id.ResourceGroupName, id.StorageAccountName, props); err != nil {
return fmt.Errorf("updating Customer Managed Key for %s: %+v", id, err)
}
Expand Down Expand Up @@ -226,28 +255,39 @@ func resourceStorageAccountCustomerManagedKeyRead(d *pluginsdk.ResourceData, met
}

userAssignedIdentity := ""
federatedIdentityClientID := ""
if props := encryption.EncryptionIdentity; props != nil {
if props.EncryptionUserAssignedIdentity != nil {
userAssignedIdentity = *props.EncryptionUserAssignedIdentity
}
if props.EncryptionFederatedIdentityClientID != nil {
federatedIdentityClientID = *props.EncryptionFederatedIdentityClientID
}
}

if keyVaultURI == "" {
return fmt.Errorf("retrieving %s: `properties.encryption.keyVaultProperties.keyVaultURI` was nil", id)
}

keyVaultID, err := keyVaultsClient.KeyVaultIDFromBaseUrl(ctx, resourcesClient, keyVaultURI)
if err != nil {
return fmt.Errorf("retrieving Key Vault ID from the Base URI %q: %+v", keyVaultURI, err)
}

// now we have the key vault uri we can look up the ID

// we can't look up the ID when using federated identity as the key will be under different tenant
keyVaultID := ""
if federatedIdentityClientID == "" {
tmpKeyVaultID, err := keyVaultsClient.KeyVaultIDFromBaseUrl(ctx, resourcesClient, keyVaultURI)
if err != nil {
return fmt.Errorf("retrieving Key Vault ID from the Base URI %q: %+v", keyVaultURI, err)
}
keyVaultID = *tmpKeyVaultID
}

d.Set("storage_account_id", id.ID())
d.Set("key_vault_id", keyVaultID)
d.Set("key_vault_uri", keyVaultURI)
d.Set("key_name", keyName)
d.Set("key_version", keyVersion)
d.Set("user_assigned_identity_id", userAssignedIdentity)
d.Set("federated_identity_client_id", federatedIdentityClientID)

return nil
}
Expand Down
Loading
Loading