Re: AW: Reimplementing permission checks for rules

Zeugswetter Andreas SB <ZeugswetterA(at)wien(dot)spardat(dot)at> writes:
>> What I'm thinking about doing is eliminating the "skipAcl" RTE field
>> and instead adding an Oid field named something like "checkAclAs".
>>
>> Comments? Is this a general enough mechanism, and does it fit well
>> with the various setUID tricks that people are thinking about?

> Sounds good, and a step in the right direction.

After looking at the rule rewriter some more, I realized that the only
way to push all permissions checks to execution time is not only to keep
skipAcl, but to generalize it. The problem is with checks on the view
itself --- if you do INSERT INTO someView, which gets rewritten into
an insert to someRealTable, then what you want the executor to check for
is
Write access on someView by current user
Write access on someRealTable by owner of rule
which is infeasible with the existing code because the executor only
checks for write access on the real target table (someRealTable).

What I have now got, and hope to commit today, is the following fields
in RangeTblEntry, replacing skipAcl:

* checkForRead, checkForWrite, and checkAsUser control run-time access
* permissions checks. A rel will be checked for read or write access
* (or both, or neither) per checkForRead and checkForWrite. If
* checkAsUser is not InvalidOid, then do the permissions checks using
* the access rights of that user, not the current effective user ID.
* (This allows rules to act as setuid gateways.)

bool checkForRead; /* check rel for read access */
bool checkForWrite; /* check rel for write access */
Oid checkAsUser; /* if not zero, check access as this user */

The parser always sets checkAsUser = 0; it sets checkForRead if the rel
is referenced explicitly anywhere in the query; and it sets
checkForWrite on the target table of INSERT/UPDATE/DELETE. It was a
little tricky to get it to do the right thing when the same table is
both a source and target, eg in INSERT ... SELECT, but not too bad.

When a rule is created, the stored parsetree has the rule creator's OID
assigned into each RTE's checkAsUser field, except for the dummy *NEW*
and *OLD* rtable entries.

The rewriter leaves the check fields on a view's RTE as they are, and
just copies the check fields from the rule for the rtable entries it
adds. No rewrite-time permission checks anywhere.

The executor just carries out the indicated checks. execMain's checking
got a *lot* simpler, too, because it doesn't have to carry around a lot
of baggage to determine whether to check a given RTE for read, write,
or both.

Seems like a big win ...

NOTE: there is a subtle change here. A rule used to be taken as
executing setUID to the owner of the table the rule is attached to.
Now it is executed as if setUID to the person who created the rule,
who could be a different user if the table owner gave away RULE rights.
I think this is a more correct behavior, but I'm open to argument.
It'd be easy to make CREATE RULE store the table owner's OID instead,
if anyone wants to argue that that was the right thing.

regards, tom lane