Key vault secret reference for secure string module param

[miqm]

• getSecret() method on KeyVault type
• Parameter with secure: true Modifier accepts output from kv.getSecret() function
• Parameter with @secure() Decorator accepts output from kv.getSecret() function
• Decompile produces external keyVault resource and getSecret() call when key vault reference is used
• Integration and scenario tests
• Update Documentation

Fixes #1028

Contributing a feature

• I have opened a new issue for the proposal, or commented on an existing one, and ensured that the bicep maintainers are good with the design of the feature being implemented
• I have included "Fixes #{issue_number}" in the PR description, so GitHub can link to the issue and close it when the PR is merged
• I have appropriate test coverage of my new feature

[miqm]

@majastrz @anthony-c-martin @shenglol @TarunSunkaraneni This is my initial implementation of referencing secrets in module parameters. I've created this PR as a draft, it still lacks tests and probably support for decorators. However I wanted to get your opinion on the approach in introducing a type secret.

My idea for implementation was that the function getSecret will return type (secret) and the module parameter, if decorated with {secret: true} will accept union of plain text and the function output: string | secret.

Is this the good approach?

[codecov-io]

Codecov Report

Merging #1571 (5069644) into main (30cb620) will increase coverage by 0.07%.
The diff coverage is 99.20%.

@@            Coverage Diff             @@
##             main    #1571      +/-   ##
==========================================
+ Coverage   95.07%   95.15%   +0.07%     
==========================================
  Files         370      371       +1     
  Lines       21543    21733     +190     
  Branches       15       15              
==========================================
+ Hits        20483    20681     +198     
+ Misses       1060     1052       -8     

Flag	Coverage Δ
dotnet	95.60% <99.20%> (+0.07%)	⬆️
typescript	26.61% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/Bicep.Core/TypeSystem/TypeValidator.cs	98.26% <85.71%> (-0.42%)	⬇️
src/Bicep.Core/Emit/ExpressionEmitter.cs	98.38% <96.87%> (-0.37%)	⬇️
src/Bicep.Core.IntegrationTests/ScenarioTests.cs	100.00% <100.00%> (ø)
src/Bicep.Core/Diagnostics/DiagnosticBuilder.cs	100.00% <100.00%> (ø)
src/Bicep.Core/Emit/TemplateWriter.cs	97.93% <100.00%> (ø)
src/Bicep.Core/LanguageConstants.cs	99.09% <100.00%> (+0.01%)	⬆️
...Core/Semantics/Namespaces/SystemNamespaceSymbol.cs	99.60% <100.00%> (ø)
src/Bicep.Core/Semantics/SymbolValidator.cs	92.94% <100.00%> (+8.73%)	⬆️
src/Bicep.Core/Syntax/ObjectSyntaxExtensions.cs	94.11% <100.00%> (+0.17%)	⬆️
...rc/Bicep.Core/Syntax/ParameterDeclarationSyntax.cs	100.00% <100.00%> (ø)
... and 7 more

[alex-frankel]

Thank you for getting this done!!

Did some testing with these files:

main.bicep which creates a keyvault and secret:

param vaultName string = 'keyVault${uniqueString(resourceGroup().id)}' // must be globally unique
param location string = resourceGroup().location
param sku string = 'Standard'
param tenant string = '72f988bf-86f1-41af-91ab-2d7cd011db47' // replace with your tenantId
param accessPolicies array = [
  {
    tenantId: tenant
    objectId: 'caeebed6-cfa8-45ff-9d8a-03dba4ef9a7d' // replace with your objectId
    permissions: {
      keys: [
        'Get'
        'List'
        'Update'
        'Create'
        'Import'
        'Delete'
        'Recover'
        'Backup'
        'Restore'
      ]
      secrets: [
        'Get'
        'List'
        'Set'
        'Delete'
        'Recover'
        'Backup'
        'Restore'
      ]
      certificates: [
        'Get'
        'List'
        'Update'
        'Create'
        'Import'
        'Delete'
        'Recover'
        'Backup'
        'Restore'
        'ManageContacts'
        'ManageIssuers'
        'GetIssuers'
        'ListIssuers'
        'SetIssuers'
        'DeleteIssuers'
      ]
    }
  }
]

param enabledForDeployment bool = true
param enabledForTemplateDeployment bool = true
param enabledForDiskEncryption bool = true
param enableRbacAuthorization bool = false
param softDeleteRetentionInDays int = 90

param keyName string = 'prodKey'
param secretName string = 'bankAccountPassword'
param secretValue string = '12345'

param networkAcls object = {
  ipRules: []
  virtualNetworkRules: []
}

