Github Oauth2 verification #1584
Conversation
pkg/detectors/github/github.go
Outdated
| @@ -48,13 +56,49 @@ type userRes struct { | |||
| // Keywords are used for efficiently pre-filtering chunks. | |||
| // Use identifiers in the secret preferably, or the provider name. | |||
| func (s Scanner) Keywords() []string { | |||
| return []string{"ghp_", "gho_", "ghu_", "ghs_", "ghr_", "github_pat_"} | |||
| return []string{"ghp_", "gho_", "ghu_", "ghs_", "ghr_", "github_pat_", "github"} | |||
There was a problem hiding this comment.
github matches means a lot of matches. Something like github_client would cut that down, but could miss creds. I erred on the side of not missing anything, but looking for other opinions.
There was a problem hiding this comment.
I'm with you, I think erring on the side of not missing anything makes sense. And to clarify a little bit here, the github keyword would be associated with oauth creds right? Would it make sense to use detectors.PrefixRegex for the oauth regexes?
There was a problem hiding this comment.
Good call. Switched.
pkg/detectors/github/github.go
Outdated
| s1 := detectors.Result{ | ||
| DetectorType: detectorspb.DetectorType_Github, | ||
| Raw: []byte(idMatch[1]), | ||
| } |
There was a problem hiding this comment.
If my understanding is correct, we should also have a RawV2: []byte(idMatch[i] + secretMatch[1]).
I could see either way on this one. I went with adding it to the existing detector to avoid confusion on github cred types, but I could see being more specific being useful also. |
| var _ detectors.Versioner = (*Scanner)(nil) | ||
| var _ detectors.EndpointCustomizer = (*Scanner)(nil) | ||
|
|
||
| func (Scanner) Version() int { return 2 } |
There was a problem hiding this comment.
I don't think this needs to be versioned (yet) since it's the only GitHubOauth2 detector.
There was a problem hiding this comment.
Good call. Definitely not v2 at least
| Raw: []byte(idMatch[1]), | ||
| RawV2: []byte(idMatch[1] + secretMatch[1]), |
There was a problem hiding this comment.
You can make Raw: []byte(idMatch[1] + secretMatch[1]) and filling omit RawV2
The only reason why RawV2 exists is because we did not capture all of the components of some credentials the first time in the Raw field.
cc @mcastorina for your understanding as well
There was a problem hiding this comment.
Won't that cause confusion in OSS where the raw value is shown?
There was a problem hiding this comment.
Oh, you're right, I didn't consider that aspect. Keep it how it is then.
| var _ detectors.EndpointCustomizer = (*Scanner)(nil) | ||
|
|
||
| func (Scanner) DefaultEndpoint() string { return "https://api.github.com" } |
There was a problem hiding this comment.
Nit (non-blocking): It doesn't look like we're using this endpoint customizer either. I think that can be added in another PR though.
No description provided.