feat(iam): resolve PolicyBinding subject uid from name

Taskfile.yaml

@@ -333,7 +333,7 @@ tasks:
       - "\"{{.TOOL_DIR}}/controller-gen\" webhook paths=\"./internal/webhooks/...\" output:dir=\"./config/webhook\""
       # Generate RBAC rules for the controllers.
       - echo "Generating RBAC rules for the controllers..."
-      - "\"{{.TOOL_DIR}}/controller-gen\" rbac:roleName=milo-controller-manager paths=\"./internal/controllers/...\" paths=\"./internal/quota/controllers/...\" output:dir=\"./config/controller-manager/overlays/core-control-plane/rbac\""
+      - "\"{{.TOOL_DIR}}/controller-gen\" rbac:roleName=milo-controller-manager paths=\"./internal/controllers/...\" paths=\"./internal/quota/controllers/...\" paths=\"./internal/webhooks/...\" output:dir=\"./config/controller-manager/overlays/core-control-plane/rbac\""
     silent: true
 
   generate:openapi:identity:

cmd/milo/controller-manager/controllermanager.go

@@ -570,6 +570,10 @@ func Run(ctx context.Context, c *config.CompletedConfig, opts *Options) error {
 				logger.Error(err, "Error setting up platform access rejection webhook")
 				klog.FlushAndExit(klog.ExitFlushTimeout, 1)
 			}
+			if err := iamv1alpha1webhook.SetupPolicyBindingWebhooksWithManager(ctrl); err != nil {
+				logger.Error(err, "Error setting up policybinding webhook")
+				klog.FlushAndExit(klog.ExitFlushTimeout, 1)
+			}
 			// Note webhooks are set up later after the multicluster manager is created,
 			// so they can use it for project control plane lookups.
 

config/controller-manager/overlays/core-control-plane/rbac/role.yaml

@@ -137,6 +137,7 @@ rules:
   - iam.miloapis.com
   resources:
   - roles
+  - serviceaccounts
   - userdeactivations
   verbs:
   - get

config/webhook/manifests.yaml

@@ -67,6 +67,28 @@ webhooks:
     resources:
     - platforminvitations
   sideEffects: None
+- admissionReviewVersions:
+  - v1
+  - v1beta1
+  clientConfig:
+    service:
+      name: milo-controller-manager
+      namespace: milo-system
+      path: /mutate-iam-miloapis-com-v1alpha1-policybinding
+      port: 9443
+  failurePolicy: Fail
+  name: mpolicybinding.iam.miloapis.com
+  rules:
+  - apiGroups:
+    - iam.miloapis.com
+    apiVersions:
+    - v1alpha1
+    operations:
+    - CREATE
+    - UPDATE
+    resources:
+    - policybindings
+  sideEffects: None
 - admissionReviewVersions:
   - v1
   - v1beta1

internal/webhooks/iam/v1alpha1/policybinding_webhook.go

