Re: PATCH: Configurable file mode mask

On Fri, Mar 30, 2018 at 01:27:11PM -0400, David Steele wrote:
> I have replaced data_directory_group_access with data_directory_mode.

That looks way better. Thanks for considering it.

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

Okay for me.

@@ -285,7 +286,7 @@ dsm_impl_posix(dsm_op op, dsm_handle handle, Size request_size,
* returning.
*/
flags = O_RDWR | (op == DSM_OP_CREATE ? O_CREAT | O_EXCL : 0);
- if ((fd = shm_open(name, flags, 0600)) == -1)
+ if ((fd = shm_open(name, flags, PG_FILE_MODE_DEFAULT)) == -1)

Hm. Sorry for the extra noise. This one is actually incorrect as
shm_open will use the path specified by glibc, which is out of
pg_dynshmem so it does not matter for base backups, and we can keep
0600. pg_dymshmem is used for mmap, still this would map with the umask
setup by the postmaster as OpenTransientFile & friends are used. sysv
uses IPCProtection but there is no need to care about it as well. No
need for a comment perhaps..

pg_basebackup.c creates recovery.conf with 0600 all the time ;)

Except for those two nits, I am fine to pass down to a committer patch
number 1. This has value in my opinion per the refactoring it does and
the umask handling of pg_rewind and pg_resetwal added.

Now for patch 2...

+ <para>
+ If the data directory allows group read access then certificate files may
+ need to be located outside of the data directory in order to conform to the
+ security requirements outlined above. Generally, group access is enabled
+ to allow an unprivileged user to backup the database, and in that case the
+ backup software will not be able to read the certificate files and will
+ likely error.
+ </para
The answer is no for me and likely the same for others, but I am raising
the point for the archives. Should we relax
check_ssl_key_file_permissions() for group permissions by the way from
0600 to 0640 when the file is owned by the user running Postgres? If we
don't do that, then SSL private keys will need to be used with 0600,
potentially breaking backups... At the same time this reduces the
security of private keys but if the administrator is ready to accept
group permissions that should be a risk he is ready to accept?

+ &DataDirMode,
+ 0700, 0000, 0777,
+ NULL, NULL, show_data_directory_mode
Instead of a camel case, renaming that to data_directory_mode would be
nice to ease future greps. I do that all the time for existing code,
pesting when things are not consistent. A nit.

There is a noise diff in miscinit.c.

I am pretty sure that we want more documentation in pg_basebackup,
pg_rewind and pg_resetwal telling that those consider grouping access.

There is no need to include sys/stat.h as this is already part of
file_perm.h as DataDirectoryMask uses mode_t for its definition.

+/*
+ * Mode of the data directory. The default is 0700 but may it be changed in
+ * checkDataDir() to 0750 if the data directory actually has that mode.
+ */
+int DataDirMode = PG_DIR_MODE_NOGROUP

/*
- * Default mode for directories.
+ * Default mode for created files, unless something else is specified using
+ * the *Perm() function variants.
*/
-#define PG_DIR_MODE_DEFAULT S_IRWXU
+#define PG_FILE_MODE_DEFAULT (S_IRUSR | S_IWUSR | S_IRGRP)
To reduce the code churn and simplify the logic, I would recommend to
not use variables which have a negative meaning, so PG_DIR_MODE_DEFAULT
should remain the same in patch 2, and there should be PG_DIR_MODE_GROUP
instead of PG_DIR_MODE_NOGROUP. That would be more consistent with the
file modes as well.

Yes, we can.
--
Michael