Re: [PATCHES] Users/Groups -> Roles

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > Second, looks like I missed fixing an owner check in pg_proc.c
>
> Got it. I was wondering if there were more --- might be worth checking
> all the superuser() calls.

Yeah, let's come up with a decision about what exactly we should allow
and then perhaps I can go through all of the superuser() calls and see
what needs fixing up.

> > Third, I feel it's incorrect to only allow superuser() to change
> > ownership of objects under a role-based system.
>
> I took that out because it struck me as a likely security hole; we don't
> allow non-superuser users to give away objects now, and we shouldn't
> allow non-superuser roles to do so either. Moreover the tests you had
> were inconsistent (not same test everyplace).

Sorry about them being inconsistent, I didn't intend for them to be.
I went through a couple of iterations of them trying to do the check the
'right' way. Thinking back on it, even the checks I ended up with were
wrong (in the superuser case), though I think they were closer.
Basically my thought was to allow the same thing you could do w/ SET
ROLE, etc:

If you are the owner of the object to be changed (following the normal
owner checking rules) AND would still be considered the owner of the
object *after* the change, then you can change the ownership.

The code I had for this was:

if (!pg_class_ownercheck(tuple,GetUserId()) ||
!is_role_member(newowner,GetUserId()))

That needs a check for superuser though because while the test will pass
on the 'pg_class_ownercheck' side, it won't on the 'is_role_member' side
(currently anyway, I suppose a superuser could be considered to be in
any role, so we could change is_role_member to always return true for
superusers, that'd probably make pg_group look ugly though, either way):

if (!superuser() && !(pg_class_ownercheck(tupe,GetUserId()) &&
is_role_member(newowner,GetUserId())))

I think that's the correct check and can be done the same way for pretty
much all of the objects. Were there other security concerns you had?
I'd be happy to look through the superuser() checks in commands/ and
develop a patch following what I described above, as well as looking for
other cases where we should be using the *_ownercheck() functions.

One place I recall seeing one and not being sure if it should be a new
*_ownercheck() function or not was in the 2PC patch- twophase.c, line
380:

if (user != gxact->owner && !superuser_arg(user))

Wasn't sure if that made sense to have *_ownercheck, or, even if we
added one for it, if it made sense to check is_member_of_role() for
prepared transactions. I don't think SQL has anything to say about it,
anyone know what other DBs do here?

> > Users must be able to
> > create objects owned by a role they're in (as opposed to owned only
> > by themselves).
>
> This is what SET SESSION AUTHORIZATION/SET ROLE is for, no? You set the
> auth to a role you are allowed to be in, then create the object. I do
> notice that we don't have this yet, but it's surely a required piece of
> the puzzle.

(Technically I think SET SESSION AUTHORIZATION is different from SET
ROLE, but anyway)

Right, that's another way to do it (as I mentioned), and that lets you
do ownership changes, but they're much more painful:

CONNECT AS joe;
CREATE TABLE abc as SELECT name,avg(a),sum(b) FROM reallybigtable;
-- Whoops, I meant for abc to be owned by role C so sally can add her
-- column to it later, or vacuum/analyze it, whatever
GRANT SELECT ON abc TO C; -- Might not be necessary
ALTER TABLE abc RENAME TO abc_temp;
SET ROLE C;
CREATE TABLE abc AS SELECT * FROM abc_temp; -- Could be big :(
SET ROLE NONE; -- Might be just 'SET ROLE;'? Gotta check the spec
DROP TABLE abc_temp;

I don't really see the point in making users go through all of these
hoops to do an ownership change. In the end, it's the same result near
as I can tell...

> Yeah, it was. I leave it to Michael Meskes to sync ecpg with the main
> parser; on the occasions where I've tried to do it for him, things
> didn't work out well.

Ah, ok.

> > src/tools/pgindent/pgindent also appears to not have gotten the
> > changes that I made.
>
> That's an automatically generated list; there's no need to edit it.

Hah, silly me.

Thanks,

Stephen