Detector-Competition-Feat - Dockerhub detector #1943
Conversation
ankushgoel27
changed the title
ahrav
added
the
Hacktoberfest-Detector-Competition-Fix
|
@ankushgoel27 thanks for submitting this PR. We're still considering it and will get a review on it soon. If merged, it'll be considered for the detector competition 👍🏻 |
|
@zricethezav i believe this is also ready for merge |
|
can i get a review here please |
There was a problem hiding this comment.
I left some inline comments, and you're going to have to update your base branch (sorry for the review delay :( ), but taking a step back, I have a question: Why did you make the existing detector v2 and add a "new" v1 rather than just adding the new detector as v2?
| emailPat = regexp.MustCompile(common.EmailPattern) | ||
|
|
||
| // Can use password or personal access token (PAT) for login, but this scanner will only check for PATs. | ||
| accessTokenPat = regexp.MustCompile(`\bdckr_pat_([a-zA-Z0-9_-]){27}\b`) | ||
| accessTokenPat = regexp.MustCompile(detectors.PrefixRegex([]string{"docker"}) + `\b([0-9Aa-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})\b`) |
There was a problem hiding this comment.
[0-9Aa-f] is a pretty unusual character group - why is A the only included capital letter? (If it was an accident, can you just reuse a pattern in common/patterns.go?)
| if len(resAccessTokenMatch) != 2 { | ||
| continue | ||
| } | ||
| pat := strings.TrimSpace(resAccessTokenMatch[1]) |
There was a problem hiding this comment.
Why is trimming necessary here? I don't see any way to capture whitespace in the access token pattern. Am I misreading it?
|
|
||
| s1 := detectors.Result{ | ||
| DetectorType: detectorspb.DetectorType_Dockerhub, | ||
| Raw: []byte(fmt.Sprintf("%s: %s", resUserMatch, resAccessTokenMatch)), | ||
| Raw: []byte(fmt.Sprintf("%s: %s", resUserMatch, pat)), |
There was a problem hiding this comment.
please also set RawV2 with the new value
| } else if res.StatusCode == 401 { | ||
| // The secret is determinately not verified (nothing to do) | ||
| } else { | ||
| s1.VerificationError = fmt.Errorf("unexpected HTTP response status %d", res.StatusCode) |
There was a problem hiding this comment.
This isn't actually possible any more - you need to use a function to set these errors now. You'll need to resolve this when you update your base branch.
|
|
||
| // Ensure the Scanner satisfies the interface at compile time. | ||
| // Ensure the Scanner satisfies the interfaces at compile time. |
There was a problem hiding this comment.
If you're going to add a version 2 of this detector you should also make version 1 implement Versioner and report its version as 1.
| // 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 | ||
| } |
There was a problem hiding this comment.
If we're touching this detector we should add indeterminacy support while we're here.
| @@ -0,0 +1,97 @@ | |||
| package dockerhub_v2 | |||
There was a problem hiding this comment.
It looks like the other versioned detectors don't use underscores in their package names. Can you do the same here for consistency? (i.e. dockerhubv2)
There was a problem hiding this comment.
it says to do so here - add _v2 to package name - https://github.com/trufflesecurity/trufflehog/blob/main/hack/docs/Adding_Detectors_external.md
There was a problem hiding this comment.
That is an excellent point. I'll make a note to get that cleaned up, thanks!
There was a problem hiding this comment.
Hot take: underscores make package names easier to read.
|
i created this new PR - #2361. This PR was stale and so many changes has happened since then in how the detectors work. |
|
closing as #2361 has been merged |
Opening this PR again as PR #1863 was closed due to inactivity.
Description:
This PR adds the older format of dockerhub token which is still valid and i have seen it in the wild. add the "id" to username regex.
Reference :
https://www.docker.com/blog/docker-hub-new-personal-access-tokens/