Skip to content

Commit

Permalink
Use RefUpdateRule in BranchProtection check (ossf#936)
Browse files Browse the repository at this point in the history 
Co-authored-by: Azeem Shaikh <azeems@google.com>
  • Loading branch information
@azeemshaikh38 @azeemsgoogle
azeemshaikh38 and azeemsgoogle committed Aug 30, 2021
1 parent d9f5209 commit 9a1978a
Show file tree
Hide file tree
Showing 15 changed files with 1,428 additions and 571 deletions.
4 changes: 3 additions & 1 deletion Makefile
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,13 @@ build: ## Build all binaries and images in the reepo.
build: $(build-targets)

build-proto: ## Compiles and generates all required protobufs
build-proto: cron/data/request.pb.go cron/data/metadata.pb.go
build-proto: cron/data/request.pb.go cron/data/metadata.pb.go clients/branch.pb.go
cron/data/request.pb.go: cron/data/request.proto | $(PROTOC)
protoc --go_out=../../../ cron/data/request.proto
cron/data/metadata.pb.go: cron/data/metadata.proto | $(PROTOC)
protoc --go_out=../../../ cron/data/metadata.proto
clients/branch.pb.go: clients/branch.proto | $(PROTOC)
protoc --go_out=../../../ clients/branch.proto

generate-docs: ## Generates docs
generate-docs: docs/checks.md
Expand Down
64 changes: 18 additions & 46 deletions checks/branch_protection.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import (
"context"
"errors"
"fmt"
"net/http"
"regexp"

"github.com/google/go-github/v38/github"
Expand All @@ -41,17 +40,13 @@ func init() {

// TODO: Use RepoClient interface instead of this.
type repositories interface {
ListBranches(ctx context.Context, owner string, repo string,
opts *github.BranchListOptions) ([]*github.Branch, *github.Response, error)
ListReleases(ctx context.Context, owner string, repo string, opts *github.ListOptions) (
[]*github.RepositoryRelease, *github.Response, error)
GetBranchProtection(context.Context, string, string, string) (
*github.Protection, *github.Response, error)
}

type branchMap map[string]*github.Branch
type branchMap map[string]*clients.BranchRef

func (b branchMap) getBranchByName(name string) (*github.Branch, error) {
func (b branchMap) getBranchByName(name string) (*clients.BranchRef, error) {
val, exists := b[name]
if exists {
return val, nil
Expand All @@ -69,7 +64,7 @@ func (b branchMap) getBranchByName(name string) (*github.Branch, error) {
return nil, fmt.Errorf("could not find branch name %s: %w", name, errInternalBranchNotFound)
}

func getBranchMapFrom(branches []*github.Branch) branchMap {
func getBranchMapFrom(branches []*clients.BranchRef) branchMap {
ret := make(branchMap)
for _, branch := range branches {
ret[branch.GetName()] = branch
Expand All @@ -87,7 +82,7 @@ func checkReleaseAndDevBranchProtection(ctx context.Context,
repoClient clients.RepoClient, r repositories, dl checker.DetailLogger,
ownerStr, repoStr string) checker.CheckResult {
// Get all branches. This will include information on whether they are protected.
branches, _, err := r.ListBranches(ctx, ownerStr, repoStr, &github.BranchListOptions{})
branches, err := repoClient.ListBranches()
if err != nil {
return checker.CreateRuntimeErrorResult(CheckBranchProtection, err)
}
Expand Down Expand Up @@ -130,7 +125,7 @@ func checkReleaseAndDevBranchProtection(ctx context.Context,
if err != nil {
return checker.CreateRuntimeErrorResult(CheckBranchProtection, err)
}
checkBranches[defaultBranch.Name] = true
checkBranches[defaultBranch.GetName()] = true

protected := true
unknown := false
Expand All @@ -143,26 +138,20 @@ func checkReleaseAndDevBranchProtection(ctx context.Context,
}
return checker.CreateRuntimeErrorResult(CheckBranchProtection, err)
}
// nolint
if !branch.GetProtected() {
protected = false
dl.Warn("branch protection not enabled for branch '%s'", b)
} else {
// The branch is protected. Check the protection.
score, err := getProtectionAndCheck(ctx, r, dl, ownerStr, repoStr, b)
if err != nil {
if errors.Is(err, errInternalBranchNotFound) {
// Without an admin token, you only get information on the protection boolean.
// Add a score of 1 (minimal branch protection) for this protected branch.
unknown = true
scores = append(scores, 1)
dl.Warn("no detailed settings available for branch protection '%s'", b)
continue
} else {
// Github timeout or other error
return checker.CreateRuntimeErrorResult(CheckBranchProtection, err)
}
if branch.GetBranchProtectionRule() == nil {
// Without an admin token, you only get information on the protection boolean.
// Add a score of 1 (minimal branch protection) for this protected branch.
unknown = true
scores = append(scores, 1)
dl.Warn("no detailed settings available for branch protection '%s'", b)
continue
}
score := isBranchProtected(branch.GetBranchProtectionRule(), b, dl)
scores = append(scores, score)
}
}
Expand Down Expand Up @@ -192,25 +181,8 @@ func checkReleaseAndDevBranchProtection(ctx context.Context,
"branch protection is not maximal on development and all release branches", score)
}

func getProtectionAndCheck(ctx context.Context, r repositories, dl checker.DetailLogger, ownerStr, repoStr,
branch string) (int, error) {
// We only call this if the branch is protected.
protection, resp, err := r.GetBranchProtection(ctx, ownerStr, repoStr, branch)
if err != nil {
// Check the type of error. A not found error indicates that permissions are denied.
if resp.StatusCode == http.StatusNotFound {
//nolint
return 1, sce.Create(errInternalBranchNotFound, errInternalBranchNotFound.Error())
}
//nolint
return checker.InconclusiveResultScore, sce.Create(sce.ErrScorecardInternal, err.Error())
}

return IsBranchProtected(protection, branch, dl), nil
}

// IsBranchProtected checks branch protection rules on a Git branch.
func IsBranchProtected(protection *github.Protection, branch string, dl checker.DetailLogger) int {
// isBranchProtected checks branch protection rules on a Git branch.
func isBranchProtected(protection *clients.BranchProtectionRule, branch string, dl checker.DetailLogger) int {
totalScore := 15
score := 0

Expand Down Expand Up @@ -259,7 +231,7 @@ func IsBranchProtected(protection *github.Protection, branch string, dl checker.

// Returns true if several PR status checks requirements are enabled. Otherwise returns false and logs why it failed.
// Maximum score returned is 2.
func requiresStatusChecks(protection *github.Protection, branch string, dl checker.DetailLogger) int {
func requiresStatusChecks(protection *clients.BranchProtectionRule, branch string, dl checker.DetailLogger) int {
score := 0

if protection.GetRequiredStatusChecks() == nil ||
Expand All @@ -283,7 +255,7 @@ func requiresStatusChecks(protection *github.Protection, branch string, dl check

// Returns true if several PR review requirements are enabled. Otherwise returns false and logs why it failed.
// Maximum score returned is 7.
func requiresThoroughReviews(protection *github.Protection, branch string, dl checker.DetailLogger) int {
func requiresThoroughReviews(protection *clients.BranchProtectionRule, branch string, dl checker.DetailLogger) int {
score := 0

if protection.GetRequiredPullRequestReviews() == nil {
Expand All @@ -296,7 +268,7 @@ func requiresThoroughReviews(protection *github.Protection, branch string, dl ch
protection.RequiredPullRequestReviews.RequiredApprovingReviewCount, branch)
score += 2
} else {
score += protection.RequiredPullRequestReviews.RequiredApprovingReviewCount
score += int(protection.RequiredPullRequestReviews.RequiredApprovingReviewCount)
dl.Warn("number of required reviewers is only %d on branch '%s'",
protection.RequiredPullRequestReviews.RequiredApprovingReviewCount, branch)
}
Expand Down
Loading

0 comments on commit 9a1978a

Please sign in to comment.