From daec3b44ec0482bb292ede4ec21fd1269aeaadab Mon Sep 17 00:00:00 2001 From: naphtalid-legit <168970889+naphtalid-legit@users.noreply.github.com> Date: Thu, 13 Jun 2024 15:27:09 +0300 Subject: [PATCH] feat: Add policy for detecting GitHub Secret-Scanning (#311) * * Add policy for detecting GitHub Secret-Scanning * Add collection of 'Security and Analysis' settings for github repos * Fix phrasing of policy desc * add e2e testing * change naming to advanced_security * add public repo for secret scanning testing * more detailed errors about secret_scanning permissions --- e2e/github_repository.go | 5 ++ internal/analyzers/skippers/skippers.go | 8 ++++ internal/clients/github/client.go | 24 ++++++++-- internal/collected/github/repository.go | 1 + internal/collectors/collector.go | 1 + .../collectors/github/repository_collector.go | 48 +++++++++++++++++-- .../collectors/github/repository_context.go | 12 ++++- policies/github/repository.rego | 19 ++++++++ test/repository_test.go | 22 +++++++++ 9 files changed, 129 insertions(+), 11 deletions(-) diff --git a/e2e/github_repository.go b/e2e/github_repository.go index 5536a65c..c11bbd2e 100644 --- a/e2e/github_repository.go +++ b/e2e/github_repository.go @@ -76,4 +76,9 @@ var testCasesGitHubRepository = []testCase{ failedEntity: "bad_repo", passedEntity: "good_repo", }, + { + path: "data.repository.secret_scanning_not_enabled", + failedEntity: "bad_public_repo", + passedEntity: "good_public_repo", + }, } diff --git a/internal/analyzers/skippers/skippers.go b/internal/analyzers/skippers/skippers.go index 8234334c..e3d4fa34 100644 --- a/internal/analyzers/skippers/skippers.go +++ b/internal/analyzers/skippers/skippers.go @@ -39,6 +39,14 @@ func NewSkipper(ctx context.Context) Skipper { "enterprise": func(_ collectors.CollectedData) bool { return !context_utils.GetIsCloud(ctx) }, + "advanced_security": func(data collectors.CollectedData) bool { + repositoryContext, ok := data.Context.(collectors.CollectedDataRepositoryContext) + if !ok { + log.Printf("invalid type %T", data.Context) + return false + } + return repositoryContext.HasGithubAdvancedSecurity() + }, }, } } diff --git a/internal/clients/github/client.go b/internal/clients/github/client.go index 0724c81d..c9717474 100644 --- a/internal/clients/github/client.go +++ b/internal/clients/github/client.go @@ -4,6 +4,12 @@ import ( "bytes" "context" "fmt" + "log" + "net/http" + "regexp" + "strings" + "sync" + "github.com/Legit-Labs/legitify/internal/clients/github/pagination" "github.com/Legit-Labs/legitify/internal/clients/github/transport" "github.com/Legit-Labs/legitify/internal/clients/github/types" @@ -12,11 +18,6 @@ import ( "github.com/Legit-Labs/legitify/internal/common/slice_utils" commontypes "github.com/Legit-Labs/legitify/internal/common/types" "github.com/Legit-Labs/legitify/internal/screen" - "log" - "net/http" - "regexp" - "strings" - "sync" githubcollected "github.com/Legit-Labs/legitify/internal/collected/github" "github.com/Legit-Labs/legitify/internal/common/permissions" @@ -656,3 +657,16 @@ func (c *Client) GetOrganizationSecrets(org string) (*gh.Secrets, error) { } return secrets, nil } + +func (c *Client) GetSecurityAndAnalysisForRepository(repo, owner string) (*gh.SecurityAndAnalysis, error) { + r, res, err := c.Client().Repositories.Get(c.context, owner, repo) + + if err != nil { + return nil, err + } + if res.StatusCode != 200 { + return nil, fmt.Errorf("unexpected HTTP status: %d", res.StatusCode) + } + + return r.SecurityAndAnalysis, nil +} diff --git a/internal/collected/github/repository.go b/internal/collected/github/repository.go index 35bfe728..0e10e72f 100644 --- a/internal/collected/github/repository.go +++ b/internal/collected/github/repository.go @@ -72,6 +72,7 @@ type Repository struct { DependencyGraphManifests *GitHubQLDependencyGraphManifests `json:"dependency_graph_manifests"` RulesSet []*types.RepositoryRule `json:"rules_set,omitempty"` RepoSecrets []*RepositorySecret `json:"repository_secrets,omitempty"` + SecurityAndAnalysis *github.SecurityAndAnalysis `json:"security_and_analysis,omitempty"` } type RepositorySecret struct { diff --git a/internal/collectors/collector.go b/internal/collectors/collector.go index 614c3e07..330754d1 100644 --- a/internal/collectors/collector.go +++ b/internal/collectors/collector.go @@ -15,6 +15,7 @@ type CollectedDataContext interface { type CollectedDataRepositoryContext interface { CollectedDataContext HasBranchProtectionPermission() bool + HasGithubAdvancedSecurity() bool } type CollectedData struct { diff --git a/internal/collectors/github/repository_collector.go b/internal/collectors/github/repository_collector.go index f3148e08..e95e45df 100644 --- a/internal/collectors/github/repository_collector.go +++ b/internal/collectors/github/repository_collector.go @@ -136,11 +136,11 @@ func (rc *repositoryCollector) collectSpecific(repositories []types.RepositoryWi hasBp := hasBranchProtection(org, query.RepositoryOwner.Repository.IsPrivate) collectionContext = newRepositoryContext([]permissions.Role{org.Role, query.RepositoryOwner.Repository.ViewerPermission}, - hasBp, org.IsEnterprise(), false) + hasBp, org.IsEnterprise(), false, false) } else { hasBp := rc.hasBranchProtectionForUser(repo.Owner, query.RepositoryOwner.Repository.IsPrivate) collectionContext = newRepositoryContext([]permissions.Role{query.RepositoryOwner.Repository.ViewerPermission}, - hasBp, false, false) + hasBp, false, false, false) } rc.collectRepository(&query.RepositoryOwner.Repository, repo.Owner, collectionContext) @@ -206,7 +206,7 @@ func (rc *repositoryCollector) collectRepositories(org *ghcollected.ExtendedOrg) node := &(nodes[i]) extraGw.Do(func() { collectionContext := newRepositoryContext([]permissions.Role{org.Role, node.ViewerPermission}, - hasBranchProtection(org, node.IsPrivate), org.IsEnterprise(), false) + hasBranchProtection(org, node.IsPrivate), org.IsEnterprise(), false, false) rc.collectRepository(node, org.Name(), collectionContext) }) } @@ -226,9 +226,10 @@ func (rc *repositoryCollector) collectRepositories(org *ghcollected.ExtendedOrg) func (rc *repositoryCollector) collectRepository(repository *ghcollected.GitHubQLRepository, login string, collectionContext *repositoryContext) { repo := rc.collectExtraData(login, repository, collectionContext.isBranchProtectionSupported) entityName := collectors.FullRepoName(login, repo.Repository.Name) - missingPermissions := rc.checkMissingPermissions(repo, entityName) + missingPermissions := rc.checkMissingPermissions(repo, entityName, collectionContext) rc.IssueMissingPermissions(missingPermissions...) collectionContext.SetHasBranchProtectionPermission(!repo.NoBranchProtectionPermission) + collectionContext.SetHasGithubAdvancedSecurity(repo.SecurityAndAnalysis != nil) rc.CollectDataWithContext(repo, repo.Repository.Url, collectionContext) rc.CollectionChangeByOne() } @@ -255,6 +256,11 @@ func (rc *repositoryCollector) collectExtraData(login string, log.Printf("error getting repository dependency manifests for %s: %s", collectors.FullRepoName(login, repo.Repository.Name), err) } + repo, err = rc.withSecurityAndAnalysis(repo, login) + if err != nil { + log.Printf("failed to collect repository Security and Analysis settings for %s: %s", repo.Repository.Name, err) + } + if isBranchProtectionSupported { repo, err = rc.fixBranchProtectionInfo(repo, login) if err != nil { @@ -407,6 +413,17 @@ func (rc *repositoryCollector) withSecrets(repository ghcollected.Repository, lo return repository, nil } +func (rc *repositoryCollector) withSecurityAndAnalysis(repo ghcollected.Repository, login string) (ghcollected.Repository, error) { + + securityAndAnalysis, err := rc.Client.GetSecurityAndAnalysisForRepository(repo.Name(), login) + if err != nil { + return repo, err + } + + repo.SecurityAndAnalysis = securityAndAnalysis + return repo, nil +} + // fixBranchProtectionInfo fixes the branch protection info for the repository, // to reflect whether there is no branch protection, or just no permission to fetch the info. func (rc *repositoryCollector) fixBranchProtectionInfo(repository ghcollected.Repository, org string) (ghcollected.Repository, error) { @@ -444,16 +461,37 @@ func (rc *repositoryCollector) fixBranchProtectionInfo(repository ghcollected.Re return repository, nil } -func (rc *repositoryCollector) checkMissingPermissions(repo ghcollected.Repository, entityName string) []collectors.MissingPermission { +func (rc *repositoryCollector) checkMissingPermissions(repo ghcollected.Repository, entityName string, repoContext *repositoryContext) []collectors.MissingPermission { var missingPermissions []collectors.MissingPermission if repo.NoBranchProtectionPermission { effect := "Cannot read repository branch protection information" perm := collectors.NewMissingPermission(permissions.RepoAdmin, entityName, effect, namespace.Repository) missingPermissions = append(missingPermissions, perm) } + if repo.SecurityAndAnalysis == nil { + var effect string + if !checkRepoAdminPermission(repoContext.roles) { + effect = "Cannot read repository Security and Analysis settings" + } else if repo.Repository.IsPrivate { + effect = "Your GitHub plan does not include a secret scanning feature." + } + perm := collectors.NewMissingPermission(permissions.RepoAdmin, entityName, effect, namespace.Repository) + missingPermissions = append(missingPermissions, perm) + } + return missingPermissions } +func checkRepoAdminPermission(roles []permissions.RepositoryRole) bool{ + for _, role := range roles { + if (permissions.IsRepositoryRole(role) && role == permissions.RepoRoleAdmin) || + (permissions.IsOrgRole(role) && role == permissions.OrgRoleOwner) { + return true + } + } + return false +} + const ( orgIsFreeEffect = "Branch protection cannot be collected because the organization is in free plan" ) diff --git a/internal/collectors/github/repository_context.go b/internal/collectors/github/repository_context.go index 2e2dffc8..276b47a2 100644 --- a/internal/collectors/github/repository_context.go +++ b/internal/collectors/github/repository_context.go @@ -9,6 +9,7 @@ type repositoryContext struct { isEnterprise bool isBranchProtectionSupported bool hasBranchProtectionPermission bool + hasGithubAdvancedSecurity bool } func (rc *repositoryContext) Premium() bool { @@ -31,11 +32,20 @@ func (rc *repositoryContext) HasBranchProtectionPermission() bool { return rc.hasBranchProtectionPermission } -func newRepositoryContext(roles []permissions.RepositoryRole, isBranchProtectionSupported bool, isEnterprise bool, hasBranchProtectionPermission bool) *repositoryContext { +func (rc *repositoryContext) SetHasGithubAdvancedSecurity(value bool) { + rc.hasGithubAdvancedSecurity = value +} + +func (rc *repositoryContext) HasGithubAdvancedSecurity() bool { + return rc.hasGithubAdvancedSecurity +} + +func newRepositoryContext(roles []permissions.RepositoryRole, isBranchProtectionSupported bool, isEnterprise bool, hasBranchProtectionPermission bool, hasGithubAdvancedSecurity bool) *repositoryContext { return &repositoryContext{ roles: roles, isEnterprise: isEnterprise, isBranchProtectionSupported: isBranchProtectionSupported, hasBranchProtectionPermission: hasBranchProtectionPermission, + hasGithubAdvancedSecurity: hasGithubAdvancedSecurity, } } diff --git a/policies/github/repository.rego b/policies/github/repository.rego index 804e0bd0..ebecfb03 100644 --- a/policies/github/repository.rego +++ b/policies/github/repository.rego @@ -716,4 +716,23 @@ repository_secret_is_stale[stale] := true{ "update date" : time.format(secret.updated_at), } +} + +# METADATA +# scope: rule +# title: Secret Scanning should be enabled +# description: Repository should have secret scanning enabled. Secret scanning helps prevent the exposure of sensitive information and ensures compliance. +# custom: +# remediationSteps: +# - 1. Go to the repository settings page +# - 2. Under the 'Security' title on the left, select 'Code security and analysis' +# - 3. Under 'Secret scanning', click 'Enable' +# severity: MEDIUM +# requiredScopes: [repo] +# prerequisites: [advanced_security] +# threat: Exposed secrets increases the risk of sensitive information such as API keys, passwords, and tokens being disclosed, leading to unauthorized access to systems and services, and data breaches. +default secret_scanning_not_enabled := true + +secret_scanning_not_enabled := false{ + input.security_and_analysis.secret_scanning.status == "enabled" } \ No newline at end of file diff --git a/test/repository_test.go b/test/repository_test.go index 2266dfb8..d9a6dda2 100644 --- a/test/repository_test.go +++ b/test/repository_test.go @@ -371,6 +371,28 @@ func TestRepositoryWithNoSecrets(t *testing.T) { repositoryTestTemplate(t, name, makeMockData(), testedPolicyName, expectFailure, scm_type.GitHub) } +func TestRepositorySecretScanning(t *testing.T) { + name := "repository secret scanning is disabled" + testedPolicyName := "secret_scanning_not_enabled" + makeMockData := func(flag string) githubcollected.Repository { + return githubcollected.Repository{ + SecurityAndAnalysis: &github.SecurityAndAnalysis{ + SecretScanning: &github.SecretScanning{Status: &flag}, + }, + } + } + + options := map[bool]string{ + false: "enabled", + true: "disabled", + } + + for _, expectFailure := range bools { + flag := options[expectFailure] + repositoryTestTemplate(t, name, makeMockData(flag), testedPolicyName, expectFailure, scm_type.GitHub) + } +} + func TestGitlabRepositoryTooManyAdmins(t *testing.T) { name := "Project Has Too Many Owners" testedPolicyName := "project_has_too_many_admins"