From b1520e93f18a7e7dee781e8e3a577d5fd71a7e90 Mon Sep 17 00:00:00 2001 From: Gabriel MABILLE Date: Wed, 5 Jun 2024 11:56:54 +0200 Subject: [PATCH] Fix: Regenerate token on failed encryption/decryption (#88732) * Add function to rotate the extsvc token * Recover from failed token decryption * add log * Remove error check * Log outside error --- .../serviceaccounts/extsvcaccounts/service.go | 49 +++++++++++++++-- .../extsvcaccounts/service_test.go | 54 +++++++++++++++++-- 2 files changed, 95 insertions(+), 8 deletions(-) diff --git a/pkg/services/serviceaccounts/extsvcaccounts/service.go b/pkg/services/serviceaccounts/extsvcaccounts/service.go index d7b462088e2..c556ee7c5f5 100644 --- a/pkg/services/serviceaccounts/extsvcaccounts/service.go +++ b/pkg/services/serviceaccounts/extsvcaccounts/service.go @@ -347,7 +347,13 @@ func (esa *ExtSvcAccountsService) getExtSvcAccountToken(ctx context.Context, org // Get credentials from store credentials, err := esa.GetExtSvcCredentials(ctx, orgID, extSvcSlug) if err != nil && !errors.Is(err, ErrCredentialsNotFound) { - return "", err + if !errors.Is(err, &satokengen.ErrInvalidApiKey{}) { + return "", err + } + ctxLogger.Warn("Invalid token found in store, recovering...", "service", extSvcSlug, "orgID", orgID) + if err := esa.removeExtSvcAccountToken(ctx, orgID, saID, extSvcSlug); err != nil { + return "", err + } } if credentials != nil { return credentials.Secret, nil @@ -362,7 +368,7 @@ func (esa *ExtSvcAccountsService) getExtSvcAccountToken(ctx context.Context, org ctxLogger.Debug("Add service account token", "service", extSvcSlug, "orgID", orgID) if _, err := esa.saSvc.AddServiceAccountToken(ctx, saID, &sa.AddServiceAccountTokenCommand{ - Name: tokenNamePrefix + "-" + extSvcSlug, + Name: tokenName(extSvcSlug), OrgId: orgID, Key: newKeyInfo.HashedKey, }); err != nil { @@ -380,6 +386,35 @@ func (esa *ExtSvcAccountsService) getExtSvcAccountToken(ctx context.Context, org return newKeyInfo.ClientSecret, nil } +func (esa *ExtSvcAccountsService) removeExtSvcAccountToken(ctx context.Context, orgID, saID int64, extSvcSlug string) error { + ctx, span := esa.tracer.Start(ctx, "ExtSvcAccountsService.removeExtSvcAccountToken") + defer span.End() + + ctxLogger := esa.logger.FromContext(ctx) + ctxLogger.Debug("List service account tokens", "service", extSvcSlug, "orgID", orgID) + tokens, err := esa.saSvc.ListTokens(ctx, &sa.GetSATokensQuery{OrgID: &orgID, ServiceAccountID: &saID}) + if err != nil { + return err + } + notFound := int64(-1) + tknID := notFound + for _, token := range tokens { + if token.Name == tokenName(extSvcSlug) { + ctxLogger.Debug("Found token", "service", extSvcSlug, "orgID", orgID) + tknID = token.ID + break + } + } + if tknID != notFound { + ctxLogger.Debug("Remove token", "service", extSvcSlug, "orgID", orgID) + if err := esa.saSvc.DeleteServiceAccountToken(ctx, orgID, saID, tknID); err != nil { + return err + } + } + return esa.DeleteExtSvcCredentials(ctx, orgID, extSvcSlug) +} + +// FIXME: If the warning log never appears, we can remove this function func genTokenWithRetries(ctxLogger log.Logger, extSvcSlug string) (satokengen.KeyGenResult, error) { var newKeyInfo satokengen.KeyGenResult var err error @@ -437,9 +472,9 @@ func (esa *ExtSvcAccountsService) GetExtSvcCredentials(ctx context.Context, orgI if !ok { return nil, ErrCredentialsNotFound.Errorf("No credential found for in store %v", extSvcSlug) } - if strings.Contains(token, "\x00") { - ctxLogger.Warn("Loaded token from store containing NUL", "service", extSvcSlug) - logTokenNULParts(ctxLogger, extSvcSlug, token) + if _, err := satokengen.Decode(token); err != nil { + ctxLogger.Error("Failed to decode token", "error", err.Error()) + return nil, err } return &Credentials{Secret: token}, nil } @@ -475,3 +510,7 @@ func (esa *ExtSvcAccountsService) handlePluginStateChanged(ctx context.Context, } return errEnable } + +func tokenName(extSvcSlug string) string { + return tokenNamePrefix + "-" + extSvcSlug +} diff --git a/pkg/services/serviceaccounts/extsvcaccounts/service_test.go b/pkg/services/serviceaccounts/extsvcaccounts/service_test.go index 39a22eee791..8eb9edca532 100644 --- a/pkg/services/serviceaccounts/extsvcaccounts/service_test.go +++ b/pkg/services/serviceaccounts/extsvcaccounts/service_test.go @@ -7,6 +7,7 @@ import ( "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" + "github.com/grafana/grafana/pkg/components/satokengen" "github.com/grafana/grafana/pkg/infra/localcache" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/infra/tracing" @@ -199,6 +200,8 @@ func TestExtSvcAccountsService_ManageExtSvcAccount(t *testing.T) { func TestExtSvcAccountsService_SaveExternalService(t *testing.T) { extSvcSlug := "grafana-test-app" + validToken, err := satokengen.New(extSvcSlug) + require.NoError(t, err, "failed to generate a valid token for the tests") tmpOrgID := int64(1) extSvcAccID := int64(10) extSvcPerms := []ac.Permission{{Action: ac.ActionUsersRead, Scope: ac.ScopeUsersAll}} @@ -235,7 +238,7 @@ func TestExtSvcAccountsService_SaveExternalService(t *testing.T) { })). Return(nil) // A token was previously stored in the secret store - _ = env.SkvStore.Set(context.Background(), tmpOrgID, extSvcSlug, kvStoreType, "ExtSvcSecretToken") + _ = env.SkvStore.Set(context.Background(), tmpOrgID, extSvcSlug, kvStoreType, validToken.ClientSecret) }, cmd: extsvcauth.ExternalServiceRegistration{ Name: extSvcSlug, @@ -264,7 +267,7 @@ func TestExtSvcAccountsService_SaveExternalService(t *testing.T) { env.SaSvc.On("DeleteServiceAccount", mock.Anything, tmpOrgID, extSvcAccID).Return(nil) env.AcStore.On("DeleteExternalServiceRole", mock.Anything, extSvcSlug).Return(nil) // A token was previously stored in the secret store - _ = env.SkvStore.Set(context.Background(), tmpOrgID, extSvcSlug, kvStoreType, "ExtSvcSecretToken") + _ = env.SkvStore.Set(context.Background(), tmpOrgID, extSvcSlug, kvStoreType, validToken.ClientSecret) }, cmd: extsvcauth.ExternalServiceRegistration{ Name: extSvcSlug, @@ -335,7 +338,52 @@ func TestExtSvcAccountsService_SaveExternalService(t *testing.T) { Return(nil) env.SaSvc.On("EnableServiceAccount", mock.Anything, extsvcauth.TmpOrgID, int64(11), true).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") + _ = env.SkvStore.Set(context.Background(), tmpOrgID, extSvcSlug, kvStoreType, validToken.ClientSecret) + }, + cmd: extsvcauth.ExternalServiceRegistration{ + Name: extSvcSlug, + Self: extsvcauth.SelfCfg{ + Enabled: true, + Permissions: extSvcPerms, + }, + }, + want: &extsvcauth.ExternalService{ + Name: extSvcSlug, + ID: extSvcSlug, + Secret: "not empty", + }, + wantErr: false, + }, + { + name: "should recover from a failed token encryption", + init: func(env *TestEnv) { + // A previous service account was attached to this slug + env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, tmpOrgID, sa.ExtSvcPrefix+extSvcSlug). + Return(int64(11), nil) + env.AcStore.On("SaveExternalServiceRole", + mock.Anything, + mock.MatchedBy(func(cmd ac.SaveExternalServiceRoleCommand) bool { + return cmd.ServiceAccountID == int64(11) && cmd.ExternalServiceID == extSvcSlug && + cmd.AssignmentOrgID == tmpOrgID && len(cmd.Permissions) == 1 && + cmd.Permissions[0] == extSvcPerms[0] + })). + Return(nil) + env.SaSvc.On("EnableServiceAccount", mock.Anything, extsvcauth.TmpOrgID, int64(11), true).Return(nil) + + // This time the token in the secret store is invalid + _ = env.SkvStore.Set(context.Background(), tmpOrgID, extSvcSlug, kvStoreType, "failedTokenEncryption") + // Delete the API key and entry in the skv store + env.SaSvc.On("ListTokens", mock.Anything, mock.Anything). + Return([]apikey.APIKey{{ID: 3, Name: tokenNamePrefix + "-" + extSvcSlug}}, nil) + env.SaSvc.On("DeleteServiceAccountToken", mock.Anything, mock.Anything, int64(11), int64(3)).Return(nil) + // Recreate the API key + env.SaSvc.On("AddServiceAccountToken", mock.Anything, mock.Anything, mock.Anything).Return(&apikey.APIKey{}, nil) + }, + checks: func(t *testing.T, env *TestEnv) { + // Make sure the secret was updated + v, ok, _ := env.SkvStore.Get(context.Background(), tmpOrgID, extSvcSlug, kvStoreType) + require.True(t, ok, "secret should have been removed from store") + require.NotEqual(t, "failedTokenEncryption", v) }, cmd: extsvcauth.ExternalServiceRegistration{ Name: extSvcSlug,