[Bug Fix] - azuredevops_serviceendpoint_npm Remove token hash

[xuzhang3]

All Submissions:

• Have you added an explanation of what your changes do and why you'd like us to include them?
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.
• My code follows the code style of this project.
• I ran lint checks locally prior to submission.
• Have you checked to ensure there aren't other open PRs for the same update/change?

What about the current behavior has changed?

Remove access_token hash

Issue Number: #692

=== PAUSE TestAccServiceEndpointNpm_complete
=== RUN   TestAccServiceEndpointNpm_update
=== PAUSE TestAccServiceEndpointNpm_update
=== RUN   TestAccServiceEndpointNpm_RequiresImportErrorStep
=== PAUSE TestAccServiceEndpointNpm_RequiresImportErrorStep
=== CONT  TestAccServiceEndpointNpm_basic
=== CONT  TestAccServiceEndpointNpm_update
=== CONT  TestAccServiceEndpointNpm_RequiresImportErrorStep
=== CONT  TestAccServiceEndpointNpm_complete
--- PASS: TestAccServiceEndpointNpm_complete (48.02s)
--- PASS: TestAccServiceEndpointNpm_RequiresImportErrorStep (51.90s)
--- PASS: TestAccServiceEndpointNpm_basic (58.12s)
--- PASS: TestAccServiceEndpointNpm_update (86.36s)
PASS
ok      github.com/microsoft/terraform-provider-azuredevops/azuredevops/internal/acceptancetests        86.894s

Does this introduce a change to go.mod, go.sum or vendor/?

• Yes
• No

Does this introduce a breaking change?

• Yes
• No

Any relevant logs, error output, etc?

(If it’s long, please paste to https://ghostbin.com/ and insert the link here.)

Other information

[davidcorrigan714]

@xuzhang3 Why are you removing the token hash? If you've decided that masking secret data in state is no longer an objective of this provider then it should be all or nothing. There's also other areas of the code affected by the same issue, well really all of them, but I'll ping you on the Artifactory PR about it specifically.

[xuzhang3]

@davidcorrigan714 The hash function is used to encode the password/token to forbidden this sensitive data leak to state/console/logs. Latest Terraform versions will forbidden the sensitive data output to console by default. Encoding does not do much help.
And you are right, the update should not be that rough, just delete does not do any compatible. This is a breaking change to cx, an update will be triggered after update the provider and cx just need to apply it. The service connection will not be changed but update the state

[davidcorrigan714]

@xuzhang3 Sensitive data in terraform is a very complex topic, as evident by their Issue on it. Outputting sensitive data to the console is one of many issues. The larger one is the handling of the data in the state file. This change effectively unmasks user data that may be stored in version control. If a user is relying on that behavior and currently storing their state files in source code this change may unmask sensitive data and cause them to inadvertently commit secrets to their VCS.

HashiCorp's current recommendation seems to be that storing sensitive data in state is fine under the assumption that users are using secure storage backend. That's a bit of a controversial outlook, but users generally expect the handling of sensitive data as plain text in the state file to be explicitly called out in the provider documentation, which this change did not add and changing the behavior on existing data could very well unmask user's data who were relying on the old behavior. See the GitHub, AzureRM, or JFrog providers for examples on what users generally expect those warnings to look like.

This commit also just doesn't fix the actual issue. BCrypt should not have been used for secret memo, at all. At the least it's completely unsuitable for Generic service connections where no guarantees can be made about the token length. I'm not going to check the full list of service connection types because it's a fruitless endeavor, caring about the token format isn't something that should be handled in the provider for most cases.

[xuzhang3]

@davidcorrigan714 There is a another issue needs to fixed if the sensitive data need to be encoded. Usually there is no error, but when user updates other attributes like "description". After the update, sensitive data will be set to an empty string. This is because the raw data will be set to an empty string after encoding the sensitive data. One way is always to get the data from raw configuration and ignore the state.