group-access-v10-03-group.patch

text/plain

Filename: group-access-v10-03-group.patch
Type: text/plain
Part: 2
Message: Re: PATCH: Configurable file mode mask

Patch

Format: format-patch
Series: patch v10-0003
Subject: Allow group access on PGDATA.
File+
doc/src/sgml/backup.sgml 5 1
doc/src/sgml/ref/initdb.sgml 19 0
doc/src/sgml/runtime.sgml 13 1
src/backend/postmaster/postmaster.c 18 11
src/bin/initdb/initdb.c 14 8
src/bin/initdb/t/001_initdb.pl 12 1
src/bin/pg_ctl/pg_ctl.c 4 0
src/bin/pg_ctl/t/001_start_stop.pl 23 1
src/bin/pg_resetwal/pg_resetwal.c 2 2
src/bin/pg_resetwal/t/001_basic.pl 7 2
src/bin/pg_rewind/pg_rewind.c 2 2
src/bin/pg_rewind/RewindTest.pm 6 3
src/bin/pg_rewind/t/002_databases.pl 4 2
src/bin/pg_upgrade/pg_upgrade.c 4 1
src/bin/pg_upgrade/test.sh 7 7
src/common/file_perm.c 48 0
src/common/Makefile 1 1
src/include/common/file_perm.h 26 5
src/test/perl/PostgresNode.pm 26 1
src/test/perl/TestLib.pm 25 0
src/tools/msvc/Mkvcbuild.pm 1 1
From 2c3f01a060e217aed284c5bed5b9377a198e0817 Mon Sep 17 00:00:00 2001
From: David Steele <david@pgmasters.net>
Date: Fri, 9 Mar 2018 13:45:50 -0500
Subject: [PATCH 3/3] Allow group access on PGDATA.

This includes backend changes, utility changes (initdb, pg_ctl, pg_upgrade, etc.) and tests for each utility.
---
 doc/src/sgml/backup.sgml             |  6 ++++-
 doc/src/sgml/ref/initdb.sgml         | 19 ++++++++++++++
 doc/src/sgml/runtime.sgml            | 14 ++++++++++-
 src/backend/postmaster/postmaster.c  | 29 +++++++++++++---------
 src/bin/initdb/initdb.c              | 22 +++++++++++------
 src/bin/initdb/t/001_initdb.pl       | 13 +++++++++-
 src/bin/pg_ctl/pg_ctl.c              |  4 +++
 src/bin/pg_ctl/t/001_start_stop.pl   | 24 +++++++++++++++++-
 src/bin/pg_resetwal/pg_resetwal.c    |  4 +--
 src/bin/pg_resetwal/t/001_basic.pl   |  9 +++++--
 src/bin/pg_rewind/RewindTest.pm      |  9 ++++---
 src/bin/pg_rewind/pg_rewind.c        |  4 +--
 src/bin/pg_rewind/t/002_databases.pl |  6 +++--
 src/bin/pg_upgrade/pg_upgrade.c      |  5 +++-
 src/bin/pg_upgrade/test.sh           | 14 +++++------
 src/common/Makefile                  |  2 +-
 src/common/file_perm.c               | 48 ++++++++++++++++++++++++++++++++++++
 src/include/common/file_perm.h       | 31 +++++++++++++++++++----
 src/test/perl/PostgresNode.pm        | 27 +++++++++++++++++++-
 src/test/perl/TestLib.pm             | 25 +++++++++++++++++++
 src/tools/msvc/Mkvcbuild.pm          |  2 +-
 21 files changed, 267 insertions(+), 50 deletions(-)
 create mode 100644 src/common/file_perm.c

diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml
index 9d8e69056f..a9912eb418 100644
--- a/doc/src/sgml/backup.sgml
+++ b/doc/src/sgml/backup.sgml
@@ -1095,7 +1095,11 @@ SELECT pg_stop_backup();
     and <filename>postmaster.opts</filename>, which record information
     about the running <application>postmaster</application>, not about the
     <application>postmaster</application> which will eventually use this backup.
-    (These files can confuse <application>pg_ctl</application>.)
+    (These files can confuse <application>pg_ctl</application>.)  If group read
+    access is enabled on the data directory and an unprivileged user in the
+    <productname>PostgreSQL</productname> group is performing the backup, then
+    <filename>postmaster.pid</filename> will not be readable and must be
+    excluded.
    </para>
 
    <para>
