Skip to content

Conversation

@cbandy
Copy link
Member

@cbandy cbandy commented Mar 20, 2025

Checklist:

  • Have you added an explanation of what your changes do and why you'd like them to be included?
  • Have you updated or added documentation for the change, as applicable?
  • Have you tested your changes on all related environments with successful results, as applicable?
    • Have you added automated tests?

Type of Changes:

  • New feature

What is the current behavior (link to any open issues here)?

Postgres requires a few options on LDAP HBA rules. You can forget to add one and not realize until reading logs.

What is the new behavior (if this is a feature change)?

Validation rules ensure that required options are present.

Other Information:

Issue: PGO-2263

// 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`
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 I'm not liking these all-on-one-long-line markers these days.

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I understand this to say that either we have hba or we have ldap with ldap_ options? Is this another way to say that?

Suggested change
rule: has(self.hba) || self.method != "ldap" || (has(self.options)
rule: has(self.hba) || self.method == "ldap" && (has(self.options)

The "A or not B or C (required by B)" feels a little harder to read for me, but am I missing something?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every rule must evaluate to true. self.method == "ldap" rejects all other methods.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read these as "valid when (1) has hba or (2) isn't ldap or (3) [ldap stuff]"

Copy link
Member Author

@cbandy cbandy Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have hba or we have ldap with ldap options

Nope. This is saying:

when [we don't have hba] and [the method is ldap],
then [options must be] and [options must blah blah];
otherwise [ok]

But that ☝🏻 "otherwise" is not intended to be "otherwise [the whole thing]." So, it's spelled:

when [we have hba] or [the method isn't ldap], then [ok];
otherwise, [options must be] and [options must blah blah].

@cbandy cbandy merged commit 797fdf1 into CrunchyData:main Mar 21, 2025
19 checks passed
@cbandy cbandy deleted the hba-validation branch March 21, 2025 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants