Skip to content

✨ Make RBACPreAuthorizer collection verbs configurable #2539

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
perdasilva wants to merge 1 commit into
base: main
Choose a base branch
from
Open

✨ Make RBACPreAuthorizer collection verbs configurable #2539

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion cmd/operator-controller/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -724,7 +724,12 @@ func (c *helmReconcilerConfigurator) Configure(ceReconciler *controllers.Cluster
// determine if PreAuthorizer should be enabled based on feature gate
var preAuth authorization.PreAuthorizer
if features.OperatorControllerFeatureGate.Enabled(features.PreflightPermissions) {
preAuth = authorization.NewRBACPreAuthorizer(c.mgr.GetClient())
preAuth = authorization.NewRBACPreAuthorizer(
c.mgr.GetClient(),
// Additional verbs / bundle manifest that are expected by the content manager to watch those resources
authorization.WithClusterCollectionVerbs("list", "watch"),
authorization.WithNamespacedCollectionVerbs("create"),
)
}

cm := contentmanager.NewManager(clientRestConfigMapper, c.mgr.GetConfig(), c.mgr.GetRESTMapper())
Expand Down
48 changes: 32 additions & 16 deletions internal/operator-controller/authorization/rbac.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import (
rbacv1helpers "k8s.io/kubernetes/pkg/apis/rbac/v1"
rbacregistry "k8s.io/kubernetes/pkg/registry/rbac"
"k8s.io/kubernetes/pkg/registry/rbac/validation"
rbac "k8s.io/kubernetes/plugin/pkg/auth/authorizer/rbac"
"k8s.io/kubernetes/plugin/pkg/auth/authorizer/rbac"
"sigs.k8s.io/controller-runtime/pkg/client"
)

Expand Down Expand Up @@ -59,26 +59,42 @@ type ScopedPolicyRules struct {

var objectVerbs = []string{"get", "patch", "update", "delete"}

// Here we are splitting collection verbs based on required scope
// NB: this split is tightly coupled to the requirements of the contentmanager, specifically
// its need for cluster-scoped list/watch permissions.
// TODO: We are accepting this coupling for now, but plan to decouple
// TODO: link for above https://github.com/operator-framework/operator-controller/issues/1911
var namespacedCollectionVerbs = []string{"create"}
var clusterCollectionVerbs = []string{"list", "watch"}
type RBACPreAuthorizerOption func(*rbacPreAuthorizer)

// WithClusterCollectionVerbs configures cluster-scoped collection verbs (e.g. list, watch)
// that are checked in addition to object and namespaced collection verbs.
func WithClusterCollectionVerbs(verbs ...string) RBACPreAuthorizerOption {
return func(a *rbacPreAuthorizer) {
a.clusterCollectionVerbs = slices.Clone(verbs)
}
}

// WithNamespacedCollectionVerbs configures namespaced collection verbs (e.g. create)
// that are checked for each unique namespace across all objects in a GVR.
func WithNamespacedCollectionVerbs(verbs ...string) RBACPreAuthorizerOption {
return func(a *rbacPreAuthorizer) {
a.namespacedCollectionVerbs = slices.Clone(verbs)
}
}

type rbacPreAuthorizer struct {
authorizer authorizer.Authorizer
ruleResolver validation.AuthorizationRuleResolver
restMapper meta.RESTMapper
authorizer authorizer.Authorizer
ruleResolver validation.AuthorizationRuleResolver
restMapper meta.RESTMapper
clusterCollectionVerbs []string
namespacedCollectionVerbs []string
}

func NewRBACPreAuthorizer(cl client.Client) PreAuthorizer {
return &rbacPreAuthorizer{
func NewRBACPreAuthorizer(cl client.Client, opts ...RBACPreAuthorizerOption) PreAuthorizer {
a := &rbacPreAuthorizer{
authorizer: newRBACAuthorizer(cl),
ruleResolver: newRBACRulesResolver(cl),
restMapper: cl.RESTMapper(),
}
for _, opt := range opts {
opt(a)
}
return a
}

func (a *rbacPreAuthorizer) PreAuthorize(ctx context.Context, user user.Info, manifestReader io.Reader, additionalRequiredPerms ...UserAuthorizerAttributesFactory) ([]ScopedPolicyRules, error) {
Expand All @@ -88,7 +104,7 @@ func (a *rbacPreAuthorizer) PreAuthorize(ctx context.Context, user user.Info, ma
}

// derive manifest related attributes records
attributesRecords := dm.asAuthorizationAttributesRecordsForUser(user)
attributesRecords := dm.asAuthorizationAttributesRecordsForUser(user, a.clusterCollectionVerbs, a.namespacedCollectionVerbs)

// append additional required perms
for _, fn := range additionalRequiredPerms {
Expand Down Expand Up @@ -324,7 +340,7 @@ func (dm *decodedManifest) rbacObjects() []client.Object {
return objects
}

func (dm *decodedManifest) asAuthorizationAttributesRecordsForUser(manifestManager user.Info) []authorizer.AttributesRecord {
func (dm *decodedManifest) asAuthorizationAttributesRecordsForUser(manifestManager user.Info, clusterCollectionVerbs, namespacedCollectionVerbs []string) []authorizer.AttributesRecord {
// Calculate initial capacity as an upper-bound estimate:
// - For each key: len(objectVerbs) records (4)
// - For unique namespaces: len(namespacedCollectionVerbs) records (1 per unique namespace across all keys in a GVR)
Expand Down Expand Up @@ -369,7 +385,7 @@ func (dm *decodedManifest) asAuthorizationAttributesRecordsForUser(manifestManag
})
}
}
// generate records for cluster-scoped collection verbs (list, watch) required by contentmanager
// generate records for cluster-scoped collection verbs (e.g. list, watch)
for _, v := range clusterCollectionVerbs {
attributeRecords = append(attributeRecords, authorizer.AttributesRecord{
User: manifestManager,
Expand Down
241 changes: 236 additions & 5 deletions internal/operator-controller/authorization/rbac_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,7 +396,7 @@ 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) {
fakeClient := setupFakeClient(privilegedClusterRole)
preAuth := NewRBACPreAuthorizer(fakeClient)
preAuth := NewRBACPreAuthorizer(fakeClient, WithClusterCollectionVerbs("list", "watch"), WithNamespacedCollectionVerbs("create"))
missingRules, err := preAuth.PreAuthorize(context.TODO(), testUser, strings.NewReader(testManifest))
require.NoError(t, err)
require.Equal(t, []ScopedPolicyRules{}, missingRules)
Expand All @@ -406,7 +406,7 @@ func TestPreAuthorize_Success(t *testing.T) {
func TestPreAuthorize_MissingRBAC(t *testing.T) {
t.Run("preauthorize fails and finds missing rbac rules", func(t *testing.T) {
fakeClient := setupFakeClient(limitedClusterRole)
preAuth := NewRBACPreAuthorizer(fakeClient)
preAuth := NewRBACPreAuthorizer(fakeClient, WithClusterCollectionVerbs("list", "watch"), WithNamespacedCollectionVerbs("create"))
missingRules, err := preAuth.PreAuthorize(context.TODO(), testUser, strings.NewReader(testManifest))
require.NoError(t, err)
require.Equal(t, expectedSingleNamespaceMissingRules, missingRules)
Expand All @@ -416,7 +416,7 @@ func TestPreAuthorize_MissingRBAC(t *testing.T) {
func TestPreAuthorizeMultiNamespace_MissingRBAC(t *testing.T) {
t.Run("preauthorize fails and finds missing rbac rules in multiple namespaces", func(t *testing.T) {
fakeClient := setupFakeClient(limitedClusterRole)
preAuth := NewRBACPreAuthorizer(fakeClient)
preAuth := NewRBACPreAuthorizer(fakeClient, WithClusterCollectionVerbs("list", "watch"), WithNamespacedCollectionVerbs("create"))
missingRules, err := preAuth.PreAuthorize(context.TODO(), testUser, strings.NewReader(testManifestMultiNamespace))
require.NoError(t, err)
require.Equal(t, expectedMultiNamespaceMissingRules, missingRules)
Expand All @@ -426,7 +426,7 @@ func TestPreAuthorizeMultiNamespace_MissingRBAC(t *testing.T) {
func TestPreAuthorize_CheckEscalation(t *testing.T) {
t.Run("preauthorize succeeds with no missing rbac rules", func(t *testing.T) {
fakeClient := setupFakeClient(escalatingClusterRole)
preAuth := NewRBACPreAuthorizer(fakeClient)
preAuth := NewRBACPreAuthorizer(fakeClient, WithClusterCollectionVerbs("list", "watch"), WithNamespacedCollectionVerbs("create"))
missingRules, err := preAuth.PreAuthorize(context.TODO(), testUser, strings.NewReader(testManifest))
require.NoError(t, err)
require.Equal(t, []ScopedPolicyRules{}, missingRules)
Expand All @@ -436,7 +436,7 @@ func TestPreAuthorize_CheckEscalation(t *testing.T) {
func TestPreAuthorize_AdditionalRequiredPerms_MissingRBAC(t *testing.T) {
t.Run("preauthorize fails and finds missing rbac rules coming from the additional required permissions", func(t *testing.T) {
fakeClient := setupFakeClient(escalatingClusterRole)
preAuth := NewRBACPreAuthorizer(fakeClient)
preAuth := NewRBACPreAuthorizer(fakeClient, WithClusterCollectionVerbs("list", "watch"), WithNamespacedCollectionVerbs("create"))
missingRules, err := preAuth.PreAuthorize(context.TODO(), testUser, strings.NewReader(testManifest), func(user user.Info) []authorizer.AttributesRecord {
return []authorizer.AttributesRecord{
{
Expand Down Expand Up @@ -465,6 +465,237 @@ func TestPreAuthorize_AdditionalRequiredPerms_MissingRBAC(t *testing.T) {
})
}

func TestPreAuthorize_WithClusterCollectionVerbs(t *testing.T) {
// expectedNamespacedMissingRules are the missing rules expected in the "test-namespace"
// namespace regardless of cluster collection verb configuration. These come from object
// verbs (get, patch, update, delete), namespaced collection verbs (create), and the
// escalation check for the role/rolebinding in the manifest.
expectedNamespacedMissingRules := ScopedPolicyRules{
Namespace: "test-namespace",
MissingRules: []rbacv1.PolicyRule{
{
Verbs: []string{"create"},
APIGroups: []string{"*"},
Resources: []string{"certificates"}},
{
Verbs: []string{"create"},
APIGroups: []string{""},
Resources: []string{"services"}},
{
Verbs: []string{"create"},
APIGroups: []string{"rbac.authorization.k8s.io"},
Resources: []string{"rolebindings"}},
{
Verbs: []string{"create"},
APIGroups: []string{"rbac.authorization.k8s.io"},
Resources: []string{"roles"}},
{
Verbs: []string{"delete", "get", "patch", "update"},
APIGroups: []string{""},
Resources: []string{"services"},
ResourceNames: []string{"test-service"}},
{
Verbs: []string{"delete", "get", "patch", "update"},
APIGroups: []string{"rbac.authorization.k8s.io"},
Resources: []string{"rolebindings"},
ResourceNames: []string{"test-extension-binding"}},
{
Verbs: []string{"delete", "get", "patch", "update"},
APIGroups: []string{"rbac.authorization.k8s.io"},
Resources: []string{"roles"},
ResourceNames: []string{"test-extension-role"}},
{
Verbs: []string{"watch"},
APIGroups: []string{"*"},
Resources: []string{"serviceaccounts"},
},
},
}

t.Run("no cluster collection verbs option omits cluster-scoped collection rules", func(t *testing.T) {
fakeClient := setupFakeClient(limitedClusterRole)
preAuth := NewRBACPreAuthorizer(fakeClient, WithNamespacedCollectionVerbs("create"))
missingRules, err := preAuth.PreAuthorize(context.TODO(), testUser, strings.NewReader(testManifest))
require.NoError(t, err)
// With no cluster collection verbs, there should be no cluster-scoped (namespace="") missing rules
require.Equal(t, []ScopedPolicyRules{expectedNamespacedMissingRules}, missingRules)
})

t.Run("cluster verbs option only checks those verbs at cluster scope", func(t *testing.T) {
fakeClient := setupFakeClient(limitedClusterRole)
preAuth := NewRBACPreAuthorizer(fakeClient, WithClusterCollectionVerbs("get", "patch", "update"), WithNamespacedCollectionVerbs("create"))
missingRules, err := preAuth.PreAuthorize(context.TODO(), testUser, strings.NewReader(testManifest))
require.NoError(t, err)
require.Equal(t, []ScopedPolicyRules{
{
Namespace: "",
MissingRules: []rbacv1.PolicyRule{
{
Verbs: []string{"get", "patch", "update"},
APIGroups: []string{""},
Resources: []string{"services"},
ResourceNames: []string(nil),
NonResourceURLs: []string(nil)},
{
Verbs: []string{"get", "patch", "update"},
APIGroups: []string{"rbac.authorization.k8s.io"},
Resources: []string{"rolebindings"},
ResourceNames: []string(nil),
NonResourceURLs: []string(nil)},
{
Verbs: []string{"get", "patch", "update"},
APIGroups: []string{"rbac.authorization.k8s.io"},
Resources: []string{"roles"},
ResourceNames: []string(nil),
NonResourceURLs: []string(nil),
},
},
},
expectedNamespacedMissingRules,
}, missingRules)
})

t.Run("privileged user with no cluster collection verbs succeeds", func(t *testing.T) {
fakeClient := setupFakeClient(privilegedClusterRole)
preAuth := NewRBACPreAuthorizer(fakeClient, WithNamespacedCollectionVerbs("create"))
missingRules, err := preAuth.PreAuthorize(context.TODO(), testUser, strings.NewReader(testManifest))
require.NoError(t, err)
require.Equal(t, []ScopedPolicyRules{}, missingRules)
})
}

func TestPreAuthorize_WithNamespacedCollectionVerbs(t *testing.T) {
// expectedClusterMissingRules are the missing rules expected at cluster scope
// when cluster collection verbs are configured as "list", "watch".
expectedClusterMissingRules := ScopedPolicyRules{
Namespace: "",
MissingRules: []rbacv1.PolicyRule{
{
Verbs: []string{"list", "watch"},
APIGroups: []string{""},
Resources: []string{"services"},
ResourceNames: []string(nil),
NonResourceURLs: []string(nil)},
{
Verbs: []string{"list", "watch"},
APIGroups: []string{"rbac.authorization.k8s.io"},
Resources: []string{"rolebindings"},
ResourceNames: []string(nil),
NonResourceURLs: []string(nil)},
{
Verbs: []string{"list", "watch"},
APIGroups: []string{"rbac.authorization.k8s.io"},
Resources: []string{"roles"},
ResourceNames: []string(nil),
NonResourceURLs: []string(nil),
},
},
}

t.Run("no namespaced collection verbs option omits namespaced collection rules", func(t *testing.T) {
fakeClient := setupFakeClient(limitedClusterRole)
preAuth := NewRBACPreAuthorizer(fakeClient, WithClusterCollectionVerbs("list", "watch"))
missingRules, err := preAuth.PreAuthorize(context.TODO(), testUser, strings.NewReader(testManifest))
require.NoError(t, err)
// Without namespaced collection verbs, no "create" rules from collection verbs should appear,
// but object verbs (get, patch, update, delete) and escalation checks still apply
require.Equal(t, []ScopedPolicyRules{
expectedClusterMissingRules,
{
Namespace: "test-namespace",
MissingRules: []rbacv1.PolicyRule{
{
Verbs: []string{"create"},
APIGroups: []string{"*"},
Resources: []string{"certificates"}},
{
Verbs: []string{"delete", "get", "patch", "update"},
APIGroups: []string{""},
Resources: []string{"services"},
ResourceNames: []string{"test-service"}},
{
Verbs: []string{"delete", "get", "patch", "update"},
APIGroups: []string{"rbac.authorization.k8s.io"},
Resources: []string{"rolebindings"},
ResourceNames: []string{"test-extension-binding"}},
{
Verbs: []string{"delete", "get", "patch", "update"},
APIGroups: []string{"rbac.authorization.k8s.io"},
Resources: []string{"roles"},
ResourceNames: []string{"test-extension-role"}},
{
Verbs: []string{"watch"},
APIGroups: []string{"*"},
Resources: []string{"serviceaccounts"},
},
},
},
}, missingRules)
})

t.Run("namespaced collection verbs option checks those verbs per namespace", func(t *testing.T) {
fakeClient := setupFakeClient(limitedClusterRole)
preAuth := NewRBACPreAuthorizer(fakeClient, WithClusterCollectionVerbs("list", "watch"), WithNamespacedCollectionVerbs("create", "deletecollection"))
missingRules, err := preAuth.PreAuthorize(context.TODO(), testUser, strings.NewReader(testManifest))
require.NoError(t, err)
// Should have cluster-scoped missing rules plus namespaced rules with both create and deletecollection.
// Note: "certificates" with apiGroup "*" comes from the escalation check on the Role, not
// from namespaced collection verbs, so it only has "create".
require.Equal(t, []ScopedPolicyRules{
expectedClusterMissingRules,
{
Namespace: "test-namespace",
MissingRules: []rbacv1.PolicyRule{
{
Verbs: []string{"create", "deletecollection"},
APIGroups: []string{""},
Resources: []string{"services"}},
{
Verbs: []string{"create", "deletecollection"},
APIGroups: []string{"rbac.authorization.k8s.io"},
Resources: []string{"rolebindings"}},
{
Verbs: []string{"create", "deletecollection"},
APIGroups: []string{"rbac.authorization.k8s.io"},
Resources: []string{"roles"}},
{
Verbs: []string{"create"},
APIGroups: []string{"*"},
Resources: []string{"certificates"}},
{
Verbs: []string{"delete", "get", "patch", "update"},
APIGroups: []string{""},
Resources: []string{"services"},
ResourceNames: []string{"test-service"}},
{
Verbs: []string{"delete", "get", "patch", "update"},
APIGroups: []string{"rbac.authorization.k8s.io"},
Resources: []string{"rolebindings"},
ResourceNames: []string{"test-extension-binding"}},
{
Verbs: []string{"delete", "get", "patch", "update"},
APIGroups: []string{"rbac.authorization.k8s.io"},
Resources: []string{"roles"},
ResourceNames: []string{"test-extension-role"}},
{
Verbs: []string{"watch"},
APIGroups: []string{"*"},
Resources: []string{"serviceaccounts"},
},
},
},
}, missingRules)
})

t.Run("privileged user with custom namespaced collection verbs succeeds", func(t *testing.T) {
fakeClient := setupFakeClient(privilegedClusterRole)
preAuth := NewRBACPreAuthorizer(fakeClient, WithNamespacedCollectionVerbs("create", "deletecollection"))
missingRules, err := preAuth.PreAuthorize(context.TODO(), testUser, strings.NewReader(testManifest))
require.NoError(t, err)
require.Equal(t, []ScopedPolicyRules{}, missingRules)
})
}

// 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_ParsingLogic(t *testing.T) {
Expand Down
Loading
Loading