minor fixes to password rotation (zalando#1796)

* minor fixes to password rotation
* rework unit test

Author: FxKu

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

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.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.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)
 

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

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

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))
+			}
+		}
 	}
 }