From 4cf02cbe37674cccfecf83e8b6e48044759d4f07 Mon Sep 17 00:00:00 2001 From: Sebastian Widmer Date: Fri, 17 Mar 2023 22:18:34 +0100 Subject: [PATCH] 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 | 2 +- config/rbac/controller/role.yaml | 6 ++ controllers/targetref/targetref.go | 12 ++- go.mod | 4 +- go.sum | 2 + webhooks/invitation_webhook.go | 75 ++++++++++++---- webhooks/invitation_webhook_test.go | 134 +++++++++++++++------------- 7 files changed, 152 insertions(+), 83 deletions(-) diff --git a/Makefile b/Makefile index a868b5f9..6600dbff 100644 --- a/Makefile +++ b/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 diff --git a/config/rbac/controller/role.yaml b/config/rbac/controller/role.yaml index 5adb39ac..db2d5d69 100644 --- a/config/rbac/controller/role.yaml +++ b/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: diff --git a/controllers/targetref/targetref.go b/controllers/targetref/targetref.go index d6dd626e..80a21473 100644 --- a/controllers/targetref/targetref.go +++ b/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. diff --git a/go.mod b/go.mod index 26d77dd7..fc97f84b 100644 --- a/go.mod +++ b/go.mod @@ -17,6 +17,7 @@ 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 @@ -24,8 +25,6 @@ require ( 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 diff --git a/go.sum b/go.sum index 5ca2bead..536f72cc 100644 --- a/go.sum +++ b/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= diff --git a/webhooks/invitation_webhook.go b/webhooks/invitation_webhook.go index b78be290..80c8176b 100644 --- a/webhooks/invitation_webhook.go +++ b/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 } diff --git a/webhooks/invitation_webhook_test.go b/webhooks/invitation_webhook_test.go index 72ef3c0b..9e25f444 100644 --- a/webhooks/invitation_webhook_test.go +++ b/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) +}