@@ -0,0 +1,128 @@
+package v1alpha1
+
+import (
+	"context"
+	"fmt"
+	"strings"
+
+	"k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	logf "sigs.k8s.io/controller-runtime/pkg/log"
+
+	iamv1alpha1 "go.miloapis.com/milo/pkg/apis/iam/v1alpha1"
+)
+
+// systemGroupPrefix identifies Group subjects that are synthesized by the
+// platform (for example "system:authenticated-users"). These groups have no
+// backing object and therefore no uid to resolve.
+const systemGroupPrefix = "system:"
+
+func SetupPolicyBindingWebhooksWithManager(mgr ctrl.Manager) error {
+	return ctrl.NewWebhookManagedBy(mgr, &iamv1alpha1.PolicyBinding{}).
+		WithDefaulter(&PolicyBindingMutator{
+			client: mgr.GetClient(),
+		}).
+		Complete()
+}
+
+// +kubebuilder:webhook:path=/mutate-iam-miloapis-com-v1alpha1-policybinding,mutating=true,failurePolicy=fail,sideEffects=None,groups=iam.miloapis.com,resources=policybindings,verbs=create;update,versions=v1alpha1,name=mpolicybinding.iam.miloapis.com,admissionReviewVersions={v1,v1beta1},serviceName=milo-controller-manager,servicePort=9443,serviceNamespace=milo-system
+
+// +kubebuilder:rbac:groups=iam.miloapis.com,resources=users;groups;serviceaccounts,verbs=get;list;watch
+
+// PolicyBindingMutator resolves the uid of each PolicyBinding subject from its
+// name. Callers may reference a User, Group, or ServiceAccount by name without
+// supplying a uid; the mutator looks up the named object and stamps its current
+// uid into the subject. This makes bindings declaratively committable (a Group
+// and its PolicyBinding can be applied together by GitOps) while preserving the
+// instance-pinning guarantee: the stored uid still identifies one specific
+// object instance, so a delete+recreate of a same-named subject yields a new
+// uid and the binding no longer matches until it is re-applied.
+type PolicyBindingMutator struct {
+	client client.Client
+}
+
+func (m *PolicyBindingMutator) Default(ctx context.Context, pb *iamv1alpha1.PolicyBinding) error {
+	log := logf.FromContext(ctx).WithValues("policybinding", pb.GetName(), "namespace", pb.GetNamespace())
+
+	var errs field.ErrorList
+	subjectsPath := field.NewPath("spec").Child("subjects")
+
+	for i := range pb.Spec.Subjects {
+		subject := &pb.Spec.Subjects[i]
+		subjectPath := subjectsPath.Index(i)
+
+		// A subject that already carries a uid is left untouched: the uid is
+		// immutable in the stored spec and callers (including internal
+		// controllers) may set it explicitly.
+		if subject.UID != "" {
+			continue
+		}
+
+		// System groups have no backing object, so there is nothing to resolve.
+		// The validating CEL rule permits these without a uid.
+		if subject.Kind == "Group" && strings.HasPrefix(subject.Name, systemGroupPrefix) {
+			continue
+		}
+
+		uid, fieldErr := m.resolveSubjectUID(ctx, subject, subjectPath)
+		if fieldErr != nil {
+			errs = append(errs, fieldErr)
+			continue
+		}
+
+		log.Info("resolved subject uid from name", "kind", subject.Kind, "name", subject.Name, "uid", uid)
+		subject.UID = uid
+	}
+
+	if len(errs) > 0 {
+		return errors.NewInvalid(iamv1alpha1.SchemeGroupVersion.WithKind("PolicyBinding").GroupKind(), pb.Name, errs)
+	}
+
+	return nil
+}
+
+// resolveSubjectUID looks up the object named by the subject and returns its
+// uid. It returns a field error when the subject is malformed or the named
+// object does not exist.
+func (m *PolicyBindingMutator) resolveSubjectUID(ctx context.Context, subject *iamv1alpha1.Subject, subjectPath *field.Path) (string, *field.Error) {
+	namePath := subjectPath.Child("name")
+
+	switch subject.Kind {
+	case "User":
+		user := &iamv1alpha1.User{}
+		if err := m.client.Get(ctx, client.ObjectKey{Name: subject.Name}, user); err != nil {
+			return "", lookupFieldError(namePath, subject.Name, "User", err)
+		}
+		return string(user.GetUID()), nil
+	case "ServiceAccount":
+		sa := &iamv1alpha1.ServiceAccount{}
+		if err := m.client.Get(ctx, client.ObjectKey{Name: subject.Name}, sa); err != nil {
+			return "", lookupFieldError(namePath, subject.Name, "ServiceAccount", err)
+		}
+		return string(sa.GetUID()), nil
+	case "Group":
+		// Groups are namespaced, so a namespace is required to resolve a
+		// non-system Group by name.
+		if subject.Namespace == "" {
+			return "", field.Required(subjectPath.Child("namespace"), "namespace is required to resolve a Group subject by name")
+		}
+		group := &iamv1alpha1.Group{}
+		if err := m.client.Get(ctx, client.ObjectKey{Namespace: subject.Namespace, Name: subject.Name}, group); err != nil {
+			return "", lookupFieldError(namePath, subject.Name, "Group", err)
+		}
+		return string(group.GetUID()), nil
+	default:
+		return "", field.NotSupported(subjectPath.Child("kind"), subject.Kind, []string{"User", "Group", "ServiceAccount"})
+	}
+}
+
+// lookupFieldError converts a client.Get error into a field error suitable for
+// an admission response.
+func lookupFieldError(namePath *field.Path, name, kind string, err error) *field.Error {
+	if errors.IsNotFound(err) {
+		return field.Invalid(namePath, name, fmt.Sprintf("%s %q not found; cannot resolve subject uid", kind, name))
+	}
+	return field.InternalError(namePath, fmt.Errorf("failed to get %s %q: %w", kind, name, err))
+}

