Re: PATCH: Configurable file mode mask

On Thu, Mar 29, 2018 at 10:59:59AM -0400, David Steele wrote:
> On 3/28/18 1:59 AM, Michael Paquier wrote:
>> On Tue, Mar 27, 2018 at 04:21:09PM -0400, David Steele wrote:
>>> These updates address Michael's latest review and implement group access
>>> for pg_basebackup, pg_receivewal, and pg_recvlogical. A new internal
>>> GUC, data_directory_group_access, allows remote processes to determine
>>> the correct mode using the existing SHOW protocol command.
>>
>> Some nits from patch 1...
>>
>> + ok (check_mode_recursive($node_master->data_dir(), 0700, 0600));
>> [...]
>> + ok (check_mode_recursive($node_master->data_dir(), 0700, 0600));
>> Incorrect indentations (space after "ok", yes that's a nit...).
>
> Fixed the space, not sure about the indentations?

That was only the space issues. A nit.

>> And more things about patch 1 which are not nits.
>>
>> 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.

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

>> +/*
>> + * 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..

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