Validate invitation target access using SARs

Test setup (at least with fake client) is pretty painful.
Looks like we found a case for envtest.

Makefile

@@ -35,7 +35,7 @@ generate: ## Generate manifests e.g. CRD, RBAC etc.
 	# Generate CRDs
 	go run sigs.k8s.io/controller-tools/cmd/controller-gen webhook paths="./..." output:crd:artifacts:config=$(CRD_ROOT_DIR)/v1/base crd:crdVersions=v1
 	go run sigs.k8s.io/controller-tools/cmd/controller-gen rbac:roleName=control-api-apiserver paths="./apiserver/...;./apis/..." output:artifacts:config=config/rbac/apiserver
-	go run sigs.k8s.io/controller-tools/cmd/controller-gen rbac:roleName=control-api-controller paths="./controllers/..." output:artifacts:config=config/rbac/controller
+	go run sigs.k8s.io/controller-tools/cmd/controller-gen rbac:roleName=control-api-controller paths="./controllers/...;./webhooks/..." output:artifacts:config=config/rbac/controller
 
 .PHONY: crd
 crd: generate ## Generate CRD to file

config/rbac/controller/role.yaml

@@ -73,6 +73,12 @@ rules:
   - get
   - patch
   - update
+- apiGroups:
+  - authorization.k8s.io
+  resources:
+  - subjectaccessreviews
+  verbs:
+  - create
 - apiGroups:
   - billing.appuio.io
   resources:

controllers/targetref/targetref.go

