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

chore: pre-commit secrets scanner #1170

Merged
merged 4 commits into from
May 8, 2020
Merged

Conversation

vuhuucuong
Copy link
Contributor

@vuhuucuong vuhuucuong commented May 7, 2020

Add pre-commit hook to scan for secrets before commit

FLOW:

  1. GET AWS secrets from environment variables and ~/.aws/credentials
  2. Merge those secrets into one file (temporary file)
  3. Register AWS provider for git-secrets using the file in step 2
  4. Scan for secrets in staged files using git secrets --scan
  5. Remove temporary file in step 2

@vuhuucuong vuhuucuong changed the title Choreprecommit secrets scanner chore: pre-commit secrets scanner May 7, 2020
Comment on lines 49 to 65
const { error: errorAddProvider, stdout: stdoutAddProvider, stderr: stderrAddProvider } = shell.exec(
`git secrets --add-provider -- git secrets --aws-provider ${TEMP_CRED_FILE}`,
{
stdio: 'inherit',
},
)
if (errorAddProvider) throw errorAddProvider
if (stderrAddProvider) throw stderrAddProvider
console.log('Added AWS provider\n', stdoutAddProvider)

// scan for secrets
const { error: errorScan, stdout: stdoutScan, stderr: stderrScan } = shell.exec('git secrets --scan --cached', {
stdio: 'inherit',
})
if (errorScan) throw errorScan
if (stderrScan) throw stderrScan
console.log('No secrets found in staged files!\n', stdoutScan)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to chain these two commands using &&, but it doesn't work as expected, need to separate into two commands

@vuhuucuong vuhuucuong force-pushed the chore/precommit-secrets-scanner branch from 017fd7f to eb65b84 Compare May 7, 2020 10:23
Copy link
Contributor

@willmcvay willmcvay left a comment

Choose a reason for hiding this comment

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

Nice work, thankyou 👍

removeTempCredFile()
} catch (err) {
console.error(err)
removeTempCredFile()
Copy link
Contributor

Choose a reason for hiding this comment

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

I thinks this part could result unhandled error if unlinksync is fail to execute and throws error

LGTM. Nice 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes if it throws an error, then just let it does, the script will auto exit with an error code.
If all things go well, then no error is returned.

Copy link
Contributor

@duong-se duong-se left a comment

Choose a reason for hiding this comment

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

For me i don't use aws credential file, only use bash profile, are there any way to check it ?

@vuhuucuong
Copy link
Contributor Author

For me i don't use aws credential file, only use bash profile, are there any way to check it ?

@tanphamhaiduong I've already checked both authentication mechanisms.

If you use bash profile, it means you're exporting AWS_ACCESS_KEY_ID & AWS_SECRET_ACCESS_KEY as environment variables.
I covered it in here https://github.com/reapit/foundations/pull/1170/files#diff-1d82deb06aec9c6a91d1e347cd934da5R11-R27

Just updated comment on that function to avoid confusing

Copy link
Contributor

@duong-se duong-se left a comment

Choose a reason for hiding this comment

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

LGTM

@vuhuucuong vuhuucuong merged commit 3ba6c55 into master May 8, 2020
@vuhuucuong vuhuucuong deleted the chore/precommit-secrets-scanner branch May 8, 2020 03:19
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.

4 participants