Re: PATCH: Configurable file mode mask

Greetings,

* David Steele (david(at)pgmasters(dot)net) wrote:
> 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.

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.

Thanks!

Stephen