-
Notifications
You must be signed in to change notification settings - Fork 3k
Add CredScan cleanup guide #22750
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
Add CredScan cleanup guide #22750
Conversation
This pull request is protected by Check Enforcer. What is Check Enforcer?Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass. Why am I getting this message?You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged. What should I do now?If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows: What if I am onboarding a new service?Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment: |
|
||
## Check CredScan status | ||
|
||
CredScan is run each night over the entire `azure-sdk-for-python` repository as part of the |
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.
We are considering to add the CredScan check inside of PR validation in .NET repo and once we get that figured out we could do that similarly in python repo if you guys feel that would be helpful for you guys.
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.
That would definitely be great to implement once that's ready! I synced with Sima about the PR validation work and think it'll be a great solution moving forward. This guide will hopefully help Python cut through our current backlog of CredScan warnings and get us in a position to use PR validation.
fake credential files, but strings in that file will still trigger warnings if present in another unsuppressed file. | ||
Files that contain more than just fake credentials shouldn't be suppressed. | ||
|
||
Credential warnings are suppressed in [eng/CredScanSuppression.json][suppression_file]. Suppressed string values are in |
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.
It is recommended to try and reuse an existing placeholder instead of adding new ones.
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.
Agreed -- I hope that's clear in this doc. Do you think this list of suppression options is accurate, in terms of which options are preferable?
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.
Yes I think that does cover it but I guess I missed it reading through the other text. Perhaps it might be worth moving up those steps so it is nice summary and add the text after for more details about the specifics.
doc/dev/credscan_process.md
Outdated
Credential warnings are suppressed in [eng/CredScanSuppression.json][suppression_file]. Suppressed string values are in | ||
the `"placeholder"` list, and suppressed files are in the `"file"` list under `"suppressions"`. | ||
|
||
Ideally, fake credential files -- which contain nothing but fake secrets -- should be suppressed and their fake |
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.
If you suppress an entire file that entire file should only contain fake secrets. We should never suppress an entire file if there is other normal code because we don't want to accidently allow other secrets to slip in a file.
/check-enforcer override |
Thanks for drafting this! We will use this as initial template. |
Description
This adds a guide for cleaning up CredScan warnings so that package owners can clean up warnings for packages they own, on their own.
All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines