Re: Add PAM Tests and Option For Custom PAM Config Location

Hi Andrew,

I reviewed the PgBouncer counterpart in #1572 [0]. This review
covers the September 4 PostgreSQL patch against d39fda1cc404.
I built with Meson 1.3.2 and PAM 1.5.3 on Linux aarch64. Five
of the 26 PAM assertions failed.

In src/test/pam/t/001_auth.pl:

> my $test_pam_exec_log = "${PostgreSQL::Test::Utils::tmp_check}/log/pam_exec.log";

Meson puts tmp_check under data/ and logs under a sibling log/
directory. The data/log/ directory does not exist. The pam_exec
cases fail with:

/bin/sh failed: exit code 2

Please use "${PostgreSQL::Test::Utils::log_path}/pam_exec.log".
I tested that change in a temporary copy. All 26 assertions passed.

Commands:

meson setup /tmp/pg-pam-build . \
-Dpam=enabled -Dtap_tests=enabled
PG_TEST_EXTRA=pam meson test -C /tmp/pg-pam-build \
--suite setup --suite pam --print-errorlogs

In src/backend/libpq/hba.c:

> + hbaline->pamconfdir = pstrdup(val);

Please add pamconfdir to get_hba_options() in hbafuncs.c.
The pg_hba_file_rules.options column omits this setting.
Please add a test for its output.

In src/test/pam/t/001_auth.pl:

> elsif (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bpam\b/)

The CI workflow omits pam from PG_TEST_EXTRA. Its Linux Autoconf
configuration includes --with-pam, but this gate skips the tests.
Please enable pam for the PAM-capable CI jobs.

In meson.build:

> + ['pam_start_confdir', {'dependencies': [pam]}],

The func_checks loop adds pam to os_deps after a successful probe.
Frontend tools and libpq then inherit PAM link arguments.
Please put this check in the PAM detection block without adding
pam to os_deps.

Please add pam to the supported PG_TEST_EXTRA values in
doc/src/sgml/regress.sgml. The test README covers it, but the
manual omits it.

Nits:

meson.build:
> + pam_header_fp = 'security/pam_appl.h'
> + pam_header_fp = 'pam/pam_appl.h'

Unused.

src/backend/libpq/hba.c:46-54, PAM includes:

Unused.

src/test/pam/t/001_auth.pl:
> use FindBin;
> use lib "$FindBin::RealBin/..";
> use File::Copy;
> use File::Basename;

Unused.

src/test/pam/t/001_auth.pl, descriptions:

pam_accept.so -> pam_permit.so
succeds -> succeeds
test5: postgresql5 -> postgresql4
test6: Account rejection.

src/backend/libpq/auth.c:2081,2111,2119,2122:

Indentation.

src/test/pam/Makefile:26, src/test/pam/meson.build:16:

Extra EOF blank.

doc/src/sgml/client-auth.sgml:

pam -> PAM
undesitable -> undesirable
note that note that -> note that

src/test/pam/README:

greatr -> greater
as well setting -> as well as setting

[0] https://github.com/pgbouncer/pgbouncer/pull/1572

Brian