Fix password rotation (zalando#2043)

* fix password rotation
* test connection with rotation user in e2e test + minor changes

Author: FxKu

e2e/tests/test_e2e.py

@@ -1442,6 +1442,10 @@ def test_password_rotation(self):
         self.eventuallyEqual(lambda: len(self.query_database(leader.metadata.name, "postgres", user_query)), 3,
             "Found incorrect number of rotation users", 10, 5)
 
+        # test that rotation_user can connect to the database
+        self.eventuallyEqual(lambda: len(self.query_database_with_user(leader.metadata.name, "postgres", "SELECT 1", "foo_user")), 1,
+            "Could not connect to the database with rotation user {}".format(rotation_user), 10, 5)
+
         # disable password rotation for all other users (foo_user)
         # and pick smaller intervals to see if the third fake rotation user is dropped 
         enable_password_rotation = {
@@ -1998,5 +2002,28 @@ def query_database(self, pod_name, db_name, query):
 
         return result_set
 
+    def query_database_with_user(self, pod_name, db_name, query, user_name):
+        '''
+           Query database and return result as a list
+        '''
+        k8s = self.k8s
+        result_set = []
+        exec_query = r"PGPASSWORD={} psql -h localhost -U {} -tAq -c \"{}\" -d {}"
+
+        try:
+            user_secret = k8s.get_secret(user_name)
+            secret_user = str(base64.b64decode(user_secret.data["username"]), 'utf-8')
+            secret_pw = str(base64.b64decode(user_secret.data["password"]), 'utf-8')
+            q = exec_query.format(secret_pw, secret_user, query, db_name)
+            q = "su postgres -c \"{}\"".format(q)
+            result = k8s.exec_with_kubectl(pod_name, q)
+            result_set = clean_list(result.stdout.split(b'\n'))
+        except Exception as ex:
+            print('Error on query execution: {}'.format(ex))
+            print('Stdout: {}'.format(result.stdout))
+            print('Stderr: {}'.format(result.stderr))
+
+        return result_set
+
 if __name__ == '__main__':
-    unittest.main()
+    unittest.main()

pkg/cluster/sync.go

@@ -638,7 +638,6 @@ func (c *Cluster) syncSecrets() error {
 	c.logger.Info("syncing secrets")
 	c.setProcessName("syncing secrets")
 	generatedSecrets := c.generateUserSecrets()
-	rotationUsers := make(spec.PgUserMap)
 	retentionUsers := make([]string, 0)
 	currentTime := time.Now()
 
@@ -650,29 +649,14 @@ func (c *Cluster) syncSecrets() error {
 			continue
 		}
 		if k8sutil.ResourceAlreadyExists(err) {
-			if err = c.updateSecret(secretUsername, generatedSecret, &rotationUsers, &retentionUsers, currentTime); err != nil {
+			if err = c.updateSecret(secretUsername, generatedSecret, &retentionUsers, currentTime); err != nil {
 				c.logger.Warningf("syncing secret %s failed: %v", util.NameFromMeta(secret.ObjectMeta), err)
 			}
 		} else {
 			return fmt.Errorf("could not create secret for user %s: in namespace %s: %v", secretUsername, generatedSecret.Namespace, err)
 		}
 	}
 
-	// add new user with date suffix and use it in the secret of the original user
-	if len(rotationUsers) > 0 {
-		err := c.initDbConn()
-		if err != nil {
-			return fmt.Errorf("could not init db connection: %v", err)
-		}
-		pgSyncRequests := c.userSyncStrategy.ProduceSyncRequests(spec.PgUserMap{}, rotationUsers)
-		if err = c.userSyncStrategy.ExecuteSyncRequests(pgSyncRequests, c.pgDb); err != nil {
-			return fmt.Errorf("error creating database roles for password rotation: %v", err)
-		}
-		if err := c.closeDbConn(); err != nil {
-			c.logger.Errorf("could not close database connection after creating users for password rotation: %v", err)
-		}
-	}
-
 	// remove rotation users that exceed the retention interval
 	if len(retentionUsers) > 0 {
 		err := c.initDbConn()
@@ -698,7 +682,6 @@ func (c *Cluster) getNextRotationDate(currentDate time.Time) (time.Time, string)
 func (c *Cluster) updateSecret(
 	secretUsername string,
 	generatedSecret *v1.Secret,
-	rotationUsers *spec.PgUserMap,
 	retentionUsers *[]string,
 	currentTime time.Time) error {
 	var (
@@ -757,7 +740,7 @@ func (c *Cluster) updateSecret(
 	rotationAllowed := !pwdUser.IsDbOwner && util.SliceContains(allowedRoleTypes, pwdUser.Origin)
 
 	if (c.OpConfig.EnablePasswordRotation && rotationAllowed) || rotationEnabledInManifest {
-		updateSecretMsg, err = c.rotatePasswordInSecret(secret, pwdUser, secretUsername, currentTime, rotationUsers, retentionUsers)
+		updateSecretMsg, err = c.rotatePasswordInSecret(secret, secretUsername, pwdUser.Origin, currentTime, retentionUsers)
 		if err != nil {
 			c.logger.Warnf("password rotation failed for user %s: %v", secretUsername, err)
 		}
@@ -782,8 +765,13 @@ func (c *Cluster) updateSecret(
 		updateSecret = true
 		updateSecretMsg = fmt.Sprintf("updating the secret %s from the infrastructure roles", secretName)
 	} else {
-		// for non-infrastructure role - update the role with the password from the secret
+		// for non-infrastructure role - update the role with username and password from secret
+		pwdUser.Name = string(secret.Data["username"])
 		pwdUser.Password = string(secret.Data["password"])
+		// update membership if we deal with a rotation user
+		if secretUsername != pwdUser.Name {
+			pwdUser.MemberOf = []string{secretUsername}
+		}
 		userMap[userKey] = pwdUser
 	}
 
@@ -800,10 +788,9 @@ func (c *Cluster) updateSecret(
 
 func (c *Cluster) rotatePasswordInSecret(
 	secret *v1.Secret,
-	secretPgUser spec.PgUser,
 	secretUsername string,
+	roleOrigin spec.RoleOrigin,
 	currentTime time.Time,
-	rotationUsers *spec.PgUserMap,
 	retentionUsers *[]string) (string, error) {
 	var (
 		err                 error
@@ -833,18 +820,14 @@ func (c *Cluster) rotatePasswordInSecret(
 	if currentTime.After(nextRotationDate) {
 		// create rotation user if role is not listed for in-place password update
 		if !util.SliceContains(c.Spec.UsersWithInPlaceSecretRotation, secretUsername) {
-			rotationUser := secretPgUser
-			newRotationUsername := fmt.Sprintf("%s%s", secretUsername, currentTime.Format("060102"))
-			rotationUser.Name = newRotationUsername
-			rotationUser.MemberOf = []string{secretUsername}
-			(*rotationUsers)[newRotationUsername] = rotationUser
-			secret.Data["username"] = []byte(newRotationUsername)
-
+			rotationUsername := fmt.Sprintf("%s%s", secretUsername, currentTime.Format("060102"))
+			secret.Data["username"] = []byte(rotationUsername)
+			c.logger.Infof("updating username in secret %s and creating rotation user %s in the database", secretName, rotationUsername)
 			// whenever there is a rotation, check if old rotation users can be deleted
 			*retentionUsers = append(*retentionUsers, secretUsername)
 		} else {
 			// when passwords of system users are rotated in place, pods have to be replaced
-			if secretPgUser.Origin == spec.RoleOriginSystem {
+			if roleOrigin == spec.RoleOriginSystem {
 				pods, err := c.listPods()
 				if err != nil {
 					return "", fmt.Errorf("could not list pods of the statefulset: %v", err)
@@ -858,7 +841,7 @@ func (c *Cluster) rotatePasswordInSecret(
 			}
 
 			// when password of connection pooler is rotated in place, pooler pods have to be replaced
-			if secretPgUser.Origin == spec.RoleOriginConnectionPooler {
+			if roleOrigin == spec.RoleOriginConnectionPooler {
 				listOptions := metav1.ListOptions{
 					LabelSelector: c.poolerLabelsSet(true).String(),
 				}
@@ -875,8 +858,8 @@ func (c *Cluster) rotatePasswordInSecret(
 			}
 
 			// when password of stream user is rotated in place, it should trigger rolling update in FES deployment
-			if secretPgUser.Origin == spec.RoleOriginStream {
-				c.logger.Warnf("secret of stream user %s changed", constants.EventStreamSourceSlotPrefix+constants.UserRoleNameSuffix)
+			if roleOrigin == spec.RoleOriginStream {
+				c.logger.Warnf("password in secret of stream user %s changed", constants.EventStreamSourceSlotPrefix+constants.UserRoleNameSuffix)
 			}
 		}
 		secret.Data["password"] = []byte(util.RandomPassword(constants.PasswordLength))

pkg/cluster/sync_test.go

@@ -276,7 +276,6 @@ func TestUpdateSecret(t *testing.T) {
 	dbname := "app"
 	dbowner := "appowner"
 	secretTemplate := config.StringTemplate("{username}.{cluster}.credentials")
-	rotationUsers := make(spec.PgUserMap)
 	retentionUsers := make([]string, 0)
 
 	// define manifest users and enable rotation for dbowner
@@ -339,8 +338,8 @@ func TestUpdateSecret(t *testing.T) {
 	dayAfterTomorrow := time.Now().AddDate(0, 0, 2)
 
 	allUsers := make(map[string]spec.PgUser)
-	for userName, pgUser := range cluster.pgUsers {
-		allUsers[userName] = pgUser
+	for _, pgUser := range cluster.pgUsers {
+		allUsers[pgUser.Name] = pgUser
 	}
 	for _, systemUser := range cluster.systemUsers {
 		allUsers[systemUser.Name] = systemUser
@@ -354,7 +353,7 @@ func TestUpdateSecret(t *testing.T) {
 		secretPassword := string(secret.Data["password"])
 
 		// now update the secret setting a next rotation date (tomorrow + interval)
-		cluster.updateSecret(username, secret, &rotationUsers, &retentionUsers, dayAfterTomorrow)
+		cluster.updateSecret(username, secret, &retentionUsers, dayAfterTomorrow)
 		updatedSecret, err := cluster.KubeClient.Secrets(namespace).Get(context.TODO(), secretName, metav1.GetOptions{})
 		assert.NoError(t, err)
 
@@ -386,9 +385,9 @@ func TestUpdateSecret(t *testing.T) {
 			if secretUsername != rotatedUsername {
 				t.Errorf("%s: updated secret does not contain correct username: expected %s, got %s", testName, rotatedUsername, secretUsername)
 			}
-
-			if len(rotationUsers) != 1 && len(retentionUsers) != 1 {
-				t.Errorf("%s: unexpected number of users to rotate - expected only %s, found %d", testName, username, len(rotationUsers))
+			// whenever there's a rotation the retentionUsers list is extended or updated
+			if len(retentionUsers) != 1 {
+				t.Errorf("%s: unexpected number of users to drop - expected only %s, found %d", testName, username, len(retentionUsers))
 			}
 		}
 	}