Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add trufflehog for secret detection #150

Open
wants to merge 8 commits into
base: devel
Choose a base branch
from

Conversation

hugo-syn
Copy link
Contributor

Hi,

I've discovered your tool via secator it's very nice thanks for your work !

In this PR I'm adding 2 things, the first big improvement is the use of trufflehog for secret detection. Trufflehog can detect a looot of credentials, the complete list can be found here. This change make the tool slower for secret detection but improve the secret detection.

I've also add a command line option to ignore some url for secret scaning. For example scaning png files for secrets can be avoided:

$ cariddi  -s -secret-extension-filter pdf,svg,png,jpeg
...

@auto-assign auto-assign bot requested a review from edoardottt April 19, 2024 12:48
@edoardottt edoardottt changed the base branch from main to devel April 19, 2024 13:03
@edoardottt edoardottt self-assigned this Apr 19, 2024
@edoardottt
Copy link
Owner

Hi @hugo-syn , thank you so much for improving cariddi! Appreciated :)

  • I don't have a lot of time and since this is a big PR I'll take some time to review it.
  • For now try to resolve the conflicts with the devel branch for go.mod and go.sum files.

@hugo-syn
Copy link
Contributor Author

Hi, @edoardottt I've fixed the merge conflicts, I've updated some packages.

For the PR I understand, don't hesitate if you have questions or remarks regarding my code :)

@edoardottt
Copy link
Owner

If possible (there could be issues with deps) revert back go.mod to using go 1.21 and remove toolchain info (the linter action fails :,( ).

@hugo-syn
Copy link
Contributor Author

The trufflehog package requires go 1.22.0:

$ go build ./cmd/cariddi
go: github.com/trufflesecurity/trufflehog/[email protected] requires go >= 1.22.0 (running go 1.21.0)

I can bump the golang version in the different CI files if you want

@hugo-syn
Copy link
Contributor Author

And I've checked it looks good with go 1.22.0

@edoardottt
Copy link
Owner

edoardottt commented Apr 22, 2024

Hi @hugo-syn,

And I've checked it looks good with go 1.22.0

the go build action is failing:

go build -v ./...
  shell: /usr/bin/bash -e {0}
  env:
    GOROOT: /opt/hostedtoolcache/go/1.18.10/x64
go: errors parsing go.mod:
/home/runner/work/cariddi/cariddi/go.mod: invalid go version '1.22.0': must match format 1.23
/home/runner/work/cariddi/cariddi/go.mod: unknown directive: toolchain

https://github.com/edoardottt/cariddi/actions/runs/8754239537/job/24025715013?pr=150#step:6:8

I think this is caused by github actions, they must be set to the correct go version too (all of them: go, linting...).

@edoardottt edoardottt added enhancement New feature or request Go labels Apr 22, 2024
@edoardottt edoardottt linked an issue Apr 22, 2024 that may be closed by this pull request
@edoardottt edoardottt linked an issue Apr 22, 2024 that may be closed by this pull request
@hugo-syn
Copy link
Contributor Author

I've change the golang version in the workflows to 1.22.0 the build pipeline is ok now but there are errors with the golang-ci

@edoardottt
Copy link
Owner

edoardottt commented Apr 29, 2024

I've change the golang version in the workflows to 1.22.0 the build pipeline is ok now but there are errors with the golang-ci

I've seen. See if locally everything is okay with golangci-lint run. If you need help I'm here. In the next days I'm gonna review it. Since it's a huge PR I guess there will be a lil bit of work to be done... :/ Let's see.

This change make the tool slower

Can you quantify this? I'd like to comprehend how much this will be slower.

cc @hugo-syn . Let me know if you need assistance :)

@hugo-syn
Copy link
Contributor Author

Hi @edoardottt Unfortunately I didn't manage to fix the CI there are new errors with code that I did not modify :/

Can you quantify this? I'd like to comprehend how much this will be slower.

It's difficult to say but since it runs all the secret detection of trufflhog the secret search will take more time but it will find lots of new secrets

@edoardottt
Copy link
Owner

Unfortunately I didn't manage to fix the CI there are new errors with code that I did not modify :/

Ok, so don't worry about golangci-lint errors. I'll fix them. Go build is the important one and it's not failing.

It's difficult to say but since it runs all the secret detection of trufflhog the secret search will take more time but it will find lots of new secrets

Ok, I'll merge in devel and perform some tests.

For now it's okay so, wait for my PR review :)

@hugo-syn
Copy link
Contributor Author

Do not hesitate if you have any questions ! :)

Copy link
Owner

@edoardottt edoardottt left a comment

Choose a reason for hiding this comment

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

Thanks @hugo-syn !

  • Requesting some changes
  • Left some thoughts, let me know yours.

I'm here if you have any doubt or need assistance :)

