Обсуждение: 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.
On 15 November 2011 20:23, Peter Eisentraut <peter_e@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
Patch attached. On tis, 2011-11-15 at 22:23 +0200, Peter Eisentraut 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.
Вложения
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.linnakangas@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
On Tue, Nov 15, 2011 at 2:23 PM, Peter Eisentraut <peter_e@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
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.
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.
Вложения
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
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
On mån, 2011-11-28 at 14:25 -0600, Merlin Moncure wrote: > On Tue, Nov 15, 2011 at 2:23 PM, Peter Eisentraut <peter_e@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.
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
On fre, 2011-12-02 at 17:11 +0100, Yeb Havinga wrote: > 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 Two excellent finds. Here is an updated patch with fixes.
Вложения
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, andother 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
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.
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. > > > * 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. Fix for that included. > > * 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. New patch attached.
Вложения
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
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 US greg@2ndQuadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us
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.