Improve reporting when assigning a secure value to an insecure target or vice versa #16665
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Test this change out locally with the following install scripts (Action run 13952431314) VSCode
Azure CLI
|
jeskew
commented
Mar 19, 2025
| @@ -52,7 +52,9 @@ resource aks 'Microsoft.ContainerService/managedClusters@2020-03-01' = { | |||
| } | |||
| servicePrincipalProfile: { | |||
| clientId: servicePrincipalClientId | |||
| //@[22:46) [BCP417 (Info)] The supplied value has been marked as secure but is being assigned to a target that is not expecting sensitive data. (bicep https://aka.ms/bicep/core-diagnostics#BCP417) |servicePrincipalClientId| | |||
| secret: servicePrincipalClientSecret | |||
There was a problem hiding this comment.
There are a couple of cases where I expected the RP to mark something as sensitive in the Swagger, but they didn't. Besides this target, the other big surprises were resourceInput<'Microsoft.Compute/virtualMachines@2019-12-01'>.properties.osProfile.adminPassword (annotated as a regular string) and resourceInput<'Microsoft.KeyVault/vaults/secrets@2019-09-01'>.properties.value (ditto). I didn't check all API versions for those.
Dotnet Test Results 56 files - 61 56 suites - 61 28m 23s ⏱️ - 22m 8s For more details on these failures, see this check. Results for commit 2afb14a. ± Comparison against base commit 180a858. This pull request removes 3787 and adds 595 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
jeskew
commented
Mar 19, 2025
| @@ -125,8 +125,11 @@ param additionalMetadata string | |||
| @maxLength(24) | |||
| @allowed([ | |||
| 'one' | |||
| //@[02:007) [BCP418 (Warning)] The assignment target is expecting sensitive data but has been provided a non-sensitive value. Consider supplying the value as a secure parameter instead to prevent unauthorized disclosure to users who can view the template (via the portal, the CLI, or in source code). (bicep https://aka.ms/bicep/core-diagnostics#BCP418) |'one'| | |||
There was a problem hiding this comment.
I'm not sure about this one. Using @allowed([]) and @secure() together seems like a niche use case (the value is a secret, but I will hardcode which values are acceptable in the template), and I couldn't figure out if it's better to warn here because the elements in the @allowed() array aren't protected in any way or drop the warning because the elements in the @allowed() array aren't protected in any way and users presumably know that.
… (wrt flag composition).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #16252
Microsoft Reviewers: Open in CodeFlow