pgbouncer user password change should not require verifier (#4084)

* regenerate verifier only when user updates pgBouncer Secret password

* improve logic for calculating verifier

* refactor to remove generatePassword func

* added comment describing MD5/SCRAM requirements

* added test for SCRAM verifier

* refactored logic to clearly capture four possible events

* refactored test

* simplified logic

* removed empty branch to pass linter

* updated test to check for setting verifier only

---------

Co-authored-by: Philip Hurst <[email protected]>

Author: philrhurst

internal/pgbouncer/postgres.go

@@ -12,8 +12,6 @@ import (
 
 	"github.com/crunchydata/postgres-operator/internal/logging"
 	"github.com/crunchydata/postgres-operator/internal/postgres"
-	"github.com/crunchydata/postgres-operator/internal/postgres/password"
-	"github.com/crunchydata/postgres-operator/internal/util"
 )
 
 const (
@@ -203,21 +201,6 @@ REVOKE ALL PRIVILEGES
 	return err
 }
 
-func generatePassword() (plaintext, verifier string, err error) {
-	// PgBouncer can login to PostgreSQL using either MD5 or SCRAM-SHA-256.
-	// When using MD5, the (hashed) verifier can be stored in PgBouncer's
-	// authentication file. When using SCRAM, the plaintext password must be
-	// stored.
-	// - https://www.pgbouncer.org/config.html#authentication-file-format
-	// - https://github.com/pgbouncer/pgbouncer/issues/508#issuecomment-713339834
-
-	plaintext, err = util.GenerateASCIIPassword(32)
-	if err == nil {
-		verifier, err = password.NewSCRAMPassword(plaintext).Build()
-	}
-	return
-}
-
 func postgresqlHBAs() []*postgres.HostBasedAuthentication {
 	// PgBouncer must connect over TLS using a SCRAM password. Other network
 	// connections are forbidden.

internal/pgbouncer/reconcile.go

@@ -18,6 +18,8 @@ import (
 	"github.com/crunchydata/postgres-operator/internal/naming"
 	"github.com/crunchydata/postgres-operator/internal/pki"
 	"github.com/crunchydata/postgres-operator/internal/postgres"
+	passwd "github.com/crunchydata/postgres-operator/internal/postgres/password"
+	"github.com/crunchydata/postgres-operator/internal/util"
 	"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
 )
 
@@ -54,14 +56,29 @@ func Secret(ctx context.Context,
 	var err error
 	initialize.Map(&outSecret.Data)
 
-	// Use the existing password and verifier. Generate both when either is missing.
+	// Use the existing password and verifier. Generate when one is missing.
+	// PgBouncer can login to PostgreSQL using either MD5 or SCRAM-SHA-256.
+	// When using MD5, the (hashed) verifier can be stored in PgBouncer's
+	// authentication file. When using SCRAM, the plaintext password must be
+	// stored.
+	// - https://www.pgbouncer.org/config.html#authentication-file-format
+	// - https://github.com/pgbouncer/pgbouncer/issues/508#issuecomment-713339834
 	// NOTE(cbandy): We don't have a function to compare a plaintext password
 	// to a SCRAM verifier.
 	password := string(inSecret.Data[passwordSecretKey])
 	verifier := string(inSecret.Data[verifierSecretKey])
 
-	if err == nil && (len(password) == 0 || len(verifier) == 0) {
-		password, verifier, err = generatePassword()
+	if len(password) == 0 {
+		// If the password is empty, generate new password and verifier.
+		password, err = util.GenerateASCIIPassword(32)
+		err = errors.WithStack(err)
+		if err == nil {
+			verifier, err = passwd.NewSCRAMPassword(password).Build()
+			err = errors.WithStack(err)
+		}
+	} else if len(password) != 0 && len(verifier) == 0 {
+		// If the password is non-empty and the verifier is empty, generate a new verifier.
+		verifier, err = passwd.NewSCRAMPassword(password).Build()
 		err = errors.WithStack(err)
 	}
 

internal/pgbouncer/reconcile_test.go

@@ -91,6 +91,53 @@ func TestSecret(t *testing.T) {
 	assert.DeepEqual(t, before, intent)
 }
 
+func TestSCRAMVerifier(t *testing.T) {
+	t.Parallel()
+
+	ctx := context.Background()
+	cluster := new(v1beta1.PostgresCluster)
+	service := new(corev1.Service)
+	existing := new(corev1.Secret)
+	intent := new(corev1.Secret)
+
+	root, err := pki.NewRootCertificateAuthority()
+	assert.NilError(t, err)
+
+	cluster.Spec.Proxy = new(v1beta1.PostgresProxySpec)
+	cluster.Spec.Proxy.PGBouncer = new(v1beta1.PGBouncerPodSpec)
+	cluster.Default()
+
+	// Simulate the setting of a password only
+	existing.Data = map[string][]byte{
+		"pgbouncer-password": []byte("password"),
+	}
+
+	// Verify that a SCRAM verifier is set
+	assert.NilError(t, Secret(ctx, cluster, root, existing, service, intent))
+	assert.Assert(t, len(intent.Data["pgbouncer-verifier"]) != 0)
+
+	// Simulate the setting of a password and a verifier
+	intent = new(corev1.Secret)
+	existing.Data = map[string][]byte{
+		"pgbouncer-verifier": []byte("SCRAM-SHA-256$4096:randomsalt:storedkey:serverkey"),
+		"pgbouncer-password": []byte("password"),
+	}
+	assert.NilError(t, Secret(ctx, cluster, root, existing, service, intent))
+	assert.Equal(t, string(intent.Data["pgbouncer-verifier"]), "SCRAM-SHA-256$4096:randomsalt:storedkey:serverkey")
+	assert.Equal(t, string(intent.Data["pgbouncer-password"]), "password")
+
+	// Simulate the setting of a verifier only
+	intent = new(corev1.Secret)
+	existing.Data = map[string][]byte{
+		"pgbouncer-verifier": []byte("SCRAM-SHA-256$4096:randomsalt:storedkey:serverkey"),
+	}
+	assert.NilError(t, Secret(ctx, cluster, root, existing, service, intent))
+	assert.Assert(t, string(intent.Data["pgbouncer-verifier"]) != "SCRAM-SHA-256$4096:randomsalt:storedkey:serverkey")
+	assert.Assert(t, len(intent.Data["pgbouncer-password"]) != 0)
+	assert.Assert(t, len(intent.Data["pgbouncer-verifier"]) != 0)
+
+}
+
 func TestPod(t *testing.T) {
 	t.Parallel()