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

How to use the reference keyword in param to retrieve keyvault secret #1028

Closed
akata72 opened this issue Nov 27, 2020 · 31 comments · Fixed by #1571
Closed

How to use the reference keyword in param to retrieve keyvault secret #1028

akata72 opened this issue Nov 27, 2020 · 31 comments · Fixed by #1571
Assignees
Labels
enhancement New feature or request investigate
Milestone

Comments

@akata72
Copy link

akata72 commented Nov 27, 2020

I apologize if this is already addressed or supported already, but I can't seem to find any way to reference keyvault secrets when using modules.

In ARM one way of doing this in a nested template was;

  "parameters": {
            "adminPassword": {
                "**reference**": {
                    "keyVault": {
                        "id": "keyvaultid"
                     },
                    "secretName": "domainjoinsecret"
               }
            }
     }

And the type in the template would be a secure string.

What would be the bicep equivalent when using params in a module?

@akata72 akata72 added the enhancement New feature or request label Nov 27, 2020
@ghost ghost added the Needs: Triage 🔍 label Nov 27, 2020
@akata72 akata72 changed the title reference keyvault in param How to retrieve keyvault secret in param (or directly) Nov 27, 2020
@akata72
Copy link
Author

akata72 commented Nov 28, 2020

Adding some more context to this one. We have a module that joins VMs to domains.
This is a typical use-case where the feature above is use to retrieve the password from the keyvault.
Not sure the best way to implement this, but I guess you will have to tag it somehow so that it translates to the proper ARM nested template similar to the syntax above (using the reference keyword).

module vmdomainJoin '../../modules/units/vm-domainjoin-extension-module.bicep' = {
  dependsOn: [
    vm1
  ]
  name: 'domainjoinextension_deploy_${timestamp}'
  params: {
    tags: tags
    vmname: vm1.outputs.name
    domainJoinOuPath: global.outputs.config.domainjoin.ouPath
    domainToJoin: global.outputs.config.domainjoin.name
    domainJoinUPN: 'svc-djoin@domain.net' 
    _domainJoinPassword: {
       reference: {
           id: keyvaultid
        }
        secretname: domainjoinsecret
     }_
  }
}

@akata72 akata72 changed the title How to retrieve keyvault secret in param (or directly) How to retrieve keyvault secret in param Nov 28, 2020
@akata72 akata72 changed the title How to retrieve keyvault secret in param How to use the reference keyword in param to retrieve keyvault secret Dec 1, 2020
@alex-frankel
Copy link
Collaborator

Thanks for pointing this out. This is indeed a gap, and we will need to figure out how to expose this in bicep.

@alex-frankel alex-frankel added this to the v0.3 milestone Dec 3, 2020
@alex-frankel
Copy link
Collaborator

Had a discussion about this yesterday. The ideal fix here is for keyVault to expose a listSecrets() function as this would allow you to easily retrieve a secret anywhere in the template, not just as a parameter reference. However, there is no ETA from key vault team on if/when this is going to happen.

So given that, we think we could do a syntax like the following:

module mod './module.bicep' = {
  name: 'test'
  params: {
    secretParam: getKeyVaultSecret(kv.id, secret.name, secret.version) 
    // the third argument (secret.version) would be optional
  }
}

We thought about "faking" a listSecret() function with no equivalent in ARM Template, but thought that may end up being more confusing.

As a sidenote, we should also improve the type system to ensure that if a module has a param with secure: true, then we should ensure the value is populated by a KV reference instead of being passed as a non-secure value.

Thoughts?

@DanniJuhl
Copy link

I'm a fan of the first option, in regards to natively exposing a function for it.

Just for my own clarifications sake, I would assume #258 would be a blocker for this implementation then, in terms of referencing kv.Id?

@alex-frankel
Copy link
Collaborator

Good question - I don't think it would be. You should also be able to pass any valid resourceId, so this should work as well:

module mod './module.bicep' = {
  name: 'test'
  params: {
    secretParam: getKeyVaultSecret(resourceId('microsoft.keyVault/vaults', 'vaultName'), secret.name, secret.version) 
    // the third argument (secret.version) would be optional
  }
}

