Re: PATCH: Configurable file mode mask

From: David Steele <david(at)pgmasters(dot)net>
To: Michael Paquier <michael(at)paquier(dot)xyz>
Cc: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, Stephen Frost <sfrost(at)snowman(dot)net>, Adam Brightwell <adam(dot)brightwell(at)crunchydata(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, "Tsunakawa, Takayuki" <tsunakawa(dot)takay(at)jp(dot)fujitsu(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: PATCH: Configurable file mode mask
Date: 2018-03-09 18:51:14
Message-ID: 9bb7cb7f-5c4b-b2e0-329e-e3f21ac9ab23@pgmasters.net
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: 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(at)pgmasters(dot)net

Attachment Content-Type Size
group-access-v10-01-pgresetwal-test.patch text/plain 2.5 KB
group-access-v10-02-file-perm.patch text/plain 30.5 KB
group-access-v10-03-group.patch text/plain 26.9 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Arthur Zakirov 2018-03-09 19:17:21 Re: [HACKERS] [FEATURE PATCH] pg_stat_statements with plans (v02)
Previous Message Jesper Pedersen 2018-03-09 18:50:54 Re: [HACKERS] Runtime Partition Pruning