From d64603e1d6fd6679a6b5002e3d3b776205917e3d Mon Sep 17 00:00:00 2001 From: Philip Hurst Date: Thu, 30 Jan 2025 22:03:50 +0000 Subject: [PATCH 01/10] regenerate verifier only when user updates pgBouncer Secret password --- internal/pgbouncer/reconcile.go | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/internal/pgbouncer/reconcile.go b/internal/pgbouncer/reconcile.go index ad4f16bb08..bb4762c9bb 100644 --- a/internal/pgbouncer/reconcile.go +++ b/internal/pgbouncer/reconcile.go @@ -17,6 +17,7 @@ import ( "github.com/crunchydata/postgres-operator/internal/naming" "github.com/crunchydata/postgres-operator/internal/pki" "github.com/crunchydata/postgres-operator/internal/postgres" + pwd "github.com/crunchydata/postgres-operator/internal/postgres/password" "github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1" ) @@ -52,17 +53,25 @@ 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. // 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) { + // If the password is empty, generate a new one. + // The user may not have provided a SCRAM verifier. + if err == nil && len(password) == 0 { password, verifier, err = generatePassword() err = errors.WithStack(err) } + // If the verifier is empty, generate a new one. + if err == nil && len(verifier) == 0 { + verifier, err = pwd.NewSCRAMPassword(password).Build() + err = errors.WithStack(err) + } + if err == nil { // Store the SCRAM verifier alongside the plaintext password so that // later reconciles don't generate it repeatedly. From 3816bb0cf32c0fc1f47f4469dc44abc37cf86607 Mon Sep 17 00:00:00 2001 From: Philip Hurst Date: Fri, 31 Jan 2025 01:01:18 +0000 Subject: [PATCH 02/10] improve logic for calculating verifier --- internal/pgbouncer/reconcile.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/pgbouncer/reconcile.go b/internal/pgbouncer/reconcile.go index bb4762c9bb..5267f54e02 100644 --- a/internal/pgbouncer/reconcile.go +++ b/internal/pgbouncer/reconcile.go @@ -60,9 +60,9 @@ func Secret(ctx context.Context, verifier := string(inSecret.Data[verifierSecretKey]) // If the password is empty, generate a new one. - // The user may not have provided a SCRAM verifier. + // Ignore the verifier for now. if err == nil && len(password) == 0 { - password, verifier, err = generatePassword() + password, _, err = generatePassword() err = errors.WithStack(err) } From c76ce140deb553726140a7d966a8509d2055f31f Mon Sep 17 00:00:00 2001 From: Philip Hurst Date: Fri, 31 Jan 2025 01:05:42 +0000 Subject: [PATCH 03/10] refactor to remove generatePassword func --- internal/pgbouncer/postgres.go | 17 ----------------- internal/pgbouncer/reconcile.go | 7 ++++--- 2 files changed, 4 insertions(+), 20 deletions(-) diff --git a/internal/pgbouncer/postgres.go b/internal/pgbouncer/postgres.go index d9a9d91539..495711012c 100644 --- a/internal/pgbouncer/postgres.go +++ b/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. diff --git a/internal/pgbouncer/reconcile.go b/internal/pgbouncer/reconcile.go index 5267f54e02..c9806b1bf4 100644 --- a/internal/pgbouncer/reconcile.go +++ b/internal/pgbouncer/reconcile.go @@ -17,7 +17,8 @@ import ( "github.com/crunchydata/postgres-operator/internal/naming" "github.com/crunchydata/postgres-operator/internal/pki" "github.com/crunchydata/postgres-operator/internal/postgres" - pwd "github.com/crunchydata/postgres-operator/internal/postgres/password" + 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" ) @@ -62,13 +63,13 @@ func Secret(ctx context.Context, // If the password is empty, generate a new one. // Ignore the verifier for now. if err == nil && len(password) == 0 { - password, _, err = generatePassword() + password, err = util.GenerateASCIIPassword(32) err = errors.WithStack(err) } // If the verifier is empty, generate a new one. if err == nil && len(verifier) == 0 { - verifier, err = pwd.NewSCRAMPassword(password).Build() + verifier, err = passwd.NewSCRAMPassword(password).Build() err = errors.WithStack(err) } From 0d815dd3409f37483a967d80a69b187d163e4f8e Mon Sep 17 00:00:00 2001 From: Philip Hurst Date: Fri, 31 Jan 2025 01:35:56 +0000 Subject: [PATCH 04/10] added comment describing MD5/SCRAM requirements --- internal/pgbouncer/reconcile.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/internal/pgbouncer/reconcile.go b/internal/pgbouncer/reconcile.go index c9806b1bf4..d25c2028fb 100644 --- a/internal/pgbouncer/reconcile.go +++ b/internal/pgbouncer/reconcile.go @@ -55,6 +55,12 @@ func Secret(ctx context.Context, initialize.Map(&outSecret.Data) // 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]) From d086472d91636979947d932ba00a618ea40507ad Mon Sep 17 00:00:00 2001 From: Philip Hurst Date: Thu, 20 Feb 2025 19:04:49 +0000 Subject: [PATCH 05/10] added test for SCRAM verifier --- internal/pgbouncer/reconcile_test.go | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/internal/pgbouncer/reconcile_test.go b/internal/pgbouncer/reconcile_test.go index c5d31bc608..65953f8126 100644 --- a/internal/pgbouncer/reconcile_test.go +++ b/internal/pgbouncer/reconcile_test.go @@ -90,6 +90,34 @@ 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 generation of SCRAM verifier + 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)) + + // Verify that the SCRAM verifier is correctly set in the intent secret + assert.Assert(t, len(intent.Data["pgbouncer-verifier"]) != 0) + assert.Equal(t, string(intent.Data["pgbouncer-verifier"]), "SCRAM-SHA-256$4096:randomsalt:storedkey:serverkey") +} + func TestPod(t *testing.T) { t.Parallel() From c0291c9387a898743994b55a4a283cc294e3b007 Mon Sep 17 00:00:00 2001 From: Philip Hurst Date: Wed, 26 Feb 2025 19:54:46 +0000 Subject: [PATCH 06/10] refactored logic to clearly capture four possible events --- internal/pgbouncer/reconcile.go | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/internal/pgbouncer/reconcile.go b/internal/pgbouncer/reconcile.go index d25c2028fb..df26e279bf 100644 --- a/internal/pgbouncer/reconcile.go +++ b/internal/pgbouncer/reconcile.go @@ -66,15 +66,22 @@ func Secret(ctx context.Context, password := string(inSecret.Data[passwordSecretKey]) verifier := string(inSecret.Data[verifierSecretKey]) - // If the password is empty, generate a new one. - // Ignore the verifier for now. - if err == nil && len(password) == 0 { + if len(password) == 0 && len(verifier) == 0 { + // If both the password and verifier are empty, generate new password and verifier. password, err = util.GenerateASCIIPassword(32) err = errors.WithStack(err) - } - - // If the verifier is empty, generate a new one. - if err == nil && len(verifier) == 0 { + if err == nil { + verifier, err = passwd.NewSCRAMPassword(password).Build() + err = errors.WithStack(err) + } + } else if len(password) != 0 && len(verifier) != 0 { + // If both the password and verifier are non-empty, use them. + } else if len(password) == 0 && len(verifier) != 0 { + // If the password is empty and the verifier is non-empty, generate a new password. + password, err = util.GenerateASCIIPassword(32) + 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) } From bdbecf122228ab336451fc2eca27137b27985080 Mon Sep 17 00:00:00 2001 From: Philip Hurst Date: Wed, 26 Feb 2025 19:54:58 +0000 Subject: [PATCH 07/10] refactored test --- internal/pgbouncer/reconcile_test.go | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/internal/pgbouncer/reconcile_test.go b/internal/pgbouncer/reconcile_test.go index 65953f8126..72652359a0 100644 --- a/internal/pgbouncer/reconcile_test.go +++ b/internal/pgbouncer/reconcile_test.go @@ -106,16 +106,25 @@ func TestSCRAMVerifier(t *testing.T) { cluster.Spec.Proxy.PGBouncer = new(v1beta1.PGBouncerPodSpec) cluster.Default() - // Simulate the generation of SCRAM verifier + // Simulate the setting of a password only existing.Data = map[string][]byte{ - "pgbouncer-verifier": []byte("SCRAM-SHA-256$4096:randomsalt:storedkey:serverkey"), + "pgbouncer-password": []byte("password"), } + // Verify that a SCRAM verifier is set assert.NilError(t, Secret(ctx, cluster, root, existing, service, intent)) - - // Verify that the SCRAM verifier is correctly set in the intent secret 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") + } func TestPod(t *testing.T) { From d5893ced619cf124a809dcdc136a008871bdc6fe Mon Sep 17 00:00:00 2001 From: Philip Hurst Date: Wed, 26 Feb 2025 20:13:08 +0000 Subject: [PATCH 08/10] simplified logic --- internal/pgbouncer/reconcile.go | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/internal/pgbouncer/reconcile.go b/internal/pgbouncer/reconcile.go index df26e279bf..45924afb29 100644 --- a/internal/pgbouncer/reconcile.go +++ b/internal/pgbouncer/reconcile.go @@ -66,24 +66,20 @@ func Secret(ctx context.Context, password := string(inSecret.Data[passwordSecretKey]) verifier := string(inSecret.Data[verifierSecretKey]) - if len(password) == 0 && len(verifier) == 0 { - // If both the password and verifier are empty, generate new password and verifier. + 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 both the password and verifier are non-empty, use them. - } else if len(password) == 0 && len(verifier) != 0 { - // If the password is empty and the verifier is non-empty, generate a new password. - password, err = util.GenerateASCIIPassword(32) - 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) + } else if len(password) != 0 && len(verifier) != 0 { + // If both the password and verifier are non-empty, use them. } if err == nil { From dc7266578fc4bf9b12af5cc7beced6cc8967adf4 Mon Sep 17 00:00:00 2001 From: Philip Hurst Date: Wed, 26 Feb 2025 20:18:34 +0000 Subject: [PATCH 09/10] removed empty branch to pass linter --- internal/pgbouncer/reconcile.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/internal/pgbouncer/reconcile.go b/internal/pgbouncer/reconcile.go index 45924afb29..c916ef2162 100644 --- a/internal/pgbouncer/reconcile.go +++ b/internal/pgbouncer/reconcile.go @@ -78,8 +78,6 @@ func Secret(ctx context.Context, // 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) - } else if len(password) != 0 && len(verifier) != 0 { - // If both the password and verifier are non-empty, use them. } if err == nil { From 72974a5705b4c6c8243e058dc1cdc2ed53b5d685 Mon Sep 17 00:00:00 2001 From: Philip Hurst Date: Tue, 11 Mar 2025 14:02:21 +0000 Subject: [PATCH 10/10] updated test to check for setting verifier only --- internal/pgbouncer/reconcile_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/internal/pgbouncer/reconcile_test.go b/internal/pgbouncer/reconcile_test.go index 58850bb262..b8c2a2a9fe 100644 --- a/internal/pgbouncer/reconcile_test.go +++ b/internal/pgbouncer/reconcile_test.go @@ -126,6 +126,16 @@ func TestSCRAMVerifier(t *testing.T) { 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) {