permissions preflight: add kubernetes compatibility tests, other small fixups

Signed-off-by: Joe Lanford <joe.lanford@gmail.com>

Author: joelanford

internal/operator-controller/authorization/rbac.go

@@ -616,7 +616,7 @@ func parseEscalationErrorForMissingRules(ecError error) (*parseResult, error) {
 func parseCompactRuleString(rule string) (*rbacv1.PolicyRule, error) {
 	var fields []string
 	if ruleText := rule[1 : len(rule)-1]; ruleText != "" {
-		fields = mapSlice(strings.Split(rule[1:len(rule)-1], ","), func(in string) string {
+		fields = mapSlice(strings.Split(ruleText, ","), func(in string) string {
 			return strings.TrimSpace(in)
 		})
 	}

internal/operator-controller/authorization/rbac_test.go

@@ -3,6 +3,7 @@ package authorization
 import (
 	"context"
 	"errors"
+	"fmt"
 	"strings"
 	"testing"
 
@@ -12,12 +13,13 @@ import (
 	"k8s.io/apimachinery/pkg/api/meta/testrestmapper"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
-	featuregatetesting "k8s.io/component-base/featuregate/testing"
+	"k8s.io/apiserver/pkg/authentication/user"
+	"k8s.io/apiserver/pkg/endpoints/request"
+	"k8s.io/kubernetes/pkg/registry/rbac/validation"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
 	ocv1 "github.com/operator-framework/operator-controller/api/v1"
-	"github.com/operator-framework/operator-controller/internal/operator-controller/features"
 )
 
 var (
@@ -226,7 +228,6 @@ func setupFakeClient(role client.Object) client.Client {
 
 func TestPreAuthorize_Success(t *testing.T) {
 	t.Run("preauthorize succeeds with no missing rbac rules", func(t *testing.T) {
-		featuregatetesting.SetFeatureGateDuringTest(t, features.OperatorControllerFeatureGate, features.PreflightPermissions, true)
 		fakeClient := setupFakeClient(privilegedClusterRole)
 		preAuth := NewRBACPreAuthorizer(fakeClient)
 		missingRules, err := preAuth.PreAuthorize(context.TODO(), &exampleClusterExtension, strings.NewReader(testManifest))
@@ -237,7 +238,6 @@ func TestPreAuthorize_Success(t *testing.T) {
 
 func TestPreAuthorize_Failure(t *testing.T) {
 	t.Run("preauthorize fails with missing rbac rules", func(t *testing.T) {
-		featuregatetesting.SetFeatureGateDuringTest(t, features.OperatorControllerFeatureGate, features.PreflightPermissions, true)
 		fakeClient := setupFakeClient(limitedClusterRole)
 		preAuth := NewRBACPreAuthorizer(fakeClient)
 		missingRules, err := preAuth.PreAuthorize(context.TODO(), &exampleClusterExtension, strings.NewReader(testManifest))
@@ -248,7 +248,6 @@ func TestPreAuthorize_Failure(t *testing.T) {
 
 func TestPreAuthorizeMultiNamespace_Failure(t *testing.T) {
 	t.Run("preauthorize fails with missing rbac rules in multiple namespaces", func(t *testing.T) {
-		featuregatetesting.SetFeatureGateDuringTest(t, features.OperatorControllerFeatureGate, features.PreflightPermissions, true)
 		fakeClient := setupFakeClient(limitedClusterRole)
 		preAuth := NewRBACPreAuthorizer(fakeClient)
 		missingRules, err := preAuth.PreAuthorize(context.TODO(), &exampleClusterExtension, strings.NewReader(testManifestMultiNamespace))
@@ -259,7 +258,6 @@ func TestPreAuthorizeMultiNamespace_Failure(t *testing.T) {
 
 func TestPreAuthorize_CheckEscalation(t *testing.T) {
 	t.Run("preauthorize succeeds with no missing rbac rules", func(t *testing.T) {
-		featuregatetesting.SetFeatureGateDuringTest(t, features.OperatorControllerFeatureGate, features.PreflightPermissions, true)
 		fakeClient := setupFakeClient(escalatingClusterRole)
 		preAuth := NewRBACPreAuthorizer(fakeClient)
 		missingRules, err := preAuth.PreAuthorize(context.TODO(), &exampleClusterExtension, strings.NewReader(testManifest))
@@ -270,7 +268,7 @@ func TestPreAuthorize_CheckEscalation(t *testing.T) {
 
 // TestParseEscalationErrorForMissingRules Are tests with respect to https://github.com/kubernetes/api/blob/e8d4d542f6a9a16a694bfc8e3b8cd1557eecfc9d/rbac/v1/types.go#L49-L74
 // Goal is: prove the regex works as planned AND that if the error messages ever change we'll learn about it with these tests
-func TestParseEscalationErrorForMissingRules(t *testing.T) {
+func TestParseEscalationErrorForMissingRules_ParsingLogic(t *testing.T) {
 	testCases := []struct {
 		name           string
 		inputError     error
@@ -279,7 +277,7 @@ func TestParseEscalationErrorForMissingRules(t *testing.T) {
 	}{
 		{
 			name: "One Missing Resource Rule",
-			inputError: errors.New(`user "test-user" (groups=test) is attempting to grant RBAC permissions not currently held:
+			inputError: errors.New(`user "test-user" (groups=["test"]) is attempting to grant RBAC permissions not currently held:
 {APIGroups:["apps"], Resources:["deployments"], Verbs:["get"]}`),
 			expectedResult: &parseResult{
 				MissingRules: []rbacv1.PolicyRule{
@@ -304,32 +302,32 @@ func TestParseEscalationErrorForMissingRules(t *testing.T) {
 		{
 			name: "One Missing Rule with Resolution Errors",
 			inputError: errors.New(`user "test-admin" (groups=["system:masters"]) is attempting to grant RBAC permissions not currently held:
-{APIGroups:["batch"], Resources:["jobs"], Verbs:["create"]}; resolution errors: role "missing-role" not found`),
+{APIGroups:["batch"], Resources:["jobs"], Verbs:["create"]}; resolution errors: [role "missing-role" not found]`),
 			expectedResult: &parseResult{
 				MissingRules: []rbacv1.PolicyRule{
 					{APIGroups: []string{"batch"}, Resources: []string{"jobs"}, Verbs: []string{"create"}},
 				},
-				ResolutionErrors: errors.New(`role "missing-role" not found`),
+				ResolutionErrors: errors.New(`[role "missing-role" not found]`),
 			},
 			expectError: require.NoError,
 		},
 		{
 			name: "Multiple Missing Rules with Resolution Errors",
 			inputError: errors.New(`user "another-user" (groups=[]) is attempting to grant RBAC permissions not currently held:
 {APIGroups:[""], Resources:["secrets"], Verbs:["get"]}
-{APIGroups:[""], Resources:["configmaps"], Verbs:["list"]}; resolution errors: clusterrole "missing-clusterrole" not found, role "other-missing" not found`),
+{APIGroups:[""], Resources:["configmaps"], Verbs:["list"]}; resolution errors: [clusterrole "missing-clusterrole" not found, role "other-missing" not found]`),
 			expectedResult: &parseResult{
 				MissingRules: []rbacv1.PolicyRule{
 					{APIGroups: []string{""}, Resources: []string{"secrets"}, Verbs: []string{"get"}},
 					{APIGroups: []string{""}, Resources: []string{"configmaps"}, Verbs: []string{"list"}},
 				},
-				ResolutionErrors: errors.New(`clusterrole "missing-clusterrole" not found, role "other-missing" not found`),
+				ResolutionErrors: errors.New(`[clusterrole "missing-clusterrole" not found, role "other-missing" not found]`),
 			},
 			expectError: require.NoError,
 		},
 		{
 			name: "Missing Rule (All Resource Fields)",
-			inputError: errors.New(`user "resource-name-user" (groups=test) is attempting to grant RBAC permissions not currently held:
+			inputError: errors.New(`user "resource-name-user" (groups=["test"]) is attempting to grant RBAC permissions not currently held:
 {APIGroups:["extensions"], Resources:["ingresses"], ResourceNames:["my-ingress"], Verbs:["update" "patch"]}`),
 			expectedResult: &parseResult{
 				MissingRules: []rbacv1.PolicyRule{
@@ -340,7 +338,7 @@ func TestParseEscalationErrorForMissingRules(t *testing.T) {
 		},
 		{
 			name: "Missing Rule (No ResourceNames)",
-			inputError: errors.New(`user "no-res-name-user" (groups=test) is attempting to grant RBAC permissions not currently held:
+			inputError: errors.New(`user "no-res-name-user" (groups=["test"]) is attempting to grant RBAC permissions not currently held:
 {APIGroups:["networking.k8s.io"], Resources:["networkpolicies"], Verbs:["watch"]}`),
 			expectedResult: &parseResult{
 				MissingRules: []rbacv1.PolicyRule{
@@ -351,7 +349,7 @@ func TestParseEscalationErrorForMissingRules(t *testing.T) {
 		},
 		{
 			name: "Missing Rule (NonResourceURLs only)",
-			inputError: errors.New(`user "url-user" (groups=test) is attempting to grant RBAC permissions not currently held:
+			inputError: errors.New(`user "url-user" (groups=["test"]) is attempting to grant RBAC permissions not currently held:
 {NonResourceURLs:["/version" "/apis"], Verbs:["get"]}`),
 			expectedResult: &parseResult{
 				MissingRules: []rbacv1.PolicyRule{
@@ -370,7 +368,7 @@ func TestParseEscalationErrorForMissingRules(t *testing.T) {
 		},
 		{
 			name: "Empty Permissions String",
-			inputError: errors.New(`user "empty-perms" (groups=test) is attempting to grant RBAC permissions not currently held:
+			inputError: errors.New(`user "empty-perms" (groups=["test"]) is attempting to grant RBAC permissions not currently held:
 `),
 			expectedResult: &parseResult{},
 			expectError: func(t require.TestingT, err error, i ...interface{}) {
@@ -379,7 +377,7 @@ func TestParseEscalationErrorForMissingRules(t *testing.T) {
 		},
 		{
 			name: "Rule with Empty Strings in lists",
-			inputError: errors.New(`user "empty-strings" (groups=test) is attempting to grant RBAC permissions not currently held:
+			inputError: errors.New(`user "empty-strings" (groups=["test"]) is attempting to grant RBAC permissions not currently held:
 {APIGroups:["" "apps"], Resources:["" "deployments"], Verbs:["get" ""]}`),
 			expectedResult: &parseResult{
 				MissingRules: []rbacv1.PolicyRule{
@@ -390,7 +388,7 @@ func TestParseEscalationErrorForMissingRules(t *testing.T) {
 		},
 		{
 			name: "Rule with Only Empty Verb",
-			inputError: errors.New(`user "empty-verb" (groups=test) is attempting to grant RBAC permissions not currently held:
+			inputError: errors.New(`user "empty-verb" (groups=["test"]) is attempting to grant RBAC permissions not currently held:
 {APIGroups:[""], Resources:["pods"], Verbs:[""]}`),
 			expectedResult: &parseResult{
 				MissingRules: []rbacv1.PolicyRule{
@@ -401,16 +399,26 @@ func TestParseEscalationErrorForMissingRules(t *testing.T) {
 		},
 		{
 			name: "Rule with no fields",
-			inputError: errors.New(`user "empty-verb" (groups=test) is attempting to grant RBAC permissions not currently held:
+			inputError: errors.New(`user "empty-verb" (groups=["test"]) is attempting to grant RBAC permissions not currently held:
 {}`),
 			expectedResult: &parseResult{
 				MissingRules: []rbacv1.PolicyRule{{}},
 			},
 			expectError: require.NoError,
 		},
+		{
+			name: "Rule with no colon separator",
+			inputError: errors.New(`user "empty-verb" (groups=["test"]) is attempting to grant RBAC permissions not currently held:
+{APIGroups:[""], Resources, Verbs:["get"]}
+`),
+			expectedResult: &parseResult{},
+			expectError: func(t require.TestingT, err error, i ...interface{}) {
+				require.ErrorContains(t, err, `unexpected item "Resources": expected <Type>:[<values>...]`)
+			},
+		},
 		{
 			name: "Rule with unknown field",
-			inputError: errors.New(`user "empty-verb" (groups=test) is attempting to grant RBAC permissions not currently held:
+			inputError: errors.New(`user "empty-verb" (groups=["test"]) is attempting to grant RBAC permissions not currently held:
 {FooBar:["baz"]}
 {APIGroups:[""], Resources:["secrets"], Verbs:["get"]}
 `),
@@ -428,9 +436,125 @@ func TestParseEscalationErrorForMissingRules(t *testing.T) {
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
 			rules, err := parseEscalationErrorForMissingRules(tc.inputError)
-
 			tc.expectError(t, err)
 			require.Equal(t, tc.expectedResult, rules)
 		})
 	}
 }
+
+func TestParseEscalationErrorForMissingRules_KubernetesCompatibility(t *testing.T) {
+	testCases := []struct {
+		name                string
+		ruleResolver        validation.AuthorizationRuleResolver
+		wantRules           []rbacv1.PolicyRule
+		expectedErrorString string
+		expectedResult      *parseResult
+	}{
+		{
+			name: "missing rules",
+			ruleResolver: mockRulesResolver{
+				rules: []rbacv1.PolicyRule{},
+				err:   nil,
+			},
+			wantRules: []rbacv1.PolicyRule{
+				{APIGroups: []string{""}, Resources: []string{"secrets"}, Verbs: []string{"get"}, ResourceNames: []string{"test-secret"}},
+				{APIGroups: []string{""}, Resources: []string{"configmaps"}, Verbs: []string{"get", "list", "watch"}},
+				{APIGroups: []string{"apps"}, Resources: []string{"deployments", "replicasets"}, Verbs: []string{"create", "update", "patch", "delete"}},
+				{NonResourceURLs: []string{"/healthz", "/livez"}, Verbs: []string{"get", "post"}},
+			},
+			expectedErrorString: `user "user" (groups=["a" "b"]) is attempting to grant RBAC permissions not currently held:
+{APIGroups:[""], Resources:["configmaps"], Verbs:["get" "list" "watch"]}
+{APIGroups:[""], Resources:["secrets"], ResourceNames:["test-secret"], Verbs:["get"]}
+{APIGroups:["apps"], Resources:["deployments"], Verbs:["create" "update" "patch" "delete"]}
+{APIGroups:["apps"], Resources:["replicasets"], Verbs:["create" "update" "patch" "delete"]}
+{NonResourceURLs:["/healthz"], Verbs:["get"]}
+{NonResourceURLs:["/healthz"], Verbs:["post"]}
+{NonResourceURLs:["/livez"], Verbs:["get"]}
+{NonResourceURLs:["/livez"], Verbs:["post"]}`,
+			expectedResult: &parseResult{
+				MissingRules: []rbacv1.PolicyRule{
+					{APIGroups: []string{""}, Resources: []string{"configmaps"}, Verbs: []string{"get", "list", "watch"}},
+					{APIGroups: []string{""}, Resources: []string{"secrets"}, Verbs: []string{"get"}, ResourceNames: []string{"test-secret"}},
+					{APIGroups: []string{"apps"}, Resources: []string{"deployments"}, Verbs: []string{"create", "update", "patch", "delete"}},
+					{APIGroups: []string{"apps"}, Resources: []string{"replicasets"}, Verbs: []string{"create", "update", "patch", "delete"}},
+					{NonResourceURLs: []string{"/healthz"}, Verbs: []string{"get"}},
+					{NonResourceURLs: []string{"/healthz"}, Verbs: []string{"post"}},
+					{NonResourceURLs: []string{"/livez"}, Verbs: []string{"get"}},
+					{NonResourceURLs: []string{"/livez"}, Verbs: []string{"post"}},
+				},
+			},
+		},
+		{
+			name: "resolution failure",
+			ruleResolver: mockRulesResolver{
+				rules: []rbacv1.PolicyRule{},
+				err:   errors.New("resolution error"),
+			},
+			wantRules: []rbacv1.PolicyRule{
+				{APIGroups: []string{""}, Resources: []string{"secrets"}, Verbs: []string{"get"}, ResourceNames: []string{"test-secret"}},
+				{APIGroups: []string{""}, Resources: []string{"configmaps"}, Verbs: []string{"get", "list", "watch"}},
+				{APIGroups: []string{"apps"}, Resources: []string{"deployments", "replicasets"}, Verbs: []string{"create", "update", "patch", "delete"}},
+				{NonResourceURLs: []string{"/healthz", "/livez"}, Verbs: []string{"get", "post"}},
+			},
+			expectedErrorString: `user "user" (groups=["a" "b"]) is attempting to grant RBAC permissions not currently held:
+{APIGroups:[""], Resources:["configmaps"], Verbs:["get" "list" "watch"]}
+{APIGroups:[""], Resources:["secrets"], ResourceNames:["test-secret"], Verbs:["get"]}
+{APIGroups:["apps"], Resources:["deployments"], Verbs:["create" "update" "patch" "delete"]}
+{APIGroups:["apps"], Resources:["replicasets"], Verbs:["create" "update" "patch" "delete"]}
+{NonResourceURLs:["/healthz"], Verbs:["get"]}
+{NonResourceURLs:["/healthz"], Verbs:["post"]}
+{NonResourceURLs:["/livez"], Verbs:["get"]}
+{NonResourceURLs:["/livez"], Verbs:["post"]}; resolution errors: [resolution error]`,
+			expectedResult: &parseResult{
+				MissingRules: []rbacv1.PolicyRule{
+					{APIGroups: []string{""}, Resources: []string{"configmaps"}, Verbs: []string{"get", "list", "watch"}},
+					{APIGroups: []string{""}, Resources: []string{"secrets"}, Verbs: []string{"get"}, ResourceNames: []string{"test-secret"}},
+					{APIGroups: []string{"apps"}, Resources: []string{"deployments"}, Verbs: []string{"create", "update", "patch", "delete"}},
+					{APIGroups: []string{"apps"}, Resources: []string{"replicasets"}, Verbs: []string{"create", "update", "patch", "delete"}},
+					{NonResourceURLs: []string{"/healthz"}, Verbs: []string{"get"}},
+					{NonResourceURLs: []string{"/healthz"}, Verbs: []string{"post"}},
+					{NonResourceURLs: []string{"/livez"}, Verbs: []string{"get"}},
+					{NonResourceURLs: []string{"/livez"}, Verbs: []string{"post"}},
+				},
+				ResolutionErrors: errors.New("[resolution error]"),
+			},
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			ctx := request.WithUser(request.WithNamespace(context.Background(), "namespace"), &user.DefaultInfo{
+				Name:   "user",
+				Groups: []string{"a", "b"},
+			})
+
+			// Let's actually call the upstream function that generates and returns the
+			// error message that we are attempting to parse correctly. The hope is that
+			// these tests will start failing if we bump to a new version of kubernetes
+			// that causes our parsing logic to be incorrect.
+			err := validation.ConfirmNoEscalation(ctx, tc.ruleResolver, tc.wantRules)
+			require.Error(t, err)
+			require.Equal(t, tc.expectedErrorString, err.Error())
+
+			res, err := parseEscalationErrorForMissingRules(err)
+			require.NoError(t, err)
+			require.Equal(t, tc.expectedResult, res)
+		})
+	}
+}
+
+type mockRulesResolver struct {
+	rules []rbacv1.PolicyRule
+	err   error
+}
+
+func (m mockRulesResolver) GetRoleReferenceRules(ctx context.Context, roleRef rbacv1.RoleRef, namespace string) ([]rbacv1.PolicyRule, error) {
+	panic("unimplemented")
+}
+
+func (m mockRulesResolver) RulesFor(ctx context.Context, user user.Info, namespace string) ([]rbacv1.PolicyRule, error) {
+	return m.rules, m.err
+}
+
+func (m mockRulesResolver) VisitRulesFor(ctx context.Context, user user.Info, namespace string, visitor func(source fmt.Stringer, rule *rbacv1.PolicyRule, err error) bool) {
+	panic("unimplemented")
+}