Hi,
On 2023-01-27 16:35:11 -0500, Robert Haas wrote:
> > Maybe a daft question:
> >
> > Have we considered using a "normal grant", e.g. on the database, instead of a
> > role? Could it e.g. be useful to grant a user the permission to create a
> > subscription in one database, but not in another?
>
> Potentially, but I didn't think we'd want to burn through permissions
> bits that fast, even given 7b378237aa805711353075de142021b1d40ff3b0.
> Still, if the consensus is otherwise, I can change it.
I don't really have an opinion on what's better. I looked briefly whether
there was discussion around ithis but I didn't see anything.
pg_create_subcription feels a bit different than most of the other pg_*
roles. For most of those there is no schema object to tie permissions to. But
here there is.
But I think there's good arguments against a GRANT approach, too. GRANT ALL ON
DATABASE would suddenly be dangerous. How does it interact with database
ownership? Etc.
> Then I guess we'd end up with GRANT CREATE ON DATABASE and GRANT CREATE
> SUBSCRIPTION ON DATABASE, which I'm sure wouldn't be confusing at all.
Heh. I guess it could just be GRANT SUBSCRIBE.
> 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.
> > The subscription code already does ownership checks before locking and now
> > there's also the passwordrequired before. Is it possible that this could open
> > up some sort of race? Could e.g. the user change the ownership to the
> > superuser in one session, do an ALTER in the other?
> >
> > It looks like your change won't increase the danger of that, as the
> > superuser() check just checks the current users permissions.
>
> 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.
> 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 ;)
Perhaps the bulk of RangeVarGetRelidExtended() could be generalized by having
a separate name->oid lookup callback?
Greetings,
Andres Freund