On 3/13/18 11:00 AM, Tom Lane wrote:
> Stephen Frost <sfr...@snowman.net> writes:
>> * Michael Paquier (mich...@paquier.xyz) wrote:
>>> If the problem is parsing, it could as well be more portable to put that
>>> in the control file, no?
>> Then we'd need a tool to allow changing it for people who do want to
>> change it.  There's no reason we should have to have an extra tool for
>> this- an administrator who chooses to change the privileges on the data
>> folder should be able to do so and I don't think anyone is going to
>> thank us for requiring them to use some special tool to do so for
>> PostgreSQL.
> FWIW, I took a quick look through this patch and don't have any problem
> with the approach, which appears to be "use the data directory's
> startup-time permissions as the status indicator".  I am kinda wondering
> about this though:
> +    (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.
> If we're allowing group read on the data directory, why should that not
> include postmaster.pid?  There's nothing terribly security-relevant in
> there, and being able to find out the postmaster PID seems useful.  I can
> see the point of restricting server key files, as documented elsewhere,
> but it's possible to keep those somewhere else so they don't cause
> problems for simple backup software.

I'm OK with that.  I had a look at the warnings regarding the required
mode of postmaster.pid in miscinit.c (889-911) and it seems to me they
still hold true if the mode is 640 instead of 600.

Do you agree, Tom?  Stephen?

If so, I'll make those changes.

> Also, in general, this patch desperately needs a round of copy-editing,
> particularly with attention to the comments.  For instance, there are a
> minimum of three grammatical errors in this one comment:
> + * 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.

> In a larger sense, this fails to explain the operating principle,
> namely what I stated above, that we allow the existing permissions
> on PGDATA to decide what we allow as group permissions.  If you
> don't already understand that, you're going to have a hard time
> extracting it from either file_perm.h or file_perm.c.  (The latter
> fails to even explain what the argument of DataDirectoryMask is.)
I'll do comment/doc review for the next round of patches.


Reply via email to