Обсуждение: current_logfiles not following group access and instead followslog_file_mode permissions

On Tue, Jan 15, 2019 at 03:08:41PM +1100, Haribabu Kommi wrote:
> current_logfiles is a meta data file, that stores the current log writing
> file, and this file presents in the data directory. This file
> doesn't follow the group access mode set at the initdb time, but it
> follows the log_file_mode permissions.
>
> Without group access permissions, backup with group access can lead to
> failure.  Attached patch fix the problem.

initdb enforces log_file_mode to 0640 when using the group mode, still
if one enforces the parameter value then current_logfiles would just
stick with it.  This is not really user-friendly.  This impacts also
normal log files as these get included in base backups if the log path
is within the data folder (not everybody uses an absolute path out of
the data folder for the logs).

One way to think about this is that we may want to worry also about
normal log files and document that one had better be careful with the
setting of log_file_mode?  Still, as we are talking about a file
aiming at storing meta-data for log files, something like what you
suggest can make sense.

When discussing about pg_current_logfile(), I raised the point about
not including as well in base backups which would also address the
problem reported here.  However we decided to keep it because it can
be helpful to know what's the last log file associated to a base
backup for debugging purposes:
https://www.postgresql.org/message-id/50b58f25-ab07-f6bd-7a68-68f29f214ce9@dalibo.com

Instead of what you are proposing, why not revisiting that and just
exclude the file from base backups.  I would be in favor of just doing
that instead of switching the file's permission from log_file_mode to
pg_file_create_mode.
--
Michael

Haribabu Kommi <kommi.haribabu@gmail.com> writes:
> Excluding the file in the backup can solve the problem of backup by an
> unprivileged user. Is there any scenarios it can cause problems if it
> doesn't follow the group access mode?

The point of this file, as I understood it, was to allow someone who's
allowed to read the log files to find out which one is the latest.  It
makes zero sense for it to have different permissions from the log files,
because doing that would break its only use-case.

I am wondering what is the use-case for a backup arrangement that's so
fragile it can't cope with varying permissions in the data directory.

            regards, tom lane

I wrote:
> Haribabu Kommi <kommi.haribabu@gmail.com> writes:
>> Excluding the file in the backup can solve the problem of backup by an
>> unprivileged user. Is there any scenarios it can cause problems if it
>> doesn't follow the group access mode?

> The point of this file, as I understood it, was to allow someone who's
> allowed to read the log files to find out which one is the latest.  It
> makes zero sense for it to have different permissions from the log files,
> because doing that would break its only use-case.

On reflection, maybe the problem is not that we're giving the file
the wrong permissions, but that we're putting it in the wrong place?
That is, seems like it should be in the logfile directory not the
data directory.  That would certainly simplify the intended use-case,
and it would fix this complaint too.

            regards, tom lane

On Tue, Jan 15, 2019 at 10:53:30AM -0500, Tom Lane wrote:
> On reflection, maybe the problem is not that we're giving the file
> the wrong permissions, but that we're putting it in the wrong place?
> That is, seems like it should be in the logfile directory not the
> data directory.  That would certainly simplify the intended use-case,
> and it would fix this complaint too.

Yeah, thinking more on this one using for this file different
permissions than the log files makes little sense, so what you propose
here seems like a sensible thing to do things.  Even if we exclude the
file from native BASE_BACKUP this would not solve the case of custom
backup solutions doing their own copy of things, when they rely on
group-read permissions.  This would not solve completely the problem
anyway if log files are in the data folder, but it would address the
case where the log files are in an absolute path out of the data
folder.

I am adding in CC Gilles who implemented current_logfiles for his
input.
--
Michael

Greetings,

