Issue with Git submodules

[ArisBee]

I've recently got the idea to consolidate Allstar configurations as git submodules. That way configurations can be centralized in several repositories and I can more easily switch between configurations for different collections of repositories.

However, I found that submodules make Allstar crash with the following error logs:

 allstar-6fcd6c87f4-bhctz allstar panic: runtime error: invalid memory address or nil pointer dereference                                                                                                                                                   │
│ allstar-6fcd6c87f4-bhctz allstar [signal SIGSEGV: segmentation violation code=0x1 addr=0x10 pc=0x821f18]                                                                                                                                                   │
│ allstar-6fcd6c87f4-bhctz allstar                                                                                                                                                                                                                           │
│ allstar-6fcd6c87f4-bhctz allstar goroutine 60 [running]:                                                                                                                                                                                                   │
│ allstar-6fcd6c87f4-bhctz allstar github.com/google/go-github/v50/github.(*RepositoryContent).GetContent(0xc000684c30?)                                                                                                                                     │
│ allstar-6fcd6c87f4-bhctz allstar     /home/user/go/pkg/mod/github.com/google/go-github/[email protected]/github/repos_contents.go:75 +0x18                                                                                                                     │
│ allstar-6fcd6c87f4-bhctz allstar github.com/ossf/allstar/pkg/config.fetchConfig({0x1a79650, 0xc000859a40}, {0x1a73320, 0xc00050cb90}, {0xc000293270, 0xd}, {0xc000293260, 0xc}, {0x16d12bc, 0xc}, ...)                                                     │
│ allstar-6fcd6c87f4-bhctz allstar     /home/user/allstar/pkg/config/config.go:206 +0x331                                                                                                                                                                  │
│ allstar-6fcd6c87f4-bhctz allstar github.com/ossf/allstar/pkg/config.getAppConfigs({0x1a79650, 0xc000859a40}, {0x1a73320, 0xc00050cb90}, {0xc000293270, 0xd}, {0xc000293260, 0xc})                                                                          │
│ allstar-6fcd6c87f4-bhctz allstar     /home/user/allstar/pkg/config/config.go:394 +0x3ed                                                                                                                                                                  │
│ allstar-6fcd6c87f4-bhctz allstar github.com/ossf/allstar/pkg/config.isBotEnabled({0x1a79650, 0xc000859a40}, {0x1a73320, 0xc00050cb90}, {0xc000293270, 0xd}, {0xc000293260, 0xc})                                                                           │
│ allstar-6fcd6c87f4-bhctz allstar     /home/user/allstar/pkg/config/config.go:343 +0x6b                                                                                                                                                                   │
│ allstar-6fcd6c87f4-bhctz allstar github.com/ossf/allstar/pkg/config.IsBotEnabled({0x1a79650?, 0xc000859a40?}, 0x16d0615?, {0xc000293270?, 0xc000701200?}, {0xc000293260?, 0xd?})                                                                           │
│ allstar-6fcd6c87f4-bhctz allstar     /home/user/allstar/pkg/config/config.go:339 +0x4a                                                                                                                                                                   │
│ allstar-6fcd6c87f4-bhctz allstar github.com/ossf/allstar/pkg/enforce.runPoliciesOnInstRepos({0x1a79650, 0xc000859a40}, {0xc0002985d0, 0x6, 0x3?}, 0x43a680?, {0x0, 0x0})                                                                                   │
│ allstar-6fcd6c87f4-bhctz allstar     /home/user/allstar/pkg/enforce/enforce.go:188 +0x112                                                                                                                                                                │
│ allstar-6fcd6c87f4-bhctz allstar github.com/ossf/allstar/pkg/enforce.EnforceAll.func1()                                                                                                                                                                    │
│ allstar-6fcd6c87f4-bhctz allstar     /home/user/allstar/pkg/enforce/enforce.go:149 +0x1b7                                                                                                                                                                │
│ allstar-6fcd6c87f4-bhctz allstar golang.org/x/sync/errgroup.(*Group).Go.func1()                                                                                                                                                                            │
│ allstar-6fcd6c87f4-bhctz allstar     /home/user/go/pkg/mod/golang.org/x/[email protected]/errgroup/errgroup.go:75 +0x64                                                                                                                                        │
│ allstar-6fcd6c87f4-bhctz allstar created by golang.org/x/sync/errgroup.(*Group).Go                                                                                                                                                                         │
│ allstar-6fcd6c87f4-bhctz allstar     /home/user/go/pkg/mod/golang.org/x/[email protected]/errgroup/errgroup.go:72 +0xa5  

I've built the container from commit 1ac94c2691ef170086b91ed9f62ec84006201cd1

Any idea how this runtime error could be fixed?

[jeffmendoza]

Looks like the code here: https://github.com/ossf/allstar/blob/main/pkg/config/contents.go#L26 is not equipped to handle submodules. It tries to get the contents of a file using:
https://github.com/google/go-github/blob/v50.2.0/github/repos_contents.go#L196
https://docs.github.com/en/rest/repos/contents?apiVersion=2022-11-28#get-repository-content

The GitHub docs say:

If the content is a submodule: The submodule_git_url identifies the location of the submodule repository, and the sha identifies a specific commit within the submodule repository. Git uses the given URL when cloning the submodule repository, and checks out the submodule at that specific commit.

It looks like the code there would need to follow the "submodule_git_url", but I don't see how the client library exposes that.

[gmlewis]

After a bit of investigation, it looks like the variable cf is nil which is causing the panic:
https://github.com/ossf/allstar/blob/main/pkg/config/config.go#L206

Digging a little deeper, it appears that there is a possibility that err and cf could both be nil when rcs is nil on this line:
https://github.com/ossf/allstar/blob/main/pkg/config/contents.go#L34

I suggest that you change line 200 here:
https://github.com/ossf/allstar/blob/main/pkg/config/config.go#L200
from this:

if err != nil {

to this:

if cf == nil || err != nil {

and that should fix the panic.

[gmlewis]

As for go-github supporting the submodule_git_url field, we welcome PRs. 😄

EDIT: Never mind. This field was added in: google/go-github#2880

[ArisBee]

The above fix still gives a SIGSEV error in the scorecard.convertLog function:

│ allstar-756f9ccdb9-kvlp4 allstar panic: runtime error: invalid memory address or nil pointer dereference                                                                                                                                                    │
│ allstar-756f9ccdb9-kvlp4 allstar [signal SIGSEGV: segmentation violation code=0x1 addr=0x30 pc=0x134fbdb]                                                                                                                                                   │
│ allstar-756f9ccdb9-kvlp4 allstar                                                                                                                                                                                                                            │
│ allstar-756f9ccdb9-kvlp4 allstar goroutine 27 [running]:                                                                                                                                                                                                    │
│ allstar-756f9ccdb9-kvlp4 allstar github.com/ossf/allstar/pkg/policies/scorecard.convertLogs({0xc00059ecc0?, 0x1, 0x2?})    

I was able to solve it by adding a condition that the finding isn't null in the corresponding function:

	for _, l := range logs {
		if (l.Msg.Finding == nil) {
		  continue
		}

The full change is available in the following commit: catawiki@9a0c4ba