Roles - SET ROLE Updated

* Stephen Frost (sfrost@snowman.net) wrote:

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.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@snowman.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@sss.pgh.pa.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@snowman.net> writes:

* Tom Lane (tgl@sss.pgh.pa.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@sss.pgh.pa.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@snowman.net> writes:

* Tom Lane (tgl@sss.pgh.pa.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@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.net> writes:

* Tom Lane (tgl@sss.pgh.pa.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@sss.pgh.pa.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@sss.pgh.pa.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@snowman.net> writes:

* Tom Lane (tgl@sss.pgh.pa.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@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.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@snowman.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@sss.pgh.pa.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@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.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@snowman.net> writes:

* Tom Lane (tgl@sss.pgh.pa.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