-
Notifications
You must be signed in to change notification settings - Fork 40
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
feat: add trivy secret scanning #440
feat: add trivy secret scanning #440
Conversation
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.
Couple discussion points. Looks mostly good
@@ -0,0 +1,158 @@ | |||
[ |
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.
Please also delete the expected_issues.json
file
- name: fs-secret | ||
files: [ALL] |
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.
Before we end up turning this on for everyone, couple verification questions:
- How does performance compare to something like trufflehog?
- How does secret scanning effectiveness compare to something like trufflehog?
- What's the relative noisiness for our monorepo?
If any of these answers are in the negative direction, we may want to consider disabling this subcommand by default.
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 think this should be disabled by default.
- It's slower than trufflehog.
- It's about the same, I think.
- Almost none - it has the added benefit of default ignoring our many fake secrets in test files.
Despite these, it has a few benefits over trufflehog - you can define your own regexes to search for and you can define regexes to not alert on (e.g. if you only use AWS keys, you can only search for those, making this tool faster). I think having trufflehog enabled for everybody, and then trivy secret scanning available but disabled by default for users looking to customize their secret scanning further is the best way to go.
linters/trivy/trivy.test.ts
Outdated
fs.readFileSync(path.resolve(__dirname, "expected_issues.json")).toString() | ||
); | ||
const callbackGenerator = | ||
(command: string, otherPreCheck?: (driver: TrunkLintDriver) => void): TestCallback => |
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.
nit: can otherPreCheck?
be a TestCallback
type?
const configExpectedFileIssues = JSON.parse( | ||
fs.readFileSync(path.resolve(__dirname, "config_expected_issues.json")).toString(), | ||
) as FileIssue[]; |
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.
nit: I would actually prefer this as a magic number rather than parsing here, just in case if we mess it up accidentally, we'll be asserting something like a 0 issue match, which will always be true
linters/trivy/trivy.test.ts
Outdated
fuzzyLinterCheckTest({ | ||
linterName: "trivy", | ||
testName: "config", | ||
args: "-a -y", |
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.
remove -y
. No autofixes
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
Adds trivy secret scanning - the reason this wasn't showing up before was because trivy by default doesn't scan for secrets in files with
/test
in the path. Also reworks the trivy tests to have separate tests for each subcommand.