@ChristopherGLewis
Copy link
Contributor

Had a discussion about this yesterday. The ideal fix here is for keyVault to expose a listSecrets() function as this would allow you to easily retrieve a secret anywhere in the template, not just as a parameter reference. However, there is no ETA from key vault team on if/when this is going to happen.

So given that, we think we could do a syntax like the following:

module mod './module.bicep' = {
  name: 'test'
  params: {
    secretParam: getKeyVaultSecret(kv.id, secret.name, secret.version) 
    // the third argument (secret.version) would be optional
  }
}

We thought about "faking" a listSecret() function with no equivalent in ARM Template, but thought that may end up being more confusing.

As a sidenote, we should also improve the type system to ensure that if a module has a param with secure: true, then we should ensure the value is populated by a KV reference instead of being passed as a non-secure value.

Thoughts?

This function would be great, although if we're waiting for the API team I'd rather have this

domainJoinPassword: {
       reference: {
           id: keyvaultid
        }
        secretname: domainjoinsecret
     }

implemented ASAP.

That being stated, I'd eventually love to see this:

external resource kv 'Microsoft.KeyVault/vaults@2019-09-01' = {
  scope: ResourceGroup()
  name: 'KeyVaultName'
}

module vmdomainJoin '../../modules/units/vm-domainjoin-extension-module.bicep' = {
  dependsOn: [
    vm1
  ]
  name: 'domainjoinextension_deploy_${timestamp}'
  params: {
    tags: tags
    vmname: vm1.outputs.name
    domainJoinOuPath: global.outputs.config.domainjoin.ouPath
    domainToJoin: global.outputs.config.domainjoin.name
    domainJoinUPN: 'svc-djoin@domain.net' 
    domainJoinPassword: getKeyVaultSecret(kv.id, 'DomainPWD') 
  }
}

@BertKleewein
Copy link
Member

BertKleewein commented Jan 28, 2021

Just adding my "yes please" to the conversation. I'm OK holding the secret inside a parameter reference -- I'm already using a parameter and passing the secret in from outside as a workaround, but I sure don't like doing this.

@miqm
Copy link
Collaborator

miqm commented Feb 2, 2021

Just to clarify. We'd like to go from this in bicep:

module mod './module.bicep' = {
  name: 'test'
  params: {
    secretParam: getKeyVaultSecret([resourceId('microsoft.keyVault/vaults', 'vaultName')|vaultRef], 'vSecretName', 'vSecretVersion') 
  }
}

to this in json

"parameters": {
  "secretParam": {
       "reference": {
          "keyVault": {
              "id": "keyvaultid"
           },
          "secretName": "vSecretName",
          "secretVersion": "vSecretVersion"
        }
  }
}

Right?

I'm also thinking that getKeyVaultSecret could receive reference to bicep resource of key vault secret directly.

Also, I'm not convinced that getKeyVaultSecret is the right method name. Perhaps just secret would be better, as this will be purely bicep compiler function like any?

@alex-frankel
Copy link
Collaborator

alex-frankel commented Feb 2, 2021

@miqm - we are going to discuss the fine details of the work required for this tomorrow in our team meeting. We will update the issue with everything you should need to get started. So I would hold off starting the work until then.

@miqm
Copy link
Collaborator

miqm commented Feb 5, 2021

@alex-frankel any update on the discussion output?

@majastrz
Copy link
Member

majastrz commented Feb 9, 2021

Meeting notes from 2/8/2021

We had a discussion about this yesterday. We discussed the following options:

Option 1

This option would model the function signature like this: getKeyVaultSecret(keyVaultId: resource, secretName: string, [secretVersion: string])

This would look as follows in bicep:

resource kv 'Microsoft.KeyVault/vaults@xxxx-xx-xx' existing = {
  name: 'myKV'
} 

module myMod 'foo.bicep' = {
  params: {
    ...
    foo: getKeyVaultSecret(kv, 'adminPassword')
    ...
  }
}

Cons:

  • Requires a resource declaration (even with "existing" requires a few extra lines of code)
  • Have to enforce that the function is the value of a module param (or nested deployment param)

