Re: Re: PATCH: Configurable file mode mask

On Thu, Dec 28, 2017 at 2:36 PM, David Steele <david(at)pgmasters(dot)net> wrote:
> Attached is a new patch set that should address various concerns raised in
> this thread.
>
> 1) group-access-v3-01-mkdir.patch
>
> Abstracts all mkdir calls in the backend into a MakeDirectory() function
> implemented in fd.c. This did not get committed in September as part of
> 0c5803b450e but I still think it has value. However, I have kept it
> separate to reduce noise in the second patch. The mkdir() calls could also
> be modified to use PG_DIR_MODE_DEFAULT with equivalent results.

+/*
+ * Default mode for created directories, unless something else is specified
+ * using the MakeDirectoryPerm() function variant.
+ */
+#define PG_DIR_MODE_DEFAULT (S_IRWXU)
+#define PG_MODE_MASK_DEFAULT (S_IRWXG | S_IRWXO)

There's only one comment here, but there are two constants that do
different things.

Also, this hunk gets removed again by 02, which is probably OK, but 02
adds the replacement stuff in a different part of the file, which
seems not as good.

I think MakeDirectory() is a good wrapper, but isn't
MakeDirectoryPerm() sort of silly?

> 2) group-access-v3-02-group.patch
>
> This is a "GUC-less" implementation of group read access that depends on the
> mode of the $PGDATA directory to determine which mode to use for subsequent
> writes. The initdb option is preserved to allow group access to be enabled
> when the cluster is initialized.
>
> Only two modes are allowed (700, 750) and the error message on startup is
> hard-coded to address translation concerns.

+ umask(~(stat_buf.st_mode & PG_DIR_MODE_DEFAULT));

Hmm, I wonder if this is going to be 100% portable. Maybe some
obscure platform won't like an argument with all the high bits set.

Also, why should we break what's now one #if into two? That seems
like it's mildly more complicated for no gain.

+ (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.

Is that actually the behavior we want?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company