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

@@ -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.

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)

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

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
 }
 

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

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{

public/app/features/provisioning/ConfigForm.tsx

@@ -228,7 +228,7 @@ export function ConfigForm({ data }: ConfigFormProps) {
           />
         </Field>
         <Field label={'Interval (seconds)'}>
-          <Input {...register('sync.intervalSeconds')} type={'number'} placeholder={'60'} />
+          <Input {...register('sync.intervalSeconds', {valueAsNumber: true})} type={'number'} placeholder={'60'} />
         </Field>
       </FieldSet>
       <FieldSet label="Advanced Settings">