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

[THOG-793] - Return all unverified results #856

Merged
merged 4 commits into from Oct 31, 2022

Conversation

ahrav
Copy link
Collaborator

@ahrav ahrav commented Oct 21, 2022

Screen Shot 2022-10-30 at 1 32 43 PM
Remove the check to filter and return only a single unverified result.

#844

@ahrav ahrav marked this pull request as ready for review October 21, 2022 19:19
@ahrav ahrav requested a review from a team as a code owner October 21, 2022 19:19
@trufflesteeeve
Copy link
Collaborator

This feels like a change that should be behind a CLI flag if we want to do it. Though CleanResults's comment explains what it does, it doesn't explain why we're doing it - which is essentially to cut down on the noise of detecting secrets that have either already been rotated, or aren't real.

@ahrav
Copy link
Collaborator Author

ahrav commented Oct 26, 2022

This feels like a change that should be behind a CLI flag if we want to do it. Though CleanResults's comment explains what it does, it doesn't explain why we're doing it - which is essentially to cut down on the noise of detecting secrets that have either already been rotated, or aren't real.

Yea I agree. So this was actually a common thread in the community slack where users were not getting all the unverified secrets. After talking to Dustin we mainly did this before we had the Dashboard to filter out verified/ non-verified. So with that change in place this was the suggested solution since this would technically be the approach where there we no longer filter out actual unverified results.

That being said I think adding a cmd line flag for this makes sense 👍

@dustin-decker dustin-decker mentioned this pull request Oct 26, 2022
Closed
@dustin-decker
Copy link
Contributor

It was put in place before we grouped credentials, but we do now, so maybe it is no longer needed? It definitely causes some unexpected behavior.

@trufflesteeeve
Copy link
Collaborator

That seems reasonable. We can always add functionality back in later if we need it. We might want to look into using more of the AWS style custom cleaner for any key/secret detector types, especially if their secrets also match a standard hash.

@ahrav
Copy link
Collaborator Author

ahrav commented Oct 28, 2022

That seems reasonable. We can always add functionality back in later if we need it. We might want to look into using more of the AWS style custom cleaner for any key/secret detector types, especially if their secrets also match a standard hash.

I have actually not seen this before. Yea I like this idea.

@ahrav ahrav requested a review from a team as a code owner October 30, 2022 20:34
@ahrav
Copy link
Collaborator Author

ahrav commented Oct 30, 2022

OKay, I think I actually found a pretty nice solution here. I move out the call to CleanResults into the engine vs within each detector. I also then included a CLI flag if we want to filter unverified results and pass that into the Engine during construction.

@ahrav ahrav merged commit fe029b1 into main Oct 31, 2022
@ahrav ahrav deleted the THOG-793-include-all-unverified-results branch October 31, 2022 16:36
Copy link
Collaborator

@trufflesteeeve trufflesteeeve left a comment

Choose a reason for hiding this comment

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

My only comments are about a couple of typos. Looks good!

@@ -180,6 +180,7 @@ Flags:
--concurrency=1 Number of concurrent workers.
--no-verification Don't verify the results.
--only-verified Only output verified results.
--filter-unverified Only output first unverified result per chunk per detector if there are more than one results.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick: Typo - "more than one results." -> "more than one result."

concurrency = cli.Flag("concurrency", "Number of concurrent workers.").Default(strconv.Itoa(runtime.NumCPU())).Int()
noVerification = cli.Flag("no-verification", "Don't verify the results.").Bool()
onlyVerified = cli.Flag("only-verified", "Only output verified results.").Bool()
filterUnverified = cli.Flag("filter-unverified", "Only output first unverified result per chunk per detector if there are more than one results.").Bool()
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick: Typo - "more than one results." -> "more than one result."

mac2000 pushed a commit to mac2000/trufflehog that referenced this pull request Nov 16, 2022
* Remove the check to filter and return only a single unverified result.

* Revert "Remove the check to filter and return only a single unverified result."

This reverts commit 494e432.

* Add new CLI flag to filter unverified results.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants