Skip to content
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

Example passwords triggering CredScan #1712

Closed
ericscheffler opened this issue Aug 19, 2021 · 12 comments · Fixed by #1716
Closed

Example passwords triggering CredScan #1712

ericscheffler opened this issue Aug 19, 2021 · 12 comments · Fixed by #1716
Labels
enhancement The issue is an enhancement request. good first issue The issue should be easier to fix and can be taken up by a beginner to learn to contribute on GitHub

Comments

@ericscheffler
Copy link
Contributor

As part of my workflow I commit a local copy of this repo to an internal Git repo that supports some on-prem and Azure deployments; this is necessary because the on-prem resources are not Internet-connected. When committing the local copy of this resource to Git, CredScan is detecting the sample passwords used in SqlServerDsc.Common.psm1 lines 1360-1361 as credentials and is blocking the sync. I'm normally able to work around this for code already in the repo by adding a suppressions file, and it's possible to suppress inline as well but this requires me modifying the code and can be finicky and error prone.


Redacted Git output log:

git push origin branchname:branchname
error: remote unpack failed: error VS403654: The push was rejected because it might contain credentials or other secrets.
To https://repo_url/_git/repo_name
! [remote rejected] branchname -> branchname (VS403654: The push was rejected because it might contain credentials or other secrets.

/Modules/SqlServerDsc/15.1.1/Modules/SqlServerDsc.Common/SqlServerDsc.Common.psm1(1360,49-72) : error : CSCAN0220 : hash BgnItBdlWIUVQv56/jWRnqZBJ3W91tQZ/wxi4+f2uTI=
error: failed to push some refs to 'https://repo_url/_git/repo_name'


Suggested modification (from CredScan documentation):
If CredScan is detecting realistic-looking, fake placeholder secrets in your test code (such as "Th!s15AFak3P4ssw0rd"), the best way to fix this is to change the fake secret to include the literal string "Placeholder" (case-insensitive). CredScan interprets these as placeholders and won't block them.

So instead of this:
new StorageCredentials("MyFakeAccountName", "MyFakePassword");

Do this instead:
new StorageCredentials("PlaceholderAccountName", "PlaceholderPassword");

@johlju
Copy link
Member

johlju commented Aug 25, 2021

There are no fake passwords, or any type of passwords, in the code. The lines 1360-1361 is just comment-based help. I suggest talking to the supplier to handle the false positive reporting of that hash.

@johlju johlju added the external The issue cannot be resolved within the DSC Resource Kit. label Aug 25, 2021
@ericscheffler
Copy link
Contributor Author

I agree, that is a false-positive finding. And yes this can be handled on a case-by-case solution by either suppressing the finding inline or via a suppressions file in the repo. My suggestion was simply to replace the existing example password with something that contains the string "placeholder"; that way nobody has to deal with the extra effort to suppress anything. It's an easy, non-impactful change that might prevent others from having to take extra steps, that's all.

@johlju
Copy link
Member

johlju commented Aug 25, 2021

But there are no example password 🤔 The text in the lines you referring to are no password, it is just normal text that cannot be changed, maybe rephrased if you suggest how to rephrase it.

@ericscheffler
Copy link
Contributor Author

Gahh... You are 100% right, my apologies; I don't know why the line numbers from my error messages don't agree with the line numbers from the actual code in the repo. I think the correct line numbers are 1389 and 1390 in https://github.com/dsccommunity/SqlServerDsc/blob/main/source/Modules/SqlServerDsc.Common/SqlServerDsc.Common.psm
Suggest maybe replacing 'Pa\ssw0rd1' with 'PlaceholderPassword', or something similar.

@johlju
Copy link
Member

johlju commented Aug 25, 2021

I understand, it see that in the comment-base examples as well. The tool does not understand PowerShell code so it cannot ignore comments.

So this:

Invoke-Query -SQLServer Server1 -SQLInstanceName MSSQLSERVER -Database MyDatabase `
-Query "select * from MyTable where password = 'Pa\ssw0rd1' and password = 'secret passphrase'" `
-WithResults -RedactText @('Pa\sSw0rd1','Secret PassPhrase') -Verbose

Can you verify that it can be changed to the below and the tool does not report false positive?

Invoke-Query -SQLServer Server1 -SQLInstanceName MSSQLSERVER -Database MyDatabase ` 
             -Query "select * from MyTable where password = 'PlaceholderPa\ssw0rd1' and password = 'placeholder secret passphrase'" ` 
             -WithResults -RedactText @('PlaceholderPa\sSw0rd1','Placeholder Secret PassPhrase') -Verbose 

Can you also verify that this is the only place it gives false positive at? We might have example code in other parts as well, but not sure.

@johlju johlju added enhancement The issue is an enhancement request. help wanted The issue is up for grabs for anyone in the community. good first issue The issue should be easier to fix and can be taken up by a beginner to learn to contribute on GitHub and removed external The issue cannot be resolved within the DSC Resource Kit. labels Aug 25, 2021
@ericscheffler
Copy link
Contributor Author

I can verify that this (SQLServerDSC) resource is the only place my organization is seeing this issue, here's a list of the other resources we have in our repo that are not triggering this finding (included so you can rule them out for consideration):
ActiveDirectoryCSDsc
ActiveDirectoryDsc
ADCSTemplate
ComputerManagementDsc
ConfigMgrCBDsc
ConfigurationManagerDsc
GroupPolicyDsc
MoveIISDsc
NetworkingDsc
StorageDsc
UpdateServicesDsc
xPSDesiredStateConfiguration
Based on the excerpt of the CredScan documentation (pasted in the original issue) your proposed solution would would work.

@johlju
Copy link
Member

johlju commented Aug 25, 2021

That is great other repos work, but my thought was if there are more lines in SqlServerDsc it will trigger on that we should fix too, or if those mentioned above is the only reported false positive?

@ericscheffler
Copy link
Contributor Author

Correct, that is the only file in this repo that was triggered, I just re-reviewed the findings from our internal team to be sure.

@johlju
Copy link
Member

johlju commented Aug 25, 2021

Is it possible for you to verify that my suggested change above works? By manually changing your local version and see if it passes the tool?

@ericscheffler
Copy link
Contributor Author

I can confirm that your suggested changes worked, and the modified block committed and synced to the repo without issues. For reference, here's the error output from the first attempt with the original code block:

! [remote rejected] Credscan_test -> Credscan_test (VS403654: The push was rejected because it might contain credentials or other secrets.
/CredScanTest.ps1(2,41-64) : error : CSCAN0220 : hash BgnItBdlWIUVQv56/jWRnqZBJ3W91tQZ/wxi4+f2uTI=

@johlju
Copy link
Member

johlju commented Aug 25, 2021

Great! Is it possible for you to send in a PR that changes to the suggested change?

@ericscheffler
Copy link
Contributor Author

Will do, I'll try to get that in today, thanks!

@johlju johlju removed the help wanted The issue is up for grabs for anyone in the community. label Aug 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement The issue is an enhancement request. good first issue The issue should be easier to fix and can be taken up by a beginner to learn to contribute on GitHub
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants