Introduce a new optional detector interface that will allow us to verify credentials not in the cache (#5298)

* Introduce a new optional detector interface that will allow us to verify credentials not in the cache

* Record verify time only for actual remote verification in verifyCacheMisses

A fully cached chunk previously recorded cache-lookup and hashing time as
remote verify time; now only VerifyResult wall time is accumulated, and
nothing is emitted when every result hits the cache, matching the
all-or-nothing path.

Co-authored-by: Cursor <[email protected]>

---------

Co-authored-by: Cursor <[email protected]>
This commit is contained in:
KevinPham-TruffleSec
2026-09-15 21:00:32 -07:00
committed by GitHub
co-authored by Cursor
parent 5a6944e878
commit 07e3ac7171
6 changed files with 450 additions and 13 deletions
+7
View File
@@ -375,3 +375,10 @@ func withDedupKey(ctx context.Context, detType detector_typepb.DetectorType, cre
func DoWithDedup(client *http.Client, detType detector_typepb.DetectorType, credential string, req *http.Request) (*http.Response, error) {
return client.Do(req.WithContext(withDedupKey(req.Context(), detType, credential)))
}
// ResultVerifier is an optional interface that a detector can implement to verify a single
// previously-extracted result independently of the chunk it came from, which lets the
// verification cache verify only cache misses instead of re-verifying an entire chunk.
type ResultVerifier interface {
VerifyResult(ctx context.Context, result *Result)
}
+34 -12
View File
@@ -19,10 +19,20 @@ type Scanner struct {
// Ensure the Scanner satisfies the interfaces at compile time.
var _ detectors.Detector = (*Scanner)(nil)
// This detector emits the cartesian product of every client ID and client secret in a
// chunk, so chunks routinely carry dozens of candidate pairs. Implementing ResultVerifier
// lets the verification cache verify only the pairs it has not seen before instead of
// re-verifying the whole product whenever one pair is novel.
var _ detectors.ResultVerifier = (*Scanner)(nil)
var (
// Oauth2 client ID and secret
oauth2ClientIDPat = regexp.MustCompile(detectors.PrefixRegex([]string{"github"}) + `\b([a-zA-Z0-9]{20})\b`)
oauth2ClientSecretPat = regexp.MustCompile(detectors.PrefixRegex([]string{"github"}) + `\b([a-f0-9]{40})\b`)
// tokenURL is a variable rather than a direct reference to github.Endpoint.TokenURL so
// that verification tests can redirect it at an httptest server.
tokenURL = github.Endpoint.TokenURL
)
const (
@@ -59,19 +69,10 @@ func (s Scanner) FromData(ctx context.Context, verify bool, data []byte) (result
"rotation_guide": "https://howtorotate.com/docs/tutorials/github/",
}
config := &clientcredentials.Config{
ClientID: idMatch[1],
ClientSecret: secretMatch[1],
TokenURL: github.Endpoint.TokenURL,
}
// Verification is delegated so that this path and the verification cache's
// per-result path share one implementation.
if verify {
_, err := config.Token(ctx)
// if client id and client secret is correct, it will return bad verification code error as we do not pass any verification code
// docs: https://docs.github.com/en/apps/oauth-apps/maintaining-oauth-apps/troubleshooting-oauth-app-access-token-request-errors#bad-verification-code
if err != nil && strings.Contains(err.Error(), githubBadVerificationCodeError) {
// mark result as verified only in case of bad verification code error, for any other error the result will be unverified
s1.Verified = true
}
s.VerifyResult(ctx, &s1)
}
results = append(results, s1)
@@ -81,6 +82,27 @@ func (s Scanner) FromData(ctx context.Context, verify bool, data []byte) (result
return
}
// VerifyResult verifies a single client ID / client secret pair.
// the verification cache calls it directly for results that missed the cache, which
// is what keeps a chunk of many pairs from re-verifying pairs whose status is already known.
func (s Scanner) VerifyResult(ctx context.Context, result *detectors.Result) {
clientID := result.SecretParts["id"]
clientSecret := result.SecretParts["secret"]
clientCredentials := &clientcredentials.Config{
ClientID: clientID,
ClientSecret: clientSecret,
TokenURL: tokenURL,
}
_, err := clientCredentials.Token(ctx)
// if client id and client secret is correct, it will return bad verification code error as we do not pass any verification code
// docs: https://docs.github.com/en/apps/oauth-apps/maintaining-oauth-apps/troubleshooting-oauth-app-access-token-request-errors#bad-verification-code
if err != nil && strings.Contains(err.Error(), githubBadVerificationCodeError) {
// mark result as verified only in case of bad verification code error, for any other error the result will be unverified
result.Verified = true
}
}
func (s Scanner) Type() detector_typepb.DetectorType {
return detector_typepb.DetectorType_GitHubOauth2
}
@@ -2,9 +2,13 @@ package github_oauth2
import (
"context"
"net/http"
"net/http/httptest"
"testing"
"github.com/google/go-cmp/cmp"
"github.com/stretchr/testify/assert"
"golang.org/x/oauth2"
"github.com/trufflesecurity/trufflehog/v3/pkg/detectors"
"github.com/trufflesecurity/trufflehog/v3/pkg/engine/ahocorasick"
@@ -74,3 +78,92 @@ func TestGithubOAuth2_Pattern(t *testing.T) {
})
}
}
// TestGithubOAuth2_VerifyResult covers the single-pair verification entry point that the
// verification cache calls for cache misses. Verification is pointed at a local server by
// reassigning tokenURL and by handing x/oauth2 a client through the context, since
// clientcredentials resolves its HTTP client from oauth2.HTTPClient.
func TestGithubOAuth2_VerifyResult(t *testing.T) {
const (
clientID = "9c14koUc3f04PrzlpCcU"
clientSecret = "caa3224b5ddb83924e6a487ee3f6543bae428309"
)
tests := []struct {
name string
status int
contentType string
body string
wantVerified bool
}{
{
// GitHub answers a valid ID/secret pair with bad_verification_code, because the
// client_credentials grant supplies no verification code. It reports this with
// HTTP 200 rather than a 4xx, which x/oauth2 still surfaces as a RetrieveError
// because the body carries an error field.
name: "live credential - bad_verification_code",
status: http.StatusOK,
contentType: "application/x-www-form-urlencoded",
body: "error=bad_verification_code&error_description=The+code+passed+is+incorrect+or+expired.",
wantVerified: true,
},
{
name: "dead credential - incorrect_client_credentials",
status: http.StatusUnauthorized,
contentType: "application/x-www-form-urlencoded",
body: "error=incorrect_client_credentials",
wantVerified: false,
},
{
// A transient server failure currently lands here as "not verified" rather than
// "unknown", because verification never calls SetVerificationError. That is a
// known false negative tracked on its own ticket; this case pins today's behavior
// so that changing it later is deliberate rather than accidental.
name: "server error - recorded unverified rather than unknown",
status: http.StatusInternalServerError,
contentType: "text/plain",
body: "boom",
wantVerified: false,
},
}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
var gotID, gotSecret string
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
// clientcredentials presents the pair either as basic auth or as form values,
// depending on which auth style x/oauth2 is probing, so accept both.
if id, secret, ok := r.BasicAuth(); ok {
gotID, gotSecret = id, secret
} else if err := r.ParseForm(); err == nil {
gotID, gotSecret = r.PostFormValue("client_id"), r.PostFormValue("client_secret")
}
w.Header().Set("Content-Type", test.contentType)
w.WriteHeader(test.status)
_, _ = w.Write([]byte(test.body))
}))
defer server.Close()
originalTokenURL := tokenURL
tokenURL = server.URL
t.Cleanup(func() { tokenURL = originalTokenURL })
// x/oauth2 resolves its HTTP client from the context, which is how verification is
// redirected at the test server without the detector knowing about it.
ctx := context.WithValue(context.Background(), oauth2.HTTPClient, server.Client())
result := detectors.Result{
Raw: []byte(clientID),
RawV2: []byte(clientID + clientSecret),
SecretParts: map[string]string{"id": clientID, "secret": clientSecret},
}
Scanner{}.VerifyResult(ctx, &result)
assert.Equal(t, test.wantVerified, result.Verified)
// Confirms the pair travelled from SecretParts into the token request, which is
// the contract the verification cache relies on when it verifies a lone result.
assert.Equal(t, clientID, gotID)
assert.Equal(t, clientSecret, gotSecret)
})
}
}
+3 -1
View File
@@ -10,7 +10,9 @@ type MetricsReporter interface {
// smaller than the cache hit count due to cache hit "wasting"; see AddResultCacheHitsWasted for more information.
AddCredentialVerificationsSaved(count int)
// AddFromDataVerifyTimeSpent records wall time spent in calls to detector.FromData with verify=true.
// AddFromDataVerifyTimeSpent records wall time spent verifying credentials remotely, either in a call to
// detector.FromData with verify=true or, for detectors that implement detectors.ResultVerifier, in the
// per-result verification of cache misses.
AddFromDataVerifyTimeSpent(wallTime time.Duration)
// AddResultCacheHits records result cache hits. Not all cache hits result in elided remote verification requests
@@ -47,6 +47,9 @@ func New(resultCache ResultCache, metrics MetricsReporter) *VerificationCache {
// returned result. If there is a cache hit for each result, these cached values are all returned. Otherwise, the
// detector's FromData method is called again, but with verify=true, and the results are stored in the cache and then
// returned.
//
// Detectors that implement detectors.ResultVerifier are handled by verifyCacheMisses instead, which verifies only the
// results that missed the cache rather than re-running the detector over the whole chunk.
func (v *VerificationCache) FromData(
ctx context.Context,
detector detectors.Detector,
@@ -76,6 +79,11 @@ func (v *VerificationCache) FromData(
return withoutRemoteVerification, nil
}
// avoiding re-running verification for every result in the chunk.
// only if a detector implements detectors.ResultVerifier
if resultVerifier, ok := detector.(detectors.ResultVerifier); ok {
return v.verifyCacheMisses(ctx, resultVerifier, withoutRemoteVerification)
}
isEverythingCached := true
var cacheHitsInCurrentChunk int
for i, r := range withoutRemoteVerification {
@@ -133,6 +141,56 @@ func (v *VerificationCache) FromData(
return withRemoteVerification, nil
}
// verifyCacheMisses serves the results whose verification status is already cached and
// remotely verifies only the misses.
func (v *VerificationCache) verifyCacheMisses(
ctx context.Context,
detector detectors.ResultVerifier,
results []detectors.Result,
) ([]detectors.Result, error) {
// Only remote verification counts toward verify time; a fully cached chunk records
// nothing, matching the all-or-nothing path's early return on full cache coverage.
var timeSpentVerifying time.Duration
defer func() {
if timeSpentVerifying > 0 {
v.metrics.AddFromDataVerifyTimeSpent(timeSpentVerifying)
}
}()
verifyResult := func(i int) {
verifyStart := time.Now()
detector.VerifyResult(ctx, &results[i])
timeSpentVerifying += time.Since(verifyStart)
}
for i := range results {
cacheKey, err := v.getResultCacheKey(results[i])
if err != nil {
ctx.Logger().Error(err, "error getting result cache key for verification caching",
"operation", "read")
// Fail open: a result we cannot key still deserves verification, matching the
// all-or-nothing path, where a key error falls through to FromData(verify=true).
verifyResult(i)
continue
}
if cacheHit, ok := v.resultCache.Get(string(cacheKey)); ok {
results[i].CopyVerificationInfo(&cacheHit)
results[i].VerificationFromCache = true
v.metrics.AddResultCacheHits(1)
v.metrics.AddCredentialVerificationsSaved(1)
continue
}
v.metrics.AddResultCacheMisses(1)
verifyResult(i)
copyForCaching := results[i]
// Do not persist raw secret values in a long-lived cache
copyForCaching.Raw = nil
copyForCaching.RawV2 = nil
v.resultCache.Set(string(cacheKey), copyForCaching)
}
return results, nil
}
func (v *VerificationCache) getResultCacheKey(result detectors.Result) ([]byte, error) {
v.hashMu.Lock()
defer v.hashMu.Unlock()
@@ -43,6 +43,41 @@ func (t *testDetector) Description() string { return "" }
var _ detectors.Detector = (*testDetector)(nil)
// testResultVerifier is a testDetector that can also verify results one at a time, so
// tests can exercise the verification cache's per-result path. It inherits
// fromDataCallCount, which is what proves that path never re-runs the detector to verify.
type testResultVerifier struct {
testDetector
verifyResultCallCount int
// verifyResultCalls records the Redacted value of each verified result in call order,
// so tests can assert exactly which results were verified rather than just how many.
verifyResultCalls []string
}
func (t *testResultVerifier) VerifyResult(_ context.Context, result *detectors.Result) {
t.verifyResultCallCount++
t.verifyResultCalls = append(t.verifyResultCalls, result.Redacted)
// Stand in for a remote verification by adopting the status the test declared for this
// credential. Results are matched on Redacted because that is how these tests identify
// them; FromData(verify=false) strips verification info, so it has to be restored here.
for i := range t.results {
if t.results[i].Redacted == result.Redacted {
result.CopyVerificationInfo(&t.results[i])
break
}
}
// As in testDetector.FromData, the metric timing resolution is 1 ms, so verification has
// to be artificially slow for the wall time it reports to be observable.
time.Sleep(2 * time.Millisecond)
}
var (
_ detectors.Detector = (*testResultVerifier)(nil)
_ detectors.ResultVerifier = (*testResultVerifier)(nil)
)
func getResultCacheKey(t *testing.T, cache *VerificationCache, result detectors.Result) string {
key, err := cache.getResultCacheKey(result)
require.NoError(t, err)
@@ -280,3 +315,223 @@ func TestVerificationCache_FromData_SameRawV2DifferentType_CacheMiss(t *testing.
}
assert.Len(t, cache.resultCache.Values(), 2)
}
// The tests below cover the per-result verification path taken by detectors that implement
// detectors.ResultVerifier.
func TestVerificationCache_FromData_ResultVerifier_PartialCacheHit(t *testing.T) {
detector := testResultVerifier{testDetector: testDetector{results: []detectors.Result{
{Redacted: "hello", Raw: []byte("hello"), RawV2: []byte("helloV2"), Verified: true},
{Redacted: "world", Raw: []byte("world"), RawV2: []byte("worldV2"), Verified: false},
}}}
detector.results[1].SetVerificationError(errors.New("test verification error"))
metrics := InMemoryMetrics{}
cache := New(simple.NewCache[detectors.Result](), &metrics)
cache.resultCache.Set(getResultCacheKey(t, cache, detector.results[0]),
detectors.Result{Redacted: "hello", Verified: true})
results, err := cache.FromData(
logContext.Background(),
&detector,
true,
false,
nil)
require.NoError(t, err)
// One extraction pass and no re-run: the whole point of the per-result path.
assert.Equal(t, 1, detector.fromDataCallCount)
assert.Equal(t, 1, detector.verifyResultCallCount)
assert.Equal(t, []string{"world"}, detector.verifyResultCalls)
wantResults := []detectors.Result{
{Redacted: "hello", Raw: []byte("hello"), RawV2: []byte("helloV2"), Verified: true,
VerificationFromCache: true},
{Redacted: "world", Raw: []byte("world"), RawV2: []byte("worldV2"), Verified: false},
}
wantResults[1].SetVerificationError(errors.New("test verification error"))
assert.ElementsMatch(t, wantResults, results)
wantCacheData := []detectors.Result{
{Redacted: "hello", Verified: true},
{Redacted: "world", Verified: false},
}
wantCacheData[1].SetVerificationError(errors.New("test verification error"))
assert.ElementsMatch(t, wantCacheData, cache.resultCache.Values())
assert.Less(t, int64(0), metrics.FromDataVerifyTimeSpentMS.Load())
assert.Equal(t, int32(1), metrics.CredentialVerificationsSaved.Load())
assert.Equal(t, int32(1), metrics.ResultCacheHits.Load())
assert.Equal(t, int32(1), metrics.ResultCacheMisses.Load())
// Nothing is ever discarded on this path, so no hit can be wasted.
assert.Equal(t, int32(0), metrics.ResultCacheHitsWasted.Load())
}
func TestVerificationCache_FromData_ResultVerifier_AllCacheHits(t *testing.T) {
detector := testResultVerifier{testDetector: testDetector{results: []detectors.Result{
{Redacted: "hello", Raw: []byte("hello"), RawV2: []byte("helloV2"), Verified: true},
{Redacted: "world", Raw: []byte("world"), RawV2: []byte("worldV2"), Verified: false},
}}}
metrics := InMemoryMetrics{}
cache := New(simple.NewCache[detectors.Result](), &metrics)
cacheData := []detectors.Result{
{Redacted: "hello", Verified: true},
{Redacted: "world", Verified: false},
}
cacheData[1].SetVerificationError(errors.New("test verification error"))
cache.resultCache.Set(getResultCacheKey(t, cache, detector.results[0]), cacheData[0])
cache.resultCache.Set(getResultCacheKey(t, cache, detector.results[1]), cacheData[1])
results, err := cache.FromData(
logContext.Background(),
&detector,
true,
false,
nil)
require.NoError(t, err)
assert.Equal(t, 1, detector.fromDataCallCount)
assert.Equal(t, 0, detector.verifyResultCallCount)
wantResults := []detectors.Result{
{Redacted: "hello", Raw: []byte("hello"), RawV2: []byte("helloV2"), Verified: true,
VerificationFromCache: true},
{Redacted: "world", Raw: []byte("world"), RawV2: []byte("worldV2"), Verified: false,
VerificationFromCache: true},
}
wantResults[1].SetVerificationError(errors.New("test verification error"))
assert.ElementsMatch(t, wantResults, results)
assert.ElementsMatch(t, cacheData, cache.resultCache.Values())
// A fully cached chunk makes no remote calls, so no verify time may be recorded,
// matching the all-or-nothing path's early return on full cache coverage.
assert.Equal(t, int64(0), metrics.FromDataVerifyTimeSpentMS.Load())
assert.Equal(t, int32(2), metrics.CredentialVerificationsSaved.Load())
assert.Equal(t, int32(2), metrics.ResultCacheHits.Load())
assert.Equal(t, int32(0), metrics.ResultCacheMisses.Load())
assert.Equal(t, int32(0), metrics.ResultCacheHitsWasted.Load())
}
func TestVerificationCache_FromData_ResultVerifier_NoCacheHits(t *testing.T) {
detector := testResultVerifier{testDetector: testDetector{results: []detectors.Result{
{Redacted: "hello", Raw: []byte("hello"), RawV2: []byte("helloV2"), Verified: true},
{Redacted: "world", Raw: []byte("world"), RawV2: []byte("worldV2"), Verified: false},
}}}
detector.results[1].SetVerificationError(errors.New("test verification error"))
metrics := InMemoryMetrics{}
cache := New(simple.NewCache[detectors.Result](), &metrics)
results, err := cache.FromData(
logContext.Background(),
&detector,
true,
false,
nil)
require.NoError(t, err)
assert.Equal(t, 1, detector.fromDataCallCount)
assert.Equal(t, 2, detector.verifyResultCallCount)
assert.Equal(t, []string{"hello", "world"}, detector.verifyResultCalls)
assert.ElementsMatch(t, detector.results, results)
// Raw and RawV2 must be absent from every cached entry: this cache outlives the scan of
// any single chunk, so raw credential material must not be retained in it.
cachedValues := cache.resultCache.Values()
assert.Len(t, cachedValues, 2)
for _, cached := range cachedValues {
assert.Nil(t, cached.Raw)
assert.Nil(t, cached.RawV2)
}
assert.Less(t, int64(0), metrics.FromDataVerifyTimeSpentMS.Load())
assert.Equal(t, int32(0), metrics.CredentialVerificationsSaved.Load())
assert.Equal(t, int32(0), metrics.ResultCacheHits.Load())
assert.Equal(t, int32(2), metrics.ResultCacheMisses.Load())
assert.Equal(t, int32(0), metrics.ResultCacheHitsWasted.Load())
}
// Identical pairs recur within a chunk, so the cache read must happen immediately before
// each verification rather than once up front. Getting this wrong would still be correct,
// just wasteful, which is exactly the class of bug this change exists to remove.
func TestVerificationCache_FromData_ResultVerifier_DuplicateResultsInChunk(t *testing.T) {
detector := testResultVerifier{testDetector: testDetector{results: []detectors.Result{
{Redacted: "hello", Raw: []byte("hello"), RawV2: []byte("helloV2"), Verified: true},
{Redacted: "hello", Raw: []byte("hello"), RawV2: []byte("helloV2"), Verified: true},
}}}
metrics := InMemoryMetrics{}
cache := New(simple.NewCache[detectors.Result](), &metrics)
results, err := cache.FromData(
logContext.Background(),
&detector,
true,
false,
nil)
require.NoError(t, err)
assert.Equal(t, 1, detector.verifyResultCallCount)
// The first occurrence is verified remotely and populates the cache; the second reads it
// back, which is why it reports VerificationFromCache despite the same scan verifying it.
assert.ElementsMatch(t, []detectors.Result{
{Redacted: "hello", Raw: []byte("hello"), RawV2: []byte("helloV2"), Verified: true},
{Redacted: "hello", Raw: []byte("hello"), RawV2: []byte("helloV2"), Verified: true,
VerificationFromCache: true},
}, results)
assert.Len(t, cache.resultCache.Values(), 1)
assert.Equal(t, int32(1), metrics.CredentialVerificationsSaved.Load())
assert.Equal(t, int32(1), metrics.ResultCacheHits.Load())
assert.Equal(t, int32(1), metrics.ResultCacheMisses.Load())
assert.Equal(t, int32(0), metrics.ResultCacheHitsWasted.Load())
}
// Implementing ResultVerifier must not make a detector verify when verification is off, so
// the dispatch has to sit after FromData's verify=false early return.
func TestVerificationCache_FromData_ResultVerifier_VerifyFalse(t *testing.T) {
detector := testResultVerifier{testDetector: testDetector{results: []detectors.Result{
{Redacted: "hello", Raw: []byte("hello"), RawV2: []byte("helloV2"), Verified: true},
}}}
metrics := InMemoryMetrics{}
cache := New(simple.NewCache[detectors.Result](), &metrics)
results, err := cache.FromData(
logContext.Background(),
&detector,
false,
false,
nil)
require.NoError(t, err)
assert.Equal(t, 1, detector.fromDataCallCount)
assert.Equal(t, 0, detector.verifyResultCallCount)
assert.ElementsMatch(t, []detectors.Result{
{Redacted: "hello", Raw: []byte("hello"), RawV2: []byte("helloV2"), Verified: false},
}, results)
assert.Empty(t, cache.resultCache.Values())
assert.Equal(t, int64(0), metrics.FromDataVerifyTimeSpentMS.Load())
assert.Equal(t, int32(0), metrics.ResultCacheHits.Load())
assert.Equal(t, int32(0), metrics.ResultCacheMisses.Load())
assert.Equal(t, int32(0), metrics.ResultCacheHitsWasted.Load())
}
// Targeted re-verification scans (chunk.SecretID != 0) deliberately bypass the cache to get
// a fresh answer, so they must keep going through FromData(verify=true) even for a detector
// that could verify per result.
func TestVerificationCache_FromData_ResultVerifier_ForceCacheUpdate(t *testing.T) {
detector := testResultVerifier{testDetector: testDetector{results: []detectors.Result{
{Redacted: "hello", Raw: []byte("hello"), RawV2: []byte("helloV2"), Verified: true},
}}}
metrics := InMemoryMetrics{}
cache := New(simple.NewCache[detectors.Result](), &metrics)
cache.resultCache.Set(getResultCacheKey(t, cache, detector.results[0]),
detectors.Result{Redacted: "hello", Verified: false})
results, err := cache.FromData(
logContext.Background(),
&detector,
true,
true,
nil)
require.NoError(t, err)
assert.Equal(t, 1, detector.fromDataCallCount)
assert.Equal(t, 0, detector.verifyResultCallCount)
assert.ElementsMatch(t, detector.results, results)
// The stale cached entry is replaced by the freshly verified status.
assert.ElementsMatch(t, []detectors.Result{{Redacted: "hello", Verified: true}},
cache.resultCache.Values())
assert.Less(t, int64(0), metrics.FromDataVerifyTimeSpentMS.Load())
assert.Equal(t, int32(0), metrics.ResultCacheHits.Load())
assert.Equal(t, int32(0), metrics.ResultCacheMisses.Load())
assert.Equal(t, int32(0), metrics.ResultCacheHitsWasted.Load())
}