Обсуждение: Bogus permissions display in 7.4

Deepak Bhole of Red Hat asked me about the following situation:

regression=# create table test (f1 int);
CREATE TABLE
regression=# revoke insert,update,delete,references on test from postgres;
REVOKE
regression=# \z test     Access privileges for database "regression"Schema | Name | Type  |       Access privileges
--------+------+-------+--------------------------------public | test | table | {postgres=*r***R**t*/postgres}
(1 row)

It seems unreasonably hard to interpret what those stars mean, don't you
think?  Certainly you can't tell which star is which without hardwired
knowledge about the order in which the bits will be printed.

The problem here is that we allow the owner to revoke his own ordinary
privileges but not his grant options; so we end up with a permissions
configuration that was not considered in the design of the external
representation for ACL lists.  (Per spec it is not possible to hold a
grant option for a privilege without holding the privilege itself too,
and I expect that this printout format was designed assuming that
restriction.)

I think the printout format is fine and the silent non-removal of grant
options was a bad idea, particularly since it doesn't seem to be saving
any code (GRANT/REVOKE check ownerness anyway).  I propose that we take
out the special cases in merge_acl_with_grant that prohibit revoking an
owner's grant options, and instead adjust the grant statement code to
act as if those options are always present.  Instead of the existing
       if (stmt->is_grant           && !pg_class_ownercheck(relOid, GetUserId())           && pg_class_aclcheck(relOid,
GetUserId(),                               ACL_GRANT_OPTION_FOR(privileges)) != ACLCHECK_OK)
aclcheck_error(ACLCHECK_NO_PRIV,ACL_KIND_CLASS, relvar->relname);
 

