Re: [PATCHES] Roles - SET ROLE Updated

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > Tom, if you're watching, are you working on this? I can probably spend
> > some time today on it, if that'd be helpful.
>
> I am not; I was hoping you'd deal with SET ROLE. Is it really much
> different from SET SESSION AUTHORIZATION?

Here's a much better version of the SET ROLE work. I'm reasonably happy
with it. The only parts I don't like are that I had to do some ugly
things in gram.y to avoid making NONE reserved, and I can't seem to see
how to avoid having ROLE be reserved (I understand it was reserved in
SQL99 but not in SQL2003...).

Another issue that I noticed is that when I created a role which didn't
have login permissions, SET ROLE to that role and then created a table,
the 'owner' for the object shown by \d came up NULL. This is almost
certainly because \d is using pg_user which filters out roles which
can't log in. Personally, I disagree with pg_user not having all roles
in it but regardless this needs to be fixed and it'd probably just be
best to update psql to use pg_authid and pg_auth_members, have a \dr,
\dm, etc. I'll try to work on that next unless someone else is already.

Thanks,

Stephen

* Stephen Frost (sfrost(at)snowman(dot)net) wrote:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> > Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > > Tom, if you're watching, are you working on this? I can probably spend
> > > some time today on it, if that'd be helpful.
> >
> > I am not; I was hoping you'd deal with SET ROLE. Is it really much
> > different from SET SESSION AUTHORIZATION?
>
> Here's a much better version of the SET ROLE work. I'm reasonably happy
> with it. The only parts I don't like are that I had to do some ugly
> things in gram.y to avoid making NONE reserved, and I can't seem to see
> how to avoid having ROLE be reserved (I understand it was reserved in
> SQL99 but not in SQL2003...).

Updated yet again, fixing a bug in the prior one that caused it to not
work properly, and some additional things:

Added a 'has_role' function that's basically is_member_of_role for the
masses. Updated information_schema to use has_role for permissions
checks in addition to the straight '=' owner-check. Also fixed up
enabled_roles and applicable_roles views. This depends somewhat on part
of my other patch where I modified is_member_of_role to always return
true for superuser(). If that doesn't end up being done then we'll need
to add some explicit superuser() checks in the SetCurrentRoleId() logic.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
>> Here's a much better version of the SET ROLE work. I'm reasonably happy
>> with it. The only parts I don't like are that I had to do some ugly
>> things in gram.y to avoid making NONE reserved, and I can't seem to see
>> how to avoid having ROLE be reserved (I understand it was reserved in
>> SQL99 but not in SQL2003...).

> Updated yet again, fixing a bug in the prior one that caused it to not
> work properly, and some additional things:

I don't think this patch works; it certainly doesn't do what I'd expect
to happen with SECURITY DEFINER functions. At the very least you'd need
to make fmgr_security_definer save/restore the current role setting.
But I doubt that this is even the direction we want to head in.

After rereading SQL99 4.31, I don't think there is any need to
distinguish CURRENT_USER from CURRENT_ROLE, mainly because our
implementation does not distinguish users from roles at all.
(Which I think is good.) So ISTM we should not change GetUserId()
as you propose, but leave it alone and implement SetRole approximately
like SetSessionUserId is implemented, ie, setting a background value
that may sometimes get copied into CurrentUserId. The "stack" aspect
only matters to the extent that SetRoleId has precedence over
SetSessionUserId for determining the outside-a-transaction value of
CurrentUserId.

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> After rereading SQL99 4.31, I don't think there is any need to
> distinguish CURRENT_USER from CURRENT_ROLE, mainly because our
> implementation does not distinguish users from roles at all.

CURRENT_USER and CURRENT_ROLE can have different values, as I understand
SQL2003, and there are places where one is used instead of the other
(such as with the 'grantor' in grants, according to SQL2003 the
'grantor' should be the CURRENT_USER, regardless of if CURRENT_ROLE is
set or not). I believe this is a seperate issue from how we implement
the accounts themselves (where we don't differentiate between users and
roles, which is fine).

> (Which I think is good.) So ISTM we should not change GetUserId()
> as you propose, but leave it alone and implement SetRole approximately
> like SetSessionUserId is implemented, ie, setting a background value
> that may sometimes get copied into CurrentUserId. The "stack" aspect
> only matters to the extent that SetRoleId has precedence over
> SetSessionUserId for determining the outside-a-transaction value of
> CurrentUserId.

SQL2003 also states that CURRENT_ROLE is NULL initially. I suppose we
could implement CURRENT_ROLE as a check to see if CurrentUserId is
different from CurrentRoleId and return NULL in that case and then just
always use CurrentRoleId (or CurrentUserId, whichever). That would
avoid having to change how GetUserId() works though this doesn't seem
like a huge change to the patch itself. Do you want me to rework the
patch along these lines or are you already working on it? I've been
having a bit of computer trouble but I think I could get the patch
changed/updated by Monday.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> After rereading SQL99 4.31, I don't think there is any need to
>> distinguish CURRENT_USER from CURRENT_ROLE, mainly because our
>> implementation does not distinguish users from roles at all.

> CURRENT_USER and CURRENT_ROLE can have different values, as I understand
> SQL2003, and there are places where one is used instead of the other

It's possible for CURRENT_ROLE to be null according to the spec; if you
like we could implement that as returning what the current outer-level
SET ROLE value is (which would then make it semantically more like
SESSION_USER than CURRENT_USER). I don't think CURRENT_USER should ever
be allowed to be null, or to be different from the active authorization
identifier, first because it's silly and second because it will break
existing applications that depend on CURRENT_USER for authorization
checking.

Given that we don't really distinguish users and roles, I would be
inclined to make the same argument for CURRENT_ROLE too, leaving
SHOW ROLE (and its function equivalent) as the only way to see what
you SET ROLE to. But it's less likely to break existing apps if we
don't.

> (such as with the 'grantor' in grants, according to SQL2003 the
> 'grantor' should be the CURRENT_USER, regardless of if CURRENT_ROLE is
> set or not).

Exactly. CURRENT_USER has to be the active authorization identifier.

> Do you want me to rework the
> patch along these lines or are you already working on it?

I'm working on it ...

regards, tom lane

BTW, I realized we do not support granting roles to PUBLIC:

regression=# create role r;
CREATE ROLE
regression=# grant r to public;
ERROR: role "public" does not exist

but as far as I can tell SQL99 expects this to work.

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> BTW, I realized we do not support granting roles to PUBLIC:
>
> regression=# create role r;
> CREATE ROLE
> regression=# grant r to public;
> ERROR: role "public" does not exist
>
> but as far as I can tell SQL99 expects this to work.

Indeed, I believe you're correct, sorry about missing that.

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> BTW, I realized we do not support granting roles to PUBLIC:
>>
>> regression=# create role r;
>> CREATE ROLE
>> regression=# grant r to public;
>> ERROR: role "public" does not exist
>>
>> but as far as I can tell SQL99 expects this to work.

