Skip to content

Commit 797fdf1

Browse files
cbandycbandy
committed
Ensure required LDAP HBA options are present
Issue: PGO-2263
1 parent 7a00601 commit 797fdf1

File tree

3 files changed

+146
-6
lines changed

3 files changed

+146
-6
lines changed

‎config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml

+18-4
Original file line numberDiff line numberDiff line change
@@ -110,11 +110,25 @@ spec:
110110
x-kubernetes-map-type: atomic
111111
x-kubernetes-validations:
112112
- message: '"hba" cannot be combined with other fields'
113-
rule: 'has(self.hba) ? !has(self.connection) && !has(self.databases)
114-
&& !has(self.method) && !has(self.options) && !has(self.users)
115-
: true'
113+
rule: '[has(self.hba), has(self.connection) || has(self.databases)
114+
|| has(self.method) || has(self.options) || has(self.users)].exists_one(b,b)'
116115
- message: '"connection" and "method" are required'
117-
rule: 'has(self.hba) ? true : has(self.connection) && has(self.method)'
116+
rule: has(self.hba) || (has(self.connection) && has(self.method))
117+
- message: the "ldap" method requires an "ldapbasedn", "ldapprefix",
118+
or "ldapsuffix" option
119+
rule: has(self.hba) || self.method != "ldap" || (has(self.options)
120+
&& ["ldapbasedn","ldapprefix","ldapsuffix"].exists(k, k
121+
in self.options))
122+
- message: cannot use "ldapbasedn", "ldapbinddn", "ldapbindpasswd",
123+
"ldapsearchattribute", or "ldapsearchfilter" options with
124+
"ldapprefix" or "ldapsuffix" options
125+
rule: has(self.hba) || self.method != "ldap" || !has(self.options)
126+
|| [["ldapprefix","ldapsuffix"], ["ldapbasedn","ldapbinddn","ldapbindpasswd","ldapsearchattribute","ldapsearchfilter"]].exists_one(a,
127+
a.exists(k, k in self.options))
128+
- message: the "radius" method requires "radiusservers" and
129+
"radiussecrets" options
130+
rule: has(self.hba) || self.method != "radius" || (has(self.options)
131+
&& ["radiusservers","radiussecrets"].all(k, k in self.options))
118132
maxItems: 10
119133
type: array
120134
x-kubernetes-list-type: atomic

‎internal/testing/validation/postgrescluster_test.go

