Azure / bicep

Bicep is a declarative language for describing and deploying Azure resources
MIT License
3.22k stars 747 forks source link

Add securestring support for template output type #2163

Open AlexanderSehr opened 3 years ago

AlexanderSehr commented 3 years ago

What

In contrast to the parameter section there seems to be no support for a securestring output type.

For example:

output storageAccountSasToken securestring = listAccountSas(storageAccountName, '2019-04-01', accountSasProperties).accountSasToken

Error

If you want to use it regardless, VSCode shows the error: The output type is not valid. Please specify one of the following types: "array", "bool", "int", "object", "string".bicep(BCP030) and you are unable to compile the bicep template.

Why

This feature is especially useful if one deploys a storage account and wants to directly leverage it in subsequent deployments. E.g. if using a linked template orchestration (aka master template) where the storage account hosts CSE scripts for a later VM deployment.

majastrz commented 3 years ago

I think the easiest and most consistent option here would be to add support for the @secure() decorator to outputs. (I don't think we should introduce the type modifier syntax to outputs just for this since it's already deprecated and we intend to remove it.)

miqm commented 3 years ago

It's not just adding decorator, but enabling this in RM itself. Currently it seems it's not supported: https://docs.microsoft.com/en-us/azure/azure-resource-manager/templates/template-syntax#outputs:

If you specify securestring for the output type, the value isn't displayed in the deployment history and can't be retrieved from another template. To use a secret value in more than one template, store the secret in a Key Vault and reference the secret in the parameter file. For more information, see Use Azure Key Vault to pass secure parameter value during deployment.

Although this leads to another problem - secret must exist before entire template deployment, as it's being checked for existence during validation phase.

Referencing KeyVault secrets in module params is tracked in #1028

majastrz commented 3 years ago

Yup, fair enough. The above is the exact reason why we never implemented the secure support for outputs. However, if there's enough interest (aka upvotes 😊), it's definitely something we can revisit in the runtime as well.

miqm commented 3 years ago

upvote 👍🏻

AlexanderSehr commented 3 years ago

The peer that implemented the securestring-output in the original template just confirmed that it was implemented as an non-supported 'option' to be switched to a plain string in case the output would be absolutely needed for some reason.

Seems like I had to much confidence into the given ARM template.

Anyways, though I still believe it is a valid use case, I guess this makes it a RM feature-request rather than a bicep request and this issue should be closed.

alex-frankel commented 3 years ago

I'm fine leaving it open and collecting more feedback. I will mark it as an "Intermediate Language" fix accordingly

anthony-c-martin commented 2 years ago

@alexjneves raised a good example of why this is an important item to get done.

  1. Ideally we'd like to be able to write code like the following. The parent module doesn't have to be aware of the naming convention enforced in the child module, it's simply able to take the primary key and use it. However, this is unsafe, as the primary key will be exposed to anyone viewing the deployment history:

    • Parent module:

      module myStorageMod 'storage.bicep' = {
        name: 'myStorageMod'
        params: {
          baseName: 'foo'
        }
      }
      
      var primaryKey = myStorageMod.outputs.primaryKey
    • Child module:

      param baseName string
      
      var formattedStorageName = '${baseName}somenamingconvention'
      
      resource storage 'Microsoft.Storage/storageAccounts@2021-04-01' = {
        name: formattedStorageName
        location: resourceGroup().location
        sku: {
          name: 'Standard_LRS'
        }
        kind: 'StorageV2'
      }
      
      output primaryKey string = storage.listKeys().keys[0].value
  2. To work around this, we can try to output the name & api version, ane have the parent module perform the listKeys() request:

    module myStorageMod 'storage.bicep' = {
      name: 'myStorageMod'
      params: {
        baseName: 'foo'
      }
    }
    
    var primaryKey = listKeys(myStorageMod.outputs.name, myStorageMod.outputs.apiVersion).keys[0].value

    This is blocked because listKeys() requires name & apiVersion to be deploy time constants

  3. This forces us to rearchitect further - at which point the parent module now has to be aware of the child module's naming convention:

    var baseName = 'foo'
    var formattedStorageName = '${baseName}somenamingconvention'
    
    module myStorageMod 'storage.bicep' = {
      name: 'myStorageMod'
      params: {
        storageName: formattedStorageName
      }
    }
    
    resource storage 'Microsoft.Storage/storageAccounts@2021-04-01' existing = {
      name: formattedStorageName
    }
    
    var primaryKey = storage.listKeys().keys[0].value
  4. This introduces another problem - it removes Bicep's ability to infer dependency orders automatically, and unless you're extremely careful to manually add a dependsOn for myStorageMod where the primaryKey variable is used, you'll end up with the listKeys() function being invoked before the storage account has been deployed.

