Horton

Thread

Commits

  1. Add "pg_database_owner" default role.

  2. Merge similar algorithms into roles_is_member_of().

  1. Tying an object's ownership to datdba

    Noah Misch <noah@leadboat.com> — 2020-12-28T04:31:48Z

    https://postgr.es/m/20201031163518.GB4039133@rfd.leadboat.com gave $SUBJECT as
one of the constituent projects for changing the public schema default ACL.
This feature stands on its own, hence the new thread.  I showed syntax "ALTER
SCHEMA public OWNER TO DATABASE_OWNER" and anticipated a new RoleSpecType.
That was fine for in-memory representation, but it lacked a value to store in
pg_namespace.nspowner.  That led me to instead add a default role,
"pg_database_owner".  That way, user queries joining owner columns to
pg_authid need no modification.  Also, user.c changes were trivial, and
pg_dump needed no changes.  The role's membership consists, implicitly, of the
current database owner.  The first patch refactors acl.c to reduce code
duplication, and the second patch adds the feature.

I ended up blocking DDL that creates role memberships involving the new role;
see reasons in user.c comments.  Lifting those restrictions looked feasible,
but it was inessential to the mission, and avoiding unintended consequences
would have been tricky.  Views "information_schema.enabled_roles" and
"information_schema.applicable_roles" list any implicit membership in
pg_database_owner, but pg_catalog.pg_group and psql \dgS do not.  If this
leads any reviewer to look closely at applicable_roles, note that its behavior
doesn't match its documentation
(https://postgr.es/m/flat/20060728170615.GY20016@kenobi.snowman.net).

This patch makes us read pg_database when reading pg_auth_members.
IndexScanOK() has been saying "during backend startup we have to be able to
use the pg_authid and pg_auth_members syscaches for authentication".  While
that's true of pg_authid, I found no sign of pg_auth_members reads that early.
(The read in InitPostgres() -> CheckMyDatabase() -> pg_database_aclcheck()
happens after RelationCacheInitializePhase3().  In a physical walsender, which
never has a mature relcache, some SHOW commands make guc.c check
DEFAULT_ROLE_READ_ALL_SETTINGS.  The walsender case, though it happens after
authentication, may necessitate IndexScanOK()'s treatment of pg_auth_members.)
For now, just in case I missed the early read, I've made IndexScanOK() treat
pg_database like pg_auth_members.

Thanks,
nm

  2. Re: Tying an object's ownership to datdba

    Noah Misch <noah@leadboat.com> — 2021-03-15T09:41:20Z

    A cfbot failure showed I had missed ORDER BY in some test queries.

On Sun, Dec 27, 2020 at 08:31:48PM -0800, Noah Misch wrote:
> I ended up blocking DDL that creates role memberships involving the new role;
> see reasons in user.c comments.  Lifting those restrictions looked feasible,
> but it was inessential to the mission, and avoiding unintended consequences
> would have been tricky.

Later, I pondered the case of pg_database_owner owning a shared object
(database or tablespace).  The behavior is certainly odd.  For a tablespace,
any database owner can act as the tablespace owner (but only when connected to
a database that the role owns).  For a database, likewise.  When connected to
a database having datdba=pg_database_owner, no particular role acts as the
owner, just superusers and SECURITY DEFINER functions owned by
pg_database_owner.  I don't have high hopes for that being useful, but I
couldn't quite convince myself to ban it.  Attached v2 does expand an
AddRoleMems() comment to discuss this, though.

  3. Re: Tying an object's ownership to datdba

    John Naylor <john.naylor@enterprisedb.com> — 2021-03-24T15:57:37Z

    On Mon, Dec 28, 2020 at 12:32 AM Noah Misch <noah@leadboat.com> wrote:
> [v2]

Hi Noah,

In the refactoring patch, there is a lingering comment reference to
roles_has_privs_of(). Aside from that, it looks good to me. A possible
thing to consider is an assert that is_admin is not null where we expect
that.

The database owner role patch looks good as well.

> I ended up blocking DDL that creates role memberships involving the new
role;
> see reasons in user.c comments.  Lifting those restrictions looked
feasible,
> but it was inessential to the mission, and avoiding unintended
consequences
> would have been tricky.  Views "information_schema.enabled_roles" and
> "information_schema.applicable_roles" list any implicit membership in
> pg_database_owner, but pg_catalog.pg_group and psql \dgS do not.  If this
> leads any reviewer to look closely at applicable_roles, note that its
behavior
> doesn't match its documentation
> (https://postgr.es/m/flat/20060728170615.GY20016@kenobi.snowman.net).

Is this something that needs fixing separately?

--
John Naylor
EDB: http://www.enterprisedb.com

  4. Re: Tying an object's ownership to datdba

    Noah Misch <noah@leadboat.com> — 2021-03-25T04:07:39Z

    On Wed, Mar 24, 2021 at 11:57:37AM -0400, John Naylor wrote:
> On Mon, Dec 28, 2020 at 12:32 AM Noah Misch <noah@leadboat.com> wrote:
> > [v2]
> 
> Hi Noah,
> 
> In the refactoring patch, there is a lingering comment reference to roles_has_privs_of(). Aside from that, it looks good to me. A possible thing to consider is an assert that is_admin is not null where we expect that.

Thanks.  The next version will contain the comment fix and
"Assert(OidIsValid(admin_of) == PointerIsValid(is_admin));".

> The database owner role patch looks good as well.

Do you plan to put the CF entry into Ready for Committer, or should the
patches wait for another review?

> > I ended up blocking DDL that creates role memberships involving the new role;
> > see reasons in user.c comments.  Lifting those restrictions looked feasible,
> > but it was inessential to the mission, and avoiding unintended consequences
> > would have been tricky.  Views "information_schema.enabled_roles" and
> > "information_schema.applicable_roles" list any implicit membership in
> > pg_database_owner, but pg_catalog.pg_group and psql \dgS do not.  If this
> > leads any reviewer to look closely at applicable_roles, note that its behavior
> > doesn't match its documentation
> > (https://postgr.es/m/flat/20060728170615.GY20016@kenobi.snowman.net).
> 
> Is this something that needs fixing separately?

It is bug, but I think fixing it is not very urgent and should happen
separately, if at all.

  5. Re: Tying an object's ownership to datdba

    John Naylor <john.naylor@enterprisedb.com> — 2021-03-25T13:55:57Z

    On Thu, Mar 25, 2021 at 12:07 AM Noah Misch <noah@leadboat.com> wrote:
>
> > In the refactoring patch, there is a lingering comment reference to
roles_has_privs_of(). Aside from that, it looks good to me. A possible
thing to consider is an assert that is_admin is not null where we expect
that.
>
> Thanks.  The next version will contain the comment fix and
> "Assert(OidIsValid(admin_of) == PointerIsValid(is_admin));".
>
> > The database owner role patch looks good as well.
>
> Do you plan to put the CF entry into Ready for Committer, or should the
> patches wait for another review?

I've marked it Ready for Committer.

--
John Naylor
EDB: http://www.enterprisedb.com