Add --results flag

[rgmz]

Description:

This is a follow-up to #2107 and #2335.

The idea — inspired by the aforementioned PRs and this Slack thread — was initially to create a flag that would show verification errors even if --only-verified is specified. This is necessary because the engine currently filters unverified results, meaning you will not see verification errors if you're using --only-verified. This is problematic because #2356 changes how the engine works, but the warning will not be visible if you're using --only-verified.

Edit: as discussed below, I ended up creating a generic --results flag. This allows you to explicitly enable or disable notifications for verified, unknown (i.e., error), and unverified results.

Before

$ trufflehog github --org="trufflesecurity" --only-verified
...
# No errors are printed, the results are skipped without notice

After

$ trufflehog github --org="trufflesecurity" --results=verified,unknown
...
# Verification skipped due to multiple matches
⚠️ Found result - unable to verify due to error 🐷🔑❗️
Verification Error: More than one detector has found this result. For your safety, verification has been disabled. You can override this behavior by using the --allow-verification-overlap flag.
Detector Type: Instabot
Decoder Type: PLAIN
Raw result: trufflesecurity/trufflehog/v3/pkg/detectors/
Commit: 4f713801df8c32062bd0815a81111d64322f71b0
Email: Dustin Decker <dustin@trufflesec.com>
File: trufflehog
Link: https://github.com/trufflesecurity/trufflehog/blob/4f713801df8c32062bd0815a81111d64322f71b0/trufflehog
Repository: https://github.com/trufflesecurity/trufflehog.git
Timestamp: 2023-10-04 16:24:42 +0000

# Verification failed due to i/o issue
⚠️ Found result - unable to verify due to error 🐷🔑❗️
Verification Error: lookup sts.amazonaws.com: i/o timeout
Detector Type: AWS
Decoder Type: PLAIN
Raw result: AKIA2OGYBAH6YWGZ2GOB
Resource_type: Access key
Account: 717712589309
Commit: 6cd53bcf39f72fb8801c0a0b9e1ae1bb44513b41
Email: Mike Vanbuskirk <mike.vanbuskirk@trufflesec.com>
File: s3.py
Line: 10
Link: https://github.com/trufflesecurity/trufflehog-github-actions-example/blob/6cd53bcf39f72fb8801c0a0b9e1ae1bb44513b41/s3.py#L10
Repository: https://github.com/trufflesecurity/trufflehog-github-actions-example.git
Timestamp: 2023-06-21 21:11:31 +0000

# Etc
...

Checklist:

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

[rosecodym]

