Avoid verifying the same key multiple times in a session

[bugbaba]

Hello Team,

Description

Verify a key only once even if it is found multiple times in the same or different files.

For example, in the below screenshot, we can see that the same gitlab key(revoked) is getting verified twice as it's mentioned twice in the file.

Preferred Solution

To avoid wasting resources on re-verification and hitting detectors with the same keys multiple times, it is ideal to check if the given key has already been verified.

Maybe a check before the if verify block? to confirm if the key has not been verified previously.

--
Best Regards,
@bugbaba

[rgmz]

This would benefit both trufflehog and any endpoints it calls.

I was actually playing around with adding caches to detectors so that known verifications only happen once. Work smarter, not harder. :)

[rgmz]

@ahrav has an experimental implementation of this in #2276.

[ahrav]

I agree, using a cache reduces duplicate external API calls for credential verification, improving performance and API stewardship. However, storing plaintext credentials presents a potential security risk if exposed, even though the chance of in-memory hash compromise is low. An option to mitigate exposure while retaining detection speed/efficiency could be hashing credentials before caching them with a high-speed algorithm like XXHash. This safeguards credentials while still allowing cache hits on matched hashes. Overall this balances security, performance, and responsible API usage - preventing duplicate verification calls for the same credentials.

[bugbaba]

Also, I think we shouldn't add ExtraData as metadata in the cache as it mostly contains sensitive info extracted from valid responses.

We just need to check if the given match exists in the cache or not. So maybe we can ignore all metadata if there are no plans to use metadata from cache.

if match in cachelist{
  ignore
}
verify block {
  verification logic
  add match to cachelist
}

[kashifkhan0771]

We want to retain duplicate secrets but avoid re-verifying them and instead, we should reuse the verification results from earlier scans. Also, this behavior should apply not just to a single chunk, but across all data.

Is that correct?

[bugbaba]

Yes, We want to first capture all the matches for a detector and deduplicate those so that each match is only checked once if its valid or not.

We can see the value kingfisher is getting by doing this.

[kashifkhan0771]

I'm assigning this to myself. This will be a significant change affecting all existing detectors. I'll start by designing the overall flow and applying the changes to a single detector first. Once that's reviewed and approved, we can roll it out to the others.

[kashifkhan0771]

I'm thinking we should store the minimum necessary information in the cache to avoid any issues with cache size over time. With that in mind, here's an example of what I mean:

// maybe we can keep this at a common place for all detectors to use 🤔 
type cacheData struct {
	verified        bool
	verificationErr error
}

// Initialize cache with expiration of 1 hour
credsCache = simple.NewCache(simple.WithExpirationInterval[cacheData](1 * time.Hour))

// Inside loop
data, exists := credsCache.Get(token)
if exists {
	// Copy data from cache
	s1.Verified = data.verified
	s1.SetVerificationError(data.verificationErr)

	// Add to results and skip re-verification
	results = append(results, s1)
	continue
}

// Inside the verification logic (if not found in cache)
credsCache.Set(token, cacheData{
	verified:        s1.Verified,
	verificationErr: s1.VerificationError(),
})

The idea is to cache only what's needed the verification result and any error. The first verified result will have the extraData, while duplicates ones sharing the same token can just reuse the verification result. We can add something in the extraData for those duplicates to indicate that their verification was cached from another result.

Big thanks to @ahrav for the original work in trufflehog#2276. I borrowed some of the ideas from there.
I am not sure if we want to reopen that PR and continue or create a new one. Just let me know.

[kashifkhan0771]

And yes we also need to hash keys before storing them in the cache 🔑

[ahrav]

If we need a larger struct with more data, (roughly ≥ 64–128 bytes) or needs concurrent in-place mutation (which we definitely don't want), we can store a *T: each entry then costs one word in the map and you avoid copying big structs on every read. For small, mostly-immutable structs, we should embed T directly, this removes a heap allocation, shrinks GC scan work, and sidesteps the extra memory-fetch (“pointer-chase”) penalty.

I like your proposal. 👍🏼