Horton

Thread

Commits

  1. Make current_logfiles use permissions assigned to files in data directory

  1. current_logfiles not following group access and instead follows log_file_mode permissions

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2019-01-15T04:08:41Z

    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.

comments?

Regards,
Haribabu Kommi
Fujitsu Australia

  2. Re: current_logfiles not following group access and instead follows log_file_mode permissions

    Michael Paquier <michael@paquier.xyz> — 2019-01-15T05:15:50Z

    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

  3. Re: current_logfiles not following group access and instead follows log_file_mode permissions

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2019-01-15T08:55:35Z

    On Tue, Jan 15, 2019 at 4:15 PM Michael Paquier <michael@paquier.xyz> wrote:

> 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).
>

we got this problem when the log_file_mode is set 0600 but the database
file are with group access permissions. In our scenario, the log files are
outside the data folder, so we faced the problem with current_logfiles
file.


> 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.
>

Yes, with log_file_mode less than 0640 containing the log files inside
the data directory can leads to backup failure. Yes, providing extra
information about group access when log_file_mode is getting chosen.

Another option is how about not letting user to choose less than 0640
when the group access mode is enabled?



> 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.
>

I am not sure how much useful having the details of the log file in the
backup.
It may be useful when there is any problem with backup.

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?

Regards,
Haribabu Kommi
Fujitsu Australia

  4. Re: current_logfiles not following group access and instead follows log_file_mode permissions

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-15T14:47:31Z

    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

  5. Re: current_logfiles not following group access and instead follows log_file_mode permissions

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-15T15:53:30Z

    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

  6. Re: current_logfiles not following group access and instead follows log_file_mode permissions

    Michael Paquier <michael@paquier.xyz> — 2019-01-16T02:08:40Z

    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

  7. Re: current_logfiles not following group access and instead follows log_file_mode permissions

    Stephen Frost <sfrost@snowman.net> — 2019-01-16T18:22:12Z

    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

  8. Re: current_logfiles not following group access and instead follows log_file_mode permissions

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-16T18:39:31Z

    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

  9. Re: current_logfiles not following group access and instead follows log_file_mode permissions

    Stephen Frost <sfrost@snowman.net> — 2019-01-16T18:49:54Z

    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

  10. Re: current_logfiles not following group access and instead follows log_file_mode permissions

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2019-01-18T04:08:15Z

    On Thu, Jan 17, 2019 at 5:49 AM Stephen Frost <sfrost@snowman.net> wrote:

> 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.
>

Thanks for sharing your opinions.

The current_logfiles is used to store the meta data information of current
writing log files, that is different to log files, so giving permissions of
the
log file may not be correct,

> 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?



> > 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.

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

Regards,
Haribabu Kommi
Fujitsu Australia

  11. Re: current_logfiles not following group access and instead follows log_file_mode permissions

    Stephen Frost <sfrost@snowman.net> — 2019-01-18T14:50:40Z

    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

  12. Re: current_logfiles not following group access and instead follows log_file_mode permissions

    Michael Paquier <michael@paquier.xyz> — 2019-01-19T01:49:14Z

    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

  13. Re: current_logfiles not following group access and instead follows log_file_mode permissions

    Stephen Frost <sfrost@snowman.net> — 2019-01-19T15:41:08Z

    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

  14. Re: current_logfiles not following group access and instead follows log_file_mode permissions

    Michael Paquier <michael@paquier.xyz> — 2019-02-01T08:22:19Z

    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

  15. Re: current_logfiles not following group access and instead follows log_file_mode permissions

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2019-02-04T01:16:56Z

    On Fri, Feb 1, 2019 at 7:22 PM Michael Paquier <michael@paquier.xyz> wrote:

> 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.
>

Thanks for the review.
I changed the log_file_mode doc patch as per your comment.

How about the attached?

And regarding current_logfiles permissions, I feel this file should have
permissions of data directory files as it is present in the data directory
whether it stores the information of log file, until this file is completely
removed with another approach to store the log file details.

I am not sure whether this has been already discussed or not? How about
using shared memory to store the log file names? So that we don't need
of this file?

Regards,
Haribabu Kommi
Fujitsu Australia

  16. Re: current_logfiles not following group access and instead follows log_file_mode permissions

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2019-02-26T01:22:53Z

    On Mon, Feb 4, 2019 at 12:16 PM Haribabu Kommi <kommi.haribabu@gmail.com>
wrote:

>
> And regarding current_logfiles permissions, I feel this file should have
> permissions of data directory files as it is present in the data directory
> whether it stores the information of log file, until this file is
> completely
> removed with another approach to store the log file details.
>
> I am not sure whether this has been already discussed or not? How about
> using shared memory to store the log file names? So that we don't need
> of this file?
>

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.

In case if we are not going to change the permissions of the file to group
access mode
instead of if we strict with log_file_mode, I just tried the attached patch
of moving the
current_logfiles patch to the log_directory. The only drawback of this
approach, is incase
if the user changes the log_directory, the current_logfiles is present in
the old log_directory.
I don't see that as a problem.

comments?

Regards,
Haribabu Kommi
Fujitsu Australia

  17. Re: current_logfiles not following group access and instead follows log_file_mode permissions

    Michael Paquier <michael@paquier.xyz> — 2019-03-12T06:03:16Z

    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

  18. Re: current_logfiles not following group access and instead follows log_file_mode permissions

    Robert Haas <robertmhaas@gmail.com> — 2019-03-12T20:08:53Z

    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

  19. Re: current_logfiles not following group access and instead follows log_file_mode permissions

    Michael Paquier <michael@paquier.xyz> — 2019-03-14T04:54:10Z

    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

  20. Re: current_logfiles not following group access and instead follows log_file_mode permissions

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2019-03-15T07:51:37Z

    On Tue, Mar 12, 2019 at 5:03 PM 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.  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?
>

If I remember correctly, in one of the mails, you mentioned that having a
separate
para is better. Attached is the updated patch as per your suggestion.

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.

Regards,
Haribabu Kommi
Fujitsu Australia

  21. Re: current_logfiles not following group access and instead follows log_file_mode permissions

    Michael Paquier <michael@paquier.xyz> — 2019-03-20T05:33:18Z

    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

  22. Re: current_logfiles not following group access and instead follows log_file_mode permissions

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2019-03-21T01:41:16Z

    On Wed, Mar 20, 2019 at 4:33 PM Michael Paquier <michael@paquier.xyz> wrote:

> 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
>

Thanks for the review.


> 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.
>

Changed the patch to use umask() instead of chmod() according to
your suggestion.

updated patch attached.

Regards,
Haribabu Kommi
Fujitsu Australia

  23. Re: current_logfiles not following group access and instead follows log_file_mode permissions

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2019-03-21T01:52:14Z

    On Thu, Mar 21, 2019 at 12:41 PM Haribabu Kommi <kommi.haribabu@gmail.com>
wrote:

>
> On Wed, Mar 20, 2019 at 4:33 PM Michael Paquier <michael@paquier.xyz>
> wrote:
>
>> 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.
>>
>
> Changed the patch to use umask() instead of chmod() according to
> your suggestion.
>
> updated patch attached.
>

Earlier attached patch is wrong.
Correct patch attached. Sorry for the inconvenience.

Regards,
Haribabu Kommi
Fujitsu Australia

  24. Re: current_logfiles not following group access and instead follows log_file_mode permissions

    Michael Paquier <michael@paquier.xyz> — 2019-03-22T01:23:54Z

    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

  25. Re: current_logfiles not following group access and instead follows log_file_mode permissions

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2019-03-22T03:35:41Z

    On Fri, Mar 22, 2019 at 12:24 PM Michael Paquier <michael@paquier.xyz>
wrote:

> 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?
>

Thanks for the correction, Yes, that is correct and it works fine.

Regards,
Haribabu Kommi
Fujitsu Australia

  26. Re: current_logfiles not following group access and instead follows log_file_mode permissions

    Michael Paquier <michael@paquier.xyz> — 2019-03-22T04:01:44Z

    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

  27. Re: current_logfiles not following group access and instead follows log_file_mode permissions

    Michael Paquier <michael@paquier.xyz> — 2019-03-24T12:16:44Z

    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

  28. Re: current_logfiles not following group access and instead follows log_file_mode permissions

    Michael Paquier <michael@paquier.xyz> — 2019-03-24T12:26:45Z

    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

  29. Re: current_logfiles not following group access and instead follows log_file_mode permissions

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2019-03-25T07:19:08Z

    On Sun, Mar 24, 2019 at 11:16 PM Michael Paquier <michael@paquier.xyz>
wrote:

> 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.
>

Thanks Micheal and others.
This really helps to choose the restrictive log file permissions even when
the data directory is enabled with group access.

Regards,
Haribabu Kommi
Fujitsu Australia