> Indeed, I believe you're correct, sorry about missing that.

However, on second thought I'm not sure that this is sensible anyway.

Consider that every role is implicitly a member of PUBLIC --- so isn't
the above a creation of a circular membership loop, which is (for good
reason) forbidden by the spec?

regards, tom lane

Another issue: I like the has_role() function and in fact think it needs
to come in multiple variants just like has_table_privilege and friends:

has_role(name, name)
has_role(name, oid)
has_role(oid, name)
has_role(oid, oid)
has_role(name) -- implicitly has_role(current_user, ...)
has_role(oid)

However I'm a bit dubious about whether "has_role" isn't an invasion of
application namespace. pg_has_role would be better, but we have the
(mis) precedent of has_table_privilege. What do you think about calling
it "has_role_privilege"?

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> >> After rereading SQL99 4.31, I don't think there is any need to
> >> distinguish CURRENT_USER from CURRENT_ROLE, mainly because our
> >> implementation does not distinguish users from roles at all.
>
> > CURRENT_USER and CURRENT_ROLE can have different values, as I understand
> > SQL2003, and there are places where one is used instead of the other
>
> It's possible for CURRENT_ROLE to be null according to the spec; if you
> like we could implement that as returning what the current outer-level
> SET ROLE value is (which would then make it semantically more like
> SESSION_USER than CURRENT_USER). I don't think CURRENT_USER should ever
> be allowed to be null, or to be different from the active authorization
> identifier, first because it's silly and second because it will break
> existing applications that depend on CURRENT_USER for authorization
> checking.

Sorry about the existing applications, but this does go directly against
the SQL2003 specification. At least from my reading of SQL2003 5.37
ROLE_COLUMN_GRANTS view, which 'Identifies the privileges on columns
defined in this catalog that are available to or granted by the
currently enabled roles':

WHERE ( GRANTEE IN ( SELECT ROLE_NAME FROM ENABLED_ROLES )

Where the ENABLED_ROLES view operates specifically off of the
'CURRENT_ROLE' value.

> Given that we don't really distinguish users and roles, I would be
> inclined to make the same argument for CURRENT_ROLE too, leaving
> SHOW ROLE (and its function equivalent) as the only way to see what
> you SET ROLE to. But it's less likely to break existing apps if we
> don't.

I don't quite follow this- the point of SET ROLE is to change your
authorization identifier to be a specific role instead of the current
role. What I had thought you were suggesting was to make it so that
after a SET ROLE the CURRENT_USER shows what you SET ROLE to. This
sounds like SET ROLE is just there for looks and completely ignored for
authorization purposes, making it next to useless.

> > (such as with the 'grantor' in grants, according to SQL2003 the
> > 'grantor' should be the CURRENT_USER, regardless of if CURRENT_ROLE is
> > set or not).
>
> Exactly. CURRENT_USER has to be the active authorization identifier.

No, that's an exception, and only for what ends up in the table recorded
as the 'grantor'. Re-reading 4.34 it's apparently actually supposed to
be a "last-in, first-out" mechanism, though I don't see any way for a
user (beyond a connect statement) to actually change CURRENT_USER,
unlike SET ROLE which can be used to change CURRENT_ROLE (and in so
doing put it at the top of the 'stack'). Technically I believe this
actually allows multiple levels of 'SET ROLE's to be done and for 'SET
ROLE NONE's to only pull off the top-level. My patch didn't handle
such multi-level SET ROLE's, but it's certainly something which could be
done.

Thanks,

Stephen

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> However, on second thought I'm not sure that this is sensible anyway.
>
> Consider that every role is implicitly a member of PUBLIC --- so isn't
> the above a creation of a circular membership loop, which is (for good
> reason) forbidden by the spec?

Ah, yes, you're right. I won't claim to have considered that in the
original working of the patch though. :)

Stephen

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Another issue: I like the has_role() function and in fact think it needs
> to come in multiple variants just like has_table_privilege and friends:
>
> has_role(name, name)
> has_role(name, oid)
> has_role(oid, name)
> has_role(oid, oid)
> has_role(name) -- implicitly has_role(current_user, ...)
> has_role(oid)
>
> However I'm a bit dubious about whether "has_role" isn't an invasion of
> application namespace. pg_has_role would be better, but we have the
> (mis) precedent of has_table_privilege. What do you think about calling
> it "has_role_privilege"?

I thought about that originally. It seemed a bit long to me and I felt
that having the 'privilege' of a role wasn't quite the same as having a
'role', but honestly I'm not terribly picky and on reflection a role
*is* like other objects in the catalog (I originally hadn't considered
it such), so, that's fine with me...

has_role() was another reason I was thinking about having a seperate
function for 'is_member_of_role' which didn't pollute the cache, just a
side-note.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> It's possible for CURRENT_ROLE to be null according to the spec; if you
>> like we could implement that as returning what the current outer-level
>> SET ROLE value is (which would then make it semantically more like
>> SESSION_USER than CURRENT_USER). I don't think CURRENT_USER should ever
>> be allowed to be null, or to be different from the active authorization
>> identifier, first because it's silly and second because it will break
>> existing applications that depend on CURRENT_USER for authorization
>> checking.

> Sorry about the existing applications, but this does go directly against
> the SQL2003 specification.

The spec isn't sufficiently well-designed in this area to make me
willing to insert security holes into existing apps in order to follow
it slavishly. They clearly failed to think through the
grant-role-to-PUBLIC business, and the whole distinction between users
and roles is pretty artificial anyway.

> At least from my reading of SQL2003 5.37
> ROLE_COLUMN_GRANTS view, which 'Identifies the privileges on columns
> defined in this catalog that are available to or granted by the
> currently enabled roles':

