Re: Non-superuser subscription owners

On Fri, Jan 27, 2023 at 5:00 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > Or, another thought, maybe this should be checking for CREATE on the
> > current database + also pg_create_subscription. That seems like it
> > might be the right idea, actually.
>
> Yes, that seems like a good idea.

Done in this version. I also changed check_conninfo to take an extra
argument instead of returning a Boolean, as per your suggestion.

I had a long think about what to do with ALTER SUBSCRIPTION ... OWNER
TO in terms of permissions checks. The previous version required that
the new owner have permissions of pg_create_subscription, but there
seems to be no particular reason for that rule except that it happens
to be what I made the code do. So I changed it to say that the current
owner must have CREATE privilege on the database, and must be able to
SET ROLE to the new owner. This matches the rule for CREATE SCHEMA.
Possibly we should *additionally* require that the person performing
the rename still have pg_create_subscription, but that shouldn't be
the only requirement. This change means that you can't just randomly
give your subscription to the superuser (with or without concurrently
attempting some other change as per your other comments) which is good
because you can't do that with other object types either.

There seems to be a good deal of inconsistency here. If you want to
give someone a schema, YOU need CREATE on the database. But if you
want to give someone a table, THEY need CREATE on the containing
schema. It make sense that we check permissions on the containing
object, which could be a database or a schema depending on what you're
renaming, but it's unclear to me why we sometimes check on the person
performing the ALTER command and at other times on the recipient. It's
also somewhat unclear to me why we are checking CREATE in the first
place, especially on the donor. It might make sense to have a rule
that you can't own an object in a place where you couldn't have
created it, but there is no such rule, because you can give someone
CREATE on a schema, they can create an object, and they you can take
CREATE a way and they still own an object there. So it kind of looks
to me like we made it up as we went along and that the result isn't
very consistent, but I'm inclined to follow CREATE SCHEMA here unless
there's some reason to do otherwise.

Another question around ALTER SUBSCRIPTION ... OWNER TO and also ALTER
SUBSCRIPTION .. RENAME is whether they ought to fail if you're not a
superuser and password_required false is set. They are separate code
paths from the rest of the ALTER SUBSCRIPTION cases, so if we want
that to be a rule we need dedicated code for it. I'm not quite sure
what's right. There's no comparable case for ALTER USER MAPPING
because a user mapping doesn't have an owner and so can't be
reassigned to a new owner. I don't see what the harm is, especially
for RENAME, but I might be missing something, and it certainly seems
arguable.

> > I'm not entirely clear whether there's a hazard there.
>
> I'm not at all either. It's just a code pattern that makes me anxious - I
> suspect there's a few places it makes us more vulnerable.

It looks likely to me that it was cut down from the CREATE SCHEMA code, FWIW.

> > If there is, I think we could fix it by moving the LockSharedObject call up
> > higher, above object_ownercheck. The only problem with that is it lets you
> > lock an object on which you have no permissions: see
> > 2ad36c4e44c8b513f6155656e1b7a8d26715bb94. To really fix that, we'd need an
> > analogue of RangeVarGetRelidExtended.
>
> Yea, we really should have something like RangeVarGetRelidExtended() for other
> kinds of objects. It'd take a fair bit of work / time to use it widely, but
> it'll take even longer if we start in 5 years ;)

We actually have something sort of like that in the form of
get_object_address(). It doesn't allow for a callback, but it does
have a retry loop.

--
Robert Haas
EDB: http://www.enterprisedb.com