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

Hi Michael,

On 3/7/18 8:51 PM, Michael Paquier wrote:
> On Wed, Mar 07, 2018 at 10:56:32AM -0500, David Steele wrote:
>> On 3/6/18 10:04 PM, Michael Paquier wrote:
>>> Seems like you forgot to re-add the chmod calls in initdb.c.
>>
>> Hmmm, I thought we were talking about moving the position of umask().
>>
>> I don't think the chmod() calls are needed because umask() is being set.
>>  The tests show that the config files have the proper permissions after
>> inidb, so this just looks like redundant code to me.
>
> Let's discuss that on a separate thread then, there could be something
> we are missing, but I agree that those should not be needed.  Looking at
> the code history, those calls have been around since the beginning of
> PG-times.

Done.

>> Another way to do this would be to make the function generic and
>> stipulate that the postmaster must be shut down before running the
>> function.  We could check postmaster.pid permissions as a separate
>> test.
>
> Yeah, that looks like a sensitive approach as this could be run
> post-initdb or after taking a backup.  This way we would avoid other
> similar behaviors in the future...  Still postmaster.pid is an
> exception.

Done.

>>> sub clean_rewind_test
>>> {
>>> +   ok (check_pg_data_perm($node_master->data_dir(), 0));
>>> +
>>>     $node_master->teardown_node  if defined $node_master;
>>> I have also a pending patch for pg_rewind which adds read-only files in
>>> the data folders of a new test, so this would cause this part to blow
>>> up.  Testing that for all the test sets does not bring additional value
>>> as well, and doing it at cleanup phase is also confusing.  So could you
>>> move that check into only one test's script?  You could remove the umask
>>> call in 003_extrafiles.pl as well this way, and reduce the patch diffs.
>>
>> I think I would rather have a way to skip the permission test rather
>> than disable it for most of the tests.  pg_rewind does more writes into
>> PGDATA that anything other than a backend.  Good coverage seems like a
>> plus.
>
> All the tests basically run the same scenario, with minimal variations,
> so let's do the test once in the test touching the highest amount of
> files and call it good.

OK, test 001 is used to check default mode and 002 is used to check
group mode.  The rest are left as-is.  Does that work for you?

> I have begun to read through patch 3.
>
> -    if (stat_buf.st_mode & (S_IRWXG | S_IRWXO))
> +    if (stat_buf.st_mode & PG_MODE_MASK_ALLOW_GROUP)
>          ereport(FATAL,
>              (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> -             errmsg("data directory \"%s\" has group or world access",
> +             errmsg("data directory \"%s\" has invalid permissions",
>                      DataDir),
> -             errdetail("Permissions should be u=rwx (0700).")));
> +             errdetail("Permissions should be u=rwx (0700) or u=rwx,g=rx (0750).")));
> Hm.  This relaxes the checks and concerns me a lot.  What if users want
> to keep strict permission all the time and rely on the existing check to
> be sure that this gets never changed post-initdb?  For such users, we
> may want to track if cluster has been initialized with group access, in
> which case tracking that in the control file would be more adapted.
> Then the startup checks should use this configuration.  Another idea
> would be a startup option.  So, I cannot believe that all users would
> like to see such checks relaxed.  Some of my users would surely complain
> about such sanity checks relaxed unconditionally, so making this
> configurable would be fine, and the current approach is not acceptable
> in my opinion.

How about a GUC that enforces one mode or the other on startup?  Default
would be 700.  The GUC can be set automatically by initdb based on the
-g option.  We had this GUC originally, but since the front-end tools
can't read it we abandoned it.  Seems like it would be good as an
enforcing mechanism, though.

Thanks,
--
-David
david@pgmasters.net