-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Lock when generating encryption key #107
Changes from 6 commits
83ab72f
ea96aac
96fb677
e1a3543
ece380f
a061368
f7c5535
cb54251
f512c0b
c9c3c73
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -155,6 +155,9 @@ func (smb *ServiceManagerBuilder) Build() *ServiceManager { | |
} | ||
|
||
func initializeSecureStorage(secureStorage storage.Security) error { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. on line
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? |
||
if err := secureStorage.Lock(); err != nil { | ||
return err | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't want to return an error if database is locked. I think it should be:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It appears that advisory locks block until a lock can be acquired: |
||
} | ||
keyFetcher := secureStorage.Fetcher() | ||
encryptionKey, err := keyFetcher.GetEncryptionKey() | ||
if err != nil { | ||
|
@@ -172,7 +175,7 @@ func initializeSecureStorage(secureStorage storage.Security) error { | |
} | ||
logrus.Debug("Successfully generated new encryption key") | ||
} | ||
return nil | ||
return secureStorage.Unlock() | ||
} | ||
|
||
func handleInterrupts(ctx context.Context, cancelFunc context.CancelFunc) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,9 +25,40 @@ import ( | |
"github.com/sirupsen/logrus" | ||
) | ||
|
||
const securityLockIndex = 111 | ||
|
||
type securityStorage struct { | ||
db *sqlx.DB | ||
encryptionKey []byte | ||
isLocked bool | ||
} | ||
|
||
// 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would it make sense to not rely on a second inmemory lock and to directly rely on the response from the db query? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add a mutex here to safeguard from future possible raceconditions |
||
return fmt.Errorf("Lock is already acquired") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) |
||
} | ||
_, err := s.db.Exec("SELECT pg_advisory_lock($1)", securityLockIndex) | ||
if err != nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. .rre this |
||
return err | ||
} | ||
s.isLocked = true | ||
return nil | ||
} | ||
|
||
// Unlock releases the database lock. | ||
func (s *securityStorage) Unlock() error { | ||
if !s.isLocked { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. again i would prefer to drop the inmemory lock There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add a mutex here to safeguard from future possible raceconditions |
||
return nil | ||
} | ||
|
||
_, err := s.db.Exec("SELECT pg_advisory_unlock($1)", securityLockIndex) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. .rre |
||
if err != nil { | ||
return err | ||
} | ||
s.isLocked = false | ||
return nil | ||
} | ||
|
||
// Fetcher returns a KeyFetcher configured to fetch a key from the database | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,14 +20,20 @@ import ( | |
"crypto/rand" | ||
"database/sql" | ||
"fmt" | ||
"time" | ||
"testing" | ||
"time" | ||
|
||
"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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. probably it would make sense to add some tests in sm_test.go (talking to a real db and validating basic scenarios)
|
||
RegisterFailHandler(Fail) | ||
RunSpecs(t, "Postgres Storage Suite") | ||
} | ||
|
||
|
||
var _ = Describe("Security", func() { | ||
|
||
|
@@ -159,4 +165,72 @@ var _ = Describe("Security", func() { | |
}) | ||
}) | ||
}) | ||
|
||
Describe("Locking", func() { | ||
var mockdb *sql.DB | ||
var mock sqlmock.Sqlmock | ||
var storage *securityStorage | ||
envEncryptionKey := make([]byte, 32) | ||
|
||
JustBeforeEach(func() { | ||
storage = &securityStorage{ | ||
db: sqlx.NewDb(mockdb, "sqlmock"), | ||
encryptionKey: envEncryptionKey, | ||
isLocked: false, | ||
} | ||
}) | ||
BeforeEach(func() { | ||
mockdb, mock, _ = sqlmock.New() | ||
rand.Read(envEncryptionKey) | ||
}) | ||
AfterEach(func() { | ||
mockdb.Close() | ||
}) | ||
|
||
Describe("Lock", func() { | ||
|
||
Context("When lock is already acquired", func() { | ||
It("Should return an error", func() { | ||
storage.isLocked = true | ||
err := storage.Lock() | ||
Expect(err).ToNot(BeNil()) | ||
}) | ||
}) | ||
|
||
Context("When lock is not yet acquired", func() { | ||
AfterEach(func() { | ||
storage.Unlock() | ||
}) | ||
BeforeEach(func() { | ||
mock.ExpectExec("SELECT").WillReturnResult(sqlmock.NewResult(int64(1), int64(1))) | ||
}) | ||
It("Should acquire lock", func() { | ||
err := storage.Lock() | ||
Expect(err).To(BeNil()) | ||
Expect(storage.isLocked).To(Equal(true)) | ||
}) | ||
}) | ||
}) | ||
|
||
Describe("Unlock", func() { | ||
Context("When lock is not acquired", func() { | ||
It("Should return nil", func() { | ||
storage.isLocked = false | ||
err := storage.Unlock() | ||
Expect(err).To(BeNil()) | ||
}) | ||
}) | ||
Context("When lock is acquired", func() { | ||
BeforeEach(func() { | ||
mock.ExpectExec("SELECT").WillReturnResult(sqlmock.NewResult(int64(1), int64(1))) | ||
}) | ||
It("Should release lock", func() { | ||
storage.isLocked = true | ||
err := storage.Unlock() | ||
Expect(err).To(BeNil()) | ||
Expect(storage.isLocked).To(Equal(false)) | ||
}) | ||
}) | ||
}) | ||
}) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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