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
