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

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
+}

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,