resource keyvault 'Microsoft.KeyVault/vaults@2019-09-01' = {
  name: vaultName
  location: location
  properties: {
    tenantId: tenant
    sku: {
      family: 'A'
      name: sku
    }
    accessPolicies: accessPolicies
    enabledForDeployment: enabledForDeployment
    enabledForDiskEncryption: enabledForDiskEncryption
    enabledForTemplateDeployment: enabledForTemplateDeployment
    softDeleteRetentionInDays: softDeleteRetentionInDays
    enableRbacAuthorization: enableRbacAuthorization
    networkAcls: networkAcls
  }
}
// create secret
resource secret 'Microsoft.KeyVault/vaults/secrets@2018-02-14' = {
  name: '${keyvault.name}/${secretName}'
  properties: {
    value: secretValue
  }
}

module secretDeploy 'module-with-secret.bicep' = {
  name: 'foo'
  params: {
    secret: keyvault.getSecret(secretName) // would also like to provide the secret resource as a valid type
  }
}

And a dummy module module-with-secret.bicep to pass the secret to:

// @secure() did not work
param secret string {
  secure: true
}

output badPractice string = secret

I hit a bug if I use a decorator @secure() to declare the parameter as secret. For some reason, the type system still sees this as a regular string. I wonder if this is related to #1535.

One potential improvement (not sure how realistic it is). I created the secret as a top-level child resource. I'd like to be able to just pass the resource secret like so:

// create secret
resource secret 'Microsoft.KeyVault/vaults/secrets@2018-02-14' = { ... }

module secretDeploy 'module-with-secret.bicep' = {
  name: 'foo'
  params: {
    secret: secret
  }
}

Or

module secretDeploy 'module-with-secret.bicep' = {
  name: 'foo'
  params: {
    secret: secret.getSecret()
  }
}

[miqm]

@alex-frankel This is still work in progress, I wanted to get some insight if my approach to implementing this is the right way to go.

In general I still have to implement that parameter with @secure() decorator accepts the secret type, write tests and update docs on the existence/usage of this function.

[miqm]

As for the improvement, I initially though about this. First option seems to me to be very hard to implement in bicep, but second one is possible, but perhaps asSecret() would be better.

However this will not work in green field deployments. When I run validate (so even before deployment begins), I receive following error (key vault existed before, but secret didn't):

{"error":{"code":"KeyVaultParameterReferenceSecretRetrieveFailed","message":"The secret of KeyVault parameter 'mySecret1' cannot be retrieved. Http status code: 'NotFound'. Error message: 'A secret with (name/id) mySecret1 was not found in this key vault. If you recently deleted this secret you may be able to recover it using the correct recovery command. For help resolving this issue, please see https://go.microsoft.com/fwlink/?linkid=2125182'. Please see https://aka.ms/arm-keyvault for usage details."}}

Or for non-existing key-vault (that is going to be deployed):

{"error":{"code":"KeyVaultParameterReferenceNotFound","message":"The specified KeyVault '/subscriptions/[GUID]/resourceGroups/bicep-rg/providers/Microsoft.KeyVault/vaults/kv-bicep-2' could not be found. Please see https://aka.ms/arm-keyvault for usage details."}}

Perhaps this is something to be fixed/changed on the Deployment Manager level.

[miqm]

Also, a question here - how to implement decompilation? Existing resource we define using 2 parameters: name and scope. In ARM we had only 1 - id.

I think of 3 options:

1. put id in name field - as decompilation is a best-effort, user will need to correct the usage.
2. try to use some string functions to parse value provided there.
3. add ability to declare existing resource by id only.

For now I think I will go with 1. as 2 might be overcomplicated and 3. can be a future enhancement.

[anthony-c-martin]

Hey @miqm - @alex-frankel, @majastrz, @shenglol and I had a chat about this PR yesterday. We thought that the approach we should follow is to use the FunctionFlags attribute and adding validation to SymbolValidator instead of adding a new type to the type system. Reasoning was:

1. This is more a question of where the function should be permitted than a question of types, and we have existing infrastructure to perform similar checks (for example, dealing with functions that are only allowed in parameter default values).
2. We're trying to be careful about expanding types, as they can add significant maintenance burden by expanding the type assignability matrix. We may want to add the notion of secure-ness in the future, but I think we would want it to be an attribute of a type, rather than a type itself.

Sorry for the delay in getting back to you - we really appreciate your contribution, and please feel free to check in with us for any pointers!

[miqm]

Thanks for the hint, I'll try to rework my PR. What about the decompilation approach? Perhaps we can catch up on Teams sometime next week?

[miqm]

After some time merging/rebasing with main (💘 LF) I managed to put some improvements.

After talking with @anthony-c-martin I renamed and changed the introduced type - it's now keyVaultSecretReference (was secret before) so not to mix up with secret or secure objects.

Function flags are used to indicate placement inside module params, although I needed to make TypeAssignmentVisitor capable of using them when checking instance functions. In the visitor at the point of validating function placement we do not have information about the type of the parameter and therefore we're unable to detect if we're using function in param that accepts it.

I didn't want to write new SyntaxVisitor and introduce a mechanism for validating function flags which might add even more complexity, have performance drawbacks and could be easily mixed up with existing one that is basically split between TypeAssignmentVisitor and NameBindingVisitor.
Perhaps this is room for improvement and making function flags to be validated in a single place, but I think this PR is not the place for it.
Right now I added checking FunctionFlags to TypeAssignmentVisitor so it can be used with InstanceFunctions (previously Default was always used).

The decompilation is not done as it's blocked by #1722 - Suggestion here is to implement it later - after having nice way of referencing external resources by id only rather than name/scope pair (which here would require some extra complexity and smart detecting "what author had id mind" style.

Also, added some scenario tests to cover proper and wrong use cases, so I think we're ready for the proper review.

[anthony-c-martin]

Just had some questions, mostly about the type system changes.

[miqm]

@anthony-c-martin I just wanted to add a bit more regarding introducing the type.

My main thinking flow is that the getSecret() method does not return a securestring. Module param with type securestring (string with @secure() decorator) is capable of accepting a StringLiteral string (primitive) and thanks to the additional validation flag - keyVaultSecretReference.

Even in the ARM output we have a different structure passed to the parameter: instead {"value": "text"} we're building "{"reference":{...} }.

If the method would return a string or securestring, it might give a false impression, that we can assign it to a variable, output, pass to a parameter and what's more - that the method does actually gets a secret from key vault and we have it where we want it.

With this type, we're saying - hey - this is a reference to the key vault secret - you can use it in the module params. And there's no confusion that this is a string. Developer would think more what she or he can do with this type, rather than: "ok, this is string, so I can use it in interpolation, get count of it, concat it, etc.".

[miqm]

@majastrz @alex-frankel - I think this is ready now for final review, I implemented all we talked about and added tests with module loops as suggested.

Only thing that is not working is support for @sys.secure() decorator (with a namespace), but this does not work for @sys.allowed() as well, so I'll create a separate issue to cover that case (I used this same code as per @shenglol suggestion).

[majastrz]

FunctionPlacementFlags

FunctionFlags already has a bunch of placement-like flags. Do we need the second enum?

[miqm]

Function flags are used in type assignment. adding a flag there and assigning it to function overload will trigger the mechanism and prevent using it - type assignment visitor will want function with default flag no with something different.

[miqm]

@majastrz - is this explanation sufficient?

[majastrz]

Both FunctionFlags and FunctionPlacementFlags are declared and assigned together in every place they appear, so their lifecycles appear to be identical.

If you look at these values of FunctionFlags, all of them control placement of the functions:

• ParamDefaultsOnly
• ParameterDecorator
• VariableDecorator
• ResourceDecorator
• ModuleDecorator
• OutputDecorator
• ResourceOrModuleDecorator
• AnyDecorator

In fact it seem like the majority of the enum values are about placement, so it's kind of weird to have a second set of flags also about placement.

You mentioned that adding the flag to FunctionFlag would trigger the type checking mechanism? Can you point me to the code that's doing it? Maybe we can tweak that to avoid it?

[miqm]

it's here:

bicep/src/Bicep.Core/Semantics/SymbolValidator.cs

Line 105 in de8fd8e

private static Symbol ResolveFunctionFlags(FunctionFlags allowedFlags, FunctionSymbol functionSymbol, IPositionable span)

In fact I can see now that additional flag will not impact the binding mechanism, but we will end up in having flags checked in 2 places.

Originally I wanted to use this mechanism, but this method for InstanceFunctions is called from TypeAssignmentVisitor so I ended up with recording placement there.

Of course joining those flags will be quite easy if we're fine with having a flag that is checked elsewhere.

[miqm]

@majastrz - as discussed, I merged it into one enum (Function Flags).

[majastrz]

This is amazing! Thanks for getting this done!

Changes have been made since.

[v-mosh21]

@miqm May I ask if there is any solution to solve the problem about using getSecret() to retrieve a secret value from a KeyVault provisioned in the same deployment?

I saw you have encountered a such problem previously as below:

For non-existing key-vault (that is going to be deployed):
{"error":{"code":"KeyVaultParameterReferenceNotFound","message":"The specified KeyVault '/subscriptions/[GUID]/resourceGroups/bicep-rg/providers/Microsoft.KeyVault/vaults/kv-bicep-2' could not be found. Please see https://aka.ms/arm-keyvault for usage details."}}

I found an other discussion on the same issue in this thread: https://githubmemory.com/repo/Azure/bicep/issues/4081.

Any suggestions will be very appreciated!