Merge pull request #129 from appuio/proper-invitation-access-validati…

…on-using-sar

Validate invitation target access using SARs

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,12 @@ import (
 	"context"
 	"fmt"
 	"net/http"
-	"strings"
 
 	"go.uber.org/multierr"
+	authenticationv1 "k8s.io/api/authentication/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	authorizationv1 "k8s.io/kubernetes/pkg/apis/authorization/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
@@ -17,6 +20,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 +40,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 +63,74 @@ 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
+
+	// I could not find a way to create a SubjectAccessReview object with the client.
+	// `no kind "CreateOptions" is registered for the internal version of group "authorization.k8s.io" in scheme`
+	// even after installing the authorization scheme.
+	rawSAR := &unstructured.Unstructured{
+		Object: map[string]any{
+			"spec": map[string]any{
+				"resourceAttributes": ra,
+
+				"user":   user.Username,
+				"groups": user.Groups,
+				"uid":    user.UID,
+			},
+		}}
+	rawSAR.SetGroupVersionKind(authorizationv1.SchemeGroupVersion.WithKind("SubjectAccessReview"))
+
+	if err := c.Create(ctx, rawSAR); err != nil {
+		return fmt.Errorf("failed to create SubjectAccessReview: %w", err)
+	}
+
+	allowed, _, err := unstructured.NestedBool(rawSAR.Object, "status", "allowed")
+	if err != nil {
+		return fmt.Errorf("failed to get SubjectAccessReview status.allowed: %w", err)
+	}
 
-	if a.HasUser(prefix, user) {
-		return nil
+	if !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 fmt.Errorf("target %q.%q/%q in namespace %q is not allowed", target.APIGroup, target.Kind, target.Name, target.Namespace)
+	return nil
+}
+
+func mapTargetRefToResourceAttribute(c client.Client, target userv1.TargetRef) (map[string]any, 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 map[string]any{
+		"group":    target.APIGroup,
+		"version":  rm.Resource.Version,
+		"resource": rm.Resource.Resource,
+
+		"namespace": target.Namespace,
+		"name":      target.Name,
+	}, nil
 }