> WHERE ( GRANTEE IN ( SELECT ROLE_NAME FROM ENABLED_ROLES )

> Where the ENABLED_ROLES view operates specifically off of the
> 'CURRENT_ROLE' value.

OK, so we make CURRENT_ROLE return the SET ROLE value (possibly NULL).

I notice that the privilege-related info schema views consistently check
privileges via locutions like

WHERE ( SCHEMA_OWNER = CURRENT_USER
OR
SCHEMA_OWNER IN
( SELECT ROLE_NAME
FROM ENABLED_ROLES ) )

which is a tad odd if it's intended to model the privileges you
currently have; the implication of that is that you cannot drop any of
your "login ID"'s privileges by doing SET ROLE, which surely is not
the intended behavior (else you might as well not have SET ROLE at all;
the only possible use of SET ROLE is to *restrict* your privileges,
since any role you can become represents privileges you'd have anyway
without SET ROLE). So I'm pretty unconvinced that the spec is being
self-consistent here.

> Technically I believe this
> actually allows multiple levels of 'SET ROLE's to be done and for 'SET
> ROLE NONE's to only pull off the top-level.

I don't see anything in the spec that suggests that reading to me.

regards, tom lane

Tom Lane wrote:

>
>However I'm a bit dubious about whether "has_role" isn't an invasion of
>application namespace. pg_has_role would be better, but we have the
>(mis) precedent of has_table_privilege. What do you think about calling
>it "has_role_privilege"?
>
>
>
>

Do we need to follow a bad precedent for the sake of consistency? If
forced to choose, in general I would prefer to sacrifice consistency.

cheers

andrew (old Emersonian)

I wrote:
> ... the implication of that is that you cannot drop any of
> your "login ID"'s privileges by doing SET ROLE, which surely is not
> the intended behavior (else you might as well not have SET ROLE at all;
> the only possible use of SET ROLE is to *restrict* your privileges,
> since any role you can become represents privileges you'd have anyway
> without SET ROLE). So I'm pretty unconvinced that the spec is being
> self-consistent here.

After some further study I see where the disconnect is coming from:
what we've implemented isn't quite what the spec has in mind. Look
at SQL99 4.31.4:

4.31.4 Security model definitions

The set of applicable roles for an <authorization identifier>
consists of all roles defined by the role authorization descriptors
whose grantee is that <authorization identifier> or PUBLIC together
with all other roles they contain.

The set of user privileges for a <user identifier> consists of all
privileges defined by the privilege descriptors whose grantee is
either that <user identifier> or PUBLIC.

The set of role privileges for a <role name> consists of all
privileges defined by the privilege descriptors whose grantee is
either that <role name>, PUBLIC, or one of the applicable roles of
that <role name>.

What this says is that when a role A is a member of another role B, A
automatically has all of B's privileges. But when a user U is a member
of role R, U does *not* have R's privileges automatically. What he has
is the right to do SET ROLE R, after which he has R's privileges in
addition to his own (see the rest of 4.31.4).

This is ... um ... a pretty bizarre way of looking at security.
U can in fact do whatever his roles allow him to do, he just needs to
say "Mother may I?" first. I suppose the fact that the spec only allows
SET ROLE at the outer level (outside any transaction) provides some
veneer of security against Trojan-horse functions, but it sure looks
lame.

But anyway, it seems that the spec sees SET ROLE as an operation that
gets you additional privileges, not as an operation that restricts your
privileges.

I don't think we can possibly emulate this definition unless we make
some pretty fundamental changes in the way the ROLE patch works.
In particular, is_member_of_role isn't in general the right way to
check applicability of privileges.

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > Sorry about the existing applications, but this does go directly against
> > the SQL2003 specification.
>
> The spec isn't sufficiently well-designed in this area to make me
> willing to insert security holes into existing apps in order to follow
> it slavishly. They clearly failed to think through the
> grant-role-to-PUBLIC business, and the whole distinction between users
> and roles is pretty artificial anyway.

Perhaps the specification isn't but I'm pretty sure other
implementations follow the SET ROLE -> current authorization
identifier (and thus dropping other rights granted to the CURRENT_USER).

Having thought about this a bit more I'd like to know what security
holes you're thinking would be introduced by this change. CURRENT_USER
was always required to be set in my original patch, and SET ROLE didn't
exist before and only ever dropped privileges anyway. A current app is
rather unlikely I'd think to use SET ROLE and *then* base authorization
decisions off the value of CURRENT_USER...

I suppose I'm being dense but I'd like to get a better explanation of
the specific problem before trying to come up with an acceptable
solution.

> > At least from my reading of SQL2003 5.37
> > ROLE_COLUMN_GRANTS view, which 'Identifies the privileges on columns
> > defined in this catalog that are available to or granted by the
> > currently enabled roles':
>
> > WHERE ( GRANTEE IN ( SELECT ROLE_NAME FROM ENABLED_ROLES )
>
> > Where the ENABLED_ROLES view operates specifically off of the
> > 'CURRENT_ROLE' value.
>
> OK, so we make CURRENT_ROLE return the SET ROLE value (possibly NULL).
>
> I notice that the privilege-related info schema views consistently check
> privileges via locutions like
>
> WHERE ( SCHEMA_OWNER = CURRENT_USER
> OR
> SCHEMA_OWNER IN
> ( SELECT ROLE_NAME
> FROM ENABLED_ROLES ) )
>
> which is a tad odd if it's intended to model the privileges you
> currently have; the implication of that is that you cannot drop any of
> your "login ID"'s privileges by doing SET ROLE, which surely is not
> the intended behavior (else you might as well not have SET ROLE at all;
> the only possible use of SET ROLE is to *restrict* your privileges,
> since any role you can become represents privileges you'd have anyway
> without SET ROLE). So I'm pretty unconvinced that the spec is being
> self-consistent here.

Looking back on it I'd have to agree that there does seem something a
bit odd here. There are some places where it's limited to the current
role (the ROLE_*_GRANTS that I had originally been looking at) but other
places indicate cases where the 'user' is the 'owner', or is in the role
of the 'owner'. The grantee cases tend to have 'public', CURRENT_USER
or an enabled_role. Interestingly, there *is* a distinction that's made
here, when you think about it:

This lists things which the CURRENT_USER or the ENABLED_ROLES (via a SET
ROLE) has access to. This does *not* list objects in the
APPLICABLE_ROLES set. This indicates that SET ROLE *does* drop
privileges, but you may still see objects which the underlying user can
directly, but not things which the underlying user can see indirectly
through other roles (unless those other roles are available under
ENABLED_ROLES).

The odd bit is that this doesn't seem to handle the case where
CURRENT_ROLE is NULL very cleanly- if you've not SET ROLE then it's
expected you have access to anything which a role you've been granted
has access to, instead you only see those things which you directly own
or which are available to 'public'.

I recall you telling me to go back and look at the spec at one point
regarding what a given user could see via information_schema and to
submit a patch if something in information_schema was wrong. Well,
seems like perhaps information_schema might have been following the
spec (since this isn't what I would have expected).

> > Technically I believe this
> > actually allows multiple levels of 'SET ROLE's to be done and for 'SET
> > ROLE NONE's to only pull off the top-level.
>
> I don't see anything in the spec that suggests that reading to me.

It's in 4.34.1.1, at least in the SQL2003 specification, and it reads:
"This stack is maintained using a "last-in, first-out" discipline, and
effectively only the top cell is visible. When an SQL-session is
started, by explicit or implicit execution of a <connect statement>, the
authorization stack is initialized with one cell, which contains only
the user identifier known as the SQL-session user identifier, a role
name, known as the SQL-session role name may be added subsequently."

It also says:
"The <set session user identifier statement> changes the value of the
current user identifier and of the SQL session user identifier. The
<set role statement> changes the value of the current role name."

Which does seem to conflict. Were it meaning that SET ROLE pushes onto
the stack I'd expect the wording to reflect that instead of saying
"chagnes". This stack-like behaviour of multiple set-role statements
isn't something I can currently think I'd have any need for, but it does
more closely follow how 'su's in Unix work.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> Perhaps the specification isn't but I'm pretty sure other
> implementations follow the SET ROLE -> current authorization
> identifier (and thus dropping other rights granted to the CURRENT_USER).

My current reading of 4.31 is that SET ROLE *doesn't* drop rights, which
means we need to rethink all of this. However, on this point:

>>> Technically I believe this
>>> actually allows multiple levels of 'SET ROLE's to be done and for 'SET
>>> ROLE NONE's to only pull off the top-level.
>>
>> I don't see anything in the spec that suggests that reading to me.

> It's in 4.34.1.1, at least in the SQL2003 specification, and it reads:
> "This stack is maintained using a "last-in, first-out" discipline, and
> effectively only the top cell is visible.

Yes, but the only events that push or pop stack entries are entry/exit
of an external procedure (think SECURITY DEFINER procedure). SET ROLE
doesn't push or pop anything, it just alters the current top entry.
(Which must in fact be the *only* entry, given that SET ROLE is only
allowed at outer level...)

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> What this says is that when a role A is a member of another role B, A
> automatically has all of B's privileges. But when a user U is a member
> of role R, U does *not* have R's privileges automatically. What he has
> is the right to do SET ROLE R, after which he has R's privileges in
> addition to his own (see the rest of 4.31.4).

Indeed, when I was looking through the information_schema views more
closely I was starting to realize something like this was going on.

> This is ... um ... a pretty bizarre way of looking at security.
> U can in fact do whatever his roles allow him to do, he just needs to
> say "Mother may I?" first. I suppose the fact that the spec only allows
> SET ROLE at the outer level (outside any transaction) provides some
> veneer of security against Trojan-horse functions, but it sure looks
> lame.
>
> But anyway, it seems that the spec sees SET ROLE as an operation that
> gets you additional privileges, not as an operation that restricts your
> privileges.

Yeah, myself, and at least one other person that I recall asking after
this stuff, felt it was the opposite.

> I don't think we can possibly emulate this definition unless we make
> some pretty fundamental changes in the way the ROLE patch works.
> In particular, is_member_of_role isn't in general the right way to
> check applicability of privileges.

It is, and it isn't... It's correct for checking role-privileges, just
not for user-privileges. That is to say, is_member_of_role works for
when CURRENT_ROLE is set, and should be started based off of whatever
CURRENT_ROLE is set to. If CURRENT_ROLE is not set then I don't think
it can be used.

Thanks,

Stephen

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > Perhaps the specification isn't but I'm pretty sure other
> > implementations follow the SET ROLE -> current authorization
> > identifier (and thus dropping other rights granted to the CURRENT_USER).
>
> My current reading of 4.31 is that SET ROLE *doesn't* drop rights, which
> means we need to rethink all of this. However, on this point:

Yeah, it seems to add them. Honestly, my recollection from working on
Oracle is that you have access to all the roles you've been granted when
you connect as a user. If it'd be useful I'd be happy to see about
finding out exactly what Oracle does in this regard and if it actually
follows the specification or what.

> > It's in 4.34.1.1, at least in the SQL2003 specification, and it reads:
> > "This stack is maintained using a "last-in, first-out" discipline, and
> > effectively only the top cell is visible.
>
> Yes, but the only events that push or pop stack entries are entry/exit
> of an external procedure (think SECURITY DEFINER procedure). SET ROLE
> doesn't push or pop anything, it just alters the current top entry.
> (Which must in fact be the *only* entry, given that SET ROLE is only
> allowed at outer level...)

Alright, that would make the later language make more sense.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> My current reading of 4.31 is that SET ROLE *doesn't* drop rights, which
>> means we need to rethink all of this. However, on this point:

> Yeah, it seems to add them. Honestly, my recollection from working on
> Oracle is that you have access to all the roles you've been granted when
> you connect as a user. If it'd be useful I'd be happy to see about
> finding out exactly what Oracle does in this regard and if it actually
> follows the specification or what.

Yeah, let's take a look. This wouldn't be the first part of the spec
we've come across that is mostly honored in the breach...

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > Perhaps the specification isn't but I'm pretty sure other
> > implementations follow the SET ROLE -> current authorization
> > identifier (and thus dropping other rights granted to the CURRENT_USER).
>
> My current reading of 4.31 is that SET ROLE *doesn't* drop rights, which
> means we need to rethink all of this. However, on this point:

Reviewing:
http://www.psoug.org/reference/roles.html

(Top link in Google - Oracle Roles):

Oracle allows a 'SET ROLE all;' syntax, which is essentially what we're
currently automatically doing. You can't deactivate a specific role,
but you can deactivate all roles using 'SET ROLE none;'. Interestingly,
on at least one Oracle setup it appears that it also has an implicit
'SET ROLE all;'. Check this out:

-----------------------------------------------------------------
melkor> sqlplus

SQL> select * from session_roles;

ROLE
------------------------------
CONNECT
NORMAL

SQL> SET ROLE none;

Role set.

SQL> select * from session_roles;

no rows selected

SQL>
-----------------------------------------------------------------

Doing this doesn't seem entirely unreasonable but we don't currently
have a way of handling 'SET ROLE none;'. We'd need to make some changes
but I think we could handle it, and correctly handle a specific
'SET ROLE <role>', which under Oracle does appear to drop any other
roles you currently have.

Thanks,

Stephen

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Yeah, let's take a look. This wouldn't be the first part of the spec
> we've come across that is mostly honored in the breach...

Oracle appears to mostly follow it, except there's an implicit
'SET ROLE all;', at least in the instance I'm looking at. I'm guessing
it's probably something which is tunable. We don't currently have a way
of having certain roles turned on and certain ones turned off for a
given session. That doesn't seem like it'd be a very hard thing to add
though. I have to apologize for not realizing this sooner and
implementing it correctly the first time, sorry about that..

Thanks,

Stephen

* Stephen Frost (sfrost(at)snowman(dot)net) wrote:
> Doing this doesn't seem entirely unreasonable but we don't currently
> have a way of handling 'SET ROLE none;'. We'd need to make some changes
> but I think we could handle it, and correctly handle a specific
> 'SET ROLE <role>', which under Oracle does appear to drop any other
> roles you currently have.

Thinking about this a bit more.. Basically what we have is:

An implicit 'SET ROLE all;' on session connect, like Oracle does.
Support from the patch for an explicit 'SET ROLE <role>;', which drops
privileges for all other roles except the role set. The only change to
correctly support that would be to add 'CURRENT_USER' back into the
resulting set of 'enabled_roles' (but not doing so recursively or we're
back to 'SET ROLE all;'). You don't appear to be able to drop rights
which you have via CURRENT_USER.

To support having certain roles turned on and certain roles turned off
would be some additional effort. I think we'd need a list of
'ENABLED_ROLES' and then correct recursion based off of that list
instead of just starting from a single point like we do now.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> To support having certain roles turned on and certain roles turned off
> would be some additional effort. I think we'd need a list of
> 'ENABLED_ROLES' and then correct recursion based off of that list
> instead of just starting from a single point like we do now.

That seems fairly ugly and messy though, because it wouldn't readily
translate to the case of SECURITY DEFINER procedures.

Let's review the bidding. As I presently understand it:

1. At session start, you are the SESSION_USER, which is also
CURRENT_USER, and (per spec anyway) CURRENT_ROLE should be NULL.
You have only the privileges granted directly to SESSION_USER
or PUBLIC.

2. You are allowed to SET ROLE to any role that the SESSION_USER is
(directly or indirectly) a member of. After you do so, the spec
says that CURRENT_USER is still SESSION_USER, CURRENT_ROLE is now
the selected role, and you have the union of the rights of those
two authorization identifiers.

3. If you enter a module (call a SECURITY DEFINER procedure), then
the current authorization identifier becomes that of the owner
of the module/procedure. Per spec this identifier is either a
user or a role, not both, and either CURRENT_USER or CURRENT_ROLE
reflects that with the other becoming NULL. (The authorization
stack will pop back to the prior state when you exit the call.)
Note: this is the case that allows CURRENT_USER to become null.

4. I'm not totally clear about whether the spec allows a module to be
entered without starting a transaction. If so, it would be legal
for a module owned by a user (not a role) to SET ROLE to one of
the roles that user is a member of. Again this would have effect
only till module exit.

Now #4 doesn't currently apply to us, since we have neither modules
nor procedures that can be entered outside a transaction. And frankly
I don't see the point in it anyway --- you may as well let the module
or procedure be owned by the desired role in the first place.

ISTM most of the messiness here comes from the spec's hard-and-fast
distinction between users and roles, which they then have to blur again
in order to talk about "authorization identifiers". Our current
implementation treats these as a single kind of entity with some
optional attributes (eg rolcanlogin), and I think that that's a much
cleaner way of doing it.

What I would like to do is say that there is exactly one active
authorization identifier at any instant. That means that if you SET
ROLE then you no longer have the privileges attached to SESSION_USER
(except the right to SET ROLE to a different one of his roles).
This is contrary to spec but it seems to be a pretty widely accepted
way of doing things; and it makes SET ROLE effectively equivalent to
the change in privileges associated with entering a SECURITY DEFINER
procedure owned by that role, so there's a much cleaner conceptual
model.

Furthermore, I still feel that both CURRENT_USER and CURRENT_ROLE should
reflect this single active authorization id. The spec's distinction is
artificial and doesn't gain anything, except the ability to break code
that assumes CURRENT_USER is always meaningful.

If we don't do it that way then we have a bunch of API that breaks down:
all of the has_foo_privilege functions stop working, because they don't
have a signature that allows both a user and a role to be passed to
them.

I think we do need to invent the concept of roles (pg_authid entries)
that either do or do not inherit privileges from roles they are members
of. A non-inheriting role can do SET ROLE to gain the privileges of a
role it is a member of, but it does not have those privileges without
SET ROLE. We could drive this off rolcanlogin but it's probably better
to invent an additional flag column in pg_authid (call it rolinherit,
maybe). Users by default would not inherit, which puts us a great deal
closer to the spec's semantics.

Thoughts?

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > To support having certain roles turned on and certain roles turned off
> > would be some additional effort. I think we'd need a list of
> > 'ENABLED_ROLES' and then correct recursion based off of that list
> > instead of just starting from a single point like we do now.
>
> That seems fairly ugly and messy though, because it wouldn't readily
> translate to the case of SECURITY DEFINER procedures.

SECURITY DEFINER is just another level in the stack, one which is above
all others. At least, in my thinking anyway. We don't actually need a
stack, just need to save off the old user/role list and reinstate it
after the SECURITY DEFINER function.

> Let's review the bidding. As I presently understand it:
[...]

That all looks right.

> What I would like to do is say that there is exactly one active
> authorization identifier at any instant. That means that if you SET
> ROLE then you no longer have the privileges attached to SESSION_USER
> (except the right to SET ROLE to a different one of his roles).
> This is contrary to spec but it seems to be a pretty widely accepted
> way of doing things; and it makes SET ROLE effectively equivalent to
> the change in privileges associated with entering a SECURITY DEFINER
> procedure owned by that role, so there's a much cleaner conceptual
> model.

I'd rather get away from the idea of only one 'active authorization
identifier'. That's rather limiting and I don't really see the point.
All that's changing is what seeds the initial list of roles prior to
doing the full hierarchical resolution to populate the full list of
roles. The original patch used CURRENT_USER or CURRENT_ROLE if it was
set. This just adds CURRENT_USER back in at the end if we used
CURRENT_ROLE initially. Except in a 'SET ROLE all' case, when we seed
with CURRENT_USER, as I had originally.

> Furthermore, I still feel that both CURRENT_USER and CURRENT_ROLE should
> reflect this single active authorization id. The spec's distinction is
> artificial and doesn't gain anything, except the ability to break code
> that assumes CURRENT_USER is always meaningful.

There is a distinction there though, at least as the SQL spec works, and
really there is when you consider that CURRENT_USER is really the
SESSION_USER generally.

> If we don't do it that way then we have a bunch of API that breaks down:
> all of the has_foo_privilege functions stop working, because they don't
> have a signature that allows both a user and a role to be passed to
> them.

It seems like there might be a way to solve this better but I'm not
coming up with it yet.

> I think we do need to invent the concept of roles (pg_authid entries)
> that either do or do not inherit privileges from roles they are members
> of. A non-inheriting role can do SET ROLE to gain the privileges of a
> role it is a member of, but it does not have those privileges without
> SET ROLE. We could drive this off rolcanlogin but it's probably better
> to invent an additional flag column in pg_authid (call it rolinherit,
> maybe). Users by default would not inherit, which puts us a great deal
> closer to the spec's semantics.

I really don't care for this idea as an alternative to what the spec
calls for. I don't think it puts us much closer to the spec's semantics
unless you let a user change pg_authid wrt this and that seems like
quite a bad idea.

> Thoughts?

Sorry if it's not entirely coherent, I've been thinking about this alot
over the past couple of hours. I'm going to sleep on it and hopefully
write up something better tommorow. Basically my thinking is that in
general the 'list of roles current enabled' is what we've currently got
already and that works. We're just changing how and what gets
added/removed from it.

Thanks,

Stephen

Am Donnerstag, 21. Juli 2005 22:55 schrieb Tom Lane:
> What this says is that when a role A is a member of another role B, A
> automatically has all of B's privileges. But when a user U is a member
> of role R, U does *not* have R's privileges automatically. What he has
> is the right to do SET ROLE R, after which he has R's privileges in
> addition to his own (see the rest of 4.31.4).
>
> This is ... um ... a pretty bizarre way of looking at security.
> U can in fact do whatever his roles allow him to do, he just needs to
> say "Mother may I?" first.

In some circles, this is considered the standard behavior of role security
models. (There is a NIST standard somewhere.) It allows (with additional
work) dynamic separation of concerns, namely that you could be a member of
roles A and B but cannot use both at the same time. This is admittedly a
fairly advanced feature, but should nevertheless be kept in mind.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

* Peter Eisentraut (peter_e(at)gmx(dot)net) wrote:
> Am Donnerstag, 21. Juli 2005 22:55 schrieb Tom Lane:
> > What this says is that when a role A is a member of another role B, A
> > automatically has all of B's privileges. But when a user U is a member
> > of role R, U does *not* have R's privileges automatically. What he has
> > is the right to do SET ROLE R, after which he has R's privileges in
> > addition to his own (see the rest of 4.31.4).
> >
> > This is ... um ... a pretty bizarre way of looking at security.
> > U can in fact do whatever his roles allow him to do, he just needs to
> > say "Mother may I?" first.
>
> In some circles, this is considered the standard behavior of role security
> models. (There is a NIST standard somewhere.) It allows (with additional
> work) dynamic separation of concerns, namely that you could be a member of
> roles A and B but cannot use both at the same time. This is admittedly a
> fairly advanced feature, but should nevertheless be kept in mind.

It sounds like this is essentially if 'SET ROLE all;' is allowed or not.
If you disallow 'SET ROLE all;' (and therefore not do it on session
start) then you would get this behaviour. I certainly see that as a
reasonable option though I think we'd want to allow 'SET ROLE all;' for
backwards compatibility to group-based systems.

This doesn't change that even after a SET ROLE <role>; you would have
the permissions available via CURRENT_USER.

Thinking about it a bit more, in our context a 'SET ROLE all;' is really
a 'SET ROLE <login-role-name>;', which I'd think would therefore mean
that upon login in a 'default' setup we'd have CURRENT_USER and
CURRENT_ROLE set to the same thing.

My patch would need to be modified to add CURRENT_USER to the role-cache
when CURRENT_USER != CURRENT_ROLE, *after* the rest of the resolution,
of course. We'd then need an option for if the 'SET ROLE all;' is done
on connect or not, perhaps seperately an option saying if it's allowed
at all, and syntax modifications to all for 'SET ROLE all;', etc.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> It sounds like this is essentially if 'SET ROLE all;' is allowed or not.
> If you disallow 'SET ROLE all;' (and therefore not do it on session
> start) then you would get this behaviour. I certainly see that as a
> reasonable option though I think we'd want to allow 'SET ROLE all;' for
> backwards compatibility to group-based systems.

'SET ROLE all' is nonstandard; it will complicate the implementation a
great deal; and it creates a problem with the permissions environment of
a SECURITY DEFINER function being different from that seen at the outer
level by the same user.

I think a better answer is to have a "rolinherit" flag in pg_authid,
which people can set "off" for spec compatibility or "on" for backwards
compatibility to the GROUP feature. In either setting, the permissions
given to a particular authid are clear from pg_authid and don't vary
depending on magic SET variables.

regards, tom lane

* Stephen Frost (sfrost(at)snowman(dot)net) wrote:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> > If we don't do it that way then we have a bunch of API that breaks down:
> > all of the has_foo_privilege functions stop working, because they don't
> > have a signature that allows both a user and a role to be passed to
> > them.
>
> It seems like there might be a way to solve this better but I'm not
> coming up with it yet.

Alright, I've thought about this some more. I think there's two ways to
deal with this. One is the solution you proposed below where we have
per-role 'rolinherit' and answers are based off of that. The other is
to just base it off of the question of if 'SET ROLE all;' is allowed or
not. If not then we basically use 'rolcanlogin' as a proxy for
'rolinherit'. Otherwise the logic stays the same as it is currently.

I like your solution more but I see it as something to do to go above
what the spec calls for, not as a replacement for doing what the spec
requires. I *think* with your solution we wouldn't need the explicit
'SET ROLE all;'-allowed option, as that would just correspond to setting
rolinherit = !rolcanlogin;.

> > I think we do need to invent the concept of roles (pg_authid entries)
> > that either do or do not inherit privileges from roles they are members
> > of. A non-inheriting role can do SET ROLE to gain the privileges of a
> > role it is a member of, but it does not have those privileges without
> > SET ROLE. We could drive this off rolcanlogin but it's probably better
> > to invent an additional flag column in pg_authid (call it rolinherit,
> > maybe). Users by default would not inherit, which puts us a great deal
> > closer to the spec's semantics.

This sounds like a good addition, though to retain backwards
compatibility to group-based systems I'd think we'd either have to have
rolinherit on by default or cause pg_dump to turn it on when coming from
group-based systems (the latter would probably be better, if it's
possible).

> Sorry if it's not entirely coherent, I've been thinking about this alot
> over the past couple of hours. I'm going to sleep on it and hopefully
> write up something better tommorow. Basically my thinking is that in
> general the 'list of roles current enabled' is what we've currently got
> already and that works. We're just changing how and what gets
> added/removed from it.

In the end I think we can have just one top-level to work from, we just
have to add CURRENT_USER to the list of roles after the resolution if
it's not in it already and it wasn't used as the base. Otherwise I
think what I had pretty much works except that we do 'SET ROLE all;'
initially, which just sets CURRENT_ROLE = CURRENT_USER and does the
role-cache generation off that. When a SET ROLE is done we change
CURRENT_ROLE to the new role and regenerate the role-cache based off the
CURRENT_ROLE and add CURRENT_USER to the list at the end. This does
mean that SET ROLE can't work off the cache, but that doesn't seem all
that terrible to me.

Thanks,

Stephen

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > It sounds like this is essentially if 'SET ROLE all;' is allowed or not.
> > If you disallow 'SET ROLE all;' (and therefore not do it on session
> > start) then you would get this behaviour. I certainly see that as a
> > reasonable option though I think we'd want to allow 'SET ROLE all;' for
> > backwards compatibility to group-based systems.
>
> 'SET ROLE all' is nonstandard; it will complicate the implementation a
> great deal; and it creates a problem with the permissions environment of
> a SECURITY DEFINER function being different from that seen at the outer
> level by the same user.

'SET ROLE all' is nonstandard but done in practice.

> I think a better answer is to have a "rolinherit" flag in pg_authid,
> which people can set "off" for spec compatibility or "on" for backwards
> compatibility to the GROUP feature. In either setting, the permissions
> given to a particular authid are clear from pg_authid and don't vary
> depending on magic SET variables.

This is nonstandard and not done in practice. Authorization changes
being allowed by 'SET ROLE' is what the spec calls for. Not supporting
that ability would be unfortunate and it seems there'd be no point to
having 'SET ROLE' at all.

Stephen

[ getting back to this thread... ]

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> I think a better answer is to have a "rolinherit" flag in pg_authid,
>> which people can set "off" for spec compatibility or "on" for backwards
>> compatibility to the GROUP feature. In either setting, the permissions
>> given to a particular authid are clear from pg_authid and don't vary
>> depending on magic SET variables.

> This is nonstandard and not done in practice. Authorization changes
> being allowed by 'SET ROLE' is what the spec calls for. Not supporting
> that ability would be unfortunate and it seems there'd be no point to
> having 'SET ROLE' at all.

I think maybe you misunderstood what I was suggesting. The function of
the flag as I imagine it is:

* rolinherit = false: role does not automatically have the privileges of
roles it is a member of. It must do SET ROLE to gain the privileges of
a role it is a member of. (This emulates the spec behavior for users.)

* rolinherit = true: role has the privileges of all roles it is a member
of, without needing to do SET ROLE. (This handles the spec behavior for
roles, and is also needed for users when backwards compatibility with our
old behavior for groups is wanted, and also provides an approximate
equivalent to Oracle's SET ROLE ALL.)

If users have rolinherit = false and roles have rolinherit = true,
everything behaves per spec, except that I don't want to support the
aspect of the spec that says you can SET ROLE at the outer level and
still have the privileges of the SESSION_USER. I think SET ROLE should
effectively drop the SESSION_USER's privileges (except that subsequent
SET ROLE commands will be checked against the SESSION_USER's role
memberships, not the current effective role).

If both users and roles have rolinherit = true, we have a good emulation
of the old group-based behavior. For backwards compatibility we
probably have to have CREATE USER defaulting to rolinherit = true.
Is it sufficient to say "if you want the spec-compatible behavior you
always have to say CREATE USER ... NOINHERIT"? Since the spec doesn't
actually define a CREATE USER command, this is not a spec violation in a
technical sense. But people who are migrating towards using SET ROLE
might wish it defaulted to NOINHERIT. We could (either now or in a
future release) add a GUC variable to control the default, I suppose.

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> [ getting back to this thread... ]

Happy to, was getting worried you'd forgotten or ignored it. ;)

> * rolinherit = false: role does not automatically have the privileges of
> roles it is a member of. It must do SET ROLE to gain the privileges of
> a role it is a member of. (This emulates the spec behavior for users.)
>
> * rolinherit = true: role has the privileges of all roles it is a member
> of, without needing to do SET ROLE. (This handles the spec behavior for
> roles, and is also needed for users when backwards compatibility with our
> old behavior for groups is wanted, and also provides an approximate
> equivalent to Oracle's SET ROLE ALL.)
>
> If users have rolinherit = false and roles have rolinherit = true,
> everything behaves per spec, except that I don't want to support the
> aspect of the spec that says you can SET ROLE at the outer level and
> still have the privileges of the SESSION_USER. I think SET ROLE should
> effectively drop the SESSION_USER's privileges (except that subsequent
> SET ROLE commands will be checked against the SESSION_USER's role
> memberships, not the current effective role).

I don't particularly like deviating from the spec in this regard (since
I don't think it'd be all that hard to implement what the spec calls
for), but it doesn't bother me that much.

> If both users and roles have rolinherit = true, we have a good emulation
> of the old group-based behavior. For backwards compatibility we
> probably have to have CREATE USER defaulting to rolinherit = true.

While I agree that this is what Oracle's SET ROLE ALL does initially,
it's possible for a user to 'SET ROLE <a>' and drop the permissions
given by the other roles in which the user is in. Will that still be
possible with your proposed solution, or will doing 'SET ROLE <a>' have
no effect when 'rolinherit = true'? That's really my main concern.

For my systems I expect to want to do 'rolinherit = true' generally but
I really don't like the idea that 'SET ROLE <a>' has no effect then.

Thinking about it a bit more I suppose I could live with it since it's
per-role and I tend to set up unprivileged accounts, which is where I'd
really be more concerned about 'SET ROLE <a>' working. We should
probably issue a warning or something if my hypothosis on 'SET ROLE'
above is correct in the 'rolinherit = true' case so that people don't
get the wrong idea that they've dropped privileges in cases when they
actually havn't.

> Is it sufficient to say "if you want the spec-compatible behavior you
> always have to say CREATE USER ... NOINHERIT"? Since the spec doesn't
> actually define a CREATE USER command, this is not a spec violation in a
> technical sense. But people who are migrating towards using SET ROLE
> might wish it defaulted to NOINHERIT. We could (either now or in a
> future release) add a GUC variable to control the default, I suppose.

Being able to control the default would be nice but I don't believe it
would be a requirement. I would actually like to have a variable to
control if SESSION_USER privileges are kept across a SET ROLE or not,
though primairly to conform to the spec than expectation that I'd
personally use it much.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> everything behaves per spec, except that I don't want to support the
>> aspect of the spec that says you can SET ROLE at the outer level and
>> still have the privileges of the SESSION_USER. I think SET ROLE should
>> effectively drop the SESSION_USER's privileges (except that subsequent
>> SET ROLE commands will be checked against the SESSION_USER's role
>> memberships, not the current effective role).

> I don't particularly like deviating from the spec in this regard (since
> I don't think it'd be all that hard to implement what the spec calls
> for), but it doesn't bother me that much.