+115
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,121 @@ func TestPostgresAuthenticationRules(t *testing.T) {
118118
assert.Assert(t, cmp.Contains(cause.Message, "unsafe"))
119119
}
120120
})
121+
122+
t.Run("LDAP", func(t *testing.T) {
123+
t.Run("Required", func(t *testing.T) {
124+
cluster := base.DeepCopy()
125+
require.UnmarshalInto(t, &cluster.Spec.Authentication, `{
126+
rules: [
127+
{ connection: hostssl, method: ldap },
128+
{ connection: hostssl, method: ldap, options: {} },
129+
{ connection: hostssl, method: ldap, options: { ldapbinddn: any } },
130+
],
131+
}`)
132+
133+
err := cc.Create(ctx, cluster, client.DryRunAll)
134+
assert.Assert(t, apierrors.IsInvalid(err))
135+
136+
status := require.StatusError(t, err)
137+
assert.Assert(t, status.Details != nil)
138+
assert.Assert(t, cmp.Len(status.Details.Causes, 3))
139+
140+
for i, cause := range status.Details.Causes {
141+
assert.Equal(t, cause.Field, fmt.Sprintf("spec.authentication.rules[%d]", i), "%#v", cause)
142+
assert.Assert(t, cmp.Contains(cause.Message, `"ldap" method requires`))
143+
}
144+
145+
// These are valid.
146+
147+
cluster.Spec.Authentication = nil
148+
require.UnmarshalInto(t, &cluster.Spec.Authentication, `{
149+
rules: [
150+
{ connection: hostssl, method: ldap, options: { ldapbasedn: any } },
151+
{ connection: hostssl, method: ldap, options: { ldapprefix: any } },
152+
{ connection: hostssl, method: ldap, options: { ldapsuffix: any } },
153+
],
154+
}`)
155+
assert.NilError(t, cc.Create(ctx, cluster, client.DryRunAll))
156+
})
157+
158+
t.Run("Mixed", func(t *testing.T) {
159+
// Some options cannot be combined with others.
160+
161+
cluster := base.DeepCopy()
162+
require.UnmarshalInto(t, &cluster.Spec.Authentication, `{
163+
rules: [
164+
{ connection: hostssl, method: ldap, options: { ldapbinddn: any, ldapprefix: other } },
165+
{ connection: hostssl, method: ldap, options: { ldapbasedn: any, ldapsuffix: other } },
166+
],
167+
}`)
168+
169+
err := cc.Create(ctx, cluster, client.DryRunAll)
170+
assert.Assert(t, apierrors.IsInvalid(err))
171+
172+
status := require.StatusError(t, err)
173+
assert.Assert(t, status.Details != nil)
174+
assert.Assert(t, cmp.Len(status.Details.Causes, 2))
175+
176+
for i, cause := range status.Details.Causes {
177+
assert.Equal(t, cause.Field, fmt.Sprintf("spec.authentication.rules[%d]", i), "%#v", cause)
178+
assert.Assert(t, cmp.Regexp(`cannot use .+? options with .+? options`, cause.Message))
179+
}
180+
181+
// These combinations are allowed.
182+
183+
cluster.Spec.Authentication = nil
184+
require.UnmarshalInto(t, &cluster.Spec.Authentication, `{
185+
rules: [
186+
{ connection: hostssl, method: ldap, options: { ldapprefix: one, ldapsuffix: two } },
187+
{ connection: hostssl, method: ldap, options: { ldapbasedn: one, ldapbinddn: two } },
188+
{ connection: hostssl, method: ldap, options: {
189+
ldapbasedn: one, ldapsearchattribute: two, ldapsearchfilter: three,
190+
} },
191+
],
192+
}`)
193+
assert.NilError(t, cc.Create(ctx, cluster, client.DryRunAll))
194+
})
195+
})
196+
197+
t.Run("RADIUS", func(t *testing.T) {
198+
t.Run("Required", func(t *testing.T) {
199+
cluster := base.DeepCopy()
200+
require.UnmarshalInto(t, &cluster.Spec.Authentication, `{
201+
rules: [
202+
{ connection: hostssl, method: radius },
203+
{ connection: hostssl, method: radius, options: {} },
204+
{ connection: hostssl, method: radius, options: { radiusidentifiers: any } },
205+
{ connection: hostssl, method: radius, options: { radiusservers: any } },
206+
{ connection: hostssl, method: radius, options: { radiussecrets: any } },
207+
],
208+
}`)
209+
210+
err := cc.Create(ctx, cluster, client.DryRunAll)
211+
assert.Assert(t, apierrors.IsInvalid(err))
212+
213+
status := require.StatusError(t, err)
214+
assert.Assert(t, status.Details != nil)
215+
assert.Assert(t, cmp.Len(status.Details.Causes, 5))
216+
217+
for i, cause := range status.Details.Causes {
218+
assert.Equal(t, cause.Field, fmt.Sprintf("spec.authentication.rules[%d]", i), "%#v", cause)
219+
assert.Assert(t, cmp.Contains(cause.Message, `"radius" method requires`))
220+
}
221+
222+
// These are valid.
223+
224+
cluster.Spec.Authentication = nil
225+
require.UnmarshalInto(t, &cluster.Spec.Authentication, `{
226+
rules: [
227+
{ connection: hostssl, method: radius, options: { radiusservers: one, radiussecrets: two } },
228+
{ connection: hostssl, method: radius, options: {
229+
radiusservers: one, radiussecrets: two, radiusports: three,
230+
} },
231+
],
232+
}`)
233+
assert.NilError(t, cc.Create(ctx, cluster, client.DryRunAll))
234+
})
235+
})
121236
}
122237

123238
func TestPostgresConfigParameters(t *testing.T) {

‎pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgres_types.go

+13-2
Original file line numberDiff line numberDiff line change
@@ -124,8 +124,19 @@ type PostgresHBARule struct {
124124

125125
// ---
126126
// Emulate OpenAPI "anyOf" aka Kubernetes union.
127-
// +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`
128-
// +kubebuilder:validation:XValidation:rule=`has(self.hba) ? true : has(self.connection) && has(self.method)`,message=`"connection" and "method" are required`
127+
// +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`
128+
// +kubebuilder:validation:XValidation:rule=`has(self.hba) || (has(self.connection) && has(self.method))`,message=`"connection" and "method" are required`
129+
//
130+
// Some authentication methods *must* be further configured via options.
131+
//
132+
// https://git.postgresql.org/gitweb/?p=postgresql.git;hb=refs/tags/REL_10_0;f=src/backend/libpq/hba.c#l1501
133+
// https://git.postgresql.org/gitweb/?p=postgresql.git;hb=refs/tags/REL_17_0;f=src/backend/libpq/hba.c#l1886
134+
// +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`
135+
// +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`
136+
//
137+
// https://git.postgresql.org/gitweb/?p=postgresql.git;hb=refs/tags/REL_10_0;f=src/backend/libpq/hba.c#l1539
138+
// https://git.postgresql.org/gitweb/?p=postgresql.git;hb=refs/tags/REL_17_0;f=src/backend/libpq/hba.c#l1945
139+
// +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`
129140
//
130141
// +structType=atomic
131142
type PostgresHBARuleSpec struct {

0 commit comments

Comments
 (0)