Pros:

  • Provides type safety for resource ID
  • Avoids the need to construct the resource ID manually.

Option 2

We could model the function signature like this: getKeyVaultSecret(keyVaultId: string, secretName: string, [secretVersion: string])

Pros:

  • Does not require a KV resource declaration.

Cons:

  • Much less type safety as the user can construct the KV resource ID manually and in some cases we couldn't validate it until runtime (when parameters are used for some of the segments).

Option 3

We could introduce a generic concept of a parameter reference.

Cons:

  • Have not found sufficient support for adding additional capabilities in this area beyond retrieving KV secrets.
  • Difficult to come up with a usable function signature that covers future use cases.

Pros:

  • Extensible first class concept

Variant A

We can support the new function as a method on a KV resource. This variant can be combined with any of the options above.

For example, option 1A would look like the following in the source:

bicep
resource kv 'Microsoft.KeyVault/vaults@xxxx-xx-xx' existing = {
  name: 'myKV'
} 

module myMod 'foo.bicep' = {
  params: {
    ...
    foo: kv.getKeyVaultSecret('adminPassword')
    ...
  }
}

Pros:

  • Easier to explain that this isn't a JSON capability and is specific to Bicep because there are no methods on resources in JSON.

Conclusion

The team consensus is Option 1 above. We feel the improved type safety is worth the extra lines of code.

We have not closed on whether we should go with just Option 1 or Option 1A.

@majastrz
Copy link
Member

majastrz commented Feb 9, 2021

My preference here is to do Option 1A over Option 1. @bmoore-msft, @anthony-c-martin and @alex-frankel can you confirm you're ok with it?

@majastrz
Copy link
Member

majastrz commented Feb 9, 2021

Implementation notes for Option 1:

  • The function call is only allowed in module parameters and potentially in nested deployment parameters.
  • The return value of the function can only be assigned to a property that is a module parameter or a nested deployment parameter. Combining the function with other expressions has to be blocked because such code cannot generate a valid template due to runtime limitations.
  • The semantic checks should enforce that the resource in the function signature is of Microsoft.KeyVault/vaults type (any applicable API version). This goes away with Option 1A because the method would only be available on those types.
  • Parser support for method calls should already be there, but this may require updating the types repo as well.

@miqm
Copy link
Collaborator

miqm commented Feb 9, 2021

If we go with Option 1A, we do not need for the method name to include KeyVault phrase, as it would be available on KeyVault resource. So we could shorten it to getSecret or just secret (as get might indicate, that we're getting value out during assignment, not referencing it)

Also a question - should we check if when using the secret reference, the parameter is securestring type? I don't know if it will work with string. If it does - we should at least warn that secret will be exposed. If not - throw error.

@alex-frankel
Copy link
Collaborator

alex-frankel commented Feb 9, 2021

I am happy with 1 or 1A. 1A aligns with the improvements we have planned for list*, so that is worth considering.

I think it's important to get people used to the existing syntax and the type-safety benefits that brings. If we think the existing syntax is too complex such that user's would rather use the reference() and/or resourceId() functions, then I think that is an indication that our existing syntax could be improved.

we do not need for the method name to include KeyVault phrase, as it would be available on KeyVault resource. So we could shorten it to getSecret or just secret

+1 to this as well

@majastrz
Copy link
Member

majastrz commented Feb 9, 2021

Yeah, good pt. With 1A, it should be called getSecret.

@majastrz
Copy link
Member

majastrz commented Feb 9, 2021

I think it's important to get people used to the existing syntax and the type-safety benefits that brings. If we think the existing syntax is too complex such that user's would rather use the reference() and/or resourceId() functions, then I think that is an indication that our existing syntax could be improved.

+1

@bmoore-msft
Copy link
Contributor

The existing syntax being difficult may be more of an indication that we need to make it easier to get resourceIds for resources not defined in the template. Think of a resource in a nested deployment and how you would construct that today vs. bicep. Maybe worth more thought, but orthogonal to this issue.

Option 1 certainly works and the second "con" bullet is the same across the board if I'm reading that right.

For 1A the method vs. property thing is a little strange to me... we have something that looks like a property but is not and as such can't be used, well, anywhere. My instinct upon seeing that is I no longer need to nest a deployment to get a secret and I can do:

someProperty: kv.getSecret()

Wherever I want and that won't be allowed. Granted that's true for a standalone function, but there are other standalone functions like that (and will be more in the future IMO). When we blur properties/methods it will be more frustrating when I fall into the pit of failure. (remember users already try to use the parameter reference syntax in templates).

@anthony-c-martin
Copy link
Member

Both 1 or 1a sound good to me, with a preference for 1a.

@majastrz
Copy link
Member

majastrz commented Feb 10, 2021

@bmoore-msft Yes that's true that methods can't be called everywhere. This is similar to any object-oriented programming language and doesn't really cause problems there. Completions help a lot with this. And we actually already have the infrastructure for method completions (it's used for namespace-qualified functions right now).

