From 700e6e328799cb6ef4680fa233d713cbc696001b Mon Sep 17 00:00:00 2001 From: Gabriel MABILLE Date: Thu, 12 Oct 2023 16:15:16 +0200 Subject: [PATCH] AuthN: Add service account token generation to `ExtSvcAccountsService` (#76327) * Manage service account secrets * Wip * WIP * WIP * Revert to keep a light interface * Implement SaveExternalService * Remove unecessary functions from the interface * Remove unused field * Better log * Leave ext svc credentials out of the extsvcauth package for now * Remove todo * Add tests to SaveExternalService * Test that secret has been removed from store * Lint * Nit. * Rename commands and structs Co-authored-by: Kalle Persson * Account for PR feedback Co-authored-by: Andres Martinez Gotor * Linting * Add nosec comment G101 - this is not a hardcoded secret * Lowercase kvStoreType --------- Co-authored-by: Kalle Persson Co-authored-by: Andres Martinez Gotor --- pkg/services/extsvcauth/errors.go | 3 +- .../extsvcauth/extsvcaccounts/models.go | 19 +- .../extsvcauth/extsvcaccounts/service.go | 136 ++++++++++- .../extsvcauth/extsvcaccounts/service_test.go | 214 +++++++++++++++++- pkg/services/extsvcauth/models.go | 3 + 5 files changed, 354 insertions(+), 21 deletions(-) diff --git a/pkg/services/extsvcauth/errors.go b/pkg/services/extsvcauth/errors.go index f1af61f4f0c..c605dd3f7ab 100644 --- a/pkg/services/extsvcauth/errors.go +++ b/pkg/services/extsvcauth/errors.go @@ -3,5 +3,6 @@ package extsvcauth import "github.com/grafana/grafana/pkg/util/errutil" var ( - ErrUnknownProvider = errutil.BadRequest("extsvcauth.unknown-provider") + ErrUnknownProvider = errutil.BadRequest("extsvcauth.unknown-provider") + ErrCredentialsNotFound = errutil.NotFound("extsvcauth.credentials-not-found") ) diff --git a/pkg/services/extsvcauth/extsvcaccounts/models.go b/pkg/services/extsvcauth/extsvcaccounts/models.go index 536d6432f32..621513e0d21 100644 --- a/pkg/services/extsvcauth/extsvcaccounts/models.go +++ b/pkg/services/extsvcauth/extsvcaccounts/models.go @@ -5,7 +5,24 @@ import ( ac "github.com/grafana/grafana/pkg/services/accesscontrol" ) -type saveExtSvcAccountCmd struct { +const ( + kvStoreType = "extsvc-token" + // #nosec G101 - this is not a hardcoded secret + tokenNamePrefix = "extsvc-token" +) + +// Credentials represents the credentials associated to an external service +type Credentials struct { + Secret string +} + +type SaveCredentialsCmd struct { + ExtSvcSlug string + OrgID int64 + Secret string +} + +type saveCmd struct { ExtSvcSlug string OrgID int64 Permissions []ac.Permission diff --git a/pkg/services/extsvcauth/extsvcaccounts/service.go b/pkg/services/extsvcauth/extsvcaccounts/service.go index 6bc60ace2c5..3041cf653a9 100644 --- a/pkg/services/extsvcauth/extsvcaccounts/service.go +++ b/pkg/services/extsvcauth/extsvcaccounts/service.go @@ -4,24 +4,32 @@ import ( "context" "errors" + "github.com/grafana/grafana/pkg/components/satokengen" + "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/infra/slugify" "github.com/grafana/grafana/pkg/models/roletype" ac "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/extsvcauth" + "github.com/grafana/grafana/pkg/services/secrets" + "github.com/grafana/grafana/pkg/services/secrets/kvstore" sa "github.com/grafana/grafana/pkg/services/serviceaccounts" ) type ExtSvcAccountsService struct { - acSvc ac.Service - logger log.Logger - saSvc sa.Service + acSvc ac.Service + logger log.Logger + saSvc sa.Service + skvStore kvstore.SecretsKVStore } -func ProvideExtSvcAccountsService(acSvc ac.Service, saSvc sa.Service) *ExtSvcAccountsService { +func ProvideExtSvcAccountsService(acSvc ac.Service, saSvc sa.Service, db db.DB, secretsSvc secrets.Service) *ExtSvcAccountsService { + logger := log.New("serviceauth.extsvcaccounts") return &ExtSvcAccountsService{ - acSvc: acSvc, - logger: log.New("serviceauth.extsvcaccounts"), - saSvc: saSvc, + acSvc: acSvc, + logger: logger, + saSvc: saSvc, + skvStore: kvstore.NewSQLSecretsKVStore(db, secretsSvc, logger), // Using SQL store to avoid a cyclic dependency } } @@ -41,6 +49,46 @@ func (esa *ExtSvcAccountsService) RetrieveExtSvcAccount(ctx context.Context, org }, nil } +// SaveExternalService creates, updates or delete a service account (and its token) with the requested permissions. +func (esa *ExtSvcAccountsService) SaveExternalService(ctx context.Context, cmd *extsvcauth.ExternalServiceRegistration) (*extsvcauth.ExternalService, error) { + if cmd == nil { + esa.logger.Warn("Received no input") + return nil, nil + } + + slug := slugify.Slugify(cmd.Name) + + if cmd.Impersonation.Enabled { + esa.logger.Warn("Impersonation setup skipped. It is not possible to impersonate with a service account token.", "service", slug) + } + + saID, err := esa.ManageExtSvcAccount(ctx, &extsvcauth.ManageExtSvcAccountCmd{ + ExtSvcSlug: slug, + Enabled: cmd.Self.Enabled, + OrgID: extsvcauth.TmpOrgID, + Permissions: cmd.Self.Permissions, + }) + if err != nil { + return nil, err + } + + // No need for a token if we don't have a service account + if saID <= 0 { + esa.logger.Debug("Skipping service account token creation", "service", slug) + return nil, nil + } + + token, err := esa.getExtSvcAccountToken(ctx, extsvcauth.TmpOrgID, saID, slug) + if err != nil { + esa.logger.Error("Could not get the external svc token", + "service", slug, + "saID", saID, + "error", err.Error()) + return nil, err + } + return &extsvcauth.ExternalService{Name: cmd.Name, ID: slug, Secret: token}, nil +} + // ManageExtSvcAccount creates, updates or deletes the service account associated with an external service func (esa *ExtSvcAccountsService) ManageExtSvcAccount(ctx context.Context, cmd *extsvcauth.ManageExtSvcAccountCmd) (int64, error) { if cmd == nil { @@ -71,7 +119,7 @@ func (esa *ExtSvcAccountsService) ManageExtSvcAccount(ctx context.Context, cmd * return 0, nil } - saID, errSave := esa.saveExtSvcAccount(ctx, &saveExtSvcAccountCmd{ + saID, errSave := esa.saveExtSvcAccount(ctx, &saveCmd{ ExtSvcSlug: cmd.ExtSvcSlug, OrgID: cmd.OrgID, Permissions: cmd.Permissions, @@ -86,7 +134,7 @@ func (esa *ExtSvcAccountsService) ManageExtSvcAccount(ctx context.Context, cmd * } // saveExtSvcAccount creates or updates the service account associated with an external service -func (esa *ExtSvcAccountsService) saveExtSvcAccount(ctx context.Context, cmd *saveExtSvcAccountCmd) (int64, error) { +func (esa *ExtSvcAccountsService) saveExtSvcAccount(ctx context.Context, cmd *saveCmd) (int64, error) { if cmd.SaID <= 0 { // Create a service account esa.logger.Debug("Create service account", "service", cmd.ExtSvcSlug, "orgID", cmd.OrgID) @@ -118,9 +166,75 @@ func (esa *ExtSvcAccountsService) saveExtSvcAccount(ctx context.Context, cmd *sa // deleteExtSvcAccount deletes a service account by ID and removes its associated role func (esa *ExtSvcAccountsService) deleteExtSvcAccount(ctx context.Context, orgID int64, slug string, saID int64) error { - esa.logger.Info("Delete service account", "service", slug, "saID", saID) + esa.logger.Info("Delete service account", "service", slug, "orgID", orgID, "saID", saID) if err := esa.saSvc.DeleteServiceAccount(ctx, orgID, saID); err != nil { return err } - return esa.acSvc.DeleteExternalServiceRole(ctx, slug) + if err := esa.acSvc.DeleteExternalServiceRole(ctx, slug); err != nil { + return err + } + return esa.DeleteExtSvcCredentials(ctx, orgID, slug) +} + +// getExtSvcAccountToken get or create the token of an External Service +func (esa *ExtSvcAccountsService) getExtSvcAccountToken(ctx context.Context, orgID, saID int64, extSvcSlug string) (string, error) { + // Get credentials from store + credentials, err := esa.GetExtSvcCredentials(ctx, orgID, extSvcSlug) + if err != nil && !errors.Is(err, extsvcauth.ErrCredentialsNotFound) { + return "", err + } + if credentials != nil { + return credentials.Secret, nil + } + + // Generate token + esa.logger.Info("Generate new service account token", "service", extSvcSlug, "orgID", orgID) + newKeyInfo, err := satokengen.New(extSvcSlug) + if err != nil { + return "", err + } + + esa.logger.Debug("Add service account token", "service", extSvcSlug, "orgID", orgID) + if _, err := esa.saSvc.AddServiceAccountToken(ctx, saID, &sa.AddServiceAccountTokenCommand{ + Name: tokenNamePrefix + "-" + extSvcSlug, + OrgId: orgID, + Key: newKeyInfo.HashedKey, + }); err != nil { + return "", err + } + + if err := esa.SaveExtSvcCredentials(ctx, &SaveCredentialsCmd{ + ExtSvcSlug: extSvcSlug, + OrgID: orgID, + Secret: newKeyInfo.ClientSecret, + }); err != nil { + return "", err + } + + return newKeyInfo.ClientSecret, nil +} + +// GetExtSvcCredentials get the credentials of an External Service from an encrypted storage +func (esa *ExtSvcAccountsService) GetExtSvcCredentials(ctx context.Context, orgID int64, extSvcSlug string) (*Credentials, error) { + esa.logger.Debug("Get service account token from skv", "service", extSvcSlug, "orgID", orgID) + token, ok, err := esa.skvStore.Get(ctx, orgID, extSvcSlug, kvStoreType) + if err != nil { + return nil, err + } + if !ok { + return nil, extsvcauth.ErrCredentialsNotFound.Errorf("No credential found for in store %v", extSvcSlug) + } + return &Credentials{Secret: token}, nil +} + +// SaveExtSvcCredentials stores the credentials of an External Service in an encrypted storage +func (esa *ExtSvcAccountsService) SaveExtSvcCredentials(ctx context.Context, cmd *SaveCredentialsCmd) error { + esa.logger.Debug("Save service account token in skv", "service", cmd.ExtSvcSlug, "orgID", cmd.OrgID) + return esa.skvStore.Set(ctx, cmd.OrgID, cmd.ExtSvcSlug, kvStoreType, cmd.Secret) +} + +// DeleteExtSvcCredentials removes the credentials of an External Service from an encrypted storage +func (esa *ExtSvcAccountsService) DeleteExtSvcCredentials(ctx context.Context, orgID int64, extSvcSlug string) error { + esa.logger.Debug("Delete service account token from skv", "service", extSvcSlug, "orgID", orgID) + return esa.skvStore.Del(ctx, orgID, extSvcSlug, kvStoreType) } diff --git a/pkg/services/extsvcauth/extsvcaccounts/service_test.go b/pkg/services/extsvcauth/extsvcaccounts/service_test.go index 0cb27a55729..92a152e618c 100644 --- a/pkg/services/extsvcauth/extsvcaccounts/service_test.go +++ b/pkg/services/extsvcauth/extsvcaccounts/service_test.go @@ -10,8 +10,10 @@ import ( ac "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/accesscontrol/acimpl" "github.com/grafana/grafana/pkg/services/accesscontrol/actest" + "github.com/grafana/grafana/pkg/services/apikey" "github.com/grafana/grafana/pkg/services/extsvcauth" "github.com/grafana/grafana/pkg/services/featuremgmt" + "github.com/grafana/grafana/pkg/services/secrets/kvstore" sa "github.com/grafana/grafana/pkg/services/serviceaccounts" "github.com/grafana/grafana/pkg/services/serviceaccounts/tests" "github.com/grafana/grafana/pkg/setting" @@ -20,9 +22,10 @@ import ( ) type TestEnv struct { - S *ExtSvcAccountsService - AcStore *actest.MockStore - SaSvc *tests.MockServiceAccountService + S *ExtSvcAccountsService + AcStore *actest.MockStore + SaSvc *tests.MockServiceAccountService + SkvStore *kvstore.FakeSecretsKVStore } func setupTestEnv(t *testing.T) *TestEnv { @@ -32,13 +35,15 @@ func setupTestEnv(t *testing.T) *TestEnv { fmgt := featuremgmt.WithFeatures(featuremgmt.FlagExternalServiceAuth) env := &TestEnv{ - AcStore: &actest.MockStore{}, - SaSvc: &tests.MockServiceAccountService{}, + AcStore: &actest.MockStore{}, + SaSvc: &tests.MockServiceAccountService{}, + SkvStore: kvstore.NewFakeSecretsKVStore(), } env.S = &ExtSvcAccountsService{ - acSvc: acimpl.ProvideOSSService(cfg, env.AcStore, localcache.New(0, 0), fmgt), - logger: log.New("extsvcaccounts.test"), - saSvc: env.SaSvc, + acSvc: acimpl.ProvideOSSService(cfg, env.AcStore, localcache.New(0, 0), fmgt), + logger: log.New("extsvcaccounts.test"), + saSvc: env.SaSvc, + skvStore: env.SkvStore, } return env } @@ -209,3 +214,196 @@ func TestExtSvcAccountsService_ManageExtSvcAccount(t *testing.T) { }) } } + +func TestExtSvcAccountsService_SaveExternalService(t *testing.T) { + extSvcSlug := "grafana-test-app" + tmpOrgID := int64(1) + extSvcAccID := int64(10) + extSvcPerms := []ac.Permission{{Action: ac.ActionUsersRead, Scope: ac.ScopeUsersAll}} + extSvcAccount := &sa.ServiceAccountDTO{ + Id: extSvcAccID, + Name: extSvcSlug, + Login: extSvcSlug, + OrgId: tmpOrgID, + IsDisabled: false, + Role: string(roletype.RoleNone), + } + + tests := []struct { + name string + init func(env *TestEnv) + cmd extsvcauth.ExternalServiceRegistration + checks func(t *testing.T, env *TestEnv) + want *extsvcauth.ExternalService + wantErr bool + }{ + { + name: "should remove service account when disabled", + init: func(env *TestEnv) { + // A previous service account was attached to this slug + env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, mock.Anything, mock.Anything).Return(extSvcAccID, nil) + env.SaSvc.On("DeleteServiceAccount", mock.Anything, mock.Anything, mock.Anything).Return(nil) + env.AcStore.On("DeleteExternalServiceRole", mock.Anything, mock.Anything).Return(nil) + // A token was previously stored in the secret store + _ = env.SkvStore.Set(context.Background(), tmpOrgID, extSvcSlug, kvStoreType, "ExtSvcSecretToken") + }, + cmd: extsvcauth.ExternalServiceRegistration{ + Name: extSvcSlug, + Self: extsvcauth.SelfCfg{ + Enabled: false, + Permissions: extSvcPerms, + }, + }, + checks: func(t *testing.T, env *TestEnv) { + env.SaSvc.AssertCalled(t, "RetrieveServiceAccountIdByName", mock.Anything, + mock.MatchedBy(func(orgID int64) bool { return orgID == tmpOrgID }), + mock.MatchedBy(func(slug string) bool { return slug == extSvcSlug })) + env.SaSvc.AssertCalled(t, "DeleteServiceAccount", mock.Anything, + mock.MatchedBy(func(orgID int64) bool { return orgID == tmpOrgID }), + mock.MatchedBy(func(saID int64) bool { return saID == extSvcAccID })) + env.AcStore.AssertCalled(t, "DeleteExternalServiceRole", mock.Anything, + mock.MatchedBy(func(slug string) bool { return slug == extSvcSlug })) + _, ok, _ := env.SkvStore.Get(context.Background(), tmpOrgID, extSvcSlug, kvStoreType) + require.False(t, ok, "secret should have been removed from store") + }, + want: nil, + wantErr: false, + }, + { + name: "should remove service account when no permission", + init: func(env *TestEnv) { + // A previous service account was attached to this slug + env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, mock.Anything, mock.Anything).Return(extSvcAccID, nil) + env.SaSvc.On("DeleteServiceAccount", mock.Anything, mock.Anything, mock.Anything).Return(nil) + env.AcStore.On("DeleteExternalServiceRole", mock.Anything, mock.Anything).Return(nil) + }, + cmd: extsvcauth.ExternalServiceRegistration{ + Name: extSvcSlug, + Self: extsvcauth.SelfCfg{ + Enabled: true, + Permissions: []ac.Permission{}, + }, + }, + checks: func(t *testing.T, env *TestEnv) { + env.SaSvc.AssertCalled(t, "RetrieveServiceAccountIdByName", mock.Anything, + mock.MatchedBy(func(orgID int64) bool { return orgID == tmpOrgID }), + mock.MatchedBy(func(slug string) bool { return slug == extSvcSlug })) + env.SaSvc.AssertCalled(t, "DeleteServiceAccount", mock.Anything, + mock.MatchedBy(func(orgID int64) bool { return orgID == tmpOrgID }), + mock.MatchedBy(func(saID int64) bool { return saID == extSvcAccID })) + env.AcStore.AssertCalled(t, "DeleteExternalServiceRole", mock.Anything, + mock.MatchedBy(func(slug string) bool { return slug == extSvcSlug })) + }, + want: nil, + wantErr: false, + }, + { + name: "should create new service account", + init: func(env *TestEnv) { + // No previous service account was attached to this slug + env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, mock.Anything, mock.Anything). + Return(int64(0), sa.ErrServiceAccountNotFound.Errorf("mock")) + env.SaSvc.On("CreateServiceAccount", mock.Anything, mock.Anything, mock.Anything). + Return(extSvcAccount, nil) + // Api Key was added without problem + env.SaSvc.On("AddServiceAccountToken", mock.Anything, mock.Anything, mock.Anything).Return(&apikey.APIKey{}, nil) + env.AcStore.On("SaveExternalServiceRole", mock.Anything, mock.Anything).Return(nil) + }, + cmd: extsvcauth.ExternalServiceRegistration{ + Name: extSvcSlug, + Self: extsvcauth.SelfCfg{ + Enabled: true, + Permissions: extSvcPerms, + }, + }, + checks: func(t *testing.T, env *TestEnv) { + env.SaSvc.AssertCalled(t, "RetrieveServiceAccountIdByName", mock.Anything, + mock.MatchedBy(func(orgID int64) bool { return orgID == tmpOrgID }), + mock.MatchedBy(func(slug string) bool { return slug == extSvcSlug })) + env.SaSvc.AssertCalled(t, "CreateServiceAccount", mock.Anything, + mock.MatchedBy(func(orgID int64) bool { return orgID == tmpOrgID }), + mock.MatchedBy(func(cmd *sa.CreateServiceAccountForm) bool { + return cmd.Name == extSvcSlug && *cmd.Role == roletype.RoleNone + }), + ) + env.AcStore.AssertCalled(t, "SaveExternalServiceRole", mock.Anything, + mock.MatchedBy(func(cmd ac.SaveExternalServiceRoleCommand) bool { + return cmd.ServiceAccountID == extSvcAccount.Id && cmd.ExternalServiceID == extSvcSlug && + cmd.OrgID == int64(ac.GlobalOrgID) && len(cmd.Permissions) == 1 && + cmd.Permissions[0] == extSvcPerms[0] + })) + }, + want: &extsvcauth.ExternalService{ + Name: extSvcSlug, + ID: extSvcSlug, + Secret: "not empty", + }, + wantErr: false, + }, + { + name: "should update service account", + init: func(env *TestEnv) { + // A previous service account was attached to this slug + env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, mock.Anything, mock.Anything). + Return(int64(11), nil) + env.AcStore.On("SaveExternalServiceRole", mock.Anything, mock.Anything).Return(nil) + // This time we don't add a token but rely on the secret store + _ = env.SkvStore.Set(context.Background(), tmpOrgID, extSvcSlug, kvStoreType, "ExtSvcSecretToken") + }, + cmd: extsvcauth.ExternalServiceRegistration{ + Name: extSvcSlug, + Self: extsvcauth.SelfCfg{ + Enabled: true, + Permissions: extSvcPerms, + }, + }, + checks: func(t *testing.T, env *TestEnv) { + env.SaSvc.AssertCalled(t, "RetrieveServiceAccountIdByName", mock.Anything, + mock.MatchedBy(func(orgID int64) bool { return orgID == tmpOrgID }), + mock.MatchedBy(func(slug string) bool { return slug == extSvcSlug })) + env.AcStore.AssertCalled(t, "SaveExternalServiceRole", mock.Anything, + mock.MatchedBy(func(cmd ac.SaveExternalServiceRoleCommand) bool { + return cmd.ServiceAccountID == int64(11) && cmd.ExternalServiceID == extSvcSlug && + cmd.OrgID == int64(ac.GlobalOrgID) && len(cmd.Permissions) == 1 && + cmd.Permissions[0] == extSvcPerms[0] + })) + }, + want: &extsvcauth.ExternalService{ + Name: extSvcSlug, + ID: extSvcSlug, + Secret: "not empty", + }, + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + + env := setupTestEnv(t) + if tt.init != nil { + tt.init(env) + } + + got, err := env.S.SaveExternalService(ctx, &tt.cmd) + + if tt.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + + if tt.checks != nil { + tt.checks(t, env) + } + + // Only check that there is a secret, not it's actual value + if tt.want != nil && len(tt.want.Secret) > 0 { + require.NotEmpty(t, got.Secret) + tt.want.Secret = got.Secret + } + + require.Equal(t, tt.want, got) + }) + } +} diff --git a/pkg/services/extsvcauth/models.go b/pkg/services/extsvcauth/models.go index cb82ed08ef7..51183a06d41 100644 --- a/pkg/services/extsvcauth/models.go +++ b/pkg/services/extsvcauth/models.go @@ -9,6 +9,9 @@ import ( const ( OAuth2Server AuthProvider = "OAuth2Server" + + // TmpOrgID is the orgID we use while global service accounts are not supported. + TmpOrgID int64 = 1 ) type AuthProvider string