From 45aa81b70282f34b89f87ee9a45de1af4fdf25da Mon Sep 17 00:00:00 2001 From: David Steele Date: Tue, 27 Mar 2018 16:10:22 -0400 Subject: [PATCH 2/2] 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/ref/initdb.sgml | 19 +++++++++ doc/src/sgml/runtime.sgml | 14 ++++++- src/backend/commands/tablespace.c | 2 +- src/backend/postmaster/postmaster.c | 39 ++++++++++++----- src/backend/replication/basebackup.c | 4 +- src/backend/utils/init/globals.c | 6 +++ src/backend/utils/init/miscinit.c | 30 ++++++++------ src/backend/utils/misc/guc.c | 11 +++++ src/bin/initdb/initdb.c | 22 ++++++---- src/bin/initdb/t/001_initdb.pl | 13 +++++- src/bin/pg_basebackup/pg_basebackup.c | 26 ++++++++---- src/bin/pg_basebackup/pg_receivewal.c | 11 +++++ src/bin/pg_basebackup/pg_recvlogical.c | 11 +++++ src/bin/pg_basebackup/streamutil.c | 62 ++++++++++++++++++++++++++++ src/bin/pg_basebackup/streamutil.h | 2 + src/bin/pg_basebackup/t/010_pg_basebackup.pl | 26 ++++++++---- src/bin/pg_ctl/pg_ctl.c | 5 ++- src/bin/pg_ctl/t/001_start_stop.pl | 24 ++++++++++- src/bin/pg_resetwal/pg_resetwal.c | 4 +- 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 | 27 +++++++++--- src/include/miscadmin.h | 2 + src/test/perl/PostgresNode.pm | 27 +++++++++++- src/test/perl/TestLib.pm | 25 +++++++++++ src/tools/msvc/Mkvcbuild.pm | 2 +- 31 files changed, 425 insertions(+), 77 deletions(-) create mode 100644 src/common/file_perm.c diff --git a/doc/src/sgml/ref/initdb.sgml b/doc/src/sgml/ref/initdb.sgml index 949b5a220f..1d41e91794 100644 --- a/doc/src/sgml/ref/initdb.sgml +++ b/doc/src/sgml/ref/initdb.sgml @@ -76,6 +76,14 @@ PostgreSQL documentation to do so.) + + For security reasons the new cluster created by initdb + will only be accessible by the cluster owner by default. The + 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. + + initdb initializes the database cluster's default locale and character set encoding. The character set encoding, @@ -301,6 +309,17 @@ PostgreSQL documentation + + + + + + Allows users in the same group as the cluster owner to read all cluster + files created by initdb. + + + + 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$ initdb -D /usr/local/pgsql/data database, it is essential that it be secured from unauthorized access. initdb therefore revokes access permissions from everyone but the - PostgreSQL user. + PostgreSQL user, and optionally, group. + Group access, when enabled, is read-only. This allows an unprivileged + user in the PostgreSQL group to take a backup of + the cluster data or perform other operations that only require read access. @@ -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. + + 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. + + 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/commands/tablespace.c b/src/backend/commands/tablespace.c index f7e1818350..2a844a56e6 100644 --- a/src/backend/commands/tablespace.c +++ b/src/backend/commands/tablespace.c @@ -576,7 +576,7 @@ 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, PG_DIR_MODE_DEFAULT) != 0) + if (chmod(location, DataDirMode()) != 0) { if (errno == ENOENT) ereport(ERROR, diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index 868fba8cac..46b705be29 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -588,7 +588,9 @@ PostmasterMain(int argc, char *argv[]) IsPostmasterEnvironment = true; /* - * for security, no dir or file created can be group or other accessible + * By default, no directory or file created can be group or other + * accessible. This may be modified later depending on the permissions of + * the data directory. */ umask(PG_MODE_MASK_DEFAULT); @@ -1522,25 +1524,42 @@ checkDataDir(void) #endif /* - * Check if the directory has group or world access. If so, reject. + * Check if the directory has correct permissions. If not, 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. + * Only two possible modes are allowed, 0700 and 0750. The latter mode + * indicates that group read/execute should be allowed on all newly created + * files and directories. * * 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. The mask was set earlier in startup to disallow group + * permissions on newly created files and directories. However, if group + * read/execute are present on the data directory then modify the mask to + * allow group read/execute on newly created files and directories and set + * the data_directory_group_access GUC to true. + * + * Suppress when on Windows, because there may not be proper support + * for Unix-y file permissions. + */ +#if !defined(WIN32) && !defined(__CYGWIN__) + if ((stat_buf.st_mode & PG_DIR_MODE_DEFAULT) == PG_DIR_MODE_DEFAULT) + { + umask(PG_MODE_MASK_ALLOW_GROUP); + DataDirGroupAccess = true; + } #endif /* Look for PG_VERSION before looking for pg_control */ diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c index e4a80edb8a..e0ae5f33d2 100644 --- a/src/backend/replication/basebackup.c +++ b/src/backend/replication/basebackup.c @@ -876,7 +876,7 @@ sendFileWithContent(const char *filename, const char *content) statbuf.st_gid = getegid(); #endif statbuf.st_mtime = time(NULL); - statbuf.st_mode = PG_FILE_MODE_DEFAULT; + statbuf.st_mode = DataDirMode() & PG_FILE_MODE_DEFAULT; statbuf.st_size = len; _tarWriteHeader(filename, NULL, &statbuf, false); @@ -1395,7 +1395,7 @@ _tarWriteDir(const char *pathbuf, int basepathlen, struct stat *statbuf, #else if (pgwin32_is_junction(pathbuf)) #endif - statbuf->st_mode = S_IFDIR | PG_DIR_MODE_DEFAULT; + statbuf->st_mode = S_IFDIR | DataDirMode(); return _tarWriteHeader(pathbuf + basepathlen + 1, NULL, statbuf, sizeonly); } diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c index 446040d816..2ac923c3ee 100644 --- a/src/backend/utils/init/globals.c +++ b/src/backend/utils/init/globals.c @@ -59,6 +59,12 @@ struct Latch *MyLatch; */ char *DataDir = NULL; +/* + * Does the data directory allow group read access? The default is false, i.e. + * mode 0700, but may be changed in checkDataDir() to true, i.e. mode 0750. + */ +bool DataDirGroupAccess = false; + char OutputFileName[MAXPGPATH]; /* debugging output file */ char my_exec_path[MAXPGPATH]; /* full path to my executable */ diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c index 82da4b9f27..627a5da49a 100644 --- a/src/backend/utils/init/miscinit.c +++ b/src/backend/utils/init/miscinit.c @@ -125,6 +125,15 @@ ChangeToDataDir(void) DataDir))); } +/* + * Get mode of DataDir which can be either 700 or 750. + */ +int +DataDirMode(void) +{ + return DataDirGroupAccess ? PG_DIR_MODE_DEFAULT : + PG_DIR_MODE_DEFAULT & ~PG_MODE_MASK_DEFAULT; +} /* ---------------------------------------------------------------- * User ID state @@ -829,7 +838,7 @@ CreateLockFile(const char *filename, bool amPostmaster, /* * Try to create the lock file --- O_EXCL makes this atomic. * - * Think not to make the file protection weaker than 0600. See + * Think not to make the file protection weaker than 0600/0640. See * comments below. */ fd = open(filename, O_RDWR | O_CREAT | O_EXCL, PG_FILE_MODE_DEFAULT); @@ -899,17 +908,14 @@ CreateLockFile(const char *filename, bool amPostmaster, * implies that the existing process has a different userid than we * do, which means it cannot be a competing postmaster. A postmaster * cannot successfully attach to a data directory owned by a userid - * other than its own. (This is now checked directly in - * checkDataDir(), but has been true for a long time because of the - * restriction that the data directory isn't group- or - * world-accessible.) Also, since we create the lockfiles mode 600, - * we'd have failed above if the lockfile belonged to another userid - * --- which means that whatever process kill() is reporting about - * isn't the one that made the lockfile. (NOTE: this last - * consideration is the only one that keeps us from blowing away a - * Unix socket file belonging to an instance of Postgres being run by - * someone else, at least on machines where /tmp hasn't got a - * stickybit.) + * other than its own, as enforced in checkDataDir(). Also, since we + * create the lockfiles mode 0600/0640, we'd have failed above if the + * lockfile belonged to another userid --- which means that whatever + * process kill() is reporting about isn't the one that made the + * lockfile. (NOTE: this last consideration is the only one that keeps + * us from blowing away a Unix socket file belonging to an instance of + * Postgres being run by someone else, at least on machines where /tmp + * hasn't got a stickybit.) */ if (other_pid != my_pid && other_pid != my_p_pid && other_pid != my_gp_pid) diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index d075cb139a..42501bd317 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -1694,6 +1694,17 @@ static struct config_bool ConfigureNamesBool[] = NULL, NULL, NULL }, + { + {"data_directory_group_access", PGC_INTERNAL, PRESET_OPTIONS, + gettext_noop("Shows whether the data directory allows group read access."), + NULL, + GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE + }, + &DataDirGroupAccess, + false, + NULL, NULL, NULL + }, + { {"syslog_sequence_numbers", PGC_SIGHUP, LOGGING_WHERE, gettext_noop("Add sequence number to syslog messages to avoid duplicate suppression."), diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c index 0b69eded81..8d16232693 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 dir/file mode 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_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c index 3d6e47008a..9c587ebdc4 100644 --- a/src/bin/pg_basebackup/pg_basebackup.c +++ b/src/bin/pg_basebackup/pg_basebackup.c @@ -2448,14 +2448,6 @@ main(int argc, char **argv) } #endif - /* - * Verify that the target directory exists, or create it. For plaintext - * backups, always require the directory. For tar backups, require it - * unless we are writing to stdout. - */ - if (format == 'p' || strcmp(basedir, "-") != 0) - verify_dir_is_empty_or_create(basedir, &made_new_pgdata, &found_existing_pgdata); - /* connection in replication mode to server */ conn = GetConnection(); if (!conn) @@ -2464,6 +2456,24 @@ main(int argc, char **argv) exit(1); } + /* + * Determine if group access is enabled for the source data directory and + * if so enable it for any created directories and files by setting the + * umask appropriately. + */ + if (!RetrieveDataDirGroupAccess(conn)) + disconnect_and_exit(1); + + umask(DataDirGroupAccess ? PG_MODE_MASK_ALLOW_GROUP : PG_MODE_MASK_DEFAULT); + + /* + * Verify that the target directory exists, or create it. For plaintext + * backups, always require the directory. For tar backups, require it + * unless we are writing to stdout. + */ + if (format == 'p' || strcmp(basedir, "-") != 0) + verify_dir_is_empty_or_create(basedir, &made_new_pgdata, &found_existing_pgdata); + /* determine remote server's xlog segment size */ if (!RetrieveWalSegSize(conn)) disconnect_and_exit(1); diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c index b82e073b86..1e36f02553 100644 --- a/src/bin/pg_basebackup/pg_receivewal.c +++ b/src/bin/pg_basebackup/pg_receivewal.c @@ -19,6 +19,7 @@ #include #include +#include "common/file_perm.h" #include "libpq-fe.h" #include "access/xlog_internal.h" #include "getopt_long.h" @@ -703,6 +704,16 @@ main(int argc, char **argv) if (!RunIdentifySystem(conn, NULL, NULL, NULL, &db_name)) disconnect_and_exit(1); + /* + * Determine if group access is enabled for the source data directory and + * if so enable it for any created directories and files by setting the + * umask appropriately. + */ + if (!RetrieveDataDirGroupAccess(conn)) + disconnect_and_exit(1); + + umask(DataDirGroupAccess ? PG_MODE_MASK_ALLOW_GROUP : PG_MODE_MASK_DEFAULT); + /* determine remote server's xlog segment size */ if (!RetrieveWalSegSize(conn)) disconnect_and_exit(1); diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c index 53e4661d68..04aa2cf260 100644 --- a/src/bin/pg_basebackup/pg_recvlogical.c +++ b/src/bin/pg_basebackup/pg_recvlogical.c @@ -23,6 +23,7 @@ #include "streamutil.h" #include "access/xlog_internal.h" +#include "common/file_perm.h" #include "common/fe_memutils.h" #include "getopt_long.h" #include "libpq-fe.h" @@ -959,6 +960,16 @@ main(int argc, char **argv) disconnect_and_exit(1); } + /* + * Determine if group access is enabled for the source data directory and + * if so enable it for any created directories and files by setting the + * umask appropriately. + */ + if (!RetrieveDataDirGroupAccess(conn)) + disconnect_and_exit(1); + + umask(DataDirGroupAccess ? PG_MODE_MASK_ALLOW_GROUP : PG_MODE_MASK_DEFAULT); + /* Drop a replication slot. */ if (do_drop_slot) { diff --git a/src/bin/pg_basebackup/streamutil.c b/src/bin/pg_basebackup/streamutil.c index 1438f368ed..6da9f5f423 100644 --- a/src/bin/pg_basebackup/streamutil.c +++ b/src/bin/pg_basebackup/streamutil.c @@ -32,9 +32,17 @@ uint32 WalSegSz; +/* Does the source data directory have group access? */ +bool DataDirGroupAccess = false; + /* SHOW command for replication connection was introduced in version 10 */ #define MINIMUM_VERSION_FOR_SHOW_CMD 100000 +/* + * Group access is supported from version 11. + */ +#define MINIMUM_VERSION_FOR_GROUP_ACCESS 110000 + const char *progname; char *connection_string = NULL; char *dbhost = NULL; @@ -327,6 +335,60 @@ RetrieveWalSegSize(PGconn *conn) return true; } +/* + * From version 11, check if group access is enabled on the source data + * directory. + */ +bool +RetrieveDataDirGroupAccess(PGconn *conn) +{ + PGresult *res; + + /* check connection existence */ + Assert(conn != NULL); + + /* for previous versions set the default group access */ + if (PQserverVersion(conn) < MINIMUM_VERSION_FOR_GROUP_ACCESS) + { + DataDirGroupAccess = false; + return false; + } + + res = PQexec(conn, "SHOW data_directory_group_access"); + if (PQresultStatus(res) != PGRES_TUPLES_OK) + { + fprintf(stderr, _("%s: could not send replication command \"%s\": %s\n"), + progname, "SHOW data_directory_group_access", PQerrorMessage(conn)); + + PQclear(res); + return false; + } + if (PQntuples(res) != 1 || PQnfields(res) < 1) + { + fprintf(stderr, + _("%s: could not fetch group access flag: got %d rows and %d fields, expected %d rows and %d or more fields\n"), + progname, PQntuples(res), PQnfields(res), 1, 1); + + PQclear(res); + return false; + } + + /* Fetch group access flag from the result */ + if (strcmp(PQgetvalue(res, 0, 0), "on") == 0) + DataDirGroupAccess = true; + else if (strcmp(PQgetvalue(res, 0, 0), "off") == 0) + DataDirGroupAccess = false; + else + { + fprintf(stderr, _("%s: group access flag could not be parsed: %s\n"), + progname, PQgetvalue(res, 0, 0)); + return false; + } + + PQclear(res); + return true; +} + /* * Run IDENTIFY_SYSTEM through a given connection and give back to caller * some result information if requested: diff --git a/src/bin/pg_basebackup/streamutil.h b/src/bin/pg_basebackup/streamutil.h index 6854bbc31d..927e34fd34 100644 --- a/src/bin/pg_basebackup/streamutil.h +++ b/src/bin/pg_basebackup/streamutil.h @@ -25,6 +25,7 @@ extern char *dbport; extern char *dbname; extern int dbgetpassword; extern uint32 WalSegSz; +extern bool DataDirGroupAccess; /* Connection kept global so we can disconnect easily */ extern PGconn *conn; @@ -42,6 +43,7 @@ extern bool RunIdentifySystem(PGconn *conn, char **sysid, XLogRecPtr *startpos, char **db_name); extern bool RetrieveWalSegSize(PGconn *conn); +extern bool RetrieveDataDirGroupAccess(PGconn *conn); extern TimestampTz feGetCurrentTimestamp(void); extern void feTimestampDifference(TimestampTz start_time, TimestampTz stop_time, long *secs, int *microsecs); diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl index 8d9f9c21f4..ce87f77e37 100644 --- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl +++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl @@ -5,7 +5,7 @@ use Config; use File::Basename qw(basename dirname); use PostgresNode; use TestLib; -use Test::More tests => 94; +use Test::More tests => 95; program_help_ok('pg_basebackup'); program_version_ok('pg_basebackup'); @@ -43,11 +43,6 @@ $node->command_fails( ok(!-d "$tempdir/backup", 'backup directory was cleaned up'); -$node->command_fails([ 'pg_basebackup', '-D', "$tempdir/backup", '-n' ], - 'failing run with no-clean option'); - -ok(-d "$tempdir/backup", 'backup directory was created and left behind'); - open my $conf, '>>', "$pgdata/postgresql.conf"; print $conf "max_replication_slots = 10\n"; print $conf "max_wal_senders = 10\n"; @@ -157,6 +152,13 @@ ok(-f "$tempdir/tarbackup/base.tar", 'backup tar was created'); $node->command_fails( [ 'pg_basebackup', '-D', "$tempdir/backup_foo", '-Fp', "-T=/foo" ], '-T with empty old directory fails'); + +$node->command_fails( + [ 'pg_basebackup', '-D', "$tempdir/backup_foo", '-Fp', "-T=/foo", "-n" ], + 'failing run with no-clean option'); + +ok(-d "$tempdir/backup", 'backup directory was created and left behind'); + $node->command_fails( [ 'pg_basebackup', '-D', "$tempdir/backup_foo", '-Fp', "-T/foo=" ], '-T with empty new directory fails'); @@ -190,11 +192,17 @@ unlink "$pgdata/$superlongname"; # skip on Windows. SKIP: { - skip "symlinks not supported on Windows", 17 if ($windows_os); + skip "symlinks not supported on Windows", 18 if ($windows_os); # Move pg_replslot out of $pgdata and create a symlink to it. $node->stop; + # Set umask so test directories and files are created with group permissions + umask (0027); + + # Enable group permissions on PGDATA + chmod_recursive("$pgdata", 0750, 0640); + rename("$pgdata/pg_replslot", "$tempdir/pg_replslot") or BAIL_OUT "could not move $pgdata/pg_replslot"; symlink("$tempdir/pg_replslot", "$pgdata/pg_replslot") @@ -264,6 +272,10 @@ SKIP: "tablespace symlink was updated"); closedir $dh; + # Group access should be enabled on all backup files + ok(check_mode_recursive("$tempdir/backup1", 0750, 0640), + "check backup dir permissions"); + # Unlogged relation forks other than init should not be copied my ($tblspc1UnloggedBackupPath) = $tblspc1UnloggedPath =~ /[^\/]*\/[^\/]*\/[^\/]*$/g; diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c index e83a7179ea..bb70e625fd 100644 --- a/src/bin/pg_ctl/pg_ctl.c +++ b/src/bin/pg_ctl/pg_ctl.c @@ -2171,7 +2171,7 @@ main(int argc, char **argv) */ argv0 = argv[0]; - /* Set dir/file mode mask */ + /* Set restrictive mode mask until PGDATA permissions are checked */ umask(PG_MODE_MASK_DEFAULT); /* support --help and --version even if invoked as root */ @@ -2407,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..91e7f00326 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)); + +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 49670a0d29..a311bb4128 100644 --- a/src/bin/pg_resetwal/pg_resetwal.c +++ b/src/bin/pg_resetwal/pg_resetwal.c @@ -363,8 +363,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_rewind/RewindTest.pm b/src/bin/pg_rewind/RewindTest.pm index 7b632c7dcd..63d9bd517d 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 BAIL_OUT( "unable to set permissions for $master_pgdata/postgresql.conf"); 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..b490220b64 --- /dev/null +++ b/src/common/file_perm.c @@ -0,0 +1,48 @@ +/*------------------------------------------------------------------------- + * + * File and directory permission routines + * + * + * 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 + +#include "common/file_perm.h" + +/* + * Determine the mask to use when writing to PGDATA by examining the mode of the + * PGDATA directory. If group read/execute are present in the PGDATA directory + * mode, the mask will be relaxed to allow group read/execute on all newly + * created files and directories. + * + * 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 the PGDATA directory cannot be accessed. + */ + 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..2176e5cae5 100644 --- a/src/include/common/file_perm.h +++ b/src/include/common/file_perm.h @@ -1,6 +1,6 @@ /*------------------------------------------------------------------------- * - * File and directory permission constants + * File and directory permission constants and routines * * * Portions Copyright (c) 1996-2018, PostgreSQL Global Development Group @@ -20,13 +20,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_FILE_MODE_DEFAULT (S_IRUSR | S_IWUSR) +#define PG_MODE_MASK_ALLOW_GROUP (S_IWGRP | S_IRWXO) /* - * Default mode for directories. + * Default mode for created files, unless something else is specified using + * the *Perm() function variants. */ -#define PG_DIR_MODE_DEFAULT S_IRWXU +#define PG_FILE_MODE_DEFAULT (S_IRUSR | S_IWUSR | S_IRGRP) + +/* + * 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/include/miscadmin.h b/src/include/miscadmin.h index a4574cd533..13d0009573 100644 --- a/src/include/miscadmin.h +++ b/src/include/miscadmin.h @@ -153,6 +153,7 @@ extern PGDLLIMPORT bool IsBinaryUpgrade; extern PGDLLIMPORT bool ExitOnAnyError; extern PGDLLIMPORT char *DataDir; +extern PGDLLIMPORT bool DataDirGroupAccess; extern PGDLLIMPORT int NBuffers; extern PGDLLIMPORT int MaxBackends; @@ -323,6 +324,7 @@ extern void SetCurrentRoleId(Oid roleid, bool is_superuser); extern void SetDataDir(const char *dir); extern void ChangeToDataDir(void); +extern int DataDirMode(void); extern void SwitchToSharedLatch(void); extern void SwitchBackToLocalLatch(void); 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 93610e4bc4..8047404efd 100644 --- a/src/test/perl/TestLib.pm +++ b/src/test/perl/TestLib.pm @@ -31,6 +31,7 @@ our @EXPORT = qw( slurp_file append_to_file check_mode_recursive + chmod_recursive check_pg_config system_or_bail system_log @@ -313,6 +314,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 5762c68c4e..c5cf3d2dfb 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)