it'd be something like
       if (pg_class_ownercheck(relOid, GetUserId())    { okay, assume we have all grant options }else if
(pg_class_aclcheck(relOid,GetUserId(), ...) != ACLCHECK_OK)    { error }else    { determine actual grant options for
non-owner}
 

Thus the effective behavior of grant/revoke would remain the same as
before, but we wouldn't have the contrary-to-spec cases in the visible
contents of the ACL list.

I am in the middle of fixing GRANT/REVOKE to conform to spec as
discussed in the bug #1150 thread, and will make this change too
if I don't hear any objections.
        regards, tom lane

Tom Lane wrote:
> I think the printout format is fine and the silent non-removal of
> grant options was a bad idea, particularly since it doesn't seem to
> be saving any code (GRANT/REVOKE check ownerness anyway).  I propose
> that we take out the special cases in merge_acl_with_grant that
> prohibit revoking an owner's grant options, and instead adjust the
> grant statement code to act as if those options are always present. 

Sounds good.

Peter Eisentraut <peter_e@gmx.net> writes:
> Tom Lane wrote:
>> I think the printout format is fine and the silent non-removal of
>> grant options was a bad idea, particularly since it doesn't seem to
>> be saving any code (GRANT/REVOKE check ownerness anyway).  I propose
>> that we take out the special cases in merge_acl_with_grant that
>> prohibit revoking an owner's grant options, and instead adjust the
>> grant statement code to act as if those options are always present. 

> Sounds good.

If you like that, I have a further suggestion, which is to not include
the owner's grant options in the default ACL, either.  This would not
affect the behavior given the above changes; what it would do is reduce
clutter in the ACL display.  Right now, if user miriam does

create table mytable(f int);
grant select on mytable to public;
\z mytable

she'll see
Schema |  Name   | Type  |            Access privileges
--------+---------+-------+------------------------------------------public | mytable | table |
{miriam=a*r*w*d*R*x*t*/miriam,=r/miriam}

Changing the default ACL would take this down to
public | mytable | table | {miriam=arwdRxt/miriam,=r/miriam}

which seems usefully more readable to me.  Comments?
        regards, tom lane

>  Schema |  Name   | Type  |            Access privileges
> --------+---------+-------+------------------------------------------
>  public | mytable | table | {miriam=a*r*w*d*R*x*t*/miriam,=r/miriam}
> 
> Changing the default ACL would take this down to
> 
>  public | mytable | table | {miriam=arwdRxt/miriam,=r/miriam}
> 
> which seems usefully more readable to me.  Comments?

Guess this means I have to tweak my ACL parser in phpPgAdmin.  If you 
could do something to make that less of a NIGHTMARE, i'd be all ears :P

ie.

1. Make it easy to convert an array to a rowset

2. Fabien's accessor functions? Would they help?

3. At least the quoting has been fixed in 7.4 though

4. Maybe even a function that takes an aclitem and returns....something...

I wrote:
> If you like that, I have a further suggestion, which is to not include
> the owner's grant options in the default ACL, either.

I've been thinking more about this, and realizing that there are more
implications than I first thought.  Specifically, we have to consider
how any hacking we do here will affect recursive_revoke().  The
different options I've suggested would have different side effects,
and I'm having a hard time deciding which is better.

In the existing 7.4 code, you can revoke the owner's privileges but not
his grant options.  This confuses the ACL display code (and possibly
clients that try to interpret ACL displays), and it means that you can't
use recursive_revoke to get rid of everyone but the owner's privileges.

In my proposal of earlier today, you can revoke the owner's grant
options, which will force recursive revocation of everyone else's
privileges (since these all flow ultimately from the owner's grants).
This implies that it is not possible for the owner to have less
privileges than anyone else.  Perhaps that is not bad, but up to now it
was possible to configure a table that way.  Another problem is that
because GRANT still acts as though the owner has grant options, he can
then go and re-grant privs to other people (or the superuser can do it).
Now you have an ACL in which privileges appear to flow from the owner
despite having no grant options.  That will confuse matters --- for
example, if the owner does REVOKE GRANT OPTION FROM himself a second
time, this time it will *not* recursively kill everyone else's
privileges, because recursive_revoke will not see any need to recurse.

If we remove the owner's grant options from the default ACL then
revoking the owner's grant options won't ever recurse (unless he first
grants them to himself explicitly and then revokes them).  Perhaps
that's good?  I'm not sure.

We could patch around some of these problems if recursive_revoke knew
who the owner was (and could thereby take into account the implicit
owner grant options that I still think we want to have).  But it does
not know that, and some of its callers do not either.

Messier and messier.  I'm beginning to see why the SQL spec wants to
introduce a _SYSTEM authid to be the original source of rights.

It could be that the only good solution is to introduce knowledge of the
owner directly into the ACL representation.  You could see the spec's
approach as doing that: he who gets his rights directly from _SYSTEM
is the owner.  Another perhaps more compact way is to make a separate
ACL_IDTYPE to represent "owner" (we are using only 3 of the 4 possible
bitpatterns so this would be easy).

Comments?
        regards, tom lane

> Guess this means I have to tweak my ACL parser in phpPgAdmin.  If you
> could do something to make that less of a NIGHTMARE, i'd be all ears :P
>
> 1. Make it easy to convert an array to a rowset
> 2. Fabien's accessor functions? Would they help?

Sure. I needed these functions to convert acl array into relations
with some plpgsql. Basically it looks like your first item.
I'm happy not to be alone in the world trying to manipulate
acl from user land...

> 3. At least the quoting has been fixed in 7.4 though
> 4. Maybe even a function that takes an aclitem and returns....something...

This would be an accessor;-)

-- 
Fabien Coelho - coelho@cri.ensmp.fr

I wrote:
> I've been thinking more about this, and realizing that there are more
> implications than I first thought.  Specifically, we have to consider
> how any hacking we do here will affect recursive_revoke().

After further consideration I think that at least part of the problem
here is that recursive_revoke is wrong.  Or you could say that our ACL
datastructure is inadequate.  Consider the following situation:

1. A, the owner of some object, grants rights with grant option to  both B and C.

2. B and C independently re-grant those rights with grant option to D.

3. D grants the rights to E.

4. A revokes the rights from B.

recursive_revoke will revoke B's grant to D, while not touching C's
grant to D, which is correct.  However it will then recurse to revoke
D's grant to E.  I claim this is wrong because D still has the rights
via C.

You could imagine extending the ACL datastructure to keep track of where
a grantor's rights came from, but this would be horribly messy, and
I'm not sure it's what we want anyway.  (For instance, we'd really need
to extend GRANT to allow D to say whether he was granting to E on the
basis of B's grant or C's grant.  Yech.)

I think the correct solution involves having recursive_revoke look to
see if D still has the rights from somewhere else before it goes off to
recursively revoke D's grants.  This makes it correctly implement the
notion that one always has the union of the rights granted by anyone.

Now having said that, does it improve the situations I was worried about
last night?  It does, but we really need to fix things so that the ACL
representation explicitly shows the owner.  Here's what I suggest:

1. The first entry of an ACL list should always be a grant of all
relevant grant options for the object type from "the system" (AclId 0,
let's say) to the object owner.  This entry represents the semantics
we've agreed to that an owner has nonrevocable grant options.

2. In a default ACL list there will also be an entry that grants all
regular privileges (not grant options) to the owner, from the owner.
This can be considered to be authorized by the system-granted grant
options.  Because it's set up this way, the owner can revoke his own
regular privileges --- make a table read-only, for instance.

3. It will not matter whether the owner grants himself grant options
(or revokes them later).  They'll be redundant with the system grant
and therefore have no effect on behavior.  Note we need the aforesaid
fix to recursive_revoke to make that true.  Also note that the system
grant *must not* grant regular privileges, or the owner couldn't make
his table read-only to himself.

4. I think that the system ACL entry should be "hidden" and not
displayed by ACL-list printing.  I'm not quite sure yet how to make
that happen.  It would be nicer if the owner ID could be passed to
recursive_revoke out-of-band, instead of being represented inside the
ACL list, but I don't see how to do that for all its callers.

Thoughts?
        regards, tom lane

Basically our whole API of communicating ACL information to the user is
poor.  Look at psql \z:
test=> create table test(x int);CREATE TABLEtest=> grant all on test to public;GRANTtest=> \z test
Accessprivileges for database "test" Schema | Name | Type  |                  Access
privileges--------+------+-------+------------------------------------------------------public | test | table |
{postgres=a*r*w*d*R*x*t*/postgres,=arwdRxt/postgres}(1row)
 

Do we actually expect folks to understand that?  What use is \z really
then?  Sure, maybe we document it in the GRANT manual page, but that
hardly makes it readable on its own.

--  Bruce Momjian                        |  http://candle.pha.pa.us pgman@candle.pha.pa.us               |  (610)
359-1001+  If your life is a hard drive,     |  13 Roberts Road +  Christ can be your backup.        |  Newtown Square,
Pennsylvania19073
 

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Basically our whole API of communicating ACL information to the user is
> poor.  Look at psql \z:

Well, if you don't like that, there's always the spec's way:

regression=> select * from information_schema.table_privileges where table_name = 'mytable';grantor | grantee |
table_catalog| table_schema | table_name | privilege_type | is_grantable | with_hierarchy
 
---------+---------+---------------+--------------+------------+----------------+--------------+----------------foo
|foo     | regression    | public       | mytable    | SELECT         | YES          | NOfoo     | foo     | regression
  | public       | mytable    | DELETE         | YES          | NOfoo     | foo     | regression    | public       |
mytable   | INSERT         | YES          | NOfoo     | foo     | regression    | public       | mytable    | UPDATE
    | YES          | NOfoo     | foo     | regression    | public       | mytable    | REFERENCES     | YES          |
NOfoo    | foo     | regression    | public       | mytable    | RULE           | YES          | NOfoo     | foo     |
regression   | public       | mytable    | TRIGGER        | YES          | NOfoo     | bar     | regression    | public
     | mytable    | SELECT         | NO           | NO
 
(8 rows)
I'm not sure this is really all that much more readable than

regression=> \z mytable         Access privileges for database "regression"Schema |  Name   | Type  |         Access
privileges
--------+---------+-------+------------------------------------public | mytable | table |
{foo=a*r*w*d*R*x*t*/foo,bar=r/foo}
(1 row)
but it's there if you want it.
        regards, tom lane

Dear Tom,

> 4. I think that the system ACL entry should be "hidden" and not
> displayed by ACL-list printing. I'm not quite sure yet how to make
> that happen.  It would be nicer if the owner ID could be passed to
> recursive_revoke out-of-band, instead of being represented inside the
> ACL list, but I don't see how to do that for all its callers.
>
> Thoughts?

(1) It seems to me that part of the consequence of what the suggest   could be that there would be no such thing as
defaultacl implied   by a null entry in an aclitem. If so, this would be a very good thing.   However, this has
implicationson pg initialization.
 

(2) Although I subscribe your first 3 points, I do not like the   4th point. I don't think it is good practice to hide
anything.  That would make the acl display less understandable, as part   for the reality is not shown. It makes any
externaltool   (pgadmin, advisor, whatever else) to have to know this fact   and possibly handle it as a special case.
 

(3) The standard name for the system grantor is "_SYSTEM". User number   0 does not seem a bad idea, but how would it
interactwith number 1?   How often in the source code will they have to be tested?
 

(4) How can/could a super user add or change these system granted   privileges? Or should it be forbidden even to the
su?

(5) Some thought could be given to the implication about future ROLEs.   I'm interested in roles for my teaching, as
theycould allow   a database owner to manage fully the rights of its database wrt   to other users without needing any
superuser privilege.   Good for students;-)
 

-- 
Fabien Coelho - coelho@cri.ensmp.fr

Christopher Kings-Lynne wrote:
> Guess this means I have to tweak my ACL parser in phpPgAdmin.  If you
> could do something to make that less of a NIGHTMARE, i'd be all ears

Check out how the information schema views relating to privileges are 
implemented.  There are some undocumented accessor functions that you 
may find useful.

I wrote:
> I think the correct solution involves having recursive_revoke look to
> see if D still has the rights from somewhere else before it goes off to
> recursively revoke D's grants.  This makes it correctly implement the
> notion that one always has the union of the rights granted by anyone.

While implementing this I noticed a fine point, which is that we need a
defense to keep people from setting up irrevocable circular chains of
rights.  Consider:

1. A, the object owner, grants some privileges with grant options to B.

2. B re-grants the same to C.

3. C re-grants the same to B.

Now A effectively cannot revoke B's privileges, because recursive_revoke
will see that B still holds the grant from C, and not recurse.

B does not even need a partner in crime ... he can just re-grant to
himself with grant option, and he's got irrevocable rights.

Fortunately, it's not too hard for aclinsert to detect an attempt to set
up such a circular chain, and reject it.  What I have the code doing
when an attempt to grant grant options is made is (a) make a copy of the
ACL in which all grant options for the would-be grantee are removed with
DROP_CASCADE, then (b) examine this ACL to verify that the would-be
grantor still has grant options for the proposed rights.  If so, he
didn't derive the rights directly or indirectly from the grantee, and
the grant is not circular.
        regards, tom lane

Fabien COELHO <coelho@cri.ensmp.fr> writes:
>> 4. I think that the system ACL entry should be "hidden" and not
>> displayed by ACL-list printing. I'm not quite sure yet how to make
>> that happen.  It would be nicer if the owner ID could be passed to
>> recursive_revoke out-of-band, instead of being represented inside the
>> ACL list, but I don't see how to do that for all its callers.

> (1) It seems to me that part of the consequence of what the suggest
>     could be that there would be no such thing as default acl implied
>     by a null entry in an aclitem. If so, this would be a very good thing.
>     However, this has implications on pg initialization.

I have no big interest in changing that right now.  It doesn't impact
any of the points at issue.

> (2) Although I subscribe your first 3 points, I do not like the
>     4th point.

I didn't either.  After working on it some more, what I want to do now
is keep the ACL representation the same as it is, but implicitly assume
that the owner has all grant options whether the ACL says so or not.
The "other callers" I was referring to above are the undocumented
aclinsert() and aclremove() functions, which I propose changing to
add an owner's-UID parameter to.  Neither of these are actually used
anywhere, so it might make more sense to just remove 'em.  We'd also
need to change aclcontains(), which is used in the information_schema
views.

With this code base it would not really matter whether default ACLs
include the owner's grant options or not.  I am inclined to the view
that they should not, so as to minimize clutter in the \z display.
One could argue that the owner doesn't really have grant options in the
normal sense anyway, the normal sense being a revocable right, which
these are not.

I have a preliminary patch for this, which I will post later --- it
needs some more work yet.  (I only just realized the need to do
something with aclcontains... at the moment the information_schema
in my patched version is showing that the owner doesn't have grant
options, which is probably not what we want it to say.)
        regards, tom lane

Dear Tom,

> > (2) Although I subscribe your first 3 points, I do not like the 4th point.
>
> I didn't either.  After working on it some more, what I want to do now
> is keep the ACL representation the same as it is, but implicitly assume
> that the owner has all grant options whether the ACL says so or not.

Mmmm... So you still want to stick to "exceptionnal" rights that are
managed somewhere explicitely in the backend code. I would much prefer
something explicit in the acl, because it would make the path to roles
easier, and I'm a little bit interested in this path.

What I was "implicitely" suggesting thru questions in my mail was an
approach where:

(0) all rights are always explicit, null means "no rights".   So the algorithm to check accesses would be:
   - if (it is a super-user) access granted;     else interpret explicitly ACL;
   Otherwise you have everywhere:
   - if (it is a super-user) access granted;     elsif (it is the owner and we just need grant options) access granted;
   else interpret acl explicitly;
 
   As I've pointed out with some bug reports, the current middle section   is quite buggy at the time, and I think I've
foundanother one not yet   reported bugs in this area.
 
   As we're dealing with security, the simpler the better. Having   implicit things just make the code harder to
understandand check   because there is always a special case, and looking at the acl   from userland needs some
interpretationof things that are not there.
 

(1) "grant option" rights are given at creation time explicitly, maybe   with your special user 0. They could then be
revokedby the owner.
 

(2) if they are revoked, they could be given back but only by the   super user, with something like:
   GRANT ALL ON ... TO calvin WITH GRANT OPTIONS FROM GRANTOR _SYSTEM;

It is a point that the super user should be able to create and
manage all rights, including explicit grantors.

So only the superuser is special in this approach, and owners are
only managed especially when creating an object, but not after.

This should make the actual code simpler and more explicit, so I feel that
it would less likely be buggy, and adding roles after that would be more
straightforward.

Well, all this is just my little opinion, and I'm not the one coding.

Have a nice day,

-- 
Fabien Coelho - coelho@cri.ensmp.fr