Fix Rule Evaluation Logic for Handling Multiple Rules of the Same Typ…

…e - Part 1 (No Data Backfilling) * Rule Name is a user-defined field for differentiating same rule types under an entity * Existing rules in DB would have rule_name as null and would be processed without a rule name * For creating/updating profiles, rule name would be mandatory to avoid collisions Signed-off-by: Vyom-Yadav <[email protected]>
mindersec · Jan 30, 2024 · c7e67ce · c7e67ce
1 parent e872e40
commit c7e67ce
Show file tree

Hide file tree

Showing 19 changed files with 1,884 additions and 973 deletions.
diff --git a/cmd/cli/app/profile/status/status_list.go b/cmd/cli/app/profile/status/status_list.go
@@ -46,7 +46,8 @@ func listCommand(ctx context.Context, cmd *cobra.Command, conn *grpc.ClientConn)
 	profileName := viper.GetString("name")
 	format := viper.GetString("output")
 	all := viper.GetBool("detailed")
-	rule := viper.GetString("rule")
+	ruleType := viper.GetString("ruleType")
+	ruleName := viper.GetString("ruleName")
 
 	// Ensure provider is supported
 	if !app.IsProviderSupported(provider) {
@@ -59,10 +60,11 @@ func listCommand(ctx context.Context, cmd *cobra.Command, conn *grpc.ClientConn)
 	}
 
 	resp, err := client.GetProfileStatusByName(ctx, &minderv1.GetProfileStatusByNameRequest{
-		Context: &minderv1.Context{Provider: &provider, Project: &project},
-		Name:    profileName,
-		All:     all,
-		Rule:    rule,
+		Context:  &minderv1.Context{Provider: &provider, Project: &project},
+		Name:     profileName,
+		All:      all,
+		RuleType: ruleType,
+		RuleName: ruleName,
 	})
 	if err != nil {
 		return cli.MessageAndError("Error getting profile status", err)
@@ -98,5 +100,6 @@ func init() {
 	profileStatusCmd.AddCommand(listCmd)
 	// Flags
 	listCmd.Flags().BoolP("detailed", "d", false, "List all profile violations")
-	listCmd.Flags().StringP("rule", "r", "", "Filter profile status list by rule")
+	listCmd.Flags().StringP("ruleType", "r", "", "Filter profile status list by rule type")
+	listCmd.Flags().String("ruleName", "", "Filter profile status list by rule name")
 }
diff --git a/cmd/cli/app/profile/table_render.go b/cmd/cli/app/profile/table_render.go
@@ -156,9 +156,17 @@ func RenderRuleEvaluationStatusTable(
 	t table.Table,
 ) {
 	for _, eval := range statuses {
+		ruleName := eval.RuleDescriptionName
+
+		// TODO: Remove this after migration, ruleName would be valid after updating existing evaluations (#1609)
+		//nolint:staticcheck // ignore SA1019: Deprecated field supported for backward compatibility
+		if ruleName == "" {
+			ruleName = eval.RuleName
+		}
+
 		t.AddRowWithColor(
 			layouts.NoColor(eval.RuleId),
-			layouts.NoColor(eval.RuleName),
+			layouts.NoColor(ruleName),
 			layouts.NoColor(eval.Entity),
 			getColoredEvalStatus(eval.Status),
 			getRemediateStatusColor(eval.RemediationStatus),

diff --git a/database/migrations/000013_rule_evaluations_rule_name.down.sql b/database/migrations/000013_rule_evaluations_rule_name.down.sql
@@ -0,0 +1,80 @@
+-- Copyright 2024 Stacklok, Inc
+--
+-- Licensed under the Apache License, Version 2.0 (the "License");
+-- you may not use this file except in compliance with the License.
+-- You may obtain a copy of the License at
+--
+--      http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+
+-- Begin transaction
+BEGIN;
+
+-- Prevent concurrent updates to rule_evaluations
+SELECT *
+FROM rule_evaluations FOR UPDATE;
+
+-- Delete duplicate rule evaluation results without considering rule_name
+-- Using CTID as postgres doesn't have min, max aggregators for uuid (too much code to add one)
+DELETE
+FROM rule_evaluations
+WHERE CTID IN (SELECT MIN(CTID) AS CTID
+               FROM rule_evaluations
+               GROUP BY entity, profile_id, repository_id, rule_type_id,
+                        COALESCE(pull_request_id, '00000000-0000-0000-0000-000000000000'::UUID),
+                        COALESCE(artifact_id, '00000000-0000-0000-0000-000000000000'::UUID)
+               HAVING COUNT(*) > 1);
+
+
+-- Drop the existing unique index on rule_evaluations
+DROP INDEX IF EXISTS rule_evaluations_results_idx;
+
+-- Recreate the unique index without rule_name
+CREATE UNIQUE INDEX rule_evaluations_results_idx
+  ON rule_evaluations (profile_id, repository_id, COALESCE(artifact_id, '00000000-0000-0000-0000-000000000000'::UUID),
+                       entity, rule_type_id, COALESCE(pull_request_id, '00000000-0000-0000-0000-000000000000'::UUID));
+
+-- Remove the rule_name column from rule_evaluations
+ALTER TABLE rule_evaluations
+  DROP COLUMN IF EXISTS rule_name;
+
+-- Function to remove the "name" field from a JSONB array. Function is immutable as it will return same results
+-- for same input arguments forever.
+CREATE OR REPLACE FUNCTION remove_name_from_jsonb_array(input_jsonb jsonb) RETURNS jsonb AS
+$$
+DECLARE
+  updated_array jsonb;
+  element       jsonb;
+BEGIN
+  updated_array := '[]'::jsonb;
+
+  FOR element IN SELECT * FROM jsonb_array_elements(input_jsonb)
+    LOOP
+      element := element - 'name';
+      updated_array := updated_array || jsonb_build_array(element);
+    END LOOP;
+
+  RETURN updated_array;
+END;
+$$ LANGUAGE plpgsql IMMUTABLE;
+
+-- Prevent concurrent updates to entity_profiles
+SELECT *
+FROM entity_profiles FOR UPDATE;
+
+-- Update the entity_profiles table to remove the "name" key from the "contextual_rules" JSONB array
+UPDATE entity_profiles
+SET contextual_rules = remove_name_from_jsonb_array(contextual_rules),
+    updated_at = now()
+WHERE contextual_rules != remove_name_from_jsonb_array(contextual_rules);
+
+-- Drop the created function
+DROP FUNCTION IF EXISTS remove_name_from_jsonb_array(input_jsonb jsonb);
+
+-- Commit transaction
+COMMIT;
diff --git a/database/migrations/000013_rule_evaluations_rule_name.up.sql b/database/migrations/000013_rule_evaluations_rule_name.up.sql
@@ -0,0 +1,32 @@
+-- Copyright 2024 Stacklok, Inc
+--
+-- Licensed under the Apache License, Version 2.0 (the "License");
+-- you may not use this file except in compliance with the License.
+-- You may obtain a copy of the License at
+--
+--      http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+
+-- Begin transaction
+BEGIN;
+
+-- Add rule_name to rule_evaluations
+ALTER TABLE rule_evaluations
+  ADD COLUMN rule_name TEXT;
+
+-- Drop the existing unique index on rule_evaluations
+DROP INDEX IF EXISTS rule_evaluations_results_idx;
+
+-- Recreate the unique index with rule_name
+CREATE UNIQUE INDEX rule_evaluations_results_idx
+  ON rule_evaluations (profile_id, repository_id, COALESCE(artifact_id, '00000000-0000-0000-0000-000000000000'::UUID),
+                       entity, rule_type_id, COALESCE(pull_request_id, '00000000-0000-0000-0000-000000000000'::UUID),
+                       rule_name) NULLS NOT DISTINCT;
+
+-- transaction commit
+COMMIT;
diff --git a/database/mock/store.go b/database/mock/store.go
diff --git a/database/query/profile_status.sql b/database/query/profile_status.sql
@@ -1,9 +1,9 @@
 
 -- name: UpsertRuleEvaluations :one
 INSERT INTO rule_evaluations (
-    profile_id, repository_id, artifact_id, pull_request_id, rule_type_id, entity
-) VALUES ($1, $2, $3, $4, $5, $6)
-ON CONFLICT (profile_id, repository_id, COALESCE(artifact_id, '00000000-0000-0000-0000-000000000000'::UUID), COALESCE(pull_request_id, '00000000-0000-0000-0000-000000000000'::UUID), entity, rule_type_id)
+    profile_id, repository_id, artifact_id, pull_request_id, rule_type_id, entity, rule_name
+) VALUES ($1, $2, $3, $4, $5, $6, sqlc.narg(rule_name))
+ON CONFLICT (profile_id, repository_id, COALESCE(artifact_id, '00000000-0000-0000-0000-000000000000'::UUID), COALESCE(pull_request_id, '00000000-0000-0000-0000-000000000000'::UUID), entity, rule_type_id, rule_name)
   DO UPDATE SET profile_id = $1
 RETURNING id;
 
@@ -79,7 +79,8 @@ WHERE p.project_id = $1;
 DELETE FROM rule_evaluations
 WHERE id IN (
     SELECT id FROM rule_evaluations as re
-    WHERE re.profile_id = $1 AND re.rule_type_id = $2 FOR UPDATE);
+    WHERE re.profile_id = $1 AND re.rule_type_id = $2 AND
+          (re.rule_name = sqlc.narg(rule_name) OR sqlc.narg(rule_name) IS NULL) FOR UPDATE);
 
 -- name: ListRuleEvaluationsByProfileId :many
 WITH
@@ -122,6 +123,7 @@ SELECT
     ad.alert_last_updated,
     res.repository_id,
     res.entity,
+    COALESCE(res.rule_name, ''),
     repo.repo_name,
     repo.repo_owner,
     repo.provider,
@@ -143,5 +145,6 @@ WHERE res.profile_id = $1 AND
             WHEN sqlc.narg(entity_id)::UUID IS NULL THEN true
             ELSE false
             END
-        ) AND (rt.name = sqlc.narg(rule_name) OR sqlc.narg(rule_name) IS NULL)
+        ) AND (rt.name = sqlc.narg(rule_type_name) OR sqlc.narg(rule_type_name) IS NULL)
+          AND (res.rule_name = sqlc.narg(rule_name) OR sqlc.narg(rule_name) IS NULL)
 ;
