Lock when generating encryption key #107

dpanayotov · 2018-08-07T10:26:56Z

Fixes #106

Opened #116 for test

coveralls · 2018-08-07T10:34:41Z

Pull Request Test Coverage Report for Build 865

29 of 33 (87.88%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.07%) to 90.71%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/sm/sm.go	6	8	75.0%
storage/postgres/security.go	20	22	90.91%

Totals
Change from base Build 856:	-0.07%
Covered Lines:	1826
Relevant Lines:	2013

💛 - Coveralls

dzahariev · 2018-08-07T11:18:17Z

pkg/sm/encryption.go

+			return err
+		}
+		if len(encryptionKey) != 0 {
+			return nil


I think here you should return the key instead null

pankrator · 2018-08-13T07:42:41Z

pkg/sm/sm.go

@@ -155,6 +155,9 @@ func (smb *ServiceManagerBuilder) Build() *ServiceManager {
 }

 func initializeSecureStorage(secureStorage storage.Security) error {
+	if err := secureStorage.Lock(); err != nil {
+		return err


We don't want to return an error if database is locked. I think it should be:

Try to lock database.

If it is already locked, then wait some time (2 seconds for example)

Try to lock again

Repeat several times(5 for example)

If it couldn't acquire the lock, then panic

If lock is acquired then check for the encryption key. If it is already there then do nothing, otherwise generate one

It appears that advisory locks block until a lock can be acquired:
https://medium.com/@codeflows/application-level-locking-with-postgres-advisory-locks-c29759eeb7a7

KirilKabakchiev · 2018-08-11T20:24:38Z

storage/postgres/security.go

+// Lock acquires a database lock so that only one process can manipulate the encryption key.
+// Returns an error if the process has already acquired the lock
+func (s *securityStorage) Lock() error {
+	if s.isLocked {


would it make sense to not rely on a second inmemory lock and to directly rely on the response from the db query?

KirilKabakchiev · 2018-08-11T20:26:56Z

storage/postgres/security.go

+// Returns an error if the process has already acquired the lock
+func (s *securityStorage) Lock() error {
+	if s.isLocked {
+		return fmt.Errorf("Lock is already acquired")


why error? The fact that the lock is locked, means its locked and if we decided that we do blocking select we should stick with that in all situations (even when in the same node)

KirilKabakchiev · 2018-08-11T20:27:43Z

storage/postgres/security.go

+		return fmt.Errorf("Lock is already acquired")
+	}
+	_, err := s.db.Exec("SELECT pg_advisory_lock($1)", securityLockIndex)
+	if err != nil {


KirilKabakchiev · 2018-08-11T20:28:17Z

storage/postgres/security.go

+		return nil
+	}
+
+	_, err := s.db.Exec("SELECT pg_advisory_unlock($1)", securityLockIndex)


KirilKabakchiev · 2018-08-11T20:29:27Z

storage/postgres/security.go

+
+// Unlock releases the database lock.
+func (s *securityStorage) Unlock() error {
+	if !s.isLocked {


again i would prefer to drop the inmemory lock

KirilKabakchiev · 2018-08-11T20:40:12Z

storage/postgres/security_test.go


 	"github.com/DATA-DOG/go-sqlmock"
 	"github.com/Peripli/service-manager/security"
 	"github.com/jmoiron/sqlx"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 )
+func TestAPostgresStorage(t *testing.T) {


probably it would make sense to add some tests in sm_test.go (talking to a real db and validating basic scenarios)

failure occurs if the first key is missing from application.yml/env/flags

things happen on startup if first key is present
2.1. second key is generated
2.2 locking works (lock twice, verify first is ok, second returns correct db response or waits and eventually when 1st unlocks, 2cd proceeds)

KirilKabakchiev · 2018-08-11T20:49:22Z

pkg/sm/sm.go

@@ -155,6 +155,9 @@ func (smb *ServiceManagerBuilder) Build() *ServiceManager {
 }

 func initializeSecureStorage(secureStorage storage.Security) error {


just thinking out loud - should we explicitly fail if this method actually attempts to generate a new key (len(encryptionKey) == 0) and we already have data in the database? Is there a sane way to validate this? Because we would be in a very messed up situation if SM starts in such state

KirilKabakchiev · 2018-08-11T20:51:40Z

pkg/sm/sm.go

@@ -155,6 +155,9 @@ func (smb *ServiceManagerBuilder) Build() *ServiceManager {
 }

 func initializeSecureStorage(secureStorage storage.Security) error {


on line

return fmt.Errorf("Could not generate encryption key: %v", err)

wrap error with %v - its fine but in some places in SM we use %s and in others we do it with %v - should we try to be consistent?

KirilKabakchiev · 2018-08-13T13:40:18Z

test/main_test.go

+			if os.Getenv("CLEAN_ENV") == "" {
+				Skip("Environment is not clean to validate behavior of parallel encryption key generation")
+			} else {
+				command := exec.Command(binaryPath)


pass port as flag here so both can use different ports

KirilKabakchiev · 2018-08-13T13:56:51Z

storage/postgres/security.go

+// Lock acquires a database lock so that only one process can manipulate the encryption key.
+// Returns an error if the process has already acquired the lock
+func (s *securityStorage) Lock() error {
+	if s.isLocked {


add a mutex here to safeguard from future possible raceconditions

KirilKabakchiev · 2018-08-13T13:57:05Z

storage/postgres/security.go

+
+// Unlock releases the database lock.
+func (s *securityStorage) Unlock() error {
+	if !s.isLocked {


add a mutex here to safeguard from future possible raceconditions

KirilKabakchiev · 2018-08-16T07:23:43Z

storage/postgres/security.go

+// Lock acquires a database lock so that only one process can manipulate the encryption key.
+// Returns an error if the process has already acquired the lock
+func (s *securityStorage) Lock(ctx context.Context) error {
+	s.mutex.Lock()


is this the correct lock ( e.x. this or RWLock )

RWMutex provides only R(W)Lock and R(W)Unlock for these explicit situations. RWMutext.Lock/Unlock is the same as Mutex.Lock/Unlock
Since this is both reading and writing the value, there's no difference between RWMutex.Lock and Mutex.Lock

Only leader should set encryption key

83ab72f

dpanayotov requested review from dzahariev, georgifarashev, pankrator and dotchev August 7, 2018 10:26

add leader field in test env

ea96aac

dzahariev requested changes Aug 7, 2018

View reviewed changes

add tests for leader encryption

96fb677

dzahariev previously approved these changes Aug 7, 2018

View reviewed changes

dpanayotov added the 👋request review label Aug 7, 2018

Remove leader and add pg_advisory_lock

e1a3543

dpanayotov dismissed dzahariev’s stale review via e1a3543 August 8, 2018 09:54

dpanayotov removed the 👋request review label Aug 8, 2018

dpanayotov added 2 commits August 8, 2018 12:58

cleanup

ece380f

add tests

a061368

dpanayotov changed the title ~~Only leader should set encryption key~~ Lock when generating encryption key Aug 8, 2018

dpanayotov added bug Something isn't working review in progress 👋request review and removed review in progress labels Aug 8, 2018

pankrator reviewed Aug 13, 2018

View reviewed changes

KirilKabakchiev requested changes Aug 13, 2018

View reviewed changes

KirilKabakchiev reviewed Aug 13, 2018

View reviewed changes

dpanayotov force-pushed the dblock branch from b243d11 to a061368 Compare August 14, 2018 10:14

dpanayotov added 3 commits August 14, 2018 13:40

add context to lock for timeout

f7c5535

add global context as parent to locking context

cb54251

cleanup

f512c0b

inline error check

c9c3c73

KirilKabakchiev reviewed Aug 16, 2018

View reviewed changes

KirilKabakchiev approved these changes Aug 16, 2018

View reviewed changes

dpanayotov merged commit f586159 into master Aug 16, 2018

dpanayotov deleted the dblock branch August 25, 2018 13:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lock when generating encryption key #107

Lock when generating encryption key #107

dpanayotov commented Aug 7, 2018 •

edited

Loading

coveralls commented Aug 7, 2018 •

edited

Loading

dzahariev Aug 7, 2018

pankrator Aug 13, 2018

pankrator Aug 13, 2018

KirilKabakchiev Aug 11, 2018

KirilKabakchiev Aug 11, 2018

KirilKabakchiev Aug 11, 2018

KirilKabakchiev Aug 11, 2018

KirilKabakchiev Aug 11, 2018

KirilKabakchiev Aug 11, 2018

KirilKabakchiev Aug 11, 2018

KirilKabakchiev Aug 11, 2018

KirilKabakchiev Aug 13, 2018

KirilKabakchiev Aug 13, 2018

KirilKabakchiev Aug 13, 2018

KirilKabakchiev Aug 16, 2018

dpanayotov Aug 16, 2018

		@@ -155,6 +155,9 @@ func (smb ServiceManagerBuilder) Build() ServiceManager {
		}

		func initializeSecureStorage(secureStorage storage.Security) error {

Lock when generating encryption key #107

Lock when generating encryption key #107

Conversation

dpanayotov commented Aug 7, 2018 • edited Loading

coveralls commented Aug 7, 2018 • edited Loading

Pull Request Test Coverage Report for Build 865

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpanayotov commented Aug 7, 2018 •

edited

Loading

coveralls commented Aug 7, 2018 •

edited

Loading