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

linter rule: outputs-should-not-contain-secrets #4716

Merged
merged 31 commits into from
Dec 9, 2021

Conversation

StephenWeatherford
Copy link
Contributor

@StephenWeatherford StephenWeatherford commented Oct 4, 2021

Parity with TTK: https://docs.microsoft.com/en-us/azure/azure-resource-manager/templates/template-test-cases#outputs-cant-include-secrets

image

Fixes #3973

Proposed documentation

Outputs should not contain secrets

Code: outputs-should-not-contain-secrets

Description: Don't include any values in an output that could potentially expose secrets. For example, secure parameters of type secureString or secureObject, or list* functions such as listKeys.

The output from a template is stored in the deployment history, so a malicious user could find that information.

The following example fails because it includes a secure parameter in an output value.

@secure()
param secureParam string

output badResult string = 'this is the value ${secureParam}'

The following example fails because it uses a list* member function in an output.

param storageName string
resource stg 'Microsoft.Storage/storageAccounts@2021-04-01' existing = {
  name: storageName
}

output badResult object = {
  value: stg.listKeys().keys[0].value
}

The following example fails because it uses a list* function in an output.

param storageName string
resource stg 'Microsoft.Storage/storageAccounts@2021-04-01' existing = {
  name: storageName
}

output badResult object = {
  value: listKeys(resourceId('Microsoft.Storage/storageAccounts', stg.name), '2021-02-01')
}

The following example fails because the output name contains 'password', indicating that it may contains a secret

output accountPassword string = '...'

@StephenWeatherford StephenWeatherford marked this pull request as draft October 4, 2021 18:22
@StephenWeatherford StephenWeatherford marked this pull request as ready for review October 6, 2021 04:12
@anthony-c-martin
Copy link
Member

This needs discussion - currently we have no mechanism in Bicep to return secure outputs from modules. It feels like we ought to prioritize #2163, otherwise it'll be difficult to practically action any fix for this linter rule without significant restructuring of the deployment and additional verbosity.

@alex-frankel
Copy link
Collaborator

I'm all for prioritizing #2163, but until that is done, I think this is an important linter check. You are right that refactoring this is not a quick change, but it is an important one.

@StephenWeatherford
Copy link
Contributor Author

I had asked Brian:

What do people do do fix this? It would think this type thing would be common, especially from a nested template? (Or are nested templates excepted?)

his answer:

nested templates are not excepted... you would pass the resourceId of the resource you need and then list inline where it's used.

@StephenWeatherford StephenWeatherford changed the title linter rule: outputs-should-not-contain-secrets linter rule: outputs-should-not-contain-secrets [BLOCKED] Oct 13, 2021
@StephenWeatherford StephenWeatherford changed the title linter rule: outputs-should-not-contain-secrets [BLOCKED] linter rule: outputs-should-not-contain-secrets Nov 19, 2021
@StephenWeatherford
Copy link
Contributor Author

StephenWeatherford commented Nov 19, 2021

@ucheNkadiCode Added you as reviewer. The new strings are here: https://github.com/Azure/bicep/pull/4716/files#diff-79c99ca30f6a204d8d88d4804da49247990c9ddeb32282ad424763d739ee0ec3. You can also see the proposed documentation above, but there will be a separate review for that.

Example errors:

image

@StephenWeatherford
Copy link
Contributor Author

@ucheNkadiCode Ready for string review

Stephen Weatherford added 2 commits December 6, 2021 10:19
@ucheNkadiCode
Copy link
Contributor

@ucheNkadiCode Uche Nkadi FTE Ready for string review

This looks good! Thanks for changing the "don't" statements to should not

Copy link
Member

@majastrz majastrz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@StephenWeatherford StephenWeatherford merged commit e2335cc into main Dec 9, 2021
@StephenWeatherford StephenWeatherford deleted the sw/outputs-secrets-rule branch December 9, 2021 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Linter rule: Outputs can't include secrets
6 participants