-
Notifications
You must be signed in to change notification settings - Fork 56
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
Conversation
This file contains hidden or 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
c1e38db
to
1da0c28
Compare
isra-fel
reviewed
Feb 20, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Overall, we should minimize public types in common repo, because more public stuff means more dependencies, and more dependencies makes it difficult to maintain.
- Instead of instanciate AzurePSSanitizer for every cmdlet, consider designing it as a singleton component like IConfigManager. This is for performance.
… ISanitizerSettings interface.
Following changes were made.
|
…erties that may cause performance concern like lazy load properties.
35515e6
to
92d8b38
Compare
vidai-msft
commented
Feb 27, 2024
isra-fel
approved these changes
Feb 28, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
msJinLei
approved these changes
Feb 28, 2024
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.
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 constantShowSecretsWarning
to theConfigKeysForCommon
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 newSanitizer
object and methods for detecting secrets in the output of Azure PowerShell commands. TheSanitizer
object is used in theWriteObject
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 theWriteShowSecretsWarningMessage
method.src/Common/MetricHelper.cs
: Modified thePopulatePropertiesFromQos
method and theAzurePSQoSEvent
class to include information about detected secrets in the telemetry data.src/Common/Properties/Resources.Designer.cs
andsrc/Common/Properties/Resources.resx
: Added a new localized stringShowSecretsWarningMessage
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 theSanitizer
object used for detecting secrets in the output. TheAzurePSSanitizer
class contains the main logic for sanitizing objects and detecting secrets. TheDefaultProviderResolver
andDefaultSanitizerSettings
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.