azurerm_key_vault - can now be created without subscription level permissions (fixes #6059) #6260

Jawvig · 2020-03-25T21:04:56Z

This commit fixes #6509 with the following changes:

On Key Vault create, only looking at the subscription level for soft deleted vaults if the RecoverSoftDeletedKeyVaults flag is set to true
Remove the (now redundant) error message returned when there's a soft deleted vault of the same name but the user doesn't want to recover it. This is already covered by an error returned by Azure.
Fix the acceptance test to expect the error message Azure returns in this instance, rather than the text in the removed error.
Fixes the test to add a secret to the vault that will be soft deleted, as Azure will otherwise silently recover the vault anyway (even if the mode is not 'recover') if there is no content in the vault.
Fixes the test so that it doesn't purge the vault on destroy.

(fixes #6059 )

This commit fixes issue hashicorp#6509, in which some subscription level rights are required in order to create a key_vault. This occurred due to the provider always looking for a soft deleted key vault prior to checking whether the feature KeyVault.RecoverSoftDeletedKeyVaults was set to true. Thus event setting the value to false did not stop it searching for the soft deletions. The rights it was requiring were to perform action 'Microsoft.KeyVault/locations/deletedVaults/read' over scope '/subscriptions/xxxxxx'

This commit fixes hashicorp#6509 by building on the previous commit to: 1) Remove the (now redundant) error message returned when there's a soft deleted vault of the same name but the user doesn't want to recover it. This is already covered by an error returned by Azure. 2) Fix the acceptance test to look for the error message Azure returns in this instance, rather than the text in the removed error. 3) Fixes the test to add a secret to the soft deleted vault, as Azure will otherwise silently recover the vault anyway (even if the mode is not 'recover') in this case. 4) Fixes the test so that it doesn't purge the vault on destroy.

Jawvig · 2020-03-25T23:31:35Z

I'd like to link issue #6059 to this PR.

mongrelion · 2020-04-01T13:04:09Z

This is good hunting, @Jawvig
Not a maintainer here but, is there anything I can do to help move this forward? I'm dealing with the same issue and running our own compiled version of the provider is not an option for production.

Jawvig · 2020-04-01T13:12:26Z

@mongrelion I've just promoted it in the Slack channel to see if we can get it into the next release.

mongrelion · 2020-04-01T13:50:31Z

Great!
By the way, I just tested this against our DTA environments and it works like a charm. Not sure if anyone else wants to take it for a spin, since there's already an acceptance test in place.
Also, to clarify, I am in the same boat where I do have access to the resource group but not to the subscription, so that's why the provider fails at the time of retrieving the soft-deleted key vaults.

ttjackott · 2020-04-03T12:41:20Z

This is going to save our bacon! Thankyou @Jawvig!

A30004028 · 2020-04-09T00:32:27Z

Hi @Jawvig , did you manage to get a response on a release for this issue? Thanks

(apologies for the incorrect tag @mongrelion)

mfolley · 2020-04-09T10:36:43Z

If this makes its way into the next release this is going to make our lives at lot easier. Thanks for the effort @Jawvig

Jawvig · 2020-04-09T17:46:32Z

@a30001539 A couple of us have pushed it on the Slack channel. I know @tombuildsstuff has been knee deep in critical AKS fixes for a while, but looks like that's released now today in 2.5.0. Here's hoping a cheeky comment mention will bump it up! It's quite a simple fix. If it wasn't, I wouldn't have had the skills. :-)

katbyte

hey @Jawvig - thanks for opening this PR. We've internally discussed it and @jackofallops has reworked it to be a bit more consistent with the rest of the provider (and will shortly dive into how recovery & child items work) - for now i've pulled his changes into this PR so we can unblock key ault creation so hope you don't mind. PR LGTM now

Jawvig · 2020-04-29T17:17:00Z

Hi @katbyte I confess I'm confused. The new code seems to ignore the RecoverSoftDeletedKeyVaults feature - is this now retired? It always attempts to recover, but if it doesn't have permissions it just quietly ignores the fact that there might be one that should have been recovered. So two people running it with different permissions would end up with different results - either a recovered one or a new one.

I feel I'm overlooking something here.

katbyte · 2020-04-29T17:37:49Z

This is to directly fix the linked issue here, unable to create a key vault due to lack of permissions - @jackofallops has been looking into the other issues raised in this PR and will follow up with a PR that fixes the issues around recovering keyvaults & child items within them, but that will not make it into the 2.8. Given the number of upvotes on the linked issue we figured it would be best to fix that issue sooner then later and follow up with the rest in 2.9/2.10.

Jawvig · 2020-04-29T17:46:55Z

I understand it fixes the linked issue. It's just the way it does it is to change the behaviour so that it will silently fail to recover a keyvault if you don't have permissions even when RecoverSoftDeletedKeyVaults is set to true. Whereas if you have permissions it will recover it. So, two different outcomes just based on permission, and ignoring the feature value.

In the original code in this PR, it would still fail if you didn't have permissions and explicitly set RecoverSoftDeletedKeyVaults to true, but would successfully create a new key vault if you set ReceoverSoftDeletedKeyVaults to false (thus solving the linked issue). I guess it's a matter of opinion, and I'm probably a bit biased here, but I reckon this is the better solution. Anyway, I won't argue it further.

And agreed - the effect of the child objects were a separate issue and can be looked at elsewhere. It's just that I noticed when running the tests under different permissions, the test still passed anyway when it shouldn't have done, so I wasn't able to check the change was having any effect until I made those test changes.

katbyte · 2020-04-29T18:26:39Z

If the keyvault is soft deleted i believe it will then fail to create it after silently failing to recover it when it tries to recreate a vault with the same name? While the error may be different i thinkt he outcome is similar

Jawvig · 2020-04-29T20:17:31Z

Yes, you're right! I hadn't considered that. Okay, I'm convinced. 😀

ghost · 2020-05-01T00:08:25Z

This has been released in version 2.8.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.8.0"
}
# ... other configuration ...

ghost · 2020-05-30T14:50:30Z

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 👉 hashibot-feedback@hashicorp.com. Thanks!

Jawvig added 2 commits March 24, 2020 21:12

ghost added the size/S label Mar 25, 2020

Jawvig mentioned this pull request Mar 25, 2020

AzureRm V2: Cannot create keyVault without Subscription Level Permissions #6059

Closed

Jawvig changed the title ~~Key Vault creation not requiring subscription rights (fixes #6509)~~ Key Vault creation not requiring subscription rights (fixes issue #6509) Mar 25, 2020

Jawvig added 2 commits March 25, 2020 22:15

Fix to spacing in test TF for tflint

a5be882

Removed unused method from key vault code

1729afb

ghost added size/M and removed size/S labels Mar 25, 2020

katbyte added bug service/key-vault Key Vault labels Mar 25, 2020

Jawvig changed the title ~~Key Vault creation not requiring subscription rights (fixes issue #6509)~~ Key Vault creation not requiring subscription rights (fixes issue #6059) Mar 25, 2020

tombuildsstuff added this to the v2.7.0 milestone Apr 17, 2020

tombuildsstuff self-assigned this Apr 17, 2020

katbyte modified the milestones: v2.7.0, v2.8.0 Apr 23, 2020

This comment has been minimized.

Sign in to view

jackofallops assigned jackofallops and unassigned tombuildsstuff Apr 29, 2020

katbyte added 2 commits April 29, 2020 09:32

Merge remote-tracking branch 'origin/master' into master-6260

6c29ab9

change to hashicorp#6693

58a7524

add in helper

2dd12af

ghost added size/S and removed size/M labels Apr 29, 2020

revert test

fea9da3

ghost added size/XS and removed size/S labels Apr 29, 2020

katbyte approved these changes Apr 29, 2020

View reviewed changes

katbyte mentioned this pull request Apr 29, 2020

Enable Keyvault soft delete recovery to be attempted when Read permissions are not allowed at Subscription #6693

Closed

katbyte changed the title ~~Key Vault creation not requiring subscription rights (fixes issue #6059)~~ azurerm_key_vault - can now be created without subscription level permissions (fixes #6059) Apr 29, 2020

katbyte merged commit bad075e into hashicorp:master Apr 29, 2020

katbyte added a commit that referenced this pull request Apr 29, 2020

update CHANGELOG.md to include #6260

5114502

hashicorp locked and limited conversation to collaborators May 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

azurerm_key_vault - can now be created without subscription level permissions (fixes #6059) #6260

azurerm_key_vault - can now be created without subscription level permissions (fixes #6059) #6260

Jawvig commented Mar 25, 2020 •

edited by WodansSon

Jawvig commented Mar 25, 2020

mongrelion commented Apr 1, 2020

Jawvig commented Apr 1, 2020

mongrelion commented Apr 1, 2020

ttjackott commented Apr 3, 2020

A30004028 commented Apr 9, 2020 •

edited

mfolley commented Apr 9, 2020

Jawvig commented Apr 9, 2020

This comment has been minimized.

This comment has been minimized.

katbyte left a comment

Jawvig commented Apr 29, 2020

katbyte commented Apr 29, 2020

Jawvig commented Apr 29, 2020

katbyte commented Apr 29, 2020

Jawvig commented Apr 29, 2020

ghost commented May 1, 2020

ghost commented May 30, 2020

azurerm_key_vault - can now be created without subscription level permissions (fixes #6059) #6260

azurerm_key_vault - can now be created without subscription level permissions (fixes #6059) #6260

Conversation

Jawvig commented Mar 25, 2020 • edited by WodansSon

Jawvig commented Mar 25, 2020

mongrelion commented Apr 1, 2020

Jawvig commented Apr 1, 2020

mongrelion commented Apr 1, 2020

ttjackott commented Apr 3, 2020

A30004028 commented Apr 9, 2020 • edited

mfolley commented Apr 9, 2020

Jawvig commented Apr 9, 2020

This comment has been minimized.

This comment has been minimized.

katbyte left a comment

Choose a reason for hiding this comment

Jawvig commented Apr 29, 2020

katbyte commented Apr 29, 2020

Jawvig commented Apr 29, 2020

katbyte commented Apr 29, 2020

Jawvig commented Apr 29, 2020

ghost commented May 1, 2020

ghost commented May 30, 2020

Jawvig commented Mar 25, 2020 •

edited by WodansSon

A30004028 commented Apr 9, 2020 •

edited