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

Feature Request: Allow access control modifications to an existing keyvault, without import. #754

Closed
jmeisner3 opened this issue Jan 25, 2018 · 7 comments

Comments

@jmeisner3
Copy link

jmeisner3 commented Jan 25, 2018

Hi there,

Recently there have been additions made to support MSI, by exposing the principal_id on the virtual machine its enabled for. This is great, but it fails to become useful when creating and populating the keyvault with secrets ahead of the full-run, when trying to leverage MSI functions as part of Terraform run.

I'd like the ability to add the principal id to an access control policy. Perhaps "keyvault access control policy" could be a separate resource, which takes the keyvault name, and the objectid, to determine state. This resource could only be created if the Terraform service principal has ability to modify the keyvault ACL.

Expected Behavior

Create Keyvault via external mechanism, and populate a secret(s).

In Terraform:

  • Enable MSI on a virtual machine
  • pass the principal id to create an access policy for an existing keyvault, like "read-only".
  • use the custom-script extension to provision the VM using MSI and the secret you populated.

Actual Behavior

This is not possible, unless terraform creates a new keyvault, or one is imported.

  • In the case of creating a new KV, terraform will also need to populate secrets with the desired values, (not ideal.)

  • In the case of importing, terraform will detect access control policy differences, and existing access control policies are potentially wiped, or not altered in the case of ignore_changes. Either scenario is not a good case, and requires additional manual steps.

There is a workaround, to require end-users to download and install az and also to az login using an SVP that has management access on the KV, prior to running terraform, so that I can leverage the az keyvault set-policy command via null_resource and/or local provisioner once the vm is up, prior to running the custom extension which leverages MSI to download secrets.


Not sure what the Azure roadmap includes, perhaps a modification to MSI to allow existing pre-configured SVPs/Roles to be assigned to VMs, which would also work for my use-case.


Implementation details open for discussion, of course, the end-game is to use pre-populated keyvault with MSI.

@kevinneufeld
Copy link

kevinneufeld commented Feb 7, 2018

+1 "KeyVault access control policy" as a separate resource.

We are looking to use MSI on WebApp and FunctionApps. The resources for Service_App and Function_App would also require a property to enable MSI and provide the required outputs.

Example Arm: app-service-msi-keyvault-dotnet

@monkey-jeff
Copy link

I also want this to simply be able to use a count resource in my access policy to setup N number of policies without having to predefine the value of N.

I started working on this myself but torn how to handle interactions with the existing resources. (ie when you specify access policy in the key vault resource as well as have a keyvault_policy resource at the same time)

Any thoughts on how you'd want it to behave in that situation?

-Jeff

@jmeisner3
Copy link
Author

@sophos-jeff

There are resources which already have to handle this situation:

azurerm_network_security_group and azurerm_network_security_group_rule

or

azurerm_route_table and azurerm_route

They include a disclaimer at the top, which indicates that there could be potential overwrites if you use both.

It may have to be done in combination with edits to the keyvault resource, such that it will not delete policies, if you dont pass any access_policy to the keyvault resource (Currently its a required attribute).

Keyvault resource expects that you are defining all policies, so it should theoretically remove policies and re-create, if that part of the resources properties is defined/changed.

In the case of Route table, in the past I've had to explicitly ignore_changes the route attribute of the route table resource.

There may be a better way, but the path of least resistance, in my opinion would be to adopt a similar strategy as the other resources. I would also fail to "Create" a access_policy resource if there is already an entry in the keyvault acl for that object id (seems to me the end-user would need to delete that entry in the acl first, or import it using the composite id key_vault_name/objectid).

Hope that makes sense. I wish I had time to invest rather than work-around, happy to provide feedback and input!

@monkey-jeff
Copy link

monkey-jeff commented Apr 23, 2018

So I opened a PR to hopefully provide support for this (as linked directly above) I hope this does what is needed to add support for this.

@jmeisner3 Thanks for the advice on where to look for an example of already doing this, it helped.

@pixelicous
Copy link

still waiting for implementation, we need this badly!!

@tombuildsstuff
Copy link
Contributor

closed by #1149

@ghost
Copy link

ghost commented Mar 30, 2020

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!

@ghost ghost locked and limited conversation to collaborators Mar 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants