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 <kalle.persson@grafana.com>

* Account for PR feedback

Co-authored-by: Andres Martinez Gotor <andres.martinez@grafana.com>

* Linting

* Add nosec comment G101 - this is not a hardcoded secret

* Lowercase kvStoreType

---------

Co-authored-by: Kalle Persson <kalle.persson@grafana.com>
Co-authored-by: Andres Martinez Gotor <andres.martinez@grafana.com>

pkg/services/extsvcauth/errors.go

@@ -4,4 +4,5 @@ import "github.com/grafana/grafana/pkg/util/errutil"
 
 var (
 	ErrUnknownProvider     = errutil.BadRequest("extsvcauth.unknown-provider")
+	ErrCredentialsNotFound = errutil.NotFound("extsvcauth.credentials-not-found")
 )

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

pkg/services/extsvcauth/extsvcaccounts/service.go

@@ -4,10 +4,15 @@ 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"
 )
 
@@ -15,13 +20,16 @@ type ExtSvcAccountsService struct {
 	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"),
+		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)
 }

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"
@@ -23,6 +25,7 @@ type TestEnv struct {
 	S        *ExtSvcAccountsService
 	AcStore  *actest.MockStore
 	SaSvc    *tests.MockServiceAccountService
+	SkvStore *kvstore.FakeSecretsKVStore
 }
 
 func setupTestEnv(t *testing.T) *TestEnv {
@@ -34,11 +37,13 @@ func setupTestEnv(t *testing.T) *TestEnv {
 	env := &TestEnv{
 		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,
+		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)
+		})
+	}
+}

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