-
Notifications
You must be signed in to change notification settings - Fork 340
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
[AVM Question/Feedback]: How do we handle resources that emit secrets? #1934
Comments
Thanks a lot @Agazoth for opening and elaborating on this issue. @ChrisSidebotham @jtracey93 @AlexanderSehr FYI this started from the 2 PRs #1920 #1932 |
Hi @ilhaan |
Hey @Agazoth, thanks for opening the issue. We'll also discuss the topic with the other maintainers to define a general guideline. Also, as you mentioned the output of secrets, I'll tag the corresponding Bicep issue here: Azure/bicep#2163 . It doesn't sound like there'll be the feature we're hoping for BUT a depends on for existing resources which makes a previously discussed workaround a lot easier. But it's still a workaround, so your point remains valid regardless. PS: When it comes to the secrets alone, I'd argue that the optional 'store it in a key vault'-feature is a valid approach. So much so, that I'd argue that it would not need require a pattern as it is essentially just optionally storing some data - which happens to be a child-resource of key vault. Yet it remains very strongly correlated and in my mind does not imply a 'workload' that Patterns are 'usually' targeted towards. To be fair - the currently existing patterns are not really a good example, but that has different reasons. 😄 |
Off the top of my head we have 3 options
|
Hey @eriqua - happy for your insights. Option 2 would possibly mean, that we would have to create a pattern for each resource that emit secrets alongside a module that does not emit the secrets for each of those resources. The secrets generated at deployment time are almost always used elsewhere in a deployment, which makes the modules practically useless. Option 3 would mean, that we write the "workaround" sample into each resource module and it is the still up to the user to write the extra files for their deployment. I agree with all your points in option 1. Option 1 is also the solution I would advocate for:
All 3 options will leave us with work, that needs to be done: Option 1:
Option 2:
Option 3:
In a more general view on modules, I find great inspiration in this article from Breandan Thomsen https://brendanthompson.com/posts/2020/terraform-to-module-or-not-to-module. He is a Terraform ambassador and has been working with modules and module building for many years. He highlights these 9 points for module creation:
I see the first point as a clear indicator for option 1. Actually, it also solves the question about whether or not to create a module in option 2. Point 5 is also a good argument for option 1. The end-user of the module gets a built-in option to safely store the secret in a place, where Azure native tooling can manage and cycle the secrets. Point 7 would also favour option 1. The module can still be used in multiple use cases, whereas option 2 would need specific patterns for each secret emitting resource leaving out the module, much like option 3 just describes how to work around a super common use case that is not implemented in the module Point 9 too can be used for favouring option 1. The simplest example is the storage account. The storage account is used in a very high percentage of implementations and is almost always consumed by some other resource, that needs the functionality it provides. Stretching point 9 a bit, one could say, that a web app consumes the connection string or primary key of the storage account for the storage queue or DataLakeGen2 access. This means that secret emitting resource modules SHOULD NOT be mere wrappers. I think my opinion is quite clear. Having secrets emitted directly to key vault from the module is the right implementation until the changes on ARM allows us to emit secrets directly from the bicep resources. It gives the module users a simple, unified solution to get access to secrets from the modules, that is secure and supported by the Azure platform without having to remember to write workarounds every time a resource has a secret they want. Modules with this implementation will also work very well in Pattern implementations, that often deploy their own key vaults. And pattern implementations that reference existing key vaults for that matter. I'm looking forward to other viewpoints or angles and very eager to go to work on this - I have a private registry to dissolve 😸 |
Hey @AlexanderSehr and @eriqua did you manage to find anyone with additional opinions or ideas on how to handle secrets? |
Hey @Agazoth, For option 1,for example, I'd argue the interface/spec should ensure
Personally I think this option makes the most sense. I'd vote for OPT2 if it would be something big, but as it is (in this case) only a rather small extra deployment that just needs one parameter, I may just be simple enough to be part of the module itself. Aside the fact that one could argue it is strongly correlated. Option 3 aligns with what I described in an earlier conversation, that is, how I usually work around the limitation of Bicep when building solutions that needs resource data (like a storage account key). And while this sentiment did not change, I don't think an example test in AVM would make sense. If we were to add one, it would only be sitting in the test file, and not be pulled into the readme. In other words - you'd need to know where it is to find it and then try and make something out of it. It may just be too complicated. |
I had a bit of time so I did some trial & error how the interface could look like and currently ended up with the following proposal (using the CosmosDB solution as a use case): keyVaultExport.bicepBased on the solution of @bryansan-msft @description('Required. The name of the key vault to set the secrets in.')
param keyVaultName string
@description('Required. The secrets to set in the key vault.')
param keySecrets keySecret[]
resource keyVault 'Microsoft.KeyVault/vaults@2022-07-01' existing = {
name: keyVaultName
}
resource keySecretsSecrets 'Microsoft.KeyVault/vaults/secrets@2022-07-01' = [
for secret in keySecrets: {
name: secret.name
parent: keyVault
properties: {
value: secret.value
}
}
]
type keySecret = {
@description('Required. The name of the secret to set.')
name: string
@description('Required. The value of the secret to set.')
@secure()
value: string
} main.bicep@description('Optional. Key vault reference and secret settings for the module''s secrets export.')
param secretsExportConfiguration secretsExportConfigurationType?
module keyVaultTest 'modules/keyVaultExport.bicep' = if (secretsExportConfiguration != null) {
name: '${uniqueString(deployment().name, location)}-secrets-kv'
scope: resourceGroup(
split(secretsExportConfiguration!.keyVaultResourceId, '/')[2],
split(secretsExportConfiguration!.keyVaultResourceId, '/')[4]
)
params: {
keyVaultName: last(split(secretsExportConfiguration!.keyVaultResourceId, '/'))
keySecrets: union(
[],
secretsExportConfiguration.?setPrimaryWriteKey != false
? [
{
name: secretsExportConfiguration.?primaryWriteKeySecretName ?? 'Primary-Write-Key'
value: databaseAccount.listKeys().primaryMasterKey
}
]
: [],
secretsExportConfiguration.?setPrimaryReadOnlyKey != false
? [
{
name: secretsExportConfiguration.?primaryReadOnlyKeySecretName ?? 'Primary-Readonly-Key'
value: databaseAccount.listKeys().primaryReadonlyMasterKey
}
]
: []
// (...)
)
}
}
type secretsExportConfigurationType = {
@description('Required. The resource ID of the key vault where to store the secrets of this module.')
keyVaultResourceId: string
@description('Optional. The name for secret to create.')
primaryWriteKeySecretName: string?
@description('Optional. Set to control wether or not to create the secret. Defaults to true.')
setPrimaryWriteKey: bool?
@description('Optional. The name for secret to create.')
primaryReadOnlyKeySecretName: string?
@description('Optional. Set to control wether or not to create the secret. Defaults to true.')
setPrimaryReadOnlyKey: bool?
// (...)
} main.test.bicepmodule testDeployment '../../../main.bicep' = [
for iteration in ['init', 'idem']: {
scope: resourceGroup
name: '${uniqueString(deployment().name, enforcedLocation)}-test-${serviceShort}-${iteration}'
params: {
secretsExportConfiguration: {
keyVaultResourceId: keyVaultResourceId
primaryReadOnlyKeySecretName: 'myName'
setPrimaryWriteConnectionString: false
}
// (...)
}] I did play around with a couple different variants like
Nothing is yet decided, but I wanted to give it a shot anyways. Happy to hear some thoughts. |
I also found a bit of time to play around with different solutions, and I do agree with all your findings. The precondition in CosmosDB is, that we want to save ALL secrets but that might not really be the case. Currently it sets a predefined name for all the secrets if a secret name is not specified. In the real world, we often only need one or 2 specified secrets. If we change the logic so only defined secrets are exported to the key vault we can keep the optional string for the secret name and let that trigger the secret deployment, thus avoiding the enabled property. That would look something like this: keyVaultExport.bicep@description('Required. The name of the key vault to set the secrets in.')
param keyVaultName string
@description('Required. The secrets to set in the key vault.')
param keySecrets keySecret[]
resource keyVault 'Microsoft.KeyVault/vaults@2022-07-01' existing = {
name: keyVaultName
}
resource keySecretsSecrets 'Microsoft.KeyVault/vaults/secrets@2022-07-01' = [
for secret in keySecrets: if (secret.name != 'DONOTDEPLOY') {
name: secret.name!
parent: keyVault
properties: {
value: secret.value
}
}
]
type keySecret = {
@description('Name of the secret. If omitted, the secret is not set.')
name: string?
@description('Required. The value of the secret to set.')
@secure()
value: string
} Then we can simplify main: main.bicep@description('Optional. Key vault reference and secret settings for the module''s secrets export.')
param secretsExportConfiguration secretsExportConfigurationType?
module keyVaultTest 'modules/keyVaultExport.bicep' = if (secretsExportConfiguration != null) {
name: '${uniqueString(deployment().name, location)}-secrets-kv'
scope: resourceGroup(
split(secretsExportConfiguration!.keyVaultResourceId, '/')[2],
split(secretsExportConfiguration!.keyVaultResourceId, '/')[4]
)
params: {
keyVaultName: last(split(secretsExportConfiguration!.keyVaultResourceId, '/'))
keySecrets: [
{
name: secretsExportConfiguration.?primaryWriteKeySecretName ?? 'DONOTDEPLOY'
value: databaseAccount.listKeys().primaryMasterKey
}
{
name: secretsExportConfiguration.?primaryReadOnlyKeySecretName ?? 'DONOTDEPLOY'
value: databaseAccount.listKeys().primaryReadonlyMasterKey
}
// (...)
]
}
type secretsExportConfigurationType = {
@description('Required. The resource ID of the key vault where to store the secrets of this module.')
keyVaultResourceId: string
@description('Optional. The name for secret to create.')
primaryWriteKeySecretName: string?
@description('Optional. The name for secret to create.')
primaryReadOnlyKeySecretName: string?
// (...)
} Then running a deployment with the module would look a bit like this if key vault is in another rg main.test.biceptargetScope = 'subscription'
resource resourcerg 'Microsoft.Resources/resourceGroups@2024-03-01' = {
name: 'rg-resource'
location: 'westeurope'
}
resource kvrg 'Microsoft.Resources/resourceGroups@2024-03-01' = {
name: 'rg-keyvault'
location: 'westeurope'
}
module kv 'br/public:avm/res/key-vault/vault:0.6.1' = {
name: 'Deploy-KeyVault'
scope: kvrg
params: {
name: 'kv-demosecret'
location: kvrg.location
enableVaultForDeployment: true
enableSoftDelete: false
enablePurgeProtection: false
}
}
module res 'main.bicep' = {
name: 'Deploy-With-Key-Vault'
scope: resourcerg
params: {
secretsExportConfiguration: {
keyVaultId: kv.outputs.resourceId
primaryKeySecretName: 'PrimaryKeyDefined'
secondaryKeySecretName: 'ThisWorksToo'
}
}
} This way we can cover all secrets emitted by a resource with an option to place it in a key vault. The key vault can be pre-existing, given that the deployer has access, or be deployed in the same template. The 'DONOTDEPLOY' bit is not too pretty, but it is easier to read then the union array which i could not get to work with an optional name. I think we need to take a basic decision on, whether all secrets should be emitted to key vault or only defined secrets should go there.
|
Hey @Agazoth, just as a quick update to you: After some firefighting, I just put this item back on the agenda. I hope we can come to an agreement this Thursday, or the next. Fingers crossed. Please excuse that it takes longer than we would have hoped. |
That is great news @AlexanderSehr - I'm looking forward to it 🙂. Thanks for your continued effort. |
After discussing with both @eriqua & @ReneHezser, this is another proposal we agreed on that requires the user to opt in for every secret instead of storing all secrets by default. Also, the resource IDs of every secret created is returned which should be helpful to reference them again later. keyVaultExport.bicepBased on the solution of @bryansan-msft @description('Required. The name of the Key Vault to set the ecrets in.')
param keyVaultName string
@description('Required. The secrets to set in the Key Vault.')
param secretsToSet secretToSetType[]
resource keyVault 'Microsoft.KeyVault/vaults@2022-07-01' existing = {
name: keyVaultName
}
resource secrets 'Microsoft.KeyVault/vaults/secrets@2023-07-01' = [
for secret in secretsToSet: {
name: secret.name
parent: keyVault
properties: {
value: secret.value
}
}
]
@description('The references to the secrets exported to the provided Key Vault.')
output secretsSet secretSetType[] = [
#disable-next-line outputs-should-not-contain-secrets // Only returning the references, not a secret value
for index in range(0, length(secretsToSet ?? [])): {
secretResourceId: secrets[index].id
secretUri: secrets[index].properties.secretUri
}
]
// =============== //
// Definitions //
// =============== //
@export()
type secretSetType = {
@description('The resourceId of the exported secret.')
secretResourceId: string
@description('The secret URI of the exported secret.')
secretUri: string
}
type secretToSetType = {
@description('Required. The name of the secret to set.')
name: string
@description('Required. The value of the secret to set.')
@secure()
value: string
} main.bicep@description('Optional. Key vault reference and secret settings for the module\'s secrets export.')
param secretsExportConfiguration secretsExportConfigurationType?
module secretsExport 'modules/keyVaultExport.bicep' = if (secretsExportConfiguration != null) {
name: '${uniqueString(deployment().name, location)}-secrets-kv'
scope: resourceGroup(
split((secretsExportConfiguration.?keyVaultResourceId ?? '//'), '/')[2],
split((secretsExportConfiguration.?keyVaultResourceId ?? '////'), '/')[4]
)
params: {
keyVaultName: last(split(secretsExportConfiguration.?keyVaultResourceId ?? '//', '/'))
secretsToSet: union(
[],
contains(secretsExportConfiguration!, 'primaryWriteKeySecretName')
? [
{
name: secretsExportConfiguration!.primaryWriteKeySecretName
value: databaseAccount.listKeys().primaryMasterKey
}
]
: [],
contains(secretsExportConfiguration!, 'primaryReadOnlyKeySecretName')
? [
{
name: secretsExportConfiguration!.primaryReadOnlyKeySecretName
value: databaseAccount.listKeys().primaryReadonlyMasterKey
}
]
: []
// (...)
)
}
}
@description('A hashtable of references to the secrets exported to the provided Key Vault. The key of each reference is each secret\'s name.')
output exportedSecrets secretsOutputType = (secretsExportConfiguration != null)
? toObject(secretsExport.outputs.secretsSet, secret => last(split(secret.secretResourceId, '/')), secret => secret)
: {}
// =============== //
// Definitions //
// =============== //
type secretsExportConfigurationType = {
@description('Required. The resource ID of the key vault where to store the secrets of this module.')
keyVaultResourceId: string
@description('Optional. The primary write key secret name to create.')
primaryWriteKeySecretName: string?
@description('Optional. The primary readonly key secret name to create.')
primaryReadOnlyKeySecretName: string?
// (...)
}
import { secretSetType } from 'modules/keyVaultExport.bicep'
type secretsOutputType = {
@description('An exported secret\'s references.')
*: secretSetType
} main.test.bicepmodule testDeployment '../../../main.bicep' = [
for iteration in ['init', 'idem']: {
scope: resourceGroup
name: '${uniqueString(deployment().name, enforcedLocation)}-test-${serviceShort}-${iteration}'
params: {
secretsExportConfiguration: {
keyVaultResourceId: keyVaultResourceId
primaryReadOnlyKeySecretName: 'myPrimarySecret'
primaryWriteConnectionStringName: 'mySecondarySecret'
}
// (...)
}] With the output being a hashtable with the secret's name as the key, you could then reference the secret properties ID & URL via the name of the secret. For example // Used an output in the test file to return the result as shown below
output exportedSecrets object = testDeployment.outputs.exportedSecrets
output specificSecret string = testDeployment.outputs.exportedSecrets.myPrimarySecret.secretResourceId
output exportedSecretResourceIds array = map(
items(testDeployment.outputs.exportedSecrets),
item => item.value.secretResourceId
) Which results in an output like {
"exportedSecrets": {
"Type": "Object",
"Value": {
"myPrimarySecret": {
"secretResourceId": "/subscriptions/<subId>/resourceGroups/dep-avm-documentdb.databaseaccounts-dddamin-rg/providers/Microsoft.KeyVault/vaults/dep-avm-kvlt-dddamin/secrets/myPrimarySecret",
"secretUri": "https://dep-avm-kvlt-dddamin.vault.azure.net/secrets/myPrimarySecret"
},
"mySecondarySecret": {
"secretResourceId": "/subscriptions/<subId>/resourceGroups/dep-avm-documentdb.databaseaccounts-dddamin-rg/providers/Microsoft.KeyVault/vaults/dep-avm-kvlt-dddamin/secrets/mySecondarySecret",
"secretUri": "https://dep-avm-kvlt-dddamin.vault.azure.net/secrets/mySecondarySecret"
}
}
},
"specificSecret": {
"Type": "String",
"Value": "/subscriptions/<subId>/resourceGroups/dep-avm-documentdb.databaseaccounts-dddamin-rg/providers/Microsoft.KeyVault/vaults/dep-avm-kvlt-dddamin/secrets/myPrimarySecret"
},
"exportedSecretResourceIds": {
"Type": "Array",
"Value": [
"/subscriptions/<subId>/resourceGroups/dep-avm-documentdb.databaseaccounts-dddamin-rg/providers/Microsoft.KeyVault/vaults/dep-avm-kvlt-dddamin/secrets/mySecondarySecret",
"/subscriptions/<subId>/resourceGroups/dep-avm-documentdb.databaseaccounts-dddamin-rg/providers/Microsoft.KeyVault/vaults/dep-avm-kvlt-dddamin/secrets/myPrimarySecret"
]
}
} |
Hey @Agazoth,
I'll go ahead and define a spec for that to document it on AVM. In the meantime, if you find time, please update your PRs to the above implementation and we can get them right in. If you have any last-minute feedback, please let me know 💪 |
That is excellent news and a really good decision! I'll update the PRs accordingly as soon as I'm back from vacation (next week). Thank you for your persistence! |
Thank you for your partience. I'm sure we'll catch up soon. Enjoy your vacation, while you can 💪 |
## Description - Updated current secrets export interface to spec as per [ref](#1934 (comment)) - Updated to latest RBAC schema (cc: @krbar) - Addressed warnings ## Pipeline Reference <!-- Insert your Pipeline Status Badge below --> | Pipeline | | -------- | | [![avm.res.document-db.database-account](https://github.com/Azure/bicep-registry-modules/actions/workflows/avm.res.document-db.database-account.yml/badge.svg?branch=users%2Falsehr%2FemittedSecretsTest&event=workflow_dispatch)](https://github.com/Azure/bicep-registry-modules/actions/workflows/avm.res.document-db.database-account.yml) | ## Type of Change <!-- Use the checkboxes [x] on the options that are relevant. --> - [ ] Update to CI Environment or utilities (Non-module affecting changes) - [x] Azure Verified Module updates: - [ ] Bugfix containing backwards-compatible bug fixes, and I have NOT bumped the MAJOR or MINOR version in `version.json`: - [ ] Someone has opened a bug report issue, and I have included "Closes #{bug_report_issue_number}" in the PR description. - [ ] The bug was found by the module author, and no one has opened an issue to report it yet. - [ ] Feature update backwards compatible feature updates, and I have bumped the MINOR version in `version.json`. - [ ] Breaking changes and I have bumped the MAJOR version in `version.json`. - [ ] Update to documentation ## Checklist - [ ] I'm sure there are no other open Pull Requests for the same update/change - [ ] I have run `Set-AVMModule` locally to generate the supporting module files. - [ ] My corresponding pipelines / checks run clean and green without any errors or warnings <!-- Please keep up to date with the contribution guide at https://aka.ms/avm/contribute/bicep -->
Hey @Agazoth, @AlexanderSehr as specs are defined and the first PRs implementing those are merged, shall we close this issue? |
@eriqua that is fine with me. So glad this got implemented. Already using it in deployments and it works like a charm 😃 |
Very glad to hear @Agazoth, thanks for triggering this conversation, that's only how this feature got implemented 😉 Will wait for Alex to confirm as well and then close. |
Sounds good. Thanks 💪 |
Check for previous/existing GitHub issues
Description
Building bicep modules is an exercise in making the module as slim and close to the desired resource as possible while still keeping it secure and compatible with larger deployments.
A SQL Server should not deploy a virtual network, but it might need some network resources to add it to a subnet. Likewise, we want our web site module to configure monitoring, without deploying the Insights resources.
Currently, the pattern (ptn) section of the repo is quite slim. We have some authorization stuff, some policy stuff and some security center stuff. According to the Module Classifications, a Pattern Module deploys multiple modules, usually in in alignment with the patterns in Azure Architect Center - that makes it safe to say, that we do not really have any pattern modules yet.
A Pattern module would always consist of several resources, that are deployed together in one or more resource groups distributed over one or more subscriptions. One might say, that any IaC deployment author is developing a pattern deployment for the specific project she is working on.
Authoring these patterns should be easy and secure and leverage the built-in benefits of the given language, be it bicep, terraform or others. If we need to attach a new cloud native application to an existing CAF Enterprise Scale environment, we need an easy way to add the application to the network, implement the required logging and store the secrets from the deployment in the appointed key vault.
Resources have some common requirements or behaviors that we need to address in a common way in order to keep the AVM slim and attractive. These are the things we always need to consider:
We often handle these requirements with the "existing" resource inside the module structure to reference the required endpoint for the integration of the specific resource we build in the module for. A quick count in the current repo reveals 358 "existing =" strings in 256 files. 32 results in 29 files are for existing key vault resources, 47 results in 46 files are for network resources and 14 results in 14 files are for insights.
The point of this specific discussion is, how we find a common way to handle resources, that emit secrets. These secrets are often used in other modules and will be visible in the deployment log, if they are added to the output of the creating module.
Ideally modules should be able to emit
@secret
values or directly available in the module output, but that is not the case now and it does not seem to be solved in any sprint anytime soon.If we want to make a secret emitting resource safe and secure for the deployment builder, we have 2 options:
I would love to hear pros and cons for different solutions. Alternative ways can also be discussed. I hope this discussion can help us find common grounds to make well structured and secure modules that deployment developers like to use.
The text was updated successfully, but these errors were encountered: