Access Control / Denial of Service (panic due to nil pointer dereference in StoreWrapper)

Reproduction for versions before 12.4.0 (multi-tenant): 1) Run Grafana in multi-tenant mode where NewAPIService wires a nil local ConfigProvider (no cfgProvider is passed to the StoreWrapper). 2) Trigger an IAM user lookup that goes through the StoreWrapper (e.g., GET /api/iam/users/{login} or a related IAM API path that invokes AfterGet/FilterList). 3) The legacy code path dereferences a nil cfgProvider (cfgProvider.Get(ctx)), causing a panic and crash of the API server (Denial of Service). Proof-of-concept snippet (illustrative; does not compile as-is due to environment specifics): package main import ( "context" iam "github.com/grafana/grafana/apps/iam/pkg/apis/iam/v0alpha1" "github.com/grafana/grafana/pkg/registry/apis/iam/user" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) func main() { // Before fix: create a StoreWrapper with a nil config provider and nil setting service sw := user.NewStoreWrapper(nil, nil) u := &iam.User{ObjectMeta: metav1.ObjectMeta{Name: "alice"}, Spec: iam.UserSpec{Login: "alice"}} // This would trigger a panic in the old code path when AfterGet is invoked _ = sw.AfterGet(context.Background(), u) }

diff --git a/pkg/registry/apis/iam/models.go b/pkg/registry/apis/iam/models.go index 9721a5923e3ce..dc7ac484abd62 100644 --- a/pkg/registry/apis/iam/models.go +++ b/pkg/registry/apis/iam/models.go @@ -22,6 +22,7 @@ import ( "github.com/grafana/grafana/pkg/services/apiserver/builder" "github.com/grafana/grafana/pkg/services/authz/zanzana" "github.com/grafana/grafana/pkg/services/featuremgmt" + settingsvc "github.com/grafana/grafana/pkg/services/setting" "github.com/grafana/grafana/pkg/services/ssosettings" "github.com/grafana/grafana/pkg/storage/legacysql/dualwrite" "github.com/grafana/grafana/pkg/storage/unified/resource" @@ -113,7 +114,8 @@ type IdentityAccessManagementAPIBuilder struct { tracing tracing.Tracer - cfgProvider configprovider.ConfigProvider + cfgProvider configprovider.ConfigProvider + settingService settingsvc.Service apiConfig Config } diff --git a/pkg/registry/apis/iam/register.go b/pkg/registry/apis/iam/register.go index b122361cca563..00957db96c0fa 100644 --- a/pkg/registry/apis/iam/register.go +++ b/pkg/registry/apis/iam/register.go @@ -53,6 +53,7 @@ import ( "github.com/grafana/grafana/pkg/services/authz/zanzana" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/org" + settingsvc "github.com/grafana/grafana/pkg/services/setting" "github.com/grafana/grafana/pkg/services/ssosettings" teamservice "github.com/grafana/grafana/pkg/services/team" legacyuser "github.com/grafana/grafana/pkg/services/user" @@ -180,6 +181,7 @@ func NewAPIService( authorizerDialConfigs map[schema.GroupResource]iamauthorizer.DialConfig, tracingService tracing.Tracer, mappers *resourcepermission.MappersRegistry, + settingService settingsvc.Service, ) *IdentityAccessManagementAPIBuilder { store := legacy.NewLegacySQLStores(dbProvider) resourcePermissionsStorage := resourcepermission.ProvideStorageBackend(dbProvider, mappers) @@ -215,6 +217,7 @@ func NewAPIService( globalRoleApiInstaller: globalRoleApiInstaller, apiConfig: Config{SingleOrganization: true}, teamLBACApiInstaller: teamLBACApiInstaller, + settingService: settingService, authorizer: authorizer.AuthorizerFunc( func(ctx context.Context, a authorizer.Attributes) (authorizer.Decision, string, error) { user, ok := types.AuthInfoFrom(ctx) @@ -545,7 +548,7 @@ func (b *IdentityAccessManagementAPIBuilder) UpdateUsersAPIGroup(opts builder.AP } } - storage[userResource.StoragePath()] = storewrapper.New(userStore, user.NewStoreWrapper(b.cfgProvider), storewrapper.WithPreserveIdentity()) + storage[userResource.StoragePath()] = storewrapper.New(userStore, user.NewStoreWrapper(b.cfgProvider, b.settingService), storewrapper.WithPreserveIdentity()) if b.dual != nil && b.unified != nil { legacyTeamBindingSearchClient := teambinding.NewLegacyTeamBindingSearchClient(b.store, b.tracing) diff --git a/pkg/registry/apis/iam/user/store_wrapper.go b/pkg/registry/apis/iam/user/store_wrapper.go index af0422c57d6bd..b35fc73c1ddf4 100644 --- a/pkg/registry/apis/iam/user/store_wrapper.go +++ b/pkg/registry/apis/iam/user/store_wrapper.go @@ -3,41 +3,85 @@ package user import ( "context" "errors" + "strings" 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/runtime" iamv0 "github.com/grafana/grafana/apps/iam/pkg/apis/iam/v0alpha1" "github.com/grafana/grafana/pkg/configprovider" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/services/apiserver/auth/authorizer/storewrapper" + settingsvc "github.com/grafana/grafana/pkg/services/setting" ) // StoreWrapper filters users based on the hidden users configuration. // It does not enforce any write authorization — those are handled at the API level. type StoreWrapper struct { - cfgProvider configprovider.ConfigProvider - logger log.Logger + cfgProvider configprovider.ConfigProvider + settingService settingsvc.Service + logger log.Logger } var _ storewrapper.ResourceStorageAuthorizer = (*StoreWrapper)(nil) -func NewStoreWrapper(cfgProvider configprovider.ConfigProvider) *StoreWrapper { +func NewStoreWrapper(cfgProvider configprovider.ConfigProvider, settingService settingsvc.Service) *StoreWrapper { return &StoreWrapper{ - cfgProvider: cfgProvider, - logger: log.New("grafana-apiserver.users.storewrapper"), + cfgProvider: cfgProvider, + settingService: settingService, + logger: log.New("grafana-apiserver.users.storewrapper"), } } +// hiddenUsersSelector selects the hidden_users setting from the users section. +var hiddenUsersSelector = metav1.LabelSelector{ + MatchLabels: map[string]string{ + "section": "users", + "key": "hidden_users", + }, +} + +// getHiddenUsers returns the set of hidden user logins by querying either the +// local ConfigProvider (single-tenant) or the remote setting.Service (multi-tenant). +func (f *StoreWrapper) getHiddenUsers(ctx context.Context) (map[string]struct{}, error) { + if f.cfgProvider != nil { + cfg, err := f.cfgProvider.Get(ctx) + if err != nil { + return nil, err + } + return cfg.HiddenUsers, nil + } + + if f.settingService != nil { + settings, err := f.settingService.List(ctx, hiddenUsersSelector) + if err != nil { + return nil, err + } + hidden := make(map[string]struct{}) + for _, s := range settings { + for _, login := range strings.Split(s.Value, ",") { + login = strings.TrimSpace(login) + if login != "" { + hidden[login] = struct{}{} + } + } + } + return hidden, nil + } + + return nil, errors.New("user store wrapper: neither cfgProvider nor settingService is configured") +} + // AfterGet returns NotFound if the user's login is in the hidden users list // and the requester is not the user themselves. func (f *StoreWrapper) AfterGet(ctx context.Context, obj runtime.Object) error { - cfg, err := f.cfgProvider.Get(ctx) + hiddenUsers, err := f.getHiddenUsers(ctx) if err != nil { return err } - if len(cfg.HiddenUsers) == 0 { + if len(hiddenUsers) == 0 { return nil } @@ -52,7 +96,7 @@ func (f *StoreWrapper) AfterGet(ctx context.Context, obj runtime.Object) error { } login := u.Spec.Login - if _, isHidden := cfg.HiddenUsers[login]; isHidden && login != authInfo.GetUsername() { + if _, isHidden := hiddenUsers[login]; isHidden && login != authInfo.GetUsername() { return apierrors.NewNotFound(iamv0.UserResourceInfo.GroupResource(), u.Name) } @@ -61,11 +105,11 @@ func (f *StoreWrapper) AfterGet(ctx context.Context, obj runtime.Object) error { // FilterList removes hidden users from the list, except for the requester themselves. func (f *StoreWrapper) FilterList(ctx context.Context, list runtime.Object) (runtime.Object, error) { - cfg, err := f.cfgProvider.Get(ctx) + hiddenUsers, err := f.getHiddenUsers(ctx) if err != nil { return nil, err } - if len(cfg.HiddenUsers) == 0 { + if len(hiddenUsers) == 0 { return list, nil } @@ -82,7 +126,7 @@ func (f *StoreWrapper) FilterList(ctx context.Context, list runtime.Object) (run requesterLogin := authInfo.GetUsername() filtered := make([]iamv0.User, 0, len(userList.Items)) for _, u := range userList.Items { - if _, isHidden := cfg.HiddenUsers[u.Spec.Login]; !isHidden || u.Spec.Login == requesterLogin { + if _, isHidden := hiddenUsers[u.Spec.Login]; !isHidden || u.Spec.Login == requesterLogin { filtered = append(filtered, u) } } @@ -92,11 +136,11 @@ func (f *StoreWrapper) FilterList(ctx context.Context, list runtime.Object) (run // BeforeCreate returns Forbidden if the new user's login is in the hidden users list. func (f *StoreWrapper) BeforeCreate(ctx context.Context, obj runtime.Object) error { - cfg, err := f.cfgProvider.Get(ctx) + hiddenUsers, err := f.getHiddenUsers(ctx) if err != nil { return err } - if len(cfg.HiddenUsers) == 0 { + if len(hiddenUsers) == 0 { return nil } @@ -105,7 +149,7 @@ func (f *StoreWrapper) BeforeCreate(ctx context.Context, obj runtime.Object) err return nil } - if _, isHidden := cfg.HiddenUsers[u.Spec.Login]; isHidden { + if _, isHidden := hiddenUsers[u.Spec.Login]; isHidden { f.logger.Info("blocked create for hidden user", "login", u.Spec.Login, "name", u.Name) return apierrors.NewForbidden(iamv0.UserResourceInfo.GroupResource(), u.Name, errors.New("operation not permitted")) } @@ -115,11 +159,11 @@ func (f *StoreWrapper) BeforeCreate(ctx context.Context, obj runtime.Object) err // BeforeUpdate returns Forbidden if the target user (old object) or the new login is in the hidden users list. func (f *StoreWrapper) BeforeUpdate(ctx context.Context, oldObj, obj runtime.Object) error { - cfg, err := f.cfgProvider.Get(ctx) + hiddenUsers, err := f.getHiddenUsers(ctx) if err != nil { return err } - if len(cfg.HiddenUsers) == 0 { + if len(hiddenUsers) == 0 { return nil } @@ -128,7 +172,7 @@ func (f *StoreWrapper) BeforeUpdate(ctx context.Context, oldObj, obj runtime.Obj return nil } - if _, isHidden := cfg.HiddenUsers[oldUser.Spec.Login]; isHidden { + if _, isHidden := hiddenUsers[oldUser.Spec.Login]; isHidden { f.logger.Info("blocked update for hidden user", "login", oldUser.Spec.Login, "name", oldUser.Name) return apierrors.NewForbidden(iamv0.UserResourceInfo.GroupResource(), oldUser.Name, errors.New("operation not permitted")) } @@ -139,7 +183,7 @@ func (f *StoreWrapper) BeforeUpdate(ctx context.Context, oldObj, obj runtime.Obj return nil } - if _, isHidden := cfg.HiddenUsers[newUser.Spec.Login]; isHidden { + if _, isHidden := hiddenUsers[newUser.Spec.Login]; isHidden { f.logger.Info("blocked update to hidden user login", "login", newUser.Spec.Login, "name", newUser.Name) return apierrors.NewForbidden(iamv0.UserResourceInfo.GroupResource(), newUser.Name, errors.New("operation not permitted")) } @@ -149,11 +193,11 @@ func (f *StoreWrapper) BeforeUpdate(ctx context.Context, oldObj, obj runtime.Obj // BeforeDelete returns Forbidden if the target user is in the hidden users list. func (f *StoreWrapper) BeforeDelete(ctx context.Context, obj runtime.Object) error { - cfg, err := f.cfgProvider.Get(ctx) + hiddenUsers, err := f.getHiddenUsers(ctx) if err != nil { return err } - if len(cfg.HiddenUsers) == 0 { + if len(hiddenUsers) == 0 { return nil } @@ -162,7 +206,7 @@ func (f *StoreWrapper) BeforeDelete(ctx context.Context, obj runtime.Object) err return nil } - if _, isHidden := cfg.HiddenUsers[u.Spec.Login]; isHidden { + if _, isHidden := hiddenUsers[u.Spec.Login]; isHidden { f.logger.Info("blocked delete for hidden user", "login", u.Spec.Login, "name", u.Name) return apierrors.NewForbidden(iamv0.UserResourceInfo.GroupResource(), u.Name, errors.New("operation not permitted")) } diff --git a/pkg/registry/apis/iam/user/store_wrapper_test.go b/pkg/registry/apis/iam/user/store_wrapper_test.go new file mode 100644 index 0000000000000..c49ac9abaf9fd --- /dev/null +++ b/pkg/registry/apis/iam/user/store_wrapper_test.go @@ -0,0 +1,310 @@ +package user + +import ( + "context" + "testing" + + "github.com/prometheus/client_golang/prometheus" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gopkg.in/ini.v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + claims "github.com/grafana/authlib/types" + + iamv0 "github.com/grafana/grafana/apps/iam/pkg/apis/iam/v0alpha1" + "github.com/grafana/grafana/pkg/apimachinery/identity" + settingsvc "github.com/grafana/grafana/pkg/services/setting" +) + +func TestStoreWrapper_SettingService_AfterGet(t *testing.T) { + tests := []struct { + name string + settings []*settingsvc.Setting + login string + requester string + expectError bool + isNotFound bool + }{ + { + name: "no hidden users setting returns nil", + settings: nil, + login: "admin", + requester: "viewer", + expectError: false, + }, + { + name: "empty hidden_users value returns nil", + settings: []*settingsvc.Setting{ + {Section: "users", Key: "hidden_users", Value: ""}, + }, + login: "admin", + requester: "viewer", + expectError: false, + }, + { + name: "hidden user returns NotFound for other requester", + settings: []*settingsvc.Setting{ + {Section: "users", Key: "hidden_users", Value: "admin"}, + }, + login: "admin", + requester: "viewer", + expectError: true, + isNotFound: true, + }, + { + name: "hidden user can see themselves", + settings: []*settingsvc.Setting{ + {Section: "users", Key: "hidden_users", Value: "admin"}, + }, + login: "admin", + requester: "admin", + expectError: false, + }, + { + name: "comma-separated hidden users", + settings: []*settingsvc.Setting{ + {Section: "users", Key: "hidden_users", Value: "admin, servicebot"}, + }, + login: "servicebot", + requester: "viewer", + expectError: true, + isNotFound: true, + }, + { + name: "non-hidden user is allowed", + settings: []*settingsvc.Setting{ + {Section: "users", Key: "hidden_users", Value: "admin"}, + }, + login: "viewer", + requester: "other", + expectError: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + sw := NewStoreWrapper(nil, &fakeSettingService{settings: tt.settings}) + ctx := withAuthInfo(context.Background(), tt.requester) + user := &iamv0.User{ + ObjectMeta: metav1.ObjectMeta{Name: tt.login}, + Spec: iamv0.UserSpec{Login: tt.login}, + } + + err := sw.AfterGet(ctx, user) + if tt.expectError { + require.Error(t, err) + if tt.isNotFound { + assert.True(t, apierrors.IsNotFound(err)) + } + } else { + require.NoError(t, err) + } + }) + } +} + +func TestStoreWrapper_SettingService_FilterList(t *testing.T) { + sw := NewStoreWrapper(nil, &fakeSettingService{ + settings: []*settingsvc.Setting{ + {Section: "users", Key: "hidden_users", Value: "hidden1, hidden2"}, + }, + }) + ctx := withAuthInfo(context.Background(), "hidden1") + + list := &iamv0.UserList{ + Items: []iamv0.User{ + {Spec: iamv0.UserSpec{Login: "visible"}}, + {Spec: iamv0.UserSpec{Login: "hidden1"}}, // requester themselves + {Spec: iamv0.UserSpec{Login: "hidden2"}}, + {Spec: iamv0.UserSpec{Login: "another"}}, + }, + } + + result, err := sw.FilterList(ctx, list) + require.NoError(t, err) + + userList := result.(*iamv0.UserList) + logins := make([]string, len(userList.Items)) + for i, u := range userList.Items { + logins[i] = u.Spec.Login + } + assert.Equal(t, []string{"visible", "hidden1", "another"}, logins) +} + +func TestStoreWrapper_SettingService_BeforeCreate(t *testing.T) { + sw := NewStoreWrapper(nil, &fakeSettingService{ + settings: []*settingsvc.Setting{ + {Section: "users", Key: " ... [truncated]