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

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@pgmasters.net