Skip to content
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.

Sign up for GitHub

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

Fix issue with rotateCredentials deadlocking with itself #7518

Merged
merged 4 commits into from
Oct 3, 2019
Merged

Fix issue with rotateCredentials deadlocking with itself #7518

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
30d75e2
Fix issue with rotateCredentials deadlocking with itself due to use of
ncabatoff Sep 26, 2019
4ce9d8f
add regression test for failing to release locks
catsby Sep 27, 2019
83c512d
change code comment wording
catsby Sep 27, 2019
6328158
Merge branch 'master' into fix-db-rotation-locking
ncabatoff Oct 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
171 changes: 87 additions & 84 deletions builtin/logical/database/rotation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,112 +127,115 @@ type setCredentialsWAL struct {
// This method loops through the priority queue, popping the highest priority
// item until it encounters the first item that does not yet need rotation,
// based on the current time.
func (b *databaseBackend) rotateCredentials(ctx context.Context, s logical.Storage) error {
for {
// Quit rotating credentials if shutdown has started
select {
case <-ctx.Done():
return nil
default:
}
item, err := b.popFromRotationQueue()
if err != nil {
if err == queue.ErrEmpty {
return nil
}
return err
}
func (b *databaseBackend) rotateCredentials(ctx context.Context, s logical.Storage) {
for b.rotateCredential(ctx, s) {
}
}

// Guard against possible nil item
if item == nil {
return nil
func (b *databaseBackend) rotateCredential(ctx context.Context, s logical.Storage) bool {
// Quit rotating credentials if shutdown has started
select {
case <-ctx.Done():
return false
default:
}
item, err := b.popFromRotationQueue()
if err != nil {
if err != queue.ErrEmpty {
b.logger.Error("error popping item from queue", "err", err)
}
return false
}

// Grab the exclusive lock for this Role, to make sure we don't incur and
// writes during the rotation process
lock := locksutil.LockForKey(b.roleLocks, item.Key)
lock.Lock()
defer lock.Unlock()
// Guard against possible nil item
if item == nil {
return false
}

// Validate the role still exists
role, err := b.StaticRole(ctx, s, item.Key)
if err != nil {
b.logger.Error("unable to load role", "role", item.Key, "error", err)
item.Priority = time.Now().Add(10 * time.Second).Unix()
if err := b.pushItem(item); err != nil {
b.logger.Error("unable to push item on to queue", "error", err)
}
continue
}
if role == nil {
b.logger.Warn("role not found", "role", item.Key, "error", err)
continue
}
// Grab the exclusive lock for this Role, to make sure we don't incur and
// writes during the rotation process
lock := locksutil.LockForKey(b.roleLocks, item.Key)
lock.Lock()
defer lock.Unlock()

// If "now" is less than the Item priority, then this item does not need to
// be rotated
if time.Now().Unix() < item.Priority {
if err := b.pushItem(item); err != nil {
b.logger.Error("unable to push item on to queue", "error", err)
}
// Break out of the for loop
break
// Validate the role still exists
role, err := b.StaticRole(ctx, s, item.Key)
if err != nil {
b.logger.Error("unable to load role", "role", item.Key, "error", err)
item.Priority = time.Now().Add(10 * time.Second).Unix()
if err := b.pushItem(item); err != nil {
b.logger.Error("unable to push item on to queue", "error", err)
}
return true
}
if role == nil {
b.logger.Warn("role not found", "role", item.Key, "error", err)
return true
}

input := &setStaticAccountInput{
RoleName: item.Key,
Role: role,
// If "now" is less than the Item priority, then this item does not need to
// be rotated
if time.Now().Unix() < item.Priority {
if err := b.pushItem(item); err != nil {
b.logger.Error("unable to push item on to queue", "error", err)
}
// Break out of the for loop
return false
}

// If there is a WAL entry related to this Role, the corresponding WAL ID
// should be stored in the Item's Value field.
if walID, ok := item.Value.(string); ok {
walEntry, err := b.findStaticWAL(ctx, s, walID)
if err != nil {
b.logger.Error("error finding static WAL", "error", err)
item.Priority = time.Now().Add(10 * time.Second).Unix()
if err := b.pushItem(item); err != nil {
b.logger.Error("unable to push item on to queue", "error", err)
}
}
if walEntry != nil && walEntry.NewPassword != "" {
input.Password = walEntry.NewPassword
input.WALID = walID
}
}
input := &setStaticAccountInput{
RoleName: item.Key,
Role: role,
}

resp, err := b.setStaticAccount(ctx, s, input)
// If there is a WAL entry related to this Role, the corresponding WAL ID
// should be stored in the Item's Value field.
if walID, ok := item.Value.(string); ok {
walEntry, err := b.findStaticWAL(ctx, s, walID)
if err != nil {
b.logger.Error("unable to rotate credentials in periodic function", "error", err)
// Increment the priority enough so that the next call to this method
// likely will not attempt to rotate it, as a back-off of sorts
b.logger.Error("error finding static WAL", "error", err)
item.Priority = time.Now().Add(10 * time.Second).Unix()

// Preserve the WALID if it was returned
if resp != nil && resp.WALID != "" {
item.Value = resp.WALID
}

if err := b.pushItem(item); err != nil {
b.logger.Error("unable to push item on to queue", "error", err)
}
// Go to next item
continue
}
if walEntry != nil && walEntry.NewPassword != "" {
input.Password = walEntry.NewPassword
input.WALID = walID
}
}

lvr := resp.RotationTime
if lvr.IsZero() {
lvr = time.Now()
resp, err := b.setStaticAccount(ctx, s, input)
if err != nil {
b.logger.Error("unable to rotate credentials in periodic function", "error", err)
// Increment the priority enough so that the next call to this method
// likely will not attempt to rotate it, as a back-off of sorts
item.Priority = time.Now().Add(10 * time.Second).Unix()

// Preserve the WALID if it was returned
if resp != nil && resp.WALID != "" {
item.Value = resp.WALID
}

// Update priority and push updated Item to the queue
nextRotation := lvr.Add(role.StaticAccount.RotationPeriod)
item.Priority = nextRotation.Unix()
if err := b.pushItem(item); err != nil {
b.logger.Warn("unable to push item on to queue", "error", err)
b.logger.Error("unable to push item on to queue", "error", err)
}
// Go to next item
return true
}
return nil

lvr := resp.RotationTime
if lvr.IsZero() {
lvr = time.Now()
}

// Update priority and push updated Item to the queue
nextRotation := lvr.Add(role.StaticAccount.RotationPeriod)
item.Priority = nextRotation.Unix()
if err := b.pushItem(item); err != nil {
b.logger.Warn("unable to push item on to queue", "error", err)
}
return true
}

// findStaticWAL loads a WAL entry by ID. If found, only return the WAL if it
Expand Down
91 changes: 82 additions & 9 deletions builtin/logical/database/rotation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,12 @@ import (
"github.com/lib/pq"
)

const dbUser = "vaultstatictest"
const (
dbUser = "vaultstatictest"
dbUserDefaultPassword = "password"

const testMongoDBRole = `{ "db": "admin", "roles": [ { "role": "readWrite" } ] }`
testMongoDBRole = `{ "db": "admin", "roles": [ { "role": "readWrite" } ] }`
)

func TestBackend_StaticRole_Rotate_basic(t *testing.T) {
cluster, sys := getCluster(t)
Expand All @@ -44,9 +47,9 @@ func TestBackend_StaticRole_Rotate_basic(t *testing.T) {
defer cleanup()

// create the database user
createTestPGUser(t, connURL, dbUser, "password", testRoleStaticCreate)
createTestPGUser(t, connURL, dbUser, dbUserDefaultPassword, testRoleStaticCreate)

verifyPgConn(t, dbUser, "password", connURL)
verifyPgConn(t, dbUser, dbUserDefaultPassword, connURL)

// Configure a connection
data := map[string]interface{}{
Expand Down Expand Up @@ -192,7 +195,7 @@ func TestBackend_StaticRole_Rotate_NonStaticError(t *testing.T) {
defer cleanup()

// create the database user
createTestPGUser(t, connURL, dbUser, "password", testRoleStaticCreate)
createTestPGUser(t, connURL, dbUser, dbUserDefaultPassword, testRoleStaticCreate)

// Configure a connection
data := map[string]interface{}{
Expand Down Expand Up @@ -254,7 +257,7 @@ func TestBackend_StaticRole_Rotate_NonStaticError(t *testing.T) {
}

// Verify username/password
verifyPgConn(t, dbUser, "password", connURL)
verifyPgConn(t, dbUser, dbUserDefaultPassword, connURL)
// Trigger rotation
data = map[string]interface{}{"name": "plugin-role-test"}
req = &logical.Request{
Expand Down Expand Up @@ -296,7 +299,7 @@ func TestBackend_StaticRole_Revoke_user(t *testing.T) {
defer cleanup()

// create the database user
createTestPGUser(t, connURL, dbUser, "password", testRoleStaticCreate)
createTestPGUser(t, connURL, dbUser, dbUserDefaultPassword, testRoleStaticCreate)

// Configure a connection
data := map[string]interface{}{
Expand Down Expand Up @@ -521,7 +524,7 @@ func TestBackend_Static_QueueWAL_discard_role_newer_rotation_date(t *testing.T)
defer cleanup()

// create the database user
createTestPGUser(t, connURL, dbUser, "password", testRoleStaticCreate)
createTestPGUser(t, connURL, dbUser, dbUserDefaultPassword, testRoleStaticCreate)

// Configure a connection
data := map[string]interface{}{
Expand Down Expand Up @@ -706,7 +709,7 @@ func TestBackend_StaticRole_Rotations_PostgreSQL(t *testing.T) {
testCases := []string{"65", "130", "5400"}
// Create database users ahead
for _, tc := range testCases {
createTestPGUser(t, connURL, dbUser+tc, "password", testRoleStaticCreate)
createTestPGUser(t, connURL, dbUser+tc, dbUserDefaultPassword, testRoleStaticCreate)
}

// Configure a connection
Expand Down Expand Up @@ -953,6 +956,76 @@ func TestBackend_StaticRole_Rotations_MongoDB(t *testing.T) {
}
}

// Demonstrates a bug fix for the credential rotation not releasing locks
func TestBackend_StaticRole_LockRegression(t *testing.T) {
cluster, sys := getCluster(t)
defer cluster.Cleanup()

config := logical.TestBackendConfig()
config.StorageView = &logical.InmemStorage{}
config.System = sys

lb, err := Factory(context.Background(), config)
if err != nil {
t.Fatal(err)
}
b, ok := lb.(*databaseBackend)
if !ok {
t.Fatal("could not convert to db backend")
}
defer b.Cleanup(context.Background())

cleanup, connURL := preparePostgresTestContainer(t, config.StorageView, b)
defer cleanup()

// Configure a connection
data := map[string]interface{}{
"connection_url": connURL,
"plugin_name": "postgresql-database-plugin",
"verify_connection": false,
"allowed_roles": []string{"*"},
"name": "plugin-test",
}

req := &logical.Request{
Operation: logical.UpdateOperation,
Path: "config/plugin-test",
Storage: config.StorageView,
Data: data,
}
resp, err := b.HandleRequest(namespace.RootContext(nil), req)
if err != nil || (resp != nil && resp.IsError()) {
t.Fatalf("err:%s resp:%#v\n", err, resp)
}

createTestPGUser(t, connURL, dbUser, dbUserDefaultPassword, testRoleStaticCreate)
for i := 0; i < 25; i++ {
data := map[string]interface{}{
"name": "plugin-role-test",
"db_name": "plugin-test",
"rotation_statements": testRoleStaticUpdate,
"username": dbUser,
"rotation_period": "7s",
}

req = &logical.Request{
Operation: logical.UpdateOperation,
Path: "static-roles/plugin-role-test",
Storage: config.StorageView,
Data: data,
}

resp, err = b.HandleRequest(namespace.RootContext(nil), req)
if err != nil || (resp != nil && resp.IsError()) {
t.Fatalf("err:%s resp:%#v\n", err, resp)
}

// sleeping is needed to trigger the deadlock, otherwise things are
// processed too quickly to trigger the rotation lock on so few roles
time.Sleep(500 * time.Millisecond)
}
}

// capturePasswords captures the current passwords at the time of calling, and
// returns a map of username / passwords building off of the input map
func capturePasswords(t *testing.T, b logical.Backend, config *logical.BackendConfig, testCases []string, pws map[string][]string) map[string][]string {
Expand Down