From 4786f53f033154009051c2b1dc300be48c55790a Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Thu, 13 Oct 2022 11:33:26 +0200 Subject: [PATCH] Fix password rotation (#2043) * fix password rotation * test connection with rotation user in e2e test + minor changes --- e2e/tests/test_e2e.py | 29 +++++++++++++++++++++++- pkg/cluster/sync.go | 49 +++++++++++++--------------------------- pkg/cluster/sync_test.go | 13 +++++------ 3 files changed, 50 insertions(+), 41 deletions(-) diff --git a/e2e/tests/test_e2e.py b/e2e/tests/test_e2e.py index 82e9cb39c..d13a51c19 100644 --- a/e2e/tests/test_e2e.py +++ b/e2e/tests/test_e2e.py @@ -1442,6 +1442,10 @@ class EndToEndTestCase(unittest.TestCase): 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 @@ class EndToEndTestCase(unittest.TestCase): 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() \ No newline at end of file + unittest.main() diff --git a/pkg/cluster/sync.go b/pkg/cluster/sync.go index 68f00533a..cc795634f 100644 --- a/pkg/cluster/sync.go +++ b/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,7 +649,7 @@ 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 { @@ -658,21 +657,6 @@ func (c *Cluster) syncSecrets() error { } } - // 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)) diff --git a/pkg/cluster/sync_test.go b/pkg/cluster/sync_test.go index cc7554b0e..785a7cca2 100644 --- a/pkg/cluster/sync_test.go +++ b/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)) } } }