From 1e0b7286bdb996d108dd1d3bf95a6e1701b2971c Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Wed, 23 Feb 2022 16:13:49 +0100 Subject: [PATCH 1/6] minor fixes to password rotation --- docs/administrator.md | 8 ++++---- manifests/complete-postgres-manifest.yaml | 8 ++++++-- pkg/cluster/sync.go | 14 +++++++------- 3 files changed, 17 insertions(+), 13 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/manifests/complete-postgres-manifest.yaml b/manifests/complete-postgres-manifest.yaml index c150b616d..276f969e0 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 c00f0a189..53784cfc2 100644 --- a/pkg/cluster/sync.go +++ b/pkg/cluster/sync.go @@ -611,11 +611,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") @@ -673,6 +668,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, @@ -718,7 +718,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, time.Now().UTC().Location()); err != nil { nextRotationDate, nextRotationDateStr = c.getNextRotationDate(currentTime) secret.Data["nextRotation"] = []byte(nextRotationDateStr) updateSecret = true @@ -748,7 +748,7 @@ func (c *Cluster) updateSecret( } secret.Data["password"] = []byte(util.RandomPassword(constants.PasswordLength)) - _, nextRotationDateStr = c.getNextRotationDate(nextRotationDate) + _, nextRotationDateStr = c.getNextRotationDate(currentTime) secret.Data["nextRotation"] = []byte(nextRotationDateStr) updateSecret = true From b3f4af063dad12657918df5ab53dec329da1a2d5 Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Wed, 23 Feb 2022 18:57:10 +0100 Subject: [PATCH 2/6] rework unit test --- pkg/cluster/sync.go | 4 +- pkg/cluster/sync_test.go | 91 +++++++++++++++++++++++++++------------- 2 files changed, 63 insertions(+), 32 deletions(-) diff --git a/pkg/cluster/sync.go b/pkg/cluster/sync.go index 53784cfc2..a139c597b 100644 --- a/pkg/cluster/sync.go +++ b/pkg/cluster/sync.go @@ -727,7 +727,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 } @@ -747,8 +747,6 @@ func (c *Cluster) updateSecret( *retentionUsers = append(*retentionUsers, secretUsername) } secret.Data["password"] = []byte(util.RandomPassword(constants.PasswordLength)) - - _, nextRotationDateStr = c.getNextRotationDate(currentTime) secret.Data["nextRotation"] = []byte(nextRotationDateStr) updateSecret = true diff --git a/pkg/cluster/sync_test.go b/pkg/cluster/sync_test.go index 80e2b8463..70a5cf82d 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] + tomorrow := 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, tomorrow) + 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(tomorrow) + 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 + tomorrow.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)) + } + } } } From 5e7f3a48175ba44508d20ed8d97ea5cc21d0b5d4 Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Wed, 23 Feb 2022 19:00:08 +0100 Subject: [PATCH 3/6] day after tomorrow --- pkg/cluster/sync_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/cluster/sync_test.go b/pkg/cluster/sync_test.go index 70a5cf82d..226555a66 100644 --- a/pkg/cluster/sync_test.go +++ b/pkg/cluster/sync_test.go @@ -319,7 +319,7 @@ func TestUpdateSecret(t *testing.T) { // initialize rotation with current time cluster.syncSecrets() - tomorrow := time.Now().AddDate(0, 0, 2) + dayAfterTomorrow := time.Now().AddDate(0, 0, 2) for username := range cluster.Spec.Users { pgUser := cluster.pgUsers[username] @@ -330,7 +330,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, tomorrow) + 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) @@ -342,7 +342,7 @@ func TestUpdateSecret(t *testing.T) { // check that next rotation date is tomorrow + interval, not date in secret + interval nextRotation := string(updatedSecret.Data["nextRotation"]) - _, nextRotationDate := cluster.getNextRotationDate(tomorrow) + _, 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) } @@ -354,7 +354,7 @@ func TestUpdateSecret(t *testing.T) { t.Errorf("%s: username differs in updated secret: expected %s, got %s", testName, username, secretUsername) } } else { - rotatedUsername := username + tomorrow.Format("060102") + 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) } From 46f595a53a59de9033f04fbdd53920858c213e42 Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Wed, 23 Feb 2022 19:01:50 +0100 Subject: [PATCH 4/6] another cosmetic change --- pkg/cluster/sync.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cluster/sync.go b/pkg/cluster/sync.go index a139c597b..9f84ef3da 100644 --- a/pkg/cluster/sync.go +++ b/pkg/cluster/sync.go @@ -718,7 +718,7 @@ func (c *Cluster) updateSecret( // initialize password rotation setting first rotation date nextRotationDateStr = string(secret.Data["nextRotation"]) - if nextRotationDate, err = time.ParseInLocation(time.RFC3339, nextRotationDateStr, time.Now().UTC().Location()); 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 From 52d6a904c479d4401df54235cbd81ed8c7cc6a80 Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Fri, 25 Feb 2022 12:13:08 +0100 Subject: [PATCH 5/6] reflect timestamp change in secret --- e2e/tests/test_e2e.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/e2e/tests/test_e2e.py b/e2e/tests/test_e2e.py index d1db4991b..a65b031fc 100644 --- a/e2e/tests/test_e2e.py +++ b/e2e/tests/test_e2e.py @@ -1232,7 +1232,7 @@ def test_password_rotation(self): # 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.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 @@ def test_password_rotation(self): 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 @@ def test_password_rotation(self): # 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.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) From afd3a5f298b70b813b8600e15655c35ad71a8b6c Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Fri, 25 Feb 2022 15:33:20 +0100 Subject: [PATCH 6/6] one datetime less --- e2e/tests/test_e2e.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/e2e/tests/test_e2e.py b/e2e/tests/test_e2e.py index a65b031fc..c4d104069 100644 --- a/e2e/tests/test_e2e.py +++ b/e2e/tests/test_e2e.py @@ -1232,7 +1232,7 @@ def test_password_rotation(self): # check if next rotation date was set in secret secret_data = k8s.get_secret_data("zalando") - next_rotation_timestamp = datetime.datetime.strptime(str(base64.b64decode(secret_data["nextRotation"]), 'utf-8'), "%Y-%m-%dT%H:%M:%SZ") + 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())) @@ -1275,7 +1275,7 @@ def test_password_rotation(self): # 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.datetime.strptime(str(base64.b64decode(secret_data["nextRotation"]), 'utf-8'), "%Y-%m-%dT%H:%M:%SZ") + 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)