Re: [HACKERS] patch for type privileges
On lör, 2011-12-10 at 16:16 +0100, Yeb Havinga wrote:
* Cannot restrict access to array types. After revoking usage from the
element type, the error is perhaps a bit misleading. (smallint[] vs
smallint)
postgres= create table a (a int2[]);
ERROR: permission denied for type smallint[]
This matter was still outstanding. The problem with fixing this is that
you need to duplicate the array type to element type conversion in two
dozen places. So I have refactored this into a separate function, which
also takes care of the call to format_type_be, which is equally
duplicated in as many places.
diff --git i/src/backend/access/common/tupdesc.c w/src/backend/access/common/tupdesc.c
index 1f40b7c..aa1ce80 100644
--- i/src/backend/access/common/tupdesc.c
+++ w/src/backend/access/common/tupdesc.c
@@ -573,8 +573,7 @@
aclresult = pg_type_aclcheck(atttypid, GetUserId(), ACL_USAGE);
if (aclresult != ACLCHECK_OK)
- aclcheck_error(aclresult, ACL_KIND_TYPE,
- format_type_be(atttypid));
+ aclcheck_error_type(aclresult, atttypid);
attcollation = GetColumnDefCollation(NULL, entry, atttypid);
attdim = list_length(entry-typeName-arrayBounds);
diff --git i/src/backend/catalog/aclchk.c w/src/backend/catalog/aclchk.c
index 9315e79..89b71b4 100644
--- i/src/backend/catalog/aclchk.c
+++ w/src/backend/catalog/aclchk.c
@@ -3389,6 +3389,19 @@ static AclMode pg_aclmask(AclObjectKind objkind, Oid table_oid, AttrNumber attnu
}
+/*
+ * Special common handling for types: use element type instead of array type,
+ * and format nicely
+ */
+void
+aclcheck_error_type(AclResult aclerr, Oid typeOid)
+{
+ Oid element_type = get_element_type(typeOid);
+
+ aclcheck_error(aclerr, ACL_KIND_TYPE, format_type_be(element_type ? element_type : typeOid));
+}
+
+
/* Check if given user has rolcatupdate privilege according to pg_authid */
static bool
has_rolcatupdate(Oid roleid)
diff --git i/src/backend/catalog/objectaddress.c w/src/backend/catalog/objectaddress.c
index 250069f..3c3fd05 100644
--- i/src/backend/catalog/objectaddress.c
+++ w/src/backend/catalog/objectaddress.c
@@ -932,8 +932,7 @@ static ObjectAddress get_object_address_opcf(ObjectType objtype, List *objname,
case OBJECT_DOMAIN:
case OBJECT_ATTRIBUTE:
if (!pg_type_ownercheck(address.objectId, roleid))
-aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_TYPE,
- format_type_be(address.objectId));
+aclcheck_error_type(ACLCHECK_NOT_OWNER, address.objectId);
break;
case OBJECT_AGGREGATE:
case OBJECT_FUNCTION:
diff --git i/src/backend/catalog/pg_aggregate.c w/src/backend/catalog/pg_aggregate.c
index 9ff70a5..82a2c9f 100644
--- i/src/backend/catalog/pg_aggregate.c
+++ w/src/backend/catalog/pg_aggregate.c
@@ -208,19 +208,16 @@ static Oid lookup_agg_function(List *fnName, int nargs, Oid *input_types,
{
aclresult = pg_type_aclcheck(aggArgTypes[i], GetUserId(), ACL_USAGE);
if (aclresult != ACLCHECK_OK)
- aclcheck_error(aclresult, ACL_KIND_TYPE,
- format_type_be(aggArgTypes[i]));
+ aclcheck_error_type(aclresult, aggArgTypes[i]);
}
aclresult = pg_type_aclcheck(aggTransType, GetUserId(), ACL_USAGE);
if (aclresult != ACLCHECK_OK)
- aclcheck_error(aclresult, ACL_KIND_TYPE,
- format_type_be(aggTransType));
+ aclcheck_error_type(aclresult, aggTransType);
aclresult = pg_type_aclcheck(finaltype, GetUserId(), ACL_USAGE);
if (aclresult != ACLCHECK_OK)
- aclcheck_error(aclresult, ACL_KIND_TYPE,
- format_type_be(finaltype));
+ aclcheck_error_type(aclresult, finaltype);
/*
diff --git i/src/backend/commands/functioncmds.c w/src/backend/commands/functioncmds.c
index 5f1c19e..2a075a1 100644
--- i/src/backend/commands/functioncmds.c
+++ w/src/backend/commands/functioncmds.c
@@ -154,8 +154,7 @@ static void AlterFunctionOwner_internal(Relation rel, HeapTuple tup,
aclresult = pg_type_aclcheck(rettype, GetUserId(), ACL_USAGE);
if (aclresult != ACLCHECK_OK)
- aclcheck_error(aclresult, ACL_KIND_TYPE,
- format_type_be(rettype));
+ aclcheck_error_type(aclresult, rettype);
*prorettype_p = rettype;
*returnsSet_p = returnType-setof;
@@ -247,8 +246,7 @@ static void AlterFunctionOwner_internal(Relation rel, HeapTuple tup,
aclresult = pg_type_aclcheck(toid, GetUserId(), ACL_USAGE);
if (aclresult != ACLCHECK_OK)
- aclcheck_error(aclresult, ACL_KIND_TYPE,
- format_type_be(toid));
+ aclcheck_error_type(aclresult, toid);
if (t-setof)
ereport(ERROR,
@@ -1509,13 +1507,11 @@ static void AlterFunctionOwner_internal(Relation rel, HeapTuple tup,
aclresult = pg_type_aclcheck(sourcetypeid, GetUserId(), ACL_USAGE);
if (aclresult != ACLCHECK_OK)
- aclcheck_error(aclresult, ACL_KIND_TYPE,
- format_type_be(sourcetypeid));
+ aclcheck_error_type(aclresult, sourcetypeid);
aclresult = pg_type_aclcheck(targettypeid, GetUserId(), ACL_USAGE);
if (aclresult != ACLCHECK_OK)
- aclcheck_error(aclresult, ACL_KIND_TYPE,
-
Re: [HACKERS] patch for type privileges
On 12/13/2011 01:13 PM, Yeb Havinga wrote: On 2011-12-12 20:53, Peter Eisentraut wrote: postgres= create table a (a int2[]); ERROR: permission denied for type smallint[] OK, that error message should be improved. Fixing this is easy, but I'd like to look into refactoring this a bit. Let's ignore that for now; it's easy to do later. My experience with ignoring things for now is not positive. This is my favorite comment from the current CommitFest. I'll probably use that line at some point. Yeb's list is now down to this and some documentation tweaking, so this may very well be ready for commit (and an Open Items link toward the do later?) I'm going to mark this one returned with feedback on the assumption there is still some refactoring going on here though; can always change it if Peter sprints back with a commit candidate. -- Greg Smith 2ndQuadrant USg...@2ndquadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch for type privileges
On 2011-12-12 20:53, Peter Eisentraut wrote: On sön, 2011-12-11 at 21:21 +0200, Peter Eisentraut wrote: * Cannot restrict access to array types. After revoking usage from the element type, the error is perhaps a bit misleading. (smallint[] vs smallint) postgres= create table a (a int2[]); ERROR: permission denied for type smallint[] OK, that error message should be improved. Fixing this is easy, but I'd like to look into refactoring this a bit. Let's ignore that for now; it's easy to do later. My experience with ignoring things for now is not positive. * The information schema view 'attributes' has this additional condition: AND (pg_has_role(t.typowner, 'USAGE') OR has_type_privilege(t.oid, 'USAGE')); What happens is that attributes in a composite type are shown, or not, if the current user has USAGE rights. The strange thing here, is that the attribute in the type being show or not, doesn't match being able to use it (in the creation of e.g. a table). Yeah, that's a bug. That should be something like AND (pg_has_role(c.relowner, 'USAGE') OR has_type_privilege(c.reltype, 'USAGE')); And fix for that included. Confirmed that this now works as expected. I have no remarks on the other parts of the patch code. After puzzling a bit more with the udt and usage privileges views, it is clear that they should complement each other. That might be reflected by adding to the 'usage_privileges' section a link back to the 'udt_privileges' section. I have no further comments on this patch. regards, Yeb Havinga -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch for type privileges
On lör, 2011-12-10 at 16:16 +0100, Yeb Havinga wrote: * ExecGrant_type() prevents 'grant usage on domain' on a type, but the converse is possible. postgres=# create domain myint as int2; CREATE DOMAIN postgres=# grant usage on type myint to public; GRANT This is the same as how we handle types vs. domains elsewhere. For example, you can use DROP TYPE to drop a domain, but you can't use DROP DOMAIN to drop a type. * Cannot restrict access to array types. After revoking usage from the element type, the error is perhaps a bit misleading. (smallint[] vs smallint) postgres= create table a (a int2[]); ERROR: permission denied for type smallint[] OK, that error message should be improved. * The patch adds the following text explaining the USAGE privilege on types. For types and domains, this privilege allow the use of the type or domain in the definition of tables, functions, and other schema objects. Since other paragraphs in USAGE use the word 'creation' instead of 'definition', I believe here the word 'creation' should be used too. IMHO it would also be good to describe what the USAGE privilege is not, but might be expected since it is such a generic term. USAGE on type: use of the type while creating new dependencies to the type, not usage in the sense of instantiating values of the type. If there are existing dependencies, revoking usage privileges will not return any warning and the dependencies still exist. Also other kinds of exceptions could be noted, such as the exception for array types and casts. The example you gave in the top mail about why restricting access to types can be useful, such as preventing that owners are prevented changing their types because others have 'blocked' them by their usage, is something that could also help readers of the documentation understand why privileges on types are useful for them (or not). Good suggestions. I'll review the text. * The information schema view 'attributes' has this additional condition: AND (pg_has_role(t.typowner, 'USAGE') OR has_type_privilege(t.oid, 'USAGE')); What happens is that attributes in a composite type are shown, or not, if the current user has USAGE rights. The strange thing here, is that the attribute in the type being show or not, doesn't match being able to use it (in the creation of e.g. a table). Yeah, that's a bug. That should be something like AND (pg_has_role(c.relowner, 'USAGE') OR has_type_privilege(c.reltype, 'USAGE')); I'll produce a new patch for these issues in a bit. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch for type privileges
On 2011-12-07 19:59, Peter Eisentraut wrote: Two excellent finds. Here is an updated patch with fixes. Thanks.. I'm sorry I cannot yet provide a complete review, but since the end of the commitfest is near, I decided to mail them anyway instead of everything on dec 15. * ExecGrant_type() prevents 'grant usage on domain' on a type, but the converse is possible. postgres=# create domain myint as int2; CREATE DOMAIN postgres=# grant usage on type myint to public; GRANT * Cannot restrict access to array types. After revoking usage from the element type, the error is perhaps a bit misleading. (smallint[] vs smallint) postgres= create table a (a int2[]); ERROR: permission denied for type smallint[] * The patch adds the following text explaining the USAGE privilege on types. For types and domains, this privilege allow the use of the type or domain in the definition of tables, functions, and other schema objects. Since other paragraphs in USAGE use the word 'creation' instead of 'definition', I believe here the word 'creation' should be used too. IMHO it would also be good to describe what the USAGE privilege is not, but might be expected since it is such a generic term. USAGE on type: use of the type while creating new dependencies to the type, not usage in the sense of instantiating values of the type. If there are existing dependencies, revoking usage privileges will not return any warning and the dependencies still exist. Also other kinds of exceptions could be noted, such as the exception for array types and casts. The example you gave in the top mail about why restricting access to types can be useful, such as preventing that owners are prevented changing their types because others have 'blocked' them by their usage, is something that could also help readers of the documentation understand why privileges on types are useful for them (or not). * The information schema view 'attributes' has this additional condition: AND (pg_has_role(t.typowner, 'USAGE') OR has_type_privilege(t.oid, 'USAGE')); What happens is that attributes in a composite type are shown, or not, if the current user has USAGE rights. The strange thing here, is that the attribute in the type being show or not, doesn't match being able to use it (in the creation of e.g. a table). Maybe that is not intended, but I would expect it matching: postgres=# create user c; CREATE ROLE postgres=# create type t as (a int2); CREATE TYPE postgres=# \c - c You are now connected to database postgres as user c. postgres= select udt_name,attribute_name from information_schema.attributes; udt_name | attribute_name --+ t| a (1 row) postgres= \c - You are now connected to database postgres as user c. postgres= \c - postgres You are now connected to database postgres as user postgres. postgres=# revoke usage on type int2 from public; REVOKE postgres=# \c - c You are now connected to database postgres as user c. postgres= select udt_name,attribute_name from information_schema.attributes; udt_name | attribute_name --+ (0 rows) postgres= create table m (a t); CREATE TABLE postgres= insert into m values (ROW(10)); INSERT 0 1 postgres= Conversely: postgres=# grant usage on type int2 to public; GRANT postgres=# revoke usage on type t from public; REVOKE postgres=# \c - c You are now connected to database postgres as user c. postgres= select udt_name,attribute_name from information_schema.attributes; udt_name | attribute_name --+ t| a (1 row) postgres= create table m2 (a t); ERROR: permission denied for type t postgres= regards, Yeb Havinga -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch for type privileges
On 2011-12-01 22:14, Peter Eisentraut wrote: On tor, 2011-12-01 at 14:37 +0100, Yeb Havinga wrote: On 2011-11-29 18:47, Peter Eisentraut wrote: On tis, 2011-11-29 at 07:07 +0200, Peter Eisentraut wrote: On mån, 2011-11-28 at 11:41 +0100, Yeb Havinga wrote: On 2011-11-15 21:50, Peter Eisentraut wrote: Patch attached. I cannot get the patch to apply, this is the output of patch -p1 --dry-run on HEAD. I need to remerge it against concurrent range type activity. New patch attached. I'm looking at your patch. One thing that puzzled me for a while was that I could not restrict access to base types (either built-in or user defined). Is this intentional? Works for me: =# create user foo; =# revoke usage on type int8 from public; =# \c - foo = create table test1 (a int4, b int8); ERROR: permission denied for type bigint Hmm even though I have 'revoke all on type int2 from public' in my psql history, I cannot repeat what I think was happening yesterday. Probably I was still superuser in the window I was testing with, but will never no until time travel is invented. Or maybe I tested with a cast. Using a cast, it is possible to create a table with a code path through OpenIntoRel: session 1: t=# revoke all on type int2 from public; session2 : t= create table t2 (a int2); ERROR: permission denied for type smallint t= create table t as (select 1::int2 as a); SELECT 1 t= \d t Table public.t Column | Type | Modifiers +--+--- a | smallint | t= Something different: as non superuser I get this error when restricting a type I don't own: t= revoke all on type int2 from public; ERROR: unrecognized objkind: 6 My current time is limited but I will be able to look more at the patch in a few more days. regards, Yeb Havinga -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch for type privileges
On 2011-11-29 18:47, Peter Eisentraut wrote: On tis, 2011-11-29 at 07:07 +0200, Peter Eisentraut wrote: On mån, 2011-11-28 at 11:41 +0100, Yeb Havinga wrote: On 2011-11-15 21:50, Peter Eisentraut wrote: Patch attached. I cannot get the patch to apply, this is the output of patch -p1 --dry-run on HEAD. I need to remerge it against concurrent range type activity. New patch attached. I'm looking at your patch. One thing that puzzled me for a while was that I could not restrict access to base types (either built-in or user defined). Is this intentional? regards, Yeb Havinga -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch for type privileges
On tor, 2011-12-01 at 14:37 +0100, Yeb Havinga wrote: On 2011-11-29 18:47, Peter Eisentraut wrote: On tis, 2011-11-29 at 07:07 +0200, Peter Eisentraut wrote: On mån, 2011-11-28 at 11:41 +0100, Yeb Havinga wrote: On 2011-11-15 21:50, Peter Eisentraut wrote: Patch attached. I cannot get the patch to apply, this is the output of patch -p1 --dry-run on HEAD. I need to remerge it against concurrent range type activity. New patch attached. I'm looking at your patch. One thing that puzzled me for a while was that I could not restrict access to base types (either built-in or user defined). Is this intentional? Works for me: =# create user foo; =# revoke usage on type int8 from public; =# \c - foo = create table test1 (a int4, b int8); ERROR: permission denied for type bigint -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch for type privileges
On mån, 2011-11-28 at 14:25 -0600, Merlin Moncure wrote: On Tue, Nov 15, 2011 at 2:23 PM, Peter Eisentraut pete...@gmx.net wrote: The basics here are mainly informed by the SQL standard. One thing from there I did not implement is checking for permission of a type used in CAST (foo AS type). This would be doable but relatively complicated, and in practice someone how is not supposed to be able to use the type wouldn't be able to create the cast or the underlying cast function anyway for lack of access to the type. I'm not quite following that: with your patch are you or are you not prohibited from utilizing casts? In other words, if you didn't have USAGE priv, what would happen if you tried this: CREATE VIEW v AS SELECT null::restricted_type::text; ? This is not affected by my patch, so it would do whatever it did before. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch for type privileges
On 2011-11-15 21:50, Peter Eisentraut wrote: Patch attached. I cannot get the patch to apply, this is the output of patch -p1 --dry-run on HEAD. patching file src/include/catalog/pg_type.h Hunk #1 succeeded at 217 (offset 1 line). Hunk #2 succeeded at 234 (offset 1 line). Hunk #3 succeeded at 264 (offset 1 line). Hunk #4 succeeded at 281 (offset 1 line). Hunk #5 FAILED at 370. Hunk #6 FAILED at 631. 2 out of 6 hunks FAILED -- saving rejects to file src/include/catalog/pg_type.h.rej I was unable to find a rev to apply the patch to do some testing: this one didn't work either commit 4429f6a9e3e12bb4af6e3677fbc78cd80f160252 Author: Heikki Linnakangas heikki.linnakan...@iki.fi Date: Thu Nov 3 13:16:28 2011 +0200 Support range data types. and that's strange since git log of pg_type.h shows a commit of april before that. regards, Yeb Havinga -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch for type privileges
On Tue, Nov 15, 2011 at 2:23 PM, Peter Eisentraut pete...@gmx.net wrote: The basics here are mainly informed by the SQL standard. One thing from there I did not implement is checking for permission of a type used in CAST (foo AS type). This would be doable but relatively complicated, and in practice someone how is not supposed to be able to use the type wouldn't be able to create the cast or the underlying cast function anyway for lack of access to the type. I'm not quite following that: with your patch are you or are you not prohibited from utilizing casts? In other words, if you didn't have USAGE priv, what would happen if you tried this: CREATE VIEW v AS SELECT null::restricted_type::text; ? merlin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch for type privileges
On mån, 2011-11-28 at 11:41 +0100, Yeb Havinga wrote: On 2011-11-15 21:50, Peter Eisentraut wrote: Patch attached. I cannot get the patch to apply, this is the output of patch -p1 --dry-run on HEAD. patching file src/include/catalog/pg_type.h Hunk #1 succeeded at 217 (offset 1 line). Hunk #2 succeeded at 234 (offset 1 line). Hunk #3 succeeded at 264 (offset 1 line). Hunk #4 succeeded at 281 (offset 1 line). Hunk #5 FAILED at 370. Hunk #6 FAILED at 631. 2 out of 6 hunks FAILED -- saving rejects to file src/include/catalog/pg_type.h.rej I need to remerge it against concurrent range type activity. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] patch for type privileges
Here is the patch to implement type privileges that I alluded to earlier. To recall, this is mainly so that owners can prevent others from using their types because that would in some cases prevent owners from changing the types. That would effectively be a denial of service. These are the interfaces that this patch implements: - GRANT USAGE ON DOMAIN - GRANT USAGE ON TYPE - default privileges for types - analogous REVOKEs - display privileges in psql \dT+ - privilege checks in various DDL commands (CREATE FUNCTION, CREATE TABLE, etc.) - various information schema views adjusted - has_type_privilege function family The basics here are mainly informed by the SQL standard. One thing from there I did not implement is checking for permission of a type used in CAST (foo AS type). This would be doable but relatively complicated, and in practice someone how is not supposed to be able to use the type wouldn't be able to create the cast or the underlying cast function anyway for lack of access to the type. As elsewhere in the system, the usage of TYPE and DOMAIN is partially overlapping and partially not. You can use GRANT ON TYPE on a domain but not GRANT ON DOMAIN on a type (compare CREATE/DROP). We only support one common set of default privileges for types and domains. I feel that's enough, but it could be adjusted. Open items: - GRANT TO ALL TYPES -- haven't gotten to that yet, but could be added A reviewer should of course particularly check if there are any holes in the privilege protection that this patch purports to afford. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch for type privileges
On 15 November 2011 20:23, Peter Eisentraut pete...@gmx.net wrote: Here is the patch to implement type privileges that I alluded to earlier. To recall, this is mainly so that owners can prevent others from using their types because that would in some cases prevent owners from changing the types. That would effectively be a denial of service. These are the interfaces that this patch implements: - GRANT USAGE ON DOMAIN - GRANT USAGE ON TYPE - default privileges for types - analogous REVOKEs - display privileges in psql \dT+ - privilege checks in various DDL commands (CREATE FUNCTION, CREATE TABLE, etc.) - various information schema views adjusted - has_type_privilege function family The basics here are mainly informed by the SQL standard. One thing from there I did not implement is checking for permission of a type used in CAST (foo AS type). This would be doable but relatively complicated, and in practice someone how is not supposed to be able to use the type wouldn't be able to create the cast or the underlying cast function anyway for lack of access to the type. As elsewhere in the system, the usage of TYPE and DOMAIN is partially overlapping and partially not. You can use GRANT ON TYPE on a domain but not GRANT ON DOMAIN on a type (compare CREATE/DROP). We only support one common set of default privileges for types and domains. I feel that's enough, but it could be adjusted. Open items: - GRANT TO ALL TYPES -- haven't gotten to that yet, but could be added A reviewer should of course particularly check if there are any holes in the privilege protection that this patch purports to afford. Want to try again but with the patch attached? ;) -- Thom Brown Twitter: @darkixion IRC (freenode): dark_ixion Registered Linux user: #516935 EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
