|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|
|Views:||Raw Message | Whole Thread | Download mbox|
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.
|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|