Fix issue with rotateCredentials deadlocking with itself (#7518)

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 {
+func (b *databaseBackend) rotateCredentials(ctx context.Context, s logical.Storage) {
-	for {
+	for b.rotateCredential(ctx, s) {
-		// Quit rotating credentials if shutdown has started
+	}
-		select {
+}
-		case <-ctx.Done():
+
-			return nil
+func (b *databaseBackend) rotateCredential(ctx context.Context, s logical.Storage) bool {
-		default:
+	// Quit rotating credentials if shutdown has started
-		}
+	select {
-		item, err := b.popFromRotationQueue()
+	case <-ctx.Done():
-		if err != nil {
+		return false
-			if err == queue.ErrEmpty {
+	default:
-				return nil
+	}
-			}
+	item, err := b.popFromRotationQueue()
-			return err
+	if err != nil {
+		if err != queue.ErrEmpty {
+			b.logger.Error("error popping item from queue", "err", err)
 		}
+		return false
+	}
 
-		// Guard against possible nil item
+	// Guard against possible nil item
-		if item == nil {
+	if item == nil {
-			return nil
+		return false
-		}
+	}
 
-		// Grab the exclusive lock for this Role, to make sure we don't incur and
+	// Grab the exclusive lock for this Role, to make sure we don't incur and
-		// writes during the rotation process
+	// writes during the rotation process
-		lock := locksutil.LockForKey(b.roleLocks, item.Key)
+	lock := locksutil.LockForKey(b.roleLocks, item.Key)
-		lock.Lock()
+	lock.Lock()
-		defer lock.Unlock()
+	defer lock.Unlock()
 
-		// Validate the role still exists
+	// Validate the role still exists
-		role, err := b.StaticRole(ctx, s, item.Key)
+	role, err := b.StaticRole(ctx, s, item.Key)
-		if err != nil {
+	if err != nil {
-			b.logger.Error("unable to load role", "role", item.Key, "error", err)
+		b.logger.Error("unable to load role", "role", item.Key, "error", err)
-			item.Priority = time.Now().Add(10 * time.Second).Unix()
+		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
-		}
-
-		// 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
-		}
-
-		input := &setStaticAccountInput{
-			RoleName: item.Key,
-			Role:     role,
-		}
-
-		// 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
-			}
-		}
-
-		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
-			}
-
-			if err := b.pushItem(item); err != nil {
-				b.logger.Error("unable to push item on to queue", "error", err)
-			}
-			// Go to next item
-			continue
-		}
-
-		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)
+			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
+	}
+
+	// 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
+	}
+
+	input := &setStaticAccountInput{
+		RoleName: item.Key,
+		Role:     role,
+	}
+
+	// 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
 		}
 	}
-	return nil
+
+	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
+		}
+
+		if err := b.pushItem(item); err != nil {
+			b.logger.Error("unable to push item on to queue", "error", err)
+		}
+		// Go to next item
+		return true
+	}
+
+	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

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 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 {