-
Notifications
You must be signed in to change notification settings - Fork 5.4k
Suppress credscan false positives #38026
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
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
5cded92
Suppress initial cred issues
fac0d23
Another bunch of supresses
e838ec3
Clean up
c371aae
Another bunch of supresses
3d177ee
Merge remote-tracking branch 'upstream/master' into jajahoda/credscan1
186991e
Revert to suppression messages
24846a5
Clean up
3fe5e24
Apply suggestions from code review
aik-jahoda 921d9f0
Revert passwords literals
b2fe3e2
Fix suppression justification comment
7332f66
Merge remote-tracking branch 'upstream/master' into jajahoda/credscan1
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,49 +1,70 @@ | ||
| { | ||
| "tool": "Credential Scanner", | ||
| "suppressions": [ | ||
| { | ||
| "file": [ | ||
| "/eng/common/internal-feed-operations.ps1", | ||
| "/eng/common/internal-feed-operations.sh", | ||
| "/src/libraries/Common/src/Interop/Windows/WinHttp/Interop.winhttp_types.cs", | ||
| "/src/libraries/Common/src/System/Security/Cryptography/EccSecurityTransforms.cs", | ||
| "/src/libraries/Common/tests/System/Net/Configuration.Certificates.cs", | ||
| "/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.Authentication.cs", | ||
| "/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.cs", | ||
| "/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.DefaultProxyCredentials.cs", | ||
| "/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.Proxy.cs", | ||
| "/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.ServerCertificates.cs", | ||
| "/src/libraries/Common/tests/System/Net/Http/PostScenarioTest.cs", | ||
| "/src/libraries/Common/tests/System/Net/Prerequisites/Deployment/setup_certificates.ps1", | ||
| "/src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/EC/ECKeyFileTests.cs", | ||
| "/src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/EC/ECKeyFileTests.LimitedPrivate.cs", | ||
| "/src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/RSA/RSAKeyFileTests.cs", | ||
| "/src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/RSA/RSAKeyPemTests.cs", | ||
| "/src/libraries/System.Data.Common/tests/System/Data/Common/DbConnectionStringBuilderTest.cs", | ||
| "/src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs", | ||
| "/src/libraries/System.DirectoryServices.AccountManagement/src/System/DirectoryServices/AccountManagement/constants.cs", | ||
| "/src/libraries/System.DirectoryServices.AccountManagement/tests/PrincipalTest.cs", | ||
| "/src/libraries/System.DirectoryServices.AccountManagement/tests/UserPrincipalTest.cs", | ||
| "/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs", | ||
| "/src/libraries/System.Net.Http/tests/UnitTests/DigestAuthenticationTests.cs", | ||
| "/src/libraries/System.Net.Http/tests/UnitTests/HttpEnvironmentProxyTest.cs", | ||
| "/src/libraries/System.Net.Mail/tests/Functional/SmtpClientTest.cs", | ||
| "/src/libraries/System.Net.Requests/src/System/Net/FtpControlStream.cs", | ||
| "/src/libraries/System.Net.Requests/src/System/Net/FtpWebRequest.cs", | ||
| "/src/libraries/System.Net.WebSockets.Client/tests/ConnectTest.cs", | ||
| "/src/libraries/System.Private.Uri/tests/ExtendedFunctionalTests/UriRelativeResolutionTest.cs", | ||
| "/src/libraries/System.Private.Uri/tests/FunctionalTests/UriBuilderRefreshTest.cs", | ||
| "/src/libraries/System.Private.Uri/tests/FunctionalTests/UriBuilderTests.cs", | ||
| "/src/libraries/System.Private.Uri/tests/FunctionalTests/UriRelativeResolutionTest.cs", | ||
| "/src/libraries/System.Runtime/tests/System/Uri.CreateStringTests.cs", | ||
| "/src/libraries/System.Security.Cryptography.Algorithms/tests/Rfc2898Tests.cs", | ||
| "/src/libraries/System.Security.Cryptography.Pkcs/tests/Pkcs12/Pkcs12Documents.cs", | ||
| "/src/libraries/System.Security.Cryptography.X509Certificates/tests/ExportTests.cs", | ||
| "/src/libraries/System.Security.Cryptography.Xml/tests/EncryptedXmlTest.cs", | ||
| "/src/libraries/System.Security.Cryptography.Xml/tests/SignedXmlTest.cs", | ||
| "/src/libraries/System.Security.Cryptography.Xml/tests/TestHelpers.cs" | ||
| ], | ||
| "_justification": "Mostly test files. Other files contain harmless examples or constants." | ||
| }, | ||
| ] | ||
| "tool": "Credential Scanner", | ||
| "suppressions": [ | ||
| { | ||
| "_justification": "Unit test containing connection strings under the test.", | ||
| "file": [ | ||
| "src/libraries/System.Data.Common/tests/System/Data/Common/DbConnectionStringBuilderTest.cs" | ||
| ] | ||
| }, | ||
| { | ||
| "_justification": "Private key for testing purpose.", | ||
| "file": [ | ||
| "src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/DSA/DSAKeyPemTests.cs", | ||
| "src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/EC/ECKeyPemTests.cs", | ||
| "src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/RSA/RSAKeyPemTests.cs", | ||
| "src/libraries/System.Security.Cryptography.X509Certificates/tests/TestData.cs" | ||
| ], | ||
| "placeholder": [ | ||
| "-----BEGIN PRIVATE KEY-----", | ||
| "-----BEGIN * PRIVATE KEY-----" | ||
| ] | ||
| }, | ||
| { | ||
| "_justification": "Test credential for Uri testing", | ||
| "file": [ | ||
| "src/libraries/System.Net.Http/tests/UnitTests/HttpEnvironmentProxyTest.cs", | ||
| "src/libraries/System.Private.Uri/tests/ExtendedFunctionalTests/UriRelativeResolutionTest.cs", | ||
| "src/libraries/System.Private.Uri/tests/FunctionalTests/UriBuilderRefreshTest.cs", | ||
| "src/libraries/System.Private.Uri/tests/FunctionalTests/UriBuilderTests.cs", | ||
| "src/libraries/System.Private.Uri/tests/FunctionalTests/UriRelativeResolutionTest.cs", | ||
| "src/libraries/System.Runtime/tests/System/Uri.CreateStringTests.cs" | ||
| ], | ||
| "placeholder": [ | ||
| "//*:;&$=123USERINFO@", | ||
| "//*:bar@", | ||
| "//*:bar1@", | ||
| "//*:password1@", | ||
| "//*:psw@", | ||
| "//*:userinfo2@" | ||
| ] | ||
| }, | ||
| { | ||
| "_justification": "Generic test password.", | ||
| "file": [ | ||
| "src/libraries/Common/tests/System/Net/Configuration.Certificates.cs", | ||
| "src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.Authentication.cs", | ||
| "src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.cs", | ||
| "src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.DefaultProxyCredentials.cs", | ||
| "src/libraries/Common/tests/System/Net/Http/PostScenarioTest.cs", | ||
| "src/libraries/Common/tests/System/Net/Prerequisites/Deployment/setup_certificates.ps1", | ||
| "src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs", | ||
| "src/libraries/System.Net.Http/tests/UnitTests/DigestAuthenticationTests.cs", | ||
| "src/libraries/System.Net.Http/tests/UnitTests/HttpEnvironmentProxyTest.cs", | ||
| "src/libraries/System.Net.Mail/tests/Functional/SmtpClientTest.cs", | ||
| "src/libraries/System.Security.Cryptography.Xml/tests/SignedXmlTest.cs", | ||
| "src/libraries/System.Security.Cryptography.Xml/tests/TestHelpers.cs" | ||
| ], | ||
| "placeholder": [ | ||
| "\"anotherpassword\"", | ||
| "\"bar\"", | ||
| "\"mono\"", | ||
| "\"password1\"", | ||
| "\"rightpassword\"", | ||
| "\"testcertificate\"", | ||
| "\"unused\"", | ||
| "\"wrongpassword\"" | ||
| ] | ||
| } | ||
| ] | ||
| } |
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
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
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
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
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
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
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
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
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
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
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
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
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
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
Oops, something went wrong.
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.
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.
Why are these commented out? Shouldn't they either be deleted or uncommented? Or does the tooling pay attention to commented out SuppressMessage attributes?
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, it is official way how to suppress report for the next line.
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.
I wonder how we will remember to not remove these as dead code! Would this also be recognized?
// [SuppressMessage("Microsoft.Security", "CS002:SecretInNextLine", Justification="It is property descriptor, not secret value.")] // Commented line recognized by credscan tool?
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.
My preference would be actually running the analyzer in dotnet/runtime.
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.
Can you clarify, I thought that's what @aik-jahoda is doing.
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.
Very few direct check-ins are made to the private mirror. It makes no sense to me to run analyzers over that source that we don't also run as part of the public repo, where 99.999% of all commits come from. Doing that only causes additional dev work and headaches, and it's caused problems as well, where mirroring fails because the already merged commits then fail the subsequent pre-merge validation. This also isn't a one-time thing; PRs like this have been submitted at multiple times in the past in order to mop up from such analyzers that are being run late. And to your question about not deleting code as dead/stale, this exact mechanism would address that. So I don't understand the concerns or pushbavk. It seems like a no brainer to me. What am I not comprehending?
And what percentage of the changes in this PR are to such files? I'm not saying it would prevent all possible problems. You're saying it's useless if it's not 100% false negatives? I highly doubt the existing tooling even gets close to that.
Uh oh!
There was an error while loading. Please reload this page.
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.
Just info about the tool and public PRs, it cannot be run out in the open: dotnet/arcade#4663
But there's an open issue https://github.com/dotnet/core-eng/issues/5747 about running it on public PRs.
Though I'm not what it will be good for without the results 🤣
Uh oh!
There was an error while loading. Please reload this page.
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.
Thanks. I don't buy the answer in that issue, though. We run analyzers that validate, for example, SHA1 isn't being used, and that's an "SDL static verification tool".
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.
@danmosemsft, are you ok to merge this PR? I think we currently don't have a better option how to suppress the bugs reported by the tool.
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.
The changes look OK to me, modulo the large conceptual problem of how there's a gap between checkins and analysis. I'm withholding a checkmark to let @danmosemsft decide if we want to block this on the conceptual problem or not.