To summarize, the refactoring you need to do to workaround the lack of secure outputs results in very un-modular code, with many potential footguns around dependency ordering.

brwilkinson commented 2 years ago

Will this not be resolved once we can output a resource (reference) directly?

var primaryKey = myStorageMod.outputs.sa. listKeys().keys[0].value

Since we can call listKeys directly on the output?

robdmoore commented 2 years ago

+1 to being able to set secure outputs. You have to write a whole bunch of code to use a keyvault as an intermediary otherwise and it gets really ugly (e.g. how do you know what the secret name is? ... I've been passing the secret name out as an output of the module that set it).

anthony-c-martin commented 2 years ago

Will this not be resolved once we can output a resource (reference) directly?

var primaryKey = myStorageMod.outputs.sa. listKeys().keys[0].value

Since we can call listKeys directly on the output?

https://github.com/Azure/bicep/issues/2716#issuecomment-991011650 just prompted me to realize - this is actually something that I think the deployment engine would block - as listKeys impacts the deployment graph, and can only depend on deploy-time constants - meaning it can't depend on module outputs.

brwilkinson commented 2 years ago

2716 (comment) just prompted me to realize - this is actually something that I think the deployment engine would block - as listKeys impacts the deployment graph, and can only depend on deploy-time constants - meaning it can't depend on module outputs.

Since this is a resource reference, will it not likely to work on object property values or module property values? However not on variables.

slapointe commented 2 years ago

+1 for secure outputs.

I wanted to have secure string/object between modules and couldn't do it properly without KV as a middle man.

itpropro commented 2 years ago

If someone stumbles upon this issue until there is a good way to either handle secure output or to hand over the whole resource reference, here is how I resolved the storage account issue:

listKeys(resourceId(subscription().subscriptionId, rgName, 'Microsoft.Storage/storageAccounts', storageAccountName), '2021-09-01').keys[0].value

You can just not refer to the outputs of the storageAccount resource itself, but as you should already hand over everything that is needed to the resource, you can just reuse these parameters.

dirkslab commented 2 years ago

@itpropro thanks for the workaround. Really hope for a secure output solution.

johnnyreilly commented 1 year ago

Would be awesome to see this feature - just did the workaround described by @anthony-c-martin and thought "there must be a better way!" - and it turns out, no! Not yet!

guri-s commented 1 year ago

+1 for secure outputs This is a missing feature in BICEP that makes working with modules hard when you have to dealing with connection strings/keys in my case EventHubs At times like this I miss the sensitive argument in terraform outputs 😞

For anyone wanting to get the connection strings from an EventHub Namespace, below worked for me based on what @itpropro mentioned but for an EventHub namespace

listKeys(resourceId(subscription().subscriptionId, eventHubConfiguration.resourceGroup, 'Microsoft.EventHub/namespaces/authorizationRules', eventHubNameSpaceName, 'RootManageSharedAccessKey'), '2022-01-01-preview').primaryConnectionString

dciborow commented 1 year ago

To summarize, the refactoring you need to do to workaround the lack of secure outputs results in very un-modular code, with many potential footguns around dependency ordering.

Even using the dependsOn, I am not able to sort out this race condition... I have extended the example where storage.bicep is defined like this

main.bicep

param location string

var name = uniqueString(resourceGroup().id, location)

module myStorageMod 'storage.bicep' = {
  name: 'myStorageMod-${uniqueString(name)}'
  params: {
    location: location
    name: name
  }
}

resource storage 'Microsoft.Storage/storageAccounts@2021-04-01' existing = {
  name: name
}

var primaryKey = storage.listKeys().keys[0].value

module sample 'sample.bicep' = {
  name: 'sample-${uniqueString(name)}'
  dependsOn: [
    myStorageMod
    storage
  ]
  params: {
    key: primaryKey
  }
}

sample.bicep

@secure()
param key string
{
  "status": "Failed",
  "error": {
    "code": "DeploymentFailed",
    "target": "/subscriptions/edf507a2-6235-46c5-b560-fd463ba2e771/resourceGroups/dcibtestDeploy/providers/Microsoft.Resources/deployments/meta-230406-2104",
    "message": "At least one resource deployment operation failed. Please list deployment operations for details. Please see https://aka.ms/arm-deployment-operations for usage details.",
    "details": [
      {
        "code": "ResourceNotFound",
        "message": "The Resource 'Microsoft.Storage/storageAccounts/foosomenamingconvention' under resource group 'dcibtestDeploy' was not found. For more details please go to https://aka.ms/ARMResourceNotFoundFix"
      }
    ]
  }
}
dciborow commented 1 year ago

Okay, I think this fixes it. Have to use an extra nesting of modules to stop the race condition.

param location string

var name = uniqueString(resourceGroup().id, location)

module myStorageMod 'storage.bicep' = {
  name: 'myStorageMod-${uniqueString(name)}'
  params: {
    location: location
    name: name
  }
}

module nested 'nested.bicep' = {
  name: 'nested-${uniqueString(name)}'
  dependsOn: [
    myStorageMod
  ]
  params: {
    name: name
  }
}

nested.bicep

param name string

resource storage 'Microsoft.Storage/storageAccounts@2021-04-01' existing = {
  name: name
}

var primaryKey = storage.listKeys().keys[0].value

module sample 'sample.bicep' = {
  name: 'sample-${uniqueString(name)}'
  dependsOn: [
    storage
  ]
  params: {
    key: primaryKey
  }
}

sample.bicep

@secure()
param key string
alex-frankel commented 1 year ago

FWIW @dciborow, this is a known issue where list*() functions fire too early that we are actually very close to resolving. We are adding support for dependsOn to these function signatures which we should be able to auto-generate just like other dependsOn in bicep. For cases, where we can't do this automatically, we will allow you to set explicit dependsOn in the function itself in bicep.

gogbg commented 8 months ago

FWIW @dciborow, this is a known issue where list*() functions fire too early that we are actually very close to resolving. We are adding support for dependsOn to these function signatures which we should be able to auto-generate just like other dependsOn in bicep. For cases, where we can't do this automatically, we will allow you to set explicit dependsOn in the function itself in bicep.

@alex-frankel Do you mean that you are planning to add dependsOn on existing resources? If so, what's the progress on that?

davidfowl commented 8 months ago

We need more upvotes!

fvilches17 commented 3 months ago

FWIW @dciborow, this is a known issue where list*() functions fire too early that we are actually very close to resolving. We are adding support for dependsOn to these function signatures which we should be able to auto-generate just like other dependsOn in bicep. For cases, where we can't do this automatically, we will allow you to set explicit dependsOn in the function itself in bicep.

@alex-frankel is there any progress on this?

fvilches17 commented 3 months ago

Will this not be resolved once we can output a resource (reference) directly?

var primaryKey = myStorageMod.outputs.sa. listKeys().keys[0].value

Since we can call listKeys directly on the output?

Unfortunately, not the case it seems.

If you enable this experimental bicep feature, and try referencing outputs as you describe, this is what you'd get:

The properties of module output resources cannot be accessed directly. To use the properties of this resource, pass it as a resource-typed parameter to another module and access the parameter's properties therein.bicep(BCP320)

alex-frankel commented 3 months ago

Updated the milestone on this one as the work is in progress. @fvilches17 FWIW, I would expect this to work once resource outputs are out of experimental.