From c340f11b1ace9cd1e6abd5a45cb73e4bfd37c429 Mon Sep 17 00:00:00 2001 From: Joseph Moukarzel Date: Fri, 21 Aug 2026 15:48:38 +0200 Subject: [PATCH 1/3] feat(controls): add projectMustHaveSecurityPolicySource (ISSUE-601, GitLab Ultimate) and renumber workflowsMustHaveExplicitName to ISSUE-422 for site parity --- .plumber.yaml | 9 + cmd/analyze_shared.go | 1 + cmd/legacy_json.go | 35 ++++ cmd/render_details.go | 18 ++ configuration/plumberconfig.go | 48 ++++++ configuration/plumberconfig_test.go | 1 + configuration/registry.go | 1 + configuration/v1_to_v2.go | 2 + control/catalog.go | 8 + control/codes.go | 15 +- control/status.go | 10 ++ control/task.go | 54 +++++- control/task_security_policy_test.go | 159 ++++++++++++++++++ control/types.go | 14 ++ defaultConfig/.plumber.yaml | 18 ++ docs/FINGERPRINT.md | 12 +- docs/GITHUB_ISSUES.md | 4 +- finding/identity/declarations.go | 6 +- finding/identity/identity_test.go | 3 +- gitlab/dataCollectionGitlabProtection.go | 22 +++ gitlab/gitlab_ir.go | 22 +++ gitlab/security_policy.go | 105 ++++++++++++ gitlab/security_policy_test.go | 101 +++++++++++ internal/ir/pipeline.go | 24 +++ policies/anonymous_definition.rego | 2 +- policies/rules_test.go | 84 ++++++++- policies/security_policy_project.rego | 87 ++++++++++ .../github/clean_named.yml | 0 .../github/violation_unnamed.yml | 0 29 files changed, 843 insertions(+), 22 deletions(-) create mode 100644 control/task_security_policy_test.go create mode 100644 gitlab/security_policy.go create mode 100644 gitlab/security_policy_test.go create mode 100644 policies/security_policy_project.rego rename policies/testdata/{ISSUE-601 => ISSUE-422}/github/clean_named.yml (100%) rename policies/testdata/{ISSUE-601 => ISSUE-422}/github/violation_unnamed.yml (100%) diff --git a/.plumber.yaml b/.plumber.yaml index f2444533..ab34f6c4 100644 --- a/.plumber.yaml +++ b/.plumber.yaml @@ -211,6 +211,15 @@ gitlab: mergeRequestApprovalRulesMustCoverAllProtectedBranches: enabled: true # =========================================== + # Project must have a security policy source + # =========================================== + # Requires the project to link a GitLab security policy project (Settings > + # Security & Compliance > Policies). Set expectedProjectId (numeric) OR + # expectedProjectPath (full path, case-insensitive) to require a specific + # policy project — the ID wins if both are set; leave both unset to require + # only that SOME policy project is linked. REQUIRES GITLAB ULTIMATE. + projectMustHaveSecurityPolicySource: + # =========================================== # Pipeline must not include hardcoded jobs # =========================================== # Detects CI/CD jobs that are defined directly in the .gitlab-ci.yml file diff --git a/cmd/analyze_shared.go b/cmd/analyze_shared.go index 8af6da26..b0eeee20 100644 --- a/cmd/analyze_shared.go +++ b/cmd/analyze_shared.go @@ -105,6 +105,7 @@ func outputTextWithProvider(p provider.Provider, result *control.AnalysisResult, renderFindingGroups(filterGroupsForDegraded(groups, result.DataCollectionDegraded)) renderWarnings(result.Warnings) renderApprovalRulesTierCaveat(result) + renderSecurityPolicyTierCaveat(result) printSectionHeader("Summary") fmt.Println() diff --git a/cmd/legacy_json.go b/cmd/legacy_json.go index 4b5b6a1c..3e6e76f0 100644 --- a/cmd/legacy_json.go +++ b/cmd/legacy_json.go @@ -108,6 +108,17 @@ func _withControlMeta(block any, e control.ControlEntry, result *control.Analysi "message": approvalRulesTierCaveatMessage, } } + if result != nil && result.SecurityPolicyTierCaveat && e.ControlName == "projectMustHaveSecurityPolicySource" { + // No policy project is linked, and security policies are an Ultimate + // feature, so we cannot tell a non-Ultimate project (unable to link) + // from an Ultimate project that left it unset. Keyed so a consumer can + // annotate ISSUE-601. + m["tierCaveat"] = map[string]any{ + "reason": "no-security-policy-project-linked", + "requiresTier": "ultimate", + "message": securityPolicyTierCaveatMessage, + } + } } return block } @@ -124,6 +135,28 @@ func isApprovalRuleControl(name string) bool { name == "mergeRequestApprovalRulesMustCoverAllProtectedBranches" } +// securityPolicyTierCaveatMessage explains the Ultimate requirement for +// ISSUE-601 when no security policy project is linked. Shared by the terminal +// caveat (render_details.go) and the JSON tierCaveat. +const securityPolicyTierCaveatMessage = "Security policies require GitLab Ultimate, and no policy project is linked. If this project is not on GitLab Ultimate it cannot link one — disable this control. If it is, link the expected security policy project to satisfy the check." + +// buildSecurityPolicyProjectBlock emits the legacy JSON block for the +// security-policy-project linkage control (ISSUE-601). The finding is a +// project-level singleton (no file/job); its linkedProjectId / linkedProjectPath +// / expectedProjectId ride in the issue's data, preserved by projectFindings. +func buildSecurityPolicyProjectBlock(c legacyCommon, findings []opaengine.Finding) map[string]any { + return map[string]any{ + "issues": projectFindings(findings, "job"), + "metrics": map[string]any{ + "projectWithoutExpectedSecurityPolicy": len(findings), + }, + "version": "0.1.0", + "ciValid": c.CiValid, + "ciMissing": c.CiMissing, + "skipped": c.Skipped, + } +} + // buildLegacyResult routes a control entry to its legacy JSON // builder and returns the (jsonKey, block) pair. func buildLegacyResult(e control.ControlEntry, result *control.AnalysisResult, pc *configuration.PlumberConfig, findings []opaengine.Finding) (string, any) { @@ -140,6 +173,8 @@ func buildLegacyResult(e control.ControlEntry, result *control.AnalysisResult, p return "imageAuthorizedSourcesResult", buildImageAuthorizedSourcesBlock(common, result, findings) case "branchMustBeProtected": return "branchProtectionResult", buildBranchProtectionBlock(common, result, pc, findings) + case "projectMustHaveSecurityPolicySource": + return "securityPolicyProjectResult", buildSecurityPolicyProjectBlock(common, findings) case "pipelineMustNotIncludeHardcodedJobs": return "hardcodedJobsResult", buildHardcodedJobsBlock(common, result, findings) case "externalRefsMustNotCollide": diff --git a/cmd/render_details.go b/cmd/render_details.go index 293fa045..908a7ba6 100644 --- a/cmd/render_details.go +++ b/cmd/render_details.go @@ -944,10 +944,28 @@ func buildGitLabControlStats(controlName string, result *control.AnalysisResult, {Label: "Unprotected", Value: fmt.Sprintf("%d", unprotected)}, {Label: "Non-Compliant", Value: fmt.Sprintf("%d", nonCompliant)}, } + case "projectMustHaveSecurityPolicySource": + return []statLine{ + {Label: "Security Policy Not Linked", Value: fmt.Sprintf("%d", findingsCount)}, + } } return nil } +// renderSecurityPolicyTierCaveat prints a caveat when the security-policy +// control flagged a project with no policy project linked: the feature requires +// GitLab Ultimate, and a non-Ultimate project cannot link one, so the failure +// may be a tier limitation rather than a real misconfiguration (see +// AnalysisResult.SecurityPolicyTierCaveat). No-op otherwise. +func renderSecurityPolicyTierCaveat(result *control.AnalysisResult) { + if result == nil || !result.SecurityPolicyTierCaveat { + return + } + fmt.Println() + fmt.Printf(" %s⚠ Security policies are a GitLab Ultimate feature, and no policy project is linked.%s\n", colorYellow, colorReset) + fmt.Printf(" %s•%s If this project isn't on GitLab Ultimate it can't link one, so disable this control. If it is, link the expected security policy project to satisfy the check.\n", colorYellow, colorReset) +} + // _countScriptLines walks the merged GitLab CI conf and totals every // script line declared on every job (script, before_script, // after_script). Used as the "Script Lines Checked" denominator diff --git a/configuration/plumberconfig.go b/configuration/plumberconfig.go index 1796c3cf..a9fd986c 100644 --- a/configuration/plumberconfig.go +++ b/configuration/plumberconfig.go @@ -48,6 +48,9 @@ var validControlSchema = map[string][]string{ "cicdVariablesMustBeMasked": { "enabled", }, + "projectMustHaveSecurityPolicySource": { + "enabled", "expectedProjectId", "expectedProjectPath", + }, "pipelineMustNotIncludeHardcodedJobs": { "enabled", }, @@ -294,6 +297,11 @@ type ControlsConfig struct { // read (ISSUE-202). Config-free; toggle via `enabled`. CicdVariablesMustBeMasked *EnabledOnlyControlConfig `yaml:"cicdVariablesMustBeMasked,omitempty"` + // ProjectMustHaveSecurityPolicySource control configuration (GitLab only). + // Requires the project to link the expected GitLab security policy project + // (ISSUE-601). Requires GitLab Ultimate. + ProjectMustHaveSecurityPolicySource *SecurityPolicyControlConfig `yaml:"projectMustHaveSecurityPolicySource,omitempty"` + // PipelineMustNotIncludeHardcodedJobs control configuration PipelineMustNotIncludeHardcodedJobs *HardcodedJobsControlConfig `yaml:"pipelineMustNotIncludeHardcodedJobs,omitempty"` @@ -441,6 +449,37 @@ type EnabledOnlyControlConfig struct { Enabled *bool `yaml:"enabled,omitempty"` } +// SecurityPolicyControlConfig configures the GitLab security-policy-project +// linkage check (ISSUE-601). GitLab-only, requires Ultimate. When +// ExpectedProjectId is set, the linked policy project must be exactly that +// project. When it is unset, any linked policy project passes and the control +// fails only when none is linked. +type SecurityPolicyControlConfig struct { + // Enabled controls whether this check runs. + Enabled *bool `yaml:"enabled,omitempty"` + + // ExpectedProjectId is the numeric GitLab project ID the security policy + // project must match. Unset => require only that some policy project is + // linked. + ExpectedProjectId *int `yaml:"expectedProjectId,omitempty"` + + // ExpectedProjectPath is the full path (namespace/project) the linked + // security policy project must match — a human-friendly alternative to the + // numeric ID, compared case-insensitively. Ignored when ExpectedProjectId is + // also set (the ID is authoritative). Unset (and no ID) => require only that + // some policy project is linked. + ExpectedProjectPath *string `yaml:"expectedProjectPath,omitempty"` +} + +// IsEnabled reports whether the control is enabled. Returns false when the +// wrapper or the field is nil — same convention as every other IsEnabled(). +func (c *SecurityPolicyControlConfig) IsEnabled() bool { + if c == nil || c.Enabled == nil { + return false + } + return *c.Enabled +} + // IsEnabled reports whether the control is enabled. Returns false when // the wrapper or the field is nil — same convention as every other // IsEnabled() in this package. @@ -1223,6 +1262,15 @@ func (c *PlumberConfig) GetCicdVariablesMustBeMaskedConfig() *EnabledOnlyControl return c.ControlsFor("gitlab").CicdVariablesMustBeMasked } +// GetProjectMustHaveSecurityPolicySourceConfig returns the GitLab +// security-policy-project linkage control configuration (ISSUE-601), or nil. +func (c *PlumberConfig) GetProjectMustHaveSecurityPolicySourceConfig() *SecurityPolicyControlConfig { + if c == nil { + return nil + } + return c.ControlsFor("gitlab").ProjectMustHaveSecurityPolicySource +} + // IsEnabled returns whether the control is enabled // Returns false if not properly configured func (c *BranchProtectionControlConfig) IsEnabled() bool { diff --git a/configuration/plumberconfig_test.go b/configuration/plumberconfig_test.go index 33ed559f..b276c1ea 100644 --- a/configuration/plumberconfig_test.go +++ b/configuration/plumberconfig_test.go @@ -379,6 +379,7 @@ func TestValidControlNames(t *testing.T) { "pipelineMustNotOverrideJobVariables", "pipelineMustNotUseDockerInDocker", "pipelineMustNotUseUnsafeVariableExpansion", + "projectMustHaveSecurityPolicySource", "pullRequestTargetMustNotCheckoutHead", "releaseWorkflowsMustNotRestoreUntrustedCache", "reusableWorkflowsMustNotInheritSecrets", diff --git a/configuration/registry.go b/configuration/registry.go index dbf1be6a..dcf8becf 100644 --- a/configuration/registry.go +++ b/configuration/registry.go @@ -35,6 +35,7 @@ var controlsMeta = map[string]ControlMeta{ "mergeRequestApprovalRulesMustCoverAllProtectedBranches": {Providers: []string{ProviderGitLab}}, "cicdVariablesMustBeProtected": {Providers: []string{ProviderGitLab}}, "cicdVariablesMustBeMasked": {Providers: []string{ProviderGitLab}}, + "projectMustHaveSecurityPolicySource": {Providers: []string{ProviderGitLab}}, "containerImageMustComeFromAuthorizedSources": {Providers: []string{ProviderGitLab, ProviderGitHub}}, "containerImageMustNotUseForbiddenTags": {Providers: []string{ProviderGitLab, ProviderGitHub}}, "externalRefsMustNotCollide": {Providers: []string{ProviderGitLab, ProviderGitHub}}, diff --git a/configuration/v1_to_v2.go b/configuration/v1_to_v2.go index dc7c5630..82d85f37 100644 --- a/configuration/v1_to_v2.go +++ b/configuration/v1_to_v2.go @@ -75,6 +75,7 @@ func controlsConfigIsZero(c ControlsConfig) bool { c.MergeRequestApprovalRulesMustCoverAllProtectedBranches == nil && c.CicdVariablesMustBeProtected == nil && c.CicdVariablesMustBeMasked == nil && + c.ProjectMustHaveSecurityPolicySource == nil && c.PipelineMustNotIncludeHardcodedJobs == nil && c.IncludesMustBeUpToDate == nil && c.IncludesMustNotUseForbiddenVersions == nil && @@ -104,6 +105,7 @@ func controlsConfigEqual(a, b ControlsConfig) bool { a.MergeRequestApprovalRulesMustCoverAllProtectedBranches == b.MergeRequestApprovalRulesMustCoverAllProtectedBranches && a.CicdVariablesMustBeProtected == b.CicdVariablesMustBeProtected && a.CicdVariablesMustBeMasked == b.CicdVariablesMustBeMasked && + a.ProjectMustHaveSecurityPolicySource == b.ProjectMustHaveSecurityPolicySource && a.PipelineMustNotIncludeHardcodedJobs == b.PipelineMustNotIncludeHardcodedJobs && a.IncludesMustBeUpToDate == b.IncludesMustBeUpToDate && a.IncludesMustNotUseForbiddenVersions == b.IncludesMustNotUseForbiddenVersions && diff --git a/control/catalog.go b/control/catalog.go index e93897c2..ad449e67 100644 --- a/control/catalog.go +++ b/control/catalog.go @@ -72,6 +72,11 @@ func GitLabControls(pc *configuration.PlumberConfig) []ControlEntry { ControlName: "cicdVariablesMustBeMasked", Skipped: c.CicdVariablesMustBeMasked == nil || !c.CicdVariablesMustBeMasked.IsEnabled(), }) + entries = append(entries, ControlEntry{ + DisplayName: "Project must have a security policy source", + ControlName: "projectMustHaveSecurityPolicySource", + Skipped: c.ProjectMustHaveSecurityPolicySource == nil || !c.ProjectMustHaveSecurityPolicySource.IsEnabled(), + }) entries = append(entries, ControlEntry{ DisplayName: "Pipeline must not include hardcoded jobs", ControlName: "pipelineMustNotIncludeHardcodedJobs", @@ -347,6 +352,9 @@ func DisabledControlNames(c *configuration.ControlsConfig) map[string]bool { if cfg := c.CicdVariablesMustBeMasked; cfg == nil || !cfg.IsEnabled() { out["cicdVariablesMustBeMasked"] = true } + if cfg := c.ProjectMustHaveSecurityPolicySource; cfg == nil || !cfg.IsEnabled() { + out["projectMustHaveSecurityPolicySource"] = true + } if cfg := c.PipelineMustNotIncludeHardcodedJobs; cfg == nil || !cfg.IsEnabled() { out["pipelineMustNotIncludeHardcodedJobs"] = true } diff --git a/control/codes.go b/control/codes.go index e2f0f83f..ed36edcb 100644 --- a/control/codes.go +++ b/control/codes.go @@ -156,8 +156,8 @@ const ( // Issue codes for workflow-hygiene controls (6xx) const ( - // ISSUE-601: Workflow has no explicit `name:` field - CodeAnonymousDefinition ErrorCode = "ISSUE-601" + // ISSUE-422: Workflow has no explicit `name:` field + CodeAnonymousDefinition ErrorCode = "ISSUE-422" // ISSUE-418: Workflow has no `concurrency:` block at either level CodeMissingConcurrency ErrorCode = "ISSUE-418" // ISSUE-419: Workflow uses a misfeature pattern (shell: cmd, inline pip install curl|sh, …) @@ -188,6 +188,8 @@ const ( CodeMRApprovalRulesAllBranchesMissing ErrorCode = "ISSUE-504" // ISSUE-505: Branch has non-compliant protection settings CodeBranchNonCompliant ErrorCode = "ISSUE-505" + // ISSUE-601: No (or the wrong) GitLab security policy project is linked + CodeSecurityPolicyProjectNotSet ErrorCode = "ISSUE-601" // ISSUE-803: Job runs with overly broad permissions (write-all) CodeExcessivePermissions ErrorCode = "ISSUE-803" ) @@ -603,6 +605,15 @@ var errorCodeRegistry = map[ErrorCode]ErrorCodeInfo{ DocURL: docsBaseURL + string(CodeBranchNonCompliant), ControlName: "branchMustBeProtected", }, + CodeSecurityPolicyProjectNotSet: { + Code: CodeSecurityPolicyProjectNotSet, + Severity: SeverityCritical, + Title: "Missing security policy source on project", + Description: "The project does not have the expected GitLab security policy project linked (none is linked, or a different one than the configured expectation), so the organization's scan-execution and merge-request approval policies are not enforced on this project.", + Remediation: "Link the expected security policy project in Settings > Security & Compliance > Policies (or set it via the API), so the org's security policies apply. Security policies require GitLab Ultimate.", + DocURL: docsBaseURL + string(CodeSecurityPolicyProjectNotSet), + ControlName: "projectMustHaveSecurityPolicySource", + }, CodeTemplateInjection: { Code: CodeTemplateInjection, Severity: SeverityCritical, diff --git a/control/status.go b/control/status.go index f9707273..9d379fd1 100644 --- a/control/status.go +++ b/control/status.go @@ -125,6 +125,16 @@ func StatusFor(e ControlEntry, result *AnalysisResult, findingCount int) string } return StatusPassed } + if e.ControlName == "projectMustHaveSecurityPolicySource" { + // Reached only with zero findings (a finding returned Failed above). The + // linkage is read over its own API surface; when it could not be read + // authoritatively (401/403 or the field is unavailable) the control never + // truly evaluated and must not read as a pass. + if result.SecurityPolicyEvaluable { + return StatusPassed + } + return StatusError + } if result.CiMissing || !result.CiValid { return StatusError } diff --git a/control/task.go b/control/task.go index 9b86375d..e218a7c5 100644 --- a/control/task.go +++ b/control/task.go @@ -72,13 +72,31 @@ func approvalRulesTierCaveatApplies(conf *configuration.Configuration, protectio // protectionDataNeeded reports whether any control needs the GitLab protection // collection this run: branchMustBeProtected, or either approval-rule control // (they all read the one GitlabProtectionAnalysisData). + +const controlSecurityPolicy = "projectMustHaveSecurityPolicySource" + +// securityPolicyControlEnabled reports whether the security-policy-project +// linkage control (ISSUE-601) is active for this run. It reads the linkage the +// GitLab protection collection fetches, so that collection must run when it is +// enabled even if branchMustBeProtected is not. +func securityPolicyControlEnabled(conf *configuration.Configuration) bool { + if conf == nil || conf.PlumberConfig == nil { + return false + } + c := conf.PlumberConfig.GetProjectMustHaveSecurityPolicySourceConfig() + return c != nil && c.IsEnabled() && shouldRunControl(controlSecurityPolicy, conf) +} + +// protectionDataNeeded reports whether any control needs the GitLab protection +// collection this run: branchMustBeProtected or the security-policy control +// (they read the one GitlabProtectionAnalysisData). func protectionDataNeeded(conf *configuration.Configuration) bool { if shouldRunControl(controlBranchMustBeProtected, conf) { if cfg := conf.PlumberConfig.GetBranchMustBeProtectedConfig(); cfg != nil && cfg.IsEnabled() { return true } } - return mrApprovalRuleControlEnabled(conf) + return mrApprovalRuleControlEnabled(conf) || securityPolicyControlEnabled(conf) } // controlCicdVariablesMustBeProtected / ...Masked are the two .plumber.yaml @@ -101,6 +119,20 @@ func cicdVariableControlEnabled(conf *configuration.Configuration) bool { return false } +// securityPolicyTierCaveatApplies reports whether to surface the conditional +// Ultimate caveat for ISSUE-601: the control ran, the linkage was read +// authoritatively, and NO policy project is linked — the tier-ambiguous case +// (a non-Ultimate project cannot link one, but an Ultimate project may simply +// have left it unset). A wrong-project-linked read is a real misconfiguration +// on a paid tier, not a tier caveat, and a non-authoritative read is +// not-evaluable, so neither triggers it. +func securityPolicyTierCaveatApplies(conf *configuration.Configuration, protectionData *gitlab.GitlabProtectionAnalysisData) bool { + if !securityPolicyControlEnabled(conf) || protectionData == nil || !protectionData.SecurityPolicyKnown { + return false + } + return protectionData.SecurityPolicyProject == nil +} + // shouldScanMutableExec reports whether the collector should fetch and // scan action source for actionsMustNotExecuteMutableRemoteCode // (ISSUE-714/715). The scan is expensive (up to ~7 sequential HTTP @@ -247,6 +279,20 @@ func buildEngineConfig(controls *configuration.ControlsConfig) map[string]any { } cfg := map[string]any{} + if c := controls.ProjectMustHaveSecurityPolicySource; c != nil { + // expectedProjectId / expectedProjectPath reach the engine only when set: + // the Rego rule treats their absence as "require any linkage", the id as + // the authoritative match, and the path as a case-insensitive fallback. + entry := map[string]any{} + if c.ExpectedProjectId != nil { + entry["expectedProjectId"] = *c.ExpectedProjectId + } + if c.ExpectedProjectPath != nil { + entry["expectedProjectPath"] = *c.ExpectedProjectPath + } + cfg["projectMustHaveSecurityPolicySource"] = entry + } + if c := controls.ContainerImageMustNotUseForbiddenTags; c != nil { if len(c.Tags) > 0 { cfg["imageMutableTag"] = map[string]any{ @@ -752,6 +798,12 @@ func RunAnalysis(conf *configuration.Configuration) (*AnalysisResult, error) { // on Free). Flag it so the renderers can surface a Premium/Ultimate caveat. result.ApprovalRulesTierCaveat = approvalRulesTierCaveatApplies(conf, protectionData) result.VariablesData = variablesData + // ISSUE-601 is not-evaluable when the linkage could not be read (auth error / + // field unavailable): the collector leaves SecurityPolicyKnown false, so + // StatusFor reports error rather than a false pass. When it WAS read but + // nothing is linked, surface the conditional Ultimate tier caveat. + result.SecurityPolicyEvaluable = protectionData != nil && protectionData.SecurityPolicyKnown + result.SecurityPolicyTierCaveat = securityPolicyTierCaveatApplies(conf, protectionData) reportProgress(conf, analysisStepCount, analysisStepCount, "Analysis complete") diff --git a/control/task_security_policy_test.go b/control/task_security_policy_test.go new file mode 100644 index 00000000..0adc327d --- /dev/null +++ b/control/task_security_policy_test.go @@ -0,0 +1,159 @@ +package control + +import ( + "context" + "testing" + + "github.com/getplumber/plumber/configuration" + "github.com/getplumber/plumber/gitlab" + opaengine "github.com/getplumber/plumber/internal/engine/opa" + "github.com/getplumber/plumber/internal/ir" + "github.com/getplumber/plumber/policies" +) + +func spBoolPtr(b bool) *bool { return &b } +func spIntPtr(i int) *int { return &i } +func spStrPtr(s string) *string { return &s } + +func spConf(c *configuration.SecurityPolicyControlConfig) *configuration.Configuration { + return &configuration.Configuration{PlumberConfig: &configuration.PlumberConfig{ + GitLab: &configuration.ProviderConfig{Controls: configuration.ControlsConfig{ + ProjectMustHaveSecurityPolicySource: c, + }}, + }} +} + +// securityPolicyControlEnabled gates the protection collection for a +// security-policy-only run: wrongly false means the linkage is never fetched +// and ISSUE-601 silently reports not-evaluable on every run. +func TestSecurityPolicyControlEnabled(t *testing.T) { + if securityPolicyControlEnabled(&configuration.Configuration{}) { + t.Fatal("expected false when PlumberConfig is nil") + } + if securityPolicyControlEnabled(spConf(nil)) { + t.Fatal("expected false when the control is not configured") + } + if securityPolicyControlEnabled(spConf(&configuration.SecurityPolicyControlConfig{Enabled: spBoolPtr(false)})) { + t.Fatal("expected false when disabled") + } + if !securityPolicyControlEnabled(spConf(&configuration.SecurityPolicyControlConfig{Enabled: spBoolPtr(true)})) { + t.Fatal("expected true when enabled") + } + skipped := spConf(&configuration.SecurityPolicyControlConfig{Enabled: spBoolPtr(true)}) + skipped.SkipControlsFilter = []string{controlSecurityPolicy} + if securityPolicyControlEnabled(skipped) { + t.Fatal("expected false when in --skip-controls") + } + + // protectionDataNeeded must be true for a security-policy-only run so the + // protection collection (which carries the linkage) actually runs. + if !protectionDataNeeded(spConf(&configuration.SecurityPolicyControlConfig{Enabled: spBoolPtr(true)})) { + t.Fatal("expected protectionDataNeeded true when only the security-policy control is enabled") + } +} + +// securityPolicyTierCaveatApplies composes the enabled gate with the +// linkage-read state: it fires only when the linkage was read and nothing is +// linked. A wrong-project read (a real misconfig on a paid tier) and a +// not-read state must not trigger it. +func TestSecurityPolicyTierCaveatApplies(t *testing.T) { + enabled := spConf(&configuration.SecurityPolicyControlConfig{Enabled: spBoolPtr(true)}) + disabled := spConf(&configuration.SecurityPolicyControlConfig{Enabled: spBoolPtr(false)}) + + noneLinked := &gitlab.GitlabProtectionAnalysisData{SecurityPolicyKnown: true, SecurityPolicyProject: nil} + linked := &gitlab.GitlabProtectionAnalysisData{SecurityPolicyKnown: true, SecurityPolicyProject: &gitlab.SecurityPolicyProjectLink{ID: 5}} + notRead := &gitlab.GitlabProtectionAnalysisData{SecurityPolicyKnown: false} + + if securityPolicyTierCaveatApplies(disabled, noneLinked) { + t.Fatal("caveat must NOT fire when the control is disabled") + } + if !securityPolicyTierCaveatApplies(enabled, noneLinked) { + t.Fatal("caveat must fire when enabled, read, and nothing linked") + } + if securityPolicyTierCaveatApplies(enabled, linked) { + t.Fatal("caveat must NOT fire when a project is linked (paid tier, real misconfig)") + } + if securityPolicyTierCaveatApplies(enabled, notRead) { + t.Fatal("caveat must NOT fire when the linkage was not read (not-evaluable)") + } + if securityPolicyTierCaveatApplies(enabled, nil) { + t.Fatal("caveat must NOT fire when there is no protection data") + } +} + +// TestSecurityPolicyConfigContract pins the struct -> map -> rego chain for +// ISSUE-601: buildEngineConfig emits expectedProjectId only when set, and the +// rego reads exactly that key, so a rename on either side would silently make +// the control assert only "any linkage" forever. +func TestSecurityPolicyConfigContract(t *testing.T) { + engine := opaengine.New() + if err := engine.LoadFromFSFiltered(policies.FS, nil); err != nil { + t.Fatalf("load policies: %v", err) + } + fires := func(linkedID int, cfg map[string]any) bool { + p := &ir.NormalizedPipeline{Provider: ir.ProviderGitLab, SecurityPolicyProject: &ir.SecurityPolicyProjectState{Known: true, LinkedProjectID: linkedID}} + findings, err := engine.Evaluate(context.Background(), p, cfg) + if err != nil { + t.Fatalf("evaluate: %v", err) + } + for _, f := range findings { + if f.Code == "ISSUE-601" { + return true + } + } + return false + } + + // expectedProjectId set via the REAL projection: a mismatch fires, a match does not. + cfgExpect := buildEngineConfig(&configuration.ControlsConfig{ + ProjectMustHaveSecurityPolicySource: &configuration.SecurityPolicyControlConfig{ + Enabled: spBoolPtr(true), ExpectedProjectId: spIntPtr(9), + }, + }) + if _, ok := cfgExpect["projectMustHaveSecurityPolicySource"]; !ok { + t.Fatal("buildEngineConfig did not project a projectMustHaveSecurityPolicySource block") + } + if !fires(5, cfgExpect) { + t.Fatal("expected id 9, linked 5: expected ISSUE-601 to fire") + } + if fires(9, cfgExpect) { + t.Fatal("expected id 9, linked 9: expected no ISSUE-601") + } + + // expectedProjectId unset -> require any linkage: nothing linked fires, a linked project passes. + cfgAny := buildEngineConfig(&configuration.ControlsConfig{ + ProjectMustHaveSecurityPolicySource: &configuration.SecurityPolicyControlConfig{Enabled: spBoolPtr(true)}, + }) + if !fires(0, cfgAny) { + t.Fatal("require-any, nothing linked: expected ISSUE-601 to fire") + } + if fires(7, cfgAny) { + t.Fatal("require-any, a project linked: expected no ISSUE-601") + } + + // expectedProjectPath via the REAL projection: case-insensitive path match. + firesPath := func(linkedPath string, cfg map[string]any) bool { + p := &ir.NormalizedPipeline{Provider: ir.ProviderGitLab, SecurityPolicyProject: &ir.SecurityPolicyProjectState{Known: true, LinkedProjectID: 5, LinkedProjectPath: linkedPath}} + findings, err := engine.Evaluate(context.Background(), p, cfg) + if err != nil { + t.Fatalf("evaluate: %v", err) + } + for _, f := range findings { + if f.Code == "ISSUE-601" { + return true + } + } + return false + } + cfgPath := buildEngineConfig(&configuration.ControlsConfig{ + ProjectMustHaveSecurityPolicySource: &configuration.SecurityPolicyControlConfig{ + Enabled: spBoolPtr(true), ExpectedProjectPath: spStrPtr("Grp/Policies"), + }, + }) + if firesPath("grp/policies", cfgPath) { + t.Fatal("path match (case-insensitive): expected no ISSUE-601") + } + if !firesPath("grp/other", cfgPath) { + t.Fatal("path mismatch: expected ISSUE-601 to fire") + } +} diff --git a/control/types.go b/control/types.go index 153af414..22f34159 100644 --- a/control/types.go +++ b/control/types.go @@ -61,6 +61,20 @@ type AnalysisResult struct { // report not-evaluable rather than a false pass (see StatusFor). VariablesData *gitlab.GitlabVariablesAnalysisData `json:"-"` + // SecurityPolicyEvaluable is true when the security policy project linkage + // was read authoritatively (a successful GraphQL read). False when it could + // not be read (auth error, or the field is unavailable on the instance), so + // StatusFor reports projectMustHaveSecurityPolicySource (ISSUE-601) as + // not-evaluable rather than a false pass. + SecurityPolicyEvaluable bool `json:"-"` + + // SecurityPolicyTierCaveat is set when the security-policy control ran, the + // linkage was read, and NO policy project is linked — the tier-ambiguous + // case (Ultimate-only feature; a non-Ultimate project cannot link one, an + // Ultimate project may have left it unset). Renderers surface a conditional + // caveat next to ISSUE-601. + SecurityPolicyTierCaveat bool `json:"-"` + // GitHubStats holds per-control denominators computed from the // GitHub IR after a GitHub analysis. Used by the GitHub renderer // to produce per-control stats blocks ("Total Images: 19, diff --git a/defaultConfig/.plumber.yaml b/defaultConfig/.plumber.yaml index 8662c551..df83f56e 100644 --- a/defaultConfig/.plumber.yaml +++ b/defaultConfig/.plumber.yaml @@ -307,6 +307,24 @@ gitlab: # Set to true to enable this control enabled: false # =========================================== + # Project must have a security policy source + # =========================================== + # Requires the project to link a GitLab security policy project (Settings > + # Security & Compliance > Policies), which carries the org's scan-execution + # and merge-request approval policies. To require a specific policy project, + # set expectedProjectId (numeric ID) OR expectedProjectPath (full path, + # matched case-insensitively); the ID wins if both are set. Leave both unset + # to require only that SOME policy project is linked. + # + # REQUIRES GITLAB ULTIMATE: on lower tiers no policy project can be linked, + # so this fires; a conditional caveat next to the finding says so. Ships + # disabled. + projectMustHaveSecurityPolicySource: + # Set to true to enable this control + enabled: false + # expectedProjectId: 123 + # expectedProjectPath: my-group/security-policy-project + # =========================================== # Pipeline must not include hardcoded jobs # =========================================== # Detects CI/CD jobs defined directly in .gitlab-ci.yml instead of being diff --git a/docs/FINGERPRINT.md b/docs/FINGERPRINT.md index 701ed202..1650c6cb 100644 --- a/docs/FINGERPRINT.md +++ b/docs/FINGERPRINT.md @@ -140,8 +140,8 @@ Finding as emitted by the rule the finding's canonical `Job` field rather than the `Data` payload bag. Most codes declare it, but nothing in the mechanism requires it: the repository- and file-level GitHub checks whose finding is not about a job leave it out -(`{file}` for ISSUE-418 / ISSUE-601, `{file, ecosystem}` for the dependabot -checks, the `{}` singleton for ISSUE-903 / ISSUE-904 / ISSUE-905), and a code +(`{file}` for ISSUE-418 / ISSUE-422, `{file, ecosystem}` for the dependabot +checks, the `{}` singleton for ISSUE-601 / ISSUE-903 / ISSUE-904 / ISSUE-905), and a code whose declaration does not name `job` does not hash on it at all. For a code that does declare it, `job` is empty when the finding is not about @@ -229,8 +229,8 @@ where the finding has no sub-finding subject: | `condition` (the `if:` expression) | ISSUE-210, ISSUE-211, ISSUE-212 | | `ecosystem` (the dependabot ecosystem) | ISSUE-901, ISSUE-902 | | `{file, job}` (one finding per job) | ISSUE-207, ISSUE-208, ISSUE-213, ISSUE-214, ISSUE-215, ISSUE-303, ISSUE-305, ISSUE-308, ISSUE-309, ISSUE-419, ISSUE-420, ISSUE-704, ISSUE-712, ISSUE-801, ISSUE-802, ISSUE-803 | -| `{file}` (one finding per workflow file) | ISSUE-418, ISSUE-601 | -| `{}` (one finding per repository) | ISSUE-903, ISSUE-904, ISSUE-905 | +| `{file}` (one finding per workflow file) | ISSUE-418, ISSUE-422 | +| `{}` (one finding per repository) | ISSUE-601, ISSUE-903, ISSUE-904, ISSUE-905 | Rewording any rule's prose no longer re-keys a registered finding. @@ -284,8 +284,8 @@ The same job cannot produce two ISSUE-803 findings, so `{file, job}` is a complete identity; two `write-all` jobs in different workflows differ on `file`. `identity.Of` reports `SubjectFromMessage == false`, and rewording the rule's message does not move the fingerprint. Coarser variants exist for -findings that are one per file (`{file}`: ISSUE-418, ISSUE-601) or one per -repository (the `{}` singleton: ISSUE-903, ISSUE-904, ISSUE-905). +findings that are one per file (`{file}`: ISSUE-418, ISSUE-422) or one per +repository (the `{}` singleton: ISSUE-601, ISSUE-903, ISSUE-904, ISSUE-905). Moving a rule from prose onto a structured payload changes its declaration and re-keys its findings once. Recipe version 2 did this for eleven finding blocks diff --git a/docs/GITHUB_ISSUES.md b/docs/GITHUB_ISSUES.md index 233c1dea..444b3175 100644 --- a/docs/GITHUB_ISSUES.md +++ b/docs/GITHUB_ISSUES.md @@ -76,7 +76,7 @@ reading the upstream docs. | Code | Name | Severity | | :--- | :--- | :--- | -| [ISSUE-601](#issue-601--anonymous-definition) | `anonymous-definition` | low | +| [ISSUE-422](#issue-422--anonymous-definition) | `anonymous-definition` | low | | [ISSUE-418](#issue-418--missing-concurrency) | `missing-concurrency` | medium | | [ISSUE-419](#issue-419--workflow-misfeature) | `workflow-misfeature` | medium | | [ISSUE-420](#issue-420--workflow-obfuscation) | `workflow-obfuscation` | high | @@ -1613,7 +1613,7 @@ jobs: --- -## ISSUE-601 — `anonymous-definition` +## ISSUE-422 — `anonymous-definition` **Severity:** `low` • **Control:** `workflowsMustHaveExplicitName` diff --git a/finding/identity/declarations.go b/finding/identity/declarations.go index d2fe5c36..03641d4c 100644 --- a/finding/identity/declarations.go +++ b/finding/identity/declarations.go @@ -165,8 +165,10 @@ var declarations = map[string][]string{ "ISSUE-504": {}, // Branch protection not compliant: keyed on the branch name. "ISSUE-505": {"file", "job", "branchName"}, - // Workflow has no explicit name: one finding per workflow file, keyed on the file (benched, not yet live: declaration provisional, revisit on unbench). - "ISSUE-601": {"file"}, + // Security policy project not linked: singleton finding (one per project); the platform IdOnly was empty, so the identity is the code alone. + "ISSUE-601": {}, + // Workflow has no explicit name: one finding per workflow file, keyed on the file (benched, not yet live: declaration provisional, revisit on unbench). Renumbered from 601 when the security-policy control took 601 (#417). + "ISSUE-422": {"file"}, // Action not pinned by commit SHA: keyed on the action ref (uses); step separates a reused action. "ISSUE-701": {"file", "job", "uses", "step"}, // Action in an archived repo: keyed on the action ref (uses); step separates a reused action. diff --git a/finding/identity/identity_test.go b/finding/identity/identity_test.go index 0305718b..4efcdfb3 100644 --- a/finding/identity/identity_test.go +++ b/finding/identity/identity_test.go @@ -364,7 +364,8 @@ func TestDeclarations_EveryCodeFingerprintIsPinned(t *testing.T) { "ISSUE-502": "b51fcaa43b9409cf", "ISSUE-504": "b698c0c9440ef0f5", "ISSUE-505": "4e929715c61fcba6", - "ISSUE-601": "9c1ecbe668ad9a36", + "ISSUE-601": "3a68700e66069498", + "ISSUE-422": "ade0bea017f69d56", "ISSUE-701": "87a2f87a752971bd", "ISSUE-702": "875ec32b1513e8a8", "ISSUE-703": "cf522b35974397b7", diff --git a/gitlab/dataCollectionGitlabProtection.go b/gitlab/dataCollectionGitlabProtection.go index 01f2fa9b..6958c318 100644 --- a/gitlab/dataCollectionGitlabProtection.go +++ b/gitlab/dataCollectionGitlabProtection.go @@ -77,6 +77,15 @@ type GitlabProtectionAnalysisData struct { MRApprovalSettings *glab.ProjectApprovals `json:"mrApprovalSettings"` MRSettings *glab.Project `json:"mrSettings"` ProjectMembers []GitlabMemberInfo `json:"projectMembers"` + + // SecurityPolicyKnown is true when the security policy project linkage was + // read authoritatively (a successful GraphQL read; nil linkage then means + // "none linked"). False when the linkage could not be read (auth error, or + // the field is unavailable) so ISSUE-601 reports not-evaluable, not a pass. + SecurityPolicyKnown bool `json:"securityPolicyKnown"` + // SecurityPolicyProject is the linked GitLab security policy project, or nil + // when none is linked. Only meaningful when SecurityPolicyKnown is true. + SecurityPolicyProject *SecurityPolicyProjectLink `json:"securityPolicyProject"` } // Run fetches all GitLab protection data needed by the controls @@ -151,6 +160,19 @@ func (dc *GitlabProtectionDataCollection) Run( returnedData.ProjectMembers = members } + // Get the linked security policy project (GraphQL; GitLab Ultimate). Fetched + // only when the control is enabled — it is a separate API surface, so a + // disabled control pays no cost. A read failure is never fatal: it leaves + // SecurityPolicyKnown false, so ISSUE-601 reports not-evaluable. + if spc := conf.PlumberConfig.GetProjectMustHaveSecurityPolicySourceConfig(); spc != nil && spc.IsEnabled() { + link, known, spErr := GetSecurityPolicyProject(project.Path, token, conf.GitlabURL, conf) + if spErr != nil { + l.WithError(spErr).Warn("Failed to fetch security policy project; ISSUE-601 will report not-evaluable") + } + returnedData.SecurityPolicyKnown = known + returnedData.SecurityPolicyProject = link + } + l.WithFields(logrus.Fields{ "branchCount": len(returnedData.Branches), "branchProtectionCount": len(returnedData.BranchProtections), diff --git a/gitlab/gitlab_ir.go b/gitlab/gitlab_ir.go index 97858d9e..26fa0cf2 100644 --- a/gitlab/gitlab_ir.go +++ b/gitlab/gitlab_ir.go @@ -51,6 +51,7 @@ func ToNormalizedPipeline( pipeline.Branches = buildBranches(protection) pipeline.MRApprovalRules, pipeline.MRApprovalRulesKnown = buildApprovalRules(protection) pipeline.SettingsVariables, pipeline.SettingsVariablesKnown = buildSettingsVariables(variables) + pipeline.SecurityPolicyProject = buildSecurityPolicyProject(protection) if origin != nil && origin.MergedConf != nil { if globals := extractGitLabVariables(origin.MergedConf.GlobalVariables); len(globals) > 0 { pipeline.GlobalVariables = globals @@ -117,6 +118,27 @@ func buildSettingsVariables(variables *GitlabVariablesAnalysisData) ([]ir.Settin return out, variables.Known } +// buildSecurityPolicyProject projects the collected security policy project +// linkage onto the IR. Returns nil when the linkage was not collected (the +// control disabled) so the rule sees no field and abstains. When collected, the +// Known flag carries whether the read was authoritative; a Known projection with +// LinkedProjectID == 0 means "no policy project linked". +func buildSecurityPolicyProject(protection *GitlabProtectionAnalysisData) *ir.SecurityPolicyProjectState { + if protection == nil { + return nil + } + // Not collected at all (control disabled): no Known flag, no linkage. + if !protection.SecurityPolicyKnown && protection.SecurityPolicyProject == nil { + return nil + } + state := &ir.SecurityPolicyProjectState{Known: protection.SecurityPolicyKnown} + if protection.SecurityPolicyProject != nil { + state.LinkedProjectID = protection.SecurityPolicyProject.ID + state.LinkedProjectPath = protection.SecurityPolicyProject.FullPath + } + return state +} + // buildBranches flattens the GitLab protection API response into // ir.Branch entries. Each repository branch is matched against the // declared protection patterns; when a pattern matches, its settings diff --git a/gitlab/security_policy.go b/gitlab/security_policy.go new file mode 100644 index 00000000..a15d0350 --- /dev/null +++ b/gitlab/security_policy.go @@ -0,0 +1,105 @@ +package gitlab + +import ( + "context" + "strconv" + "strings" + + "github.com/getplumber/plumber/configuration" + "github.com/machinebox/graphql" + "github.com/sirupsen/logrus" +) + +// SecurityPolicyProjectLink is the GitLab security policy project linked to the +// analysed project. GitLab Ultimate: a project points at a single security +// policy project that carries the org's scan-execution and merge-request +// approval policies. +type SecurityPolicyProjectLink struct { + // ID is the numeric project ID of the linked security policy project. + ID int + // FullPath is its namespace/path. + FullPath string +} + +// GetSecurityPolicyProject fetches the project's linked security policy project +// via GraphQL. It returns: +// - (link, true, nil) on a successful read where a policy project is linked; +// - (nil, true, nil) on a successful read where NONE is linked (the +// GitLab-Free/Ultimate-but-unlinked case — the field answers null); +// - (nil, false, err) when the linkage could not be read authoritatively (an +// auth error, or the field is unavailable on the instance). The bool is the +// "known" flag: a false known maps to not-evaluable, never a false pass. +// +// Security policies require GitLab Ultimate. On a non-Ultimate project the field +// answers null (no linkage), which is indistinguishable from an Ultimate project +// that simply has not linked one — the caller surfaces a conditional tier caveat +// rather than asserting the tier. +func GetSecurityPolicyProject(fullPath, token, instanceUrl string, conf *configuration.Configuration) (*SecurityPolicyProjectLink, bool, error) { + l := logrus.WithFields(logrus.Fields{ + "platform": "gitlab", + "action": "GetSecurityPolicyProject", + "projectFullPath": fullPath, + "instanceUrl": instanceUrl, + }) + + request := ` + query getSecurityPolicyProject($fullPath: ID!) { + project(fullPath: $fullPath) { + securityPolicyProject { + id + fullPath + } + } + } + ` + + type policyProject struct { + ID string `json:"id"` + FullPath string `json:"fullPath"` + } + type response struct { + Project *struct { + SecurityPolicyProject *policyProject `json:"securityPolicyProject"` + } `json:"project"` + } + + client := GetGraphQLClient(instanceUrl, conf) + req := graphql.NewRequest(request) + req.Var("fullPath", fullPath) + req.Header.Add("Authorization", "Bearer "+token) + + var respData response + if err := client.Run(context.Background(), req, &respData); err != nil { + // The field is absent from this instance's schema (old or unlicensed + // self-managed): treat as not-evaluable rather than a failure, matching + // the platform's "continue without security policy data" handling. + if strings.Contains(err.Error(), "securityPolicyProject") && strings.Contains(err.Error(), "doesn't exist") { + l.WithError(err).Warning("securityPolicyProject field unavailable on this GitLab instance; reporting not-evaluable") + return nil, false, nil + } + l.WithError(err).Error("Failed to read the security policy project through the GitLab GraphQL API") + return nil, false, err + } + + if respData.Project == nil || respData.Project.SecurityPolicyProject == nil { + return nil, true, nil // read succeeded; nothing linked + } + p := respData.Project.SecurityPolicyProject + return &SecurityPolicyProjectLink{ID: parseGitlabGID(p.ID), FullPath: p.FullPath}, true, nil +} + +// parseGitlabGID extracts the trailing numeric id from a GitLab GraphQL global +// id such as "gid://gitlab/Project/12345". Returns 0 when the tail is not a +// number (an unexpected id shape), so a malformed id never matches a configured +// expectedProjectId by accident. +func parseGitlabGID(gid string) int { + idx := strings.LastIndex(gid, "/") + if idx < 0 || idx+1 >= len(gid) { + return 0 + } + n, err := strconv.Atoi(gid[idx+1:]) + if err != nil { + return 0 + } + return n +} diff --git a/gitlab/security_policy_test.go b/gitlab/security_policy_test.go new file mode 100644 index 00000000..b9ded0f6 --- /dev/null +++ b/gitlab/security_policy_test.go @@ -0,0 +1,101 @@ +package gitlab + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/getplumber/plumber/configuration" +) + +// TestGetSecurityPolicyProject pins the four read outcomes the ISSUE-601 +// not-evaluable design depends on: a linked project, no linkage, the +// field-unavailable case, and an auth error. +func TestGetSecurityPolicyProject(t *testing.T) { + conf := &configuration.Configuration{HTTPClientTimeout: 30 * time.Second} + + t.Run("linked -> parsed id/path, known", func(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/api/graphql", func(w http.ResponseWriter, _ *http.Request) { + _ = json.NewEncoder(w).Encode(map[string]any{"data": map[string]any{ + "project": map[string]any{"securityPolicyProject": map[string]any{ + "id": "gid://gitlab/Project/4242", "fullPath": "grp/security-policies", + }}, + }}) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + link, known, err := GetSecurityPolicyProject("grp/app", "tok", srv.URL, conf) + if err != nil || !known { + t.Fatalf("expected a known success, got known=%v err=%v", known, err) + } + if link == nil || link.ID != 4242 || link.FullPath != "grp/security-policies" { + t.Fatalf("unexpected link: %+v", link) + } + }) + + t.Run("none linked -> nil link, known", func(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/api/graphql", func(w http.ResponseWriter, _ *http.Request) { + _ = json.NewEncoder(w).Encode(map[string]any{"data": map[string]any{ + "project": map[string]any{"securityPolicyProject": nil}, + }}) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + link, known, err := GetSecurityPolicyProject("grp/app", "tok", srv.URL, conf) + if err != nil || !known || link != nil { + t.Fatalf("none linked: expected (nil, true, nil), got (%+v, %v, %v)", link, known, err) + } + }) + + t.Run("field unavailable -> not-evaluable, no error", func(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/api/graphql", func(w http.ResponseWriter, _ *http.Request) { + _ = json.NewEncoder(w).Encode(map[string]any{"errors": []map[string]any{ + {"message": "Field 'securityPolicyProject' doesn't exist on type 'Project'"}, + }}) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + link, known, err := GetSecurityPolicyProject("grp/app", "tok", srv.URL, conf) + if err != nil || known || link != nil { + t.Fatalf("field unavailable: expected (nil, false, nil), got (%+v, %v, %v)", link, known, err) + } + }) + + t.Run("auth error -> not-evaluable with error", func(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/api/graphql", func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + link, known, err := GetSecurityPolicyProject("grp/app", "tok", srv.URL, conf) + if err == nil || known || link != nil { + t.Fatalf("auth error: expected (nil, false, err), got (%+v, %v, %v)", link, known, err) + } + }) +} + +func TestParseGitlabGID(t *testing.T) { + cases := map[string]int{ + "gid://gitlab/Project/12345": 12345, + "gid://gitlab/Project/1": 1, + "": 0, + "gid://gitlab/Project/": 0, + "not-a-gid": 0, + "gid://gitlab/Project/abc": 0, + } + for in, want := range cases { + if got := parseGitlabGID(in); got != want { + t.Errorf("parseGitlabGID(%q) = %d, want %d", in, got, want) + } + } +} diff --git a/internal/ir/pipeline.go b/internal/ir/pipeline.go index dcc231dd..f5d26ee4 100644 --- a/internal/ir/pipeline.go +++ b/internal/ir/pipeline.go @@ -101,6 +101,15 @@ type NormalizedPipeline struct { // ProtectionDetailsKnown. SettingsVariablesKnown bool `json:"settingsVariablesKnown,omitempty"` + // SecurityPolicyProject is the GitLab security policy project linked to this + // project (Settings > Security & Compliance > Policies), projected from the + // protection collection. nil when the linkage was not collected (the control + // is disabled) or could not be read authoritatively, so the + // projectMustHaveSecurityPolicySource control (ISSUE-601) abstains and + // reports not-evaluable. Distinct from SecurityPolicyPath above, which is the + // repository's SECURITY.md file (a GitHub-oriented, unrelated control). + SecurityPolicyProject *SecurityPolicyProjectState `json:"securityPolicyProject,omitempty"` + // Dockerfiles lists every Dockerfile the collector scanned at the // repo root and under common build directories, with each FROM // base-image extracted so policies can check pinning state. @@ -118,6 +127,21 @@ type NormalizedPipeline struct { Raw map[string]any `json:"raw,omitempty"` } +// SecurityPolicyProjectState is the GitLab security policy project linkage +// projected onto the IR. Known is true when the linkage was read +// authoritatively; a Known projection with LinkedProjectID == 0 means no policy +// project is linked. When Known is false the rule abstains (not-evaluable). +type SecurityPolicyProjectState struct { + // Known is true when the linkage was read authoritatively. + Known bool `json:"known"` + // LinkedProjectID is the numeric ID of the linked security policy project, + // or 0 when none is linked. + LinkedProjectID int `json:"linkedProjectId"` + // LinkedProjectPath is the full path of the linked security policy project, + // or "" when none is linked. + LinkedProjectPath string `json:"linkedProjectPath"` +} + // Dockerfile captures the result of parsing a single Dockerfile's // FROM directives for supply-chain auditing. type Dockerfile struct { diff --git a/policies/anonymous_definition.rego b/policies/anonymous_definition.rego index ee6404de..ea050a7b 100644 --- a/policies/anonymous_definition.rego +++ b/policies/anonymous_definition.rego @@ -16,7 +16,7 @@ deny contains finding if { input.pipeline.provider == "github" some file in _anonymous_workflow_files finding := { - "code": "ISSUE-601", + "code": "ISSUE-422", "severity": "low", "message": sprintf("workflow file %q has no top-level `name:` — GitHub falls back to the file path", [file]), "file": file, diff --git a/policies/rules_test.go b/policies/rules_test.go index 04a39f21..eba9b538 100644 --- a/policies/rules_test.go +++ b/policies/rules_test.go @@ -3609,9 +3609,79 @@ func TestIssue705_CachePoisoning_Configurable(t *testing.T) { }) } -// TestIssue601_AnonymousDefinition flags workflow files without a -// top-level `name:`. One finding per file (not per job). -func TestIssue601_AnonymousDefinition(t *testing.T) { +// TestIssue601_SecurityPolicyProject flags a GitLab project that does not link +// the expected security policy project. Singleton. Abstains when the linkage +// could not be read (known=false) or was not collected. +func TestIssue601_SecurityPolicyProject(t *testing.T) { + engine := opaengine.New() + if err := engine.LoadFromFSFiltered(policies.FS, nil); err != nil { + t.Fatalf("load embedded policies: %v", err) + } + count601 := func(p *ir.NormalizedPipeline, cfg map[string]any) int { + findings, err := engine.Evaluate(context.Background(), p, cfg) + if err != nil { + t.Fatalf("evaluate: %v", err) + } + n := 0 + for _, f := range findings { + if f.Code == "ISSUE-601" { + n++ + } + } + return n + } + gl := func(sp *ir.SecurityPolicyProjectState) *ir.NormalizedPipeline { + return &ir.NormalizedPipeline{Provider: ir.ProviderGitLab, SecurityPolicyProject: sp} + } + anyLinkage := map[string]any{"projectMustHaveSecurityPolicySource": map[string]any{}} + expect9 := map[string]any{"projectMustHaveSecurityPolicySource": map[string]any{"expectedProjectId": 9}} + + // Require-any: nothing linked -> fires; something linked -> passes. + if got := count601(gl(&ir.SecurityPolicyProjectState{Known: true, LinkedProjectID: 0}), anyLinkage); got != 1 { + t.Fatalf("require-any, nothing linked: expected 1 ISSUE-601, got %d", got) + } + if got := count601(gl(&ir.SecurityPolicyProjectState{Known: true, LinkedProjectID: 5, LinkedProjectPath: "grp/pol"}), anyLinkage); got != 0 { + t.Fatalf("require-any, a project linked: expected 0 ISSUE-601, got %d", got) + } + + // Expected id: wrong linked -> fires; matching -> passes. + if got := count601(gl(&ir.SecurityPolicyProjectState{Known: true, LinkedProjectID: 5}), expect9); got != 1 { + t.Fatalf("expected id 9, linked 5: expected 1 ISSUE-601, got %d", got) + } + if got := count601(gl(&ir.SecurityPolicyProjectState{Known: true, LinkedProjectID: 9}), expect9); got != 0 { + t.Fatalf("expected id 9, linked 9: expected 0 ISSUE-601, got %d", got) + } + + // Path mode: expectedProjectPath, compared case-insensitively. + expectPath := map[string]any{"projectMustHaveSecurityPolicySource": map[string]any{"expectedProjectPath": "Grp/Policies"}} + if got := count601(gl(&ir.SecurityPolicyProjectState{Known: true, LinkedProjectID: 5, LinkedProjectPath: "grp/policies"}), expectPath); got != 0 { + t.Fatalf("path mode, case-insensitive match: expected 0 ISSUE-601, got %d", got) + } + if got := count601(gl(&ir.SecurityPolicyProjectState{Known: true, LinkedProjectID: 5, LinkedProjectPath: "grp/other"}), expectPath); got != 1 { + t.Fatalf("path mode, mismatch: expected 1 ISSUE-601, got %d", got) + } + + // Precedence: when both id and path are set, the id is authoritative — a + // matching id passes even if the path would mismatch. + both := map[string]any{"projectMustHaveSecurityPolicySource": map[string]any{"expectedProjectId": 9, "expectedProjectPath": "grp/other"}} + if got := count601(gl(&ir.SecurityPolicyProjectState{Known: true, LinkedProjectID: 9, LinkedProjectPath: "grp/policies"}), both); got != 0 { + t.Fatalf("id precedence: matching id must pass despite path mismatch, got %d ISSUE-601", got) + } + + // Abstain: linkage not read authoritatively (known=false) -> no finding even + // with an expectation, and no projection at all -> no finding. + if got := count601(gl(&ir.SecurityPolicyProjectState{Known: false}), expect9); got != 0 { + t.Fatalf("known=false must abstain (not-evaluable), got %d ISSUE-601", got) + } + if got := count601(gl(nil), expect9); got != 0 { + t.Fatalf("no security-policy projection must abstain, got %d ISSUE-601", got) + } +} + +// TestIssue422_AnonymousDefinition flags workflow files without a +// top-level `name:`. One finding per file (not per job). Renumbered from +// ISSUE-601 when the security-policy control took 601 (#417). +func TestIssue422_AnonymousDefinition(t *testing.T) { cases := []struct { fixture string wantCount int @@ -3631,7 +3701,7 @@ func TestIssue601_AnonymousDefinition(t *testing.T) { if err := os.MkdirAll(wfDir, 0o755); err != nil { t.Fatal(err) } - src := filepath.Join("testdata", "ISSUE-601", "github", tc.fixture) + src := filepath.Join("testdata", "ISSUE-422", "github", tc.fixture) data, err := os.ReadFile(src) if err != nil { t.Fatalf("read fixture: %v", err) @@ -3649,7 +3719,7 @@ func TestIssue601_AnonymousDefinition(t *testing.T) { } hits := 0 for _, f := range findings { - if f.Code == "ISSUE-601" { + if f.Code == "ISSUE-422" { hits++ } } @@ -3660,9 +3730,9 @@ func TestIssue601_AnonymousDefinition(t *testing.T) { } } -// TestIssue602_MissingConcurrency flags workflow files with no +// TestIssue422_MissingConcurrency flags workflow files with no // concurrency block at either workflow or job level. -func TestIssue602_MissingConcurrency(t *testing.T) { +func TestIssue422_MissingConcurrency(t *testing.T) { cases := []struct { fixture string wantCount int diff --git a/policies/security_policy_project.rego b/policies/security_policy_project.rego new file mode 100644 index 00000000..7017c715 --- /dev/null +++ b/policies/security_policy_project.rego @@ -0,0 +1,87 @@ +# security-policy-project — flag a GitLab project that does not link the +# expected security policy project (Settings > Security & Compliance > +# Policies). A linked security policy project carries the organization's +# scan-execution and merge-request approval policies; without it (or with the +# wrong one linked) those policies are not enforced on the project. GitLab-only +# singleton finding (one per project); the legacy platform's identity was empty, +# so the identity here is the code alone. +# +# Config projectMustHaveSecurityPolicySource, matched with this precedence: +# - expectedProjectId set => the linked project's numeric id must equal it +# exactly (authoritative; the front end always sends the id); +# - else expectedProjectPath set => the linked project's full path must equal +# it, compared case-insensitively (a human-friendly alternative); +# - else (neither set) => any linked policy project passes, and the +# control fails only when none is linked. +# +# Reads input.pipeline.securityPolicyProject, projected from the protection +# collection (gitlab/gitlab_ir.go::buildSecurityPolicyProject). The projection +# is absent when the control did not collect it, and carries known=false when +# the linkage could not be read (a 401/403, or the field is unavailable on the +# instance); the rule abstains in both cases, so the control reports +# not-evaluable, not a pass. +# +# Security policies require GitLab Ultimate. On a non-Ultimate project the +# linkage reads as none, which is indistinguishable from an Ultimate project +# that has not linked one — the Go layer surfaces a conditional Ultimate tier +# caveat next to this finding rather than the rule asserting the tier. +package security_policy_project + +import rego.v1 + +deny contains finding if { + input.pipeline.provider == "gitlab" + sp := input.pipeline.securityPolicyProject + sp.known == true + cfg := object.get(input.config, "projectMustHaveSecurityPolicySource", {}) + finding := { + "code": "ISSUE-601", + "severity": "critical", + "message": _violation(sp, cfg), + "linkedProjectId": sp.linkedProjectId, + "linkedProjectPath": sp.linkedProjectPath, + } +} + +# expectedProjectId 0 (or absent) means "no id configured"; GitLab project ids +# start at 1, so 0 is a safe sentinel. expectedProjectPath "" means "no path +# configured". The three modes below are mutually exclusive by their guards, so +# exactly one _violation body can match: id wins, then path, then any-linkage. +_expected_id(cfg) := object.get(cfg, "expectedProjectId", 0) + +_expected_path(cfg) := object.get(cfg, "expectedProjectPath", "") + +# Normalise a path for comparison: trim surrounding slashes and lowercase, since +# GitLab namespaces are case-insensitive. +_norm(p) := trim(lower(p), "/") + +# ID mode (id set, authoritative): the linked id must equal it. +_violation(sp, cfg) := _mismatch_msg(sp, sprintf("id %d", [_expected_id(cfg)])) if { + _expected_id(cfg) != 0 + sp.linkedProjectId != _expected_id(cfg) +} + +# Path mode (no id, path set): the linked path must equal it, normalised. +_violation(sp, cfg) := _mismatch_msg(sp, sprintf("path %q", [_expected_path(cfg)])) if { + _expected_id(cfg) == 0 + _expected_path(cfg) != "" + _norm(sp.linkedProjectPath) != _norm(_expected_path(cfg)) +} + +# Any-linkage mode (neither set): fail only when nothing is linked. +_violation(sp, cfg) := "no GitLab security policy project is linked to this project" if { + _expected_id(cfg) == 0 + _expected_path(cfg) == "" + sp.linkedProjectId == 0 +} + +_mismatch_msg(sp, want) := sprintf("no GitLab security policy project is linked (expected %s)", [want]) if { + sp.linkedProjectId == 0 +} + +_mismatch_msg(sp, want) := sprintf( + "the linked GitLab security policy project (id %d, path %q) is not the expected project (%s)", + [sp.linkedProjectId, sp.linkedProjectPath, want], +) if { + sp.linkedProjectId != 0 +} diff --git a/policies/testdata/ISSUE-601/github/clean_named.yml b/policies/testdata/ISSUE-422/github/clean_named.yml similarity index 100% rename from policies/testdata/ISSUE-601/github/clean_named.yml rename to policies/testdata/ISSUE-422/github/clean_named.yml diff --git a/policies/testdata/ISSUE-601/github/violation_unnamed.yml b/policies/testdata/ISSUE-422/github/violation_unnamed.yml similarity index 100% rename from policies/testdata/ISSUE-601/github/violation_unnamed.yml rename to policies/testdata/ISSUE-422/github/violation_unnamed.yml From 8cc55f334b2b0dc8aa4bbd11764a44adc126a502 Mon Sep 17 00:00:00 2001 From: Joseph Moukarzel Date: Fri, 21 Aug 2026 18:36:19 +0200 Subject: [PATCH 2/3] docs(controls): update controls doc to make it more precise --- control/codes.go | 4 +-- gitlab/gitlab_ir_test.go | 51 +++++++++++++++++++++++++++ policies/security_policy_project.rego | 16 +++++---- 3 files changed, 63 insertions(+), 8 deletions(-) diff --git a/control/codes.go b/control/codes.go index ed36edcb..ef399a79 100644 --- a/control/codes.go +++ b/control/codes.go @@ -609,8 +609,8 @@ var errorCodeRegistry = map[ErrorCode]ErrorCodeInfo{ Code: CodeSecurityPolicyProjectNotSet, Severity: SeverityCritical, Title: "Missing security policy source on project", - Description: "The project does not have the expected GitLab security policy project linked (none is linked, or a different one than the configured expectation), so the organization's scan-execution and merge-request approval policies are not enforced on this project.", - Remediation: "Link the expected security policy project in Settings > Security & Compliance > Policies (or set it via the API), so the org's security policies apply. Security policies require GitLab Ultimate.", + Description: "The project does not directly link the expected GitLab security policy project (none is linked, or a different one than the configured expectation). This checks the project's own link only, so a security policy source inherited from a parent group is not detected.", + Remediation: "Link the expected security policy project in Settings > Security & Compliance > Policies (or set it via the API). If your policies are enforced at a parent group and inherited, this project-scoped check will not see them, so link at the project level too or disable this control. Security policies require GitLab Ultimate.", DocURL: docsBaseURL + string(CodeSecurityPolicyProjectNotSet), ControlName: "projectMustHaveSecurityPolicySource", }, diff --git a/gitlab/gitlab_ir_test.go b/gitlab/gitlab_ir_test.go index 90ed5e88..1f477798 100644 --- a/gitlab/gitlab_ir_test.go +++ b/gitlab/gitlab_ir_test.go @@ -80,6 +80,57 @@ func TestBuildSettingsVariables(t *testing.T) { } } +// TestBuildSecurityPolicyProject pins the collector-data -> IR projection that +// decides abstain vs fire for ISSUE-601. The subtle case is a successful read +// with nothing linked (Known=true, no project): the projection must return a +// non-nil state with LinkedProjectID 0 so the rule's require-any mode fires, +// rather than nil (which would abstain and silently miss an unlinked project). +func TestBuildSecurityPolicyProject(t *testing.T) { + cases := []struct { + name string + in *GitlabProtectionAnalysisData + want *ir.SecurityPolicyProjectState + }{ + { + name: "nil protection -> nil (not collected)", + in: nil, + want: nil, + }, + { + name: "not collected (Known=false, no project) -> nil (abstain)", + in: &GitlabProtectionAnalysisData{SecurityPolicyKnown: false, SecurityPolicyProject: nil}, + want: nil, + }, + { + name: "read OK, none linked -> non-nil Known with id 0 (must fire)", + in: &GitlabProtectionAnalysisData{SecurityPolicyKnown: true, SecurityPolicyProject: nil}, + want: &ir.SecurityPolicyProjectState{Known: true, LinkedProjectID: 0, LinkedProjectPath: ""}, + }, + { + name: "read OK, one linked -> non-nil Known with id/path", + in: &GitlabProtectionAnalysisData{SecurityPolicyKnown: true, SecurityPolicyProject: &SecurityPolicyProjectLink{ID: 42, FullPath: "grp/pol"}}, + want: &ir.SecurityPolicyProjectState{Known: true, LinkedProjectID: 42, LinkedProjectPath: "grp/pol"}, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := buildSecurityPolicyProject(tc.in) + if tc.want == nil { + if got != nil { + t.Fatalf("expected nil projection, got %+v", got) + } + return + } + if got == nil { + t.Fatalf("expected non-nil projection %+v, got nil", tc.want) + } + if got.Known != tc.want.Known || got.LinkedProjectID != tc.want.LinkedProjectID || got.LinkedProjectPath != tc.want.LinkedProjectPath { + t.Fatalf("projection mismatch: got %+v, want %+v", got, tc.want) + } + }) + } +} + func TestToNormalizedPipeline_Empty(t *testing.T) { pipeline := ToNormalizedPipeline("group/project", "main", "", nil, nil, nil, nil) if pipeline.Provider != ir.ProviderGitLab { diff --git a/policies/security_policy_project.rego b/policies/security_policy_project.rego index 7017c715..3353db0f 100644 --- a/policies/security_policy_project.rego +++ b/policies/security_policy_project.rego @@ -1,10 +1,14 @@ -# security-policy-project — flag a GitLab project that does not link the -# expected security policy project (Settings > Security & Compliance > +# security-policy-project — flag a GitLab project that does not directly link +# the expected security policy project (Settings > Security & Compliance > # Policies). A linked security policy project carries the organization's -# scan-execution and merge-request approval policies; without it (or with the -# wrong one linked) those policies are not enforced on the project. GitLab-only -# singleton finding (one per project); the legacy platform's identity was empty, -# so the identity here is the code alone. +# scan-execution and merge-request approval policies. This checks the project's +# own link only, matching the legacy platform: a policy source inherited from a +# parent group is not detected (GitLab's project-scoped securityPolicyProject +# field is null for an inherited source), and the linked project's policy +# contents are not inspected, so "linked" means a source is attached, not that a +# policy is necessarily enforced. GitLab-only singleton finding (one per +# project); the legacy platform's identity was empty, so the identity here is +# the code alone. # # Config projectMustHaveSecurityPolicySource, matched with this precedence: # - expectedProjectId set => the linked project's numeric id must equal it From 281872fc301ebdc78ed40664e57879d3ba9f4ecd Mon Sep 17 00:00:00 2001 From: Joseph Moukarzel Date: Tue, 25 Aug 2026 15:16:05 +0200 Subject: [PATCH 3/3] fix(controls): address review on the security policy source control --- .plumber.yaml | 4 ++ control/codes.go | 10 +++-- control/degraded.go | 7 ++++ control/mrcomment.go | 1 + control/status.go | 9 ++++- control/task.go | 50 ++++++++++++++++-------- control/task_security_policy_test.go | 21 ++++++---- gitlab/dataCollectionGitlabProtection.go | 22 ----------- gitlab/gitlab_ir.go | 20 +++++----- gitlab/gitlab_ir_test.go | 18 ++++----- gitlab/security_policy.go | 42 +++++++++++++++++++- gitlab/security_policy_test.go | 24 ++++++++++++ 12 files changed, 158 insertions(+), 70 deletions(-) diff --git a/.plumber.yaml b/.plumber.yaml index ab34f6c4..f5657a96 100644 --- a/.plumber.yaml +++ b/.plumber.yaml @@ -219,6 +219,10 @@ gitlab: # policy project — the ID wins if both are set; leave both unset to require # only that SOME policy project is linked. REQUIRES GITLAB ULTIMATE. projectMustHaveSecurityPolicySource: + # Set to true to enable this control + enabled: false + # expectedProjectId: 123 + # expectedProjectPath: my-group/security-policy-project # =========================================== # Pipeline must not include hardcoded jobs # =========================================== diff --git a/control/codes.go b/control/codes.go index ef399a79..ca6f766a 100644 --- a/control/codes.go +++ b/control/codes.go @@ -154,7 +154,7 @@ const ( CodeRequiredActionMissing ErrorCode = "ISSUE-417" ) -// Issue codes for workflow-hygiene controls (6xx) +// Issue codes for workflow-hygiene controls (4xx, plus the 9xx repo-hygiene codes) const ( // ISSUE-422: Workflow has no explicit `name:` field CodeAnonymousDefinition ErrorCode = "ISSUE-422" @@ -188,12 +188,16 @@ const ( CodeMRApprovalRulesAllBranchesMissing ErrorCode = "ISSUE-504" // ISSUE-505: Branch has non-compliant protection settings CodeBranchNonCompliant ErrorCode = "ISSUE-505" - // ISSUE-601: No (or the wrong) GitLab security policy project is linked - CodeSecurityPolicyProjectNotSet ErrorCode = "ISSUE-601" // ISSUE-803: Job runs with overly broad permissions (write-all) CodeExcessivePermissions ErrorCode = "ISSUE-803" ) +// Issue codes for security-policy-source controls (6xx) +const ( + // ISSUE-601: No (or the wrong) GitLab security policy project is linked + CodeSecurityPolicyProjectNotSet ErrorCode = "ISSUE-601" +) + // ErrorCodeInfo provides metadata about an issue code. type ErrorCodeInfo struct { // Code is the unique issue code (e.g., ISSUE-102). diff --git a/control/degraded.go b/control/degraded.go index 545cb20c..21bd4522 100644 --- a/control/degraded.go +++ b/control/degraded.go @@ -71,6 +71,13 @@ const degradedReasonBranchProtectionPrefix = "branch protection could not be fet // flipping every unrelated CI-file control to error. const degradedReasonVariablesPrefix = "CI/CD variables could not be fetched" +// degradedReasonSecurityPolicyPrefix is the shared prefix of the +// security-policy-linkage fetch degraded reason. Same contract as the two +// above: a transient network failure on the GraphQL read degrades the run so a +// blip cannot read as a clean exit-0 pass, while the carve-out in StatusFor +// keeps every unrelated control from flipping to error over it. +const degradedReasonSecurityPolicyPrefix = "security policy project could not be fetched" + // degradedReasonsFromGitHubCollection builds the human-readable list of // collection failures behind a degraded GitHub run (#220). partialCount // is the number of workflow files that could not be fetched/parsed and diff --git a/control/mrcomment.go b/control/mrcomment.go index 3cbf4ca9..3c6ffa49 100644 --- a/control/mrcomment.go +++ b/control/mrcomment.go @@ -252,6 +252,7 @@ func writeIssueDetails(b *strings.Builder, result *AnalysisResult) { {"containerImageMustNotUseForbiddenTags", "Container images must not use forbidden tags"}, {"containerImageMustComeFromAuthorizedSources", "Container images must come from authorized sources"}, {"branchMustBeProtected", "Branch must be protected"}, + {"projectMustHaveSecurityPolicySource", "Project must have a security policy source"}, {"cicdVariablesMustBeProtected", "CI/CD variables must be protected"}, {"cicdVariablesMustBeMasked", "CI/CD variables must be masked"}, {"pipelineMustNotIncludeHardcodedJobs", "Pipeline must not include hardcoded jobs"}, diff --git a/control/status.go b/control/status.go index 9d379fd1..5ce67954 100644 --- a/control/status.go +++ b/control/status.go @@ -34,6 +34,13 @@ func degradedReasonIsVariables(reason string) bool { return strings.HasPrefix(reason, degradedReasonVariablesPrefix) } +// degradedReasonIsSecurityPolicy classifies a DegradedReasons entry as the +// security-policy-linkage fetch failure, so it taints only ISSUE-601 rather +// than every CI-file control (task.go builds the string from the prefix). +func degradedReasonIsSecurityPolicy(reason string) bool { + return strings.HasPrefix(reason, degradedReasonSecurityPolicyPrefix) +} + // StatusFor derives a control's evaluation status for a run. // // Order matters: findings trump degradation — when a control found real @@ -139,7 +146,7 @@ func StatusFor(e ControlEntry, result *AnalysisResult, findingCount int) string return StatusError } for _, r := range result.DegradedReasons { - if !degradedReasonIsBranchProtection(r) && !degradedReasonIsVariables(r) { + if !degradedReasonIsBranchProtection(r) && !degradedReasonIsVariables(r) && !degradedReasonIsSecurityPolicy(r) { return StatusError } } diff --git a/control/task.go b/control/task.go index e218a7c5..acc293ca 100644 --- a/control/task.go +++ b/control/task.go @@ -69,16 +69,12 @@ func approvalRulesTierCaveatApplies(conf *configuration.Configuration, protectio return mrApprovalRuleControlEnabled(conf) && approvalRulesReturnedNone(protectionData) } -// protectionDataNeeded reports whether any control needs the GitLab protection -// collection this run: branchMustBeProtected, or either approval-rule control -// (they all read the one GitlabProtectionAnalysisData). - const controlSecurityPolicy = "projectMustHaveSecurityPolicySource" // securityPolicyControlEnabled reports whether the security-policy-project -// linkage control (ISSUE-601) is active for this run. It reads the linkage the -// GitLab protection collection fetches, so that collection must run when it is -// enabled even if branchMustBeProtected is not. +// linkage control (ISSUE-601) is active for this run. Its linkage is read by +// its own GraphQL collection, independent of the REST protection endpoints, so +// this does NOT imply the protection collection has to run. func securityPolicyControlEnabled(conf *configuration.Configuration) bool { if conf == nil || conf.PlumberConfig == nil { return false @@ -88,15 +84,17 @@ func securityPolicyControlEnabled(conf *configuration.Configuration) bool { } // protectionDataNeeded reports whether any control needs the GitLab protection -// collection this run: branchMustBeProtected or the security-policy control -// (they read the one GitlabProtectionAnalysisData). +// collection this run: branchMustBeProtected, or either approval-rule control +// (they all read the one GitlabProtectionAnalysisData). The security-policy +// control is deliberately absent: it has its own GraphQL collection, so it must +// not be able to force these REST fetches, nor be blocked when they fail. func protectionDataNeeded(conf *configuration.Configuration) bool { if shouldRunControl(controlBranchMustBeProtected, conf) { if cfg := conf.PlumberConfig.GetBranchMustBeProtectedConfig(); cfg != nil && cfg.IsEnabled() { return true } } - return mrApprovalRuleControlEnabled(conf) || securityPolicyControlEnabled(conf) + return mrApprovalRuleControlEnabled(conf) } // controlCicdVariablesMustBeProtected / ...Masked are the two .plumber.yaml @@ -126,11 +124,11 @@ func cicdVariableControlEnabled(conf *configuration.Configuration) bool { // have left it unset). A wrong-project-linked read is a real misconfiguration // on a paid tier, not a tier caveat, and a non-authoritative read is // not-evaluable, so neither triggers it. -func securityPolicyTierCaveatApplies(conf *configuration.Configuration, protectionData *gitlab.GitlabProtectionAnalysisData) bool { - if !securityPolicyControlEnabled(conf) || protectionData == nil || !protectionData.SecurityPolicyKnown { +func securityPolicyTierCaveatApplies(conf *configuration.Configuration, data *gitlab.SecurityPolicyData) bool { + if !securityPolicyControlEnabled(conf) || data == nil || !data.Known { return false } - return protectionData.SecurityPolicyProject == nil + return data.Project == nil } // shouldScanMutableExec reports whether the collector should fetch and @@ -219,6 +217,7 @@ func runRegoEngine( imageData *gitlab.GitlabPipelineImageData, protectionData *gitlab.GitlabProtectionAnalysisData, variablesData *gitlab.GitlabVariablesAnalysisData, + securityPolicyData *gitlab.SecurityPolicyData, ) []opaengine.Finding { pipeline := gitlab.ToNormalizedPipeline( conf.ProjectPath, @@ -228,6 +227,7 @@ func runRegoEngine( imageData, protectionData, variablesData, + securityPolicyData, ) return evaluatePolicies(l, conf, "gitlab", pipeline) } @@ -788,10 +788,28 @@ func RunAnalysis(conf *configuration.Configuration) (*AnalysisResult, error) { } } + // The security-policy linkage is read on its own, NOT inside the protection + // collection. It is a GraphQL surface, unrelated to the REST protection + // endpoints, and nesting it there made ISSUE-601 hostage to them: a token + // that cannot list branches aborts that collection before the read is ever + // reached, leaving the control not-evaluable on a linkage it could have read. + var securityPolicyData *gitlab.SecurityPolicyData + if securityPolicyControlEnabled(conf) { + var spErr error + securityPolicyData, spErr = gitlab.CollectSecurityPolicy(conf.ProjectPath, conf.GitlabToken, conf.GitlabURL, conf) + if spErr != nil && isNetworkError(spErr) { + // A transient network failure must not read as a clean pass: degrade + // the run (exit 3) the way the variables and branch collectors do. A + // permission failure is NOT network, so it stays a plain + // not-evaluable via Known=false without failing a complete run. + markDegraded(result, degradedReasonSecurityPolicyPrefix+" (network or timeout)") + } + } + // Rego/OPA rule engine evaluation — the single authoritative // compliance path (the legacy Go controls were retired in // docs/REFACTOR_MULTI_PROVIDER.md §8 Phase A). - result.Findings = runRegoEngine(l, conf, project, pipelineOriginData, pipelineImageData, protectionData, variablesData) + result.Findings = runRegoEngine(l, conf, project, pipelineOriginData, pipelineImageData, protectionData, variablesData, securityPolicyData) result.ProtectionData = protectionData // An approval-rule control that ran but saw zero rules is the ambiguous // GitLab-Free-vs-premium-with-no-rules case (the approvals API 200-empties @@ -802,8 +820,8 @@ func RunAnalysis(conf *configuration.Configuration) (*AnalysisResult, error) { // field unavailable): the collector leaves SecurityPolicyKnown false, so // StatusFor reports error rather than a false pass. When it WAS read but // nothing is linked, surface the conditional Ultimate tier caveat. - result.SecurityPolicyEvaluable = protectionData != nil && protectionData.SecurityPolicyKnown - result.SecurityPolicyTierCaveat = securityPolicyTierCaveatApplies(conf, protectionData) + result.SecurityPolicyEvaluable = securityPolicyData != nil && securityPolicyData.Known + result.SecurityPolicyTierCaveat = securityPolicyTierCaveatApplies(conf, securityPolicyData) reportProgress(conf, analysisStepCount, analysisStepCount, "Analysis complete") diff --git a/control/task_security_policy_test.go b/control/task_security_policy_test.go index 0adc327d..69cbee00 100644 --- a/control/task_security_policy_test.go +++ b/control/task_security_policy_test.go @@ -45,10 +45,15 @@ func TestSecurityPolicyControlEnabled(t *testing.T) { t.Fatal("expected false when in --skip-controls") } - // protectionDataNeeded must be true for a security-policy-only run so the - // protection collection (which carries the linkage) actually runs. - if !protectionDataNeeded(spConf(&configuration.SecurityPolicyControlConfig{Enabled: spBoolPtr(true)})) { - t.Fatal("expected protectionDataNeeded true when only the security-policy control is enabled") + // The linkage is read by its own GraphQL collection, so a + // security-policy-only run must NOT drag in the REST protection collection. + // The two were coupled originally, which is precisely what broke the + // control: FetchProjectBranchData aborts the whole collection on a 403, so a + // token that could not list branches left ISSUE-601 permanently + // not-evaluable on a linkage it could have read perfectly well. Keeping them + // independent is the fix; this assertion is what stops them being re-coupled. + if protectionDataNeeded(spConf(&configuration.SecurityPolicyControlConfig{Enabled: spBoolPtr(true)})) { + t.Fatal("protectionDataNeeded must be false for a security-policy-only run: the linkage has its own collection, and coupling it to the branch/approval fetches makes a 403 on branches silence ISSUE-601") } } @@ -60,9 +65,9 @@ func TestSecurityPolicyTierCaveatApplies(t *testing.T) { enabled := spConf(&configuration.SecurityPolicyControlConfig{Enabled: spBoolPtr(true)}) disabled := spConf(&configuration.SecurityPolicyControlConfig{Enabled: spBoolPtr(false)}) - noneLinked := &gitlab.GitlabProtectionAnalysisData{SecurityPolicyKnown: true, SecurityPolicyProject: nil} - linked := &gitlab.GitlabProtectionAnalysisData{SecurityPolicyKnown: true, SecurityPolicyProject: &gitlab.SecurityPolicyProjectLink{ID: 5}} - notRead := &gitlab.GitlabProtectionAnalysisData{SecurityPolicyKnown: false} + noneLinked := &gitlab.SecurityPolicyData{Known: true, Project: nil} + linked := &gitlab.SecurityPolicyData{Known: true, Project: &gitlab.SecurityPolicyProjectLink{ID: 5}} + notRead := &gitlab.SecurityPolicyData{Known: false} if securityPolicyTierCaveatApplies(disabled, noneLinked) { t.Fatal("caveat must NOT fire when the control is disabled") @@ -77,7 +82,7 @@ func TestSecurityPolicyTierCaveatApplies(t *testing.T) { t.Fatal("caveat must NOT fire when the linkage was not read (not-evaluable)") } if securityPolicyTierCaveatApplies(enabled, nil) { - t.Fatal("caveat must NOT fire when there is no protection data") + t.Fatal("caveat must NOT fire when the linkage was never collected") } } diff --git a/gitlab/dataCollectionGitlabProtection.go b/gitlab/dataCollectionGitlabProtection.go index 6958c318..01f2fa9b 100644 --- a/gitlab/dataCollectionGitlabProtection.go +++ b/gitlab/dataCollectionGitlabProtection.go @@ -77,15 +77,6 @@ type GitlabProtectionAnalysisData struct { MRApprovalSettings *glab.ProjectApprovals `json:"mrApprovalSettings"` MRSettings *glab.Project `json:"mrSettings"` ProjectMembers []GitlabMemberInfo `json:"projectMembers"` - - // SecurityPolicyKnown is true when the security policy project linkage was - // read authoritatively (a successful GraphQL read; nil linkage then means - // "none linked"). False when the linkage could not be read (auth error, or - // the field is unavailable) so ISSUE-601 reports not-evaluable, not a pass. - SecurityPolicyKnown bool `json:"securityPolicyKnown"` - // SecurityPolicyProject is the linked GitLab security policy project, or nil - // when none is linked. Only meaningful when SecurityPolicyKnown is true. - SecurityPolicyProject *SecurityPolicyProjectLink `json:"securityPolicyProject"` } // Run fetches all GitLab protection data needed by the controls @@ -160,19 +151,6 @@ func (dc *GitlabProtectionDataCollection) Run( returnedData.ProjectMembers = members } - // Get the linked security policy project (GraphQL; GitLab Ultimate). Fetched - // only when the control is enabled — it is a separate API surface, so a - // disabled control pays no cost. A read failure is never fatal: it leaves - // SecurityPolicyKnown false, so ISSUE-601 reports not-evaluable. - if spc := conf.PlumberConfig.GetProjectMustHaveSecurityPolicySourceConfig(); spc != nil && spc.IsEnabled() { - link, known, spErr := GetSecurityPolicyProject(project.Path, token, conf.GitlabURL, conf) - if spErr != nil { - l.WithError(spErr).Warn("Failed to fetch security policy project; ISSUE-601 will report not-evaluable") - } - returnedData.SecurityPolicyKnown = known - returnedData.SecurityPolicyProject = link - } - l.WithFields(logrus.Fields{ "branchCount": len(returnedData.Branches), "branchProtectionCount": len(returnedData.BranchProtections), diff --git a/gitlab/gitlab_ir.go b/gitlab/gitlab_ir.go index 26fa0cf2..5d00a739 100644 --- a/gitlab/gitlab_ir.go +++ b/gitlab/gitlab_ir.go @@ -35,6 +35,7 @@ func ToNormalizedPipeline( images *GitlabPipelineImageData, protection *GitlabProtectionAnalysisData, variables *GitlabVariablesAnalysisData, + securityPolicy *SecurityPolicyData, ) *ir.NormalizedPipeline { pipeline := &ir.NormalizedPipeline{ Provider: ir.ProviderGitLab, @@ -51,7 +52,7 @@ func ToNormalizedPipeline( pipeline.Branches = buildBranches(protection) pipeline.MRApprovalRules, pipeline.MRApprovalRulesKnown = buildApprovalRules(protection) pipeline.SettingsVariables, pipeline.SettingsVariablesKnown = buildSettingsVariables(variables) - pipeline.SecurityPolicyProject = buildSecurityPolicyProject(protection) + pipeline.SecurityPolicyProject = buildSecurityPolicyProject(securityPolicy) if origin != nil && origin.MergedConf != nil { if globals := extractGitLabVariables(origin.MergedConf.GlobalVariables); len(globals) > 0 { pipeline.GlobalVariables = globals @@ -123,18 +124,19 @@ func buildSettingsVariables(variables *GitlabVariablesAnalysisData) ([]ir.Settin // control disabled) so the rule sees no field and abstains. When collected, the // Known flag carries whether the read was authoritative; a Known projection with // LinkedProjectID == 0 means "no policy project linked". -func buildSecurityPolicyProject(protection *GitlabProtectionAnalysisData) *ir.SecurityPolicyProjectState { - if protection == nil { +func buildSecurityPolicyProject(data *SecurityPolicyData) *ir.SecurityPolicyProjectState { + if data == nil { return nil } - // Not collected at all (control disabled): no Known flag, no linkage. - if !protection.SecurityPolicyKnown && protection.SecurityPolicyProject == nil { + // Read failed (auth error, null project, field unavailable): abstain rather + // than let an unreadable linkage read as "none linked". + if !data.Known && data.Project == nil { return nil } - state := &ir.SecurityPolicyProjectState{Known: protection.SecurityPolicyKnown} - if protection.SecurityPolicyProject != nil { - state.LinkedProjectID = protection.SecurityPolicyProject.ID - state.LinkedProjectPath = protection.SecurityPolicyProject.FullPath + state := &ir.SecurityPolicyProjectState{Known: data.Known} + if data.Project != nil { + state.LinkedProjectID = data.Project.ID + state.LinkedProjectPath = data.Project.FullPath } return state } diff --git a/gitlab/gitlab_ir_test.go b/gitlab/gitlab_ir_test.go index 1f477798..3b8c141a 100644 --- a/gitlab/gitlab_ir_test.go +++ b/gitlab/gitlab_ir_test.go @@ -88,27 +88,27 @@ func TestBuildSettingsVariables(t *testing.T) { func TestBuildSecurityPolicyProject(t *testing.T) { cases := []struct { name string - in *GitlabProtectionAnalysisData + in *SecurityPolicyData want *ir.SecurityPolicyProjectState }{ { - name: "nil protection -> nil (not collected)", + name: "nil data -> nil (control disabled, never collected)", in: nil, want: nil, }, { - name: "not collected (Known=false, no project) -> nil (abstain)", - in: &GitlabProtectionAnalysisData{SecurityPolicyKnown: false, SecurityPolicyProject: nil}, + name: "unreadable (Known=false, no project) -> nil (abstain)", + in: &SecurityPolicyData{Known: false, Project: nil}, want: nil, }, { name: "read OK, none linked -> non-nil Known with id 0 (must fire)", - in: &GitlabProtectionAnalysisData{SecurityPolicyKnown: true, SecurityPolicyProject: nil}, + in: &SecurityPolicyData{Known: true, Project: nil}, want: &ir.SecurityPolicyProjectState{Known: true, LinkedProjectID: 0, LinkedProjectPath: ""}, }, { name: "read OK, one linked -> non-nil Known with id/path", - in: &GitlabProtectionAnalysisData{SecurityPolicyKnown: true, SecurityPolicyProject: &SecurityPolicyProjectLink{ID: 42, FullPath: "grp/pol"}}, + in: &SecurityPolicyData{Known: true, Project: &SecurityPolicyProjectLink{ID: 42, FullPath: "grp/pol"}}, want: &ir.SecurityPolicyProjectState{Known: true, LinkedProjectID: 42, LinkedProjectPath: "grp/pol"}, }, } @@ -132,7 +132,7 @@ func TestBuildSecurityPolicyProject(t *testing.T) { } func TestToNormalizedPipeline_Empty(t *testing.T) { - pipeline := ToNormalizedPipeline("group/project", "main", "", nil, nil, nil, nil) + pipeline := ToNormalizedPipeline("group/project", "main", "", nil, nil, nil, nil, nil) if pipeline.Provider != ir.ProviderGitLab { t.Fatalf("expected provider gitlab, got %q", pipeline.Provider) } @@ -162,7 +162,7 @@ func TestToNormalizedPipeline_JobsAndImages(t *testing.T) { }, } - pipeline := ToNormalizedPipeline("grp/proj", "main", "", origin, images, nil, nil) + pipeline := ToNormalizedPipeline("grp/proj", "main", "", origin, images, nil, nil, nil) if got := len(pipeline.Jobs); got != 3 { t.Fatalf("expected 3 jobs, got %d", got) @@ -196,7 +196,7 @@ func TestToNormalizedPipeline_NilJobInMap(t *testing.T) { }, } - pipeline := ToNormalizedPipeline("grp/proj", "main", "", origin, nil, nil, nil) + pipeline := ToNormalizedPipeline("grp/proj", "main", "", origin, nil, nil, nil, nil) if got := len(pipeline.Jobs); got != 1 { t.Fatalf("expected 1 job (nil entry skipped), got %d", got) } diff --git a/gitlab/security_policy.go b/gitlab/security_policy.go index a15d0350..5a498950 100644 --- a/gitlab/security_policy.go +++ b/gitlab/security_policy.go @@ -81,8 +81,17 @@ func GetSecurityPolicyProject(fullPath, token, instanceUrl string, conf *configu return nil, false, err } - if respData.Project == nil || respData.Project.SecurityPolicyProject == nil { - return nil, true, nil // read succeeded; nothing linked + if respData.Project == nil { + // A null `project` on HTTP 200 is GitLab's signature for "this token + // cannot see the project through GraphQL" — the GraphQL API answers + // with a null node rather than the 403 the REST API would return. It + // is NOT an authoritative "nothing linked": reporting it as one fires + // a Critical ISSUE-601 on a project whose linkage was never read. + l.Warning("GraphQL returned a null project (token cannot read it); reporting not-evaluable") + return nil, false, nil + } + if respData.Project.SecurityPolicyProject == nil { + return nil, true, nil // the project WAS read; nothing is linked } p := respData.Project.SecurityPolicyProject return &SecurityPolicyProjectLink{ID: parseGitlabGID(p.ID), FullPath: p.FullPath}, true, nil @@ -103,3 +112,32 @@ func parseGitlabGID(gid string) int { } return n } + +// SecurityPolicyData is the collected security-policy-project linkage for a +// run. It is deliberately its own collection rather than a field on +// GitlabProtectionAnalysisData: the linkage is read over GraphQL, a separate +// API surface from the REST protection endpoints, and folding it in made +// ISSUE-601 hostage to them. A token that cannot list branches aborts the +// protection collection before the GraphQL read is ever reached, which left +// the control reporting not-evaluable forever on a linkage it could have read +// perfectly well. +type SecurityPolicyData struct { + // Known is true when the linkage was read authoritatively. A nil Project + // then means "none linked", a real state the rule fires on. False means + // the read failed (auth error, null project, or the field is unavailable + // on this instance), so ISSUE-601 reports not-evaluable, not a false pass. + Known bool + // Project is the linked security policy project, or nil when none is + // linked. Only meaningful when Known is true. + Project *SecurityPolicyProjectLink +} + +// CollectSecurityPolicy reads the project's security-policy-project linkage. +// The returned error is the transport/API failure, if any: the caller decides +// whether it degrades the run (a network blip should not read as a clean pass) +// while the returned data already carries Known=false so the control reports +// not-evaluable either way. +func CollectSecurityPolicy(fullPath, token, instanceUrl string, conf *configuration.Configuration) (*SecurityPolicyData, error) { + link, known, err := GetSecurityPolicyProject(fullPath, token, instanceUrl, conf) + return &SecurityPolicyData{Known: known, Project: link}, err +} diff --git a/gitlab/security_policy_test.go b/gitlab/security_policy_test.go index b9ded0f6..b665128b 100644 --- a/gitlab/security_policy_test.go +++ b/gitlab/security_policy_test.go @@ -53,6 +53,30 @@ func TestGetSecurityPolicyProject(t *testing.T) { } }) + // The regression this branch shipped: GitLab answers HTTP 200 with a null + // `project` when the token cannot see the project through GraphQL (there is + // no 403 on this path). Reading that as an authoritative "nothing linked" + // fired a Critical ISSUE-601 on a project whose linkage was never read. + t.Run("null project (token cannot see it) -> not-evaluable, NOT none-linked", func(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/api/graphql", func(w http.ResponseWriter, _ *http.Request) { + _ = json.NewEncoder(w).Encode(map[string]any{"data": map[string]any{"project": nil}}) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + link, known, err := GetSecurityPolicyProject("grp/app", "tok", srv.URL, conf) + if err != nil { + t.Fatalf("a null project is not a transport error, got %v", err) + } + if link != nil { + t.Fatalf("expected no link, got %+v", link) + } + if known { + t.Fatal("null project must report known=false (not-evaluable); known=true makes ISSUE-601 fire a false Critical on an unread linkage") + } + }) + t.Run("field unavailable -> not-evaluable, no error", func(t *testing.T) { mux := http.NewServeMux() mux.HandleFunc("/api/graphql", func(w http.ResponseWriter, _ *http.Request) {