From c0cf72a25ffb5e4cb03ceea05bd476c342f0f728 Mon Sep 17 00:00:00 2001 From: Chris Bandy Date: Thu, 20 Mar 2025 10:37:36 -0500 Subject: [PATCH] Ensure required LDAP HBA options are present Issue: PGO-2263 --- ...ator.crunchydata.com_postgresclusters.yaml | 22 +++- .../validation/postgrescluster_test.go | 115 ++++++++++++++++++ .../v1beta1/postgres_types.go | 15 ++- 3 files changed, 146 insertions(+), 6 deletions(-) diff --git a/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml b/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml index d0891d05ba..3834ebf654 100644 --- a/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml +++ b/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml @@ -110,11 +110,25 @@ spec: x-kubernetes-map-type: atomic x-kubernetes-validations: - message: '"hba" cannot be combined with other fields' - rule: 'has(self.hba) ? !has(self.connection) && !has(self.databases) - && !has(self.method) && !has(self.options) && !has(self.users) - : true' + rule: '[has(self.hba), has(self.connection) || has(self.databases) + || has(self.method) || has(self.options) || has(self.users)].exists_one(b,b)' - message: '"connection" and "method" are required' - rule: 'has(self.hba) ? true : has(self.connection) && has(self.method)' + rule: has(self.hba) || (has(self.connection) && has(self.method)) + - message: the "ldap" method requires an "ldapbasedn", "ldapprefix", + or "ldapsuffix" option + rule: has(self.hba) || self.method != "ldap" || (has(self.options) + && ["ldapbasedn","ldapprefix","ldapsuffix"].exists(k, k + in self.options)) + - message: cannot use "ldapbasedn", "ldapbinddn", "ldapbindpasswd", + "ldapsearchattribute", or "ldapsearchfilter" options with + "ldapprefix" or "ldapsuffix" options + rule: has(self.hba) || self.method != "ldap" || !has(self.options) + || [["ldapprefix","ldapsuffix"], ["ldapbasedn","ldapbinddn","ldapbindpasswd","ldapsearchattribute","ldapsearchfilter"]].exists_one(a, + a.exists(k, k in self.options)) + - message: the "radius" method requires "radiusservers" and + "radiussecrets" options + rule: has(self.hba) || self.method != "radius" || (has(self.options) + && ["radiusservers","radiussecrets"].all(k, k in self.options)) maxItems: 10 type: array x-kubernetes-list-type: atomic diff --git a/internal/testing/validation/postgrescluster_test.go b/internal/testing/validation/postgrescluster_test.go index 18a17de069..f10fbe8023 100644 --- a/internal/testing/validation/postgrescluster_test.go +++ b/internal/testing/validation/postgrescluster_test.go @@ -118,6 +118,121 @@ func TestPostgresAuthenticationRules(t *testing.T) { assert.Assert(t, cmp.Contains(cause.Message, "unsafe")) } }) + + t.Run("LDAP", func(t *testing.T) { + t.Run("Required", func(t *testing.T) { + cluster := base.DeepCopy() + require.UnmarshalInto(t, &cluster.Spec.Authentication, `{ + rules: [ + { connection: hostssl, method: ldap }, + { connection: hostssl, method: ldap, options: {} }, + { connection: hostssl, method: ldap, options: { ldapbinddn: any } }, + ], + }`) + + err := cc.Create(ctx, cluster, client.DryRunAll) + assert.Assert(t, apierrors.IsInvalid(err)) + + status := require.StatusError(t, err) + assert.Assert(t, status.Details != nil) + assert.Assert(t, cmp.Len(status.Details.Causes, 3)) + + for i, cause := range status.Details.Causes { + assert.Equal(t, cause.Field, fmt.Sprintf("spec.authentication.rules[%d]", i), "%#v", cause) + assert.Assert(t, cmp.Contains(cause.Message, `"ldap" method requires`)) + } + + // These are valid. + + cluster.Spec.Authentication = nil + require.UnmarshalInto(t, &cluster.Spec.Authentication, `{ + rules: [ + { connection: hostssl, method: ldap, options: { ldapbasedn: any } }, + { connection: hostssl, method: ldap, options: { ldapprefix: any } }, + { connection: hostssl, method: ldap, options: { ldapsuffix: any } }, + ], + }`) + assert.NilError(t, cc.Create(ctx, cluster, client.DryRunAll)) + }) + + t.Run("Mixed", func(t *testing.T) { + // Some options cannot be combined with others. + + cluster := base.DeepCopy() + require.UnmarshalInto(t, &cluster.Spec.Authentication, `{ + rules: [ + { connection: hostssl, method: ldap, options: { ldapbinddn: any, ldapprefix: other } }, + { connection: hostssl, method: ldap, options: { ldapbasedn: any, ldapsuffix: other } }, + ], + }`) + + err := cc.Create(ctx, cluster, client.DryRunAll) + assert.Assert(t, apierrors.IsInvalid(err)) + + status := require.StatusError(t, err) + assert.Assert(t, status.Details != nil) + assert.Assert(t, cmp.Len(status.Details.Causes, 2)) + + for i, cause := range status.Details.Causes { + assert.Equal(t, cause.Field, fmt.Sprintf("spec.authentication.rules[%d]", i), "%#v", cause) + assert.Assert(t, cmp.Regexp(`cannot use .+? options with .+? options`, cause.Message)) + } + + // These combinations are allowed. + + cluster.Spec.Authentication = nil + require.UnmarshalInto(t, &cluster.Spec.Authentication, `{ + rules: [ + { connection: hostssl, method: ldap, options: { ldapprefix: one, ldapsuffix: two } }, + { connection: hostssl, method: ldap, options: { ldapbasedn: one, ldapbinddn: two } }, + { connection: hostssl, method: ldap, options: { + ldapbasedn: one, ldapsearchattribute: two, ldapsearchfilter: three, + } }, + ], + }`) + assert.NilError(t, cc.Create(ctx, cluster, client.DryRunAll)) + }) + }) + + t.Run("RADIUS", func(t *testing.T) { + t.Run("Required", func(t *testing.T) { + cluster := base.DeepCopy() + require.UnmarshalInto(t, &cluster.Spec.Authentication, `{ + rules: [ + { connection: hostssl, method: radius }, + { connection: hostssl, method: radius, options: {} }, + { connection: hostssl, method: radius, options: { radiusidentifiers: any } }, + { connection: hostssl, method: radius, options: { radiusservers: any } }, + { connection: hostssl, method: radius, options: { radiussecrets: any } }, + ], + }`) + + err := cc.Create(ctx, cluster, client.DryRunAll) + assert.Assert(t, apierrors.IsInvalid(err)) + + status := require.StatusError(t, err) + assert.Assert(t, status.Details != nil) + assert.Assert(t, cmp.Len(status.Details.Causes, 5)) + + for i, cause := range status.Details.Causes { + assert.Equal(t, cause.Field, fmt.Sprintf("spec.authentication.rules[%d]", i), "%#v", cause) + assert.Assert(t, cmp.Contains(cause.Message, `"radius" method requires`)) + } + + // These are valid. + + cluster.Spec.Authentication = nil + require.UnmarshalInto(t, &cluster.Spec.Authentication, `{ + rules: [ + { connection: hostssl, method: radius, options: { radiusservers: one, radiussecrets: two } }, + { connection: hostssl, method: radius, options: { + radiusservers: one, radiussecrets: two, radiusports: three, + } }, + ], + }`) + assert.NilError(t, cc.Create(ctx, cluster, client.DryRunAll)) + }) + }) } func TestPostgresConfigParameters(t *testing.T) { diff --git a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgres_types.go b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgres_types.go index 8f950dbfa9..b70a21a88d 100644 --- a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgres_types.go +++ b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgres_types.go @@ -124,8 +124,19 @@ type PostgresHBARule struct { // --- // Emulate OpenAPI "anyOf" aka Kubernetes union. -// +kubebuilder:validation:XValidation:rule=`has(self.hba) ? !has(self.connection) && !has(self.databases) && !has(self.method) && !has(self.options) && !has(self.users) : true`,message=`"hba" cannot be combined with other fields` -// +kubebuilder:validation:XValidation:rule=`has(self.hba) ? true : has(self.connection) && has(self.method)`,message=`"connection" and "method" are required` +// +kubebuilder:validation:XValidation:rule=`[has(self.hba), has(self.connection) || has(self.databases) || has(self.method) || has(self.options) || has(self.users)].exists_one(b,b)`,message=`"hba" cannot be combined with other fields` +// +kubebuilder:validation:XValidation:rule=`has(self.hba) || (has(self.connection) && has(self.method))`,message=`"connection" and "method" are required` +// +// Some authentication methods *must* be further configured via options. +// +// https://git.postgresql.org/gitweb/?p=postgresql.git;hb=refs/tags/REL_10_0;f=src/backend/libpq/hba.c#l1501 +// https://git.postgresql.org/gitweb/?p=postgresql.git;hb=refs/tags/REL_17_0;f=src/backend/libpq/hba.c#l1886 +// +kubebuilder:validation:XValidation:rule=`has(self.hba) || self.method != "ldap" || (has(self.options) && ["ldapbasedn","ldapprefix","ldapsuffix"].exists(k, k in self.options))`,message=`the "ldap" method requires an "ldapbasedn", "ldapprefix", or "ldapsuffix" option` +// +kubebuilder:validation:XValidation:rule=`has(self.hba) || self.method != "ldap" || !has(self.options) || [["ldapprefix","ldapsuffix"], ["ldapbasedn","ldapbinddn","ldapbindpasswd","ldapsearchattribute","ldapsearchfilter"]].exists_one(a, a.exists(k, k in self.options))`,message=`cannot use "ldapbasedn", "ldapbinddn", "ldapbindpasswd", "ldapsearchattribute", or "ldapsearchfilter" options with "ldapprefix" or "ldapsuffix" options` +// +// https://git.postgresql.org/gitweb/?p=postgresql.git;hb=refs/tags/REL_10_0;f=src/backend/libpq/hba.c#l1539 +// https://git.postgresql.org/gitweb/?p=postgresql.git;hb=refs/tags/REL_17_0;f=src/backend/libpq/hba.c#l1945 +// +kubebuilder:validation:XValidation:rule=`has(self.hba) || self.method != "radius" || (has(self.options) && ["radiusservers","radiussecrets"].all(k, k in self.options))`,message=`the "radius" method requires "radiusservers" and "radiussecrets" options` // // +structType=atomic type PostgresHBARuleSpec struct {