Re: PATCH: Configurable file mode mask - Mailing list pgsql-hackers

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?

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

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

> And then for patch 2, a couple of issues spotted..
>
> +   /* for previous versions set the default group access */
> +   if (PQserverVersion(conn) < MINIMUM_VERSION_FOR_GROUP_ACCESS)
> +   {
> +       DataDirGroupAccess = false;
> +       return false;
> +   }
> Enforcing DataDirGroupAccess to false for servers older than v11 is
> fine, but RetrieveDataDirGroupAccess() should return true.  If I read
> your patch correctly then support for pg_basebackup on older server
> would just fail.

You are correct, fixed.

> +#if !defined(WIN32) && !defined(__CYGWIN__)
> +   if ((stat_buf.st_mode & PG_DIR_MODE_DEFAULT) == PG_DIR_MODE_DEFAULT)
> +   {
> +       umask(PG_MODE_MASK_ALLOW_GROUP);
> +       DataDirGroupAccess = true;
> +   }
> This should use SetConfigOption() or I am missing something?

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?

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

Thanks,
--
-David
david@pgmasters.net