Chore: Use KVStore for the manifest public key (#66839)

pkg/plugins/manager/signature/manifest.go

@@ -58,17 +58,25 @@ type Signature struct {
 
 var _ plugins.SignatureCalculator = &Signature{}
 
-func ProvideService(cfg *config.Cfg) *Signature {
+func ProvideService(cfg *config.Cfg, kv plugins.KeyStore) *Signature {
 	log := log.New("plugin.signature")
 	return &Signature{
-		verifier: manifestverifier.New(cfg, log),
+		verifier: manifestverifier.New(cfg, log, kv),
 		mlog:     log,
 	}
 }
 
+func (s *Signature) IsDisabled() bool {
+	return s.verifier.IsDisabled()
+}
+
+func (s *Signature) Run(ctx context.Context) error {
+	return s.verifier.Run(ctx)
+}
+
 // readPluginManifest attempts to read and verify the plugin manifest
 // if any error occurs or the manifest is not valid, this will return an error
-func (s *Signature) readPluginManifest(body []byte) (*PluginManifest, error) {
+func (s *Signature) readPluginManifest(ctx context.Context, body []byte) (*PluginManifest, error) {
 	block, _ := clearsign.Decode(body)
 	if block == nil {
 		return nil, errors.New("unable to decode manifest")
@@ -81,7 +89,7 @@ func (s *Signature) readPluginManifest(body []byte) (*PluginManifest, error) {
 		return nil, fmt.Errorf("%v: %w", "Error parsing manifest JSON", err)
 	}
 
-	if err = s.validateManifest(manifest, block); err != nil {
+	if err = s.validateManifest(ctx, manifest, block); err != nil {
 		return nil, err
 	}
 
@@ -131,7 +139,7 @@ func (s *Signature) Calculate(ctx context.Context, src plugins.PluginSource, plu
 		}, nil
 	}
 
-	manifest, err := s.readPluginManifest(byteValue)
+	manifest, err := s.readPluginManifest(ctx, byteValue)
 	if err != nil {
 		s.mlog.Debug("Plugin signature invalid", "id", plugin.JSONData.ID, "err", err)
 		return plugins.Signature{
@@ -286,7 +294,7 @@ func (r invalidFieldErr) Error() string {
 	return fmt.Sprintf("valid manifest field %s is required", r.field)
 }
 
-func (s *Signature) validateManifest(m PluginManifest, block *clearsign.Block) error {
+func (s *Signature) validateManifest(ctx context.Context, m PluginManifest, block *clearsign.Block) error {
 	if len(m.Plugin) == 0 {
 		return invalidFieldErr{field: "plugin"}
 	}
@@ -314,5 +322,5 @@ func (s *Signature) validateManifest(m PluginManifest, block *clearsign.Block) e
 		}
 	}
 
-	return s.verifier.Verify(m.KeyID, block)
+	return s.verifier.Verify(ctx, m.KeyID, block)
 }

pkg/plugins/manager/signature/manifest_test.go

@@ -10,9 +10,11 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
+	"github.com/grafana/grafana/pkg/infra/kvstore"
 	"github.com/grafana/grafana/pkg/plugins"
 	"github.com/grafana/grafana/pkg/plugins/config"
 	"github.com/grafana/grafana/pkg/plugins/manager/fakes"
+	"github.com/grafana/grafana/pkg/services/pluginsintegration/keystore"
 	"github.com/grafana/grafana/pkg/setting"
 )
 
@@ -49,8 +51,8 @@ NR7DnB0CCQHO+4FlSPtXFTzNepoc+CytQyDAeOLMLmf2Tqhk2YShk+G/YlVX
 -----END PGP SIGNATURE-----`
 
 	t.Run("valid manifest", func(t *testing.T) {
-		s := ProvideService(&config.Cfg{})
-		manifest, err := s.readPluginManifest([]byte(txt))
+		s := ProvideService(&config.Cfg{}, keystore.ProvideService(kvstore.NewFakeKVStore()))
+		manifest, err := s.readPluginManifest(context.Background(), []byte(txt))
 
 		require.NoError(t, err)
 		require.NotNil(t, manifest)
@@ -66,8 +68,8 @@ NR7DnB0CCQHO+4FlSPtXFTzNepoc+CytQyDAeOLMLmf2Tqhk2YShk+G/YlVX
 
 	t.Run("invalid manifest", func(t *testing.T) {
 		modified := strings.ReplaceAll(txt, "README.md", "xxxxxxxxxx")
-		s := ProvideService(&config.Cfg{})
-		_, err := s.readPluginManifest([]byte(modified))
+		s := ProvideService(&config.Cfg{}, keystore.ProvideService(kvstore.NewFakeKVStore()))
+		_, err := s.readPluginManifest(context.Background(), []byte(modified))
 		require.Error(t, err)
 	})
 }
@@ -104,8 +106,8 @@ khdr/tZ1PDgRxMqB/u+Vtbpl0xSxgblnrDOYMSI=
 -----END PGP SIGNATURE-----`
 
 	t.Run("valid manifest", func(t *testing.T) {
-		s := ProvideService(&config.Cfg{})
-		manifest, err := s.readPluginManifest([]byte(txt))
+		s := ProvideService(&config.Cfg{}, keystore.ProvideService(kvstore.NewFakeKVStore()))
+		manifest, err := s.readPluginManifest(context.Background(), []byte(txt))
 
 		require.NoError(t, err)
 		require.NotNil(t, manifest)
@@ -158,7 +160,7 @@ func TestCalculate(t *testing.T) {
 			setting.AppUrl = tc.appURL
 
 			basePath := filepath.Join(parentDir, "testdata/non-pvt-with-root-url/plugin")
-			s := ProvideService(&config.Cfg{})
+			s := ProvideService(&config.Cfg{}, keystore.ProvideService(kvstore.NewFakeKVStore()))
 			sig, err := s.Calculate(context.Background(), &fakes.FakePluginSource{
 				PluginClassFunc: func(ctx context.Context) plugins.Class {
 					return plugins.External
@@ -189,7 +191,7 @@ func TestCalculate(t *testing.T) {
 		basePath := "../testdata/renderer-added-file/plugin"
 
 		runningWindows = true
-		s := ProvideService(&config.Cfg{})
+		s := ProvideService(&config.Cfg{}, keystore.ProvideService(kvstore.NewFakeKVStore()))
 		sig, err := s.Calculate(context.Background(), &fakes.FakePluginSource{
 			PluginClassFunc: func(ctx context.Context) plugins.Class {
 				return plugins.External
@@ -238,7 +240,7 @@ func TestCalculate(t *testing.T) {
 
 				basePath := "../testdata/app-with-child/dist"
 
-				s := ProvideService(&config.Cfg{})
+				s := ProvideService(&config.Cfg{}, keystore.ProvideService(kvstore.NewFakeKVStore()))
 				sig, err := s.Calculate(context.Background(), &fakes.FakePluginSource{
 					PluginClassFunc: func(ctx context.Context) plugins.Class {
 						return plugins.External
@@ -683,8 +685,8 @@ func Test_validateManifest(t *testing.T) {
 	}
 	for _, tc := range tcs {
 		t.Run(tc.name, func(t *testing.T) {
-			s := ProvideService(&config.Cfg{})
-			err := s.validateManifest(*tc.manifest, nil)
+			s := ProvideService(&config.Cfg{}, keystore.ProvideService(kvstore.NewFakeKVStore()))
+			err := s.validateManifest(context.Background(), *tc.manifest, nil)
 			require.Errorf(t, err, tc.expectedErr)
 		})
 	}

pkg/plugins/manager/signature/manifestverifier/verifier.go

@@ -2,6 +2,7 @@ package manifestverifier
 
 import (
 	"bytes"
+	"context"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -14,10 +15,16 @@ import (
 	"github.com/ProtonMail/go-crypto/openpgp"
 	"github.com/ProtonMail/go-crypto/openpgp/clearsign"
 	"github.com/ProtonMail/go-crypto/openpgp/packet"
+	"github.com/grafana/grafana/pkg/plugins"
 	"github.com/grafana/grafana/pkg/plugins/config"
 	"github.com/grafana/grafana/pkg/plugins/log"
+
+	// Only used for getting the feature flag value
+	"github.com/grafana/grafana/pkg/services/featuremgmt"
 )
 
+const publicKeySyncInterval = 10 * 24 * time.Hour // 10 days
+
 // ManifestKeys is the database representation of public keys
 // used to verify plugin manifests.
 type ManifestKeys struct {
@@ -30,20 +37,82 @@ type ManifestVerifier struct {
 	cfg  *config.Cfg
 	mlog log.Logger
 
-	lock       sync.Mutex
-	cli        http.Client
-	publicKeys map[string]ManifestKeys
+	lock    sync.Mutex
+	cli     http.Client
+	kv      plugins.KeyStore
+	hasKeys bool
 }
 
-func New(cfg *config.Cfg, mlog log.Logger) *ManifestVerifier {
-	return &ManifestVerifier{
-		cfg:        cfg,
-		publicKeys: map[string]ManifestKeys{},
-		mlog:       mlog,
-		cli:        makeHttpClient(),
+func New(cfg *config.Cfg, mlog log.Logger, kv plugins.KeyStore) *ManifestVerifier {
+	pmv := &ManifestVerifier{
+		cfg:  cfg,
+		mlog: mlog,
+		cli:  makeHttpClient(),
+		kv:   kv,
 	}
+	return pmv
 }
 
+// IsDisabled disables dynamic retrieval of public keys from the API server.
+func (pmv *ManifestVerifier) IsDisabled() bool {
+	return pmv.cfg == nil || pmv.cfg.Features == nil || !pmv.cfg.Features.IsEnabled(featuremgmt.FlagPluginsAPIManifestKey)
+}
+
+func (pmv *ManifestVerifier) Run(ctx context.Context) error {
+	// do an initial update if necessary
+	err := pmv.updateKeys(ctx)
+	if err != nil {
+		pmv.mlog.Error("Error downloading plugin manifest keys", "error", err)
+	}
+
+	// calculate initial send delay
+	lastUpdated, err := pmv.kv.GetLastUpdated(ctx)
+	if err != nil {
+		return err
+	}
+	nextSendInterval := time.Until(lastUpdated.Add(publicKeySyncInterval))
+	if nextSendInterval < time.Minute {
+		nextSendInterval = time.Minute
+	}
+
+	downloadKeysTicker := time.NewTicker(nextSendInterval)
+	defer downloadKeysTicker.Stop()
+
+	select {
+	case <-downloadKeysTicker.C:
+		err = pmv.updateKeys(ctx)
+		if err != nil {
+			pmv.mlog.Error("Error downloading plugin manifest keys", "error", err)
+		}
+
+		if nextSendInterval != publicKeySyncInterval {
+			nextSendInterval = publicKeySyncInterval
+			downloadKeysTicker.Reset(nextSendInterval)
+		}
+	case <-ctx.Done():
+		return ctx.Err()
+	}
+
+	return ctx.Err()
+}
+
+func (pmv *ManifestVerifier) updateKeys(ctx context.Context) error {
+	pmv.lock.Lock()
+	defer pmv.lock.Unlock()
+
+	lastUpdated, err := pmv.kv.GetLastUpdated(ctx)
+	if err != nil {
+		return err
+	}
+	if time.Since(*lastUpdated) < publicKeySyncInterval {
+		// Cache is still valid
+		return nil
+	}
+
+	return pmv.downloadKeys(ctx)
+}
+
+const publicKeyID = "7e4d0c6a708866e7"
 const publicKeyText = `-----BEGIN PGP PUBLIC KEY BLOCK-----
 Version: OpenPGP.js v4.10.1
 Comment: https://openpgpjs.org
@@ -68,40 +137,24 @@ N1c5v9v/4h6qeA==
 -----END PGP PUBLIC KEY BLOCK-----
 `
 
-// getPublicKey loads public keys from:
-//   - The hard-coded value if the feature flag is not enabled.
-//   - A cached value from memory if it has been already retrieved.
-//   - The Grafana.com API if the database is empty.
-func (pmv *ManifestVerifier) GetPublicKey(keyID string) (string, error) {
-	if pmv.cfg == nil || pmv.cfg.Features == nil || !pmv.cfg.Features.IsEnabled("pluginsAPIManifestKey") {
-		return publicKeyText, nil
-	}
-
-	pmv.lock.Lock()
-	defer pmv.lock.Unlock()
-
-	key, exist := pmv.publicKeys[keyID]
-	if exist {
-		return key.PublicKey, nil
-	}
-
-	// Retrieve the key from the API and store it in the database
+// Retrieve the key from the API and store it in the database
+func (pmv *ManifestVerifier) downloadKeys(ctx context.Context) error {
 	var data struct {
 		Items []ManifestKeys
 	}
 
 	url, err := url.JoinPath(pmv.cfg.GrafanaComURL, "/api/plugins/ci/keys") // nolint:gosec URL is provided by config
 	if err != nil {
-		return "", err
+		return err
 	}
-	req, err := http.NewRequest(http.MethodGet, url, nil)
+	req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
 	if err != nil {
-		return "", err
+		return err
 	}
 
 	resp, err := pmv.cli.Do(req)
 	if err != nil {
-		return "", err
+		return err
 	}
 	defer func() {
 		err := resp.Body.Close()
@@ -111,27 +164,89 @@ func (pmv *ManifestVerifier) GetPublicKey(keyID string) (string, error) {
 	}()
 
 	if err := json.NewDecoder(resp.Body).Decode(&data); err != nil {
-		return "", err
+		return err
 	}
 
 	if len(data.Items) == 0 {
-		return "", errors.New("missing public key")
+		return errors.New("missing public key")
 	}
 
+	cachedKeys, err := pmv.kv.ListKeys(ctx)
+	if err != nil {
+		return err
+	}
+
+	shouldKeep := make(map[string]bool)
 	for _, key := range data.Items {
-		pmv.publicKeys[key.KeyID] = key
+		err = pmv.kv.Set(ctx, key.KeyID, key.PublicKey)
+		if err != nil {
+			return err
+		}
+		shouldKeep[key.KeyID] = true
 	}
 
-	key, exist = pmv.publicKeys[keyID]
+	// Delete keys that are no longer in the API
+	for _, key := range cachedKeys {
+		if !shouldKeep[key] {
+			err = pmv.kv.Del(ctx, key)
+			if err != nil {
+				return err
+			}
+		}
+	}
+
+	// Update the last updated timestamp
+	return pmv.kv.SetLastUpdated(ctx)
+}
+
+func (pmv *ManifestVerifier) ensureKeys(ctx context.Context) error {
+	if pmv.hasKeys {
+		return nil
+	}
+	keys, err := pmv.kv.ListKeys(ctx)
+	if err != nil {
+		return err
+	}
+	if len(keys) == 0 {
+		// Populate with the default key
+		err := pmv.kv.Set(ctx, publicKeyID, publicKeyText)
+		if err != nil {
+			return err
+		}
+	}
+	pmv.hasKeys = true
+	return nil
+}
+
+// getPublicKey loads public keys from:
+//   - The hard-coded value if the feature flag is not enabled.
+//   - A cached value from kv storage if it has been already retrieved. This cache is populated from the grafana.com API.
+func (pmv *ManifestVerifier) getPublicKey(ctx context.Context, keyID string) (string, error) {
+	if pmv.IsDisabled() {
+		return publicKeyText, nil
+	}
+
+	pmv.lock.Lock()
+	defer pmv.lock.Unlock()
+
+	err := pmv.ensureKeys(ctx)
+	if err != nil {
+		return "", err
+	}
+
+	key, exist, err := pmv.kv.Get(ctx, keyID)
+	if err != nil {
+		return "", err
+	}
 	if exist {
-		return key.PublicKey, nil
+		return key, nil
 	}
 
 	return "", fmt.Errorf("missing public key for %s", keyID)
 }
 
-func (pmv *ManifestVerifier) Verify(keyID string, block *clearsign.Block) error {
-	publicKey, err := pmv.GetPublicKey(keyID)
+func (pmv *ManifestVerifier) Verify(ctx context.Context, keyID string, block *clearsign.Block) error {
+	publicKey, err := pmv.getPublicKey(ctx, keyID)
 	if err != nil {
 		return err
 	}

pkg/plugins/manager/signature/manifestverifier/verifier_test.go

@@ -1,22 +1,26 @@
 package manifestverifier
 
 import (
+	"context"
 	"encoding/json"
 	"net/http"
 	"net/http/httptest"
 	"os"
 	"testing"
+	"time"
 
 	"github.com/ProtonMail/go-crypto/openpgp/clearsign"
+	"github.com/grafana/grafana/pkg/infra/kvstore"
 	"github.com/grafana/grafana/pkg/plugins/config"
 	"github.com/grafana/grafana/pkg/plugins/log"
 	"github.com/grafana/grafana/pkg/services/featuremgmt"
+	"github.com/grafana/grafana/pkg/services/pluginsintegration/keystore"
 	"github.com/stretchr/testify/require"
 )
 
 func Test_Verify(t *testing.T) {
 	t.Run("it should verify a manifest with the default key", func(t *testing.T) {
-		v := New(&config.Cfg{}, log.New("test"))
+		v := New(&config.Cfg{}, log.New("test"), keystore.ProvideService(kvstore.NewFakeKVStore()))
 
 		body, err := os.ReadFile("../../testdata/test-app/MANIFEST.txt")
 		if err != nil {
@@ -28,47 +32,107 @@ func Test_Verify(t *testing.T) {
 			t.Fatal("failed to decode")
 		}
 
-		err = v.Verify("7e4d0c6a708866e7", block)
+		err = v.Verify(context.Background(), "7e4d0c6a708866e7", block)
 		require.NoError(t, err)
 	})
+}
 
+func setFakeAPIServer(t *testing.T, publicKey string, keyID string) (*httptest.Server, chan bool) {
+	done := make(chan bool)
+	return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.URL.Path == "/api/plugins/ci/keys" {
+			w.WriteHeader(http.StatusOK)
+			data := struct {
+				Items []ManifestKeys `json:"items"`
+			}{
+				Items: []ManifestKeys{{PublicKey: publicKey, KeyID: keyID}},
+			}
+			b, err := json.Marshal(data)
+			if err != nil {
+				t.Fatal(err)
+			}
+			_, err = w.Write(b)
+			if err != nil {
+				t.Fatal(err)
+			}
+			require.NoError(t, err)
+			done <- true
+			return
+		}
+		w.WriteHeader(http.StatusNotFound)
+		done <- true
+	})), done
+}
+func Test_PublicKeyUpdate(t *testing.T) {
 	t.Run("it should verify a manifest with the API key", func(t *testing.T) {
 		cfg := &config.Cfg{
-			Features: featuremgmt.WithFeatures([]interface{}{"pluginsAPIManifestKey"}...),
+			Features: featuremgmt.WithFeatures([]interface{}{featuremgmt.FlagPluginsAPIManifestKey}...),
 		}
-		v := New(cfg, log.New("test"))
-		apiCalled := false
-		s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-			if r.URL.Path == "/api/plugins/ci/keys" {
-				w.WriteHeader(http.StatusOK)
-				data := struct {
-					Items []ManifestKeys `json:"items"`
-				}{
-					Items: []ManifestKeys{{PublicKey: publicKeyText, KeyID: "7e4d0c6a708866e7"}},
-				}
-				b, err := json.Marshal(data)
-				require.NoError(t, err)
-				_, err = w.Write(b)
-				require.NoError(t, err)
-				apiCalled = true
-				return
-			}
-			w.WriteHeader(http.StatusNotFound)
-		}))
+		expectedKey := "fake"
+		s, done := setFakeAPIServer(t, expectedKey, "7e4d0c6a708866e7")
 		cfg.GrafanaComURL = s.URL
+		v := New(cfg, log.New("test"), keystore.ProvideService(kvstore.NewFakeKVStore()))
+		go func() {
+			err := v.Run(context.Background())
+			require.NoError(t, err)
+		}()
+		<-done
 
-		body, err := os.ReadFile("../../testdata/test-app/MANIFEST.txt")
-		if err != nil {
-			t.Fatal(err)
+		// wait for the lock to be free
+		v.lock.Lock()
+		defer v.lock.Unlock()
+		res, found, err := v.kv.Get(context.Background(), "7e4d0c6a708866e7")
+		require.NoError(t, err)
+		require.Equal(t, true, found)
+		require.Equal(t, expectedKey, res)
+	})
+
+	t.Run("it should update the latest update date", func(t *testing.T) {
+		cfg := &config.Cfg{
+			Features: featuremgmt.WithFeatures([]interface{}{featuremgmt.FlagPluginsAPIManifestKey}...),
 		}
+		expectedKey := "fake"
+		s, done := setFakeAPIServer(t, expectedKey, "7e4d0c6a708866e7")
+		cfg.GrafanaComURL = s.URL
+		v := New(cfg, log.New("test"), keystore.ProvideService(kvstore.NewFakeKVStore()))
+		go func() {
+			err := v.Run(context.Background())
+			require.NoError(t, err)
+		}()
+		<-done
+
+		// wait for the lock to be free
+		v.lock.Lock()
+		defer v.lock.Unlock()
+		ti, err := v.kv.GetLastUpdated(context.Background())
+		require.NoError(t, err)
+		require.Less(t, time.Time{}, *ti)
+	})
 
-		block, _ := clearsign.Decode(body)
-		if block == nil {
-			t.Fatal("failed to decode")
+	t.Run("it should remove old keys", func(t *testing.T) {
+		cfg := &config.Cfg{
+			Features: featuremgmt.WithFeatures([]interface{}{featuremgmt.FlagPluginsAPIManifestKey}...),
 		}
+		expectedKey := "fake"
+		s, done := setFakeAPIServer(t, expectedKey, "other")
+		cfg.GrafanaComURL = s.URL
+		v := New(cfg, log.New("test"), keystore.ProvideService(kvstore.NewFakeKVStore()))
+		go func() {
+			err := v.Run(context.Background())
+			require.NoError(t, err)
+		}()
+		<-done
+
+		// wait for the lock to be free
+		v.lock.Lock()
+		defer v.lock.Unlock()
+		_, found, err := v.kv.Get(context.Background(), "7e4d0c6a708866e7")
+		require.NoError(t, err)
+		require.Equal(t, false, found)
 
-		err = v.Verify("7e4d0c6a708866e7", block)
+		res, found, err := v.kv.Get(context.Background(), "other")
 		require.NoError(t, err)
-		require.Equal(t, true, apiCalled)
+		require.Equal(t, true, found)
+		require.Equal(t, expectedKey, res)
 	})
 }