Thread: Rollback issue with SET ROLE

The new SET ROLE code has a bit of a problem with rollback of SET
operations.  For example,

regression=# create user foo;
CREATE ROLE
regression=# create role bar;
CREATE ROLE
regression=# set role bar;
SET
regression=> show role;role 
------bar
(1 row)

regression=> begin;
BEGIN
regression=> set session authorization foo;
SET
regression=> show role;role 
------none
(1 row)

regression=> rollback;
ROLLBACK
regression=# show role;role 
------none
(1 row)

Ideally the ROLLBACK should have restored the ROLE setting that obtained
prior to BEGIN.  The reason it doesn't is that the ROLLBACK effectively
does a "SET SESSION AUTHORIZATION <prior-auth-value>", and that naturally
clears the ROLE setting.

I've been chewing on this problem for a couple hours and have come to
the conclusion that it's not going to be possible to fix it without some
changes to the GUC infrastructure --- there just isn't support for
tracking changes to related but separate GUC variables.

Even with a fix for that, there are some related nasty cases.  ConsiderBEGIN;SET LOCAL SESSION AUTHORIZATION foo;SET
ROLEbar;COMMIT;
 
The SET ROLE, being nonlocal, should persist past the COMMIT.  But the
right to do "SET ROLE bar" would have been checked against foo's role
memberships.  If the outer-level session user doesn't have membership
in foo, this leaves us in an illegal situation.

A possible plan B is to forbid doing either SET SESSION AUTHORIZATION
or SET ROLE inside a transaction block, so that none of these cases
arise.  This restriction is sanctioned by the SQL spec.  However, we've
historically allowed SET SESSION AUTHORIZATION inside a transaction
block, and in fact there's a SET LOCAL SESSION AUTHORIZATION in the
regression tests.

Thoughts?
        regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Ideally the ROLLBACK should have restored the ROLE setting that obtained
> prior to BEGIN.  The reason it doesn't is that the ROLLBACK effectively
> does a "SET SESSION AUTHORIZATION <prior-auth-value>", and that naturally
> clears the ROLE setting.

In this case '<prior-auth-value>' is really both the 'user' and the
'role'.  Not sure that really helps though.

> Even with a fix for that, there are some related nasty cases.  Consider
>     BEGIN;
>     SET LOCAL SESSION AUTHORIZATION foo;
>     SET ROLE bar;
>     COMMIT;
> The SET ROLE, being nonlocal, should persist past the COMMIT.  But the
> right to do "SET ROLE bar" would have been checked against foo's role
> memberships.  If the outer-level session user doesn't have membership
> in foo, this leaves us in an illegal situation.

To do SET SESSION AUTH, wouldn't the outer-level session user have to be
superuser, and therefore you're actually fine (considering superuser is
in all roles, etc)?

> A possible plan B is to forbid doing either SET SESSION AUTHORIZATION
> or SET ROLE inside a transaction block, so that none of these cases
> arise.  This restriction is sanctioned by the SQL spec.  However, we've
> historically allowed SET SESSION AUTHORIZATION inside a transaction
> block, and in fact there's a SET LOCAL SESSION AUTHORIZATION in the
> regression tests.

My expectation is that they wouldn't be allowed in a transaction, I
wasn't actually aware that we allowed SET SESSION AUTH in a transaction.
I'm not sure I see what the use-case for it would be, to do SET SESSION
AUTH you have to be superuser, aiui, and that's not something I'd expect
an application to have generally (which is where I might think that
dropping privileges inside a transaction might make sense, maybe...).

I don't see a simple answer really.  GUC variable settings still call C
functions though, so I would have thought it'd be possible to track
prior settings and reset them upon commit through various variables.
Not exactly pretty and perhaps I'm misunderstanding how GUC deals with
resetting back at a commit, but I though it called the function to deal
with that.  Does the function not know if it's being called for
commit or rollback?  Perhaps that's it, sorry I'm not of more help with
this...
Thanks,
    Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> Ideally the ROLLBACK should have restored the ROLE setting that obtained
>> prior to BEGIN.  The reason it doesn't is that the ROLLBACK effectively
>> does a "SET SESSION AUTHORIZATION <prior-auth-value>", and that naturally
>> clears the ROLE setting.

> In this case '<prior-auth-value>' is really both the 'user' and the
> 'role'.  Not sure that really helps though.

Yeah, the solutions I was looking at involved various combinations of
storing both values in one or both of the GUC variables.  They all
seemed pretty messy and fragile though.

>> Even with a fix for that, there are some related nasty cases.  Consider
>> BEGIN;
>> SET LOCAL SESSION AUTHORIZATION foo;
>> SET ROLE bar;
>> COMMIT;
>> The SET ROLE, being nonlocal, should persist past the COMMIT.  But the
>> right to do "SET ROLE bar" would have been checked against foo's role
>> memberships.  If the outer-level session user doesn't have membership
>> in foo, this leaves us in an illegal situation.

> To do SET SESSION AUTH, wouldn't the outer-level session user have to be
> superuser, and therefore you're actually fine (considering superuser is
> in all roles, etc)?

Hmm, true, but that doesn't mean you're out of the woods.  If you fix
the other problem by making AUTH and ROLE be effectively a single
variable, then what will happen here is that SET ROLE will set the
variable's value as foo/bar, and then that value will persist past
COMMIT, leaving you with the wrong AUTH setting at the outer level.

>> A possible plan B is to forbid doing either SET SESSION AUTHORIZATION
>> or SET ROLE inside a transaction block, so that none of these cases
>> arise.  This restriction is sanctioned by the SQL spec.  However, we've
>> historically allowed SET SESSION AUTHORIZATION inside a transaction
>> block, and in fact there's a SET LOCAL SESSION AUTHORIZATION in the
>> regression tests.

> My expectation is that they wouldn't be allowed in a transaction, I
> wasn't actually aware that we allowed SET SESSION AUTH in a transaction.
> I'm not sure I see what the use-case for it would be,

Yeah.  I actually put in code to forbid them in a transaction, but took
it out again when I found the regression tests failing :-(.  The
offending code is in the sequence test:

CREATE USER seq_user;

BEGIN;
SET LOCAL SESSION AUTHORIZATION seq_user;
CREATE SEQUENCE seq3;
SELECT nextval('seq3');
REVOKE ALL ON seq3 FROM seq_user;
SELECT lastval();
ROLLBACK;

DROP USER seq_user;
DROP SEQUENCE seq;

There isn't any amazingly good reason why this couldn't be handled
without a transaction, viz

SET SESSION AUTHORIZATION seq_user;
...
RESET SESSION AUTHORIZATION;

so unless someone comes up with a reasonable implementation plan
for handling changes to both values within a transaction,
I'll probably fall back to doing that.
        regards, tom lane