The problem I have with the spec's way is that it creates a disconnect
between the privilege environment seen at the outer level and the
environment seen within SECURITY DEFINER functions --- unless you want
to allow SET ROLE to have the union behavior within SECURITY DEFINER
functions too, which I don't want to support (and it's not legal per
spec anyway to do SET ROLE inside a function).

> While I agree that this is what Oracle's SET ROLE ALL does initially,
> it's possible for a user to 'SET ROLE <a>' and drop the permissions
> given by the other roles in which the user is in. Will that still be
> possible with your proposed solution, or will doing 'SET ROLE <a>' have
> no effect when 'rolinherit = true'? That's really my main concern.

According to my proposal "SET ROLE x" would drop the user's privileges
and thus be a privilege restriction operation, never a privilege
addition operation, if the user has rolinherit = true. If we don't say
that SET ROLE drops the session user's privileges then indeed SET ROLE
would be a no-op when the session user has rolinherit = true...

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> The problem I have with the spec's way is that it creates a disconnect
> between the privilege environment seen at the outer level and the
> environment seen within SECURITY DEFINER functions --- unless you want
> to allow SET ROLE to have the union behavior within SECURITY DEFINER
> functions too, which I don't want to support (and it's not legal per
> spec anyway to do SET ROLE inside a function).

