Re: PATCH: Configurable file mode mask

From: David Steele <david(at)pgmasters(dot)net>
To: Stephen Frost <sfrost(at)snowman(dot)net>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Michael Paquier <michael(at)paquier(dot)xyz>
Cc: Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, Adam Brightwell <adam(dot)brightwell(at)crunchydata(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, "Tsunakawa, Takayuki" <tsunakawa(dot)takay(at)jp(dot)fujitsu(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: PATCH: Configurable file mode mask
Date: 2018-03-14 18:08:19
Message-ID: cf97fcb0-31ed-232c-1c51-3c4b8894588d@pgmasters.net
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

Hi,

On 3/13/18 12:13 PM, Stephen Frost wrote:
> * David Steele (david(at)pgmasters(dot)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(at)pgmasters(dot)net

Attachment Content-Type Size
group-access-v11-01-pgresetwal-test.patch text/plain 2.5 KB
group-access-v11-02-file-perm.patch text/plain 31.8 KB
group-access-v11-03-group.patch text/plain 28.3 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2018-03-14 18:09:58 Re: WIP: a way forward on bootstrap data
Previous Message Andres Freund 2018-03-14 17:58:53 Re: ExplainProperty* and units