From 3d9908f36316c00c53e766753ebfe8cdc77324cb Mon Sep 17 00:00:00 2001 From: Gabriel MABILLE Date: Tue, 28 May 2024 10:39:46 +0200 Subject: [PATCH] Fix: Prevent ExtSvcTokens from containing nil characters (#88243) * Fix: Prevent ExtSvcTokens from containing nil characters * Rebase * Add more logs * Nit. nil -> NUL * Nit. Part -> Parts * Back to const * Account for comments Co-authored-by: Claudiu Dragalina-Paraipan --------- Co-authored-by: Claudiu Dragalina-Paraipan --- .../pluginconfig/envvars.go | 2 +- .../serviceaccounts/extsvcaccounts/models.go | 17 +++--- .../serviceaccounts/extsvcaccounts/service.go | 52 ++++++++++++++++++- 3 files changed, 62 insertions(+), 9 deletions(-) diff --git a/pkg/services/pluginsintegration/pluginconfig/envvars.go b/pkg/services/pluginsintegration/pluginconfig/envvars.go index 2871ece5789..fe3579661e1 100644 --- a/pkg/services/pluginsintegration/pluginconfig/envvars.go +++ b/pkg/services/pluginsintegration/pluginconfig/envvars.go @@ -179,7 +179,7 @@ func (p *EnvVarsProvider) pluginSettingsEnvVars(pluginID string) []string { func (p *EnvVarsProvider) envVar(key, value string) string { if strings.Contains(value, "\x00") { - p.logger.Error("Variable with key '%s' contains a nil character", key) + p.logger.Error("Variable with key '%s' contains NUL", key) } return fmt.Sprintf("%s=%s", key, value) } diff --git a/pkg/services/serviceaccounts/extsvcaccounts/models.go b/pkg/services/serviceaccounts/extsvcaccounts/models.go index f6ab4ee4636..fd74e821e7f 100644 --- a/pkg/services/serviceaccounts/extsvcaccounts/models.go +++ b/pkg/services/serviceaccounts/extsvcaccounts/models.go @@ -15,16 +15,19 @@ const ( kvStoreType = "extsvc-token" // #nosec G101 - this is not a hardcoded secret tokenNamePrefix = "extsvc-token" + + maxTokenGenRetries = 10 ) var ( - ErrCannotBeDeleted = errutil.BadRequest("extsvcaccounts.ErrCannotBeDeleted", errutil.WithPublicMessage("external service account cannot be deleted")) - ErrCannotBeUpdated = errutil.BadRequest("extsvcaccounts.ErrCannotBeUpdated", errutil.WithPublicMessage("external service account cannot be updated")) - ErrCannotCreateToken = errutil.BadRequest("extsvcaccounts.ErrCannotCreateToken", errutil.WithPublicMessage("cannot add external service account token")) - ErrCannotDeleteToken = errutil.BadRequest("extsvcaccounts.ErrCannotDeleteToken", errutil.WithPublicMessage("cannot delete external service account token")) - ErrCannotListTokens = errutil.BadRequest("extsvcaccounts.ErrCannotListTokens", errutil.WithPublicMessage("cannot list external service account tokens")) - ErrCredentialsNotFound = errutil.NotFound("extsvcaccounts.credentials-not-found") - ErrInvalidName = errutil.BadRequest("extsvcaccounts.ErrInvalidName", errutil.WithPublicMessage("only external service account names can be prefixed with 'extsvc-'")) + ErrCannotBeDeleted = errutil.BadRequest("extsvcaccounts.ErrCannotBeDeleted", errutil.WithPublicMessage("external service account cannot be deleted")) + ErrCannotBeUpdated = errutil.BadRequest("extsvcaccounts.ErrCannotBeUpdated", errutil.WithPublicMessage("external service account cannot be updated")) + ErrCannotCreateToken = errutil.BadRequest("extsvcaccounts.ErrCannotCreateToken", errutil.WithPublicMessage("cannot add external service account token")) + ErrCannotDeleteToken = errutil.BadRequest("extsvcaccounts.ErrCannotDeleteToken", errutil.WithPublicMessage("cannot delete external service account token")) + ErrCannotListTokens = errutil.BadRequest("extsvcaccounts.ErrCannotListTokens", errutil.WithPublicMessage("cannot list external service account tokens")) + ErrCredentialsGenFailed = errutil.Internal("extsvcaccounts.ErrCredentialsGenFailed") + ErrCredentialsNotFound = errutil.NotFound("extsvcaccounts.ErrCredentialsNotFound") + ErrInvalidName = errutil.BadRequest("extsvcaccounts.ErrInvalidName", errutil.WithPublicMessage("only external service account names can be prefixed with 'extsvc-'")) extsvcuser = &user.SignedInUser{ OrgID: extsvcauth.TmpOrgID, diff --git a/pkg/services/serviceaccounts/extsvcaccounts/service.go b/pkg/services/serviceaccounts/extsvcaccounts/service.go index 3c722a8c19f..d7b462088e2 100644 --- a/pkg/services/serviceaccounts/extsvcaccounts/service.go +++ b/pkg/services/serviceaccounts/extsvcaccounts/service.go @@ -355,7 +355,7 @@ func (esa *ExtSvcAccountsService) getExtSvcAccountToken(ctx context.Context, org // Generate token ctxLogger.Info("Generate new service account token", "service", extSvcSlug, "orgID", orgID) - newKeyInfo, err := satokengen.New(extSvcSlug) + newKeyInfo, err := genTokenWithRetries(ctxLogger, extSvcSlug) if err != nil { return "", err } @@ -380,6 +380,52 @@ func (esa *ExtSvcAccountsService) getExtSvcAccountToken(ctx context.Context, org return newKeyInfo.ClientSecret, nil } +func genTokenWithRetries(ctxLogger log.Logger, extSvcSlug string) (satokengen.KeyGenResult, error) { + var newKeyInfo satokengen.KeyGenResult + var err error + retry := 0 + for retry < maxTokenGenRetries { + newKeyInfo, err = satokengen.New(extSvcSlug) + if err != nil { + return satokengen.KeyGenResult{}, err + } + + if !strings.Contains(newKeyInfo.ClientSecret, "\x00") { + return newKeyInfo, nil + } + + retry++ + + ctxLogger.Warn("Generated a token containing NUL, retrying", + "service", extSvcSlug, + "retry", retry, + ) + // On first retry, log the token parts that contain a nil byte + if retry == 1 { + logTokenNULParts(ctxLogger, extSvcSlug, newKeyInfo.ClientSecret) + } + } + + return satokengen.KeyGenResult{}, ErrCredentialsGenFailed.Errorf("Failed to generate a token for %s", extSvcSlug) +} + +// logTokenNULParts logs a warning if the external service token contains a nil byte +// Tokens normally have 3 parts "gl+serviceID_secret_checksum" +// Log the part of the generated token that contains a nil byte +func logTokenNULParts(ctxLogger log.Logger, extSvcSlug string, token string) { + parts := strings.Split(token, "_") + for i := range parts { + if strings.Contains(parts[i], "\x00") { + ctxLogger.Warn("Token contains NUL", + "service", extSvcSlug, + "part", i, + "part_len", len(parts[i]), + "parts_count", len(parts), + ) + } + } +} + // 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) { ctxLogger := esa.logger.FromContext(ctx) @@ -391,6 +437,10 @@ 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) + } return &Credentials{Secret: token}, nil }