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) + } + }) + } +} 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