From 8b404fd0492de92edc7a0da924b0bebcdcd6f743 Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Fri, 25 Feb 2022 17:46:26 +0100 Subject: [PATCH] minor fixes to password rotation (#1796) * minor fixes to password rotation * rework unit test --- docs/administrator.md | 8 +- e2e/tests/test_e2e.py | 6 +- manifests/complete-postgres-manifest.yaml | 8 +- pkg/cluster/sync.go | 16 ++-- pkg/cluster/sync_test.go | 91 +++++++++++++++-------- 5 files changed, 82 insertions(+), 47 deletions(-) diff --git a/docs/administrator.md b/docs/administrator.md index 3c5d8ae46..e68427658 100644 --- a/docs/administrator.md +++ b/docs/administrator.md @@ -306,10 +306,10 @@ The interval of days can be set with `password_rotation_interval` (default are replaced in the K8s secret. They belong to a newly created user named after the original role plus rotation date in YYMMDD format. All priviliges are inherited meaning that migration scripts should still grant and revoke rights -against the original role. The timestamp of the next rotation is written to the -secret as well. Note, if the rotation interval is decreased it is reflected in -the secrets only if the next rotation date is more days away than the new -length of the interval. +against the original role. The timestamp of the next rotation (in RFC 3339 +format, UTC timezone) is written to the secret as well. Note, if the rotation +interval is decreased it is reflected in the secrets only if the next rotation +date is more days away than the new length of the interval. Pods still using the previous secret values which they keep in memory continue to connect to the database since the password of the corresponding user is not diff --git a/e2e/tests/test_e2e.py b/e2e/tests/test_e2e.py index d1db4991b..c4d104069 100644 --- a/e2e/tests/test_e2e.py +++ b/e2e/tests/test_e2e.py @@ -1232,7 +1232,7 @@ class EndToEndTestCase(unittest.TestCase): # check if next rotation date was set in secret secret_data = k8s.get_secret_data("zalando") - next_rotation_timestamp = datetime.fromisoformat(str(base64.b64decode(secret_data["nextRotation"]), 'utf-8')) + next_rotation_timestamp = datetime.strptime(str(base64.b64decode(secret_data["nextRotation"]), 'utf-8'), "%Y-%m-%dT%H:%M:%SZ") today90days = today+timedelta(days=90) self.assertEqual(today90days, next_rotation_timestamp.date(), "Unexpected rotation date in secret of zalando user: expected {}, got {}".format(today90days, next_rotation_timestamp.date())) @@ -1247,7 +1247,7 @@ class EndToEndTestCase(unittest.TestCase): self.query_database(leader.metadata.name, "postgres", create_fake_rotation_user) # patch foo_user secret with outdated rotation date - fake_rotation_date = today.isoformat() + ' 00:00:00' + fake_rotation_date = today.isoformat() + 'T00:00:00Z' fake_rotation_date_encoded = base64.b64encode(fake_rotation_date.encode('utf-8')) secret_fake_rotation = { "data": { @@ -1275,7 +1275,7 @@ class EndToEndTestCase(unittest.TestCase): # check if next rotation date and username have been replaced secret_data = k8s.get_secret_data("foo_user") secret_username = str(base64.b64decode(secret_data["username"]), 'utf-8') - next_rotation_timestamp = datetime.fromisoformat(str(base64.b64decode(secret_data["nextRotation"]), 'utf-8')) + next_rotation_timestamp = datetime.strptime(str(base64.b64decode(secret_data["nextRotation"]), 'utf-8'), "%Y-%m-%dT%H:%M:%SZ") rotation_user = "foo_user"+today.strftime("%y%m%d") today30days = today+timedelta(days=30) diff --git a/manifests/complete-postgres-manifest.yaml b/manifests/complete-postgres-manifest.yaml index 577b2711b..890f5eed3 100644 --- a/manifests/complete-postgres-manifest.yaml +++ b/manifests/complete-postgres-manifest.yaml @@ -17,8 +17,12 @@ spec: - superuser - createdb foo_user: [] -# usersWithSecretRotation: "foo_user" -# usersWithInPlaceSecretRotation: "flyway,bar_owner_user" +# flyway: [] +# usersWithSecretRotation: +# - foo_user +# usersWithInPlaceSecretRotation: +# - flyway +# - bar_owner_user enableMasterLoadBalancer: false enableReplicaLoadBalancer: false enableConnectionPooler: false # enable/disable connection pooler deployment diff --git a/pkg/cluster/sync.go b/pkg/cluster/sync.go index 24c68dd54..bbf023764 100644 --- a/pkg/cluster/sync.go +++ b/pkg/cluster/sync.go @@ -620,11 +620,6 @@ func (c *Cluster) checkAndSetGlobalPostgreSQLConfiguration(pod *v1.Pod, patroniC return requiresMasterRestart, nil } -func (c *Cluster) getNextRotationDate(currentDate time.Time) (time.Time, string) { - nextRotationDate := currentDate.AddDate(0, 0, int(c.OpConfig.PasswordRotationInterval)) - return nextRotationDate, nextRotationDate.Format("2006-01-02 15:04:05") -} - func (c *Cluster) syncSecrets() error { c.logger.Info("syncing secrets") @@ -682,6 +677,11 @@ func (c *Cluster) syncSecrets() error { return nil } +func (c *Cluster) getNextRotationDate(currentDate time.Time) (time.Time, string) { + nextRotationDate := currentDate.AddDate(0, 0, int(c.OpConfig.PasswordRotationInterval)) + return nextRotationDate, nextRotationDate.Format(time.RFC3339) +} + func (c *Cluster) updateSecret( secretUsername string, generatedSecret *v1.Secret, @@ -727,7 +727,7 @@ func (c *Cluster) updateSecret( // initialize password rotation setting first rotation date nextRotationDateStr = string(secret.Data["nextRotation"]) - if nextRotationDate, err = time.ParseInLocation("2006-01-02 15:04:05", nextRotationDateStr, time.Local); err != nil { + if nextRotationDate, err = time.ParseInLocation(time.RFC3339, nextRotationDateStr, currentTime.UTC().Location()); err != nil { nextRotationDate, nextRotationDateStr = c.getNextRotationDate(currentTime) secret.Data["nextRotation"] = []byte(nextRotationDateStr) updateSecret = true @@ -736,7 +736,7 @@ func (c *Cluster) updateSecret( // check if next rotation can happen sooner // if rotation interval has been decreased - currentRotationDate, _ := c.getNextRotationDate(currentTime) + currentRotationDate, nextRotationDateStr := c.getNextRotationDate(currentTime) if nextRotationDate.After(currentRotationDate) { nextRotationDate = currentRotationDate } @@ -756,8 +756,6 @@ func (c *Cluster) updateSecret( *retentionUsers = append(*retentionUsers, secretUsername) } secret.Data["password"] = []byte(util.RandomPassword(constants.PasswordLength)) - - _, nextRotationDateStr = c.getNextRotationDate(nextRotationDate) secret.Data["nextRotation"] = []byte(nextRotationDateStr) updateSecret = true diff --git a/pkg/cluster/sync_test.go b/pkg/cluster/sync_test.go index 80e2b8463..226555a66 100644 --- a/pkg/cluster/sync_test.go +++ b/pkg/cluster/sync_test.go @@ -270,13 +270,29 @@ func TestUpdateSecret(t *testing.T) { clusterName := "acid-test-cluster" namespace := "default" - username := "foo" + dbname := "app" + dbowner := "appowner" secretTemplate := config.StringTemplate("{username}.{cluster}.credentials") rotationUsers := make(spec.PgUserMap) retentionUsers := make([]string, 0) - yesterday := time.Now().AddDate(0, 0, -1) - // new cluster with pvc storage resize mode and configured labels + // define manifest users and enable rotation for dbowner + pg := acidv1.Postgresql{ + ObjectMeta: metav1.ObjectMeta{ + Name: clusterName, + Namespace: namespace, + }, + Spec: acidv1.PostgresSpec{ + Databases: map[string]string{dbname: dbowner}, + Users: map[string]acidv1.UserFlags{"foo": {}, dbowner: {}}, + UsersWithInPlaceSecretRotation: []string{dbowner}, + Volume: acidv1.Volume{ + Size: "1Gi", + }, + }, + } + + // new cluster with enabled password rotation var cluster = New( Config{ OpConfig: config.Config{ @@ -291,44 +307,61 @@ func TestUpdateSecret(t *testing.T) { ClusterNameLabel: "cluster-name", }, }, - }, client, acidv1.Postgresql{}, logger, eventRecorder) + }, client, pg, logger, eventRecorder) cluster.Name = clusterName cluster.Namespace = namespace cluster.pgUsers = map[string]spec.PgUser{} - cluster.Spec.Users = map[string]acidv1.UserFlags{username: {}} cluster.initRobotUsers() - // create a secret for user foo + // create secrets + cluster.syncSecrets() + // initialize rotation with current time cluster.syncSecrets() - secret, err := cluster.KubeClient.Secrets(namespace).Get(context.TODO(), secretTemplate.Format("username", username, "cluster", clusterName), metav1.GetOptions{}) - assert.NoError(t, err) - generatedSecret := cluster.Secrets[secret.UID] + dayAfterTomorrow := time.Now().AddDate(0, 0, 2) - // now update the secret setting next rotation date (yesterday + interval) - cluster.updateSecret(username, generatedSecret, &rotationUsers, &retentionUsers, yesterday) - updatedSecret, err := cluster.KubeClient.Secrets(namespace).Get(context.TODO(), secretTemplate.Format("username", username, "cluster", clusterName), metav1.GetOptions{}) - assert.NoError(t, err) + for username := range cluster.Spec.Users { + pgUser := cluster.pgUsers[username] - nextRotation := string(updatedSecret.Data["nextRotation"]) - _, nextRotationDate := cluster.getNextRotationDate(yesterday) - if nextRotation != nextRotationDate { - t.Errorf("%s: updated secret does not contain correct rotation date: expected %s, got %s", testName, nextRotationDate, nextRotation) - } + // first, get the secret + secret, err := cluster.KubeClient.Secrets(namespace).Get(context.TODO(), secretTemplate.Format("username", username, "cluster", clusterName), metav1.GetOptions{}) + assert.NoError(t, err) + secretPassword := string(secret.Data["password"]) - // update secret again but use current time to trigger rotation - cluster.updateSecret(username, generatedSecret, &rotationUsers, &retentionUsers, time.Now()) - updatedSecret, err = cluster.KubeClient.Secrets(namespace).Get(context.TODO(), secretTemplate.Format("username", username, "cluster", clusterName), metav1.GetOptions{}) - assert.NoError(t, err) + // now update the secret setting a next rotation date (tomorrow + interval) + cluster.updateSecret(username, secret, &rotationUsers, &retentionUsers, dayAfterTomorrow) + updatedSecret, err := cluster.KubeClient.Secrets(namespace).Get(context.TODO(), secretTemplate.Format("username", username, "cluster", clusterName), metav1.GetOptions{}) + assert.NoError(t, err) - if len(rotationUsers) != 1 && len(retentionUsers) != 1 { - t.Errorf("%s: unexpected number of users to rotate - expected only foo, found %d", testName, len(rotationUsers)) - } + // check that passwords are different + rotatedPassword := string(updatedSecret.Data["password"]) + if secretPassword == rotatedPassword { + t.Errorf("%s: password unchanged in updated secret for %s", testName, username) + } - secretUsername := string(updatedSecret.Data["username"]) - rotatedUsername := username + time.Now().Format("060102") - if secretUsername != rotatedUsername { - t.Errorf("%s: updated secret does not contain correct username: expected %s, got %s", testName, rotatedUsername, secretUsername) + // check that next rotation date is tomorrow + interval, not date in secret + interval + nextRotation := string(updatedSecret.Data["nextRotation"]) + _, nextRotationDate := cluster.getNextRotationDate(dayAfterTomorrow) + if nextRotation != nextRotationDate { + t.Errorf("%s: updated secret of %s does not contain correct rotation date: expected %s, got %s", testName, username, nextRotationDate, nextRotation) + } + + // compare username, when it's dbowner they should be equal because of UsersWithInPlaceSecretRotation + secretUsername := string(updatedSecret.Data["username"]) + if pgUser.IsDbOwner { + if secretUsername != username { + t.Errorf("%s: username differs in updated secret: expected %s, got %s", testName, username, secretUsername) + } + } else { + rotatedUsername := username + dayAfterTomorrow.Format("060102") + 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)) + } + } } }