diff --git a/Makefile b/Makefile index 95cb77887..1d9a1d55f 100644 --- a/Makefile +++ b/Makefile @@ -21,16 +21,16 @@ check: go vet $(shell go list ./... | grep -v /vendor/) test-failing: - CGO_ENABLED=0 go test -timeout=30s $(shell go list ./... | grep -v /vendor/) | grep FAIL + CGO_ENABLED=0 go test -timeout=5m $(shell go list ./... | grep -v /vendor/) | grep FAIL test: - CGO_ENABLED=0 go test -timeout=30s $(shell go list ./... | grep -v /vendor/ | grep -v /pkg/detectors) + CGO_ENABLED=0 go test -timeout=5m $(shell go list ./... | grep -v /vendor/ | grep -v /pkg/detectors) test-race: - CGO_ENABLED=1 go test -timeout=30s -race $(shell go list ./... | grep -v /vendor/ | grep -v /pkg/detectors) + CGO_ENABLED=1 go test -timeout=5m -race $(shell go list ./... | grep -v /vendor/ | grep -v /pkg/detectors) test-detectors: - CGO_ENABLED=0 go test -timeout=30s $(shell go list ./... | grep /pkg/detectors) + CGO_ENABLED=0 go test -timeout=5m $(shell go list ./... | grep /pkg/detectors) bench: CGO_ENABLED=0 go test $(shell go list ./pkg/secrets/... | grep -v /vendor/) -benchmem -run=xxx -bench . diff --git a/pkg/common/utils.go b/pkg/common/utils.go new file mode 100644 index 000000000..e6a02e963 --- /dev/null +++ b/pkg/common/utils.go @@ -0,0 +1,19 @@ +package common + +func AddStringSliceItem(item string, slice *[]string) { + for _, i := range *slice { + if i == item { + return + } + } + *slice = append(*slice, item) +} + +func RemoveStringSliceItem(item string, slice *[]string) { + for i, listItem := range *slice { + if item == listItem { + (*slice)[i] = (*slice)[len(*slice)-1] + *slice = (*slice)[:len(*slice)-1] + } + } +} diff --git a/pkg/common/utils_test.go b/pkg/common/utils_test.go new file mode 100644 index 000000000..1185a330e --- /dev/null +++ b/pkg/common/utils_test.go @@ -0,0 +1,76 @@ +package common + +import ( + "reflect" + "testing" +) + +func TestAddItem(t *testing.T) { + type Case struct { + Slice []string + Modifier []string + Expected []string + } + tests := map[string]Case{ + "newItem": { + Slice: []string{"a", "b", "c"}, + Modifier: []string{"d"}, + Expected: []string{"a", "b", "c", "d"}, + }, + "newDuplicate": { + Slice: []string{"a", "b", "c"}, + Modifier: []string{"c"}, + Expected: []string{"a", "b", "c"}, + }, + } + + for name, test := range tests { + for _, item := range test.Modifier { + AddStringSliceItem(item, &test.Slice) + } + + if !reflect.DeepEqual(test.Slice, test.Expected) { + t.Errorf("%s: expected:%v, got:%v", name, test.Expected, test.Slice) + } + } +} + +func TestRemoveItem(t *testing.T) { + type Case struct { + Slice []string + Modifier []string + Expected []string + } + tests := map[string]Case{ + "existingItemEnd": { + Slice: []string{"a", "b", "c"}, + Modifier: []string{"c"}, + Expected: []string{"a", "b"}, + }, + "existingItemMiddle": { + Slice: []string{"a", "b", "c"}, + Modifier: []string{"b"}, + Expected: []string{"a", "c"}, + }, + "existingItemBeginning": { + Slice: []string{"a", "b", "c"}, + Modifier: []string{"a"}, + Expected: []string{"c", "b"}, + }, + "nonExistingItem": { + Slice: []string{"a", "b", "c"}, + Modifier: []string{"d"}, + Expected: []string{"a", "b", "c"}, + }, + } + + for name, test := range tests { + for _, item := range test.Modifier { + RemoveStringSliceItem(item, &test.Slice) + } + + if !reflect.DeepEqual(test.Slice, test.Expected) { + t.Errorf("%s: expected:%v, got:%v", name, test.Expected, test.Slice) + } + } +} diff --git a/pkg/sources/github/github.go b/pkg/sources/github/github.go index 61f9a032b..f9c6cf967 100644 --- a/pkg/sources/github/github.go +++ b/pkg/sources/github/github.go @@ -141,30 +141,18 @@ func (s *Source) Chunks(ctx context.Context, chunksChan chan *sources.Chunk) err s.repos = s.conn.Repositories s.orgs = s.conn.Organizations + var apiClient *github.Client switch cred := s.conn.GetCredential().(type) { case *sourcespb.GitHub_Unauthenticated: - apiClient := github.NewClient(s.httpClient) + apiClient = github.NewClient(s.httpClient) if len(s.orgs) > 30 { log.Warn("You may experience rate limiting when using the unauthenticated GitHub api. Consider using an authenticated scan instead.") } - if len(s.repos) > 0 { - for i, repo := range s.repos { - if !strings.HasSuffix(repo, ".git") { - if repo, err := giturl.NormalizeGithubRepo(repo); err != nil { - // This wasn't formatted as expected, let the user know why that might be. - log.WithError(err).Warnf("Repo not in expected format, attempting to paginate repos instead.") - } else { - s.repos[i] = repo - } - s.paginateRepos(ctx, apiClient, repo) - } - } - } - if len(s.orgs) > 0 { for _, org := range s.orgs { - s.paginateRepos(ctx, apiClient, org) + s.addReposByOrg(ctx, apiClient, org) + s.addReposByUser(ctx, apiClient, org) } } case *sourcespb.GitHub_Token: @@ -177,7 +165,6 @@ func (s *Source) Chunks(ctx context.Context, chunksChan chan *sources.Chunk) err ) tc := oauth2.NewClient(context.TODO(), ts) - var apiClient *github.Client var err error // If we're using public github, make a regular client. // Otherwise make an enterprise client @@ -196,24 +183,14 @@ func (s *Source) Chunks(ctx context.Context, chunksChan chan *sources.Chunk) err if len(s.repos) > 0 { specificScope = true - for i, repo := range s.repos { - if !strings.HasSuffix(repo, ".git") { - if repo, err := giturl.NormalizeGithubRepo(repo); err != nil { - // This wasn't formatted as expected, let the user know why that might be. - log.WithError(err).Warnf("Repo not in expected format, attempting to paginate repos instead.") - } else { - s.repos[i] = repo - } - s.paginateRepos(ctx, apiClient, repo) - } - } } if len(s.orgs) > 0 { specificScope = true for _, org := range s.orgs { if !strings.HasSuffix(org, ".git") { - s.paginateRepos(ctx, apiClient, org) + s.addReposByOrg(ctx, apiClient, org) + s.addReposByUser(ctx, apiClient, org) } } } @@ -222,14 +199,21 @@ func (s *Source) Chunks(ctx context.Context, chunksChan chan *sources.Chunk) err if err != nil { return errors.New(err) } - // TODO: this should enumerate an organizations gists too... - s.paginateGists(ctx, user.GetLogin(), chunksChan) if !specificScope { - s.paginateRepos(ctx, apiClient, user.GetLogin()) + s.addReposByUser(ctx, apiClient, user.GetLogin()) // Scan for orgs is default with a token. GitHub App enumerates the repositories // that were assigned to it in GitHub App settings. - s.paginateOrgs(ctx, apiClient, *user.Name) + s.addOrgsByUser(ctx, apiClient, user.GetLogin()) + for _, org := range s.orgs { + s.addReposByOrg(ctx, apiClient, org) + } + } + + s.addGistsByUser(ctx, apiClient, user.GetLogin()) + for _, org := range s.orgs { + // TODO: Test it actually works to list org gists like this. + s.addGistsByUser(ctx, apiClient, org) } case *sourcespb.GitHub_GithubApp: installationID, err := strconv.ParseInt(cred.GithubApp.InstallationId, 10, 64) @@ -252,7 +236,7 @@ func (s *Source) Chunks(ctx context.Context, chunksChan chan *sources.Chunk) err return errors.New(err) } itr.BaseURL = apiEndpoint - apiClient, err := github.NewEnterpriseClient(apiEndpoint, apiEndpoint, &http.Client{Transport: itr}) + apiClient, err = github.NewEnterpriseClient(apiEndpoint, apiEndpoint, &http.Client{Transport: itr}) if err != nil { return errors.New(err) } @@ -272,22 +256,21 @@ func (s *Source) Chunks(ctx context.Context, chunksChan chan *sources.Chunk) err return errors.New(err) } - err = s.paginateApp(ctx, apiClient) + err = s.addReposByApp(ctx, apiClient) if err != nil { return err } //check if we need to find user repos if s.conn.ScanUsers { - err := s.paginateMembers(ctx, installationClient, apiClient) + err := s.addMembersByApp(ctx, installationClient, apiClient) if err != nil { return err } log.Infof("Scanning repos from %v organization members.", len(s.members)) for _, member := range s.members { - //all org member's gists - s.paginateGists(ctx, member, chunksChan) - s.paginateRepos(ctx, apiClient, member) + s.addGistsByUser(ctx, apiClient, member) + s.addReposByUser(ctx, apiClient, member) } } @@ -295,6 +278,8 @@ func (s *Source) Chunks(ctx context.Context, chunksChan chan *sources.Chunk) err return errors.Errorf("Invalid configuration given for source. Name: %s, Type: %s", s.name, s.Type()) } + s.normalizeRepos(ctx, apiClient) + if _, ok := os.LookupEnv("DO_NOT_RANDOMIZE"); !ok { //Randomize channel scan order on each scan rand.Seed(time.Now().UnixNano()) @@ -341,7 +326,7 @@ func (s *Source) Chunks(ctx context.Context, chunksChan chan *sources.Chunk) err defer os.RemoveAll(path) if err != nil { - log.WithError(err).Errorf("unable to clone repo, continuing") + log.WithError(err).Errorf("unable to clone repo (%s), continuing", repoURL) return } err = s.git.ScanRepo(ctx, repo, git.NewScanOptions(), chunksChan) @@ -377,7 +362,7 @@ func handleRateLimit(err error) bool { return true } -func (s *Source) paginateReposByOrg(ctx context.Context, apiClient *github.Client, org string) { +func (s *Source) addReposByOrg(ctx context.Context, apiClient *github.Client, org string) { opts := &github.RepositoryListByOrgOptions{ ListOptions: github.ListOptions{ PerPage: 100, @@ -395,7 +380,7 @@ func (s *Source) paginateReposByOrg(ctx context.Context, apiClient *github.Clien break } for _, r := range someRepos { - s.repos = append(s.repos, r.GetCloneURL()) + common.AddStringSliceItem(r.GetCloneURL(), &s.repos) } if res.NextPage == 0 { break @@ -404,9 +389,8 @@ func (s *Source) paginateReposByOrg(ctx context.Context, apiClient *github.Clien } } -func (s *Source) paginateRepos(ctx context.Context, apiClient *github.Client, user string) { +func (s *Source) addReposByUser(ctx context.Context, apiClient *github.Client, user string) { opts := &github.RepositoryListOptions{ - // Visibility: "all", ListOptions: github.ListOptions{ PerPage: 50, }, @@ -423,7 +407,7 @@ func (s *Source) paginateRepos(ctx context.Context, apiClient *github.Client, us break } for _, r := range someRepos { - s.repos = append(s.repos, r.GetCloneURL()) + common.AddStringSliceItem(r.GetCloneURL(), &s.repos) } if res.NextPage == 0 { break @@ -432,34 +416,31 @@ func (s *Source) paginateRepos(ctx context.Context, apiClient *github.Client, us } } -func (s *Source) paginateGists(ctx context.Context, user string, chunksChan chan *sources.Chunk) { - apiClient := github.NewClient(s.httpClient) - gists, _, err := apiClient.Gists.List(ctx, user, &github.GistListOptions{}) - if err != nil { - log.WithError(err).Warnf("Could not get gists for user %s", user) - return - } - for _, gist := range gists { - path, repo, err := git.CloneRepoUsingUnauthenticated(*gist.GitPullURL) - defer os.RemoveAll(path) - if err != nil { - log.WithError(err).Warnf("Could not get gist %s from user %s", *gist.HTMLURL, user) +func (s *Source) addGistsByUser(ctx context.Context, apiClient *github.Client, user string) { + gistOpts := &github.GistListOptions{} + for { + gists, resp, err := apiClient.Gists.List(ctx, user, gistOpts) + if err == nil { + defer resp.Body.Close() + } + if handled := handleRateLimit(err); handled { continue } - s.log.WithField("repo", *gist.HTMLURL).Debugf("attempting to clone gist from user %s", user) - - scanCtx := context.Background() - err = s.git.ScanRepo(scanCtx, repo, git.NewScanOptions(), chunksChan) if err != nil { - log.WithError(err).Warnf("Could not scan after clone: %s", *gist.HTMLURL) - continue + log.WithError(err).Warnf("Could not get gists for user %s", user) } - + for _, gist := range gists { + common.AddStringSliceItem(gist.GetGitPullURL(), &s.repos) + } + if resp == nil || resp.NextPage == 0 { + break + } + gistOpts.Page = resp.NextPage } - + return } -func (s *Source) paginateMembers(ctx context.Context, installationClient *github.Client, apiClient *github.Client) error { +func (s *Source) addMembersByApp(ctx context.Context, installationClient *github.Client, apiClient *github.Client) error { opts := &github.ListOptions{ PerPage: 500, @@ -493,20 +474,19 @@ func (s *Source) paginateMembers(ctx context.Context, installationClient *github if usr == nil || *usr == "" { continue } - s.members = append(s.members, *usr) + common.AddStringSliceItem(*usr, &s.members) } if res.NextPage == 0 { break } opts.Page = res.NextPage } - } return nil } -func (s *Source) paginateApp(ctx context.Context, apiClient *github.Client) error { +func (s *Source) addReposByApp(ctx context.Context, apiClient *github.Client) error { // Authenticated enumeration of repos opts := &github.ListOptions{ PerPage: 100, @@ -523,7 +503,7 @@ func (s *Source) paginateApp(ctx context.Context, apiClient *github.Client) erro return errors.WrapPrefix(err, "unable to list repositories", 0) } for _, r := range someRepos.Repositories { - s.repos = append(s.repos, r.GetCloneURL()) + common.AddStringSliceItem(r.GetCloneURL(), &s.repos) } if res.NextPage == 0 { break @@ -533,23 +513,57 @@ func (s *Source) paginateApp(ctx context.Context, apiClient *github.Client) erro return nil } -func (s *Source) paginateOrgs(ctx context.Context, apiClient *github.Client, user string) { - orgOpts := &github.ListOptions{} - orgs, _, err := apiClient.Organizations.List(ctx, "", orgOpts) - if err != nil { - log.WithError(err).Errorf("Could not list organizations for %s", user) - return +func (s *Source) addOrgsByUser(ctx context.Context, apiClient *github.Client, user string) { + orgOpts := &github.ListOptions{ + PerPage: 100, } - for _, org := range orgs { - var name string - if org.Name != nil { - name = *org.Name - } else if org.Login != nil { - name = *org.Login - } else { + for { + orgs, resp, err := apiClient.Organizations.List(ctx, "", orgOpts) + if err == nil { + defer resp.Body.Close() + } + if handled := handleRateLimit(err); handled { continue } - s.paginateReposByOrg(ctx, apiClient, name) + if err != nil { + log.WithError(err).Errorf("Could not list organizations for %s", user) + return + } + for _, org := range orgs { + var name string + if org.Name != nil { + name = *org.Name + } else if org.Login != nil { + name = *org.Login + } else { + continue + } + common.AddStringSliceItem(name, &s.orgs) + } + if resp.NextPage == 0 { + break + } + orgOpts.Page = resp.NextPage + } +} + +func (s *Source) normalizeRepos(ctx context.Context, apiClient *github.Client) { + // TODO: Add check/fix for repos that are missing scheme + var newRepoList []string + for _, repo := range s.repos { + if parts := strings.Split(repo, "/"); len(parts) == 1 { + origSources := len(s.repos) + s.addGistsByUser(ctx, apiClient, repo) + s.addReposByUser(ctx, apiClient, repo) + if origSources != len(s.repos) { + common.RemoveStringSliceItem(repo, &s.repos) + continue + } + } + repoNormalized, err := giturl.NormalizeGithubRepo(repo) + if err != nil { + log.WithError(err).Warnf("Repo not in expected format: %s", repo) + } + newRepoList = append(newRepoList, repoNormalized) } - } diff --git a/pkg/sources/github/github_test.go b/pkg/sources/github/github_test.go index ddae1a27b..97660cde1 100644 --- a/pkg/sources/github/github_test.go +++ b/pkg/sources/github/github_test.go @@ -5,6 +5,7 @@ import ( "encoding/base64" "errors" "fmt" + "github.com/google/go-github/v41/github" "os" "testing" "time" @@ -378,6 +379,7 @@ func TestSource_paginateGists(t *testing.T) { wantChunk *sources.Chunk wantErr bool user string + minRepos int }{ { name: "get gist secret", @@ -404,8 +406,28 @@ func TestSource_paginateGists(t *testing.T) { }, Verify: false, }, - wantErr: false, - user: "dustin-decker", + wantErr: false, + user: "dustin-decker", + minRepos: 1, + }, + { + name: "get multiple pages of gists", + init: init{ + name: "test source", + connection: &sourcespb.GitHub{ + Credential: &sourcespb.GitHub_GithubApp{ + GithubApp: &credentialspb.GitHubApp{ + PrivateKey: githubPrivateKeyNew, + InstallationId: githubInstallationIDNew, + AppId: githubAppIDNew, + }, + }, + }, + }, + wantChunk: nil, + wantErr: false, + user: "andrew", + minRepos: 101, }, /* { name: "get multiple pages of gists", @@ -459,15 +481,37 @@ func TestSource_paginateGists(t *testing.T) { } chunksCh := make(chan *sources.Chunk, 5) go func() { - s.paginateGists(ctx, tt.user, chunksCh) + s.addGistsByUser(ctx, github.NewClient(s.httpClient), tt.user) + chunksCh <- &sources.Chunk{} }() - if err = handleChannel(chunksCh, basicCheckFunc(0, 0, tt.wantChunk, &s)); err != nil { + var wantedRepo string + if tt.wantChunk != nil { + wantedRepo = tt.wantChunk.SourceMetadata.GetGithub().Repository + } + if err = handleChannel(chunksCh, gistsCheckFunc(wantedRepo, tt.minRepos, &s)); err != nil { t.Error(err) } }) } } +func gistsCheckFunc(expected string, minRepos int, s *Source) chunkFunc { + return func(chunk *sources.Chunk) error { + if minRepos != 0 && minRepos > len(s.repos) { + return fmt.Errorf("didn't find enough repos. expected: %d, got :%d", minRepos, len(s.repos)) + } + if expected != "" { + for _, repo := range s.repos { + if repo == expected { + return nil + } + } + return fmt.Errorf("expected repo not included: %s", expected) + } + return nil + } +} + func basicCheckFunc(minOrg, minRepo int, wantChunk *sources.Chunk, s *Source) chunkFunc { return func(chunk *sources.Chunk) error { if minOrg != 0 && minOrg > len(s.orgs) {