Re: PATCH: Configurable file mode mask

On 3/29/18 11:04 PM, Michael Paquier wrote:
> On Thu, Mar 29, 2018 at 10:59:59AM -0400, David Steele wrote:
>> On 3/28/18 1:59 AM, Michael Paquier wrote:
>>
>>> In pg_backup_tar.c, we still have a 0600 hardcoded. You should use
>>> PG_FILE_MODE_DEFAULT there as well.
>>
>> I'm starting to wonder if these changes in pg_dump make sense. The
>> file/tar permissions here do not map directly to anything in the PGDATA
>> directory (since the dump and restore are logical). Perhaps we should
>> be adding a -g option for pg_dump (in a separate patch) if we want this
>> functionality?
>
> Yeah. I am having second thoughts on this one actually. pg_dump
> handles logical backups which require just a connection to Postgres and
> it does not care about the physical state of the relation files. So I
> am dropping my comment, and let's not bother about changing things
> here.

Glad you agree. I have reverted the mode changes in pg_dump.

>>> dsm_impl_posix() can create a new segment with shm_open using as mode
>>> 0600. This is present in pg_dynshmem, which would be included in
>>> backups. First, it seems to me that this should use
>>> PG_FILE_MODE_DEFAULT. Then, with patch 2 it seems to me that if an
>>> instance is using DSM then there is a risk of breaking a simple backup
>>> which uses for example "tar" without --exclude filters with group access
>>> (sometimes scripts are not smart enough to skip the same contents as
>>> base backups). So it seems to me that DSM should be also made more
>>> aware of group access to ease the life of users.
>>
>> Done in patch 1. For patch 2, do you see any issues with the shared
>> memory being group readable from a security perspective? The user can
>> read everything on disk so it's hard to see how it's a problem. Also,
>> if these files are ending up in people's backups...
>
> They would be nuked from the surface of earth when recovery kicks.
> People should filter this folder, which is why any popular Postgres
> backup tool I believe does so, and now so do both pg_rewind and
> pg_basebackup. Still if a user is able to read everything, being able
> to read as well pg_dynshmem does not change much from a security's point
> of view. So consistency makes the most sense to me.

Done.

>>> +/*
>>> + * Does the data directory allow group read access? The default is false, i.e.
>>> + * mode 0700, but may be changed in checkDataDir() to true, i.e. mode 0750.
>>> + */
>>> +bool DataDirGroupAccess = false;
>>>
>>> Instead of a boolean, I would suggest to use an integer, this is more
>>> consistent with log_file_mode.
>>
>> Well, the goal was to make this implementation independent, but I'm not
>> against the idea. Anybody have a preference?
>
> You mean Windows here? Perhaps you are right and just using a boolean
> would be fine. When commenting on that I have been likely wondering
> about potential extensions in the future, like allowing a user to write
> files in a data folder as well if member of the same group as the user
> managing the Postgres intance, like for backup deployment? Perhaps
> that's just a crazy man's thoughts..

Haha! But I think you are right that using the mode is more consistent
with how other GUCs are done. It hardly makes sense to take a stand on
unix-y things here when we use them in other GUCs already.

I have replaced data_directory_group_access with data_directory_mode.

>>> Actually, about that, should we complain
>>> if log_file_mode is set to a value incompatible?
>>
>> I think control of the log file mode should be independent. I usually
>> don't store log files in PGDATA at all. What if we set log_file_mode
>> based on the -g option passed to initdb? That will work well for
>> default installations while providing flexibility to others.
>
> Let's do nothing here as well. This will keep the code more simple, and
> normally-sane deployments put the log directory in an absolute path out
> of the data folder. Normally... So it means that if I create a number
> out of thin air I would imagine that less than 20% of deployments are
> like that.

I decided this made sense to do. It was only a few lines in initdb.c
using a very well established pattern. It would be surprising if log
files did not follow the mode of the rest of PGDATA after initdb -g,
even if it is standard practice to relocate them.

Thanks,
--
-David
david(at)pgmasters(dot)net