* Michael Paquier (michael@paquier.xyz) wrote:
> On Tue, Jan 15, 2019 at 10:53:30AM -0500, Tom Lane wrote:
> > On reflection, maybe the problem is not that we're giving the file
> > the wrong permissions, but that we're putting it in the wrong place?
> > That is, seems like it should be in the logfile directory not the
> > data directory.  That would certainly simplify the intended use-case,
> > and it would fix this complaint too.
>
> Yeah, thinking more on this one using for this file different
> permissions than the log files makes little sense, so what you propose
> here seems like a sensible thing to do things.  Even if we exclude the
> file from native BASE_BACKUP this would not solve the case of custom
> backup solutions doing their own copy of things, when they rely on
> group-read permissions.  This would not solve completely the problem
> anyway if log files are in the data folder, but it would address the
> case where the log files are in an absolute path out of the data
> folder.

Actually, I agree with the initial patch on the basis that this file
that's being created (which I'm honestly a bit amazed that we're doing
this way; certainly seems rather grotty to me) is surely not an actual
*log* file and therefore using logfile_open() to open it doesn't seem
quite right.  I would have hoped for a way to pass this information that
didn't involve a file at all, but I'll assume that was discussed already
and good reasons put forth as to why we can't avoid it.

I'm not really sure putting it into the logfile directory is such a hot
idea as users might have set up external log file rotation of files in
that directory.  Of course, in that case they'd probably signal PG right
afterwards and PG would go write out a new file, but it still seems
pretty awkward.  I'm not terribly against solving this issue that way
either though, but I tend to think the originally proposed patch is more
sensible.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Michael Paquier (michael@paquier.xyz) wrote:
>> On Tue, Jan 15, 2019 at 10:53:30AM -0500, Tom Lane wrote:
>>> On reflection, maybe the problem is not that we're giving the file
>>> the wrong permissions, but that we're putting it in the wrong place?

> I'm not really sure putting it into the logfile directory is such a hot
> idea as users might have set up external log file rotation of files in
> that directory.  Of course, in that case they'd probably signal PG right
> afterwards and PG would go write out a new file, but it still seems
> pretty awkward.  I'm not terribly against solving this issue that way
> either though, but I tend to think the originally proposed patch is more
> sensible.

I dunno, I think that the current design was made without any thought
whatsoever about the log-files-outside-the-data-directory case.  If
you're trying to set things up that way, it's because you want to give
logfile read access to people who shouldn't be able to look into the
data directory proper.  That makes current_logfiles pretty useless
to such people, as it's now designed.

Now, if the expectation is that current_logfiles is just an internal
working file that users shouldn't access directly, then this argument
is wrong --- but then why is it documented in user-facing docs?

If we're going to accept the patch as-is, then it logically follows
that we should de-document current_logfiles, because we're taking the
position that it's an internal temporary file not meant for user access.

