Re: PATCH: Configurable file mode mask

Hi,

On 3/13/18 12:13 PM, Stephen Frost wrote:
> * David Steele (david@pgmasters.net) wrote:
>> On 3/13/18 11:00 AM, Tom Lane wrote:
>>>
>>> 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.
> 
> I agree that we can still consider an EPERM-error case as being ok even
> with the changes proposed, and with the same-user check happening
> earlier in checkDataDir(), we won't even get to the point of looking at
> the pid file if the userid's don't match.  The historical comment about
> the old datadir permissions can likely just be removed, perhaps replaced
> with a bit more commentary above that check in checkDataDir().  The
> open() call should also fail if we only have group-read privileges on
> the file (0640), but surely the kill() will in any case.

OK, that being the case a new patch set is attached that sets the mode
of postmaster.pid the same as other files in PGDATA.

I also cleaned up / corrected / added comments in various places.

Thanks,
-- 
-David
david@pgmasters.net