From f7e55f2c5dfe46ba3c71232c5ee24f7b77c107a3 Mon Sep 17 00:00:00 2001 From: Matheus Macabu Date: Mon, 21 Jul 2025 11:09:07 +0200 Subject: [PATCH] Secrets: Add authz checks for the single-tenant SecureValue client (#108216) --- .../apis/secret/secure_value_client.go | 53 ++++++++++++++- .../apis/secret/secure_value_client_test.go | 67 ++++++++++++++++++- .../apis/secret/testutils/testutils.go | 6 +- pkg/server/wire_gen.go | 4 +- 4 files changed, 124 insertions(+), 6 deletions(-) diff --git a/pkg/registry/apis/secret/secure_value_client.go b/pkg/registry/apis/secret/secure_value_client.go index 612191d3d28..3d7edfe0e8d 100644 --- a/pkg/registry/apis/secret/secure_value_client.go +++ b/pkg/registry/apis/secret/secure_value_client.go @@ -5,17 +5,21 @@ import ( "fmt" claims "github.com/grafana/authlib/types" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/watch" "k8s.io/apiserver/pkg/admission" + "k8s.io/apiserver/pkg/authorization/authorizer" "k8s.io/client-go/dynamic" secretv1beta1 "github.com/grafana/grafana/apps/secret/pkg/apis/secret/v1beta1" + "github.com/grafana/grafana/pkg/apimachinery/utils" "github.com/grafana/grafana/pkg/registry/apis/secret/contracts" "github.com/grafana/grafana/pkg/registry/apis/secret/xkube" + authsvc "github.com/grafana/grafana/pkg/services/apiserver/auth/authorizer" ) // SecureValueClient is a CRUD client for the secure value API. @@ -25,14 +29,16 @@ type secureValueClient struct { namespace string service contracts.SecureValueService validator contracts.SecureValueValidator + access authorizer.Authorizer } var _ SecureValueClient = &secureValueClient{} -func ProvideSecureValueClient(service contracts.SecureValueService, validator contracts.SecureValueValidator) SecureValueClient { +func ProvideSecureValueClient(service contracts.SecureValueService, validator contracts.SecureValueValidator, access claims.AccessClient) SecureValueClient { return &secureValueClient{ service: service, validator: validator, + access: authsvc.NewResourceAuthorizer(access), } } @@ -62,6 +68,10 @@ func (c *secureValueClient) Create(ctx context.Context, obj *unstructured.Unstru return nil, fmt.Errorf("namespace is required") } + if err := c.checkAccess(ctx, obj.GetName(), utils.VerbCreate); err != nil { + return nil, err + } + sv, err := fromUnstructured(obj) if err != nil { return nil, err @@ -96,6 +106,10 @@ func (c *secureValueClient) Get(ctx context.Context, name string, _ metav1.GetOp return nil, fmt.Errorf("name is required") } + if err := c.checkAccess(ctx, name, utils.VerbGet); err != nil { + return nil, err + } + sv, err := c.service.Read(ctx, xkube.Namespace(c.namespace), name) if err != nil { return nil, err @@ -110,6 +124,10 @@ func (c *secureValueClient) Update(ctx context.Context, obj *unstructured.Unstru return nil, fmt.Errorf("namespace is required") } + if err := c.checkAccess(ctx, obj.GetName(), utils.VerbUpdate); err != nil { + return nil, err + } + oldUnstructured, err := c.Get(ctx, obj.GetName(), metav1.GetOptions{}) if err != nil { return nil, err @@ -154,6 +172,10 @@ func (c *secureValueClient) Delete(ctx context.Context, name string, _ metav1.De return fmt.Errorf("name is required") } + if err := c.checkAccess(ctx, name, utils.VerbDelete); err != nil { + return err + } + _, err := c.service.Delete(ctx, xkube.Namespace(c.namespace), name) return err } @@ -164,6 +186,10 @@ func (c *secureValueClient) List(ctx context.Context, _ metav1.ListOptions) (*un return nil, fmt.Errorf("namespace is required") } + if err := c.checkAccess(ctx, "", utils.VerbList); err != nil { + return nil, err + } + list, err := c.service.List(ctx, xkube.Namespace(c.namespace)) if err != nil { return nil, err @@ -214,6 +240,31 @@ func (c *secureValueClient) ApplyStatus(_ context.Context, _ string, _ *unstruct return nil, fmt.Errorf("applyStatus is not supported") } +func (c *secureValueClient) checkAccess(ctx context.Context, name, verb string) error { + gr := secretv1beta1.SecureValuesResourceInfo.GroupResource() + + decision, reason, err := c.access.Authorize(ctx, authorizer.AttributesRecord{ + Verb: verb, + Namespace: c.namespace, + APIGroup: secretv1beta1.APIGroup, + APIVersion: secretv1beta1.APIVersion, + Resource: gr.Resource, + Subresource: "", + Name: name, + ResourceRequest: true, + }) + + if err != nil { + return apierrors.NewForbidden(gr, name, fmt.Errorf("failed to check access: %w", err)) + } + + if decision != authorizer.DecisionAllow { + return apierrors.NewForbidden(gr, name, fmt.Errorf("no access to %s: %s", verb, reason)) + } + + return nil +} + func toUnstructured(sv *secretv1beta1.SecureValue) (*unstructured.Unstructured, error) { unstructuredObj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(sv) if err != nil { diff --git a/pkg/registry/apis/secret/secure_value_client_test.go b/pkg/registry/apis/secret/secure_value_client_test.go index 3f67763c329..d4aaaf5f91c 100644 --- a/pkg/registry/apis/secret/secure_value_client_test.go +++ b/pkg/registry/apis/secret/secure_value_client_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/stretchr/testify/require" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/utils/ptr" @@ -21,11 +22,15 @@ func TestIntegration_SecureValueClient_CRUD(t *testing.T) { client := ProvideSecureValueClient( setup.SecureValueService, validator, + setup.AccessClient, ) ns := "stacks-1234" ctx := testutils.CreateUserAuthContext(t.Context(), ns, map[string][]string{ - "securevalues:read": {"securevalues:uid:*"}, + "securevalues:create": {"securevalues:uid:*"}, + "securevalues:read": {"securevalues:uid:*"}, + "securevalues:write": {"securevalues:uid:*"}, + "securevalues:delete": {"securevalues:uid:*"}, }) nsClient, err := client.Client(ctx, ns) @@ -107,3 +112,63 @@ func TestIntegration_SecureValueClient_CRUD(t *testing.T) { require.ErrorIs(t, err, contracts.ErrSecureValueNotFound) require.Nil(t, read) } + +func Test_SecureValueClient_CRUD_NoPermissions(t *testing.T) { + setup := testutils.Setup(t) + + validator := validator.ProvideSecureValueValidator() + + client := ProvideSecureValueClient( + setup.SecureValueService, + validator, + setup.AccessClient, + ) + + ns := "stacks-1234" + ctx := testutils.CreateUserAuthContext(t.Context(), ns, nil) + + nsClient, err := client.Client(ctx, ns) + require.NoError(t, err) + require.NotNil(t, nsClient) + + sv := &secretv1beta1.SecureValue{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-sv", + Namespace: ns, + }, + } + + unstructured, err := toUnstructured(sv) + require.NoError(t, err) + + // Create + created, err := nsClient.Create(ctx, unstructured, metav1.CreateOptions{}) + var apiErr *apierrors.StatusError + require.ErrorAs(t, err, &apiErr) + require.Equal(t, apiErr.ErrStatus.Reason, metav1.StatusReasonForbidden) + require.Nil(t, created) + + // Read + read, err := nsClient.Get(ctx, sv.Name, metav1.GetOptions{}) + require.ErrorAs(t, err, &apiErr) + require.Equal(t, apiErr.ErrStatus.Reason, metav1.StatusReasonForbidden) + require.Nil(t, created) + + // Update + updated, err := nsClient.Update(ctx, unstructured, metav1.UpdateOptions{}) + require.ErrorAs(t, err, &apiErr) + require.Equal(t, apiErr.ErrStatus.Reason, metav1.StatusReasonForbidden) + require.Nil(t, updated) + + // List + list, err := nsClient.List(ctx, metav1.ListOptions{}) + require.ErrorAs(t, err, &apiErr) + require.Equal(t, apiErr.ErrStatus.Reason, metav1.StatusReasonForbidden) + require.Nil(t, list) + + // Delete + err = nsClient.Delete(ctx, sv.Name, metav1.DeleteOptions{}) + require.ErrorAs(t, err, &apiErr) + require.Equal(t, apiErr.ErrStatus.Reason, metav1.StatusReasonForbidden) + require.Nil(t, read) +} diff --git a/pkg/registry/apis/secret/testutils/testutils.go b/pkg/registry/apis/secret/testutils/testutils.go index 5bac9a4d174..3b41784786f 100644 --- a/pkg/registry/apis/secret/testutils/testutils.go +++ b/pkg/registry/apis/secret/testutils/testutils.go @@ -23,7 +23,7 @@ import ( "github.com/grafana/grafana/pkg/registry/apis/secret/service" "github.com/grafana/grafana/pkg/registry/apis/secret/xkube" "github.com/grafana/grafana/pkg/services/accesscontrol" - "github.com/grafana/grafana/pkg/services/accesscontrol/actest" + "github.com/grafana/grafana/pkg/services/accesscontrol/acimpl" "github.com/grafana/grafana/pkg/services/sqlstore" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/storage/secret/database" @@ -70,7 +70,7 @@ func Setup(t *testing.T, opts ...func(*SetupConfig)) Sut { require.NoError(t, err) // Initialize access client + access control - accessControl := &actest.FakeAccessControl{ExpectedEvaluate: true} + accessControl := acimpl.ProvideAccessControl(nil) accessClient := accesscontrol.NewLegacyAccessClient(accessControl, accesscontrol.ResourceAuthorizerOptions{ Resource: "securevalues", Attr: "uid", @@ -132,6 +132,7 @@ func Setup(t *testing.T, opts ...func(*SetupConfig)) Sut { EncryptedValueStorage: encryptedValueStorage, SQLKeeper: sqlKeeper, Database: database, + AccessClient: accessClient, } } @@ -143,6 +144,7 @@ type Sut struct { EncryptedValueStorage contracts.EncryptedValueStorage SQLKeeper *sqlkeeper.SQLKeeper Database *database.Database + AccessClient types.AccessClient } type CreateSvConfig struct { diff --git a/pkg/server/wire_gen.go b/pkg/server/wire_gen.go index 2f5f6dc98c2..b776f88f1df 100644 --- a/pkg/server/wire_gen.go +++ b/pkg/server/wire_gen.go @@ -777,7 +777,7 @@ func Initialize(cfg *setting.Cfg, opts Options, apiOpts api.ServerOptions) (*Ser } secureValueService := service12.ProvideSecureValueService(tracer, accessClient, databaseDatabase, secureValueMetadataStorage, keeperMetadataStorage, ossKeeperService) secureValueValidator := validator3.ProvideSecureValueValidator() - secureValueClient := secret.ProvideSecureValueClient(secureValueService, secureValueValidator) + secureValueClient := secret.ProvideSecureValueClient(secureValueService, secureValueValidator, accessClient) decryptAuthorizer := decrypt.ProvideDecryptAuthorizer(tracer) decryptStorage, err := metadata.ProvideDecryptStorage(tracer, ossKeeperService, keeperMetadataStorage, secureValueMetadataStorage, decryptAuthorizer, registerer) if err != nil { @@ -1330,7 +1330,7 @@ func InitializeForTest(t sqlutil.ITestDB, testingT interface { } secureValueService := service12.ProvideSecureValueService(tracer, accessClient, databaseDatabase, secureValueMetadataStorage, keeperMetadataStorage, ossKeeperService) secureValueValidator := validator3.ProvideSecureValueValidator() - secureValueClient := secret.ProvideSecureValueClient(secureValueService, secureValueValidator) + secureValueClient := secret.ProvideSecureValueClient(secureValueService, secureValueValidator, accessClient) decryptAuthorizer := decrypt.ProvideDecryptAuthorizer(tracer) decryptStorage, err := metadata.ProvideDecryptStorage(tracer, ossKeeperService, keeperMetadataStorage, secureValueMetadataStorage, decryptAuthorizer, registerer) if err != nil {