- verify allowedTo field exist before accessing it

alcideio · Jan 27, 2022 · 880d95f · 880d95f
1 parent 7c64cc6
commit 880d95f
Show file tree

Hide file tree

Showing 2 changed files with 46 additions and 9 deletions.
diff --git a/pkg/analysis/default-rules.yaml b/pkg/analysis/default-rules.yaml
@@ -40,7 +40,7 @@ Rules:
     # In the expression when evaluating resources - use plural form (secrets not secret)
     AnalysisExpr: |
         subjects.filter(
-            subject, subject.allowedTo.exists(
+            subject, has(subject.allowedTo) && subject.allowedTo.exists(
               rule,
               (has(rule.verb)     && rule.verb in ['get', '*'])         && (has(rule.resource)
               && rule.resource in ['secrets', '*']) &&                         (has(rule.apiGroup)
@@ -71,7 +71,7 @@ Rules:
     # In the expression when evaluating rule.resource - use plural form (secrets not secret)
     AnalysisExpr: |
       subjects.filter(
-        subject, subject.allowedTo.exists(
+        subject, has(subject.allowedTo) && subject.allowedTo.exists(
           rule,
           (has(rule.verb)
            && rule.verb in ['create', 'update', 'patch', '*']) &&(has(rule.resource)
@@ -97,7 +97,7 @@ Rules:
     # In the expression when evaluating rule.resource - use plural form (secrets not secret)
     AnalysisExpr: |
       subjects.filter(
-        subject, subject.allowedTo.exists(
+        subject, has(subject.allowedTo) && subject.allowedTo.exists(
           rule,
           (has(rule.verb)
            && rule.verb in ['impersonate', '*'])         &&(has(rule.resource)
@@ -125,7 +125,7 @@ Rules:
     # In the expression when evaluating rule.resource - use plural form (secrets not secret)
     AnalysisExpr: |
       subjects.filter(
-          subject, subject.allowedTo.exists(
+          subject, has(subject.allowedTo) && subject.allowedTo.exists(
               rule,
               (has(rule.verb)     && rule.verb in ['bind', 'create', 'update', 'patch', 'escalate', '*']) &&
               (has(rule.resource) && rule.resource in ['clusterroles', 'roles', '*']) &&
@@ -150,7 +150,7 @@ Rules:
     # In the expression when evaluating rule.resource - use plural form (secrets not secret)
     AnalysisExpr: |
       subjects.filter(
-        subject, subject.allowedTo.exists(
+        subject, has(subject.allowedTo) && subject.allowedTo.exists(
           rule,
           (has(rule.verb) && rule.verb in ['create', 'delete', 'update', 'patch', '*']) &&
           (
@@ -186,7 +186,7 @@ Rules:
     # In the expression when evaluating rule.resource - use plural form (secrets not secret)
     AnalysisExpr: |
       subjects.filter(
-        subject, subject.allowedTo.exists(
+        subject, has(subject.allowedTo) && subject.allowedTo.exists(
           rule,
           (has(rule.verb) && rule.verb in ['create', 'delete', 'update', 'patch', '*']) &&
           (
@@ -224,7 +224,7 @@ Rules:
     # In the expression when evaluating rule.resource - use plural form (secrets not secret)
     AnalysisExpr: |
       subjects.filter(
-        subject, subject.allowedTo.exists(
+        subject, has(subject.allowedTo) && subject.allowedTo.exists(
           rule,
           (has(rule.verb) && rule.verb in ['create', 'update', 'patch', '*']) &&
           (
@@ -250,7 +250,7 @@ Rules:
     # In the expression when evaluating rule.resource - use plural form (secrets not secret)
     AnalysisExpr: |
       subjects.filter(
-        subject, subject.allowedTo.exists(
+        subject, has(subject.allowedTo) && subject.allowedTo.exists(
           rule,
           (has(rule.verb) && rule.verb in ['create', 'update', 'patch', 'delete', '*']) &&
           (
@@ -277,7 +277,7 @@ Rules:
     # In the expression when evaluating rule.resource - use plural form (secrets not secret)
     AnalysisExpr: |
       subjects.filter(
-        subject, subject.allowedTo.exists(
+        subject, has(subject.allowedTo) && subject.allowedTo.exists(
           rule,
           (has(rule.verb) && rule.verb in ['create', 'update', 'patch', 'delete', '*']) &&
           (

diff --git a/pkg/analysis/default_rules_test.go b/pkg/analysis/default_rules_test.go
@@ -158,3 +158,40 @@ func Test__RuleExclusion(t *testing.T) {
 
 	//t.Logf("%v", pretty.Sprint(report))
 }
+
+func Test__EmptySubjectPolicyList(t *testing.T) {
+	defer klog.Flush()
+
+	config := DefaultAnalysisConfig()
+
+	analyzer := CreateAnalyzer(
+		config,
+		[]rbac.SubjectPolicyList{
+			{Subject: v1.Subject{
+				Kind:      "ServiceAccount",
+				APIGroup:  "",
+				Name:      "test-sa",
+				Namespace: "test",
+			},
+				AllowedTo: []rbac.NamespacedPolicyRule{},
+			},
+		},
+	)
+
+	if analyzer == nil {
+		t.Fail()
+	}
+
+	report, err := analyzer.Analyze()
+	if err != nil {
+		t.Fatalf("Analysis failed - %v", err)
+		t.Fail()
+	}
+
+	if len(report.Findings) != 0 {
+		t.Fatalf("Expecting no findings")
+		t.Fail()
+	}
+
+	//t.Logf("%v", pretty.Sprint(report))
+}