From 1c7a08843540d622c6c3b0c3960fc4fe7e992f13 Mon Sep 17 00:00:00 2001 From: Javier Rodriguez Date: Tue, 1 Apr 2025 11:08:55 +0200 Subject: [PATCH 1/4] feat(policy): Avoid policy evaluation creation Signed-off-by: Javier Rodriguez --- pkg/policies/engine/engine.go | 1 + pkg/policies/engine/rego/rego.go | 10 ++ pkg/policies/engine/rego/rego_test.go | 29 ++++ .../engine/rego/testfiles/result_format.rego | 7 + .../result_format_without_applicability.rego | 30 ++++ pkg/policies/policies.go | 23 ++- pkg/policies/policies_test.go | 151 +++++++++++++++--- pkg/policies/policy_groups.go | 10 ++ pkg/policies/policy_groups_test.go | 79 +++++++++ .../testdata/policy_group_multikind.yaml | 16 ++ .../policy_multi_kind_applicable.yaml | 69 ++++++++ .../testdata/policy_not_applicable.yaml | 47 ++++++ .../testdata/policy_openvex_applicable.yaml | 33 ++++ 13 files changed, 482 insertions(+), 23 deletions(-) create mode 100644 pkg/policies/engine/rego/testfiles/result_format_without_applicability.rego create mode 100644 pkg/policies/testdata/policy_group_multikind.yaml create mode 100644 pkg/policies/testdata/policy_multi_kind_applicable.yaml create mode 100644 pkg/policies/testdata/policy_not_applicable.yaml create mode 100644 pkg/policies/testdata/policy_openvex_applicable.yaml diff --git a/pkg/policies/engine/engine.go b/pkg/policies/engine/engine.go index dd61453ee..7e0522b5f 100644 --- a/pkg/policies/engine/engine.go +++ b/pkg/policies/engine/engine.go @@ -29,6 +29,7 @@ type EvaluationResult struct { Violations []*PolicyViolation Skipped bool SkipReason string + Applicable bool } // PolicyViolation represents a policy failure diff --git a/pkg/policies/engine/rego/rego.go b/pkg/policies/engine/rego/rego.go index a8395b592..b516ac5c4 100644 --- a/pkg/policies/engine/rego/rego.go +++ b/pkg/policies/engine/rego/rego.go @@ -166,6 +166,7 @@ func parseViolationsRule(res rego.ResultSet, policy *engine.Policy) (*engine.Eva Violations: violations, Skipped: false, // best effort SkipReason: "", + Applicable: true, // Assume old rules are applicable }, nil } @@ -189,6 +190,14 @@ func parseResultRule(res rego.ResultSet, policy *engine.Policy) (*engine.Evaluat reason = val } + var applicable bool + if val, ok := ruleResult["applicable"].(bool); ok { + applicable = val + } else { + // To be backwards compatible, if applicable is not present, we assume it is true + applicable = true + } + violations, ok := ruleResult["violations"].([]any) if !ok { return nil, engine.ResultFormatError{Field: "violations"} @@ -196,6 +205,7 @@ func parseResultRule(res rego.ResultSet, policy *engine.Policy) (*engine.Evaluat result.Skipped = skipped result.SkipReason = reason + result.Applicable = applicable for _, violation := range violations { vs, ok := violation.(string) diff --git a/pkg/policies/engine/rego/rego_test.go b/pkg/policies/engine/rego/rego_test.go index 2ee748de8..b2fda0464 100644 --- a/pkg/policies/engine/rego/rego_test.go +++ b/pkg/policies/engine/rego/rego_test.go @@ -193,6 +193,7 @@ func TestRego_ResultFormat(t *testing.T) { require.NoError(t, err) assert.True(t, result.Skipped) assert.Equal(t, "invalid input", result.SkipReason) + assert.True(t, result.Applicable) }) t.Run("valid input, no violations", func(t *testing.T) { @@ -200,6 +201,7 @@ func TestRego_ResultFormat(t *testing.T) { require.NoError(t, err) assert.False(t, result.Skipped) assert.Len(t, result.Violations, 0) + assert.True(t, result.Applicable) }) t.Run("valid input, violations", func(t *testing.T) { @@ -208,6 +210,33 @@ func TestRego_ResultFormat(t *testing.T) { assert.False(t, result.Skipped) assert.Len(t, result.Violations, 1) assert.Equal(t, "wrong CycloneDX version. Expected 1.5, but it was 1.4", result.Violations[0].Violation) + assert.True(t, result.Applicable) + }) + + t.Run("valid input, not applicable", func(t *testing.T) { + result, err := r.Verify(context.TODO(), policy, []byte("{\"specVersion\": \"1.0\"}"), nil) + require.NoError(t, err) + assert.False(t, result.Skipped) + assert.Len(t, result.Violations, 1) + assert.Equal(t, "wrong CycloneDX version. Expected 1.5, but it was 1.0", result.Violations[0].Violation) + assert.False(t, result.Applicable) + }) +} + +func TestRego_ResultFormatWithoutApplicability(t *testing.T) { + regoContent, err := os.ReadFile("testfiles/result_format_without_applicability.rego") + require.NoError(t, err) + + r := &Rego{} + policy := &engine.Policy{ + Name: "result-output", + Source: regoContent, + } + + t.Run("by default return always applicability to true", func(t *testing.T) { + result, err := r.Verify(context.TODO(), policy, []byte("{\"foo\": \"bar\"}"), nil) + require.NoError(t, err) + assert.True(t, result.Applicable) }) } diff --git a/pkg/policies/engine/rego/testfiles/result_format.rego b/pkg/policies/engine/rego/testfiles/result_format.rego index 89dd3cbd7..3bbcb7e00 100644 --- a/pkg/policies/engine/rego/testfiles/result_format.rego +++ b/pkg/policies/engine/rego/testfiles/result_format.rego @@ -6,6 +6,7 @@ result := { "skipped": skipped, "violations": violations, "skip_reason": skip_reason, + "applicable": applicable, } default skip_reason := "" @@ -17,8 +18,14 @@ skip_reason := m if { default skipped := true +default applicable := true + skipped := false if valid_input +applicable := false if { + input.specVersion == "1.0" +} + violations contains msg if { valid_input input.specVersion != "1.5" diff --git a/pkg/policies/engine/rego/testfiles/result_format_without_applicability.rego b/pkg/policies/engine/rego/testfiles/result_format_without_applicability.rego new file mode 100644 index 000000000..89dd3cbd7 --- /dev/null +++ b/pkg/policies/engine/rego/testfiles/result_format_without_applicability.rego @@ -0,0 +1,30 @@ +package main + +import rego.v1 + +result := { + "skipped": skipped, + "violations": violations, + "skip_reason": skip_reason, +} + +default skip_reason := "" + +skip_reason := m if { + not valid_input + m := "invalid input" +} + +default skipped := true + +skipped := false if valid_input + +violations contains msg if { + valid_input + input.specVersion != "1.5" + msg := sprintf("wrong CycloneDX version. Expected 1.5, but it was %s", [input.specVersion]) +} + +valid_input if { + input.specVersion +} diff --git a/pkg/policies/policies.go b/pkg/policies/policies.go index 6296e093b..f535a8386 100644 --- a/pkg/policies/policies.go +++ b/pkg/policies/policies.go @@ -95,7 +95,9 @@ func (pv *PolicyVerifier) VerifyMaterial(ctx context.Context, material *v12.Atte return nil, NewPolicyError(err) } - result = append(result, ev) + if ev != nil { + result = append(result, ev) + } } return result, nil @@ -142,6 +144,7 @@ func (pv *PolicyVerifier) evaluatePolicyAttachment(ctx context.Context, attachme sources := make([]string, 0) evalResults := make([]*engine.EvaluationResult, 0) skipped := true + scriptApplicableFound := false reasons := make([]string, 0) for _, script := range scripts { r, err := pv.executeScript(ctx, policy, script, material, args) @@ -149,6 +152,15 @@ func (pv *PolicyVerifier) evaluatePolicyAttachment(ctx context.Context, attachme return nil, NewPolicyError(err) } + // Skip if the script is not applicable, it basically skips adding the script to the + // evaluation results + if !r.Applicable { + continue + } + + // Mark the script as applicable + scriptApplicableFound = true + // Gather merged results evalResults = append(evalResults, r) @@ -161,6 +173,11 @@ func (pv *PolicyVerifier) evaluatePolicyAttachment(ctx context.Context, attachme sources = append(sources, base64.StdEncoding.EncodeToString(script.Source)) } + if !scriptApplicableFound { + pv.logger.Debug().Msgf("policy %s explicitily ignored by definition", policy.Metadata.Name) + return nil, nil + } + var evaluationSources []string if ref != nil && !IsProviderScheme(ref.URI) { evaluationSources = sources @@ -263,7 +280,9 @@ func (pv *PolicyVerifier) VerifyStatement(ctx context.Context, statement *intoto return nil, NewPolicyError(err) } - result = append(result, ev) + if ev != nil { + result = append(result, ev) + } } return result, nil diff --git a/pkg/policies/policies_test.go b/pkg/policies/policies_test.go index 3025f7be5..5ea419206 100644 --- a/pkg/policies/policies_test.go +++ b/pkg/policies/policies_test.go @@ -950,33 +950,43 @@ func (s *testSuite) TestComputePolicyArguments() { func (s *testSuite) TestNewResultFormat() { cases := []struct { - name string - policy string - material string - expectErr bool - expectViolations int - expectSkipped bool - expectReasons []string + name string + policy string + material string + expectErr bool + expectViolations int + expectSkipped bool + expectedApplicable bool + expectReasons []string }{ { - name: "result.violations", - policy: "file://testdata/policy_result_format.yaml", - material: "{\"specVersion\": \"1.4\"}", - expectViolations: 1, + name: "result.violations", + policy: "file://testdata/policy_result_format.yaml", + material: "{\"specVersion\": \"1.4\"}", + expectViolations: 1, + expectedApplicable: true, }, { - name: "skip", - policy: "file://testdata/policy_result_format.yaml", - material: "{\"invalid\": \"1.4\"}", - expectSkipped: true, - expectReasons: []string{"invalid input"}, + name: "skip", + policy: "file://testdata/policy_result_format.yaml", + material: "{\"invalid\": \"1.4\"}", + expectSkipped: true, + expectReasons: []string{"invalid input"}, + expectedApplicable: true, }, { - name: "skip multiple", - policy: "file://testdata/policy_result_skipped.yaml", - material: "{}", - expectSkipped: true, - expectReasons: []string{"this one is skipped", "this is also skipped"}, + name: "skip multiple", + policy: "file://testdata/policy_result_skipped.yaml", + material: "{}", + expectSkipped: true, + expectReasons: []string{"this one is skipped", "this is also skipped"}, + expectedApplicable: true, + }, + { + name: "not applicable", + policy: "file://testdata/policy_not_applicable.yaml", + material: "{\"specVersion\": \"1.0\"}", + expectedApplicable: false, }, } @@ -1014,6 +1024,12 @@ func (s *testSuite) TestNewResultFormat() { return } + if !tc.expectedApplicable { + s.Nil(err) + s.Equal([]*v1.PolicyEvaluation{}, res) + return + } + s.Require().NoError(err) s.Len(res, 1) s.Len(res[0].Violations, tc.expectViolations) @@ -1089,6 +1105,99 @@ func (s *testSuite) TestContainerMaterial() { } } +func (s *testSuite) TestMultiKindApplicable() { + cases := []struct { + name string + policy string + material string + expectErr bool + expectedEvaluations int + expectSkipped bool + expectReasons []string + expectedNotApplicable bool + }{ + { + name: "scripts applicable and skipped", + policy: "file://testdata/policy_multi_kind_applicable.yaml", + material: "{\"specVersion\": \"1.4\"}", + expectedEvaluations: 1, + expectSkipped: true, + expectReasons: []string{"this on is skipped"}, + }, + { + name: "scripts not applicable", + policy: "file://testdata/policy_multi_kind_applicable.yaml", + material: "{\"specVersion\": \"1.0\"}", + expectedEvaluations: 0, + expectedNotApplicable: true, + }, + { + name: "all scripts applicable", + policy: "file://testdata/policy_multi_kind_applicable.yaml", + material: "{\"specVersion\": \"1.4\"}", + expectedEvaluations: 1, + expectSkipped: false, + }, + } + + for _, tc := range cases { + s.Run(tc.name, func() { + schema := &v12.CraftingSchema{ + Materials: []*v12.CraftingSchema_Material{ + { + Name: "sbom", + Type: v12.CraftingSchema_Material_SBOM_CYCLONEDX_JSON, + }, + }, + Policies: &v12.Policies{ + Materials: []*v12.PolicyAttachment{ + { + Policy: &v12.PolicyAttachment_Ref{Ref: tc.policy}, + }, + }, + }, + } + + material := &v1.Attestation_Material{ + M: &v1.Attestation_Material_Artifact_{Artifact: &v1.Attestation_Material_Artifact{ + Content: []byte(tc.material), + }}, + MaterialType: v12.CraftingSchema_Material_SBOM_CYCLONEDX_JSON, + InlineCas: true, + } + + if tc.expectedNotApplicable { + material.MaterialType = v12.CraftingSchema_Material_SARIF + } + + if tc.name == "all scripts applicable" { + material.MaterialType = v12.CraftingSchema_Material_OPENVEX + } + + verifier := NewPolicyVerifier(schema, nil, &s.logger) + res, err := verifier.VerifyMaterial(context.TODO(), material, "") + + if tc.expectErr { + s.Error(err) + return + } + + if tc.expectedNotApplicable { + s.Nil(err) + s.Len(res, tc.expectedEvaluations) + return + } + + s.Require().NoError(err) + s.Len(res, tc.expectedEvaluations) + s.Equal(tc.expectSkipped, res[0].Skipped) + if len(res[0].SkipReasons) > 0 { + s.Equal(tc.expectReasons, res[0].SkipReasons) + } + }) + } +} + type testSuite struct { suite.Suite diff --git a/pkg/policies/policy_groups.go b/pkg/policies/policy_groups.go index 9a75ea421..38e733d9f 100644 --- a/pkg/policies/policy_groups.go +++ b/pkg/policies/policy_groups.go @@ -87,6 +87,11 @@ func (pgv *PolicyGroupVerifier) VerifyMaterial(ctx context.Context, material *ap return nil, NewPolicyError(err) } + if ev == nil { + // no evaluation, skip + continue + } + // Assign group reference to this evaluation ev.GroupReference = &api.PolicyEvaluation_Reference{ Name: group.GetMetadata().GetName(), @@ -133,6 +138,11 @@ func (pgv *PolicyGroupVerifier) VerifyStatement(ctx context.Context, statement * return nil, NewPolicyError(err) } + if ev == nil { + // no evaluation, skip + continue + } + // Assign group reference to this evaluation ev.GroupReference = &api.PolicyEvaluation_Reference{ Name: group.GetMetadata().GetName(), diff --git a/pkg/policies/policy_groups_test.go b/pkg/policies/policy_groups_test.go index c58b976df..7e221df96 100644 --- a/pkg/policies/policy_groups_test.go +++ b/pkg/policies/policy_groups_test.go @@ -270,6 +270,85 @@ func (s *groupsTestSuite) TestVerifyStatement() { } } +func (s *groupsTestSuite) TestVerifyMaterialMultiKind() { + cases := []struct { + name string + policyGroup string + material string + expectErr bool + expectedEvaluations int + expectSkipped bool + expectReasons []string + expectedNotApplicable bool + }{ + { + name: "not evaluation results, not applicable", + policyGroup: "file://testdata/policy_group_multikind.yaml", + material: "{\"specVersion\": \"1.0\"}", + expectedEvaluations: 0, + expectedNotApplicable: true, + }, + { + name: "evaluation results, applicable", + policyGroup: "file://testdata/policy_group_multikind.yaml", + material: "{\"specVersion\": \"1.4\"}", + expectedEvaluations: 1, + expectedNotApplicable: false, + }, + } + + for _, tc := range cases { + s.Run(tc.name, func() { + schema := &v1.CraftingSchema{ + Materials: []*v1.CraftingSchema_Material{ + { + Name: "sbom", + Type: v1.CraftingSchema_Material_SBOM_CYCLONEDX_JSON, + }, + }, + PolicyGroups: []*v1.PolicyGroupAttachment{ + { + Ref: tc.policyGroup, + }, + }, + } + + material := &api.Attestation_Material{ + M: &api.Attestation_Material_Artifact_{Artifact: &api.Attestation_Material_Artifact{ + Content: []byte(tc.material), + }}, + MaterialType: v1.CraftingSchema_Material_SBOM_CYCLONEDX_JSON, + InlineCas: true, + } + + if !tc.expectedNotApplicable { + material.MaterialType = v1.CraftingSchema_Material_OPENVEX + } + + verifier := NewPolicyGroupVerifier(schema, nil, &s.logger) + res, err := verifier.VerifyMaterial(context.TODO(), material, "") + + if tc.expectErr { + s.Error(err) + return + } + + if tc.expectedNotApplicable { + s.Nil(err) + s.Len(res, tc.expectedEvaluations) + return + } + + s.Require().NoError(err) + s.Len(res, tc.expectedEvaluations) + s.Equal(tc.expectSkipped, res[0].Skipped) + if len(res[0].SkipReasons) > 0 { + s.Equal(tc.expectReasons, res[0].SkipReasons) + } + }) + } +} + func (s *groupsTestSuite) TestGroupInputs() { cases := []struct { name string diff --git a/pkg/policies/testdata/policy_group_multikind.yaml b/pkg/policies/testdata/policy_group_multikind.yaml new file mode 100644 index 000000000..0c374459a --- /dev/null +++ b/pkg/policies/testdata/policy_group_multikind.yaml @@ -0,0 +1,16 @@ +apiVersion: workflowcontract.chainloop.dev/v1 +kind: PolicyGroup +metadata: + name: sbom-quality + description: This policy group applies a number of SBOM-related policies + annotations: + category: SBOM +spec: + policies: + materials: + - type: SBOM_CYCLONEDX_JSON + policies: + - ref: file://testdata/policy_not_applicable.yaml + - type: OPENVEX + policies: + - ref: file://testdata/policy_openvex_applicable.yaml diff --git a/pkg/policies/testdata/policy_multi_kind_applicable.yaml b/pkg/policies/testdata/policy_multi_kind_applicable.yaml new file mode 100644 index 000000000..5889d6683 --- /dev/null +++ b/pkg/policies/testdata/policy_multi_kind_applicable.yaml @@ -0,0 +1,69 @@ +apiVersion: workflowcontract.chainloop.dev/v1 +kind: Policy +metadata: + name: multikindapplicable + description: multikind policy + annotations: + category: SBOM +spec: + policies: + - kind: SARIF + embedded: | + package main + + import rego.v1 + + result := { + "skipped": true, + "violations": [], + "skip_reason": "this one is not applicable", + "applicable": false, + } + - kind: SBOM_CYCLONEDX_JSON + embedded: | + package main + + import rego.v1 + + result := { + "skipped": true, + "violations": [], + "skip_reason": "this one is not applicable", + "applicable": false, + } + - kind: SBOM_CYCLONEDX_JSON + embedded: | + package main + + import rego.v1 + + result := { + "skipped": true, + "violations": [], + "skip_reason": "this on is skipped", + "applicable": true, + } + - kind: OPENVEX + embedded: | + package main + + import rego.v1 + + result := { + "skipped": false, + "violations": [], + "skip_reason": "", + "applicable": true, + } + - kind: OPENVEX + embedded: | + package main + + import rego.v1 + + result := { + "skipped": false, + "violations": [], + "skip_reason": "", + "applicable": true, + } diff --git a/pkg/policies/testdata/policy_not_applicable.yaml b/pkg/policies/testdata/policy_not_applicable.yaml new file mode 100644 index 000000000..d8e22318c --- /dev/null +++ b/pkg/policies/testdata/policy_not_applicable.yaml @@ -0,0 +1,47 @@ +apiVersion: workflowcontract.chainloop.dev/v1 +kind: Policy +metadata: + name: policy-result-format + description: Policy with new result format + annotations: + category: SBOM +spec: + policies: + - kind: SBOM_CYCLONEDX_JSON + embedded: | + package main + + import rego.v1 + + result := { + "skipped": skipped, + "violations": violations, + "skip_reason": skip_reason, + "applicable": applicable, + } + + default skip_reason := "" + + skip_reason := m if { + not valid_input + m := "invalid input" + } + + default skipped := true + default applicable := true + + skipped := false if valid_input + + applicable := false if { + input.specVersion == "1.0" + } + + violations contains msg if { + valid_input + input.specVersion != "1.5" + msg := sprintf("wrong CycloneDX version. Expected 1.5, but it was %s", [input.specVersion]) + } + + valid_input if { + input.specVersion + } diff --git a/pkg/policies/testdata/policy_openvex_applicable.yaml b/pkg/policies/testdata/policy_openvex_applicable.yaml new file mode 100644 index 000000000..8f0f1012f --- /dev/null +++ b/pkg/policies/testdata/policy_openvex_applicable.yaml @@ -0,0 +1,33 @@ +apiVersion: workflowcontract.chainloop.dev/v1 +kind: Policy +metadata: + name: multikindapplicable + description: multikind policy + annotations: + category: SBOM +spec: + policies: + - kind: OPENVEX + embedded: | + package main + + import rego.v1 + + result := { + "skipped": false, + "violations": [], + "skip_reason": "", + "applicable": true, + } + - kind: OPENVEX + embedded: | + package main + + import rego.v1 + + result := { + "skipped": false, + "violations": [], + "skip_reason": "", + "applicable": true, + } From ac94415417b4a6a1467eaf8fd5b5b694fc1b0249 Mon Sep 17 00:00:00 2001 From: Javier Rodriguez Date: Tue, 1 Apr 2025 11:31:05 +0200 Subject: [PATCH 2/4] fix typo Signed-off-by: Javier Rodriguez --- pkg/policies/policies.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/policies/policies.go b/pkg/policies/policies.go index f535a8386..2ded29e39 100644 --- a/pkg/policies/policies.go +++ b/pkg/policies/policies.go @@ -174,7 +174,7 @@ func (pv *PolicyVerifier) evaluatePolicyAttachment(ctx context.Context, attachme } if !scriptApplicableFound { - pv.logger.Debug().Msgf("policy %s explicitily ignored by definition", policy.Metadata.Name) + pv.logger.Debug().Msgf("policy %s explicitly ignored by definition", policy.Metadata.Name) return nil, nil } From 0f1eb38ed0f3587342b41ae3d9415bd224ed8e19 Mon Sep 17 00:00:00 2001 From: Javier Rodriguez Date: Tue, 1 Apr 2025 12:50:51 +0200 Subject: [PATCH 3/4] tackle feedback Signed-off-by: Javier Rodriguez --- pkg/policies/engine/engine.go | 2 +- pkg/policies/engine/rego/rego.go | 13 +-- pkg/policies/engine/rego/rego_test.go | 16 +-- .../engine/rego/testfiles/result_format.rego | 6 +- ...rego => result_format_without_ignore.rego} | 0 pkg/policies/policies.go | 10 +- pkg/policies/policies_test.go | 102 +++++++++--------- pkg/policies/policy_groups_test.go | 40 +++---- .../testdata/policy_group_multikind.yaml | 4 +- ...aml => policy_multi_kind_with_ignore.yaml} | 16 +-- ...ble.yaml => policy_openvex_no_ignore.yaml} | 6 +- ...pplicable.yaml => policy_with_ignore.yaml} | 6 +- 12 files changed, 107 insertions(+), 114 deletions(-) rename pkg/policies/engine/rego/testfiles/{result_format_without_applicability.rego => result_format_without_ignore.rego} (100%) rename pkg/policies/testdata/{policy_multi_kind_applicable.yaml => policy_multi_kind_with_ignore.yaml} (80%) rename pkg/policies/testdata/{policy_openvex_applicable.yaml => policy_openvex_no_ignore.yaml} (86%) rename pkg/policies/testdata/{policy_not_applicable.yaml => policy_with_ignore.yaml} (90%) diff --git a/pkg/policies/engine/engine.go b/pkg/policies/engine/engine.go index 7e0522b5f..294ef051b 100644 --- a/pkg/policies/engine/engine.go +++ b/pkg/policies/engine/engine.go @@ -29,7 +29,7 @@ type EvaluationResult struct { Violations []*PolicyViolation Skipped bool SkipReason string - Applicable bool + Ignore bool } // PolicyViolation represents a policy failure diff --git a/pkg/policies/engine/rego/rego.go b/pkg/policies/engine/rego/rego.go index b516ac5c4..7845b229f 100644 --- a/pkg/policies/engine/rego/rego.go +++ b/pkg/policies/engine/rego/rego.go @@ -166,7 +166,7 @@ func parseViolationsRule(res rego.ResultSet, policy *engine.Policy) (*engine.Eva Violations: violations, Skipped: false, // best effort SkipReason: "", - Applicable: true, // Assume old rules are applicable + Ignore: false, // Assume old rules should not be ignored }, nil } @@ -190,12 +190,9 @@ func parseResultRule(res rego.ResultSet, policy *engine.Policy) (*engine.Evaluat reason = val } - var applicable bool - if val, ok := ruleResult["applicable"].(bool); ok { - applicable = val - } else { - // To be backwards compatible, if applicable is not present, we assume it is true - applicable = true + var ignore bool + if val, ok := ruleResult["ignore"].(bool); ok { + ignore = val } violations, ok := ruleResult["violations"].([]any) @@ -205,7 +202,7 @@ func parseResultRule(res rego.ResultSet, policy *engine.Policy) (*engine.Evaluat result.Skipped = skipped result.SkipReason = reason - result.Applicable = applicable + result.Ignore = ignore for _, violation := range violations { vs, ok := violation.(string) diff --git a/pkg/policies/engine/rego/rego_test.go b/pkg/policies/engine/rego/rego_test.go index b2fda0464..453e67f71 100644 --- a/pkg/policies/engine/rego/rego_test.go +++ b/pkg/policies/engine/rego/rego_test.go @@ -193,7 +193,7 @@ func TestRego_ResultFormat(t *testing.T) { require.NoError(t, err) assert.True(t, result.Skipped) assert.Equal(t, "invalid input", result.SkipReason) - assert.True(t, result.Applicable) + assert.False(t, result.Ignore) }) t.Run("valid input, no violations", func(t *testing.T) { @@ -201,7 +201,7 @@ func TestRego_ResultFormat(t *testing.T) { require.NoError(t, err) assert.False(t, result.Skipped) assert.Len(t, result.Violations, 0) - assert.True(t, result.Applicable) + assert.False(t, result.Ignore) }) t.Run("valid input, violations", func(t *testing.T) { @@ -210,21 +210,21 @@ func TestRego_ResultFormat(t *testing.T) { assert.False(t, result.Skipped) assert.Len(t, result.Violations, 1) assert.Equal(t, "wrong CycloneDX version. Expected 1.5, but it was 1.4", result.Violations[0].Violation) - assert.True(t, result.Applicable) + assert.False(t, result.Ignore) }) - t.Run("valid input, not applicable", func(t *testing.T) { + t.Run("valid input, not ignore", func(t *testing.T) { result, err := r.Verify(context.TODO(), policy, []byte("{\"specVersion\": \"1.0\"}"), nil) require.NoError(t, err) assert.False(t, result.Skipped) assert.Len(t, result.Violations, 1) assert.Equal(t, "wrong CycloneDX version. Expected 1.5, but it was 1.0", result.Violations[0].Violation) - assert.False(t, result.Applicable) + assert.True(t, result.Ignore) }) } func TestRego_ResultFormatWithoutApplicability(t *testing.T) { - regoContent, err := os.ReadFile("testfiles/result_format_without_applicability.rego") + regoContent, err := os.ReadFile("testfiles/result_format_without_ignore.rego") require.NoError(t, err) r := &Rego{} @@ -233,10 +233,10 @@ func TestRego_ResultFormatWithoutApplicability(t *testing.T) { Source: regoContent, } - t.Run("by default return always applicability to true", func(t *testing.T) { + t.Run("by default return always ignore to false", func(t *testing.T) { result, err := r.Verify(context.TODO(), policy, []byte("{\"foo\": \"bar\"}"), nil) require.NoError(t, err) - assert.True(t, result.Applicable) + assert.False(t, result.Ignore) }) } diff --git a/pkg/policies/engine/rego/testfiles/result_format.rego b/pkg/policies/engine/rego/testfiles/result_format.rego index 3bbcb7e00..2cb5484fb 100644 --- a/pkg/policies/engine/rego/testfiles/result_format.rego +++ b/pkg/policies/engine/rego/testfiles/result_format.rego @@ -6,7 +6,7 @@ result := { "skipped": skipped, "violations": violations, "skip_reason": skip_reason, - "applicable": applicable, + "ignore": ignore, } default skip_reason := "" @@ -18,11 +18,11 @@ skip_reason := m if { default skipped := true -default applicable := true +default ignore := false skipped := false if valid_input -applicable := false if { +ignore := true if { input.specVersion == "1.0" } diff --git a/pkg/policies/engine/rego/testfiles/result_format_without_applicability.rego b/pkg/policies/engine/rego/testfiles/result_format_without_ignore.rego similarity index 100% rename from pkg/policies/engine/rego/testfiles/result_format_without_applicability.rego rename to pkg/policies/engine/rego/testfiles/result_format_without_ignore.rego diff --git a/pkg/policies/policies.go b/pkg/policies/policies.go index 2ded29e39..07d0240e0 100644 --- a/pkg/policies/policies.go +++ b/pkg/policies/policies.go @@ -144,7 +144,6 @@ func (pv *PolicyVerifier) evaluatePolicyAttachment(ctx context.Context, attachme sources := make([]string, 0) evalResults := make([]*engine.EvaluationResult, 0) skipped := true - scriptApplicableFound := false reasons := make([]string, 0) for _, script := range scripts { r, err := pv.executeScript(ctx, policy, script, material, args) @@ -152,15 +151,12 @@ func (pv *PolicyVerifier) evaluatePolicyAttachment(ctx context.Context, attachme return nil, NewPolicyError(err) } - // Skip if the script is not applicable, it basically skips adding the script to the + // Skip if the script is telling us to specifically to ignore it, it basically skips adding the script to the // evaluation results - if !r.Applicable { + if r.Ignore { continue } - // Mark the script as applicable - scriptApplicableFound = true - // Gather merged results evalResults = append(evalResults, r) @@ -173,7 +169,7 @@ func (pv *PolicyVerifier) evaluatePolicyAttachment(ctx context.Context, attachme sources = append(sources, base64.StdEncoding.EncodeToString(script.Source)) } - if !scriptApplicableFound { + if len(sources) == 0 { pv.logger.Debug().Msgf("policy %s explicitly ignored by definition", policy.Metadata.Name) return nil, nil } diff --git a/pkg/policies/policies_test.go b/pkg/policies/policies_test.go index 5ea419206..19ce899b0 100644 --- a/pkg/policies/policies_test.go +++ b/pkg/policies/policies_test.go @@ -950,43 +950,43 @@ func (s *testSuite) TestComputePolicyArguments() { func (s *testSuite) TestNewResultFormat() { cases := []struct { - name string - policy string - material string - expectErr bool - expectViolations int - expectSkipped bool - expectedApplicable bool - expectReasons []string + name string + policy string + material string + expectErr bool + expectViolations int + expectSkipped bool + expectIgnore bool + expectReasons []string }{ { - name: "result.violations", - policy: "file://testdata/policy_result_format.yaml", - material: "{\"specVersion\": \"1.4\"}", - expectViolations: 1, - expectedApplicable: true, + name: "result.violations", + policy: "file://testdata/policy_result_format.yaml", + material: "{\"specVersion\": \"1.4\"}", + expectViolations: 1, + expectIgnore: false, }, { - name: "skip", - policy: "file://testdata/policy_result_format.yaml", - material: "{\"invalid\": \"1.4\"}", - expectSkipped: true, - expectReasons: []string{"invalid input"}, - expectedApplicable: true, + name: "skip", + policy: "file://testdata/policy_result_format.yaml", + material: "{\"invalid\": \"1.4\"}", + expectSkipped: true, + expectReasons: []string{"invalid input"}, + expectIgnore: false, }, { - name: "skip multiple", - policy: "file://testdata/policy_result_skipped.yaml", - material: "{}", - expectSkipped: true, - expectReasons: []string{"this one is skipped", "this is also skipped"}, - expectedApplicable: true, + name: "skip multiple", + policy: "file://testdata/policy_result_skipped.yaml", + material: "{}", + expectSkipped: true, + expectReasons: []string{"this one is skipped", "this is also skipped"}, + expectIgnore: false, }, { - name: "not applicable", - policy: "file://testdata/policy_not_applicable.yaml", - material: "{\"specVersion\": \"1.0\"}", - expectedApplicable: false, + name: "ignore", + policy: "file://testdata/policy_with_ignore.yaml", + material: "{\"specVersion\": \"1.0\"}", + expectIgnore: true, }, } @@ -1024,7 +1024,7 @@ func (s *testSuite) TestNewResultFormat() { return } - if !tc.expectedApplicable { + if tc.expectIgnore { s.Nil(err) s.Equal([]*v1.PolicyEvaluation{}, res) return @@ -1105,35 +1105,35 @@ func (s *testSuite) TestContainerMaterial() { } } -func (s *testSuite) TestMultiKindApplicable() { +func (s *testSuite) TestMultiKindAWithIgnore() { cases := []struct { - name string - policy string - material string - expectErr bool - expectedEvaluations int - expectSkipped bool - expectReasons []string - expectedNotApplicable bool + name string + policy string + material string + expectErr bool + expectedEvaluations int + expectSkipped bool + expectReasons []string + expecteIgnore bool }{ { - name: "scripts applicable and skipped", - policy: "file://testdata/policy_multi_kind_applicable.yaml", + name: "scripts should not be ignored and skipped", + policy: "file://testdata/policy_multi_kind_with_ignore.yaml", material: "{\"specVersion\": \"1.4\"}", expectedEvaluations: 1, expectSkipped: true, expectReasons: []string{"this on is skipped"}, }, { - name: "scripts not applicable", - policy: "file://testdata/policy_multi_kind_applicable.yaml", - material: "{\"specVersion\": \"1.0\"}", - expectedEvaluations: 0, - expectedNotApplicable: true, + name: "scripts should be ignored", + policy: "file://testdata/policy_multi_kind_with_ignore.yaml", + material: "{\"specVersion\": \"1.0\"}", + expectedEvaluations: 0, + expecteIgnore: true, }, { - name: "all scripts applicable", - policy: "file://testdata/policy_multi_kind_applicable.yaml", + name: "all scripts should not be ignored", + policy: "file://testdata/policy_multi_kind_with_ignore.yaml", material: "{\"specVersion\": \"1.4\"}", expectedEvaluations: 1, expectSkipped: false, @@ -1166,11 +1166,11 @@ func (s *testSuite) TestMultiKindApplicable() { InlineCas: true, } - if tc.expectedNotApplicable { + if tc.expecteIgnore { material.MaterialType = v12.CraftingSchema_Material_SARIF } - if tc.name == "all scripts applicable" { + if tc.name == "all scripts should not be ignored" { material.MaterialType = v12.CraftingSchema_Material_OPENVEX } @@ -1182,7 +1182,7 @@ func (s *testSuite) TestMultiKindApplicable() { return } - if tc.expectedNotApplicable { + if tc.expecteIgnore { s.Nil(err) s.Len(res, tc.expectedEvaluations) return diff --git a/pkg/policies/policy_groups_test.go b/pkg/policies/policy_groups_test.go index 7e221df96..17aaffcde 100644 --- a/pkg/policies/policy_groups_test.go +++ b/pkg/policies/policy_groups_test.go @@ -272,28 +272,28 @@ func (s *groupsTestSuite) TestVerifyStatement() { func (s *groupsTestSuite) TestVerifyMaterialMultiKind() { cases := []struct { - name string - policyGroup string - material string - expectErr bool - expectedEvaluations int - expectSkipped bool - expectReasons []string - expectedNotApplicable bool + name string + policyGroup string + material string + expectErr bool + expectedEvaluations int + expectSkipped bool + expectReasons []string + expectIgnore bool }{ { - name: "not evaluation results, not applicable", - policyGroup: "file://testdata/policy_group_multikind.yaml", - material: "{\"specVersion\": \"1.0\"}", - expectedEvaluations: 0, - expectedNotApplicable: true, + name: "not evaluation results, ignore", + policyGroup: "file://testdata/policy_group_multikind.yaml", + material: "{\"specVersion\": \"1.0\"}", + expectedEvaluations: 0, + expectIgnore: true, }, { - name: "evaluation results, applicable", - policyGroup: "file://testdata/policy_group_multikind.yaml", - material: "{\"specVersion\": \"1.4\"}", - expectedEvaluations: 1, - expectedNotApplicable: false, + name: "evaluation results, no ignore", + policyGroup: "file://testdata/policy_group_multikind.yaml", + material: "{\"specVersion\": \"1.4\"}", + expectedEvaluations: 1, + expectIgnore: false, }, } @@ -321,7 +321,7 @@ func (s *groupsTestSuite) TestVerifyMaterialMultiKind() { InlineCas: true, } - if !tc.expectedNotApplicable { + if !tc.expectIgnore { material.MaterialType = v1.CraftingSchema_Material_OPENVEX } @@ -333,7 +333,7 @@ func (s *groupsTestSuite) TestVerifyMaterialMultiKind() { return } - if tc.expectedNotApplicable { + if tc.expectIgnore { s.Nil(err) s.Len(res, tc.expectedEvaluations) return diff --git a/pkg/policies/testdata/policy_group_multikind.yaml b/pkg/policies/testdata/policy_group_multikind.yaml index 0c374459a..ea828e1ac 100644 --- a/pkg/policies/testdata/policy_group_multikind.yaml +++ b/pkg/policies/testdata/policy_group_multikind.yaml @@ -10,7 +10,7 @@ spec: materials: - type: SBOM_CYCLONEDX_JSON policies: - - ref: file://testdata/policy_not_applicable.yaml + - ref: file://testdata/policy_with_ignore.yaml - type: OPENVEX policies: - - ref: file://testdata/policy_openvex_applicable.yaml + - ref: file://testdata/policy_openvex_no_ignore.yaml diff --git a/pkg/policies/testdata/policy_multi_kind_applicable.yaml b/pkg/policies/testdata/policy_multi_kind_with_ignore.yaml similarity index 80% rename from pkg/policies/testdata/policy_multi_kind_applicable.yaml rename to pkg/policies/testdata/policy_multi_kind_with_ignore.yaml index 5889d6683..9bdc95b28 100644 --- a/pkg/policies/testdata/policy_multi_kind_applicable.yaml +++ b/pkg/policies/testdata/policy_multi_kind_with_ignore.yaml @@ -1,7 +1,7 @@ apiVersion: workflowcontract.chainloop.dev/v1 kind: Policy metadata: - name: multikindapplicable + name: multikindignore description: multikind policy annotations: category: SBOM @@ -16,8 +16,8 @@ spec: result := { "skipped": true, "violations": [], - "skip_reason": "this one is not applicable", - "applicable": false, + "skip_reason": "this one should be ignored", + "ignore": true, } - kind: SBOM_CYCLONEDX_JSON embedded: | @@ -28,8 +28,8 @@ spec: result := { "skipped": true, "violations": [], - "skip_reason": "this one is not applicable", - "applicable": false, + "skip_reason": "this one should be ignored", + "ignore": true, } - kind: SBOM_CYCLONEDX_JSON embedded: | @@ -41,7 +41,7 @@ spec: "skipped": true, "violations": [], "skip_reason": "this on is skipped", - "applicable": true, + "ignore": false, } - kind: OPENVEX embedded: | @@ -53,7 +53,7 @@ spec: "skipped": false, "violations": [], "skip_reason": "", - "applicable": true, + "ignore": false, } - kind: OPENVEX embedded: | @@ -65,5 +65,5 @@ spec: "skipped": false, "violations": [], "skip_reason": "", - "applicable": true, + "ignore": false, } diff --git a/pkg/policies/testdata/policy_openvex_applicable.yaml b/pkg/policies/testdata/policy_openvex_no_ignore.yaml similarity index 86% rename from pkg/policies/testdata/policy_openvex_applicable.yaml rename to pkg/policies/testdata/policy_openvex_no_ignore.yaml index 8f0f1012f..c5eb0faf4 100644 --- a/pkg/policies/testdata/policy_openvex_applicable.yaml +++ b/pkg/policies/testdata/policy_openvex_no_ignore.yaml @@ -1,7 +1,7 @@ apiVersion: workflowcontract.chainloop.dev/v1 kind: Policy metadata: - name: multikindapplicable + name: multikindignore description: multikind policy annotations: category: SBOM @@ -17,7 +17,7 @@ spec: "skipped": false, "violations": [], "skip_reason": "", - "applicable": true, + "ignore": false, } - kind: OPENVEX embedded: | @@ -29,5 +29,5 @@ spec: "skipped": false, "violations": [], "skip_reason": "", - "applicable": true, + "ignore": false, } diff --git a/pkg/policies/testdata/policy_not_applicable.yaml b/pkg/policies/testdata/policy_with_ignore.yaml similarity index 90% rename from pkg/policies/testdata/policy_not_applicable.yaml rename to pkg/policies/testdata/policy_with_ignore.yaml index d8e22318c..9942fa993 100644 --- a/pkg/policies/testdata/policy_not_applicable.yaml +++ b/pkg/policies/testdata/policy_with_ignore.yaml @@ -17,7 +17,7 @@ spec: "skipped": skipped, "violations": violations, "skip_reason": skip_reason, - "applicable": applicable, + "ignore": ignore, } default skip_reason := "" @@ -28,11 +28,11 @@ spec: } default skipped := true - default applicable := true + default ignore := false skipped := false if valid_input - applicable := false if { + ignore := true if { input.specVersion == "1.0" } From c538cac2ad569ff8369eeb149bf0aa7c53ba2c8f Mon Sep 17 00:00:00 2001 From: Javier Rodriguez Date: Tue, 1 Apr 2025 12:53:08 +0200 Subject: [PATCH 4/4] refactor Signed-off-by: Javier Rodriguez --- pkg/policies/engine/rego/rego_test.go | 2 +- pkg/policies/policies.go | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/pkg/policies/engine/rego/rego_test.go b/pkg/policies/engine/rego/rego_test.go index 453e67f71..948db9037 100644 --- a/pkg/policies/engine/rego/rego_test.go +++ b/pkg/policies/engine/rego/rego_test.go @@ -223,7 +223,7 @@ func TestRego_ResultFormat(t *testing.T) { }) } -func TestRego_ResultFormatWithoutApplicability(t *testing.T) { +func TestRego_ResultFormatWithoutIgnoreValue(t *testing.T) { regoContent, err := os.ReadFile("testfiles/result_format_without_ignore.rego") require.NoError(t, err) diff --git a/pkg/policies/policies.go b/pkg/policies/policies.go index 07d0240e0..22f091277 100644 --- a/pkg/policies/policies.go +++ b/pkg/policies/policies.go @@ -151,8 +151,7 @@ func (pv *PolicyVerifier) evaluatePolicyAttachment(ctx context.Context, attachme return nil, NewPolicyError(err) } - // Skip if the script is telling us to specifically to ignore it, it basically skips adding the script to the - // evaluation results + // Skip if the script explicitly instructs us to ignore it, effectively preventing it from being added to the evaluation results if r.Ignore { continue }