Hi Michael, Thanks for having a look at the patches.
On 1/30/18 3:01 AM, Michael Paquier wrote: > On Mon, Jan 29, 2018 at 04:29:08PM -0500, David Steele wrote: >> >> Adds a *very* basic test suite for pg_resetwal. I was able to make this >> utility core dump (floating point errors) pretty easily with empty or >> malformed pg_control files so I focused on WAL reset functionality plus >> the basic help/command tests that every utility has. > > A little is better than nothing, so that's an improvement, thanks! > > +# Remove WAL from pg_wal and make sure it gets rebuilt > +$node->stop; > + > +unlink("$pgwal/000000010000000000000001") == 1 > + or die("unable to remove 000000010000000000000001"); > + > +is_deeply( > + [sort(slurp_dir($pgwal))], [sort(qw(. .. archive_status))], 'no WAL'); > This is_deeply test serves little purpose. The segment gets removed > directly so I would just remove it. The purpose of this test to be ensure nothing else is in the directory. As the tests get more complex I think keeping track of the state will be important. In other words, this is really an assertion that the current test state is correct, not that the prior command succeeded. > Another test that could be easily included is with -o, then create a > table and check for its OID value. Also for -x, then just use > txid_current to check the value consistency. For -c, with > track_commit_timestamp set to on, you can set a couple of values and > then check for pg_last_committed_xact() which would be NULL if you use > an XID older than the minimum set for example. All those are simple > enough so they could easily be added in the first version of this test > suite. I would prefer to leave this for another patch. > You need to update src/bin/pg_resetxlog/.gitignore to include > tmp_check/. Added. >> 2) 02-mkdir >> >> Converts mkdir() calls to MakeDirectoryDefaultPerm() if the original >> call used default permissions. > > So the only call not converted to that API is in ipc.c... OK, this one > is pretty simple so nothing really to say about it, the real meat comes > with patch 3. I would rather see with a good eye if this patch > introduces file_perm.h which includes both PG_FILE_MODE_DEFAULT and > PG_DIR_MODE_DEFAULT, and have the frontends use those values. This > would make the switch to 0003 a bit easier if you look at pg_resetwal's > diffs for example. Agreed. I thought about this originally but could not come up with a good split. I spent some time on it and came up with something that I think works (and would make sense to commit separately). >> 3) 03-group >> >> Allow group access on PGDATA. This includes backend changes, utility >> changes (initdb, pg_ctl, pg_upgrade, etc.) and tests for each utility. > > +++ b/src/include/common/file_perm.h > + * > + * This module is located in common so the backend can use the constants. > + * > This is the case of more or less all the content in src/common/, except > for the things which are frontend-only, so this comment could just be > removed. Removed. > +command_ok( > + ['chmod', "-R", 'g+rX', "$pgdata"], > + 'add group perms to PGDATA'); > > That would blow up on Windows. You should replace that by perl's chmod > and look at its return result for sanity checks, likely with ($cnt != 0). > And let's not use icacls please... Fixed with a new function, add_pg_data_group_perm(). > + if ($params{has_group_access}) > + { > + push(@{$params{extra}}, '-g'); > + } > No need to introduce a new parameter here, please use directly extra => > [ '-g' ] in the routines calling PostgresNode::init. Other code needs to know if group access is enabled on the node (see RewindTest.pm) so it's not just a matter of passing the option. > The efforts put in the tests are good. Thanks! New patches are attached. The end result is almost identical to v6 but code was moved from 03 to 02 as per Michael's suggestion. 1) 01-pgresetwal-test Adds a very basic test suite for pg_resetwal. 2) 02-file-perm Adds a new header (file_perm.h) and makes all front-end utilities use the new constants for setting umask. Converts mkdir() calls in the backend to MakeDirectoryDefaultPerm() if the original call used default permissions. Adds tests to make sure permissions in PGDATA are set correctly by the front-end tools. 3) 03-group Allow group access on PGDATA. This includes backend changes, utility changes (initdb, pg_ctl, pg_upgrade, etc.) and tests for each utility. Thanks! -- -David da...@pgmasters.net
diff --git a/src/bin/pg_resetwal/.gitignore b/src/bin/pg_resetwal/.gitignore index 236abb4323..a950255fd7 100644 --- a/src/bin/pg_resetwal/.gitignore +++ b/src/bin/pg_resetwal/.gitignore @@ -1 +1,2 @@ /pg_resetwal +/tmp_check diff --git a/src/bin/pg_resetwal/Makefile b/src/bin/pg_resetwal/Makefile index 5ab7ad33e0..13c9ca6279 100644 --- a/src/bin/pg_resetwal/Makefile +++ b/src/bin/pg_resetwal/Makefile @@ -33,3 +33,9 @@ uninstall: clean distclean maintainer-clean: rm -f pg_resetwal$(X) $(OBJS) + +check: + $(prove_check) + +installcheck: + $(prove_installcheck) diff --git a/src/bin/pg_resetwal/t/001_basic.pl b/src/bin/pg_resetwal/t/001_basic.pl new file mode 100644 index 0000000000..234bd70303 --- /dev/null +++ b/src/bin/pg_resetwal/t/001_basic.pl @@ -0,0 +1,53 @@ +use strict; +use warnings; + +use Config; +use PostgresNode; +use TestLib; +use Test::More tests => 13; + +my $tempdir = TestLib::tempdir; +my $tempdir_short = TestLib::tempdir_short; + +program_help_ok('pg_resetwal'); +program_version_ok('pg_resetwal'); +program_options_handling_ok('pg_resetwal'); + +# Initialize node without replication settings +my $node = get_new_node('main'); +my $pgdata = $node->data_dir; +my $pgwal = "$pgdata/pg_wal"; +$node->init; +$node->start; + +# Remove WAL from pg_wal and make sure it gets rebuilt +$node->stop; + +unlink("$pgwal/000000010000000000000001") == 1 + or die("unable to remove 000000010000000000000001"); + +is_deeply( + [sort(slurp_dir($pgwal))], [sort(qw(. .. archive_status))], 'no WAL'); + +$node->command_ok(['pg_resetwal', '-D', $pgdata], 'recreate pg_wal'); + +is_deeply( + [sort(slurp_dir($pgwal))], + [sort(qw(. .. archive_status 000000010000000000000002))], + 'WAL recreated'); + +$node->start; + +# Reset to specific WAL segment +$node->stop; + +$node->command_ok( + ['pg_resetwal', '-l', '000000070000000700000007', '-D', $pgdata], + 'set to specific WAL'); + +is_deeply( + [sort(slurp_dir($pgwal))], + [sort(qw(. .. archive_status 000000070000000700000007))], + 'WAL recreated'); + +$node->start;
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 47a6c4d895..bf07bbd746 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -4086,7 +4086,7 @@ ValidateXLOGDirectoryStructure(void) { ereport(LOG, (errmsg("creating missing WAL directory \"%s\"", path))); - if (mkdir(path, S_IRWXU) < 0) + if (MakeDirectoryDefaultPerm(path) < 0) ereport(FATAL, (errmsg("could not create missing directory \"%s\": %m", path))); diff --git a/src/backend/commands/tablespace.c b/src/backend/commands/tablespace.c index 5c450caa4e..c4ee987446 100644 --- a/src/backend/commands/tablespace.c +++ b/src/backend/commands/tablespace.c @@ -68,6 +68,7 @@ #include "commands/seclabel.h" #include "commands/tablecmds.h" #include "commands/tablespace.h" +#include "common/file_perm.h" #include "miscadmin.h" #include "postmaster/bgwriter.h" #include "storage/fd.h" @@ -151,7 +152,7 @@ TablespaceCreateDbspace(Oid spcNode, Oid dbNode, bool isRedo) else { /* Directory creation failed? */ - if (mkdir(dir, S_IRWXU) < 0) + if (MakeDirectoryDefaultPerm(dir) < 0) { char *parentdir; @@ -173,7 +174,7 @@ TablespaceCreateDbspace(Oid spcNode, Oid dbNode, bool isRedo) get_parent_directory(parentdir); get_parent_directory(parentdir); /* Can't create parent and it doesn't already exist? */ - if (mkdir(parentdir, S_IRWXU) < 0 && errno != EEXIST) + if (MakeDirectoryDefaultPerm(parentdir) < 0 && errno != EEXIST) ereport(ERROR, (errcode_for_file_access(), errmsg("could not create directory \"%s\": %m", @@ -184,7 +185,7 @@ TablespaceCreateDbspace(Oid spcNode, Oid dbNode, bool isRedo) parentdir = pstrdup(dir); get_parent_directory(parentdir); /* Can't create parent and it doesn't already exist? */ - if (mkdir(parentdir, S_IRWXU) < 0 && errno != EEXIST) + if (MakeDirectoryDefaultPerm(parentdir) < 0 && errno != EEXIST) ereport(ERROR, (errcode_for_file_access(), errmsg("could not create directory \"%s\": %m", @@ -192,7 +193,7 @@ TablespaceCreateDbspace(Oid spcNode, Oid dbNode, bool isRedo) pfree(parentdir); /* Create database directory */ - if (mkdir(dir, S_IRWXU) < 0) + if (MakeDirectoryDefaultPerm(dir) < 0) ereport(ERROR, (errcode_for_file_access(), errmsg("could not create directory \"%s\": %m", @@ -279,7 +280,8 @@ CreateTableSpace(CreateTableSpaceStmt *stmt) /* * Check that location isn't too long. Remember that we're going to append * 'PG_XXX/<dboid>/<relid>_<fork>.<nnn>'. FYI, we never actually - * reference the whole path here, but mkdir() uses the first two parts. + * reference the whole path here, but MakeDirectoryDefaultPerm() uses the first + * two parts. */ if (strlen(location) + 1 + strlen(TABLESPACE_VERSION_DIRECTORY) + 1 + OIDCHARS + 1 + OIDCHARS + 1 + FORKNAMECHARS + 1 + OIDCHARS > MAXPGPATH) @@ -565,6 +567,7 @@ create_tablespace_directories(const char *location, const Oid tablespaceoid) char *linkloc; char *location_with_version_dir; struct stat st; + mode_t current_umask; /* Current umask value */ linkloc = psprintf("pg_tblspc/%u", tablespaceoid); location_with_version_dir = psprintf("%s/%s", location, @@ -574,7 +577,10 @@ create_tablespace_directories(const char *location, const Oid tablespaceoid) * Attempt to coerce target directory to safe permissions. If this fails, * it doesn't exist or has the wrong owner. */ - if (chmod(location, S_IRWXU) != 0) + current_umask = umask(0); + umask(current_umask); + + if (chmod(location, PG_DIR_MODE_DEFAULT & ~current_umask) != 0) { if (errno == ENOENT) ereport(ERROR, @@ -599,7 +605,7 @@ create_tablespace_directories(const char *location, const Oid tablespaceoid) if (stat(location_with_version_dir, &st) == 0 && S_ISDIR(st.st_mode)) { if (!rmtree(location_with_version_dir, true)) - /* If this failed, mkdir() below is going to error. */ + /* If this failed, MakeDirectoryDefaultPerm() below is going to error. */ ereport(WARNING, (errmsg("some useless files may be left behind in old database directory \"%s\"", location_with_version_dir))); @@ -610,7 +616,7 @@ create_tablespace_directories(const char *location, const Oid tablespaceoid) * The creation of the version directory prevents more than one tablespace * in a single location. */ - if (mkdir(location_with_version_dir, S_IRWXU) < 0) + if (MakeDirectoryDefaultPerm(location_with_version_dir) < 0) { if (errno == EEXIST) ereport(ERROR, diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index f3ddf828bb..e13a2ae8c4 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -4495,7 +4495,7 @@ internal_forkexec(int argc, char *argv[], Port *port) * As in OpenTemporaryFileInTablespace, try to make the temp-file * directory */ - mkdir(PG_TEMP_FILES_DIR, S_IRWXU); + MakeDirectoryDefaultPerm(PG_TEMP_FILES_DIR); fp = AllocateFile(tmpfilename, PG_BINARY_W); if (!fp) diff --git a/src/backend/postmaster/syslogger.c b/src/backend/postmaster/syslogger.c index f70eea37df..ae23941f59 100644 --- a/src/backend/postmaster/syslogger.c +++ b/src/backend/postmaster/syslogger.c @@ -41,6 +41,7 @@ #include "postmaster/postmaster.h" #include "postmaster/syslogger.h" #include "storage/dsm.h" +#include "storage/fd.h" #include "storage/ipc.h" #include "storage/latch.h" #include "storage/pg_shmem.h" @@ -322,7 +323,7 @@ SysLoggerMain(int argc, char *argv[]) /* * Also, create new directory if not present; ignore errors */ - mkdir(Log_directory, S_IRWXU); + MakeDirectoryDefaultPerm(Log_directory); } if (strcmp(Log_filename, currentLogFilename) != 0) { @@ -564,7 +565,7 @@ SysLogger_Start(void) /* * Create log directory if not present; ignore errors */ - mkdir(Log_directory, S_IRWXU); + MakeDirectoryDefaultPerm(Log_directory); /* * The initial logfile is created right in the postmaster, to verify that diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c index fc9ef22b0b..3a98360b50 100644 --- a/src/backend/replication/slot.c +++ b/src/backend/replication/slot.c @@ -1166,13 +1166,14 @@ CreateSlotOnDisk(ReplicationSlot *slot) * It's just barely possible that some previous effort to create or drop a * slot with this name left a temp directory lying around. If that seems * to be the case, try to remove it. If the rmtree() fails, we'll error - * out at the mkdir() below, so we don't bother checking success. + * out at the MakeDirectoryDefaultPerm() below, so we don't bother checking + * success. */ if (stat(tmppath, &st) == 0 && S_ISDIR(st.st_mode)) rmtree(tmppath, true); /* Create and fsync the temporary slot directory. */ - if (mkdir(tmppath, S_IRWXU) < 0) + if (MakeDirectoryDefaultPerm(tmppath) < 0) ereport(ERROR, (errcode_for_file_access(), errmsg("could not create directory \"%s\": %m", diff --git a/src/backend/storage/file/copydir.c b/src/backend/storage/file/copydir.c index ca6342db0d..ff980dc1f5 100644 --- a/src/backend/storage/file/copydir.c +++ b/src/backend/storage/file/copydir.c @@ -41,7 +41,7 @@ copydir(char *fromdir, char *todir, bool recurse) char fromfile[MAXPGPATH * 2]; char tofile[MAXPGPATH * 2]; - if (mkdir(todir, S_IRWXU) != 0) + if (MakeDirectoryDefaultPerm(todir) != 0) ereport(ERROR, (errcode_for_file_access(), errmsg("could not create directory \"%s\": %m", todir))); diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c index 2a18e94ff4..1491665375 100644 --- a/src/backend/storage/file/fd.c +++ b/src/backend/storage/file/fd.c @@ -84,6 +84,7 @@ #include "access/xlog.h" #include "catalog/catalog.h" #include "catalog/pg_tablespace.h" +#include "common/file_perm.h" #include "pgstat.h" #include "portability/mem.h" #include "storage/fd.h" @@ -124,12 +125,6 @@ */ #define FD_MINFREE 10 -/* - * Default mode for created files, unless something else is specified using - * the *Perm() function variants. - */ -#define PG_FILE_MODE_DEFAULT (S_IRUSR | S_IWUSR) - /* * A number of platforms allow individual processes to open many more files * than they can really support when *many* processes do the same thing. @@ -1435,7 +1430,7 @@ PathNameOpenFilePerm(const char *fileName, int fileFlags, mode_t fileMode) void PathNameCreateTemporaryDir(const char *basedir, const char *directory) { - if (mkdir(directory, S_IRWXU) < 0) + if (MakeDirectoryDefaultPerm(directory) < 0) { if (errno == EEXIST) return; @@ -1445,14 +1440,14 @@ PathNameCreateTemporaryDir(const char *basedir, const char *directory) * EEXIST to close a race against another process following the same * algorithm. */ - if (mkdir(basedir, S_IRWXU) < 0 && errno != EEXIST) + if (MakeDirectoryDefaultPerm(basedir) < 0 && errno != EEXIST) ereport(ERROR, (errcode_for_file_access(), errmsg("cannot create temporary directory \"%s\": %m", basedir))); /* Try again. */ - if (mkdir(directory, S_IRWXU) < 0 && errno != EEXIST) + if (MakeDirectoryDefaultPerm(directory) < 0 && errno != EEXIST) ereport(ERROR, (errcode_for_file_access(), errmsg("cannot create temporary subdirectory \"%s\": %m", @@ -1602,11 +1597,11 @@ OpenTemporaryFileInTablespace(Oid tblspcOid, bool rejectError) * We might need to create the tablespace's tempfile directory, if no * one has yet done so. * - * Don't check for error from mkdir; it could fail if someone else - * just did the same thing. If it doesn't work then we'll bomb out on + * Don't check error from MakeDirectoryDefaultPerm; it could fail if someone + * else just did the same thing. If it doesn't work then we'll bomb out on * the second create attempt, instead. */ - mkdir(tempdirpath, S_IRWXU); + MakeDirectoryDefaultPerm(tempdirpath); file = PathNameOpenFile(tempfilepath, O_RDWR | O_CREAT | O_TRUNC | PG_BINARY); @@ -3555,3 +3550,16 @@ fsync_parent_path(const char *fname, int elevel) return 0; } + +/* + * Create a directory using PG_DIR_MODE_DEFAULT for permissions + * + * Directories in PGDATA should normally have specific permissions -- when + * using this function the caller does not need to know what they should be. + * For permissions other than the default use mkdir() directly. + */ +int +MakeDirectoryDefaultPerm(const char *directoryName) +{ + return mkdir(directoryName, PG_DIR_MODE_DEFAULT); +} diff --git a/src/backend/storage/ipc/ipc.c b/src/backend/storage/ipc/ipc.c index 726db7b7f1..466613e4af 100644 --- a/src/backend/storage/ipc/ipc.c +++ b/src/backend/storage/ipc/ipc.c @@ -137,6 +137,10 @@ proc_exit(int code) else snprintf(gprofDirName, 32, "gprof/%d", (int) getpid()); + /* + * Use mkdir() instead of MakeDirectoryDefaultPerm() here because non-default + * permissions are required. + */ mkdir("gprof", S_IRWXU | S_IRWXG | S_IRWXO); mkdir(gprofDirName, S_IRWXU | S_IRWXG | S_IRWXO); chdir(gprofDirName); diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c index 2efd3b75b1..4ef06c2349 100644 --- a/src/bin/initdb/initdb.c +++ b/src/bin/initdb/initdb.c @@ -64,6 +64,7 @@ #include "catalog/pg_authid.h" #include "catalog/pg_class.h" #include "catalog/pg_collation.h" +#include "common/file_perm.h" #include "common/file_utils.h" #include "common/restricted_token.h" #include "common/username.h" @@ -144,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 */ @@ -1170,12 +1172,6 @@ setup_config(void) snprintf(path, sizeof(path), "%s/postgresql.conf", pg_data); writefile(path, conflines); - if (chmod(path, S_IRUSR | S_IWUSR) != 0) - { - fprintf(stderr, _("%s: could not change permissions of \"%s\": %s\n"), - progname, path, strerror(errno)); - exit_nicely(); - } /* * create the automatic configuration file to store the configuration @@ -1190,12 +1186,6 @@ setup_config(void) sprintf(path, "%s/postgresql.auto.conf", pg_data); writefile(path, autoconflines); - if (chmod(path, S_IRUSR | S_IWUSR) != 0) - { - fprintf(stderr, _("%s: could not change permissions of \"%s\": %s\n"), - progname, path, strerror(errno)); - exit_nicely(); - } free(conflines); @@ -1277,12 +1267,6 @@ setup_config(void) snprintf(path, sizeof(path), "%s/pg_hba.conf", pg_data); writefile(path, conflines); - if (chmod(path, S_IRUSR | S_IWUSR) != 0) - { - fprintf(stderr, _("%s: could not change permissions of \"%s\": %s\n"), - progname, path, strerror(errno)); - exit_nicely(); - } free(conflines); @@ -1293,12 +1277,6 @@ setup_config(void) snprintf(path, sizeof(path), "%s/pg_ident.conf", pg_data); writefile(path, conflines); - if (chmod(path, S_IRUSR | S_IWUSR) != 0) - { - fprintf(stderr, _("%s: could not change permissions of \"%s\": %s\n"), - progname, path, strerror(errno)); - exit_nicely(); - } free(conflines); @@ -2692,7 +2670,7 @@ create_data_directory(void) pg_data); fflush(stdout); - if (pg_mkdir_p(pg_data, S_IRWXU) != 0) + if (pg_mkdir_p(pg_data, PG_DIR_MODE_DEFAULT) != 0) { fprintf(stderr, _("%s: could not create directory \"%s\": %s\n"), progname, pg_data, strerror(errno)); @@ -2710,7 +2688,7 @@ create_data_directory(void) pg_data); fflush(stdout); - if (chmod(pg_data, S_IRWXU) != 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)); @@ -2778,7 +2756,7 @@ create_xlog_or_symlink(void) xlog_dir); fflush(stdout); - if (pg_mkdir_p(xlog_dir, S_IRWXU) != 0) + if (pg_mkdir_p(xlog_dir, PG_DIR_MODE_DEFAULT) != 0) { fprintf(stderr, _("%s: could not create directory \"%s\": %s\n"), progname, xlog_dir, strerror(errno)); @@ -2796,7 +2774,7 @@ create_xlog_or_symlink(void) xlog_dir); fflush(stdout); - if (chmod(xlog_dir, S_IRWXU) != 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)); @@ -2846,7 +2824,7 @@ create_xlog_or_symlink(void) else { /* Without -X option, just make the subdirectory normally */ - if (mkdir(subdirloc, S_IRWXU) < 0) + if (mkdir(subdirloc, PG_DIR_MODE_DEFAULT) < 0) { fprintf(stderr, _("%s: could not create directory \"%s\": %s\n"), progname, subdirloc, strerror(errno)); @@ -2882,8 +2860,6 @@ initialize_data_directory(void) setup_signals(); - umask(S_IRWXG | S_IRWXO); - create_data_directory(); create_xlog_or_symlink(); @@ -2902,7 +2878,7 @@ initialize_data_directory(void) * The parent directory already exists, so we only need mkdir() not * pg_mkdir_p() here, which avoids some failure modes; cf bug #13853. */ - if (mkdir(path, S_IRWXU) < 0) + if (mkdir(path, PG_DIR_MODE_DEFAULT) < 0) { fprintf(stderr, _("%s: could not create directory \"%s\": %s\n"), progname, path, strerror(errno)); @@ -3159,6 +3135,8 @@ main(int argc, char *argv[]) } } + /* Set the file mode creation mask */ + umask(file_mode_mask); /* * Non-option argument specifies data directory as long as it wasn't diff --git a/src/bin/initdb/t/001_initdb.pl b/src/bin/initdb/t/001_initdb.pl index c0cfa6e92c..561608f1d8 100644 --- a/src/bin/initdb/t/001_initdb.pl +++ b/src/bin/initdb/t/001_initdb.pl @@ -6,7 +6,7 @@ use strict; use warnings; use PostgresNode; use TestLib; -use Test::More tests => 15; +use Test::More tests => 16; my $tempdir = TestLib::tempdir; my $xlogdir = "$tempdir/pgxlog"; @@ -45,6 +45,8 @@ mkdir $datadir; command_ok([ 'initdb', '-N', '-T', 'german', '-X', $xlogdir, $datadir ], 'successful creation'); + + ok(check_pg_data_perm($datadir, 0)); } command_ok([ 'initdb', '-S', $datadir ], 'sync only'); command_fails([ 'initdb', $datadir ], 'existing data directory'); diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c index 9bc830b085..1eef7179cc 100644 --- a/src/bin/pg_ctl/pg_ctl.c +++ b/src/bin/pg_ctl/pg_ctl.c @@ -25,6 +25,7 @@ #include "catalog/pg_control.h" #include "common/controldata_utils.h" +#include "common/file_perm.h" #include "getopt_long.h" #include "utils/pidfile.h" @@ -2170,7 +2171,7 @@ main(int argc, char **argv) */ argv0 = argv[0]; - umask(S_IRWXG | S_IRWXO); + umask(PG_MODE_MASK_DEFAULT); /* support --help and --version even if invoked as root */ if (argc > 1) diff --git a/src/bin/pg_ctl/t/001_start_stop.pl b/src/bin/pg_ctl/t/001_start_stop.pl index 5da4746cb4..909bf4d465 100644 --- a/src/bin/pg_ctl/t/001_start_stop.pl +++ b/src/bin/pg_ctl/t/001_start_stop.pl @@ -4,7 +4,7 @@ use warnings; use Config; use PostgresNode; use TestLib; -use Test::More tests => 19; +use Test::More tests => 20; my $tempdir = TestLib::tempdir; my $tempdir_short = TestLib::tempdir_short; @@ -57,10 +57,15 @@ command_ok([ 'pg_ctl', 'stop', '-D', "$tempdir/data" ], 'pg_ctl stop'); command_fails([ 'pg_ctl', 'stop', '-D', "$tempdir/data" ], 'second pg_ctl stop fails'); +# Log file for first perm test +my $logFileName = "$tempdir/data/perm-test-600.log"; + command_ok( - [ 'pg_ctl', 'restart', '-D', "$tempdir/data" ], + [ 'pg_ctl', 'restart', '-D', "$tempdir/data", '-l', $logFileName ], 'pg_ctl restart with server not running'); -command_ok([ 'pg_ctl', 'restart', '-D', "$tempdir/data" ], - 'pg_ctl restart with server running'); + +# Log file should exist and have no group permissions +ok(-f $logFileName); +ok(check_pg_data_perm("$tempdir/data", 0)); 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 a132cf2e9f..47abd41fc3 100644 --- a/src/bin/pg_resetwal/pg_resetwal.c +++ b/src/bin/pg_resetwal/pg_resetwal.c @@ -52,6 +52,7 @@ #include "catalog/catversion.h" #include "catalog/pg_control.h" #include "common/fe_memutils.h" +#include "common/file_perm.h" #include "common/restricted_token.h" #include "storage/large_object.h" #include "pg_getopt.h" @@ -327,6 +328,9 @@ main(int argc, char *argv[]) exit(1); } + /* Set dir/file mode mask */ + umask(PG_MODE_MASK_DEFAULT); + /* Check that data directory matches our server version */ CheckDataVersion(); @@ -919,7 +923,7 @@ RewriteControlFile(void) fd = open(XLOG_CONTROL_FILE, O_RDWR | O_CREAT | O_EXCL | PG_BINARY, - S_IRUSR | S_IWUSR); + PG_FILE_MODE_DEFAULT); if (fd < 0) { fprintf(stderr, _("%s: could not create pg_control file: %s\n"), @@ -1201,7 +1205,7 @@ WriteEmptyXLOG(void) unlink(path); fd = open(path, O_RDWR | O_CREAT | O_EXCL | PG_BINARY, - S_IRUSR | S_IWUSR); + PG_FILE_MODE_DEFAULT); if (fd < 0) { fprintf(stderr, _("%s: could not open file \"%s\": %s\n"), diff --git a/src/bin/pg_resetwal/t/001_basic.pl b/src/bin/pg_resetwal/t/001_basic.pl index 234bd70303..db0c2a630a 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 => 13; +use Test::More tests => 14; my $tempdir = TestLib::tempdir; my $tempdir_short = TestLib::tempdir_short; @@ -36,6 +36,8 @@ is_deeply( [sort(qw(. .. archive_status 000000010000000000000002))], 'WAL recreated'); +ok(check_pg_data_perm($pgdata, 0), 'check PGDATA permissions'); + $node->start; # Reset to specific WAL segment diff --git a/src/bin/pg_rewind/RewindTest.pm b/src/bin/pg_rewind/RewindTest.pm index 00b5b42dd7..b18a7f954c 100644 --- a/src/bin/pg_rewind/RewindTest.pm +++ b/src/bin/pg_rewind/RewindTest.pm @@ -237,6 +237,9 @@ sub run_pg_rewind "$tmp_folder/master-postgresql.conf.tmp", "$master_pgdata/postgresql.conf"); + chmod(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 my $port_standby = $node_standby->port; $node_master->append_conf( @@ -255,6 +258,8 @@ recovery_target_timeline='latest' # Clean up after the test. Stop both servers, if they're still running. sub clean_rewind_test { + ok (check_pg_data_perm($node_master->data_dir(), 0)); + $node_master->teardown_node if defined $node_master; $node_standby->teardown_node if defined $node_standby; } diff --git a/src/bin/pg_rewind/file_ops.c b/src/bin/pg_rewind/file_ops.c index 705383d184..d51914e901 100644 --- a/src/bin/pg_rewind/file_ops.c +++ b/src/bin/pg_rewind/file_ops.c @@ -18,6 +18,7 @@ #include <fcntl.h> #include <unistd.h> +#include "common/file_perm.h" #include "file_ops.h" #include "filemap.h" #include "logging.h" @@ -58,7 +59,7 @@ open_target_file(const char *path, bool trunc) mode = O_WRONLY | O_CREAT | PG_BINARY; if (trunc) mode |= O_TRUNC; - dstfd = open(dstpath, mode, 0600); + dstfd = open(dstpath, mode, PG_FILE_MODE_DEFAULT); if (dstfd < 0) pg_fatal("could not open target file \"%s\": %s\n", dstpath, strerror(errno)); @@ -190,7 +191,7 @@ truncate_target_file(const char *path, off_t newsize) snprintf(dstpath, sizeof(dstpath), "%s/%s", datadir_target, path); - fd = open(dstpath, O_WRONLY, 0); + fd = open(dstpath, O_WRONLY, PG_FILE_MODE_DEFAULT); if (fd < 0) pg_fatal("could not open file \"%s\" for truncation: %s\n", dstpath, strerror(errno)); @@ -211,7 +212,7 @@ create_target_dir(const char *path) return; snprintf(dstpath, sizeof(dstpath), "%s/%s", datadir_target, path); - if (mkdir(dstpath, S_IRWXU) != 0) + if (mkdir(dstpath, PG_DIR_MODE_DEFAULT) != 0) pg_fatal("could not create directory \"%s\": %s\n", dstpath, strerror(errno)); } diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c index 72ab2f8d5e..3d179e2c7d 100644 --- a/src/bin/pg_rewind/pg_rewind.c +++ b/src/bin/pg_rewind/pg_rewind.c @@ -24,6 +24,7 @@ #include "access/xlog_internal.h" #include "catalog/catversion.h" #include "catalog/pg_control.h" +#include "common/file_perm.h" #include "common/restricted_token.h" #include "getopt_long.h" #include "storage/bufpage.h" @@ -185,6 +186,9 @@ main(int argc, char **argv) exit(1); } + /* Set dir/file mode mask */ + umask(PG_MODE_MASK_DEFAULT); + /* * Don't allow pg_rewind to be run as root, to avoid overwriting the * ownership of files in the data directory. We need only check for root diff --git a/src/bin/pg_rewind/t/001_basic.pl b/src/bin/pg_rewind/t/001_basic.pl index 736f34eae3..e7fc200fc0 100644 --- a/src/bin/pg_rewind/t/001_basic.pl +++ b/src/bin/pg_rewind/t/001_basic.pl @@ -1,7 +1,7 @@ use strict; use warnings; use TestLib; -use Test::More tests => 8; +use Test::More tests => 10; use RewindTest; diff --git a/src/bin/pg_rewind/t/002_databases.pl b/src/bin/pg_rewind/t/002_databases.pl index 37cdd712f3..0e0140b96f 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; diff --git a/src/bin/pg_rewind/t/003_extrafiles.pl b/src/bin/pg_rewind/t/003_extrafiles.pl index 2433a4aab6..28933da343 100644 --- a/src/bin/pg_rewind/t/003_extrafiles.pl +++ b/src/bin/pg_rewind/t/003_extrafiles.pl @@ -3,7 +3,7 @@ use strict; use warnings; use TestLib; -use Test::More tests => 4; +use Test::More tests => 6; use File::Find; @@ -14,6 +14,8 @@ sub run_test { my $test_mode = shift; + umask(0077); + RewindTest::setup_cluster($test_mode); RewindTest::start_master(); diff --git a/src/bin/pg_rewind/t/004_pg_xlog_symlink.pl b/src/bin/pg_rewind/t/004_pg_xlog_symlink.pl index feadaa6a0f..70dd061915 100644 --- a/src/bin/pg_rewind/t/004_pg_xlog_symlink.pl +++ b/src/bin/pg_rewind/t/004_pg_xlog_symlink.pl @@ -14,7 +14,7 @@ if ($windows_os) } else { - plan tests => 4; + plan tests => 6; } use RewindTest; diff --git a/src/bin/pg_rewind/t/005_same_timeline.pl b/src/bin/pg_rewind/t/005_same_timeline.pl index 0e334ee191..2fbca4ad7c 100644 --- a/src/bin/pg_rewind/t/005_same_timeline.pl +++ b/src/bin/pg_rewind/t/005_same_timeline.pl @@ -1,7 +1,7 @@ use strict; use warnings; use TestLib; -use Test::More tests => 1; +use Test::More tests => 2; use RewindTest; diff --git a/src/bin/pg_upgrade/file.c b/src/bin/pg_upgrade/file.c index f38bfacf02..595d43ee31 100644 --- a/src/bin/pg_upgrade/file.c +++ b/src/bin/pg_upgrade/file.c @@ -10,6 +10,7 @@ #include "postgres_fe.h" #include "access/visibilitymap.h" +#include "common/file_perm.h" #include "pg_upgrade.h" #include "storage/bufpage.h" #include "storage/checksum.h" @@ -44,7 +45,7 @@ copyFile(const char *src, const char *dst, schemaName, relName, src, strerror(errno)); if ((dest_fd = open(dst, O_RDWR | O_CREAT | O_EXCL | PG_BINARY, - S_IRUSR | S_IWUSR)) < 0) + PG_FILE_MODE_DEFAULT)) < 0) pg_fatal("error while copying relation \"%s.%s\": could not create file \"%s\": %s\n", schemaName, relName, dst, strerror(errno)); @@ -151,7 +152,7 @@ rewriteVisibilityMap(const char *fromfile, const char *tofile, schemaName, relName, fromfile, strerror(errno)); if ((dst_fd = open(tofile, O_RDWR | O_CREAT | O_EXCL | PG_BINARY, - S_IRUSR | S_IWUSR)) < 0) + PG_FILE_MODE_DEFAULT)) < 0) pg_fatal("error while copying relation \"%s.%s\": could not create file \"%s\": %s\n", schemaName, relName, tofile, strerror(errno)); diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c index d12412799f..59df6fd88f 100644 --- a/src/bin/pg_upgrade/pg_upgrade.c +++ b/src/bin/pg_upgrade/pg_upgrade.c @@ -38,6 +38,7 @@ #include "pg_upgrade.h" #include "catalog/pg_class.h" +#include "common/file_perm.h" #include "common/restricted_token.h" #include "fe_utils/string_utils.h" @@ -79,7 +80,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 */ - umask(S_IRWXG | S_IRWXO); + umask(PG_MODE_MASK_DEFAULT); parseCommandLine(argc, argv); diff --git a/src/bin/pg_upgrade/test.sh b/src/bin/pg_upgrade/test.sh index 39983abea1..4e8db4aaf4 100644 --- a/src/bin/pg_upgrade/test.sh +++ b/src/bin/pg_upgrade/test.sh @@ -230,6 +230,17 @@ 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"; + exit 1; +fi + +if [ $(find ${PGDATA} -type d ! -perm 700 | wc -l) -ne 0 ]; then + echo "directories in PGDATA with permission != 700"; + exit 1; +fi + pg_ctl start -l "$logdir/postmaster2.log" -o "$POSTMASTER_OPTS" -w case $testhost in diff --git a/src/bin/pg_upgrade/version.c b/src/bin/pg_upgrade/version.c index 76e9d65537..205f772fc5 100644 --- a/src/bin/pg_upgrade/version.c +++ b/src/bin/pg_upgrade/version.c @@ -53,7 +53,7 @@ new_9_0_populate_pg_largeobject_metadata(ClusterInfo *cluster, bool check_mode) { PQExpBufferData connectbuf; - if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL) + if (script == NULL && (script = fopen(output_path, "w")) == NULL) pg_fatal("could not open file \"%s\": %s\n", output_path, strerror(errno)); @@ -152,7 +152,7 @@ old_9_3_check_for_line_data_type_usage(ClusterInfo *cluster) for (rowno = 0; rowno < ntups; rowno++) { found = true; - if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL) + if (script == NULL && (script = fopen(output_path, "w")) == NULL) pg_fatal("could not open file \"%s\": %s\n", output_path, strerror(errno)); if (!db_used) @@ -253,7 +253,7 @@ old_9_6_check_for_unknown_data_type_usage(ClusterInfo *cluster) for (rowno = 0; rowno < ntups; rowno++) { found = true; - if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL) + if (script == NULL && (script = fopen(output_path, "w")) == NULL) pg_fatal("could not open file \"%s\": %s\n", output_path, strerror(errno)); if (!db_used) @@ -335,7 +335,7 @@ old_9_6_invalidate_hash_indexes(ClusterInfo *cluster, bool check_mode) found = true; if (!check_mode) { - if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL) + if (script == NULL && (script = fopen(output_path, "w")) == NULL) pg_fatal("could not open file \"%s\": %s\n", output_path, strerror(errno)); if (!db_used) diff --git a/src/include/common/file_perm.h b/src/include/common/file_perm.h new file mode 100644 index 0000000000..5b823cbd39 --- /dev/null +++ b/src/include/common/file_perm.h @@ -0,0 +1,32 @@ +/*------------------------------------------------------------------------- + * + * File and directory permission constants + * + * + * Portions Copyright (c) 1996-2018, PostgreSQL Global Development Group + * Portions Copyright (c) 1994, Regents of the University of California + * + * src/include/common/file_perm.h + * + *------------------------------------------------------------------------- + */ +#ifndef FILE_PERM_H +#define FILE_PERM_H + +/* + * Default mode mask for data directory permissions that does not allow + * group execute/read. + */ +#define PG_MODE_MASK_DEFAULT (S_IRWXG | S_IRWXO) + +/* + * Default mode for created files. + */ +#define PG_FILE_MODE_DEFAULT (S_IRUSR | S_IWUSR | S_IRGRP) + +/* + * Default mode for directories. + */ +#define PG_DIR_MODE_DEFAULT (S_IRWXU | S_IRGRP | S_IXGRP) + +#endif /* FILE_PERM_H */ diff --git a/src/include/storage/fd.h b/src/include/storage/fd.h index 4244e7b1fd..33c636471b 100644 --- a/src/include/storage/fd.h +++ b/src/include/storage/fd.h @@ -112,6 +112,9 @@ extern int CloseTransientFile(int fd); extern int BasicOpenFile(const char *fileName, int fileFlags); extern int BasicOpenFilePerm(const char *fileName, int fileFlags, mode_t fileMode); + /* Make a directory with default permissions */ +extern int MakeDirectoryDefaultPerm(const char *directoryName); + /* Miscellaneous support routines */ extern void InitFileAccess(void); extern void set_max_safe_fds(void); diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm index 80188315f1..76e571b98c 100644 --- a/src/test/perl/PostgresNode.pm +++ b/src/test/perl/PostgresNode.pm @@ -484,6 +484,9 @@ sub append_conf my $conffile = $self->data_dir . '/' . $filename; TestLib::append_to_file($conffile, $str . "\n"); + + chmod(0600, $conffile) + or die("unable to set permissions for $conffile"); } =pod diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm index fdd427608b..acccecc5de 100644 --- a/src/test/perl/TestLib.pm +++ b/src/test/perl/TestLib.pm @@ -12,8 +12,11 @@ use warnings; use Config; use Exporter 'import'; +use Fcntl qw(:mode); use File::Basename; +use File::Find; use File::Spec; +use File::stat qw(stat); use File::Temp (); use IPC::Run; use SimpleTee; @@ -26,6 +29,7 @@ our @EXPORT = qw( slurp_dir slurp_file append_to_file + check_pg_data_perm check_pg_config system_or_bail system_log @@ -222,6 +226,83 @@ sub append_to_file close $fh; } +# Ensure all permissions in the pg_data directory are correct. When allow_group +# is true then permissions should be dir = 0750, file = 0640. When allow_group +# is false then permissions should be dir = 0700, file = 0600. +sub check_pg_data_perm +{ + my ($pgdata, $allow_group) = @_; + + # Result defaults to true + my $result = 1; + + # Expected permission + my $expected_file_perm = $allow_group ? 0640 : 0600; + my $expected_dir_perm = $allow_group ? 0750 : 0700; + + find + ( + sub + { + my $file_stat = stat($File::Find::name); + + defined($file_stat) + or die("unable to stat $File::Find::name"); + + my $file_mode = S_IMODE($file_stat->mode); + + # Is this a file? + if (S_ISREG($file_stat->mode)) + { + # postmaster.pid file must be 600 + if ($File::Find::name =~ /\/postmaster\.pid$/) + { + if ($file_mode != 0600) + { + print(*STDERR, "$File::Find::name mode must be 0600\n"); + + $result = 0; + return + } + } + else + { + if ($file_mode != $expected_file_perm) + { + print(*STDERR, + sprintf("$File::Find::name mode must be %04o\n", + $expected_file_perm)); + + $result = 0; + return + } + } + } + # Else a directory? + elsif (S_ISDIR($file_stat->mode)) + { + if ($file_mode != $expected_dir_perm) + { + print(*STDERR, + sprintf("$File::Find::name mode must be %04o\n", + $expected_dir_perm)); + + $result = 0; + return + } + } + # Else something we can't handle + else + { + die "unknown file type for $File::Find::name"; + } + }, + $pgdata + ); + + return $result; +} + # 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/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 71f02300c2..8d8a48aad4 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> @@ -2220,6 +2223,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 e13a2ae8c4..65f528147a 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); @@ -1524,25 +1526,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 4ef06c2349..56356d75c5 100644 --- a/src/bin/initdb/initdb.c +++ b/src/bin/initdb/initdb.c @@ -2289,6 +2289,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" @@ -2992,6 +2993,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} }; @@ -3033,7 +3035,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) { @@ -3127,6 +3129,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 561608f1d8..f767f07ed5 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_pg_data_perm($datadir_group, 1)); 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 909bf4d465..f8facc3f00 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_pg_data_perm("$tempdir/data", 0)); +# 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"; + +add_pg_data_group_perm("$tempdir/data"); + +command_ok( + [ 'pg_ctl', 'start', '-D', "$tempdir/data", '-l', $logFileName ], + 'start server to check group permissions'); + +ok(-f $logFileName); +ok(check_pg_data_perm("$tempdir/data", 1)); + +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 db0c2a630a..4a53358f58 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_pg_data_perm($pgdata, 0), '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; +add_pg_data_group_perm($pgdata); + $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_pg_data_perm($pgdata, 1), 'check PGDATA permissions'); + $node->start; diff --git a/src/bin/pg_rewind/RewindTest.pm b/src/bin/pg_rewind/RewindTest.pm index b18a7f954c..10d44fd259 100644 --- a/src/bin/pg_rewind/RewindTest.pm +++ b/src/bin/pg_rewind/RewindTest.pm @@ -116,10 +116,12 @@ sub check_query sub setup_cluster { my $extra_name = shift; + my $group_access = shift; # 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, + has_group_access => defined($group_access) ? $group_access : 0); # 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,7 @@ 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 @@ -258,7 +260,8 @@ recovery_target_timeline='latest' # Clean up after the test. Stop both servers, if they're still running. sub clean_rewind_test { - ok (check_pg_data_perm($node_master->data_dir(), 0)); + ok (check_pg_data_perm( + $node_master->data_dir(), $node_master->group_access())); $node_master->teardown_node if defined $node_master; $node_standby->teardown_node if defined $node_standby; 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/001_basic.pl b/src/bin/pg_rewind/t/001_basic.pl index e7fc200fc0..205bdd77ef 100644 --- a/src/bin/pg_rewind/t/001_basic.pl +++ b/src/bin/pg_rewind/t/001_basic.pl @@ -9,7 +9,7 @@ sub run_test { my $test_mode = shift; - RewindTest::setup_cluster($test_mode); + RewindTest::setup_cluster($test_mode, 1); RewindTest::start_master(); # Create a test table and insert a row in master. 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..2be76913cd --- /dev/null +++ b/src/common/file_perm.c @@ -0,0 +1,50 @@ +/*------------------------------------------------------------------------- + * + * 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 + * + *------------------------------------------------------------------------- + */ +#include "postgres_fe.h" + +#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. + */ +#ifdef FRONTEND + +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; +} + +#endif /* FRONTEND */ diff --git a/src/include/common/file_perm.h b/src/include/common/file_perm.h index 5b823cbd39..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. + * Default mode for directories created with MakeDirectoryDefaultPerm(). */ #define PG_DIR_MODE_DEFAULT (S_IRWXU | S_IRGRP | S_IXGRP) +#ifdef FRONTEND + +/* + * Determine the mask to use when writing to PGDATA + */ +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..8e9c6c11f0 100644 --- a/src/test/perl/PostgresNode.pm +++ b/src/test/perl/PostgresNode.pm @@ -269,6 +269,20 @@ sub connstr =pod +=item $node->group_access() + +Does the data dir allow group access? + +=cut + +sub group_access +{ + my ($self) = @_; + return $self->{_group_access}; +} + +=pod + =item $node->data_dir() Returns the path to the data directory. postgresql.conf and pg_hba.conf are @@ -406,10 +420,16 @@ sub init $params{allows_streaming} = 0 unless defined $params{allows_streaming}; $params{has_archiving} = 0 unless defined $params{has_archiving}; + $params{has_group_access} = 0 unless defined $params{has_group_access}; mkdir $self->backup_dir; mkdir $self->archive_dir; + if ($params{has_group_access}) + { + push(@{$params{extra}}, '-g'); + } + TestLib::system_or_bail('initdb', '-D', $pgdata, '-A', 'trust', '-N', @{ $params{extra} }); TestLib::system_or_bail($ENV{PG_REGRESS}, '--config-auth', $pgdata); @@ -460,8 +480,12 @@ sub init } close $conf; + chmod($params{has_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}; + $self->{_group_access} = 1 if $params{has_group_access}; } =pod @@ -485,7 +509,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 acccecc5de..3d404eefac 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_pg_data_perm + add_pg_data_group_perm check_pg_config system_or_bail system_log @@ -303,6 +304,29 @@ sub check_pg_data_perm return $result; } +# Add group permissions to a PGDATA directory +sub add_pg_data_group_perm +{ + my ($pgdata) = @_; + + find + ( + sub + { + my $file_stat = stat($File::Find::name); + + defined($file_stat) + or die("unable to stat $file_stat"); + + chmod(S_IMODE($file_stat->mode) | + (S_ISDIR($file_stat->mode) ? 0050 : 0040), + $File::Find::name) + or die "unable to chmod $File::Find::name"; + }, + $pgdata + ); +} + # 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.
signature.asc
Description: OpenPGP digital signature