From 158eebe45b7f35e7965542fa81dceaf4a5298024 Mon Sep 17 00:00:00 2001 From: Mariell Hoversholm Date: Thu, 13 Feb 2025 10:33:26 +0100 Subject: [PATCH] Provisioning: Encrypt GitHub token (#100515) * feat: new secrets impl * fix: send interval as a number * feat: use UUIDs for webhook secrets * Provisioning: Encrypt GitHub token * refactor: have a wrapping interface * refactor: simplify decryption * refactor: move encryption to mutation hook * feat: use omitempty again * feat: always decrypt * chore: update comment to reality --- pkg/apis/provisioning/v0alpha1/types.go | 5 +- .../provisioning/controller/repository.go | 22 +++++--- pkg/registry/apis/provisioning/register.go | 29 +++++++++- .../apis/provisioning/repository/github.go | 55 ++++++++++--------- .../apis/provisioning/secrets/secret.go | 33 +++++++---- pkg/services/secrets/migrator/migrator.go | 1 + .../app/features/provisioning/ConfigForm.tsx | 2 +- 7 files changed, 94 insertions(+), 53 deletions(-) diff --git a/pkg/apis/provisioning/v0alpha1/types.go b/pkg/apis/provisioning/v0alpha1/types.go index 203c59d4dd3..a245246335f 100644 --- a/pkg/apis/provisioning/v0alpha1/types.go +++ b/pkg/apis/provisioning/v0alpha1/types.go @@ -51,10 +51,13 @@ type GitHubRepositoryConfig struct { // By default, this is the main branch. Branch string `json:"branch,omitempty"` - // Token for accessing the repository. + // Token for accessing the repository. If set, it will be encrypted into encryptedToken, then set to an empty string again. // TODO: this should be part of secrets and a simple reference. Token string `json:"token,omitempty"` + // Token for accessing the repository, but encrypted. This is not possible to read back to a user decrypted. + EncryptedToken []byte `json:"encryptedToken,omitempty"` + // Workflow allowed for changes to the repository. // The order is relevant for defining the precedence of the workflows. // Possible values: pull-request, branch, push. diff --git a/pkg/registry/apis/provisioning/controller/repository.go b/pkg/registry/apis/provisioning/controller/repository.go index 987db9fb99a..2cbbd48e5af 100644 --- a/pkg/registry/apis/provisioning/controller/repository.go +++ b/pkg/registry/apis/provisioning/controller/repository.go @@ -24,6 +24,7 @@ import ( "github.com/grafana/grafana/pkg/registry/apis/provisioning/jobs" "github.com/grafana/grafana/pkg/registry/apis/provisioning/repository" "github.com/grafana/grafana/pkg/registry/apis/provisioning/resources" + "github.com/grafana/grafana/pkg/registry/apis/provisioning/secrets" ) type RepoGetter interface { @@ -57,6 +58,7 @@ type RepositoryController struct { repoSynced cache.InformerSynced parsers *resources.ParserFactory logger logging.Logger + secrets secrets.Service jobs jobs.JobQueue finalizer *finalizer @@ -81,6 +83,7 @@ func NewRepositoryController( parsers *resources.ParserFactory, tester RepositoryTester, jobs jobs.JobQueue, + secrets secrets.Service, ) (*RepositoryController, error) { rc := &RepositoryController{ client: provisioningClient, @@ -99,9 +102,10 @@ func NewRepositoryController( lister: resourceLister, client: parsers.Client, }, - tester: tester, - jobs: jobs, - logger: logging.DefaultLogger.With("logger", loggerName), + tester: tester, + jobs: jobs, + logger: logging.DefaultLogger.With("logger", loggerName), + secrets: secrets, } _, err := repoInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ @@ -229,6 +233,12 @@ func (rc *RepositoryController) process(item *queueItem) error { return err } + ctx, _, err := identity.WithProvisioningIdentitiy(context.Background(), namespace) + if err != nil { + return err + } + logger = logger.WithContext(ctx) + healthAge := time.Since(time.UnixMilli(obj.Status.Health.Checked)) syncAge := time.Since(time.UnixMilli(obj.Status.Sync.Finished)) syncInterval := time.Duration(obj.Spec.Sync.IntervalSeconds) * time.Second @@ -244,12 +254,6 @@ func (rc *RepositoryController) process(item *queueItem) error { logger.Info("conditions met", "status", obj.Status, "generation", obj.Generation, "deletion_timestamp", obj.DeletionTimestamp, "sync_spec", obj.Spec.Sync) } - ctx, _, err := identity.WithProvisioningIdentitiy(context.Background(), namespace) - if err != nil { - return err - } - logger = logger.WithContext(ctx) - repo, err := rc.repoGetter.AsRepository(ctx, obj) if err != nil { return fmt.Errorf("unable to create repository from configuration: %w", err) diff --git a/pkg/registry/apis/provisioning/register.go b/pkg/registry/apis/provisioning/register.go index 3a95640f7b9..28003b9cb3b 100644 --- a/pkg/registry/apis/provisioning/register.go +++ b/pkg/registry/apis/provisioning/register.go @@ -44,6 +44,7 @@ import ( "github.com/grafana/grafana/pkg/services/apiserver/builder" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/rendering" + grafanasecrets "github.com/grafana/grafana/pkg/services/secrets" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/storage/unified/blob" "github.com/grafana/grafana/pkg/storage/unified/resource" @@ -77,6 +78,7 @@ type APIBuilder struct { tester *RepositoryTester resourceLister resources.ResourceLister repositoryLister listers.RepositoryLister + secrets secrets.Service } // NewAPIBuilder creates an API builder. @@ -93,6 +95,7 @@ func NewAPIBuilder( clonedir string, // where repo clones are managed configProvider apiserver.RestConfigProvider, ghFactory github.ClientFactory, + secrets secrets.Service, ) *APIBuilder { clientFactory := resources.NewFactory(configProvider) return &APIBuilder{ @@ -109,6 +112,7 @@ func NewAPIBuilder( clonedir: clonedir, resourceLister: resources.NewResourceLister(index), blobstore: blobstore, + secrets: secrets, } } @@ -124,6 +128,8 @@ func RegisterAPIService( client resource.ResourceClient, // implements resource.RepositoryClient configProvider apiserver.RestConfigProvider, ghFactory github.ClientFactory, + // FIXME: use multi-tenant service when one exists. In this state, we can't make this a multi-tenant service! + secretssvc grafanasecrets.Service, ) (*APIBuilder, error) { if !features.IsEnabledGlobally(featuremgmt.FlagProvisioning) && !features.IsEnabledGlobally(featuremgmt.FlagGrafanaAPIServerWithExperimentalAPIs) { @@ -147,7 +153,7 @@ func RegisterAPIService( builder := NewAPIBuilder(folderResolver, urlProvider, cfg.SecretKey, features, render, client, store, filepath.Join(cfg.DataPath, "clone"), // where repositories are cloned (temporarialy for now) - configProvider, ghFactory) + configProvider, ghFactory, secrets.NewSingleTenant(secretssvc)) apiregistration.RegisterAPI(builder) return builder, nil } @@ -311,8 +317,7 @@ func (b *APIBuilder) AsRepository(ctx context.Context, r *provisioning.Repositor gvr.Resource, r.GetName(), ) - secretsSvc := secrets.NewService(b.webhookSecretKey) - return repository.NewGitHub(ctx, r, b.ghFactory, secretsSvc, webhookURL), nil + return repository.NewGitHub(ctx, r, b.ghFactory, b.secrets, webhookURL) case provisioning.S3RepositoryType: return repository.NewS3(r), nil default: @@ -366,6 +371,10 @@ func (b *APIBuilder) Mutate(ctx context.Context, a admission.Attributes, o admis } } + if err := b.encryptSecrets(ctx, r); err != nil { + return fmt.Errorf("failed to encrypt secrets: %w", err) + } + return nil } @@ -475,6 +484,7 @@ func (b *APIBuilder) GetPostStartHooks() (map[string]genericapiserver.PostStartH b.parsers, &repository.Tester{}, b.jobs, + b.secrets, ) if err != nil { return err @@ -761,3 +771,16 @@ spec: return oas, nil } + +func (b *APIBuilder) encryptSecrets(ctx context.Context, repo *provisioning.Repository) error { + var err error + if repo.Spec.GitHub != nil && + repo.Spec.GitHub.Token != "" { + repo.Spec.GitHub.EncryptedToken, err = b.secrets.Encrypt(ctx, []byte(repo.Spec.GitHub.Token)) + if err != nil { + return err + } + repo.Spec.GitHub.Token = "" + } + return nil +} diff --git a/pkg/registry/apis/provisioning/repository/github.go b/pkg/registry/apis/provisioning/repository/github.go index 850e125e276..38392403c0d 100644 --- a/pkg/registry/apis/provisioning/repository/github.go +++ b/pkg/registry/apis/provisioning/repository/github.go @@ -17,22 +17,20 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/validation/field" + "github.com/google/uuid" "github.com/grafana/grafana-app-sdk/logging" provisioning "github.com/grafana/grafana/pkg/apis/provisioning/v0alpha1" pgh "github.com/grafana/grafana/pkg/registry/apis/provisioning/repository/github" + "github.com/grafana/grafana/pkg/registry/apis/provisioning/secrets" ) var subscribedEvents = []string{"push", "pull_request"} -type SecretsService interface { - Encrypt(ctx context.Context, data string) (string, error) -} - // Make sure all public functions of this struct call the (*githubRepository).logger function, to ensure the GH repo details are included. type githubRepository struct { config *provisioning.Repository gh pgh.Client // assumes github.com base URL - secrets SecretsService + secrets secrets.Service webhookURL string owner string @@ -45,18 +43,22 @@ func NewGitHub( ctx context.Context, config *provisioning.Repository, factory pgh.ClientFactory, - secrets SecretsService, + secrets secrets.Service, webhookURL string, -) *githubRepository { +) (*githubRepository, error) { owner, repo, _ := parseOwnerRepo(config.Spec.GitHub.URL) + decrypted, err := secrets.Decrypt(ctx, config.Spec.GitHub.EncryptedToken) + if err != nil { + return nil, err + } return &githubRepository{ config: config, - gh: factory.New(ctx, config.Spec.GitHub.Token), // TODO -- base from URL + gh: factory.New(ctx, string(decrypted)), // TODO -- base from URL secrets: secrets, webhookURL: webhookURL, owner: owner, repo: repo, - } + }, nil } func (r *githubRepository) Config() *provisioning.Repository { @@ -86,7 +88,8 @@ func (r *githubRepository) Validate() (list field.ErrorList) { if !isValidGitBranchName(gh.Branch) { list = append(list, field.Invalid(field.NewPath("spec", "github", "branch"), gh.Branch, "invalid branch name")) } - if gh.Token == "" { + // TODO: Use two fields for token + if gh.Token == "" && len(gh.EncryptedToken) == 0 { list = append(list, field.Required(field.NewPath("spec", "github", "token"), "a github access token is required")) } @@ -741,14 +744,14 @@ func (r *githubRepository) CommentPullRequestFile(ctx context.Context, prNumber } func (r *githubRepository) createWebhook(ctx context.Context) (pgh.WebhookConfig, error) { - secret, err := r.secrets.Encrypt(ctx, r.config.Spec.GitHub.Token) + secret, err := uuid.NewRandom() if err != nil { - return pgh.WebhookConfig{}, fmt.Errorf("encrypt webhook secret: %w", err) + return pgh.WebhookConfig{}, fmt.Errorf("could not generate secret: %w", err) } cfg := pgh.WebhookConfig{ URL: r.webhookURL, - Secret: secret, + Secret: secret.String(), ContentType: "json", Events: subscribedEvents, Active: true, @@ -759,6 +762,9 @@ func (r *githubRepository) createWebhook(ctx context.Context) (pgh.WebhookConfig return pgh.WebhookConfig{}, err } + // HACK: GitHub does not return the secret, so we need to update it manually + hook.Secret = cfg.Secret + logging.FromContext(ctx).Info("webhook created", "url", cfg.URL, "id", hook.ID) return hook, nil } @@ -786,19 +792,10 @@ func (r *githubRepository) updateWebhook(ctx context.Context) (pgh.WebhookConfig return pgh.WebhookConfig{}, false, fmt.Errorf("get webhook: %w", err) } + hook.Secret = r.config.Status.Webhook.Secret // we always random gen this, so don't use it for mustUpdate below. + var mustUpdate bool - secret, err := r.secrets.Encrypt(ctx, r.config.Spec.GitHub.Token) - if err != nil { - return pgh.WebhookConfig{}, false, fmt.Errorf("encrypt webhook secret: %w", err) - } - - // Compare with status secret as we cannot get the screen from the webhook - if secret != r.config.Status.Webhook.Secret { - mustUpdate = true - hook.Secret = r.config.Status.Webhook.Secret - } - if hook.URL != r.config.Status.Webhook.URL { mustUpdate = true hook.URL = r.webhookURL @@ -813,13 +810,17 @@ func (r *githubRepository) updateWebhook(ctx context.Context) (pgh.WebhookConfig return hook, false, nil } + // Something has changed in the webhook. Let's rotate the secret as well, so as to ensure we end up with a 100% correct webhook. + secret, err := uuid.NewRandom() + if err != nil { + return pgh.WebhookConfig{}, false, fmt.Errorf("could not generate secret: %w", err) + } + hook.Secret = secret.String() + if err := r.gh.EditWebhook(ctx, r.owner, r.repo, hook); err != nil { return pgh.WebhookConfig{}, false, fmt.Errorf("edit webhook: %w", err) } - // HACK: GitHub does not return the secret, so we need to update it manually - hook.Secret = secret - return hook, true, nil } diff --git a/pkg/registry/apis/provisioning/secrets/secret.go b/pkg/registry/apis/provisioning/secrets/secret.go index 3caa141f04a..b7699ab057e 100644 --- a/pkg/registry/apis/provisioning/secrets/secret.go +++ b/pkg/registry/apis/provisioning/secrets/secret.go @@ -2,25 +2,34 @@ package secrets import ( "context" - "crypto/hmac" - "crypto/sha256" - "encoding/hex" + + "github.com/grafana/grafana/pkg/services/secrets" ) +// A secrets encryption service. It only operates on values, no names or similar. +// It is likely we will need to change this when the multi-tenant service comes around. +// // FIXME: this is a temporary service/package until we can make use of // the new secrets service in app platform -type Service struct { - encryptionKey []byte +type Service interface { + Encrypt(ctx context.Context, data []byte) ([]byte, error) + Decrypt(ctx context.Context, data []byte) ([]byte, error) } -func NewService(encryptionKey string) *Service { - return &Service{encryptionKey: []byte(encryptionKey)} +var _ Service = (*singleTenant)(nil) + +type singleTenant struct { + inner secrets.Service } -func (s *Service) Encrypt(ctx context.Context, data string) (string, error) { - h := hmac.New(sha256.New, s.encryptionKey) - h.Write([]byte(data)) - hashed := h.Sum(nil) +func NewSingleTenant(svc secrets.Service) *singleTenant { + return &singleTenant{svc} +} - return hex.EncodeToString(hashed), nil +func (s *singleTenant) Encrypt(ctx context.Context, data []byte) ([]byte, error) { + return s.inner.Encrypt(ctx, data, secrets.WithoutScope()) +} + +func (s *singleTenant) Decrypt(ctx context.Context, data []byte) ([]byte, error) { + return s.inner.Decrypt(ctx, data) } diff --git a/pkg/services/secrets/migrator/migrator.go b/pkg/services/secrets/migrator/migrator.go index 3581c2afd99..d0dd383a62d 100644 --- a/pkg/services/secrets/migrator/migrator.go +++ b/pkg/services/secrets/migrator/migrator.go @@ -51,6 +51,7 @@ func ProvideSecretsMigrator( b64Secret{simpleSecret: simpleSecret{tableName: "user_external_session", columnName: "refresh_token"}, encoding: base64.StdEncoding}, b64Secret{simpleSecret: simpleSecret{tableName: "user_external_session", columnName: "session_id"}, encoding: base64.StdEncoding}, b64Secret{simpleSecret: simpleSecret{tableName: "user_external_session", columnName: "name_id"}, encoding: base64.StdEncoding}, + // FIXME: Rotate provisioning secrets } return &SecretsMigrator{ diff --git a/public/app/features/provisioning/ConfigForm.tsx b/public/app/features/provisioning/ConfigForm.tsx index 00d8b338ba0..feee5b851aa 100644 --- a/public/app/features/provisioning/ConfigForm.tsx +++ b/public/app/features/provisioning/ConfigForm.tsx @@ -228,7 +228,7 @@ export function ConfigForm({ data }: ConfigFormProps) { /> - +