From 37668e8d9115a8701d8a5989804eabd1dc594191 Mon Sep 17 00:00:00 2001
From: Alex Pana <8968914+acpana@users.noreply.github.com>
Date: Thu, 12 Jan 2023 00:05:29 +0000
Subject: [PATCH] feat: enforce kind on admisisno review

Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
---
 pkg/gator/errors.go             |  2 ++
 pkg/gator/fixtures/fixtures.go  | 41 +++++++++++++++++++++++++++++++++
 pkg/gator/verify/runner.go      | 14 +++++++++++
 pkg/gator/verify/runner_test.go | 33 ++++++++++++++++++++++++++
 4 files changed, 90 insertions(+)

diff --git a/pkg/gator/errors.go b/pkg/gator/errors.go
index 2595970a8d2..101fa6958b4 100644
--- a/pkg/gator/errors.go
+++ b/pkg/gator/errors.go
@@ -56,4 +56,6 @@ var (
 	ErrNilOldObject = errors.New("oldObject is nil")
 	// ErrInvalidYAML indicates that a .yaml/.yml file was not parseable.
 	ErrInvalidYAML = errors.New("invalid yaml")
+	// ErrUnmarshallObject happens when the yaml defines an invalid object or oldObject.
+	ErrUnmarshallObject = errors.New("object or oldObject cannot be unmarshalled")
 )
diff --git a/pkg/gator/fixtures/fixtures.go b/pkg/gator/fixtures/fixtures.go
index 5c57c5b21ac..9a4a0b7e4d0 100644
--- a/pkg/gator/fixtures/fixtures.go
+++ b/pkg/gator/fixtures/fixtures.go
@@ -489,6 +489,7 @@ kind: AdmissionReview
 apiVersion: admission.k8s.io/v1beta1
 request:
   oldObject:
+    kind: Pod
     labels: 
       - app: "bar"
 `
@@ -500,6 +501,21 @@ apiVersion: admission.k8s.io/v1beta1
 request:
   operation: "DELETE"
   object:
+    kind: Pod
+    labels:
+      - app: "bar"
+`
+
+	DeleteAdmissionReviewWithOldObjectMissingKind = `
+kind: AdmissionReview
+apiVersion: admission.k8s.io/v1beta1
+request:
+  operation: "DELETE"
+  object:
+    kind: Pod
+    labels:
+      - app: "bar"
+  oldObject:
     labels:
       - app: "bar"
 `
@@ -507,6 +523,18 @@ request:
 	SystemAdmissionReview = `
 kind: AdmissionReview
 apiVersion: admission.k8s.io/v1beta1
+request:
+  userInfo:
+    username: "system:foo"
+  object:
+    kind: Pod
+    labels:
+      - app: "bar"
+`
+
+	SystemAdmissionReviewMissingKind = `
+kind: AdmissionReview
+apiVersion: admission.k8s.io/v1beta1
 request:
   userInfo:
     username: "system:foo"
@@ -523,6 +551,7 @@ request:
   userInfo:
     username: "foo"
   object:
+    kind: Pod
     labels: 
       - app: "bar"
 `