diff --git a/doc/src/sgml/ref/initdb.sgml b/doc/src/sgml/ref/initdb.sgml
index 585665f161..5f57b933b9 100644
--- a/doc/src/sgml/ref/initdb.sgml
+++ b/doc/src/sgml/ref/initdb.sgml
@@ -76,6 +76,14 @@ PostgreSQL documentation
    to do so.)
   </para>
 
+  <para>
+    For security reasons the new cluster created by <command>initdb</command>
+    will only be accessible by the cluster owner by default.  The
+    <option>--allow-group-access</option> option allows any user in the same
+    group as the cluster owner to read files in the cluster.  This is useful
+    for performing backups as a non-privileged user.
+  </para>
+
   <para>
    <command>initdb</command> initializes the database cluster's default
    locale and character set encoding. The character set encoding,
@@ -301,6 +309,17 @@ PostgreSQL documentation
       </listitem>
      </varlistentry>
 
+     <varlistentry>
+      <term><option>-g</option></term>
+      <term><option>--allow-group-access</option></term>
+      <listitem>
+       <para>
+        Allows users in the same group as the cluster owner to read all cluster
+        files created by <command>initdb</command>.
+       </para>
+      </listitem>
+     </varlistentry>
+
      <varlistentry>
       <term><option>-W</option></term>
       <term><option>--pwprompt</option></term>
diff --git a/doc/src/sgml/runtime.sgml b/doc/src/sgml/runtime.sgml
index 587b430527..40ce3d9813 100644
--- a/doc/src/sgml/runtime.sgml
+++ b/doc/src/sgml/runtime.sgml
@@ -137,7 +137,10 @@ postgres$ <userinput>initdb -D /usr/local/pgsql/data</userinput>
    database, it is essential that it be secured from unauthorized
    access. <command>initdb</command> therefore revokes access
    permissions from everyone but the
-   <productname>PostgreSQL</productname> user.
+   <productname>PostgreSQL</productname> user, and optionally, group.
+   Group access, when enabled, is read-only.  This allows an unprivileged
+   user in the <productname>PostgreSQL</productname> group to take a backup of
+   the cluster data or perform other operations that only require read access.
   </para>
 
   <para>
@@ -2194,6 +2197,15 @@ pg_dumpall -p 5432 | psql -d postgres -p 5433
    member of the group that has access to those certificate and key files.
   </para>
 
+  <para>
+    If the data directory allows group read access then certificate files may
+    need to be located outside of the data directory in order to conform to the
+    security requirements outlined above.  Generally, group access is enabled
+    to allow an unprivileged user to backup the database, and in that case the
+    backup software will not be able to read the certificate files and will
+    likely error.
+  </para>
+
   <para>
    If the private key is protected with a passphrase, the
    server will prompt for the passphrase and will not start until it has
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 956960fd4b..9ce5535327 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -97,6 +97,7 @@
 #include "access/xlog.h"
 #include "bootstrap/bootstrap.h"
 #include "catalog/pg_control.h"
+#include "common/file_perm.h"
 #include "common/ip.h"
 #include "lib/ilist.h"
 #include "libpq/auth.h"
@@ -587,7 +588,8 @@ PostmasterMain(int argc, char *argv[])
 	IsPostmasterEnvironment = true;
 
 	/*
-	 * for security, no dir or file created can be group or other accessible
+	 * By default, no dir or file created can be group or other accessible.  This
+	 * may be modified later depending in the permissions of the data directory.
 	 */
 	umask(S_IRWXG | S_IRWXO);
 
@@ -1521,25 +1523,30 @@ checkDataDir(void)
 #endif
 
 	/*
-	 * Check if the directory has group or world access.  If so, reject.
-	 *
-	 * It would be possible to allow weaker constraints (for example, allow
-	 * group access) but we cannot make a general assumption that that is
-	 * okay; for example there are platforms where nearly all users
-	 * customarily belong to the same group.  Perhaps this test should be
-	 * configurable.
+	 * Check if the directory has correct permissions.  If not, reject.
 	 *
 	 * XXX temporarily suppress check when on Windows, because there may not
 	 * be proper support for Unix-y file permissions.  Need to think of a
 	 * reasonable check to apply on Windows.
 	 */
 #if !defined(WIN32) && !defined(__CYGWIN__)
