Skip to content

azurerm_static_webapp - Mark app_settings sensitive in schema#28689

Merged
jackofallops merged 1 commit intohashicorp:mainfrom
procter-gamble-oss:feat/static-webapp-app-settings-sensitive
Apr 1, 2025
Merged

azurerm_static_webapp - Mark app_settings sensitive in schema#28689
jackofallops merged 1 commit intohashicorp:mainfrom
procter-gamble-oss:feat/static-webapp-app-settings-sensitive

Conversation

@arnoldrw
Copy link
Contributor

@arnoldrw arnoldrw commented Feb 4, 2025

Community Note

  • Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
  • Please do not leave comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

Description

This pr updates app_settings in static_webapp_resource to be a sensitive field.

Official Microsoft setup documentation related to setting up authentication for a static webapp indicate that setting an EntraID app registration client secret as an app setting is an expected configuration. Currently with app_settings not marked as a sensitive field, this results in the actual secret value being displayed.

PR Checklist

  • I have followed the guidelines in our Contributing Documentation.
  • I have checked to ensure there aren't other open Pull Requests for the same update/change.
  • I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
  • I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
  • I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
    For example: “resource_name_here - description of change e.g. adding property new_property_name_here

Changes to existing Resource / Data Source

  • I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
  • I have written new tests for my resource or datasource changes & updated any relevent documentation.
  • I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
  • (For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

  • My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

  • static_webapp_resource - Make app_settings sensitive

This is a (please select all that apply):

  • Bug Fix
  • New Feature (ie adding a service, resource, or data source)
  • Enhancement
  • Breaking Change

Related Issue(s)

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

@bubbletroubles
Copy link
Contributor

Isn't the fix here to store the secret in a keyvault, then reference the keyvault from the app settings? See https://learn.microsoft.com/en-us/azure/static-web-apps/key-vault-secrets

@arnoldrw
Copy link
Contributor Author

arnoldrw commented Feb 6, 2025

Isn't the fix here to store the secret in a keyvault, then reference the keyvault from the app settings? See https://learn.microsoft.com/en-us/azure/static-web-apps/key-vault-secrets

Keyvault integration is certainly one answer but I do not believe it can/should be the only answer for a few reasons eg:

In the current state of the provider, the only option to hide these secrets is to use a key vault integration. This forces developers down a single path, when there are multiple available. The provider should be unopinionated on something that comes down to a choice by the developer. In the current state, if a developer wants to treat app_settings as sensitive they have no ability to do so as their resource cannot override the provider output. On the flip side if this change is made and a developer wants to see the values of app_settings when it is marked as sensitive they have that option available to them.

@jackofallops jackofallops changed the title static_webapp_resource - Make app_settings sensitive azurerm_static_webapp - Mark app_settings sensitive in schema Apr 1, 2025
Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

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

Thanks @arnoldrw - LGTM 👍

@jackofallops jackofallops merged commit 8ac2f11 into hashicorp:main Apr 1, 2025
4 checks passed
@github-actions github-actions bot added this to the v4.26.0 milestone Apr 1, 2025
jackofallops added a commit that referenced this pull request Apr 1, 2025
jackofallops added a commit that referenced this pull request Apr 4, 2025
* new changelog for 4.26.0

* Update CHANGELOG.md #29212

* Update CHANGELOG.md #28786

* Update CHANGELOG.md for #29095

* Update CHANGELOG.md #29225

* Update CHANGELOG.md #29168

* Update CHANGELOG.md #28890

* Update CHANGELOG.md for #28689

* Update CHANGELOG.md #28354

* Update CHANGELOG.md #29240

* Update CHANGELOG.md for #29211

* Update CHANGELOG.md #29214

* Update CHANGELOG.md for #29209

* Update CHANGELOG.md for 28690

* Update CHANGELOG.md for #28577

* Update for #29217 #29144 #29145

* Update CHANGELOG.md for #29185

* prep for release

---------

Co-authored-by: sreallymatt <106555974+sreallymatt@users.noreply.github.com>
Co-authored-by: catriona-m <86247157+catriona-m@users.noreply.github.com>
Co-authored-by: Wyatt Fry <wyattfry@gmail.com>
Co-authored-by: Matthew Frahry <mbfrahry@gmail.com>
Co-authored-by: Wodans Son <20408400+WodansSon@users.noreply.github.com>
Co-authored-by: stephybun <steph@hashicorp.com>
@feichler-or
Copy link

@arnoldrw you mentioned

On the flip side if this change is made and a developer wants to see the values of app_settings when it is marked as sensitive they have that option available to them.

How would I exactly do that? When I just set app_settings = nonsensitive(...) the terraform plan output still shows

~ app_settings = (sensitive value)

instead of previously (up to version 4.25)

 ~ app_settings = {
          + "Some:Connection"                                = (sensitive value)
          + "Some:Name"                                      = "name"
          + "Some:Value"                                     = "200"
            # (21 unchanged elements hidden)
        }

@dantape
Copy link

dantape commented Apr 15, 2025

@jackofallops I think you need to seriously consider reverting this PR and @arnoldrw I think you made the changes on the wrong resource.

The title of the PR indicates that these changes were meant for the azurerm_static_webapp resource. However, upon further investigation, the changes appear to have been made to the azurerm_windows_function_app resource.

What was actually intended here? I believe this change is hiding all of our app_settings on windows function apps. With all the options available with Key Vault and App Config Service there is no reason to ever store sensitive values in the app_settings for a windows function app.

teowa pushed a commit to teowa/terraform-provider-azurerm that referenced this pull request May 8, 2025
teowa pushed a commit to teowa/terraform-provider-azurerm that referenced this pull request May 8, 2025
* new changelog for 4.26.0

* Update CHANGELOG.md hashicorp#29212

* Update CHANGELOG.md hashicorp#28786

* Update CHANGELOG.md for hashicorp#29095

* Update CHANGELOG.md hashicorp#29225

* Update CHANGELOG.md hashicorp#29168

* Update CHANGELOG.md hashicorp#28890

* Update CHANGELOG.md for hashicorp#28689

* Update CHANGELOG.md hashicorp#28354

* Update CHANGELOG.md hashicorp#29240

* Update CHANGELOG.md for hashicorp#29211

* Update CHANGELOG.md hashicorp#29214

* Update CHANGELOG.md for hashicorp#29209

* Update CHANGELOG.md for 28690

* Update CHANGELOG.md for hashicorp#28577

* Update for hashicorp#29217 hashicorp#29144 hashicorp#29145

* Update CHANGELOG.md for hashicorp#29185

* prep for release

---------

Co-authored-by: sreallymatt <106555974+sreallymatt@users.noreply.github.com>
Co-authored-by: catriona-m <86247157+catriona-m@users.noreply.github.com>
Co-authored-by: Wyatt Fry <wyattfry@gmail.com>
Co-authored-by: Matthew Frahry <mbfrahry@gmail.com>
Co-authored-by: Wodans Son <20408400+WodansSon@users.noreply.github.com>
Co-authored-by: stephybun <steph@hashicorp.com>
@github-actions
Copy link
Contributor

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 16, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants