From 6a5e52fa30e1f29306c2ca77a75bb18a67516ebb Mon Sep 17 00:00:00 2001 From: Evan Vetere Date: Mon, 15 Jun 2026 16:39:10 -0400 Subject: [PATCH 1/2] feat(iam): resolve PolicyBinding subject uid from name Add a mutating/defaulting webhook on PolicyBinding that resolves a subject's metadata.uid from its name when the uid is omitted. The webhook looks up the named User, Group, or ServiceAccount and stamps its current uid into the subject before the validating CEL rule runs. This makes Group/User/ServiceAccount bindings declaratively committable: a Group and its PolicyBinding can be applied together by GitOps without an out-of-band render-after-reconcile step to read the uid back. It also centralizes the imperative uid read-back that internal controllers already perform. The instance-pinning guarantee is preserved. The webhook stamps the current uid, so a delete+recreate of a same-named subject yields a different uid and the binding no longer matches until re-applied. System groups (name starting with "system:") have no backing object and are left without a uid, matching the existing CEL exemption. Subjects that already carry a uid are untouched. Closes #652 Co-Authored-By: Claude Opus 4.8 (1M context) --- Taskfile.yaml | 2 +- .../controller-manager/controllermanager.go | 4 + .../core-control-plane/rbac/role.yaml | 1 + config/webhook/manifests.yaml | 22 +++ .../iam/v1alpha1/policybinding_webhook.go | 128 ++++++++++++++++++ .../v1alpha1/policybinding_webhook_test.go | 124 +++++++++++++++++ 6 files changed, 280 insertions(+), 1 deletion(-) create mode 100644 internal/webhooks/iam/v1alpha1/policybinding_webhook.go create mode 100644 internal/webhooks/iam/v1alpha1/policybinding_webhook_test.go diff --git a/Taskfile.yaml b/Taskfile.yaml index b95f48e4..03d2048f 100644 --- a/Taskfile.yaml +++ b/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: diff --git a/cmd/milo/controller-manager/controllermanager.go b/cmd/milo/controller-manager/controllermanager.go index cee6c3a9..73ed013c 100644 --- a/cmd/milo/controller-manager/controllermanager.go +++ b/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. diff --git a/config/controller-manager/overlays/core-control-plane/rbac/role.yaml b/config/controller-manager/overlays/core-control-plane/rbac/role.yaml index cc95140d..abe4c7f9 100644 --- a/config/controller-manager/overlays/core-control-plane/rbac/role.yaml +++ b/config/controller-manager/overlays/core-control-plane/rbac/role.yaml @@ -137,6 +137,7 @@ rules: - iam.miloapis.com resources: - roles + - serviceaccounts - userdeactivations verbs: - get diff --git a/config/webhook/manifests.yaml b/config/webhook/manifests.yaml index 64f46d8e..e59827dc 100644 --- a/config/webhook/manifests.yaml +++ b/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 diff --git a/internal/webhooks/iam/v1alpha1/policybinding_webhook.go b/internal/webhooks/iam/v1alpha1/policybinding_webhook.go new file mode 100644 index 00000000..a8ae7e9d --- /dev/null +++ b/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)) +} diff --git a/internal/webhooks/iam/v1alpha1/policybinding_webhook_test.go b/internal/webhooks/iam/v1alpha1/policybinding_webhook_test.go new file mode 100644 index 00000000..f0e7b93c --- /dev/null +++ b/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) + } + }) + } +} From bc87f8af108e451be88b967f7e37014acbdbe870 Mon Sep 17 00:00:00 2001 From: Evan Vetere Date: Mon, 15 Jun 2026 17:21:41 -0400 Subject: [PATCH 2/2] test(iam): e2e for PolicyBinding subject uid resolution Add a Chainsaw test exercising the PolicyBinding mutating webhook against a running Milo API server: - A name-only Group subject (no uid) gets the Group's metadata.uid stamped into the stored binding. - A subject naming a non-existent Group is rejected at admission. - A system group subject is admitted without a uid. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../chainsaw-test.yaml | 44 +++++++++++++++++++ .../resources/assert-policy-binding.yaml | 13 ++++++ .../assert-system-group-binding.yaml | 11 +++++ .../resources/group.yaml | 5 +++ .../policy-binding-missing-group.yaml | 19 ++++++++ .../resources/policy-binding.yaml | 20 +++++++++ .../resources/system-group-binding.yaml | 18 ++++++++ 7 files changed, 130 insertions(+) create mode 100644 test/iam/policybinding-subject-uid-resolution/chainsaw-test.yaml create mode 100644 test/iam/policybinding-subject-uid-resolution/resources/assert-policy-binding.yaml create mode 100644 test/iam/policybinding-subject-uid-resolution/resources/assert-system-group-binding.yaml create mode 100644 test/iam/policybinding-subject-uid-resolution/resources/group.yaml create mode 100644 test/iam/policybinding-subject-uid-resolution/resources/policy-binding-missing-group.yaml create mode 100644 test/iam/policybinding-subject-uid-resolution/resources/policy-binding.yaml create mode 100644 test/iam/policybinding-subject-uid-resolution/resources/system-group-binding.yaml diff --git a/test/iam/policybinding-subject-uid-resolution/chainsaw-test.yaml b/test/iam/policybinding-subject-uid-resolution/chainsaw-test.yaml new file mode 100644 index 00000000..8d525829 --- /dev/null +++ b/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 diff --git a/test/iam/policybinding-subject-uid-resolution/resources/assert-policy-binding.yaml b/test/iam/policybinding-subject-uid-resolution/resources/assert-policy-binding.yaml new file mode 100644 index 00000000..e67edba5 --- /dev/null +++ b/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) diff --git a/test/iam/policybinding-subject-uid-resolution/resources/assert-system-group-binding.yaml b/test/iam/policybinding-subject-uid-resolution/resources/assert-system-group-binding.yaml new file mode 100644 index 00000000..e6517e4b --- /dev/null +++ b/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 diff --git a/test/iam/policybinding-subject-uid-resolution/resources/group.yaml b/test/iam/policybinding-subject-uid-resolution/resources/group.yaml new file mode 100644 index 00000000..6954dbac --- /dev/null +++ b/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) diff --git a/test/iam/policybinding-subject-uid-resolution/resources/policy-binding-missing-group.yaml b/test/iam/policybinding-subject-uid-resolution/resources/policy-binding-missing-group.yaml new file mode 100644 index 00000000..d896c5d8 --- /dev/null +++ b/test/iam/policybinding-subject-uid-resolution/resources/policy-binding-missing-group.yaml @@ -0,0 +1,19 @@ +apiVersion: iam.miloapis.com/v1alpha1 +kind: PolicyBinding +metadata: + name: missing-group-binding + namespace: ($namespace) +spec: + roleRef: + name: resolve-me-role + namespace: ($namespace) + # No Group named "does-not-exist" exists, so the webhook cannot resolve a + # uid and must reject the request. + subjects: + - kind: Group + name: does-not-exist + namespace: ($namespace) + resourceSelector: + resourceKind: + apiGroup: resourcemanager.miloapis.com + kind: Organization diff --git a/test/iam/policybinding-subject-uid-resolution/resources/policy-binding.yaml b/test/iam/policybinding-subject-uid-resolution/resources/policy-binding.yaml new file mode 100644 index 00000000..5f15cdf4 --- /dev/null +++ b/test/iam/policybinding-subject-uid-resolution/resources/policy-binding.yaml @@ -0,0 +1,20 @@ +apiVersion: iam.miloapis.com/v1alpha1 +kind: PolicyBinding +metadata: + name: name-only-group-binding + namespace: ($namespace) +spec: + roleRef: + name: resolve-me-role + namespace: ($namespace) + # The Group subject is referenced by name only, with no uid. The mutating + # webhook must resolve the Group's metadata.uid before the validating CEL + # rule runs. + subjects: + - kind: Group + name: resolve-me-group + namespace: ($namespace) + resourceSelector: + resourceKind: + apiGroup: resourcemanager.miloapis.com + kind: Organization diff --git a/test/iam/policybinding-subject-uid-resolution/resources/system-group-binding.yaml b/test/iam/policybinding-subject-uid-resolution/resources/system-group-binding.yaml new file mode 100644 index 00000000..2ca93251 --- /dev/null +++ b/test/iam/policybinding-subject-uid-resolution/resources/system-group-binding.yaml @@ -0,0 +1,18 @@ +apiVersion: iam.miloapis.com/v1alpha1 +kind: PolicyBinding +metadata: + name: system-group-binding + namespace: ($namespace) +spec: + roleRef: + name: resolve-me-role + namespace: ($namespace) + # System groups have no backing object. The webhook leaves them without a + # uid and the validating CEL rule permits this. + subjects: + - kind: Group + name: system:authenticated-users + resourceSelector: + resourceKind: + apiGroup: resourcemanager.miloapis.com + kind: Organization