SecretsManager: Move actor/auth filtering layer to rest storage (#105414)

Co-authored-by: PoorlyDefinedBehaviour <brunotj2015@hotmail.com>
Co-authored-by: Matheus Macabu <macabu@users.noreply.github.com>

pkg/registry/apis/secret/register.go

@@ -133,7 +133,7 @@ func (b *SecretAPIBuilder) UpdateAPIGroupInfo(apiGroupInfo *genericapiserver.API
 		secureValueResource.StoragePath(): reststorage.NewSecureValueRest(b.secureValueMetadataStorage, secureValueResource),
 
 		// The `reststorage.KeeperRest` struct will implement interfaces for CRUDL operations on `keeper`.
-		keeperResource.StoragePath(): reststorage.NewKeeperRest(b.keeperMetadataStorage, keeperResource),
+		keeperResource.StoragePath(): reststorage.NewKeeperRest(b.keeperMetadataStorage, b.accessClient, keeperResource),
 	}
 
 	apiGroupInfo.VersionedResourcesStorageMap[secretv0alpha1.VERSION] = secureRestStorage

pkg/registry/apis/secret/reststorage/keeper_rest.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"strings"
 
+	claims "github.com/grafana/authlib/types"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/internalversion"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -36,13 +37,14 @@ var (
 // KeeperRest is an implementation of CRUDL operations on a `keeper` backed by TODO.
 type KeeperRest struct {
 	storage        contracts.KeeperMetadataStorage
+	accessClient   claims.AccessClient
 	resource       utils.ResourceInfo
 	tableConverter rest.TableConvertor
 }
 
 // NewKeeperRest is a returns a constructed `*KeeperRest`.
-func NewKeeperRest(storage contracts.KeeperMetadataStorage, resource utils.ResourceInfo) *KeeperRest {
-	return &KeeperRest{storage, resource, resource.TableConverter()}
+func NewKeeperRest(storage contracts.KeeperMetadataStorage, accessClient claims.AccessClient, resource utils.ResourceInfo) *KeeperRest {
+	return &KeeperRest{storage, accessClient, resource, resource.TableConverter()}
 }
 
 // New returns an empty `*Keeper` that is used by the `Create` method.
@@ -80,6 +82,21 @@ func (s *KeeperRest) List(ctx context.Context, options *internalversion.ListOpti
 		return nil, fmt.Errorf("missing namespace")
 	}
 
+	user, ok := claims.AuthInfoFrom(ctx)
+	if !ok {
+		return nil, fmt.Errorf("missing auth info in context")
+	}
+
+	hasPermissionFor, err := s.accessClient.Compile(ctx, user, claims.ListRequest{
+		Group:     secretv0alpha1.GROUP,
+		Resource:  secretv0alpha1.KeeperResourceInfo.GetName(),
+		Namespace: namespace,
+		Verb:      utils.VerbGet,
+	})
+	if err != nil {
+		return nil, fmt.Errorf("failed to compile checker: %w", err)
+	}
+
 	labelSelector := options.LabelSelector
 	if labelSelector == nil {
 		labelSelector = labels.Everything()
@@ -93,6 +110,11 @@ func (s *KeeperRest) List(ctx context.Context, options *internalversion.ListOpti
 	allowedKeepers := make([]secretv0alpha1.Keeper, 0)
 
 	for _, keeper := range keepersList {
+		// Check whether the user has permission to access this specific Keeper in the namespace.
+		if !hasPermissionFor(keeper.Name, "") {
+			continue
+		}
+
 		if labelSelector.Matches(labels.Set(keeper.Labels)) {
 			allowedKeepers = append(allowedKeepers, keeper)
 		}
@@ -134,11 +156,16 @@ func (s *KeeperRest) Create(
 		return nil, fmt.Errorf("expected Keeper for create")
 	}
 
+	user, ok := claims.AuthInfoFrom(ctx)
+	if !ok {
+		return nil, fmt.Errorf("missing auth info in context")
+	}
+
 	if err := createValidation(ctx, obj); err != nil {
 		return nil, err
 	}
 
-	createdKeeper, err := s.storage.Create(ctx, kp, "todo-user")
+	createdKeeper, err := s.storage.Create(ctx, kp, user.GetUID())
 	if err != nil {
 		var kErr xkube.ErrorLister
 		if errors.As(err, &kErr) {
@@ -161,6 +188,11 @@ func (s *KeeperRest) Update(
 	forceAllowCreate bool,
 	options *metav1.UpdateOptions,
 ) (runtime.Object, bool, error) {
+	user, ok := claims.AuthInfoFrom(ctx)
+	if !ok {
+		return nil, false, fmt.Errorf("missing auth info in context")
+	}
+
 	oldObj, err := s.Get(ctx, name, &metav1.GetOptions{})
 	if err != nil {
 		return nil, false, err
@@ -189,7 +221,7 @@ func (s *KeeperRest) Update(
 	newKeeper.Annotations = xkube.CleanAnnotations(newKeeper.Annotations)
 
 	// Current implementation replaces everything passed in the spec, so it is not a PATCH. Do we want/need to support that?
-	updatedKeeper, err := s.storage.Update(ctx, newKeeper, "todo-user")
+	updatedKeeper, err := s.storage.Update(ctx, newKeeper, user.GetUID())
 	if err != nil {
 		var kErr xkube.ErrorLister
 		if errors.As(err, &kErr) {

pkg/registry/apis/secret/reststorage/secure_value_rest.go

@@ -7,6 +7,7 @@ import (
 	"strconv"
 	"strings"
 
+	claims "github.com/grafana/authlib/types"
 	"k8s.io/apimachinery/pkg/apis/meta/internalversion"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/fields"
@@ -136,7 +137,7 @@ func (s *SecureValueRest) Create(
 	ctx context.Context,
 	obj runtime.Object,
 	createValidation rest.ValidateObjectFunc,
-	options *metav1.CreateOptions,
+	_ *metav1.CreateOptions,
 ) (runtime.Object, error) {
 	sv, ok := obj.(*secretv0alpha1.SecureValue)
 	if !ok {
@@ -147,12 +148,17 @@ func (s *SecureValueRest) Create(
 		return nil, err
 	}
 
-	createdSecureValue, err := s.storage.Create(ctx, sv, "todo-user")
+	user, ok := claims.AuthInfoFrom(ctx)
+	if !ok {
+		return nil, fmt.Errorf("missing auth info in context")
+	}
+
+	createdSecureValueMetadata, err := s.storage.Create(ctx, sv, user.GetUID())
 	if err != nil {
-		return nil, fmt.Errorf("failed to create secure value: %w", err)
+		return nil, fmt.Errorf("creating secure value %+w", err)
 	}
 
-	return createdSecureValue, nil
+	return createdSecureValueMetadata, nil
 }
 
 // Update a `securevalue`'s `value`. The second return parameter indicates whether the resource was newly created.
@@ -161,10 +167,10 @@ func (s *SecureValueRest) Update(
 	ctx context.Context,
 	name string,
 	objInfo rest.UpdatedObjectInfo,
-	createValidation rest.ValidateObjectFunc,
+	_ rest.ValidateObjectFunc,
 	updateValidation rest.ValidateObjectUpdateFunc,
-	forceAllowCreate bool,
-	options *metav1.UpdateOptions,
+	_forceAllowCreate bool,
+	_ *metav1.UpdateOptions,
 ) (runtime.Object, bool, error) {
 	oldObj, err := s.Get(ctx, name, &metav1.GetOptions{})
 	if err != nil {
@@ -190,16 +196,20 @@ func (s *SecureValueRest) Update(
 	// TODO: do we need to do this here again? Probably not, but double-check!
 	newSecureValue.Annotations = xkube.CleanAnnotations(newSecureValue.Annotations)
 
+	user, ok := claims.AuthInfoFrom(ctx)
+	if !ok {
+		return nil, false, fmt.Errorf("missing auth info in context")
+	}
+
 	// Current implementation replaces everything passed in the spec, so it is not a PATCH. Do we want/need to support that?
-	updatedSecureValue, err := s.storage.Update(ctx, newSecureValue, "todo-user")
+	updatedSecureValueMetadata, err := s.storage.Update(ctx, newSecureValue, user.GetUID())
 	if err != nil {
-		return nil, false, fmt.Errorf("failed to update secure value: %w", err)
+		return updatedSecureValueMetadata, false, fmt.Errorf("updating secure value metadata: %+w", err)
 	}
 
-	return updatedSecureValue, false, nil
+	return updatedSecureValueMetadata, false, nil
 }
 
-// Delete calls the inner `store` (persistence) in order to delete the `securevalue`.
 // The second return parameter `bool` indicates whether the delete was instant or not. It always is for `securevalues`.
 func (s *SecureValueRest) Delete(ctx context.Context, name string, _ rest.ValidateObjectFunc, _ *metav1.DeleteOptions) (runtime.Object, bool, error) {
 	namespace, ok := request.NamespaceFrom(ctx)