Skip to content

Impelement secrets detection for Az modules #412

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

Merged
merged 8 commits into from
Feb 28, 2024

Conversation

vidai-msft
Copy link
Contributor

This pull request introduces changes primarily focused on adding a new feature for detecting and warning about potential secrets in the output of Azure PowerShell commands. The changes span across multiple files, with the addition of a new Sanitizer class and modifications to existing classes to incorporate the use of this new class.

Here are the most important changes:

New feature for secrets detection:

  • src/Authentication.Abstractions/Models/ConfigKeysForCommon.cs: Added a new constant ShowSecretsWarning to the ConfigKeysForCommon class. This constant will be used to control whether the new secrets detection feature is enabled. It is disabled by default for now.

  • src/Common/AzurePSCmdlet.cs: Added a new Sanitizer object and methods for detecting secrets in the output of Azure PowerShell commands. The Sanitizer object is used in the WriteObject methods to sanitize the output before it's written to the pipeline. If any secrets are detected, a warning message is written to the console in the WriteShowSecretsWarningMessage method.

  • src/Common/MetricHelper.cs: Modified the PopulatePropertiesFromQos method and the AzurePSQoSEvent class to include information about detected secrets in the telemetry data.

  • src/Common/Properties/Resources.Designer.cs and src/Common/Properties/Resources.resx: Added a new localized string ShowSecretsWarningMessage for the warning message displayed when secrets are detected in the output.

  • src/Common/Sanitizer/AzurePSSanitizer.cs, src/Common/Sanitizer/DefaultProviderResolver.cs, src/Common/Sanitizer/DefaultSanitizerSettings.cs: Added new classes for the Sanitizer object used for detecting secrets in the output. The AzurePSSanitizer class contains the main logic for sanitizing objects and detecting secrets. The DefaultProviderResolver and DefaultSanitizerSettings classes provide additional support for the sanitization process.

  • The main logic to detect secrets in the output object is to traverse each property of this object with pre-defined types, which are SanitizerCollectionProvider, SanitizerDictionaryProvider, SanitizerJsonArrayProvider, SanitizerJsonObjectProvider, SanitizerStringProvider. If later on, new types are necessary, new providers as classes will be added and no existing class modification is needed.

@vidai-msft vidai-msft force-pushed the vidai/feature-secrets-handling branch from c1e38db to 1da0c28 Compare February 20, 2024 03:23
Copy link
Member

@isra-fel isra-fel left a comment

Choose a reason for hiding this comment

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

  1. Overall, we should minimize public types in common repo, because more public stuff means more dependencies, and more dependencies makes it difficult to maintain.
  2. Instead of instanciate AzurePSSanitizer for every cmdlet, consider designing it as a singleton component like IConfigManager. This is for performance.

@vidai-msft
Copy link
Contributor Author

  1. Overall, we should minimize public types in common repo, because more public stuff means more dependencies, and more dependencies makes it difficult to maintain.
  2. Instead of instanciate AzurePSSanitizer for every cmdlet, consider designing it as a singleton component like IConfigManager. This is for performance.

Following changes were made.

  1. ISanitizerSettings was renamed to ISanitizerService and the methods were also updated per discussion.
  2. Moved DefaultSanitizerService and AzurePSSanitizer from out of common to Az.Accounts.

@vidai-msft vidai-msft force-pushed the vidai/feature-secrets-handling branch from 35515e6 to 92d8b38 Compare February 25, 2024 14:56
@vidai-msft vidai-msft requested a review from isra-fel February 27, 2024 15:40
Copy link
Member

@isra-fel isra-fel left a comment

Choose a reason for hiding this comment

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

LGTM

@msJinLei msJinLei merged commit 0b6cb59 into main Feb 28, 2024
@vidai-msft vidai-msft deleted the vidai/feature-secrets-handling branch April 2, 2024 03:28
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.

3 participants