-	if (stat_buf.st_mode & (S_IRWXG | S_IRWXO))
+	if (stat_buf.st_mode & PG_MODE_MASK_ALLOW_GROUP)
 		ereport(FATAL,
 				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
-				 errmsg("data directory \"%s\" has group or world access",
+				 errmsg("data directory \"%s\" has invalid permissions",
 						DataDir),
-				 errdetail("Permissions should be u=rwx (0700).")));
+				 errdetail("Permissions should be u=rwx (0700) or u=rwx,g=rx (0750).")));
+#endif
+
+	/*
+	 * Reset the file mode creation mask based on the mode of the data
+	 * directory.
+	 *
+	 * Suppress when on Windows, because there may not be proper support
+	 * for Unix-y file permissions.
+	 */
+#if !defined(WIN32) && !defined(__CYGWIN__)
+	umask(PG_MODE_MASK_DEFAULT & ~stat_buf.st_mode);
 #endif
 
 	/* Look for PG_VERSION before looking for pg_control */
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 278e56bf95..f77b58bf74 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -145,6 +145,7 @@ static bool data_checksums = false;
 static char *xlog_dir = NULL;
 static char *str_wal_segment_size_mb = NULL;
 static int	wal_segment_size_mb;
+static mode_t file_mode_mask = PG_MODE_MASK_DEFAULT;
 
 
 /* internal vars */
@@ -1171,7 +1172,7 @@ setup_config(void)
 	snprintf(path, sizeof(path), "%s/postgresql.conf", pg_data);
 
 	writefile(path, conflines);