@@ -14,6 +14,15 @@ import (
 // GetTarget returns the target object for the given TargetRef.
 // Returns an error if the target is not supported.
 func GetTarget(ctx context.Context, c client.Client, target userv1.TargetRef) (client.Object, error) {
+	obj, err := NewObjectFromRef(target)
+	if err != nil {
+		return nil, err
+	}
+	return obj, c.Get(ctx, client.ObjectKey{Name: target.Name, Namespace: target.Namespace}, obj)
+}
+
+// NewObjectFromRef returns a new object for the given TargetRef or an error if the target is not supported.
+func NewObjectFromRef(target userv1.TargetRef) (client.Object, error) {
 	var obj client.Object
 	switch {
 	case target.APIGroup == "appuio.io" && target.Kind == "OrganizationMembers":
@@ -28,8 +37,7 @@ func GetTarget(ctx context.Context, c client.Client, target userv1.TargetRef) (c
 		return nil, fmt.Errorf("unsupported target %q.%q", target.APIGroup, target.Kind)
 	}
 
-	err := c.Get(ctx, client.ObjectKey{Name: target.Name, Namespace: target.Namespace}, obj)
-	return obj, err
+	return obj, nil
 }
 
 // UserAccessor is an interface for accessing users from objects supported by this project.

go.mod

@@ -17,15 +17,14 @@ require (
 	k8s.io/apiserver v0.26.2
 	k8s.io/client-go v0.26.2
 	k8s.io/klog/v2 v2.90.1
+	k8s.io/kubernetes v1.26.2
 	sigs.k8s.io/apiserver-runtime v1.1.2-0.20221226021050-33c901856927
 	sigs.k8s.io/controller-runtime v0.14.5
 	sigs.k8s.io/controller-tools v0.11.3
 	sigs.k8s.io/kind v0.17.0
 	sigs.k8s.io/kustomize/kustomize/v5 v5.0.0
 )
 
-require github.com/gorilla/mux v1.8.0 // indirect
-
 require (
 	github.com/BurntSushi/toml v1.2.1 // indirect
 	github.com/NYTimes/gziphandler v1.1.1 // indirect
@@ -61,6 +60,7 @@ require (
 	github.com/google/gofuzz v1.2.0 // indirect
 	github.com/google/safetext v0.0.0-20230103122802-988f86758b18 // indirect
 	github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 // indirect
+	github.com/gorilla/mux v1.8.0 // indirect
 	github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0 // indirect
 	github.com/grpc-ecosystem/grpc-gateway/v2 v2.15.0 // indirect
 	github.com/imdario/mergo v0.3.13 // indirect

go.sum

@@ -803,6 +803,8 @@ k8s.io/kms v0.26.2 h1:GM1gg3tFK3OUU/QQFi93yGjG3lJT8s8l3Wkn2+VxBLM=
 k8s.io/kms v0.26.2/go.mod h1:69qGnf1NsFOQP07fBYqNLZklqEHSJF024JqYCaeVxHg=
 k8s.io/kube-openapi v0.0.0-20230109183929-3758b55a6596 h1:8cNCQs+WqqnSpZ7y0LMQPKD+RZUHU17VqLPMW3qxnxc=
 k8s.io/kube-openapi v0.0.0-20230109183929-3758b55a6596/go.mod h1:/BYxry62FuDzmI+i9B+X2pqfySRmSOW2ARmj5Zbqhj0=
+k8s.io/kubernetes v1.26.2 h1:6Ve0nzlF2noVXf9jMHSJgbRZC0EkyOV22GYEv1K7MZI=
+k8s.io/kubernetes v1.26.2/go.mod h1:cv07eVU5+kF6ibpVtAvOGjIBsrfgevQL4ORK85/oqWc=
 k8s.io/utils v0.0.0-20221128185143-99ec85e7a448 h1:KTgPnR10d5zhztWptI952TNtt/4u5h3IzDXkdIMuo2Y=
 k8s.io/utils v0.0.0-20221128185143-99ec85e7a448/go.mod h1:OLgZIPagt7ERELqWJFomSt595RzquPNLL48iOWgYOg0=
 rsc.io/binaryregexp v0.2.0/go.mod h1:qTv7/COck+e2FymRvadv62gMdZztPaShugOCi3I+8D8=

webhooks/invitation_webhook.go

@@ -4,9 +4,13 @@ import (
 	"context"
 	"fmt"
 	"net/http"
-	"strings"
 
+	"github.com/google/uuid"
 	"go.uber.org/multierr"
+	authenticationv1 "k8s.io/api/authentication/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/kubernetes/pkg/apis/authorization"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
@@ -17,6 +21,8 @@ import (
 
 // +kubebuilder:webhook:path=/validate-user-appuio-io-v1-invitation,mutating=false,failurePolicy=fail,groups="user.appuio.io",resources=invitations,verbs=create;update,versions=v1,name=validate-invitations.user.appuio.io,admissionReviewVersions=v1,sideEffects=None
 
+// +kubebuilder:rbac:groups=authorization.k8s.io,resources=subjectaccessreviews,verbs=create
+
 // InvitationValidator holds context for the validating admission webhook for users.appuio.io
 type InvitationValidator struct {
 	client  client.Client
@@ -35,18 +41,12 @@ func (v *InvitationValidator) Handle(ctx context.Context, req admission.Request)
 	}
 	log.V(1).WithValues("invitation", inv).Info("Validating")
 
-	username := req.UserInfo.Username
-	if !strings.HasPrefix(username, v.UsernamePrefix) {
-		return admission.Denied(fmt.Sprintf("Invalid username: only usernames with prefix %q are accepted, got username %q", v.UsernamePrefix, username))
-	}
-	username = strings.TrimPrefix(username, v.UsernamePrefix)
-
 	authErrors := make([]error, 0, len(inv.Spec.TargetRefs))
 	for _, target := range inv.Spec.TargetRefs {
-		authErrors = append(authErrors, authorizeTarget(ctx, v.client, username, v.UsernamePrefix, target))
+		authErrors = append(authErrors, authorizeTarget(ctx, v.client, req.UserInfo, target))
 	}
 	if err := multierr.Combine(authErrors...); err != nil {
-		return admission.Denied(fmt.Sprintf("user %q is not allowed to invite to the targets: %s", username, err))
+		return admission.Denied(fmt.Sprintf("user %q is not allowed to invite to the targets: %s", req.UserInfo.Username, err))
 	}
 
 	return admission.Allowed("target refs are valid")
@@ -64,20 +64,65 @@ func (v *InvitationValidator) InjectClient(c client.Client) error {
 	return nil
 }
 
-func authorizeTarget(ctx context.Context, c client.Client, user, prefix string, target userv1.TargetRef) error {
-	o, err := targetref.GetTarget(ctx, c, target)
+func authorizeTarget(ctx context.Context, c client.Client, user authenticationv1.UserInfo, target userv1.TargetRef) error {
+	// Check if the target references a supported resource
+	_, err := targetref.NewObjectFromRef(target)
 	if err != nil {
 		return err
 	}
 
-	a, err := targetref.NewUserAccessor(o)
+	return canEditTarget(ctx, c, user, target)
+}
+
+// canEditTarget checks if the user is allowed to edit the target.
+// it does so by creating a SubjectAccessReview to `update` the resource and checking the response.
+func canEditTarget(ctx context.Context, c client.Client, user authenticationv1.UserInfo, target userv1.TargetRef) error {
+	const verb = "update"
+
+	ra, err := mapTargetRefToResourceAttribute(c, target)
 	if err != nil {
 		return err
 	}
+	ra.Verb = verb
+
+	rw := authorization.SubjectAccessReview{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: uuid.New().String(),
+		},
+		Spec: authorization.SubjectAccessReviewSpec{
+			ResourceAttributes: ra,
+			User:               user.Username,
+			Groups:             user.Groups,
+			UID:                user.UID,
+		},
+	}
+
+	if err := c.Create(ctx, &rw); err != nil {
+		return fmt.Errorf("failed to create SubjectAccessReview: %w", err)
+	}
+
+	if !rw.Status.Allowed {
+		return fmt.Errorf("%q on target %q.%q/%q in namespace %q is not allowed", verb, target.APIGroup, target.Kind, target.Name, target.Namespace)
+	}
+
+	return nil
+}
 
-	if a.HasUser(prefix, user) {
-		return nil
+func mapTargetRefToResourceAttribute(c client.Client, target userv1.TargetRef) (*authorization.ResourceAttributes, error) {
+	rm, err := c.RESTMapper().RESTMapping(schema.GroupKind{
+		Group: target.APIGroup,
+		Kind:  target.Kind,
+	})
+
+	if err != nil {
+		return nil, fmt.Errorf("failed to get REST mapping for %q.%q: %w", target.APIGroup, target.Kind, err)
 	}
 
-	return fmt.Errorf("target %q.%q/%q in namespace %q is not allowed", target.APIGroup, target.Kind, target.Name, target.Namespace)
+	return &authorization.ResourceAttributes{
+		Namespace: target.Namespace,
+		Group:     target.APIGroup,
+		Version:   rm.Resource.Version,
+		Resource:  rm.Resource.Resource,
+		Name:      target.Name,
+	}, nil
 }

webhooks/invitation_webhook_test.go

@@ -8,14 +8,16 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-
 	admissionv1 "k8s.io/api/admission/v1"
 	authenticationv1 "k8s.io/api/authentication/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
+	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
-	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
+	ktesting "k8s.io/client-go/testing"
+	"k8s.io/kubernetes/pkg/apis/authorization"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
@@ -100,20 +102,6 @@ func TestInvitationValidator_Handle(t *testing.T) {
 			errcode: http.StatusForbidden,
 		},
 
-		"OrganizationMembers not found, denied": {
-			requestUser: allowedUser,
-			targets: []userv1.TargetRef{
-				{
-					APIGroup:  "appuio.io",
-					Kind:      "OrganizationMembers",
-					Namespace: testOrg,
-					Name:      "members" + "-not-found",
-				},
-			},
-			allowed: false,
-			errcode: http.StatusForbidden,
-		},
-
 		"Team allowed": {
 			requestUser: allowedUser,
 			targets: []userv1.TargetRef{
@@ -142,20 +130,6 @@ func TestInvitationValidator_Handle(t *testing.T) {
 			errcode: http.StatusForbidden,
 		},
 
-		"Team not found, denied": {
-			requestUser: allowedUser,
-			targets: []userv1.TargetRef{
-				{
-					APIGroup:  "appuio.io",
-					Kind:      "Team",
-					Namespace: testOrg,
-					Name:      testTeam + "-not-found",
-				},
-			},
-			allowed: false,
-			errcode: http.StatusForbidden,
-		},
-
 		"ClusterRoleBinding allowed": {
 			requestUser: allowedUser,
 			targets: []userv1.TargetRef{
@@ -182,19 +156,6 @@ func TestInvitationValidator_Handle(t *testing.T) {
 			errcode: http.StatusForbidden,
 		},
 
-		"ClusterRoleBinding not found, denied": {
-			requestUser: allowedUser,
-			targets: []userv1.TargetRef{
-				{
-					APIGroup: rbacv1.GroupName,
-					Kind:     "ClusterRoleBinding",
-					Name:     testRoleName + "-not-found",
-				},
-			},
-			allowed: false,
-			errcode: http.StatusForbidden,
-		},
-
 		"RoleBinding allowed": {
 			requestUser: allowedUser,
 			targets: []userv1.TargetRef{
@@ -222,20 +183,6 @@ func TestInvitationValidator_Handle(t *testing.T) {
 			allowed: false,
 			errcode: http.StatusForbidden,
 		},
-
-		"RoleBinding not found, denied": {
-			requestUser: allowedUser,
-			targets: []userv1.TargetRef{
-				{
-					APIGroup:  rbacv1.GroupName,
-					Kind:      "RoleBinding",
-					Namespace: testOrg,
-					Name:      testRoleName + "-not-found",
-				},
-			},
-			allowed: false,
-			errcode: http.StatusForbidden,
-		},
 	}
 
 	for name, tc := range tests {
@@ -293,7 +240,7 @@ func TestInvitationValidator_Handle(t *testing.T) {
 				},
 			}
 
-			iv := prepareInvitationValidatorTest(t, &invitation, &org, &team, &rb, &crb)
+			iv := prepareInvitationValidatorTest(t, allowedUser, &invitation, &org, &team, &rb, &crb)
 
 			invJson, err := json.Marshal(invitation)
 			require.NoError(t, err)
@@ -336,19 +283,64 @@ func TestInvitationValidator_Handle(t *testing.T) {
 	}
 }
 
-func prepareInvitationValidatorTest(t *testing.T, initObjs ...client.Object) *InvitationValidator {
+func prepareInvitationValidatorTest(t *testing.T, sarAllowedUser string, initObjs ...client.Object) *InvitationValidator {
 	scheme := runtime.NewScheme()
-	utilruntime.Must(clientgoscheme.AddToScheme(scheme))
-	utilruntime.Must(orgv1.AddToScheme(scheme))
-	utilruntime.Must(controlv1.AddToScheme(scheme))
-	utilruntime.Must(userv1.AddToScheme(scheme))
+	require.NoError(t, clientgoscheme.AddToScheme(scheme))
+	require.NoError(t, orgv1.AddToScheme(scheme))
+	require.NoError(t, controlv1.AddToScheme(scheme))
+	require.NoError(t, userv1.AddToScheme(scheme))
+	require.NoError(t, authorization.AddToScheme(scheme))
 
 	decoder, err := admission.NewDecoder(scheme)
 	require.NoError(t, err)
 
+	// This would be auto-discovered by the real client
+	drm := meta.NewDefaultRESTMapper(
+		[]schema.GroupVersion{
+			{Group: "appuio.io", Version: "v1"},
+			{Group: "rbac.authorization.k8s.io", Version: "v1"},
+		},
+	)
+	drm.AddSpecific(schema.GroupVersionKind{
+		Group:   "appuio.io",
+		Version: "v1",
+		Kind:    "OrganizationMembers",
+	}, schema.GroupVersionResource{
+		Group:    "appuio.io",
+		Version:  "v1",
+		Resource: "organizationmembers",
+	}, schema.GroupVersionResource{
+		Group:    "appuio.io",
+		Version:  "v1",
+		Resource: "organizationmembers",
+	}, meta.RESTScopeNamespace)
+
+	drm.Add(schema.GroupVersionKind{
+		Group:   "appuio.io",
+		Version: "v1",
+		Kind:    "Team",
+	}, meta.RESTScopeNamespace)
+	drm.Add(schema.GroupVersionKind{
+		Group:   "rbac.authorization.k8s.io",
+		Version: "v1",
+		Kind:    "ClusterRoleBinding",
+	}, meta.RESTScopeRoot)
+	drm.Add(schema.GroupVersionKind{
+		Group:   "rbac.authorization.k8s.io",
+		Version: "v1",
+		Kind:    "RoleBinding",
+	}, meta.RESTScopeNamespace)
+
+	tr := subjectAccessReviewResponder{
+		ktesting.NewObjectTracker(scheme, clientgoscheme.Codecs.UniversalDecoder()),
+		sarAllowedUser,
+	}
+
 	client := fake.NewClientBuilder().
 		WithScheme(scheme).
 		WithObjects(initObjs...).
+		WithRESTMapper(drm).
+		WithObjectTracker(tr).
 		Build()
 
 	iv := &InvitationValidator{}
@@ -357,3 +349,19 @@ func prepareInvitationValidatorTest(t *testing.T, initObjs ...client.Object) *In
 
 	return iv
 }
+
+// subjectAccessReviewResponder is a wrapper for testing.ObjectTracker that responds to SubjectAccessReview create requests
+// and allows or denies the request based on the allowedUser name.
+type subjectAccessReviewResponder struct {
+	ktesting.ObjectTracker
+
+	allowedUser string
+}
+
+func (r subjectAccessReviewResponder) Create(gvr schema.GroupVersionResource, obj runtime.Object, ns string) error {
+	if sar, ok := obj.(*authorization.SubjectAccessReview); ok {
+		sar.Status.Allowed = sar.Spec.User == r.allowedUser
+		return nil
+	}
+	return r.ObjectTracker.Create(gvr, obj, ns)
+}