Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add explicit warning when an overlap is detected #2929

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rgmz
Copy link
Contributor

@rgmz rgmz commented Jun 6, 2024

Description:

Similar to #2922, the goal of this change is to provide clear and actionable information if a detection is impacted.

The problem is currently that "verification overlap" doesn't provide useful feedback. Even if you use --results=unknown to see the error, it doesn't tell you which detectors overlapped.

2024-06-06T08:27:05-04:00       info-0  trufflehog      WARNING: A result will not be verified because more than one detector matches. You can override this behavior by using the --allow-verification-overlap flag    {"verification_overlap_worker_id": "2hlPu", "detectors": ["AzureSearchAdminKey", "AzureDevopsPersonalAccessToken"]}

Checklist:

  • Tests passing (make test-community)?
  • Lint passing (make lint this requires golangci-lint)?

@CLAassistant
Copy link

CLAassistant commented Jun 6, 2024

CLA assistant check
All committers have signed the CLA.

@rgmz rgmz force-pushed the feat/overlap-notify branch 5 times, most recently from f334a37 to 503266e Compare June 6, 2024 12:42
@rgmz rgmz marked this pull request as ready for review June 6, 2024 12:42
@rgmz rgmz requested review from a team as code owners June 6, 2024 12:42
@rgmz rgmz force-pushed the feat/overlap-notify branch from 503266e to 7bc6358 Compare June 6, 2024 12:49
@@ -752,7 +779,20 @@ func (e *Engine) verificationOverlapWorker(ctx context.Context) {
continue
}

if likelyDuplicate(ctx, key, chunkSecrets) {
if ok, t := likelyDuplicate(ctx, key, chunkSecrets); ok {
Copy link
Contributor Author

@rgmz rgmz Jun 6, 2024

Choose a reason for hiding this comment

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

Personally, I don't understand the threat model for this feature. It silently skips verification if a result matches more than one detector, which happens frequently during normal operation and can result in valid secrets being skipped.

The notion of a "malicious" detector is confusing — is Truffle adding malicious detectors to TruffleHog? Either this check needs a whitelist of common overlaps (e.g., Azure-related, URL and FTP), or it should only trigger between built-in and custom detectors (IMO).

@@ -633,7 +633,7 @@ func TestLikelyDuplicate(t *testing.T) {
name: "empty strings",
val: chunkSecretKey{"", detectorA.Key},
dupes: map[chunkSecretKey]struct{}{{"", detectorB.Key}: {}},
expected: true,
expected: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should empty strings be considered duplicates? I guess it makes sense, but I'd initially tweaked the logic for likelyDuplicate to ignore anything smaller than 3 characters.

@rgmz rgmz force-pushed the feat/overlap-notify branch 3 times, most recently from adb2b14 to e9988fa Compare June 21, 2024 02:51
@rgmz rgmz force-pushed the feat/overlap-notify branch 2 times, most recently from ae3db9c to 582444e Compare July 1, 2024 18:37
@rgmz rgmz force-pushed the feat/overlap-notify branch from 582444e to c3e49e8 Compare September 13, 2024 11:45
@rgmz rgmz force-pushed the feat/overlap-notify branch from c3e49e8 to 7021992 Compare November 3, 2024 14:37
@rgmz rgmz requested a review from a team as a code owner November 3, 2024 14:37
@rgmz rgmz force-pushed the feat/overlap-notify branch 3 times, most recently from 5ca7755 to 9dd4a83 Compare November 11, 2024 19:20
@rgmz rgmz force-pushed the feat/overlap-notify branch from 9dd4a83 to 1841161 Compare December 2, 2024 13:29
@rgmz rgmz requested a review from a team as a code owner December 2, 2024 13:29
@rgmz rgmz force-pushed the feat/overlap-notify branch 2 times, most recently from 099544a to 030ca35 Compare December 21, 2024 16:00
@rgmz rgmz force-pushed the feat/overlap-notify branch from 030ca35 to 6946367 Compare December 31, 2024 14:57
@rgmz rgmz force-pushed the feat/overlap-notify branch 2 times, most recently from a43588c to 6fca032 Compare January 17, 2025 19:05
@rgmz rgmz force-pushed the feat/overlap-notify branch from 6fca032 to 8ac50dd Compare January 20, 2025 14:56
@rgmz rgmz force-pushed the feat/overlap-notify branch 4 times, most recently from 78b089f to 28d2d8d Compare February 2, 2025 23:03
@rgmz rgmz force-pushed the feat/overlap-notify branch from 28d2d8d to 93c9024 Compare February 13, 2025 02:01
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants