Re: PATCH: Configurable file mode mask

On 3/13/18 11:00 AM, Tom Lane wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
>> * Michael Paquier (michael(at)paquier(dot)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.

Thanks,
--
-David
david(at)pgmasters(dot)net