On Sun, Sep 21, 2014 at 02:31:15AM -0400, Noah Misch wrote:
> It then dawned on me that every Windows build of PostgreSQL already has a way
> to limit connections to a particular OS user. SSPI authentication is
> essentially the Windows equivalent of peer authentication. A brief trial
> thereof looked promising. Regression runs will need a pg_ident.conf listing
> each role used in the regression tests. That's not ideal, but the buildfarm
> will quickly reveal any omissions. Unless someone sees a problem here, I will
> look at fleshing this out into a complete patch. I bet it will even turn out
> to be back-patchable.
That worked out nicely. "pg_regress --temp-install" rewrites pg_ident.conf
and pg_hba.conf such that the current OS user may authenticate as the
bootstrap superuser and as any user named in --create-role. Suites not using
--temp-install (pg_upgrade, TAP) call "pg_regress --config-auth=DATADIR" to
pick up those same configuration changes. My hope is that out-of-tree test
harnesses wanting this hardening can do likewise. On non-Windows systems,
"pg_regress --config-auth" does nothing.
The TAP suite did not and does not succeed on Windows. I have good confidence
in my changes to make it use SSPI, but I tested them fully on GNU/Linux only.
Adding the explicit PGHOST=localhost to the pg_upgrade test suite is necessary
to avoid the "host name must be specified" error under SSPI authentication. I
tentatively view that as a bug in libpq, but it's orthogonal to this patch.
pg_regress.c already sets PGHOST explicitly.
Since I was rewriting various test suite "initdb" calls anyway, I made a few
use "-N" that weren't using it previously.
Thanks,
nm
diff --git a/contrib/pg_upgrade/test.sh b/contrib/pg_upgrade/test.sh
index 7bbd2c7..3bda790 100644
--- a/contrib/pg_upgrade/test.sh
+++ b/contrib/pg_upgrade/test.sh
@@ -17,13 +17,20 @@ set -e
unset MAKEFLAGS
unset MAKELEVEL
+# Run a given "initdb" binary and overlay the regression testing
+# authentication configuration.
+standard_initdb() {
+ "$1" -N
+ ../../src/test/regress/pg_regress --config-auth "$PGDATA"
+}
+
# Establish how the server will listen for connections
testhost=`uname -s`
case $testhost in
MINGW*)
LISTEN_ADDRESSES="localhost"
- PGHOST=""; unset PGHOST
+ PGHOST=localhost
;;
*)
LISTEN_ADDRESSES=""
@@ -49,11 +56,11 @@ case $testhost in
trap 'rm -rf "$PGHOST"' 0
trap 'exit 3' 1 2 13 15
fi
- export PGHOST
;;
esac
POSTMASTER_OPTS="-F -c listen_addresses=$LISTEN_ADDRESSES -k \"$PGHOST\""
+export PGHOST
temp_root=$PWD/tmp_check
@@ -141,7 +148,7 @@ export EXTRA_REGRESS_OPTS
# enable echo so the user can see what is being executed
set -x
-"$oldbindir"/initdb -N
+standard_initdb "$oldbindir"/initdb
"$oldbindir"/pg_ctl start -l "$logdir/postmaster1.log" -o "$POSTMASTER_OPTS" -w
if "$MAKE" -C "$oldsrc" installcheck; then
pg_dumpall -f "$temp_root"/dump1.sql || pg_dumpall1_status=$?
@@ -181,7 +188,7 @@ fi
PGDATA=$BASE_PGDATA
-initdb -N
+standard_initdb 'initdb'
pg_upgrade $PG_UPGRADE_OPTS -d "${PGDATA}.old" -D "${PGDATA}" -b "$oldbindir"
-B "$bindir" -p "$PGPORT" -P "$PGPORT"
diff --git a/doc/src/sgml/regress.sgml b/doc/src/sgml/regress.sgml
index 71196a1..504d8da 100644
--- a/doc/src/sgml/regress.sgml
+++ b/doc/src/sgml/regress.sgml
@@ -56,19 +56,6 @@ make check
<quote>failure</> represents a serious problem.
</para>
- <warning>
- <para>
- On systems lacking Unix-domain sockets, notably Windows, this test method
- starts a temporary server configured to accept any connection originating
- on the local machine. Any local user can gain database superuser
- privileges when connecting to this server, and could in principle exploit
- all privileges of the operating-system user running the tests. Therefore,
- it is not recommended that you use <literal>make check</> on an affected
- system shared with untrusted users. Instead, run the tests after
- completing the installation, as described in the next section.
- </para>
- </warning>
-
<para>
Because this test method runs a temporary server, it will not work
if you did the build as the root user, since the server will not start as
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 63ff50b..2743c8f 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -320,7 +320,7 @@ endef
define prove_check
$(MKDIR_P) tmp_check/log
$(MAKE) -C $(top_builddir) DESTDIR='$(CURDIR)'/tmp_check/install install
>'$(CURDIR)'/tmp_check/log/install.log 2>&1
-cd $(srcdir) && TESTDIR='$(CURDIR)'
PATH="$(CURDIR)/tmp_check/install$(bindir):$$PATH" $(call
add_to_path,$(ld_library_path_var),$(CURDIR)/tmp_check/install$(libdir))
PGPORT='6$(DEF_PGPORT)' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) t/*.pl
+cd $(srcdir) && TESTDIR='$(CURDIR)'
PATH="$(CURDIR)/tmp_check/install$(bindir):$$PATH" $(call
add_to_path,$(ld_library_path_var),$(CURDIR)/tmp_check/install$(libdir))
top_srcdir='$(top_srcdir)' PGPORT='6$(DEF_PGPORT)' $(PROVE) $(PG_PROVE_FLAGS)
$(PROVE_FLAGS) t/*.pl
endef
else
diff --git a/src/bin/pg_ctl/t/001_start_stop.pl
b/src/bin/pg_ctl/t/001_start_stop.pl
index 5a95ebd..fb3e7a0 100644
--- a/src/bin/pg_ctl/t/001_start_stop.pl
+++ b/src/bin/pg_ctl/t/001_start_stop.pl
@@ -1,7 +1,7 @@
use strict;
use warnings;
use TestLib;
-use Test::More tests => 16;
+use Test::More tests => 17;
my $tempdir = TestLib::tempdir;
my $tempdir_short = TestLib::tempdir_short;
@@ -14,6 +14,10 @@ command_exit_is([ 'pg_ctl', 'start', '-D',
"$tempdir/nonexistent" ],
1, 'pg_ctl start with nonexistent directory');
command_ok([ 'pg_ctl', 'initdb', '-D', "$tempdir/data" ], 'pg_ctl initdb');
+command_ok(
+ [ "$ENV{top_srcdir}/src/test/regress/pg_regress", '--config-auth',
+ "$tempdir/data" ],
+ 'configure authentication');
open CONF, ">>$tempdir/data/postgresql.conf";
print CONF "listen_addresses = ''\n";
print CONF "unix_socket_directories = '$tempdir_short'\n";
diff --git a/src/bin/pg_ctl/t/002_status.pl b/src/bin/pg_ctl/t/002_status.pl
index 9502b6f..b8cbbda 100644
--- a/src/bin/pg_ctl/t/002_status.pl
+++ b/src/bin/pg_ctl/t/002_status.pl
@@ -9,7 +9,7 @@ my $tempdir_short = TestLib::tempdir_short;
command_exit_is([ 'pg_ctl', 'status', '-D', "$tempdir/nonexistent" ],
4, 'pg_ctl status with nonexistent directory');
-system_or_bail "initdb -D '$tempdir'/data -A trust >/dev/null";
+standard_initdb "$tempdir/data";
open CONF, ">>$tempdir/data/postgresql.conf";
print CONF "listen_addresses = ''\n";
print CONF "unix_socket_directories = '$tempdir_short'\n";
diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index 46a8bec..57abdb9 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -7,6 +7,7 @@ use Exporter 'import';
our @EXPORT = qw(
tempdir
tempdir_short
+ standard_initdb
start_test_server
restart_test_server
psql
@@ -69,6 +70,14 @@ sub tempdir_short
return File::Temp::tempdir(CLEANUP => 1);
}
+sub standard_initdb
+{
+ my $pgdata = shift;
+ system_or_bail("initdb -D '$pgdata' -A trust -N >/dev/null");
+ system_or_bail("$ENV{top_srcdir}/src/test/regress/pg_regress",
+ '--config-auth', $pgdata);
+}
+
my ($test_server_datadir, $test_server_logfile);
sub start_test_server
@@ -78,7 +87,7 @@ sub start_test_server
my $tempdir_short = tempdir_short;
- system "initdb -D '$tempdir'/pgdata -A trust -N >/dev/null";
+ standard_initdb "$tempdir/pgdata";
$ret = system 'pg_ctl', '-D', "$tempdir/pgdata", '-s', '-w', '-l',
"$tempdir/logfile", '-o',
"--fsync=off -k $tempdir_short --listen-addresses=''
--log-statement=all",
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index 27c46ab..cb092f9 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -29,6 +29,7 @@
#include <sys/resource.h>
#endif
+#include "common/username.h"
#include "getopt_long.h"
#include "libpq/pqcomm.h" /* needed for UNIXSOCK_PATH() */
#include "pg_config_paths.h"
@@ -104,6 +105,7 @@ static char *dlpath = PKGLIBDIR;
static char *user = NULL;
static _stringlist *extraroles = NULL;
static _stringlist *extra_install = NULL;
+static char *config_auth_datadir = NULL;
/* internal variables */
static const char *progname;
@@ -965,6 +967,150 @@ initialize_environment(void)
load_resultmap();
}
+#ifdef ENABLE_SSPI
+/*
+ * Get account and domain/realm names for the current user. This is based on
+ * pg_SSPI_recvauth(). The returned strings use static storage.
+ */
+static void
+current_windows_user(const char **acct, const char **dom)
+{
+ static char accountname[MAXPGPATH];
+ static char domainname[MAXPGPATH];
+ HANDLE token;
+ TOKEN_USER *tokenuser;
+ DWORD retlen;
+ DWORD accountnamesize = sizeof(accountname);
+ DWORD domainnamesize = sizeof(domainname);
+ SID_NAME_USE accountnameuse;
+
+ if (!OpenProcessToken(GetCurrentProcess(), TOKEN_READ, &token))
+ {
+ fprintf(stderr,
+ _("%s: could not open process token: error code
%lu\n"),
+ progname, GetLastError());
+ exit(2);
+ }
+
+ if (!GetTokenInformation(token, TokenUser, NULL, 0, &retlen) &&
GetLastError() != 122)
+ {
+ fprintf(stderr,
+ _("%s: could not get token user size: error
code %lu\n"),
+ progname, GetLastError());
+ exit(2);
+ }
+ tokenuser = malloc(retlen);
+ if (!GetTokenInformation(token, TokenUser, tokenuser, retlen, &retlen))
+ {
+ fprintf(stderr,
+ _("%s: could not get token user: error code
%lu\n"),
+ progname, GetLastError());
+ exit(2);
+ }
+
+ if (!LookupAccountSid(NULL, tokenuser->User.Sid, accountname,
&accountnamesize,
+ domainname, &domainnamesize,
&accountnameuse))
+ {
+ fprintf(stderr,
+ _("%s: could not look up account SID: error
code %lu\n"),
+ progname, GetLastError());
+ exit(2);
+ }
+
+ free(tokenuser);
+
+ *acct = accountname;
+ *dom = domainname;
+}
+
+/*
+ * 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.
+ */
+static void
+config_sspi_auth(const char *pgdata)
+{
+ const char *accountname,
+ *domainname;
+ const char *username;
+ char *errstr;
+ char fname[MAXPGPATH];
+ int res;
+ FILE *hba,
+ *ident;
+ _stringlist *sl;
+
+ /*
+ * "username", the initdb-chosen bootstrap superuser name, may always
+ * match "accountname", the value SSPI authentication discovers. The
+ * underlying system functions do not clearly guarantee that.
+ */
+ current_windows_user(&accountname, &domainname);
+ username = get_user_name(&errstr);
+ if (username == NULL)
+ {
+ fprintf(stderr, "%s: %s\n", progname, errstr);
+ exit(2);
+ }
+
+ /* Check a Write outcome and report any error. */
+#define CW(cond) \
+ do { \
+ if (!(cond)) \
+ { \
+ fprintf(stderr, _("%s: could not write to file \"%s\":
%s\n"), \
+ progname, fname, strerror(errno)); \
+ exit(2); \
+ } \
+ } while (0)
+
+ res = snprintf(fname, sizeof(fname), "%s/pg_hba.conf", pgdata);
+ if (res < 0 || res >= sizeof(fname) - 1)
+ {
+ /*
+ * Truncating this name is a fatal error, because we must not
fail to
+ * overwrite an original trust-authentication pg_hba.conf.
+ */
+ fprintf(stderr, _("%s: directory name too long\n"), progname);
+ exit(2);
+ }
+ hba = fopen(fname, "w");
+ if (hba == NULL)
+ {
+ fprintf(stderr, _("%s: could not open file \"%s\" for writing:
%s\n"),
+ progname, fname, strerror(errno));
+ exit(2);
+ }
+ CW(fputs("# Configuration written by config_sspi_auth()\n", hba) >= 0);
+ CW(fputs("host all all 127.0.0.1/32 sspi include_realm=1
map=regress\n",
+ hba) >= 0);
+ CW(fclose(hba) == 0);
+
+ snprintf(fname, sizeof(fname), "%s/pg_ident.conf", pgdata);
+ ident = fopen(fname, "w");
+ if (ident == NULL)
+ {
+ fprintf(stderr, _("%s: could not open file \"%s\" for writing:
%s\n"),
+ progname, fname, strerror(errno));
+ exit(2);
+ }
+ CW(fputs("# Configuration written by config_sspi_auth()\n", ident) >=
0);
+
+ /*
+ * Double-quote for the benefit of account names containing whitespace
or
+ * '#'. Windows forbids the double-quote character itself, so don't
+ * bother escaping embedded double-quote characters.
+ */
+ CW(fprintf(ident, "regress \"%s@%s\" \"%s\"\n",
+ accountname, domainname, username) >= 0);
+ for (sl = extraroles; sl; sl = sl->next)
+ CW(fprintf(ident, "regress \"%s@%s\" \"%s\"\n",
+ accountname, domainname, sl->str) >= 0);
+ CW(fclose(ident) == 0);
+}
+#endif
+
/*
* Issue a command via psql, connecting to the specified database
*
@@ -1957,6 +2103,7 @@ help(void)
printf(_("Usage:\n %s [OPTION]... [EXTRA-TEST]...\n"), progname);
printf(_("\n"));
printf(_("Options:\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"));
@@ -2023,6 +2170,7 @@ regression_main(int argc, char *argv[], init_function
ifunc, test_function tfunc
{"launcher", required_argument, NULL, 21},
{"load-extension", required_argument, NULL, 22},
{"extra-install", required_argument, NULL, 23},
+ {"config-auth", required_argument, NULL, 24},
{NULL, 0, NULL, 0}
};
@@ -2137,6 +2285,9 @@ regression_main(int argc, char *argv[], init_function
ifunc, test_function tfunc
case 23:
add_stringlist_item(&extra_install, optarg);
break;
+ case 24:
+ config_auth_datadir = pstrdup(optarg);
+ break;
default:
/* getopt_long already emitted a complaint */
fprintf(stderr, _("\nTry \"%s -h\" for more
information.\n"),
@@ -2154,6 +2305,14 @@ regression_main(int argc, char *argv[], init_function
ifunc, test_function tfunc
optind++;
}
+ if (config_auth_datadir)
+ {
+#ifdef ENABLE_SSPI
+ config_sspi_auth(config_auth_datadir);
+#endif
+ exit(0);
+ }
+
if (temp_install && !port_specified_by_user)
/*
@@ -2298,6 +2457,18 @@ regression_main(int argc, char *argv[], init_function
ifunc, test_function tfunc
fclose(pg_conf);
+#ifdef ENABLE_SSPI
+
+ /*
+ * Since we successfully used the same buffer for the
much-longer
+ * "initdb" command, this can't truncate.
+ */
+ snprintf(buf, sizeof(buf), "%s/data", temp_install);
+ config_sspi_auth(buf);
+#elif !defined(HAVE_UNIX_SOCKETS)
+#error Platform has no means to secure the test installation.
+#endif
+
/*
* Check if there is a postmaster running already.
*/
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index b84f70d..1b8fb54 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -247,6 +247,15 @@ sub contribcheck
exit $mstat if $mstat;
}
+# 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);
+}
+
sub upgradecheck
{
my $status;
@@ -258,6 +267,7 @@ sub upgradecheck
# i.e. only this version to this version check. That's
# what pg_upgrade's "make check" does.
+ $ENV{PGHOST} = 'localhost';
$ENV{PGPORT} ||= 50432;
my $tmp_root = "$topdir/contrib/pg_upgrade/tmp_check";
(mkdir $tmp_root || die $!) unless -d $tmp_root;
@@ -275,7 +285,7 @@ sub upgradecheck
my $logdir = "$topdir/contrib/pg_upgrade/log";
(mkdir $logdir || die $!) unless -d $logdir;
print "\nRunning initdb on old cluster\n\n";
- system("initdb") == 0 or exit 1;
+ standard_initdb() or exit 1;
print "\nStarting old cluster\n\n";
system("pg_ctl start -l $logdir/postmaster1.log -w") == 0 or exit 1;
print "\nSetting up data for upgrading\n\n";
@@ -289,7 +299,7 @@ sub upgradecheck
system("pg_ctl -m fast stop") == 0 or exit 1;
$ENV{PGDATA} = "$data";
print "\nSetting up new cluster\n\n";
- system("initdb") == 0 or exit 1;
+ standard_initdb() or exit 1;
print "\nRunning pg_upgrade\n\n";
system("pg_upgrade -d $data.old -D $data -b $bindir -B $bindir") == 0
or exit 1;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