diff --git a/docs/docs/how-to/create_profile.md b/docs/docs/how-to/create_profile.md
@@ -230,6 +230,8 @@ alert: "on"
 remediate: "on"
 repository:
   - type: secret_scanning
+    name: "secret_scanning_github" # Optional, as there aren't multiple rules
+                                   # of the same type under the entity - repository
     def:
       enabled: true
 ```
@@ -250,4 +252,86 @@ filter the output using `jq`. For example, to get all rules that pertain to the
 run the following command:
 ```bash
 minder profile status list --name stacklok-remediate-profile -d -ojson 2>/dev/null | jq  -C '.ruleEvaluationStatus | map(select(.entityInfo.repo_name == "minder" and .status == "failure"))'
-```
+```
+
+## Defining Rule Names in Profiles
+
+In Minder profiles, rules are identified by their type and, optionally, a unique name.
+
+### Rule Types vs Rule Names
+
+Rule types are mandatory and refer to the kind of rule being applied. Rule names, on the other hand, are optional 
+identifiers that become crucial when multiple rules of the same type exist under an entity.
+
+```yaml
+repository:
+  - type: secret_scanning
+    name: "secret_scanning_github"
+    def:
+      enabled: true
+```
+In this example, `secret_scanning` is the rule type and `secret_scanning_github` is the rule name.
+
+### When are Rule Names Mandatory?
+
+If you're using multiple rules of the same type under an entity, each rule must have a unique name. This helps 
+distinguish between rules and understand their specific purpose.
+
+```yaml
+repository:
+  - type: secret_scanning
+    name: "secret_scanning_github"
+    def:
+      enabled: true
+  - type: secret_scanning
+    name: "secret_scanning_github_2"
+    def:
+      enabled: false
+```
+Here, we have two rules of the same type `secret_scanning` under the `repository` entity. Each rule has a unique name.
+
+### Uniqueness of Rule Names
+
+No two rules, whether of the same type or different types, can have the same name under an entity. This avoids 
+confusion and ensures each rule can be individually managed.
+
+```yaml
+repository: # Would return an error while creating
+  - type: secret_scanning
+    name: "protect_github"
+    def:
+      enabled: true
+  - type: secret_push_protection
+    name: "protect_github"
+    def:
+      enabled: false
+```
+In this example, even though the rules are of different types (`secret_scanning` and `secret_push_protection`), 
+Minder will return an error while creating this profile as rule names are same under the same entity. 
+You may use same rule names under different entities (repository, artifacts, etc.)
+
+Also, a rule's `type` and `name` should not be identical.
+
+### Example
+
+Consider a profile with two `dependabot_configured` rules under the `repository` entity. The first rule has a unique 
+name, "Dependabot Configured for GoLang". The second rule doesn't have a name, which is acceptable as Minder would 
+internally use the rule type as the name for the rule.
+
+```yaml
+repository:
+  - type: dependabot_configured
+    name: "Dependabot Configured for GoLang"
+    def:
+      package_ecosystem: gomod
+      schedule_interval: daily
+      apply_if_file: go.mod
+  - type: dependabot_configured # internally 'name' would be 'dependabot_configured'
+    def:
+      package_ecosystem: npm
+      schedule_interval: daily
+      apply_if_file: docs/package.json
+```
+
+You can find the rule definitions used above and many profile examples at 
+[minder-rules-and-profiles](https://github.com/stacklok/minder-rules-and-profiles) repository.
diff --git a/docs/docs/ref/proto.md b/docs/docs/ref/proto.md