internal/webhooks/iam/v1alpha1/policybinding_webhook_test.go

@@ -0,0 +1,124 @@
+package v1alpha1
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	iamv1alpha1 "go.miloapis.com/milo/pkg/apis/iam/v1alpha1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+)
+
+func policyBinding(subjects ...iamv1alpha1.Subject) *iamv1alpha1.PolicyBinding {
+	return &iamv1alpha1.PolicyBinding{
+		ObjectMeta: metav1.ObjectMeta{Name: "binding", Namespace: "organization-acme"},
+		Spec: iamv1alpha1.PolicyBindingSpec{
+			RoleRef:  iamv1alpha1.RoleReference{Name: "viewer"},
+			Subjects: subjects,
+			ResourceSelector: iamv1alpha1.ResourceSelector{
+				ResourceKind: &iamv1alpha1.ResourceKind{APIGroup: "resourcemanager.miloapis.com", Kind: "Organization"},
+			},
+		},
+	}
+}
+
+func TestPolicyBindingMutator_Default(t *testing.T) {
+	user := &iamv1alpha1.User{
+		ObjectMeta: metav1.ObjectMeta{Name: "alice", UID: types.UID("user-uid-1")},
+	}
+	group := &iamv1alpha1.Group{
+		ObjectMeta: metav1.ObjectMeta{Name: "loaders", Namespace: "organization-acme", UID: types.UID("group-uid-1")},
+	}
+	sa := &iamv1alpha1.ServiceAccount{
+		ObjectMeta: metav1.ObjectMeta{Name: "robot", UID: types.UID("sa-uid-1")},
+	}
+
+	tests := map[string]struct {
+		preObjects  []client.Object
+		subjects    []iamv1alpha1.Subject
+		expectError bool
+		contains    string
+		assertUID   func(t *testing.T, subjects []iamv1alpha1.Subject)
+	}{
+		"resolves user uid from name": {
+			preObjects: []client.Object{user},
+			subjects:   []iamv1alpha1.Subject{{Kind: "User", Name: "alice"}},
+			assertUID: func(t *testing.T, s []iamv1alpha1.Subject) {
+				assert.Equal(t, "user-uid-1", s[0].UID)
+			},
+		},
+		"resolves group uid from name and namespace": {
+			preObjects: []client.Object{group},
+			subjects:   []iamv1alpha1.Subject{{Kind: "Group", Name: "loaders", Namespace: "organization-acme"}},
+			assertUID: func(t *testing.T, s []iamv1alpha1.Subject) {
+				assert.Equal(t, "group-uid-1", s[0].UID)
+			},
+		},
+		"resolves serviceaccount uid from name": {
+			preObjects: []client.Object{sa},
+			subjects:   []iamv1alpha1.Subject{{Kind: "ServiceAccount", Name: "robot"}},
+			assertUID: func(t *testing.T, s []iamv1alpha1.Subject) {
+				assert.Equal(t, "sa-uid-1", s[0].UID)
+			},
+		},
+		"leaves an already-set uid untouched": {
+			preObjects: []client.Object{user},
+			subjects:   []iamv1alpha1.Subject{{Kind: "User", Name: "alice", UID: "preset-uid"}},
+			assertUID: func(t *testing.T, s []iamv1alpha1.Subject) {
+				assert.Equal(t, "preset-uid", s[0].UID)
+			},
+		},
+		"skips system groups": {
+			subjects: []iamv1alpha1.Subject{{Kind: "Group", Name: "system:authenticated-users"}},
+			assertUID: func(t *testing.T, s []iamv1alpha1.Subject) {
+				assert.Empty(t, s[0].UID)
+			},
+		},
+		"resolves multiple subjects": {
+			preObjects: []client.Object{user, group},
+			subjects: []iamv1alpha1.Subject{
+				{Kind: "User", Name: "alice"},
+				{Kind: "Group", Name: "loaders", Namespace: "organization-acme"},
+			},
+			assertUID: func(t *testing.T, s []iamv1alpha1.Subject) {
+				assert.Equal(t, "user-uid-1", s[0].UID)
+				assert.Equal(t, "group-uid-1", s[1].UID)
+			},
+		},
+		"errors when the named user does not exist": {
+			subjects:    []iamv1alpha1.Subject{{Kind: "User", Name: "ghost"}},
+			expectError: true,
+			contains:    `User "ghost" not found`,
+		},
+		"errors when a group subject omits the namespace": {
+			subjects:    []iamv1alpha1.Subject{{Kind: "Group", Name: "loaders"}},
+			expectError: true,
+			contains:    "namespace is required",
+		},
+	}
+
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			cl := fake.NewClientBuilder().WithScheme(runtimeScheme).WithObjects(tc.preObjects...).Build()
+			mutator := &PolicyBindingMutator{client: cl}
+
+			pb := policyBinding(tc.subjects...)
+			err := mutator.Default(context.Background(), pb)
+
+			if tc.expectError {
+				require.Error(t, err)
+				assert.Contains(t, err.Error(), tc.contains)
+				return
+			}
+
+			require.NoError(t, err)
+			if tc.assertUID != nil {
+				tc.assertUID(t, pb.Spec.Subjects)
+			}
+		})
+	}
+}

