Horton

Thread

Commits

  1. Fix "unexpected relkind" error when denying permissions on toast tables.

  1. Include RELKIND_TOASTVALUE in get_relkind_objtype

    Hsu, John <hsuchen@amazon.com> — 2019-10-01T00:10:50Z

    Hello,

get_relkind_objtype(...) was introduced as part of 8b9e9644dc, and it doesn't include
RELKIND_TOASTVALUE. As a result when a user who has usage rights on schema pg_toast
and attempts to reindex a table it is not the owner of it fails with the wrong error
message.

testuser is a non-superuser role who has been granted all on pg_toast

postgres=> \c
You are now connected to database "postgres" as user "testuser".
postgres=> REINDEX TABLE pg_toast.pg_toast_16388;
ERROR:  unexpected relkind: 116

It seems get_relkind_objtype(...) is only used as part of aclcheck_error(...)
I've attached a patch to include RELKIND_TOASTVALUE in get_relkind_objtype. 
Now it fails with the proper error message.

postgres=> \c
You are now connected to database "postgres" as user "testuser".
postgres=> REINDEX TABLE pg_toast.pg_toast_16388;
ERROR:  must be owner of table pg_toast_16388

Cheers,

John H

  2. Re: Include RELKIND_TOASTVALUE in get_relkind_objtype

    Michael Paquier <michael@paquier.xyz> — 2019-10-03T06:37:39Z

    On Tue, Oct 01, 2019 at 12:10:50AM +0000, Hsu, John wrote:
> get_relkind_objtype(...) was introduced as part of 8b9e9644dc, and it doesn't include
> RELKIND_TOASTVALUE. As a result when a user who has usage rights on schema pg_toast
> and attempts to reindex a table it is not the owner of it fails with the wrong error
> message.

(Adding Peter E. in CC)

Sure.  However this implies that the user doing the reindex not only
has ownership of the relation worked on, but is also able to work
directly on the schema pg_toast.  Should we really encourage people to
do that with non-superusers?

> It seems get_relkind_objtype(...) is only used as part of aclcheck_error(...)
> I've attached a patch to include RELKIND_TOASTVALUE in get_relkind_objtype. 
> Now it fails with the proper error message.
> 
> postgres=> \c
> You are now connected to database "postgres" as user "testuser".
> postgres=> REINDEX TABLE pg_toast.pg_toast_16388;
> ERROR:  must be owner of table pg_toast_16388

Here is a set of commands to see the failure:
=# CREATE ROLE testuser LOGIN;
=# GRANT USAGE ON SCHEMA pg_toast TO testuser;
\c postgres testuser
=> REINDEX TABLE pg_toast.pg_toast_2609;
ERROR:  XX000: unexpected relkind: 116
=> REINDEX INDEX pg_toast.pg_toast_2609_index;
ERROR:  42501: must be owner of index pg_toast_2609_index
LOCATION:  aclcheck_error, aclchk.c:3623

As you wrote, get_relkind_objtype() is primarily used for ACL errors,
but we have another set of code paths with get_object_type() which
gets called for a subset of ALTER TABLE commands.  So this error can
be triggered in more ways, though you had better not do the following
one:
=# ALTER TABLE pg_toast.pg_toast_1260 rename to popo;
ERROR:  XX000: unexpected relkind: 116

The comment about OBJECT_* in get_relkind_objtype() is here because
there is no need for toast objects to have object address support
(there is a test in object_address.sql about that), and ObjectTypeMap
has no mapping OBJECT_* <-> toast table, so the change proposed is not
correct from this perspective.
--
Michael

  3. Re: Include RELKIND_TOASTVALUE in get_relkind_objtype

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-10-03T13:52:34Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Tue, Oct 01, 2019 at 12:10:50AM +0000, Hsu, John wrote:
>> get_relkind_objtype(...) was introduced as part of 8b9e9644dc, and it doesn't include
>> RELKIND_TOASTVALUE. As a result when a user who has usage rights on schema pg_toast
>> and attempts to reindex a table it is not the owner of it fails with the wrong error
>> message.

> (Adding Peter E. in CC)

> Sure.  However this implies that the user doing the reindex not only
> has ownership of the relation worked on, but is also able to work
> directly on the schema pg_toast.  Should we really encourage people to
> do that with non-superusers?

FWIW, I really dislike this patch, mainly because it is based on the 
assumption (as John said) that get_relkind_objtype is used only
in aclcheck_error calls.  However it's not obvious why that should
be true, and there certainly is no documentation suggesting that
it needs to be true.  That's mainly because get_relkind_objtype has no
documentation period, which if you ask me is flat out unacceptable
for a globally-exposed function.  (Same comment about its wrapper
get_object_type.)

The patch also falsifies the comment just a few lines away that

            /*
             * other relkinds are not supported here because they don't map to
             * OBJECT_* values
             */

without doing anything about that.

I'm inclined to think that we should redefine the charter of
get_relkind_objtype/get_object_type to be that they'll produce
some OBJECT_* value for any relkind whatever, on the grounds
that throwing an error here isn't a particularly useful behavior;
we'd rather come out with a possibly-slightly-inaccurate generic
message about a "table".  And they need to be documented that way.

Alternatively, instead of mapping other relkinds to OBJECT_TABLE,
we could invent a new enum entry OBJECT_RELATION.  There's precedent
for that in OBJECT_ROUTINE ... but I don't know that we want to
build out all the other infrastructure for a new ObjectType right now.

			regards, tom lane

  4. Re: Include RELKIND_TOASTVALUE in get_relkind_objtype

    Michael Paquier <michael@paquier.xyz> — 2019-10-04T08:55:40Z

    On Thu, Oct 03, 2019 at 09:52:34AM -0400, Tom Lane wrote:
> FWIW, I really dislike this patch, mainly because it is based on the 
> assumption (as John said) that get_relkind_objtype is used only
> in aclcheck_error calls.  However it's not obvious why that should
> be true, and there certainly is no documentation suggesting that
> it needs to be true.  That's mainly because get_relkind_objtype has no
> documentation period, which if you ask me is flat out unacceptable
> for a globally-exposed function.  (Same comment about its wrapper
> get_object_type.)

Yes, I agree that the expectations that the caller of this function
can have are hard to guess.  So we could tackle this occasion to add
more comments.  I could try to come up with a better patch.  Or
perhaps you have already your mind on it?

> The patch also falsifies the comment just a few lines away that
> 
>             /*
>              * other relkinds are not supported here because they don't map to
>              * OBJECT_* values
>              */
> 
> without doing anything about that.

That's actually what I was referring to in my previous email.

> I'm inclined to think that we should redefine the charter of
> get_relkind_objtype/get_object_type to be that they'll produce
> some OBJECT_* value for any relkind whatever, on the grounds
> that throwing an error here isn't a particularly useful behavior;
> we'd rather come out with a possibly-slightly-inaccurate generic
> message about a "table".  And they need to be documented that way.

This is tempting.

> Alternatively, instead of mapping other relkinds to OBJECT_TABLE,
> we could invent a new enum entry OBJECT_RELATION.  There's precedent
> for that in OBJECT_ROUTINE ... but I don't know that we want to
> build out all the other infrastructure for a new ObjectType right now.

I am too lazy to check the thread that led to 8b9e964, but I recall
that Peter wanted to get rid of OBJECT_RELATION because that's
confusing as that's not an purely exclusive object type, and it mapped
with other object types.
--
Michael

  5. Re: Include RELKIND_TOASTVALUE in get_relkind_objtype

    Michael Paquier <michael@paquier.xyz> — 2019-10-10T05:07:03Z

    On Fri, Oct 04, 2019 at 05:55:40PM +0900, Michael Paquier wrote:
> On Thu, Oct 03, 2019 at 09:52:34AM -0400, Tom Lane wrote:
>> FWIW, I really dislike this patch, mainly because it is based on the 
>> assumption (as John said) that get_relkind_objtype is used only
>> in aclcheck_error calls.  However it's not obvious why that should
>> be true, and there certainly is no documentation suggesting that
>> it needs to be true.  That's mainly because get_relkind_objtype has no
>> documentation period, which if you ask me is flat out unacceptable
>> for a globally-exposed function.  (Same comment about its wrapper
>> get_object_type.)
> 
> Yes, I agree that the expectations that the caller of this function
> can have are hard to guess.  So we could tackle this occasion to add
> more comments.  I could try to come up with a better patch.  Or
> perhaps you have already your mind on it?

Okay.  Attached is what I was thinking about, with extra regression
tests to cover the ground for toast tables and indexes that are able
to reproduce the original failure, and more comments for the routines
as they should be used only for ACL error messages.

Any thoughts?
--
Michael

  6. Re: Include RELKIND_TOASTVALUE in get_relkind_objtype

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-11-04T20:31:27Z

    Michael Paquier <michael@paquier.xyz> writes:
> Okay.  Attached is what I was thinking about, with extra regression
> tests to cover the ground for toast tables and indexes that are able
> to reproduce the original failure, and more comments for the routines
> as they should be used only for ACL error messages.

I'd rather do something like the attached, which makes it more of an
explicit goal that we won't fail on bad input.  (As written, we'd only
fail on bad classId, which is a case that really shouldn't happen.)

Tests are the same as yours, but I revised the commentary and got
rid of the elog-for-bad-relkind.  I also made some cosmetic changes
in commands/alter.c, so as to (1) make it clear by inspection that
those calls are only used to feed aclcheck_error, and (2) avoid
uselessly computing a value that we won't need in normal non-error
cases.

			regards, tom lane

  7. Re: Include RELKIND_TOASTVALUE in get_relkind_objtype

    Michael Paquier <michael@paquier.xyz> — 2019-11-05T02:29:02Z

    On Mon, Nov 04, 2019 at 03:31:27PM -0500, Tom Lane wrote:
> I'd rather do something like the attached, which makes it more of an
> explicit goal that we won't fail on bad input.  (As written, we'd only
> fail on bad classId, which is a case that really shouldn't happen.)

Okay, that part looks fine.

> Tests are the same as yours, but I revised the commentary and got
> rid of the elog-for-bad-relkind.

No objections on that part either.

> I also made some cosmetic changes in commands/alter.c, so as to (1)
> make it clear by inspection that those calls are only used to feed
> aclcheck_error, and (2) avoid uselessly computing a value that we
> won't need in normal non-error cases.

Makes also sense.  Thanks for looking at it!
--
Michael

  8. Re: Include RELKIND_TOASTVALUE in get_relkind_objtype

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-11-05T18:41:28Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Mon, Nov 04, 2019 at 03:31:27PM -0500, Tom Lane wrote:
>> I'd rather do something like the attached, which makes it more of an
>> explicit goal that we won't fail on bad input.  (As written, we'd only
>> fail on bad classId, which is a case that really shouldn't happen.)

> Okay, that part looks fine.

Pushed like that, then.

			regards, tom lane

  9. Re: Include RELKIND_TOASTVALUE in get_relkind_objtype

    Michael Paquier <michael@paquier.xyz> — 2019-11-06T04:43:14Z

    On Tue, Nov 05, 2019 at 01:41:28PM -0500, Tom Lane wrote:
> Pushed like that, then.

Thanks for the commit.
--
Michael