We have also already agreed to have list*() work as a method for resources that support it as well. (With an equivalent global function syntax.)

@miqm
Copy link
Collaborator

miqm commented Feb 12, 2021

I can start working on this. As I think we all agreed - Option 1A short: kv.getSecret()

@miqm
Copy link
Collaborator

miqm commented Feb 14, 2021

I just thought about another variant of referencing secret:

resource kvSecret 'Microsoft.KeyVault/vaults/secrets@2019-09-01' existing = {
  scope: resourceGroup('sub-id','rg-name')
  name: 'kvName/secretName'
}

module secretTest 'secretModule.bicep' = {
  params:{
    mySecret: kvSecret
  }
}

WDYT?

@DanniJuhl
Copy link

@miqm - I'm still a fan of option 1A short.
Assuming I read it correctly, one would have to extend the existing with another resource for every secret required.
So if for instance 3 secret values from same the key vault are required, there would be a lot of repetitive code, rather than just a singular reference to the key vault.
Of course this comes down to preference, but I believe 1A short would lead to leaner and more manageable scripts.

@miqm
Copy link
Collaborator

miqm commented Feb 15, 2021

point taken, thanks @DanniJuhl.

@pbstrein
Copy link

@miqm
FWIW:
The last option you suggested is more declarative and is closer to how Terraform works.
Option 1A is more object oriented, which means some of the declarative nature of ARM/Bicep goes away.

I think either work and are good, it just depends on what direction bicep should go in.

@slavizh
Copy link
Contributor

slavizh commented Mar 10, 2021

@alex-frankel I guess Option 1 seems the fastest and cleanest way as seems Option 3 requires a lot of work.
Whatever the solution is I hope it will work with if and loops. In some cases we have the property (for the secret) is not always required for a resource so we often do ifs key vault resource id is not provided we do not fetch the secret and we apply null or empty string to the resource. In other cases we might create multiple resources based on input array so the secret reference could be different for each resource. It could be that the same secret is used, it could be that different secrets are used but in the same key vault but it could be also that the secrets are located in different key vaults.

@slavizh
Copy link
Contributor

slavizh commented Apr 8, 2021

is there any other workaround for this besides just the obvious and ugly way of passing this trough the main template?

@anthony-c-martin
Copy link
Member

is there any other workaround for this besides just the obvious and ugly way of passing this trough the main template?

I don't believe so unfortunately. We're fairly close to adding support at this point. Currently waiting for #1915 to be merged, which will then unblock @miqm's PR to add support for this feature #1571.

@lelandvelasco
Copy link

lelandvelasco commented Apr 10, 2021

Or is it possible to use reference and use keyvault uri? like the one I used for App Service Secret URI?

@miqm
Copy link
Collaborator

miqm commented Apr 10, 2021

Or is it possible to use reference and use keyvault uri? like the one I used for App Service Secret URI?

not yet. in current state bicep will always generate value object, not reference. we've managed to unblock the PR and it's being reviewed.

@onionhammer
Copy link

I think just being able to create a parameters file which reference secrets that bicep could use if they are parameters. Today if you want to use a parameters json file with secrets in them, you cannot define any parameters that arent directly utilized by the bicep file

@ghost ghost locked as resolved and limited conversation to collaborators May 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request investigate
Projects
None yet
Development

Successfully merging a pull request may close this issue.