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@pgmasters.net