Thread: Include RELKIND_TOASTVALUE in get_relkind_objtype
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
Attachment
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
Attachment
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
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
Attachment
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
Attachment
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
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index ce8a4e9..b8cbe6a 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -2612,6 +2612,13 @@ get_object_attnum_acl(Oid class_id)
return prop->attnum_acl;
}
+/*
+ * get_object_type
+ *
+ * Return the object type associated with a given object. This routine
+ * is primarily used to determine the object type to mention in ACL check
+ * error messages, so it's desirable for it to avoid failing.
+ */
ObjectType
get_object_type(Oid class_id, Oid object_id)
{
@@ -5333,6 +5340,16 @@ strlist_to_textarray(List *list)
return arr;
}
+/*
+ * get_relkind_objtype
+ *
+ * Return the object type for the relkind given by the caller.
+ *
+ * If an unexpected relkind is passed, we say OBJECT_TABLE rather than
+ * failing. That's because this is mostly used for generating error messages
+ * for failed ACL checks on relations, and we'd rather produce a generic
+ * message saying "table" than fail entirely.
+ */
ObjectType
get_relkind_objtype(char relkind)
{
@@ -5352,13 +5369,10 @@ get_relkind_objtype(char relkind)
return OBJECT_MATVIEW;
case RELKIND_FOREIGN_TABLE:
return OBJECT_FOREIGN_TABLE;
-
- /*
- * other relkinds are not supported here because they don't map to
- * OBJECT_* values
- */
+ case RELKIND_TOASTVALUE:
+ return OBJECT_TABLE;
default:
- elog(ERROR, "unexpected relkind: %d", relkind);
- return 0;
+ /* Per above, don't raise an error */
+ return OBJECT_TABLE;
}
}
diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c
index 70dbcb0..562e3d3 100644
--- a/src/backend/commands/alter.c
+++ b/src/backend/commands/alter.c
@@ -172,7 +172,6 @@ AlterObjectRename_internal(Relation rel, Oid objectId, const char *new_name)
AttrNumber Anum_name = get_object_attnum_name(classId);
AttrNumber Anum_namespace = get_object_attnum_namespace(classId);
AttrNumber Anum_owner = get_object_attnum_owner(classId);
- ObjectType objtype = get_object_type(classId, objectId);
HeapTuple oldtup;
HeapTuple newtup;
Datum datum;
@@ -224,7 +223,8 @@ AlterObjectRename_internal(Relation rel, Oid objectId, const char *new_name)
ownerId = DatumGetObjectId(datum);
if (!has_privs_of_role(GetUserId(), DatumGetObjectId(ownerId)))
- aclcheck_error(ACLCHECK_NOT_OWNER, objtype, old_name);
+ aclcheck_error(ACLCHECK_NOT_OWNER, get_object_type(classId, objectId),
+ old_name);
/* User must have CREATE privilege on the namespace */
if (OidIsValid(namespaceId))
@@ -670,7 +670,6 @@ AlterObjectNamespace_internal(Relation rel, Oid objid, Oid nspOid)
AttrNumber Anum_name = get_object_attnum_name(classId);
AttrNumber Anum_namespace = get_object_attnum_namespace(classId);
AttrNumber Anum_owner = get_object_attnum_owner(classId);
- ObjectType objtype = get_object_type(classId, objid);
Oid oldNspOid;
Datum name,
namespace;
@@ -726,7 +725,7 @@ AlterObjectNamespace_internal(Relation rel, Oid objid, Oid nspOid)
ownerId = DatumGetObjectId(owner);
if (!has_privs_of_role(GetUserId(), ownerId))
- aclcheck_error(ACLCHECK_NOT_OWNER, objtype,
+ aclcheck_error(ACLCHECK_NOT_OWNER, get_object_type(classId, objid),
NameStr(*(DatumGetName(name))));
/* User must have CREATE privilege on new namespace */
@@ -950,8 +949,6 @@ AlterObjectOwner_internal(Relation rel, Oid objectId, Oid new_ownerId)
/* Superusers can bypass permission checks */
if (!superuser())
{
- ObjectType objtype = get_object_type(classId, objectId);
-
/* must be owner */
if (!has_privs_of_role(GetUserId(), old_ownerId))
{
@@ -970,7 +967,8 @@ AlterObjectOwner_internal(Relation rel, Oid objectId, Oid new_ownerId)
snprintf(namebuf, sizeof(namebuf), "%u", objectId);
objname = namebuf;
}
- aclcheck_error(ACLCHECK_NOT_OWNER, objtype, objname);
+ aclcheck_error(ACLCHECK_NOT_OWNER, get_object_type(classId, objectId),
+ objname);
}
/* Must be able to become new owner */
check_is_member_of_role(GetUserId(), new_ownerId);
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index 3c2b166..1cdb7a9 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2434,8 +2434,17 @@ CREATE ROLE regress_reindexuser NOLOGIN;
SET SESSION ROLE regress_reindexuser;
REINDEX SCHEMA schema_to_reindex;
ERROR: must be owner of schema schema_to_reindex
+-- Permission failures with toast tables and indexes (pg_authid here)
+RESET ROLE;
+GRANT USAGE ON SCHEMA pg_toast TO regress_reindexuser;
+SET SESSION ROLE regress_reindexuser;
+REINDEX TABLE pg_toast.pg_toast_1260;
+ERROR: must be owner of table pg_toast_1260
+REINDEX INDEX pg_toast.pg_toast_1260_index;
+ERROR: must be owner of index pg_toast_1260_index
-- Clean up
RESET ROLE;
+REVOKE USAGE ON SCHEMA pg_toast FROM regress_reindexuser;
DROP ROLE regress_reindexuser;
DROP SCHEMA schema_to_reindex CASCADE;
NOTICE: drop cascades to 6 other objects
diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql
index 26640f0..7659808 100644
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -1003,8 +1003,15 @@ REINDEX SCHEMA CONCURRENTLY schema_to_reindex;
CREATE ROLE regress_reindexuser NOLOGIN;
SET SESSION ROLE regress_reindexuser;
REINDEX SCHEMA schema_to_reindex;
+-- Permission failures with toast tables and indexes (pg_authid here)
+RESET ROLE;
+GRANT USAGE ON SCHEMA pg_toast TO regress_reindexuser;
+SET SESSION ROLE regress_reindexuser;
+REINDEX TABLE pg_toast.pg_toast_1260;
+REINDEX INDEX pg_toast.pg_toast_1260_index;
-- Clean up
RESET ROLE;
+REVOKE USAGE ON SCHEMA pg_toast FROM regress_reindexuser;
DROP ROLE regress_reindexuser;
DROP SCHEMA schema_to_reindex CASCADE;
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
Attachment
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
On Tue, Nov 05, 2019 at 01:41:28PM -0500, Tom Lane wrote: > Pushed like that, then. Thanks for the commit. -- Michael