From 30d75e224fb91028eeae99b77473e24b0e84acdd Mon Sep 17 00:00:00 2001 From: Nick Cabatoff Date: Thu, 26 Sep 2019 09:28:58 -0400 Subject: [PATCH 1/3] Fix issue with rotateCredentials deadlocking with itself due to use of defer within loop body, by factoring out loop body into its own func. --- builtin/logical/database/rotation.go | 171 ++++++++++++++------------- 1 file changed, 87 insertions(+), 84 deletions(-) diff --git a/builtin/logical/database/rotation.go b/builtin/logical/database/rotation.go index 46759cbec681..9ac4374e6b48 100644 --- a/builtin/logical/database/rotation.go +++ b/builtin/logical/database/rotation.go @@ -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 From 4ce9d8f4a88e7001d085e1d0d79d3ebafca247e2 Mon Sep 17 00:00:00 2001 From: catsby Date: Fri, 27 Sep 2019 16:29:44 -0500 Subject: [PATCH 2/3] add regression test for failing to release locks --- builtin/logical/database/rotation_test.go | 91 ++++++++++++++++++++--- 1 file changed, 82 insertions(+), 9 deletions(-) diff --git a/builtin/logical/database/rotation_test.go b/builtin/logical/database/rotation_test.go index 644d92a760c6..3b7d86a63e12 100644 --- a/builtin/logical/database/rotation_test.go +++ b/builtin/logical/database/rotation_test.go @@ -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) @@ -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{}{ @@ -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{}{ @@ -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{ @@ -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{}{ @@ -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{}{ @@ -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 @@ -953,6 +956,76 @@ func TestBackend_StaticRole_Rotations_MongoDB(t *testing.T) { } } +// Demonstrates a bug fix for collision of 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 { From 83c512d3da01e033d585092e1ad8dca8dbe7a17e Mon Sep 17 00:00:00 2001 From: catsby Date: Fri, 27 Sep 2019 16:30:59 -0500 Subject: [PATCH 3/3] change code comment wording --- builtin/logical/database/rotation_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/logical/database/rotation_test.go b/builtin/logical/database/rotation_test.go index 3b7d86a63e12..6d6a8d0ce27a 100644 --- a/builtin/logical/database/rotation_test.go +++ b/builtin/logical/database/rotation_test.go @@ -956,7 +956,7 @@ func TestBackend_StaticRole_Rotations_MongoDB(t *testing.T) { } } -// Demonstrates a bug fix for collision of locks +// 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()