From a0108a1e5b5639699728629d72f75e4fcbf03b9a Mon Sep 17 00:00:00 2001 From: Tania B Date: Mon, 30 Aug 2021 20:39:55 +0300 Subject: [PATCH] Encryption: Extract encryption into service (#38442) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 * Migrate to Wire * Undo non-desired changes * Move Encryption bindings to OSS Wire set Co-authored-by: Joan López de la Franca Beltran Co-authored-by: Joan López de la Franca Beltran <5459617+joanlopez@users.noreply.github.com> Co-authored-by: Emil Tullstedt --- pkg/api/http_server.go | 23 ++--- pkg/api/login.go | 9 +- pkg/api/login_test.go | 14 +-- pkg/server/wireexts_oss.go | 4 + pkg/services/encryption/encryption.go | 6 ++ .../encryption/ossencryption/ossencryption.go | 88 +++++++++++++++++++ .../ossencryption/ossencryption_test.go | 39 ++++++++ 7 files changed, 161 insertions(+), 22 deletions(-) create mode 100644 pkg/services/encryption/encryption.go create mode 100644 pkg/services/encryption/ossencryption/ossencryption.go create mode 100644 pkg/services/encryption/ossencryption/ossencryption_test.go diff --git a/pkg/api/http_server.go b/pkg/api/http_server.go index 23b291c8ec3..57826632acc 100644 --- a/pkg/api/http_server.go +++ b/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") diff --git a/pkg/api/login.go b/pkg/api/login.go index 4fb6bcf5042..35440745e43 100644 --- a/pkg/api/login.go +++ b/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 } diff --git a/pkg/api/login_test.go b/pkg/api/login_test.go index bdeceada8a9..dc06e65c5ba 100644 --- a/pkg/api/login_test.go +++ b/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 { diff --git a/pkg/server/wireexts_oss.go b/pkg/server/wireexts_oss.go index f340c772105..e348b3a6faa 100644 --- a/pkg/server/wireexts_oss.go +++ b/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( diff --git a/pkg/services/encryption/encryption.go b/pkg/services/encryption/encryption.go new file mode 100644 index 00000000000..8e60c781374 --- /dev/null +++ b/pkg/services/encryption/encryption.go @@ -0,0 +1,6 @@ +package encryption + +type Service interface { + Encrypt([]byte, string) ([]byte, error) + Decrypt([]byte, string) ([]byte, error) +} diff --git a/pkg/services/encryption/ossencryption/ossencryption.go b/pkg/services/encryption/ossencryption/ossencryption.go new file mode 100644 index 00000000000..c10c68afe62 --- /dev/null +++ b/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 +} diff --git a/pkg/services/encryption/ossencryption/ossencryption_test.go b/pkg/services/encryption/ossencryption/ossencryption_test.go new file mode 100644 index 00000000000..de98d2f09f9 --- /dev/null +++ b/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()) + }) +}