Essentially the union behavior is what the spec seems to say- except
that only one or the other is valid inside a SECURITY DEFINER, as I
understand it. So, you make everything do the union, but when you go
into a SECURITY DEFINER function you set the one-not-set to NULL and
handle that correctly in the union.

I'm not advocating allowing SET ROLE inside a function, no. Again, this
is more about the spec than an actual use-case that I have for it, so we
can ignore it until someone with a more concrete problem with it comes
along.

> > While I agree that this is what Oracle's SET ROLE ALL does initially,
> > it's possible for a user to 'SET ROLE <a>' and drop the permissions
> > given by the other roles in which the user is in. Will that still be
> > possible with your proposed solution, or will doing 'SET ROLE <a>' have
> > no effect when 'rolinherit = true'? That's really my main concern.
>
> According to my proposal "SET ROLE x" would drop the user's privileges
> and thus be a privilege restriction operation, never a privilege
> addition operation, if the user has rolinherit = true. If we don't say
> that SET ROLE drops the session user's privileges then indeed SET ROLE
> would be a no-op when the session user has rolinherit = true...

Right, I would expect it to drop privileges when rolinherit = true. The
second issue is one reason I don't particularly care for locking it into
the catalog- it means we're building the system in such a way as to be
unable to support what Oracle (at least) does today. If we end up
needing to support it later, or wanting to, perhaps because the spec
follows Oracle's lead and adds SET ROLE ALL, then we've got alot that
would need to be changed because things have become dependent on the
catalog directly.