test/iam/policybinding-subject-uid-resolution/chainsaw-test.yaml

@@ -0,0 +1,44 @@
+apiVersion: chainsaw.kyverno.io/v1alpha1
+kind: Test
+metadata:
+  name: policybinding-subject-uid-resolution
+spec:
+  description: |
+    Verifies the PolicyBinding mutating webhook resolves a subject's uid from
+    its name.
+
+    This test verifies:
+    - A PolicyBinding referencing a Group by name (no uid) has the Group's
+      metadata.uid stamped into the stored subject.
+    - A PolicyBinding referencing a non-existent Group is rejected at admission.
+    - A system group subject (name starting with "system:") is accepted and
+      left without a uid.
+
+  steps:
+  - name: resolve-group-uid
+    description: A name-only Group subject gets its uid resolved by the webhook
+    try:
+    - apply:
+        file: resources/group.yaml
+        outputs:
+          - name: grp
+            value: (@)
+    - apply:
+        file: resources/policy-binding.yaml
+    - assert:
+        file: resources/assert-policy-binding.yaml
+  - name: reject-missing-group
+    description: A subject naming a non-existent Group is rejected at admission
+    try:
+    - apply:
+        file: resources/policy-binding-missing-group.yaml
+        expect:
+        - check:
+            ($error != null): true
+  - name: allow-system-group-without-uid
+    description: A system group subject is accepted without a uid
+    try:
+    - apply:
+        file: resources/system-group-binding.yaml
+    - assert:
+        file: resources/assert-system-group-binding.yaml

test/iam/policybinding-subject-uid-resolution/resources/assert-policy-binding.yaml

@@ -0,0 +1,13 @@
+apiVersion: iam.miloapis.com/v1alpha1
+kind: PolicyBinding
+metadata:
+  name: name-only-group-binding
+  namespace: ($namespace)
+spec:
+  subjects:
+    # The webhook resolved the uid from the named Group, and it matches the
+    # uid the API server assigned the Group at create time.
+    - kind: Group
+      name: resolve-me-group
+      namespace: ($namespace)
+      uid: ($grp.metadata.uid)

test/iam/policybinding-subject-uid-resolution/resources/assert-system-group-binding.yaml

@@ -0,0 +1,11 @@
+apiVersion: iam.miloapis.com/v1alpha1
+kind: PolicyBinding
+metadata:
+  name: system-group-binding
+  namespace: ($namespace)
+spec:
+  # The binding was admitted with a uid-less system group subject: the webhook
+  # skipped resolution and the validating CEL rule permitted it.
+  subjects:
+    - kind: Group
+      name: system:authenticated-users

test/iam/policybinding-subject-uid-resolution/resources/group.yaml

@@ -0,0 +1,5 @@
+apiVersion: iam.miloapis.com/v1alpha1
+kind: Group
+metadata:
+  name: resolve-me-group
+  namespace: ($namespace)