Re: 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(at)gmx(dot)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(dot)linnakangas(at)iki(dot)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(at)gmx(dot)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(at)gmx(dot)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, 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

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(at)2ndQuadrant(dot)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.