Otherwise, I think your proposal is fine. :)

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> The problem I have with the spec's way is that it creates a disconnect
>> between the privilege environment seen at the outer level and the
>> environment seen within SECURITY DEFINER functions

> Essentially the union behavior is what the spec seems to say- except
> that only one or the other is valid inside a SECURITY DEFINER, as I
> understand it. So, you make everything do the union, but when you go
> into a SECURITY DEFINER function you set the one-not-set to NULL and
> handle that correctly in the union.

My understanding of things is that per spec, a SECURITY DEFINER function
can be owned by either a user or a role, and so within the function
either CURRENT_USER or CURRENT_ROLE would return the owner and the other
would return NULL. Emulating this would require a hard distinction
between users and roles that is simply not there in our implementation,
which is why I think they should both return the owner.

> Right, I would expect it to drop privileges when rolinherit = true. The
> second issue is one reason I don't particularly care for locking it into
> the catalog- it means we're building the system in such a way as to be
> unable to support what Oracle (at least) does today. If we end up
> needing to support it later, or wanting to, perhaps because the spec
> follows Oracle's lead and adds SET ROLE ALL, then we've got alot that
> would need to be changed because things have become dependent on the
> catalog directly.

To some extent SET ROLE ALL can be emulated by ALTER USER ... INHERIT.
I'm of two minds about whether an unprivileged user should be allowed
to adjust his own rolinherit flag --- in most cases it seems pretty
harmless (and Oracle evidently thinks it is) --- but one could imagine
that the roles have been set up on the assumption that you can't get
more than one role's privileges at a time. INHERIT (or SET ROLE ALL)
would break that assumption, and perhaps allow people to do unwanted
stuff.

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> My understanding of things is that per spec, a SECURITY DEFINER function
> can be owned by either a user or a role, and so within the function
> either CURRENT_USER or CURRENT_ROLE would return the owner and the other
> would return NULL. Emulating this would require a hard distinction
> between users and roles that is simply not there in our implementation,
> which is why I think they should both return the owner.

