On 2022-12-01 Th 21:10, Andres Freund wrote:
> Hi,
>
> On 2022-12-01 20:56:18 -0500, Tom Lane wrote:
>> Andres Freund <and...@anarazel.de> writes:
>>> On 2022-12-01 20:30:36 -0500, Tom Lane wrote:
>>>> If we remove that, won't we have a whole lot of code that's not
>>>> tested at all on any platform, ie all the TCP-socket code?
>>> There's some coverage via the auth and ssl tests. But I agree it's an
>>> issue. But to me the fix for that seems to be to add a dedicated test for
>>> that, rather than relying on windows to test our socket code - that's quite
>>> a
>>> few separate code paths from the tcp support of other platforms.
>> IMO that's not the best way forward, because you'll always have
>> nagging questions about whether a single-purpose test covers
>> everything that needs coverage.
> Still seems better than not having any coverage in our development
> environments...
>
>
>> I think the best place to be in would be to be able to run the whole test
>> suite using either TCP or UNIX sockets, on any platform (with stuff like the
>> SSL test overriding the choice as needed).
> I agree that that's useful. But it seems somewhat independent from the
> majority of the proposed changes. To be able to test force-tcp-everywhere we
> don't need e.g. code for setting sspi auth in pg_regress etc - it's afaik
> just needed so there's a secure way of running tests at all on windows.
>
> I think 0003 should be "trimmed" to only change the default for
> $use_unix_sockets on windows and to remove PG_TEST_USE_UNIX_SOCKETS. Whoever
> wants to, can then add a new environment variable to force tap tests to use
> tcp.
>
Not sure if it's useful here, but a few months ago I prepared patches to
remove the config-auth option of pg_regress, which struck me as more
than odd, and replace it with a perl module. I didn't get around to
finishing them, but the patches as of then are attached.
I agree that having some switch that says "run everything with TCP" or
"run (almost) everything with Unix sockets" would be good.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
From 5c7fad30aa2db2d47d0cc3869f132ca9d14a8814 Mon Sep 17 00:00:00 2001
From: Andrew Dunstan <and...@dunslane.net>
Date: Tue, 26 Apr 2022 10:49:26 -0400
Subject: [PATCH 1/2] Do config_auth in perl code for TAP tests and
vcregress.pl.
That means there is no longer any need to call pg_regress --config-auth
to set up Windows authentication. Instead a simple perl function call
does the work.
---
contrib/basebackup_to_shell/t/001_basic.pl | 2 +-
src/bin/pg_basebackup/t/010_pg_basebackup.pl | 2 +-
src/bin/pg_ctl/t/001_start_stop.pl | 4 +-
src/bin/pg_dump/t/010_dump_connstr.pl | 16 ++-
src/bin/pg_rewind/t/RewindTest.pm | 2 +-
src/test/perl/PostgreSQL/Test/Cluster.pm | 7 +-
src/test/perl/PostgreSQL/Test/ConfigAuth.pm | 115 +++++++++++++++++++
src/test/recovery/t/001_stream_rep.pl | 2 +-
src/tools/msvc/vcregress.pl | 8 +-
9 files changed, 135 insertions(+), 23 deletions(-)
create mode 100644 src/test/perl/PostgreSQL/Test/ConfigAuth.pm
diff --git a/contrib/basebackup_to_shell/t/001_basic.pl b/contrib/basebackup_to_shell/t/001_basic.pl
index 350d42079a..b5de23b48f 100644
--- a/contrib/basebackup_to_shell/t/001_basic.pl
+++ b/contrib/basebackup_to_shell/t/001_basic.pl
@@ -21,7 +21,7 @@ my $node = PostgreSQL::Test::Cluster->new('primary');
# Make sure pg_hba.conf is set up to allow connections from backupuser.
# This is only needed on Windows machines that don't use UNIX sockets.
$node->init('allows_streaming' => 1,
- 'auth_extra' => [ '--create-role', 'backupuser' ]);
+ 'auth_extra' => [ extra_roles => 'backupuser' ]);
$node->append_conf('postgresql.conf',
"shared_preload_libraries = 'basebackup_to_shell'");
diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index 056fcf3597..a0f4fd7b92 100644
--- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
+++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
@@ -29,7 +29,7 @@ umask(0077);
# Initialize node without replication settings
$node->init(extra => ['--data-checksums'],
- auth_extra => [ '--create-role', 'backupuser' ]);
+ auth_extra => [ extra_roles => 'backupuser' ]);
$node->start;
my $pgdata = $node->data_dir;
diff --git a/src/bin/pg_ctl/t/001_start_stop.pl b/src/bin/pg_ctl/t/001_start_stop.pl
index fdffd76d99..c92b06ad85 100644
--- a/src/bin/pg_ctl/t/001_start_stop.pl
+++ b/src/bin/pg_ctl/t/001_start_stop.pl
@@ -4,6 +4,7 @@
use strict;
use warnings;
+use PostgreSQL::Test::ConfigAuth;
use PostgreSQL::Test::Cluster;
use PostgreSQL::Test::Utils;
use Test::More;
@@ -20,8 +21,7 @@ command_exit_is([ 'pg_ctl', 'start', '-D', "$tempdir/nonexistent" ],
command_ok([ 'pg_ctl', 'initdb', '-D', "$tempdir/data", '-o', '-N' ],
'pg_ctl initdb');
-command_ok([ $ENV{PG_REGRESS}, '--config-auth', "$tempdir/data" ],
- 'configure authentication');
+config_auth("$tempdir/data");
my $node_port = PostgreSQL::Test::Cluster::get_free_port();
open my $conf, '>>', "$tempdir/data/postgresql.conf";
print $conf "fsync = off\n";
diff --git a/src/bin/pg_dump/t/010_dump_connstr.pl b/src/bin/pg_dump/t/010_dump_connstr.pl
index 7a745ade0f..576dd18f3a 100644
--- a/src/bin/pg_dump/t/010_dump_connstr.pl
+++ b/src/bin/pg_dump/t/010_dump_connstr.pl
@@ -4,6 +4,7 @@
use strict;
use warnings;
+use PostgreSQL::Test::ConfigAuth;
use PostgreSQL::Test::Cluster;
use PostgreSQL::Test::Utils;
use Test::More;
@@ -52,13 +53,10 @@ $node->init(extra =>
[ '-U', $src_bootstrap_super, '--locale=C', '--encoding=LATIN1' ]);
# prep pg_hba.conf and pg_ident.conf
-$node->run_log(
- [
- $ENV{PG_REGRESS}, '--config-auth',
- $node->data_dir, '--user',
- $src_bootstrap_super, '--create-role',
- "$username1,$username2,$username3,$username4"
- ]);
+config_auth(
+ $node->data_dir,
+ user => $src_bootstrap_super,
+ extra_roles => "$username1,$username2,$username3,$username4");
$node->start;
my $backupdir = $node->backup_dir;
@@ -182,7 +180,7 @@ $envar_node->init(
extra =>
[ '-U', $dst_bootstrap_super, '--locale=C', '--encoding=LATIN1' ],
auth_extra =>
- [ '--user', $dst_bootstrap_super, '--create-role', $restore_super ]);
+ [ user => $dst_bootstrap_super, extra_roles => $restore_super ]);
$envar_node->start;
# make superuser for restore
@@ -209,7 +207,7 @@ $cmdline_node->init(
extra =>
[ '-U', $dst_bootstrap_super, '--locale=C', '--encoding=LATIN1' ],
auth_extra =>
- [ '--user', $dst_bootstrap_super, '--create-role', $restore_super ]);
+ [ user => $dst_bootstrap_super, extra_roles => $restore_super ]);
$cmdline_node->start;
$cmdline_node->run_log(
[ 'createuser', '-U', $dst_bootstrap_super, '-s', $restore_super ]);
diff --git a/src/bin/pg_rewind/t/RewindTest.pm b/src/bin/pg_rewind/t/RewindTest.pm
index 8fd1f4b9de..d4e146324e 100644
--- a/src/bin/pg_rewind/t/RewindTest.pm
+++ b/src/bin/pg_rewind/t/RewindTest.pm
@@ -123,7 +123,7 @@ sub setup_cluster
$node_primary->init(
allows_streaming => 1,
extra => $extra,
- auth_extra => [ '--create-role', 'rewind_user' ]);
+ auth_extra => [ extra_roles => 'rewind_user' ]);
# Set wal_keep_size to prevent WAL segment recycling after enforced
# checkpoints in the tests.
diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm
index 9a2ada0a10..c9c53d39e1 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -104,6 +104,7 @@ use PostgreSQL::Version;
use PostgreSQL::Test::RecursiveCopy;
use Socket;
use Test::More;
+use PostgreSQL::Test::ConfigAuth;
use PostgreSQL::Test::Utils ();
use Time::HiRes qw(usleep);
use Scalar::Util qw(blessed);
@@ -428,8 +429,7 @@ Initialize a new cluster for testing.
Authentication is set up so that only the current OS user can access the
cluster. On Unix, we use Unix domain socket connections, with the socket in
a directory that's only accessible to the current user to ensure that.
-On Windows, we use SSPI authentication to ensure the same (by pg_regress
---config-auth).
+On Windows, we use SSPI authentication to ensure the same (via config_auth()).
WAL archiving can be enabled on this node by passing the keyword parameter
has_archiving => 1. This is disabled by default.
@@ -461,8 +461,7 @@ sub init
PostgreSQL::Test::Utils::system_or_bail('initdb', '-D', $pgdata, '-A', 'trust', '-N',
@{ $params{extra} });
- PostgreSQL::Test::Utils::system_or_bail($ENV{PG_REGRESS}, '--config-auth', $pgdata,
- @{ $params{auth_extra} });
+ config_auth($pgdata, @{ $params{auth_extra} });
open my $conf, '>>', "$pgdata/postgresql.conf";
print $conf "\n# Added by PostgreSQL::Test::Cluster.pm\n";
diff --git a/src/test/perl/PostgreSQL/Test/ConfigAuth.pm b/src/test/perl/PostgreSQL/Test/ConfigAuth.pm
new file mode 100644
index 0000000000..6aee9d61a7
--- /dev/null
+++ b/src/test/perl/PostgreSQL/Test/ConfigAuth.pm
@@ -0,0 +1,115 @@
+# Copyright (c) 2022, PostgreSQL Global Development Group
+
+=pod
+
+=head1 NAME
+
+PostgreSQL::Test::ConfigAuth - module to set up SSPI auth for a data directory
+
+=head1 SYNOPSIS
+
+ use PostgreSQL::Test::ConfigAuth;
+
+ config_auth($datadir);
+ config_auth($datadir, user => $username);
+ config_auth($datadir, user => $username, extra_roles => "$role1,$role2");
+
+=head1 DESCRIPTION
+
+PostgreSQL::Test::ConfigAuth::config_auth() sets up SSPI authentication for a
+data directory. If called in a platform other than on Windows, or if
+PG_TEST_USE_UNIX_SOCKETS is true, it is a noop.
+
+=cut
+
+
+package PostgreSQL::Test::ConfigAuth;
+
+use strict;
+use warnings;
+
+use Config;
+use Exporter qw(import);
+use Socket qw(:addrinfo);
+
+our (@EXPORT, @EXPORT_OK, %EXPORT_TAGS);
+
+@EXPORT = qw(config_auth);
+%EXPORT_TAGS = ();
+@EXPORT_OK = ();
+
+sub _have_ipv6
+{
+ my %hints = (flags => AI_NUMERIC_HOST, family => AF_INET6);
+ my ($err, @res) = getaddrinfo("::1", undef, \%hints);
+ return $err ? 0 : 1;
+}
+
+=pod
+
+=head1 METHODS
+
+=over
+
+=item config_auth($datadir, %options);
+
+Set up SSPI authentication on Windows, noop elsewhere. Uses the DOMAIN and
+USERNAME from the environment as the authenticating user.
+
+=over
+
+=item user => $username
+
+The database superuser to authenticate as. The role should exist or be created
+before a connection is attempted. By default the environment variable USERNAME
+is used.
+
+=item extra_roles => "$role1,$role2,..."
+
+Comma separated list of roles that can also be authenticated as. These
+roles are not created - only authentication permission is added.
+
+=back
+
+=back
+
+=cut
+
+
+sub config_auth
+{
+ # only do this on Windows and if not using Unix sockets
+ return 1 unless $Config{osname} eq 'MSWin32' || $Config{osname} eq 'msys';
+ return 1 if $ENV{PG_TEST_USE_UNIX_SOCKETS};
+ my $datadir = shift;
+ my %params = @_;
+ my $username = $ENV{USERNAME};
+ my $domain = $ENV{USERDOMAIN};
+ die "missing username or domain" unless $username && $domain;
+ my $dbsuper = $params{user} || $username;
+ my $extras = $params{extra_roles};
+ my @extras = split(/,/,$extras) if $extras;
+ open (my $hba, '>', "$datadir/pg_hba.conf") ||
+ die "opening $datadir/pg_hba.conf: $!";
+ my $have_ip6 = _have_ipv6();
+ print $hba
+ "# Configuration written by PostgreSQL::Test::ConfigAuth\n",
+ "host all all 127.0.0.1/32 sspi include_realm=1 map=regress\n";
+ print $hba
+ "host all all ::1/128 sspi include_realm=1 map=regress\n"
+ if $have_ip6;
+ close($hba);
+ open (my $ident, '>', "$datadir/pg_ident.conf") ||
+ die "opening $datadir/pg_hba.conf: $!";
+ print $ident
+ "# Configuration written by PostgreSQL::Test::ConfigAuth\n",
+ qq{regress "$username@$domain" "$dbsuper"\n};
+ foreach my $role (@extras)
+ {
+ print $ident qq{regress "$username@$domain" "$role"\n};
+ }
+ close($ident);
+ return 1;
+}
+
+1;
diff --git a/src/test/recovery/t/001_stream_rep.pl b/src/test/recovery/t/001_stream_rep.pl
index 583ee87da8..366769033f 100644
--- a/src/test/recovery/t/001_stream_rep.pl
+++ b/src/test/recovery/t/001_stream_rep.pl
@@ -14,7 +14,7 @@ my $node_primary = PostgreSQL::Test::Cluster->new('primary');
# and it needs proper authentication configuration.
$node_primary->init(
allows_streaming => 1,
- auth_extra => [ '--create-role', 'repl_role' ]);
+ auth_extra => [ extra_roles => 'repl_role' ]);
$node_primary->start;
my $backup_name = 'my_backup';
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index fc7aa8b9a3..e8fb1d856d 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -18,6 +18,9 @@ use File::Path qw(rmtree);
use FindBin;
use lib $FindBin::RealBin;
+use lib "$Findbin::Realbin/../../test/perl";
+use PostgreSQL::Test::ConfigAuth;
+
use Install qw(Install);
my $startdir = getcwd();
@@ -473,10 +476,7 @@ sub recoverycheck
# Run "initdb", then reconfigure authentication.
sub standard_initdb
{
- return (
- system('initdb', '-N') == 0 and system(
- "$topdir/$Config/pg_regress/pg_regress", '--config-auth',
- $ENV{PGDATA}) == 0);
+ return (system('initdb', '-N') == 0 and config_auth($ENV{PGDATA}));
}
# This is similar to appendShellString(). Perl system(@args) bypasses
--
2.25.1
From f82b06ed1119bfcd6872458b3a55192098f5dabb Mon Sep 17 00:00:00 2001
From: Andrew Dunstan <and...@dunslane.net>
Date: Tue, 26 Apr 2022 11:53:22 -0400
Subject: [PATCH 2/2] Remove the --config-auth option for pg_regress.
This is now redundant since TAP tests now know how to do this on their
own. The code which does this for pg_regress itself is kept.
---
src/test/regress/pg_regress.c | 22 +---------------------
1 file changed, 1 insertion(+), 21 deletions(-)
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index 982801e029..589a02052f 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -94,7 +94,6 @@ static bool port_specified_by_user = false;
static char *dlpath = PKGLIBDIR;
static char *user = NULL;
static _stringlist *extraroles = NULL;
-static char *config_auth_datadir = NULL;
/* internal variables */
static const char *progname;
@@ -851,12 +850,7 @@ current_windows_user(const char **acct, const char **dom)
}
/*
- * Rewrite pg_hba.conf and pg_ident.conf to use SSPI authentication. Permit
- * the current OS user to authenticate as the bootstrap superuser and as any
- * user named in a --create-role option.
- *
- * In --config-auth mode, the --user switch can be used to specify the
- * bootstrap superuser's name, otherwise we assume it is the default.
+ * Rewrite pg_hba.conf and pg_ident.conf to use SSPI authentication.
*/
static void
config_sspi_auth(const char *pgdata, const char *superuser_name)
@@ -1983,7 +1977,6 @@ help(void)
printf(_("Options:\n"));
printf(_(" --bindir=BINPATH use BINPATH for programs that are run;\n"));
printf(_(" if empty, use PATH from the environment\n"));
- printf(_(" --config-auth=DATADIR update authentication settings for DATADIR\n"));
printf(_(" --create-role=ROLE create the specified role before testing\n"));
printf(_(" --dbname=DB use database DB (default \"regression\")\n"));
printf(_(" --debug turn on debug mode in programs that are run\n"));
@@ -2050,7 +2043,6 @@ regression_main(int argc, char *argv[],
{"use-existing", no_argument, NULL, 20},
{"launcher", required_argument, NULL, 21},
{"load-extension", required_argument, NULL, 22},
- {"config-auth", required_argument, NULL, 24},
{"max-concurrent-tests", required_argument, NULL, 25},
{NULL, 0, NULL, 0}
};
@@ -2175,9 +2167,6 @@ regression_main(int argc, char *argv[],
case 22:
add_stringlist_item(&loadextension, optarg);
break;
- case 24:
- config_auth_datadir = pg_strdup(optarg);
- break;
case 25:
max_concurrent_tests = atoi(optarg);
break;
@@ -2198,15 +2187,6 @@ regression_main(int argc, char *argv[],
optind++;
}
- if (config_auth_datadir)
- {
-#ifdef ENABLE_SSPI
- if (!use_unix_sockets)
- config_sspi_auth(config_auth_datadir, user);
-#endif
- exit(0);
- }
-
if (temp_instance && !port_specified_by_user)
/*
--
2.25.1
