Encryption: Extract encryption into service (grafana#38442)

* Add encryption service

* Add tests for encryption service

* Inject encryption service into http server

* Replace encryption global function usage in login tests

* Apply suggestions from code review

Co-authored-by: Emil Tullstedt <[email protected]>

* Migrate to Wire

* Undo non-desired changes

* Move Encryption bindings to OSS Wire set

Co-authored-by: Joan López de la Franca Beltran <[email protected]>
Co-authored-by: Joan López de la Franca Beltran <[email protected]>
Co-authored-by: Emil Tullstedt <[email protected]>

Author: 3 people

pkg/api/http_server.go

@@ -13,15 +13,6 @@ import (
 	"strings"
 	"sync"
 
-	"github.com/grafana/grafana/pkg/login/social"
-	"github.com/grafana/grafana/pkg/services/cleanup"
-	"github.com/grafana/grafana/pkg/services/ngalert"
-	"github.com/grafana/grafana/pkg/services/notifications"
-
-	"github.com/grafana/grafana/pkg/services/libraryelements"
-	"github.com/grafana/grafana/pkg/services/librarypanels"
-	"github.com/grafana/grafana/pkg/services/oauthtoken"
-
 	"github.com/grafana/grafana/pkg/api/routing"
 	httpstatic "github.com/grafana/grafana/pkg/api/static"
 	"github.com/grafana/grafana/pkg/bus"
@@ -32,6 +23,7 @@ import (
 	"github.com/grafana/grafana/pkg/infra/remotecache"
 	"github.com/grafana/grafana/pkg/infra/tracing"
 	"github.com/grafana/grafana/pkg/infra/usagestats"
+	"github.com/grafana/grafana/pkg/login/social"
 	"github.com/grafana/grafana/pkg/middleware"
 	"github.com/grafana/grafana/pkg/models"
 	"github.com/grafana/grafana/pkg/plugins"
@@ -40,13 +32,20 @@ import (
 	"github.com/grafana/grafana/pkg/plugins/plugincontext"
 	"github.com/grafana/grafana/pkg/services/accesscontrol"
 	"github.com/grafana/grafana/pkg/services/alerting"
+	"github.com/grafana/grafana/pkg/services/cleanup"
 	"github.com/grafana/grafana/pkg/services/contexthandler"
 	"github.com/grafana/grafana/pkg/services/datasourceproxy"
 	"github.com/grafana/grafana/pkg/services/datasources"
+	"github.com/grafana/grafana/pkg/services/encryption"
 	"github.com/grafana/grafana/pkg/services/hooks"
+	"github.com/grafana/grafana/pkg/services/libraryelements"
+	"github.com/grafana/grafana/pkg/services/librarypanels"
 	"github.com/grafana/grafana/pkg/services/live"
 	"github.com/grafana/grafana/pkg/services/live/pushhttp"
 	"github.com/grafana/grafana/pkg/services/login"
+	"github.com/grafana/grafana/pkg/services/ngalert"
+	"github.com/grafana/grafana/pkg/services/notifications"
+	"github.com/grafana/grafana/pkg/services/oauthtoken"
 	"github.com/grafana/grafana/pkg/services/provisioning"
 	"github.com/grafana/grafana/pkg/services/quota"
 	"github.com/grafana/grafana/pkg/services/rendering"
@@ -56,7 +55,6 @@ import (
 	"github.com/grafana/grafana/pkg/services/sqlstore"
 	"github.com/grafana/grafana/pkg/setting"
 	"github.com/grafana/grafana/pkg/tsdb"
-
 	"github.com/grafana/grafana/pkg/util/errutil"
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/promhttp"
@@ -107,6 +105,7 @@ type HTTPServer struct {
 	SocialService          social.Service
 	OAuthTokenService      oauthtoken.OAuthTokenService
 	Listener               net.Listener
+	EncryptionService      encryption.Service
 	cleanUpService         *cleanup.CleanUpService
 	tracingService         *tracing.TracingService
 	internalMetricsSvc     *metrics.InternalMetricsService
@@ -133,7 +132,8 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi
 	libraryPanelService librarypanels.Service, libraryElementService libraryelements.Service,
 	notificationService *notifications.NotificationService, tracingService *tracing.TracingService,
 	internalMetricsSvc *metrics.InternalMetricsService, quotaService *quota.QuotaService,
-	socialService social.Service, oauthTokenService oauthtoken.OAuthTokenService) (*HTTPServer, error) {
+	socialService social.Service, oauthTokenService oauthtoken.OAuthTokenService,
+	encryptionService encryption.Service) (*HTTPServer, error) {
 	macaron.Env = cfg.Env
 	m := macaron.New()
 
@@ -180,6 +180,7 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi
 		Listener:               opts.Listener,
 		SocialService:          socialService,
 		OAuthTokenService:      oauthTokenService,
+		EncryptionService:      encryptionService,
 	}
 	if hs.Listener != nil {
 		hs.log.Debug("Using provided listener")

pkg/api/login.go

@@ -18,7 +18,6 @@ import (
 	"github.com/grafana/grafana/pkg/middleware/cookies"
 	"github.com/grafana/grafana/pkg/models"
 	"github.com/grafana/grafana/pkg/setting"
-	"github.com/grafana/grafana/pkg/util"
 	"github.com/grafana/grafana/pkg/util/errutil"
 )
 
@@ -99,7 +98,7 @@ func (hs *HTTPServer) LoginView(c *models.ReqContext) {
 	viewData.Settings["oauth"] = enabledOAuths
 	viewData.Settings["samlEnabled"] = hs.samlEnabled()
 
-	if loginError, ok := tryGetEncryptedCookie(c, loginErrorCookieName); ok {
+	if loginError, ok := hs.tryGetEncryptedCookie(c, loginErrorCookieName); ok {
 		// this cookie is only set whenever an OAuth login fails
 		// therefore the loginError should be passed to the view data
 		// and the view should return immediately before attempting
@@ -299,7 +298,7 @@ func (hs *HTTPServer) Logout(c *models.ReqContext) {
 	}
 }
 
-func tryGetEncryptedCookie(ctx *models.ReqContext, cookieName string) (string, bool) {
+func (hs *HTTPServer) tryGetEncryptedCookie(ctx *models.ReqContext, cookieName string) (string, bool) {
 	cookie := ctx.GetCookie(cookieName)
 	if cookie == "" {
 		return "", false
@@ -310,12 +309,12 @@ func tryGetEncryptedCookie(ctx *models.ReqContext, cookieName string) (string, b
 		return "", false
 	}
 
-	decryptedError, err := util.Decrypt(decoded, setting.SecretKey)
+	decryptedError, err := hs.EncryptionService.Decrypt(decoded, setting.SecretKey)
 	return string(decryptedError), err == nil
 }
 
 func (hs *HTTPServer) trySetEncryptedCookie(ctx *models.ReqContext, cookieName string, value string, maxAge int) error {
-	encryptedError, err := util.Encrypt([]byte(value), setting.SecretKey)
+	encryptedError, err := hs.EncryptionService.Encrypt([]byte(value), setting.SecretKey)
 	if err != nil {
 		return err
 	}

pkg/api/login_test.go

@@ -10,6 +10,8 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/grafana/grafana/pkg/services/encryption/ossencryption"
+
 	"github.com/grafana/grafana/pkg/api/dtos"
 	"github.com/grafana/grafana/pkg/api/response"
 	"github.com/grafana/grafana/pkg/api/routing"
@@ -23,7 +25,6 @@ import (
 	"github.com/grafana/grafana/pkg/services/hooks"
 	"github.com/grafana/grafana/pkg/services/licensing"
 	"github.com/grafana/grafana/pkg/setting"
-	"github.com/grafana/grafana/pkg/util"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -105,10 +106,11 @@ func TestLoginErrorCookieAPIEndpoint(t *testing.T) {
 	sc := setupScenarioContext(t, "/login")
 	cfg := setting.NewCfg()
 	hs := &HTTPServer{
-		Cfg:              cfg,
-		SettingsProvider: &setting.OSSImpl{Cfg: cfg},
-		License:          &licensing.OSSLicensingService{},
-		SocialService:    &mockSocialService{},
+		Cfg:               cfg,
+		SettingsProvider:  &setting.OSSImpl{Cfg: cfg},
+		License:           &licensing.OSSLicensingService{},
+		SocialService:     &mockSocialService{},
+		EncryptionService: &ossencryption.Service{},
 	}
 
 	sc.defaultHandler = routing.Wrap(func(w http.ResponseWriter, c *models.ReqContext) {
@@ -121,7 +123,7 @@ func TestLoginErrorCookieAPIEndpoint(t *testing.T) {
 	setting.OAuthAutoLogin = true
 
 	oauthError := errors.New("User not a member of one of the required organizations")
-	encryptedError, err := util.Encrypt([]byte(oauthError.Error()), setting.SecretKey)
+	encryptedError, err := hs.EncryptionService.Encrypt([]byte(oauthError.Error()), setting.SecretKey)
 	require.NoError(t, err)
 	expCookiePath := "/"
 	if len(setting.AppSubUrl) > 0 {

pkg/server/wireexts_oss.go

@@ -11,6 +11,8 @@ import (
 	"github.com/grafana/grafana/pkg/services/accesscontrol/ossaccesscontrol"
 	"github.com/grafana/grafana/pkg/services/auth"
 	"github.com/grafana/grafana/pkg/services/datasources"
+	"github.com/grafana/grafana/pkg/services/encryption"
+	"github.com/grafana/grafana/pkg/services/encryption/ossencryption"
 	"github.com/grafana/grafana/pkg/services/licensing"
 	"github.com/grafana/grafana/pkg/services/login"
 	"github.com/grafana/grafana/pkg/services/login/authinfoservice"
@@ -43,6 +45,8 @@ var wireExtsBasicSet = wire.NewSet(
 	wire.Bind(new(registry.DatabaseMigrator), new(*migrations.OSSMigrations)),
 	authinfoservice.ProvideOSSUserProtectionService,
 	wire.Bind(new(login.UserProtectionService), new(*authinfoservice.OSSUserProtectionImpl)),
+	ossencryption.ProvideService,
+	wire.Bind(new(encryption.Service), new(*ossencryption.Service)),
 )
 
 var wireExtsSet = wire.NewSet(

pkg/services/encryption/encryption.go

@@ -0,0 +1,6 @@
+package encryption
+
+type Service interface {
+	Encrypt([]byte, string) ([]byte, error)
+	Decrypt([]byte, string) ([]byte, error)
+}

pkg/services/encryption/ossencryption/ossencryption.go

@@ -0,0 +1,88 @@
+package ossencryption
+
+import (
+	"crypto/aes"
+	"crypto/cipher"
+	"crypto/rand"
+	"crypto/sha256"
+	"errors"
+	"fmt"
+	"io"
+
+	"github.com/grafana/grafana/pkg/util"
+	"golang.org/x/crypto/pbkdf2"
+)
+
+type Service struct{}
+
+func ProvideService() *Service {
+	return &Service{}
+}
+
+const saltLength = 8
+
+func (s *Service) Decrypt(payload []byte, secret string) ([]byte, error) {
+	if len(payload) < saltLength {
+		return nil, fmt.Errorf("unable to compute salt")
+	}
+	salt := payload[:saltLength]
+	key, err := encryptionKeyToBytes(secret, string(salt))
+	if err != nil {
+		return nil, err
+	}
+
+	block, err := aes.NewCipher(key)
+	if err != nil {
+		return nil, err
+	}
+
+	// The IV needs to be unique, but not secure. Therefore it's common to
+	// include it at the beginning of the ciphertext.
+	if len(payload) < aes.BlockSize {
+		return nil, errors.New("payload too short")
+	}
+	iv := payload[saltLength : saltLength+aes.BlockSize]
+	payload = payload[saltLength+aes.BlockSize:]
+	payloadDst := make([]byte, len(payload))
+
+	stream := cipher.NewCFBDecrypter(block, iv)
+
+	// XORKeyStream can work in-place if the two arguments are the same.
+	stream.XORKeyStream(payloadDst, payload)
+	return payloadDst, nil
+}
+
+func (s *Service) Encrypt(payload []byte, secret string) ([]byte, error) {
+	salt, err := util.GetRandomString(saltLength)
+	if err != nil {
+		return nil, err
+	}
+
+	key, err := encryptionKeyToBytes(secret, salt)
+	if err != nil {
+		return nil, err
+	}
+	block, err := aes.NewCipher(key)
+	if err != nil {
+		return nil, err
+	}
+
+	// The IV needs to be unique, but not secure. Therefore it's common to
+	// include it at the beginning of the ciphertext.
+	ciphertext := make([]byte, saltLength+aes.BlockSize+len(payload))
+	copy(ciphertext[:saltLength], salt)
+	iv := ciphertext[saltLength : saltLength+aes.BlockSize]
+	if _, err := io.ReadFull(rand.Reader, iv); err != nil {
+		return nil, err
+	}
+
+	stream := cipher.NewCFBEncrypter(block, iv)
+	stream.XORKeyStream(ciphertext[saltLength+aes.BlockSize:], payload)
+
+	return ciphertext, nil
+}
+
+// Key needs to be 32bytes
+func encryptionKeyToBytes(secret, salt string) ([]byte, error) {
+	return pbkdf2.Key([]byte(secret), []byte(salt), 10000, 32, sha256.New), nil
+}

pkg/services/encryption/ossencryption/ossencryption_test.go

@@ -0,0 +1,39 @@
+package ossencryption
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestEncryption(t *testing.T) {
+	svc := Service{}
+
+	t.Run("getting encryption key", func(t *testing.T) {
+		key, err := encryptionKeyToBytes("secret", "salt")
+		require.NoError(t, err)
+		assert.Len(t, key, 32)
+
+		key, err = encryptionKeyToBytes("a very long secret key that is larger then 32bytes", "salt")
+		require.NoError(t, err)
+		assert.Len(t, key, 32)
+	})
+
+	t.Run("decrypting basic payload", func(t *testing.T) {
+		encrypted, err := svc.Encrypt([]byte("grafana"), "1234")
+		require.NoError(t, err)
+
+		decrypted, err := svc.Decrypt(encrypted, "1234")
+		require.NoError(t, err)
+
+		assert.Equal(t, []byte("grafana"), decrypted)
+	})
+
+	t.Run("decrypting empty payload should return error", func(t *testing.T) {
+		_, err := svc.Decrypt([]byte(""), "1234")
+		require.Error(t, err)
+
+		assert.Equal(t, "unable to compute salt", err.Error())
+	})
+}