I would have been more inclined to just pick one and always set it and
leave the other always null. For that, CURRENT_USER would be more
backwards-compatible, but for our implementation I'd tend to think
CURRENT_ROLE is more appropriate. That'd follow the spec closer and
would be closer to what functions written to the spec would expect.

I don't use SECURITY DEFINER functions much though so perhaps others
have a stronger opinion. I've been a bit suprised at the lack of
commentary from other people, perhaps they're just waiting to destroy
whatever we come up with once it's actually been implemented. :)

> To some extent SET ROLE ALL can be emulated by ALTER USER ... INHERIT.

Yeah, but that affects all sessions too, not just a single one, which
makes it quite a different thing.

> I'm of two minds about whether an unprivileged user should be allowed
> to adjust his own rolinherit flag --- in most cases it seems pretty
> harmless (and Oracle evidently thinks it is) --- but one could imagine
> that the roles have been set up on the assumption that you can't get
> more than one role's privileges at a time. INHERIT (or SET ROLE ALL)
> would break that assumption, and perhaps allow people to do unwanted
> stuff.

This is actually what I was thinking about when I was saying at some
point prior in this thread that we should have an option to indicate if
SET ROLE ALL is allowed or not. I don't think that users should be
allowed to adjust their own rolinherit flag. I think the default should
probably be 'true', even for users, but if an admin sets it to false
then I think that should be enforced and users shouldn't be allowed to
change it.