I don't really believe your argument about log rotation: a rotator
would presumably be configured either to pay attention to file name
patterns (which current_logfiles wouldn't match) or to file age
(which current_logfiles shouldn't satisfy either, since it's always
rewritten when we switch logfiles).

If we wanted to worry about that case, a possible solution is to make the
current_logfiles pathname user-configurable so it could be put in some
third directory.  But I think that adds more complexity than is justified
--- and not just for us, but for programs trying to find and use
current_logfiles.

            regards, tom lane

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Michael Paquier (michael@paquier.xyz) wrote:
> >> On Tue, Jan 15, 2019 at 10:53:30AM -0500, Tom Lane wrote:
> >>> On reflection, maybe the problem is not that we're giving the file
> >>> the wrong permissions, but that we're putting it in the wrong place?
>
> > I'm not really sure putting it into the logfile directory is such a hot
> > idea as users might have set up external log file rotation of files in
> > that directory.  Of course, in that case they'd probably signal PG right
> > afterwards and PG would go write out a new file, but it still seems
> > pretty awkward.  I'm not terribly against solving this issue that way
> > either though, but I tend to think the originally proposed patch is more
> > sensible.
>
> I dunno, I think that the current design was made without any thought
> whatsoever about the log-files-outside-the-data-directory case.  If
> you're trying to set things up that way, it's because you want to give
> logfile read access to people who shouldn't be able to look into the
> data directory proper.  That makes current_logfiles pretty useless
> to such people, as it's now designed.

... or you just want to move the log files to a more sensible location
than the data directory.  The justification for log_file_mode existing
is because you might want to have log files with different privileges,
but that's quite a different thing.

> Now, if the expectation is that current_logfiles is just an internal
> working file that users shouldn't access directly, then this argument
> is wrong --- but then why is it documented in user-facing docs?

I really couldn't say why it's documented in the user-facing docs, and
for my 2c I don't really think it should be- there's a function to get
that information.  Sprinkling the data directory with files for users to
access directly doesn't exactly fit my view of what a good API looks
like.

The fact that there isn't any discussion about where that file actually
lives does make me suspect you're right that log files outside the data
directory wasn't really contemplated.

> If we're going to accept the patch as-is, then it logically follows
> that we should de-document current_logfiles, because we're taking the
> position that it's an internal temporary file not meant for user access.

... and hopefully we'd get rid of it one day entirely.

> I don't really believe your argument about log rotation: a rotator
> would presumably be configured either to pay attention to file name
> patterns (which current_logfiles wouldn't match) or to file age
> (which current_logfiles shouldn't satisfy either, since it's always
> rewritten when we switch logfiles).

Yes, a good pattern would avoid picking up on this file and most are
configured that way (though they are maybe not as specific as you might
think- the default here is just /var/log/postgresql/*.log).

> If we wanted to worry about that case, a possible solution is to make the
> current_logfiles pathname user-configurable so it could be put in some
> third directory.  But I think that adds more complexity than is justified
> --- and not just for us, but for programs trying to find and use
> current_logfiles.

I'd much rather move to get rid of that file rather than increase its
visability- programs should be using the provided function.

Thanks!

Stephen

Greetings,

* Haribabu Kommi (kommi.haribabu@gmail.com) wrote:
> On Thu, Jan 17, 2019 at 5:49 AM Stephen Frost <sfrost@snowman.net> wrote:
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> > > Now, if the expectation is that current_logfiles is just an internal
> > > working file that users shouldn't access directly, then this argument
> > > is wrong --- but then why is it documented in user-facing docs?
> >
> > I really couldn't say why it's documented in the user-facing docs, and
> > for my 2c I don't really think it should be- there's a function to get
> > that information.  Sprinkling the data directory with files for users to
> > access directly doesn't exactly fit my view of what a good API looks
> > like.
> >
> > The fact that there isn't any discussion about where that file actually
> > lives does make me suspect you're right that log files outside the data
> > directory wasn't really contemplated.
>
> I can only think of reading this file by the user directly when the server
> is not available, but I don't find any scenario where that is required?

Yeah, I agree, and if the server isn't running then there really isn't
a "current" logfile, as defined, since the server isn't writing to any
particular log file.

> > > If we're going to accept the patch as-is, then it logically follows
> > > that we should de-document current_logfiles, because we're taking the
> > > position that it's an internal temporary file not meant for user access.
> >
> > ... and hopefully we'd get rid of it one day entirely.
>
> If there is no use of it when server is offline, it will be better to
> remove that
> file with an alternative to provide the current log file name.

It'd probably be good to give folks an opportunity to voice their
opinion regarding their use-case for the file existing as it does and
being documented as it is.  At first blush, to me anyway, it seems like
maybe this was a case of "over-documenting" of the feature by including
in user-facing documentation something that was really there for
internal reasons, but I could certainly be wrong and maybe there's a
reason why it's really necessary to have the file around for users.

> With group access mode, the default value of log_file_mode is changed,
> Attached patch reflects the same in docs.

Yes, we should update the documentation in this regard, though it's
really an independent thing as that documentation should have been
updated in the original group-access patch, so I'll see about fixing
it and back-patching it.

Thanks!

Stephen

On Fri, Jan 18, 2019 at 09:50:40AM -0500, Stephen Frost wrote:
> It'd probably be good to give folks an opportunity to voice their
> opinion regarding their use-case for the file existing as it does and
> being documented as it is.  At first blush, to me anyway, it seems like
> maybe this was a case of "over-documenting" of the feature by including
> in user-facing documentation something that was really there for
> internal reasons, but I could certainly be wrong and maybe there's a
> reason why it's really necessary to have the file around for users.

It's not only that.  By keeping the file in its current location, we
can prevent base backups to work even if logs files are out of the
data folder, which is rather user-friendly, and I think that advanced
users of Postgres are careful enough to split log files and main data
folders into different partitions, without symlinks from the data
folder to the log location and with log_directory set to an absolute
path, independent of the rest.  So moving current_logfiles out of the
data folder to the base location of the log paths makes quite some
sense in my opinion for consistency.

Using a new GUC to specify where current_logfiles should be located
does not really justify the code complications in my opinion, and I'd
think that we should allow users with log file access to still look at
it, even manually and connected from the host as this can be useful
for debugging purposes (sometimes clocks of systems get changed as
they are not all the time going throuhg ntpd).
--
Michael

Greetings,

* Michael Paquier (michael@paquier.xyz) wrote:
> On Fri, Jan 18, 2019 at 09:50:40AM -0500, Stephen Frost wrote:
> > It'd probably be good to give folks an opportunity to voice their
> > opinion regarding their use-case for the file existing as it does and
> > being documented as it is.  At first blush, to me anyway, it seems like
> > maybe this was a case of "over-documenting" of the feature by including
> > in user-facing documentation something that was really there for
> > internal reasons, but I could certainly be wrong and maybe there's a
> > reason why it's really necessary to have the file around for users.
>
> It's not only that.  By keeping the file in its current location, we
> can prevent base backups to work even if logs files are out of the
> data folder, which is rather user-friendly, and I think that advanced
> users of Postgres are careful enough to split log files and main data
> folders into different partitions, without symlinks from the data
> folder to the log location and with log_directory set to an absolute
> path, independent of the rest.  So moving current_logfiles out of the
> data folder to the base location of the log paths makes quite some
> sense in my opinion for consistency.

As discussed up-thread, if we change current_logfiles to work the way
the rest of our data files do, then base backups would work fine with
the file in its current location.  I don't buy how having that file in
the logfiles directory is more "consistent" with anything either- it's
certainly not a log file itself.

> Using a new GUC to specify where current_logfiles should be located
> does not really justify the code complications in my opinion, and I'd
> think that we should allow users with log file access to still look at
> it, even manually and connected from the host as this can be useful
> for debugging purposes (sometimes clocks of systems get changed as
> they are not all the time going throuhg ntpd).

I agree that we don't need a new GUC for this.  I also don't really see
the use-case for this file being directly exposed to users- we have a
function specifically for this information and that's generally how
users should expect to get information like this- or like what the log
directory *is* to begin with, or where other files reside... I sure hope
that we aren't suggesting that asking users to write a parser for
postgresql.conf, with include directories and files, able to also handle
postgresql.auto.conf, is somehow user-friendly.

Thanks!

Stephen

On Fri, Jan 18, 2019 at 09:50:40AM -0500, Stephen Frost wrote:
> Yes, we should update the documentation in this regard, though it's
> really an independent thing as that documentation should have been
> updated in the original group-access patch, so I'll see about fixing
> it and back-patching it.

Stephen, could you apply Hari's patch then?  I am not sure what the
consensus is, but documenting the restriction is the minimum we can
do.

-    The default permissions are <literal>0600</literal>, meaning only the
-    server owner can read or write the log files.  The other commonly
-    useful setting is <literal>0640</literal>, allowing members of the owner's
-    group to read the files.  Note however that to make use of such a
-    setting, you'll need to alter <xref linkend="guc-log-directory"/> to
-    store the files somewhere outside the cluster data directory.  In
-    any case, it's unwise to make the log files world-readable, since
-    they might contain sensitive data.
+    The default permissions are either <literal>0600</literal>, meaning only the
+    server owner can read or write the log files or <literal>0640</literal>, that
+    allows any user in the same group can read the log files, based on the new
+    cluster created with <option>--allow-group-access</option> option of <command>initdb</command>
+    command. Note however that to make use of any setting other than default,
+    you'll need to alter <xref linkend="guc-log-directory"/> to store the files
+    somewhere outside the cluster data directory.

I would formulate that differently, by just adding an extra paragraph
to mention that using <literal>0640</literal> is recommended to be
compatible with initdb's --allow-group-access instead of sticking it
on the middle of the existing paragraph.
--
Michael

On Tue, Feb 26, 2019 at 12:22:53PM +1100, Haribabu Kommi wrote:
> I checked the code why the current_logfiles is not implemented as
> shared memory and found that the current syslogger doesn't attach to
> the shared memory of the postmaster. To support storing the
> current_logfiles in shared memory, the syslogger process also needs
> to attach to the shared memory, this seems to be a new
> infrastructure change.

I don't think you can do that anyway and we should not do it.  Shared
memory can be reset after a backend exits abnormally, but the
syslogger lives across that.  What you sent upthread to improve the
documentation is in my opinion sufficient:
https://www.postgresql.org/message-id/CAJrrPGe-v2_LMFD9nHrBEjJy3vVOKJwY3w_h+Fs2nxCJg3PbaA@mail.gmail.com

I would not have split the paragraph you broke into two, but instead
just add this part in-between:
+       <para>
+        Permissions <literal>0640</literal> are recommended to be compatible with
+        <application>initdb</application> option <option>--allow-group-access</option>.
+       </para>
Any objections in doing that?
--
Michael

On Tue, Mar 12, 2019 at 2:03 AM Michael Paquier <michael@paquier.xyz> wrote:
> On Tue, Feb 26, 2019 at 12:22:53PM +1100, Haribabu Kommi wrote:
> > I checked the code why the current_logfiles is not implemented as
> > shared memory and found that the current syslogger doesn't attach to
> > the shared memory of the postmaster. To support storing the
> > current_logfiles in shared memory, the syslogger process also needs
> > to attach to the shared memory, this seems to be a new
> > infrastructure change.
>
> I don't think you can do that anyway and we should not do it.  Shared
> memory can be reset after a backend exits abnormally, but the
> syslogger lives across that.

I think we should do what Haribabu proposed originally.  Moving
current_logfiles out of the data directory doesn't make sense,
because:

(1) If you're trying to find the log files, having a file that
contains their pathnames in the place where those files are does not
help you.  Having such a file in the known location, namely the data
directory, does.

(2) Someone might have logs from multiple PostgreSQL clusters in the
same external log directory, but there can only be one file named
current_logfiles.

(3) Someone might store PostgreSQL log files in the same directory as
non-PostgreSQL log files, and having a file called current_logfiles
floating around will be confusingly ambiguous.

On the other hand, changing the file to have the same permissions as
everything else in the data directory has basically no disadvantages.
I agree with Stephen's analysis that a file containing the names of
the current log files is not itself a log file.  Tom's idea that
making the permissions consistent with everything else in the data
directory would "break its only use-case" seems completely wrong.
Anybody who has permission to read the log files but not the data
directory will presumably hit the directory-level permissions on
$PGDATA before the issue of the permissions on current_logfiles() per
se become relevant, except in corner cases that I don't care about.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Mar 12, 2019 at 04:08:53PM -0400, Robert Haas wrote:
> Anybody who has permission to read the log files but not the data
> directory will presumably hit the directory-level permissions on
> $PGDATA before the issue of the permissions on current_logfiles() per
> se become relevant, except in corner cases that I don't care about.

Sane deployments normally split the log directory and the main data
folder into separate partitions, and use an absolute path for
log_directory.  So, FWIW, I can live with the original proposal as
well.
--
Michael

On Fri, Mar 15, 2019 at 06:51:37PM +1100, Haribabu Kommi wrote:
> IMO, this update is just a recommendation to the user, and sometimes it is
> still possible that there may be strict permissions for the log file
> even the data directory is allowed for the group access. So I feel
> it is still better to update the permissions of the current_logfiles
> to the database files permissions than log file permissions.

I was just reading again this thread, and the suggestions that
current_logfiles is itself not a log file is also a sensible
position.  I was just looking at the patch that you sent at the top of
the thread here:
https://www.postgresql.org/message-id/CAJrrPGcEotF1P7AWoeQyD3Pqr-0xkQg_Herv98DjbaMj+naozw@mail.gmail.com

And actually it seems to me that you have a race condition in that
stuff.  I think that you had better use umask(), then fopen, and then
once again umask() to put back the previous permissions, removing the
extra chmod() call.
--
Michael

On Thu, Mar 21, 2019 at 12:52:14PM +1100, Haribabu Kommi wrote:
> Earlier attached patch is wrong.

-   oumask = umask(pg_file_create_mode);
+   oumask = umask(pg_mode_mask);
Indeed that was wrong.

> Correct patch attached. Sorry for the inconvenience.

This looks better for the umask setting, still it could be more
simple.

 #include <sys/time.h>
-
+#include "common/file_perm.h"
 #include "lib/stringinfo.h"
Nit: it is better for readability to keep an empty line between the
system includes and the Postgres ones.

A second thing, more important, is that you can reset umask just after
opening the file, as attached.  This way there is no need to reset the
umask in all the code paths leaving update_metainfo_datafile().  Does
that look fine to you?
--
Michael

On Fri, Mar 22, 2019 at 02:35:41PM +1100, Haribabu Kommi wrote:
> Thanks for the correction.  Yes, that is correct and it works fine.

Thanks for double-checking.  Are there any objections with this patch?
--
Michael

On Fri, Mar 22, 2019 at 01:01:44PM +0900, Michael Paquier wrote:
> On Fri, Mar 22, 2019 at 02:35:41PM +1100, Haribabu Kommi wrote:
> > Thanks for the correction.  Yes, that is correct and it works fine.
>
> Thanks for double-checking.  Are there any objections with this patch?

Done and committed down to v11 where group access has been added.
There could be an argument to do the same in v10, but as the root of
the problem is the interaction between a data folder using 0640 as
base mode for files and log_file_mode being more restrictive, then it
cannot apply to v10.

After testing and reviewing the patch, I noticed that all versions
sent up to now missed two things done by logfile_open():
- Bufferring is line-buffered.  For current_logfiles it may not matter
much as the contents are first written into a temporary file and then
the file is renamed, but for debugging having the insurance of
consistent contents is nice even for the temporary file.
- current_logfiles uses \r\n.  While it does not have a consequence
for the parsing of the file by pg_current_logfile, it breaks the
readability of the file on Windows, which is not nice.
So I have kept the patch with the previous defaults for consistency.
Perhaps they could be changed, but the current set is a good set.
--
Michael

On Sun, Mar 24, 2019 at 09:16:44PM +0900, Michael Paquier wrote:
> After testing and reviewing the patch, I noticed that all versions
> sent up to now missed two things done by logfile_open():
> - Bufferring is line-buffered.  For current_logfiles it may not matter
> much as the contents are first written into a temporary file and then
> the file is renamed, but for debugging having the insurance of
> consistent contents is nice even for the temporary file.
> - current_logfiles uses \r\n.  While it does not have a consequence
> for the parsing of the file by pg_current_logfile, it breaks the
> readability of the file on Windows, which is not nice.
> So I have kept the patch with the previous defaults for consistency.
> Perhaps they could be changed, but the current set is a good set.

By the way, this also fixes a cosmetic issue with a failure in
creating current_logfiles: when update_metainfo_datafile() fails to
create the file, it logs a LOG message, but logfile_open() does the
same thing, so this finishes with two log entries for the same
failure.  v10 still has that issue, I don't think that it is worth
fixing as it has no actual consequence except perhaps bringing some
confusion.
--
Michael