Support indeterminacy in alchemy and update detector docs #1510

rosecodym · 2023-07-18T20:51:10Z

This isn't an "important" detector - except that it's the template detector that the detector generator runs on! This PR also:

Updates the associated documentation
Changes the method that creates a common testing HTTP client with a timeout to accept a Duration directly instead of interpreting a int64 as a number of seconds
- Go will silently convert int64 values to Durations, so we need to be careful when making this change elsewhere

hack/docs/Adding_Detectors_external.md

samdatkins

Docs look great to me, it's really nice to have a concrete example

mcastorina

lgtm with one non-blocking suggestion

mcastorina · 2023-07-20T18:24:19Z

pkg/common/http.go

-func SaneHttpClientTimeOut(timeOutSeconds int64) *http.Client {
+func SaneHttpClientTimeOut(timeoutMs int64) *http.Client {
 	httpClient := &http.Client{}
-	httpClient.Timeout = time.Second * time.Duration(timeOutSeconds)
+	httpClient.Timeout = time.Millisecond * time.Duration(timeoutMs)


Since we're changing the units here already, could we change the input parameter to be a time.Duration so the user can choose? It also help readability:

client := SaneHttpClientTimeOut(10 * time.Second) // 10s client := SaneHttpClientTimeOut(10000) // ???

This will also make code that imports this function not compile* when it gets updated, which is better than silently making all timeouts 1000x faster.

*I think.. hopefully Go doesn't silently promote an int64 to a time.Duration

That's a good suggestion but it looks like Go will silently interpret int64 values as nanoseconds. As you pointed out, this means we should be careful when vendoring the change in.

trufflesteeeve

Looks good, just some small adjustments.

trufflesteeeve · 2023-07-20T19:00:22Z

pkg/detectors/alchemy/alchemy.go

@@ -47,6 +48,10 @@ func (s Scanner) FromData(ctx context.Context, verify bool, data []byte) (result
 		}

 		if verify {
+			client := s.client


optional: You could maybe just check s.client == nil and use it on line 59 below, rather than creating a client every time.

do you mean mutating the scanner struct?

Yep. Or set it as a default somewhere? As it is, every time this attempts to verify it will create a new client. Which is fine, but probably not necessary.

hack/docs/Adding_Detectors_external.md

trufflesteeeve · 2023-07-20T19:24:13Z

hack/docs/Adding_Detectors_external.md

+In Trufflehog parlance, the first type of verification response is called _determinate_ and the second type is called _indeterminate_. Verification code should distinguish between the two by returning an error object in the result struct **only** for indeterminate failures. In general, a verifier should return an error (indicating an indeterminate failure) in all cases that haven't been explicitly identified as determinate failure states. For example, if a verifier makes an HTTP request to a hypothetical authentication endpoint, the response codes could determine what to return:
+* Any `2xx` response would indicate that verification succeeded.
+* A `403` response would indicate that verification failed **determinately** and no error object should be returned.
+* Any other response would indicate that verification failed **indeterminately** and an error object should be returned.


I'm not sure we can say this is true for all apis we test against. e.g. Docker can still be a valid credential and return a 401 -

trufflehog/pkg/detectors/dockerhub/dockerhub.go

Lines 79 to 82 in 8ec5e49

// Valid credentials can still return a 401 status code if 2FA is enabled

if (res.StatusCode >= 200 && res.StatusCode < 300) || (res.StatusCode == 401 && strings.Contains(string(body), "login_2fa_token")) {

s1.Verified = true

}

So maybe adjust the language to be less certain about the indeterminate verification failure in the case of non 2XX or 403 responses.

I was trying to pick language that made it clear that was a hypothetical case, but I guess I didn't do a good job :(

Maybe a separate paragraph for the start of the hypothetical case? And potentially changing the would to would likely?

rosecodym requested a review from a team July 19, 2023 14:24

rosecodym requested a review from a team as a code owner July 19, 2023 14:57

ahrav approved these changes Jul 19, 2023

View reviewed changes

hack/docs/Adding_Detectors_external.md Outdated Show resolved Hide resolved

rosecodym changed the title ~~Migrate alchemy detector to tri-state verification~~ Support indeterminacy in alchemy and update detector docs Jul 19, 2023

rosecodym requested review from a team July 19, 2023 17:56

samdatkins approved these changes Jul 19, 2023

View reviewed changes

mcastorina approved these changes Jul 20, 2023

View reviewed changes

trufflesteeeve approved these changes Jul 20, 2023

View reviewed changes

rosecodym force-pushed the alchemy-indeterminacy branch from 1c719c7 to 96ab00c Compare July 21, 2023 14:53

rosecodym added 18 commits July 21, 2023 14:11

add new test

6ec7852

capture http error

492a180

handle other status codes

384e202

rename bad method

c18faa5

add explicit test output flags

f771f8d

clarify test name

b89f762

add comment on timeout

180a9bf

update docs

039bf44

fix typo

1372722

make names match

df096b5

inject http client

7189ce3

move to common code

347a6ed

use injected client for timeout

5d89ffc

accept duration directly

e0c3886

update example copy

1636d95

tweak language more

ea21dd5

update internal docs

5cf522e

reuse default client

af642d6

rosecodym force-pushed the alchemy-indeterminacy branch from 96ab00c to af642d6 Compare July 21, 2023 18:11

rosecodym merged commit ebf1038 into main Jul 21, 2023
10 checks passed

rosecodym deleted the alchemy-indeterminacy branch July 21, 2023 18:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support indeterminacy in alchemy and update detector docs #1510

Support indeterminacy in alchemy and update detector docs #1510

rosecodym commented Jul 18, 2023 •

edited

Loading

samdatkins left a comment

mcastorina left a comment

mcastorina Jul 20, 2023

rosecodym Jul 20, 2023

trufflesteeeve left a comment

trufflesteeeve Jul 20, 2023

rosecodym Jul 20, 2023

trufflesteeeve Jul 21, 2023

trufflesteeeve Jul 20, 2023

rosecodym Jul 20, 2023

trufflesteeeve Jul 20, 2023

	// Valid credentials can still return a 401 status code if 2FA is enabled
	if (res.StatusCode >= 200 && res.StatusCode < 300) \|\| (res.StatusCode == 401 && strings.Contains(string(body), "login_2fa_token")) {
	s1.Verified = true
	}

Support indeterminacy in alchemy and update detector docs #1510

Support indeterminacy in alchemy and update detector docs #1510

Conversation

rosecodym commented Jul 18, 2023 • edited Loading

samdatkins left a comment

Choose a reason for hiding this comment

mcastorina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trufflesteeeve left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosecodym commented Jul 18, 2023 •

edited

Loading