Rua: flags.Rua,
Proxy: flags.Proxy,
SecretsFlag: flags.Secrets,
SecretExtensionFilter: strings.Split(flags.SecretExtensionFilter, ","),
Copy link
Owner

Choose a reason for hiding this comment

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

Better to perform this in another part, maybe in pkg/input/flags.go. Consider that it may be necessary to trim spaces too (TrimSpace() function).

@@ -69,3 +71,25 @@ func intensiveOk(target string, urlInput string, debug bool) bool {

return root == target
}

// Return the extension of an URL
func GetFileExtensionFromUrl(rawUrl string) (string, error) {
Copy link
Owner

Choose a reason for hiding this comment

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

We need some tests for this function since it's an important one. You can use the ones you find in this repo as example.
Try to insert as many test cases as possible (use double dots, query parameters, fragment, invalid URL...).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

where do you want to do tests ?

@@ -120,6 +122,8 @@ func ScanFlag() Input {
secretsPtr := flag.Bool("s", false, "Hunt for secrets.")
secretsFilePtr := flag.String("sf", "", "Use an external file (txt, one per line)"+
" to use custom regexes for secrets hunting.")
secretsExtensionFilterPtr := flag.String("secret-extension-filter", "", "Comma"+
Copy link
Owner

Choose a reason for hiding this comment

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

Can we have a shorter name for this? Like sef... do you like it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah sure !

@@ -163,6 +167,7 @@ func ScanFlag() Input {
*proxyPtr,
*secretsPtr,
*secretsFilePtr,
*secretsExtensionFilterPtr,
Copy link
Owner

Choose a reason for hiding this comment

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

Modify the README.md file according to the modifications made. In particular the new flag provided.


wgScanners.Wait()

// keep legacy code, the original list contains secrets that are not present in trufflehog like s3 buckets.
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not fully understanding this. I guess now the code performs both trufflehog and cariddi secrets detections right? Maybe we should find a solution for this.

A simple solution could be move the S3 regexes to info of the cariddi engine and run only the trufflehog engine for the secrets. But I'm open to suggestions if you have them.

Also, we have to take in mind that the user can input a file with custom regexes, I don't know if trufflehog can do this. If not, maybe we could leave the custom cariddi engine for the custom secrets.

Let me know your thoughts on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes I did not know so I kept both way.

I don't know if you can add custom regexes with trufflehog when used ad a library. I think you should keep both.

Copy link
Owner

@edoardottt edoardottt left a comment

Choose a reason for hiding this comment

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

PR review adjustments @hugo-syn


require (
github.com/fatih/color v1.17.0
github.com/gocolly/colly v1.2.0
github.com/trufflesecurity/trufflehog/v3 v3.73.0
Copy link
Owner

Choose a reason for hiding this comment

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

I'm leaving this comment here but it's not related to this line specifically.

The goreleaser CI is giving these errors:

Error: ../../../go/pkg/mod/github.com/trufflesecurity/trufflehog/[email protected]/pkg/gitparse/gitparse.go:200:18: cannot use defaultMaxDiffSize (untyped int constant 2147483648) as int value in struct literal (overflows)
Error: ../../../go/pkg/mod/github.com/trufflesecurity/trufflehog/[email protected]/pkg/gitparse/gitparse.go:201:18: cannot use defaultMaxCommitSize (untyped int constant 2147[48](https://github.com/edoardottt/cariddi/actions/runs/9128423415/job/25100742967?pr=150#step:4:49)3648) as int value in struct literal (overflows)

Do you have any clue about what they mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no clue on this I'm not used to this

@hugo-syn
Copy link
Contributor Author

hugo-syn commented Aug 5, 2024

Hi @edoardottt I'm not used to dev practices what should I do ? feel free to rewrite what I did to fit your way :)

@edoardottt
Copy link
Owner

Hi @edoardottt I'm not used to dev practices what should I do ? feel free to rewrite what I did to fit your way :)

Hi @hugo-syn . I don't have write access to your fork + this PR is huge + it seems it was created a while ago and since this would be a big improvement I'll proceed in this way:
I'll create a PR by myself to merge in devel and you can review that for me. Maybe this will speed up the process.

@hugo-syn
Copy link
Contributor Author

hugo-syn commented Aug 6, 2024

Oh ok I thought there would be a mechanism so that you could modify the code directly. Sounds good to me. Do not hesitate if you have any question

@ocervell
Copy link
Contributor

ocervell commented Nov 20, 2024

@hugo-syn great idea to add trufflehog !
@edoardottt for what it's worth, maintainers can make changes on PRs using the GitHub CLI by running gh pr checkout 150

@edoardottt
Copy link
Owner

Hi @ocervell !
The changes were applied in https://github.com/edoardottt/cariddi/tree/trufflehog.
However it produces a lot of false positives, it's far than acceptable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Go
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add -secret-extension-filter Add trufflehog for secret detection
3 participants