Fix Rule Evaluation Logic for Handling Multiple Rules of the Same Type

* Introduced a new 'name' field for rules, which is mandatory when having different rules of the same type under one entity * Rule type collisions are handled by unique names, existing rules in DB are modified to have name_random-uuid format * Existing names can be overridden by peforming a `profile apply` to use sensible rule names * If no rule name is specified, and the profile is compliant i.e. rule type not repeated, rule-type is used as the default rule name Signed-off-by: Vyom-Yadav <[email protected]>
mindersec · Jan 22, 2024 · fa60455 · fa60455
1 parent d24f543
commit fa60455
Show file tree

Hide file tree

Showing 19 changed files with 1,452 additions and 550 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
@@ -158,7 +158,7 @@ func RenderRuleEvaluationStatusTable(
 	for _, eval := range statuses {
 		t.AddRowWithColor(
 			layouts.NoColor(eval.RuleId),
-			layouts.NoColor(eval.RuleName),
+			layouts.NoColor(eval.RuleDescriptionName),
 			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,79 @@
+-- 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
+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;
+
+-- 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 true;
+
+-- 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,111 @@
+-- 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);
+
+-- Function to add a descriptive name to each element in a JSONB array
+CREATE OR REPLACE FUNCTION add_descriptive_name_to_jsonb_array(input_jsonb jsonb) RETURNS jsonb AS
+$$
+DECLARE
+  updated_array jsonb;
+  element       jsonb;
+  element_type  text;
+BEGIN
+  updated_array := '[]'::jsonb;
+
+  FOR element IN SELECT * FROM jsonb_array_elements(input_jsonb)
+    LOOP
+      element_type := element ->> 'type';
+
+      IF (SELECT COUNT(*) FROM jsonb_array_elements(input_jsonb) WHERE value ->> 'type' = element_type) > 1 THEN
+        element := jsonb_set(element, '{name}', ('"' || element_type || '_' || gen_random_uuid() || '"')::jsonb);
+      ELSE
+        element := jsonb_set(element, '{name}', ('"' || element_type || '"')::jsonb);
+      END IF;
+
+      updated_array := updated_array || jsonb_build_array(element);
+    END LOOP;
+
+  RETURN updated_array;
+END;
+$$ LANGUAGE plpgsql;
+
+-- Function to get the rule name for a given entity, profile and rule id.
+CREATE OR REPLACE FUNCTION get_rule_name(entity entities, profile_id uuid, rule_type_id uuid) RETURNS text AS
+$$
+DECLARE
+  rule_name      text;
+  rule_type_name text;
+  rules          jsonb;
+BEGIN
+  SELECT entity_profiles.contextual_rules
+  INTO rules
+  FROM entity_profiles
+  WHERE entity_profiles.profile_id = get_rule_name.profile_id
+    AND entity_profiles.entity = get_rule_name.entity;
+
+  SELECT rule_type.name INTO rule_type_name FROM rule_type WHERE id = get_rule_name.rule_type_id;
+
+  SELECT rule_element ->> 'name'
+  INTO rule_name
+  FROM jsonb_array_elements(rules) rule_element
+  WHERE rule_element ->> 'type' = rule_type_name;
+
+  RETURN rule_name;
+END;
+$$ LANGUAGE plpgsql STABLE;
+
+-- Prevent entity_profiles to be updated outside the transaction
+SELECT *
+FROM entity_profiles FOR UPDATE;
+
+-- Update existing rules to have the name field, internally this field is mandatory
+UPDATE entity_profiles
+SET contextual_rules = add_descriptive_name_to_jsonb_array(entity_profiles.contextual_rules),
+    updated_at = now()
+WHERE true;
+
+-- Prevent rule_evaluations to be updated outside the transaction
+SELECT *
+FROM rule_evaluations FOR UPDATE;
+
+-- Update rule evaluations
+UPDATE rule_evaluations
+SET rule_name = get_rule_name(rule_evaluations.entity, rule_evaluations.profile_id, rule_evaluations.rule_type_id)
+WHERE true;
+
+-- Add non null constraint on rule_name
+ALTER TABLE rule_evaluations
+  ALTER COLUMN rule_name SET NOT NULL;
+
+-- Drop the created functions
+DROP FUNCTION IF EXISTS add_descriptive_name_to_jsonb_array(input_jsonb jsonb);
+DROP FUNCTION IF EXISTS get_rule_name(entity entities, profile_id uuid, rule_type_id uuid);
+
+-- 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, $7)
+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,7 @@ 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 = $3 FOR UPDATE);
 
 -- name: ListRuleEvaluationsByProfileId :many
 WITH
@@ -122,6 +122,7 @@ SELECT
     ad.alert_last_updated,
     res.repository_id,
     res.entity,
+    res.rule_name,
     repo.repo_name,
     repo.repo_owner,
     repo.provider,
@@ -143,5 +144,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.