-	if (chmod(path, PG_FILE_MODE_DEFAULT) != 0)
+	if (chmod(path, PG_FILE_MODE_DEFAULT & ~file_mode_mask) != 0)
 	{
 		fprintf(stderr, _("%s: could not change permissions of \"%s\": %s\n"),
 				progname, path, strerror(errno));
@@ -1191,7 +1192,7 @@ setup_config(void)
 	sprintf(path, "%s/postgresql.auto.conf", pg_data);
 
 	writefile(path, autoconflines);
-	if (chmod(path, PG_FILE_MODE_DEFAULT) != 0)
+	if (chmod(path, PG_FILE_MODE_DEFAULT & ~file_mode_mask) != 0)
 	{
 		fprintf(stderr, _("%s: could not change permissions of \"%s\": %s\n"),
 				progname, path, strerror(errno));
@@ -1278,7 +1279,7 @@ setup_config(void)
 	snprintf(path, sizeof(path), "%s/pg_hba.conf", pg_data);
 
 	writefile(path, conflines);
-	if (chmod(path, PG_FILE_MODE_DEFAULT) != 0)
+	if (chmod(path, PG_FILE_MODE_DEFAULT & ~file_mode_mask) != 0)
 	{
 		fprintf(stderr, _("%s: could not change permissions of \"%s\": %s\n"),
 				progname, path, strerror(errno));
@@ -1294,7 +1295,7 @@ setup_config(void)
 	snprintf(path, sizeof(path), "%s/pg_ident.conf", pg_data);
 
 	writefile(path, conflines);
-	if (chmod(path, PG_FILE_MODE_DEFAULT) != 0)
+	if (chmod(path, PG_FILE_MODE_DEFAULT & ~file_mode_mask) != 0)
 	{
 		fprintf(stderr, _("%s: could not change permissions of \"%s\": %s\n"),
 				progname, path, strerror(errno));
@@ -2312,6 +2313,7 @@ usage(const char *progname)
 	printf(_("      --auth-local=METHOD   default authentication method for local-socket connections\n"));
 	printf(_(" [-D, --pgdata=]DATADIR     location for this database cluster\n"));
 	printf(_("  -E, --encoding=ENCODING   set default encoding for new databases\n"));
+	printf(_("  -g, --allow-group-access  allow group read/execute on data directory\n"));
 	printf(_("      --locale=LOCALE       set default locale for new databases\n"));
 	printf(_("      --lc-collate=, --lc-ctype=, --lc-messages=LOCALE\n"
 			 "      --lc-monetary=, --lc-numeric=, --lc-time=LOCALE\n"
@@ -2711,7 +2713,7 @@ create_data_directory(void)
 				   pg_data);
 			fflush(stdout);
 
-			if (chmod(pg_data, PG_DIR_MODE_DEFAULT) != 0)
+			if (chmod(pg_data, PG_DIR_MODE_DEFAULT & ~file_mode_mask) != 0)
 			{
 				fprintf(stderr, _("%s: could not change permissions of directory \"%s\": %s\n"),
 						progname, pg_data, strerror(errno));
@@ -2797,7 +2799,7 @@ create_xlog_or_symlink(void)
 					   xlog_dir);
 				fflush(stdout);
 
-				if (chmod(xlog_dir, PG_DIR_MODE_DEFAULT) != 0)
+				if (chmod(xlog_dir, PG_DIR_MODE_DEFAULT & ~file_mode_mask) != 0)
 				{
 					fprintf(stderr, _("%s: could not change permissions of directory \"%s\": %s\n"),
 							progname, xlog_dir, strerror(errno));
@@ -2884,7 +2886,7 @@ initialize_data_directory(void)
 	setup_signals();
 
 	/* Set the file mode creation mask */
-	umask(PG_MODE_MASK_DEFAULT);
+	umask(file_mode_mask);
 
 	create_data_directory();
 
@@ -3018,6 +3020,7 @@ main(int argc, char *argv[])
 		{"waldir", required_argument, NULL, 'X'},
 		{"wal-segsize", required_argument, NULL, 12},
 		{"data-checksums", no_argument, NULL, 'k'},
+		{"allow-group-access", no_argument, NULL, 'g'},
 		{NULL, 0, NULL, 0}
 	};
 
@@ -3059,7 +3062,7 @@ main(int argc, char *argv[])
 
 	/* process command-line options */
 
-	while ((c = getopt_long(argc, argv, "dD:E:kL:nNU:WA:sST:X:", long_options, &option_index)) != -1)
+	while ((c = getopt_long(argc, argv, "dD:E:kL:nNU:WA:sST:X:g", long_options, &option_index)) != -1)
 	{
 		switch (c)
 		{
@@ -3153,6 +3156,9 @@ main(int argc, char *argv[])
 			case 12:
 				str_wal_segment_size_mb = pg_strdup(optarg);
 				break;
+			case 'g':
+				file_mode_mask = PG_MODE_MASK_ALLOW_GROUP;
+				break;
 			default:
 				/* getopt_long already emitted a complaint */
 				fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
diff --git a/src/bin/initdb/t/001_initdb.pl b/src/bin/initdb/t/001_initdb.pl
index 3331560445..f08fc6ea8f 100644
--- a/src/bin/initdb/t/001_initdb.pl
+++ b/src/bin/initdb/t/001_initdb.pl
@@ -4,9 +4,11 @@
 
 use strict;
 use warnings;
+use Fcntl ':mode';
+use File::stat qw{lstat};
 use PostgresNode;
 use TestLib;
-use Test::More tests => 16;
+use Test::More tests => 18;
 
 my $tempdir = TestLib::tempdir;
 my $xlogdir = "$tempdir/pgxlog";
@@ -50,3 +52,12 @@ mkdir $datadir;
 }
 command_ok([ 'initdb', '-S', $datadir ], 'sync only');
 command_fails([ 'initdb', $datadir ], 'existing data directory');
+
+# Init a new db with group access
+my $datadir_group = "$tempdir/data_group";
+
+command_ok(
+	[ 'initdb', '-g', $datadir_group ],
+	'successful creation with group access');
+
+ok(check_mode_recursive($datadir_group, 0750, 0640));
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 1eef7179cc..bb70e625fd 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -2171,6 +2171,7 @@ main(int argc, char **argv)
 	 */
 	argv0 = argv[0];
 
+	/* Set restrictive mode mask until PGDATA permissions are checked */
 	umask(PG_MODE_MASK_DEFAULT);
 
 	/* support --help and --version even if invoked as root */
@@ -2406,6 +2407,9 @@ main(int argc, char **argv)
 		snprintf(version_file, MAXPGPATH, "%s/PG_VERSION", pg_data);
 		snprintf(pid_file, MAXPGPATH, "%s/postmaster.pid", pg_data);
 		snprintf(backup_file, MAXPGPATH, "%s/backup_label", pg_data);
+
+		/* Set mask based on PGDATA permissions */
+		umask(DataDirectoryMask(pg_data));
 	}
 
 	switch (ctl_command)
diff --git a/src/bin/pg_ctl/t/001_start_stop.pl b/src/bin/pg_ctl/t/001_start_stop.pl
index 3d82abe696..7318c27c85 100644
--- a/src/bin/pg_ctl/t/001_start_stop.pl
+++ b/src/bin/pg_ctl/t/001_start_stop.pl
@@ -2,9 +2,11 @@ use strict;
 use warnings;
 
 use Config;
+use Fcntl ':mode';
+use File::stat qw{lstat};
 use PostgresNode;
 use TestLib;
-use Test::More tests => 20;
+use Test::More tests => 24;
 
 my $tempdir       = TestLib::tempdir;
 my $tempdir_short = TestLib::tempdir_short;
@@ -68,4 +70,24 @@ command_ok(
 ok(-f $logFileName);
 ok(check_mode_recursive("$tempdir/data", 0700, 0600));
 
+# Log file for second perm test
+$logFileName = "$tempdir/data/perm-test-640.log";
+
+# Change the data dir mode so log file will be created with group read
+# privileges on the next start
+system_or_bail 'pg_ctl', 'stop', '-D', "$tempdir/data";
+
+chmod_recursive("$tempdir/data", 0750, 0640);
+
+command_ok(
+	[ 'pg_ctl', 'start', '-D', "$tempdir/data", '-l', $logFileName ],
+	'start server to check group permissions');
+
+ok(-f $logFileName);
+ok(check_mode_recursive("$tempdir/data", 0750, 0640, ['postmaster.pid']));
+
+command_ok(
+	[ 'pg_ctl', 'restart', '-D', "$tempdir/data", '-l', $logFileName ],
+	'pg_ctl restart with server running');
+
 system_or_bail 'pg_ctl', 'stop', '-D', "$tempdir/data";
diff --git a/src/bin/pg_resetwal/pg_resetwal.c b/src/bin/pg_resetwal/pg_resetwal.c
index 47abd41fc3..6a4dc502c4 100644
--- a/src/bin/pg_resetwal/pg_resetwal.c
+++ b/src/bin/pg_resetwal/pg_resetwal.c
@@ -328,8 +328,8 @@ main(int argc, char *argv[])
 		exit(1);
 	}
 
-	/* Set dir/file mode mask */
-	umask(PG_MODE_MASK_DEFAULT);
+	/* Set mask based on PGDATA permissions */
+	umask(DataDirectoryMask(DataDir));
 
 	/* Check that data directory matches our server version */
 	CheckDataVersion();
diff --git a/src/bin/pg_resetwal/t/001_basic.pl b/src/bin/pg_resetwal/t/001_basic.pl
index 8f3f2990f3..8736130731 100644
--- a/src/bin/pg_resetwal/t/001_basic.pl
+++ b/src/bin/pg_resetwal/t/001_basic.pl
@@ -4,7 +4,7 @@ use warnings;
 use Config;
 use PostgresNode;
 use TestLib;
-use Test::More tests => 14;
+use Test::More tests => 15;
 
 my $tempdir       = TestLib::tempdir;
 my $tempdir_short = TestLib::tempdir_short;
@@ -40,9 +40,12 @@ ok(check_mode_recursive($pgdata, 0700, 0600), 'check PGDATA permissions');
 
 $node->start;
 
-# Reset to specific WAL segment
+# Reset to specific WAL segment.  Also enable group access to make sure files
+# and directories are created with group permissions.
 $node->stop;
 
+chmod_recursive($pgdata, 0750, 0640);
+
 $node->command_ok(
 	['pg_resetwal', '-l', '000000070000000700000007', '-D', $pgdata],
 	'set to specific WAL');
@@ -52,4 +55,6 @@ is_deeply(
 	[sort(qw(. .. archive_status 000000070000000700000007))],
 	'WAL recreated');
 
+ok(check_mode_recursive($pgdata, 0750, 0640), 'check PGDATA permissions');
+
 $node->start;
diff --git a/src/bin/pg_rewind/RewindTest.pm b/src/bin/pg_rewind/RewindTest.pm
index 1751de1a35..9dab6bd7e8 100644
--- a/src/bin/pg_rewind/RewindTest.pm
+++ b/src/bin/pg_rewind/RewindTest.pm
@@ -115,11 +115,13 @@ sub check_query
 
 sub setup_cluster
 {
-	my $extra_name = shift;
+	my $extra_name = shift;		# Used to differentiate clusters
+    my $extra = shift;			# Extra params for initdb
 
 	# Initialize master, data checksums are mandatory
 	$node_master = get_new_node('master' . ($extra_name ? "_${extra_name}" : ''));
-	$node_master->init(allows_streaming => 1);
+	$node_master->init(
+		allows_streaming => 1, extra => $extra);
 	# Set wal_keep_segments to prevent WAL segment recycling after enforced
 	# checkpoints in the tests.
 	$node_master->append_conf('postgresql.conf', qq(
@@ -237,7 +239,8 @@ sub run_pg_rewind
 		"$tmp_folder/master-postgresql.conf.tmp",
 		"$master_pgdata/postgresql.conf");
 
-	chmod(0600, "$master_pgdata/postgresql.conf")
+	chmod($node_master->group_access() ? 0640 : 0600,
+		  "$master_pgdata/postgresql.conf")
 		or die("unable to set permissions for $master_pgdata/postgresql.conf");
 
 	# Plug-in rewound node to the now-promoted standby node
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 3d179e2c7d..11153e5f2f 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -186,8 +186,8 @@ main(int argc, char **argv)
 		exit(1);
 	}
 
-	/* Set dir/file mode mask */
-	umask(PG_MODE_MASK_DEFAULT);
+	/* Set mask based on PGDATA permissions */
+	umask(DataDirectoryMask(datadir_target));
 
 	/*
 	 * Don't allow pg_rewind to be run as root, to avoid overwriting the
diff --git a/src/bin/pg_rewind/t/002_databases.pl b/src/bin/pg_rewind/t/002_databases.pl
index 37cdd712f3..570fa5cee0 100644
--- a/src/bin/pg_rewind/t/002_databases.pl
+++ b/src/bin/pg_rewind/t/002_databases.pl
@@ -1,7 +1,7 @@
 use strict;
 use warnings;
 use TestLib;
-use Test::More tests => 4;
+use Test::More tests => 6;
 
 use RewindTest;
 
@@ -9,7 +9,7 @@ sub run_test
 {
 	my $test_mode = shift;
 
-	RewindTest::setup_cluster($test_mode);
+	RewindTest::setup_cluster($test_mode, ['-g']);
 	RewindTest::start_master();
 
 	# Create a database in master.
@@ -42,6 +42,8 @@ template1
 ),
 		'database names');
 
+	ok (check_mode_recursive(
+		$node_master->data_dir(), 0750, 0640, ['postmaster.pid']));
 	RewindTest::clean_rewind_test();
 }
 
diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c
index 59df6fd88f..54f54fb0a4 100644
--- a/src/bin/pg_upgrade/pg_upgrade.c
+++ b/src/bin/pg_upgrade/pg_upgrade.c
@@ -79,7 +79,7 @@ main(int argc, char **argv)
 
 	set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_upgrade"));
 
-	/* Ensure that all files created by pg_upgrade are non-world-readable */
+	/* Set default restrictive mask until new cluster permissions are read */
 	umask(PG_MODE_MASK_DEFAULT);
 
 	parseCommandLine(argc, argv);
@@ -100,6 +100,9 @@ main(int argc, char **argv)
 
 	check_cluster_compatibility(live_check);
 
+	/* Set mask based on new PGDATA permissions */
+	umask(DataDirectoryMask(new_cluster.pgdata));
+
 	check_and_dump_old_cluster(live_check);
 
 
diff --git a/src/bin/pg_upgrade/test.sh b/src/bin/pg_upgrade/test.sh
index 4e8db4aaf4..24631a91c6 100644
--- a/src/bin/pg_upgrade/test.sh
+++ b/src/bin/pg_upgrade/test.sh
@@ -20,9 +20,9 @@ unset MAKELEVEL
 # Run a given "initdb" binary and overlay the regression testing
 # authentication configuration.
 standard_initdb() {
-	# To increase coverage of non-standard segment size without
-	# increase test runtime, run these tests with a lower setting.
-	"$1" -N --wal-segsize 1
+	# To increase coverage of non-standard segment size and group access
+	# without increasing test runtime, run these tests with a custom setting.
+	"$1" -N --wal-segsize 1 -g
 	if [ -n "$TEMP_CONFIG" -a -r "$TEMP_CONFIG" ]
 	then
 		cat "$TEMP_CONFIG" >> "$PGDATA/postgresql.conf"
@@ -231,13 +231,13 @@ standard_initdb 'initdb'
 pg_upgrade $PG_UPGRADE_OPTS -d "${PGDATA}.old" -D "${PGDATA}" -b "$oldbindir" -B "$bindir" -p "$PGPORT" -P "$PGPORT"
 
 # make sure all directories and files have group permissions
-if [ $(find ${PGDATA} -type f ! -perm 600 | wc -l) -ne 0 ]; then
-	echo "files in PGDATA with permission != 600";
+if [ $(find ${PGDATA} -type f ! -perm 640 | wc -l) -ne 0 ]; then
+	echo "files in PGDATA with permission != 640";
 	exit 1;
 fi
 
-if [ $(find ${PGDATA} -type d ! -perm 700 | wc -l) -ne 0 ]; then
-	echo "directories in PGDATA with permission != 700";
+if [ $(find ${PGDATA} -type d ! -perm 750 | wc -l) -ne 0 ]; then
+	echo "directories in PGDATA with permission != 750";
 	exit 1;
 fi
 
diff --git a/src/common/Makefile b/src/common/Makefile
index 80e78d72fe..504375bef4 100644
--- a/src/common/Makefile
+++ b/src/common/Makefile
@@ -51,7 +51,7 @@ else
 OBJS_COMMON += sha2.o
 endif
 
-OBJS_FRONTEND = $(OBJS_COMMON) fe_memutils.o file_utils.o restricted_token.o
+OBJS_FRONTEND = $(OBJS_COMMON) fe_memutils.o file_perm.o file_utils.o restricted_token.o
 
 OBJS_SRV = $(OBJS_COMMON:%.o=%_srv.o)
 
diff --git a/src/common/file_perm.c b/src/common/file_perm.c
new file mode 100644
index 0000000000..f01d43725e
--- /dev/null
+++ b/src/common/file_perm.c
@@ -0,0 +1,48 @@
+/*-------------------------------------------------------------------------
+ *
+ * File and directory permission routines
+ *
+ * Group read/execute may optional be enabled on PGDATA so any frontend tools
+ * That write into PGDATA must know what mask to set and the permissions to
+ * use for creating files and directories.
+ *
+ *
+ * Portions Copyright (c) 1996-2018, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/include/common/file_perm.c
+ *
+ *-------------------------------------------------------------------------
+ */
+
+#ifndef FRONTEND
+#error "This file is not expected to be compiled for backend code"
+#endif
+
+#include <sys/stat.h>
+
+#include "common/file_perm.h"
+
+/*
+ * Determine the mask to use when writing to PGDATA.
+ *
+ * Errors are not handled here and should be checked by the frontend
+ * application.
+ */
+mode_t
+DataDirectoryMask(const char *dataDir)
+{
+	struct stat statBuf;
+
+	/*
+	 * If an error occurs getting the mode then return the more restrictive mask.
+	 * It is the reponsibility of the frontend application to generate an error.
+	 */
+	if (stat(dataDir, &statBuf) != 0)
+		return PG_MODE_MASK_DEFAULT;
+
+	/*
+	 * Construct the mask that the caller should pass to umask().
+	 */
+	return PG_MODE_MASK_DEFAULT & ~statBuf.st_mode;
+}
diff --git a/src/include/common/file_perm.h b/src/include/common/file_perm.h
index 3c6da0e000..35dcbe6108 100644
--- a/src/include/common/file_perm.h
+++ b/src/include/common/file_perm.h
@@ -1,6 +1,10 @@
 /*-------------------------------------------------------------------------
  *
- * File and directory permission constants
+ * File and directory permission constants and routines
+ *
+ * Group read/execute may optional be enabled on PGDATA so any frontend tools
+ * That write into PGDATA must know what mask to set and the permissions to
+ * use for creating files and directories.
  *
  *
  * Portions Copyright (c) 1996-2018, PostgreSQL Global Development Group
@@ -20,13 +24,30 @@
 #define PG_MODE_MASK_DEFAULT		(S_IRWXG | S_IRWXO)
 
 /*
- * Default mode for created files.
+ * Optional mode mask for data directory permissions that allows group
+ * read/execute.
+ */
+#define PG_MODE_MASK_ALLOW_GROUP	(S_IWGRP | S_IRWXO)
+
+/*
+ * Default mode for created files, unless something else is specified using
+ * the *Perm() function variants.
+ */
+#define PG_FILE_MODE_DEFAULT		(S_IRUSR | S_IWUSR | S_IRGRP)
+
+/*
+ * Default mode for directories created with MakeDirectoryDefaultPerm().
  */
-#define PG_FILE_MODE_DEFAULT		(S_IRUSR | S_IWUSR)
+#define PG_DIR_MODE_DEFAULT			(S_IRWXU | S_IRGRP | S_IXGRP)
+
+#ifdef FRONTEND
 
 /*
- * Default mode for directories.
+ * Determine the mask to use when writing to PGDATA
  */
-#define PG_DIR_MODE_DEFAULT			S_IRWXU
+mode_t DataDirectoryMask(const char *dataDir);
+
+#endif							/* FRONTEND */
+
 
 #endif							/* FILE_PERM_H */
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 76e571b98c..1bd91524d7 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -86,9 +86,11 @@ use Carp;
 use Config;
 use Cwd;
 use Exporter 'import';
+use Fcntl qw(:mode);
 use File::Basename;
 use File::Path qw(rmtree);
 use File::Spec;
+use File::stat qw(stat);
 use File::Temp ();
 use IPC::Run;
 use RecursiveCopy;
@@ -269,6 +271,26 @@ sub connstr
 
 =pod
 
+=item $node->group_access()
+
+Does the data dir allow group access?
+
+=cut
+
+sub group_access
+{
+	my ($self) = @_;
+
+	my $dir_stat = stat($self->data_dir);
+
+	defined($dir_stat)
+		or die('unable to stat ' . $self->data_dir);
+
+	return (S_IMODE($dir_stat->mode) == 0750);
+}
+
+=pod
+
 =item $node->data_dir()
 
 Returns the path to the data directory. postgresql.conf and pg_hba.conf are
@@ -460,6 +482,9 @@ sub init
 	}
 	close $conf;
 
+    chmod($self->group_access ? 0640 : 0600, "$pgdata/postgresql.conf")
+        or die("unable to set permissions for $pgdata/postgresql.conf");
+
 	$self->set_replication_conf if $params{allows_streaming};
 	$self->enable_archiving     if $params{has_archiving};
 }
@@ -485,7 +510,7 @@ sub append_conf
 
 	TestLib::append_to_file($conffile, $str . "\n");
 
-    chmod(0600, $conffile)
+    chmod($self->group_access() ? 0640 : 0600, $conffile)
         or die("unable to set permissions for $conffile");
 }
 
diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index 9de8dc9041..93ac24e261 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -30,6 +30,7 @@ our @EXPORT = qw(
   slurp_file
   append_to_file
   check_mode_recursive
+  chmod_recursive
   check_pg_config
   system_or_bail
   system_log
@@ -295,6 +296,30 @@ sub check_mode_recursive
 	return $result;
 }
 
+# Change mode recursively on a directory
+sub chmod_recursive
+{
+	my ($dir, $dir_mode, $file_mode) = @_;
+
+	find
+	(
+		{follow_fast => 1,
+		wanted =>
+			sub
+			{
+				my $file_stat = stat($File::Find::name);
+
+				if (defined($file_stat))
+				{
+					chmod(S_ISDIR($file_stat->mode) ? $dir_mode : $file_mode,
+						  $File::Find::name)
+						or die "unable to chmod $File::Find::name";
+				}
+			}},
+		$dir
+	);
+}
+
 # Check presence of a given regexp within pg_config.h for the installation
 # where tests are running, returning a match status result depending on
 # that.
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index 72976f44d8..78d4934ec6 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -125,7 +125,7 @@ sub mkvcbuild
 	}
 
 	our @pgcommonfrontendfiles = (
-		@pgcommonallfiles, qw(fe_memutils.c file_utils.c
+		@pgcommonallfiles, qw(fe_memutils.c file_perm.c file_utils.c
 		  restricted_token.c));
 
 	our @pgcommonbkndfiles = @pgcommonallfiles;
-- 
2.14.3 (Apple Git-98)