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
da...@pgmasters.net
signature.asc
Description: OpenPGP digital signature