@@ -541,4 +570,16 @@ apiVersion: constraints.gatekeeper.sh/v1beta1
 metadata:
   name: always-pass
 `
+
+	ConstraintAlwaysValidateUserInfoWithMatch = `
+kind: ValidateUserInfo
+apiVersion: constraints.gatekeeper.sh/v1beta1
+metadata:
+  name: always-pass
+spec:
+  match:
+    kinds:
+      - apiGroups: ["*"]
+        kinds: ["*"]
+`
 )
diff --git a/pkg/gator/verify/runner.go b/pkg/gator/verify/runner.go
index 099f0e0c751..3a240adbcdd 100644
--- a/pkg/gator/verify/runner.go
+++ b/pkg/gator/verify/runner.go
@@ -336,6 +336,20 @@ func (r *Runner) validateAndReviewAdmissionReviewRequest(ctx context.Context, c
 		return nil, fmt.Errorf("%w: AdmissionRequest does not contain an \"object\" or \"oldObject\" to review", gator.ErrNoObjectForReview)
 	}
 
+	// make sure that kind is set for object, oldObject if present since we expect it on Decode
+	// https://github.com/kubernetes/apimachinery/blob/27a96d86e70e9bbd40639a0539a14423f55afa07/pkg/apis/meta/v1/unstructured/helpers.go#L341
+	obj := &unstructured.Unstructured{}
+	if ar.Request.Object.Raw != nil {
+		if err := obj.UnmarshalJSON(ar.Request.Object.Raw); err != nil {
+			return nil, fmt.Errorf("%w: %v", gator.ErrUnmarshallObject, err.Error())
+		}
+	}
+	if ar.Request.OldObject.Raw != nil {
+		if err := obj.UnmarshalJSON(ar.Request.OldObject.Raw); err != nil {
+			return nil, fmt.Errorf("%w: %v", gator.ErrUnmarshallObject, err.Error())
+		}
+	}
+
 	// parse into webhook/admission type
 	req := &admission.Request{AdmissionRequest: *ar.Request}
 	if err := util.SetObjectOnDelete(req); err != nil {
diff --git a/pkg/gator/verify/runner_test.go b/pkg/gator/verify/runner_test.go
index 89cf3d57c73..c2d232660ad 100644
--- a/pkg/gator/verify/runner_test.go
+++ b/pkg/gator/verify/runner_test.go
@@ -1077,6 +1077,23 @@ func TestRunner_Run(t *testing.T) {
 							},
 						},
 					},
+					{
+						Name:       "invalid admission review request usage", // this test covers error handling for invalid admission review objects
+						Template:   "template.yaml",
+						Constraint: "constraint-with-match.yaml",
+						Cases: []*Case{
+							{
+								Name:       "no kind on object", // this is an AdmissionRequest w a DELETE operation but no oldObject provided
+								Object:     "no-kind-object-ar.yaml",
+								Assertions: []Assertion{{}},
+							},
+							{
+								Name:       "no kind on old object", // this is an AdmissionRequest w a DELETE operation but no oldObject provided
+								Object:     "no-kind-oldObject-ar.yaml",
+								Assertions: []Assertion{{}},
+							},
+						},
+					},
 				},
 			},
 			f: fstest.MapFS{
@@ -1086,6 +1103,9 @@ func TestRunner_Run(t *testing.T) {
 				"constraint.yaml": &fstest.MapFile{
 					Data: []byte(fixtures.ConstraintAlwaysValidateUserInfo),
 				},
+				"constraint-with-match.yaml": &fstest.MapFile{
+					Data: []byte(fixtures.ConstraintAlwaysValidateUserInfoWithMatch),
+				},
 				"no-objects-ar.yaml": &fstest.MapFile{
 					Data: []byte(fixtures.AdmissionReviewMissingObjectAndOldObject),
 				},
@@ -1107,6 +1127,12 @@ func TestRunner_Run(t *testing.T) {
 				"no-oldObject-ar.yaml": &fstest.MapFile{
 					Data: []byte(fixtures.DeleteAdmissionReviewWithNoOldObject),
 				},
+				"no-kind-object-ar.yaml": &fstest.MapFile{
+					Data: []byte(fixtures.SystemAdmissionReviewMissingKind),
+				},
+				"no-kind-oldObject-ar.yaml": &fstest.MapFile{
+					Data: []byte(fixtures.DeleteAdmissionReviewWithOldObjectMissingKind),
+				},
 			},
 			want: SuiteResult{
 				TestResults: []TestResult{
@@ -1132,6 +1158,13 @@ func TestRunner_Run(t *testing.T) {
 							{Name: "no oldObject on delete", Error: gator.ErrNilOldObject},
 						},
 					},
+					{
+						Name: "invalid admission review request usage",
+						CaseResults: []CaseResult{
+							{Name: "no kind on object", Error: gator.ErrUnmarshallObject},
+							{Name: "no kind on old object", Error: gator.ErrUnmarshallObject},
+						},
+					},
 				},
 			},
 		},