I suspect it's possible to disable 'SET ROLE ALL' in Oracle, and to turn
off having it done upon connection. I'd be somewhat suprised if it
wasn't possible but I havn't really investigated it either way. I don't
know if Oracle has a way to let you do it per-user/per-role though.

Thanks,

Stephen

I've committed changes to add a "rolinherit" flag to pg_authid as per
discussion. The pg_has_role function now distinguishes USAGE rights
on a role (do you currently have the privileges of that role) from
MEMBER rights (do you have the ability to SET ROLE to that role).
A couple of loose ends remain:

* Should is_admin_of_role pay attention to rolinherit? I suspect it
should but can't quite face going through the SQL spec again to be sure.
This only affects the right to GRANT role membership to someone else.

* The information_schema needs another pass to see which pg_has_role
usages ought to be testing USAGE instead of MEMBER. I think most of
them should, but in and around applicable_roles and enabled_roles
some more thought and spec-reading is needed.

I'm planning on doing some documentation work next, and was hoping
someone else would look at the above items.

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> I've committed changes to add a "rolinherit" flag to pg_authid as per
> discussion. The pg_has_role function now distinguishes USAGE rights
> on a role (do you currently have the privileges of that role) from
> MEMBER rights (do you have the ability to SET ROLE to that role).

Great, thanks.

> A couple of loose ends remain:
>
> * Should is_admin_of_role pay attention to rolinherit? I suspect it
> should but can't quite face going through the SQL spec again to be sure.
> This only affects the right to GRANT role membership to someone else.
>
> * The information_schema needs another pass to see which pg_has_role
> usages ought to be testing USAGE instead of MEMBER. I think most of
> them should, but in and around applicable_roles and enabled_roles
> some more thought and spec-reading is needed.

I'll look into what the spec says for these, hopefully anoncvs is
working now...

> I'm planning on doing some documentation work next, and was hoping
> someone else would look at the above items.

Will do. I'll be using the SQL2003 draft. Should be able to run these
down later today.

Thanks,

Stephen