Thanks for highlighting this. From a design perspective, I think the root issue is that --only-verified is actually obsolete now, because the new verification "errors" are effectively a third verification state of "unknown". (--only-verified assumes a world where a secret is either verified or not verified, but that's not the actual world anymore.) You've added a new flag that can be combined with --only-verified to show various subsets of results, but I'm thinking that this might be easier to understand and maintain if we make our flags match the logical model more directly with something like --log-verified=. E.g. --log-verified=verified shows only verified secrets, and --log-verified=verified,unknown shows both verified secrets and secrets with errors. --only-verified could be aliased to some version of the new flag.

I'm not happy with those specific names, but what do you think of the general approach? Have I missed a use case?

[rgmz]

I agree that it's more of an design issue. I like your solution more overall, assuming we can find a compelling flag to "replace" --only-verified.

From a user perspective, --only-verified is a simple and easy to understand flag; I'm not sure that --log-verified has the same intuitive appeal. A few other ideas come to mind:

• --results=verified,unknown
• --filter=verified,unverified,unknown
• --show=verified,unknown

[rgmz]

@rosecodym I've pushed a different approach (--results=verified ...; using --only-verified is equivalent to just --results=verified).

Annoyingly, Kingpin doesn't allow you to provide comma-separated values (--results=verified,unknown), you have to repeat the flag (--results=verified --results=unknown).

[zricethezav]

Annoyingly, Kingpin doesn't allow you to provide comma-separated values (--results=verified,unknown), you have to repeat the flag (--results=verified --results=unknown).

That is annoying. I suppose we could do some funky parsing ourselves if we wanted to support comma-separated values.
I'm not a fan of --results=verified --results=unknown, etc.

[zricethezav]

@rgmz @rosecodym what about just adding --no-verification-warnings which would effectively be --only-verified (and the proposed --results=verified)

[rosecodym]

@rgmz @rosecodym what about just adding --no-verification-warnings which would effectively be --only-verified (and the proposed --results=verified)

@zricethezav do you mean modifying --only-verified to include verification warnings and then adding --no-verification-warnings on top of that? I can see the case, but people might find it confusing for --only-verified to print a bunch of unverified (sort of) results.

You're right about the lack of comma support being annoying though :(

[rgmz]

@zricethezav do you mean modifying --only-verified to include verification warnings and then adding --no-verification-warnings on top of that? I can see the case, but people might find it confusing for --only-verified to print a bunch of unverified (sort of) results.

Since "Polite verification"¹ can result in valid results being missed (#2515), having verification warnings on by default is a must.

I think that recommending something like --results=verified,warnings is more intuitive than having --only-verified show technically unverified results. Still, perfect is the enemy of good enough.

You're right about the lack of comma support being annoying though :(

The worst part is that it likely isn't coming. Kingpin seems like a dead-end: "v3" was announced in 2015 and it doesn't seem like there's been meaningful development in recent years.

Footnotes

1. TBH I don't understand the use case for this. I assume I'm missing something. ↩

[zricethezav]

The worst part is that it likely isn't coming. Kingpin seems like a dead-end: "v3" was announced in 2015 and it doesn't seem like there's been meaningful development in recent years.

Yea this is something we might want to address in the future. However, it looks like it is still maintained and dependency are being kept up to date.

I think that recommending something like --results=verified,warnings is more intuitive than having --only-verified show technically unverified results. Still, perfect is the enemy of good enough.

A solution could be to do manual string parsing. Easy enough.

func WithResults(results string) Option {
	return func(e *Engine) {
		splits := slices.Split(results, ",")
		if len(splits) > 0 {
			if slices.Contains(splits, "verified") {
				e.notifyVerifiedResults = true
			}
			if slices.Contains(splits, "unknown") {
				e.notifyUnknownResults = true
			}
			if slices.Contains(splits, "unverified") {
				e.notifyUnverifiedResults = true
			}
		} else {
			e.notifyVerifiedResults = true
			e.notifyUnknownResults = true
			e.notifyUnverifiedResults = true
		}
	}
}

We will also want to keep the existing --only-verified flag so as to not break existing commands. We can hide that command from the cli help with cli.Flag("only-verified").Hidden().Bool()

@rosecodym @dustin-decker @ahrav does implementing this plan sound good to you?

@rgmz would you mind updating the PR description to reflect these changes?

[dustin-decker]

I like the --results=verified,unknown,unverified solution with the manual splitting that Zach outlined.
--only-verified becomes a hidden flag, possibly marked for removal in TruffleHog v4.

Kingpin is in 'contributions only' status, the author has moved on: https://github.com/alecthomas/kingpin?tab=readme-ov-file#contributions-only

[rgmz]

A solution could be to do manual string parsing. Easy enough.
...
@rgmz would you mind updating the PR description to reflect these changes?

Done.

I like the --results=verified,unknown,unverified solution with the manual splitting that Zach outlined.
--only-verified becomes a hidden flag, possibly marked for removal in TruffleHog v4.

This makes sense. It might be best to introduce --results as a hidden command to allow for tweaking/bugfixes before making it generally available and hiding --only-verified.

[dustin-decker]

It might be best to introduce --results as a hidden command to allow for tweaking/bugfixes before making it generally available and hiding --only-verified.

Great consideration!

[rosecodym]

This looks pretty good - thanks for your effort and patience! I have a couple of specific questions about the implementation.

[rosecodym]

I can imagine someone running with --results=unknown to check Trufflehog's network access.

[rosecodym]

Looks great, thanks for doing this!

[rosecodym]

this looks like an extra newline

[jingles]

@rosecodym if this is how this works now, should the doc be updated here? https://github.com/trufflesecurity/trufflehog?tab=readme-ov-file#1-scan-a-repo-for-only-verified-secrets

thanks