Re: PATCH: Configurable file mode mask

Hi Michael,

On 4/2/18 2:28 AM, Michael Paquier wrote:
>
> @@ -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..

Yeah, I think I figured that out when I first went through the code but
managed to forget it. Reverted.

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

Fixed.

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

I feel this should be considered in a separate patch. These files are
not created by initdb so it seems to be an admin/packaging issue. There
is always the option to locate the certs outside the data directory and
some distros do that by default.

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

Done.

> There is a noise diff in miscinit.c.

Fixed.

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

I think this makes sense for pg_basebackup, pg_receivewal, and
pg_recvlogical so I have added notes for those. Not sure I'm happy with
the language but at least we have something to bikeshed.

It seems to me that pg_resetwal and pg_rewind should be expected to not
break the permissions in PGDATA, just as they do now.

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

I removed it from file_perm.h instead. With the new variables (see
below), most call sites will have no need for the mode constants.

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

I decided that the constants were a bit confusing in general. What does
"default" mean, anyway? Instead I have created variables in file_perm.c
that hold the current file create mode, dir create mode, and mode mask.
All call sites use those variables (e.g. pg_dir_create_mode), which I
think are much clear.

This causes a bit of churn with the constants we added last September
but those were added for v11 so it won't create more complications for
back-patching.

> Yes, we can.

Yes! We can!

New patches attached.

Thanks,
--
-David
david(at)pgmasters(dot)net